Skip to content

driver: video: esp32: add video_flush() callback #86915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 13, 2025
Merged

driver: video: esp32: add video_flush() callback #86915

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion drivers/video/video_esp32_dvp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,36 @@ static int video_esp32_get_ctrl(const struct device *dev, unsigned int cid, void
return video_get_ctrl(cfg->source_dev, cid, value);
}

static int video_esp32_flush(const struct device *dev, enum video_endpoint_id ep, bool cancel)
{
struct video_esp32_data *data = dev->data;
struct video_buffer *vbuf = NULL;

if (cancel) {
if (data->is_streaming) {
video_esp32_set_stream(dev, false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to comment a bit late when this is just merged but when video_esp32_flush() is called (by video_stream_stop()), the video_esp32_set_stream() is already called. This is the same error as in video_emul driver which I fixed here.

static inline int video_stream_stop(const struct device *dev)
{
	const struct video_driver_api *api = (const struct video_driver_api *)dev->api;
	int ret;

	if (api->set_stream == NULL) {
		return -ENOSYS;
	}

	ret = api->set_stream(dev, false);
	video_flush(dev, VIDEO_EP_ALL, true);

	return ret;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the driver's flush() does not perform the needed "stop" actions, then the user should always call video_stream_stop() before video_flush()... But as video_stream_stop() already calls video_flush(), that suggests that video_flush() should be deprecated in the long run?

There remain the need to handle bool cancel... In that case, set_stream() would need to be a more generic operation maybe... Like set_stream(START), set_stream(STOP), set_stream(CANCEL) etc.

Maybe this comes as part of the video control framework PR, in which case I'll just wait patiently. :)

Copy link
Collaborator

@ngphibang ngphibang Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the driver's flush() does not perform the needed "stop" actions, then the user should always call video_stream_stop() before video_flush()...

No, here we talk only the case cancel = true that means the flush callback is surely called by video_stream_stop() and in video_stream_stop(), set_stream(false) is already called before calling the flush, so no need to call it again.

Copy link
Collaborator

@ngphibang ngphibang Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a broader scope of discussion about video_flush(), there's my long comment here.

Firstly, I think we need to agree on what to do to "flush" when stopping stream. It then depends on what we agree on what to do to restart. As widely adopted in Linux as a general rule (in my quoted comment), restart is like a new start, user needs to (re-)enqueue buffers. So, when flushing, both fifo_in and fifo_out need to be emptied / cleaned so that everything can be restarted again.

However, current implementation of flush callback in some Zephyr drivers like esp32, csi, sw_generator is just taking all the buffers in the fifo_in and put them to fifo_out. It needs to be rethinked.

Another difficulty is low level drivers (which Zephyr drivers are based on) do it differently, for example, the CSI low level driver puts all the buffers in fifo_out into the fifo_in (contracdictory with what Zephyr driver does) as it expected to be able to restart right away without re-enqueuing buffer (by user).

And Linux does not expose flushing API to driver but does it implicitly in the v4l2 framework ...

So it's a bit complicated. I will try to address this because it is also related to removing the video_endpoint_id in flushing API.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I don't see any usecase / need to force a flush when stream is on-going (cancel = false) ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will pursue on this PR if that makes sense...
#87070 (comment)

Thank you for your in-depth answer!

}
if (data->active_vbuf) {
k_fifo_put(&data->fifo_out, data->active_vbuf);
data->active_vbuf = NULL;
}
while ((vbuf = k_fifo_get(&data->fifo_in, K_NO_WAIT)) != NULL) {
k_fifo_put(&data->fifo_out, vbuf);
#ifdef CONFIG_POLL
if (data->signal_out) {
k_poll_signal_raise(data->signal_out, VIDEO_BUF_ABORTED);
}
#endif
}
} else {
while (!k_fifo_is_empty(&data->fifo_in)) {
k_sleep(K_MSEC(1));
}
}

return 0;
}

#ifdef CONFIG_POLL
int video_esp32_set_signal(const struct device *dev, enum video_endpoint_id ep,
struct k_poll_signal *sig)
Expand Down Expand Up @@ -401,7 +431,7 @@ static DEVICE_API(video, esp32_driver_api) = {
/* optional callbacks */
.enqueue = video_esp32_enqueue,
.dequeue = video_esp32_dequeue,
.flush = NULL,
.flush = video_esp32_flush,
.set_ctrl = video_esp32_set_ctrl,
.get_ctrl = video_esp32_get_ctrl,
#ifdef CONFIG_POLL
Expand Down
Loading