drivers: video: esp32s3: Add set_signal video api function

[epc-ake]

This PR implements the set_signal callback from the video API.
It introduces two signals:

1. A signal registered on VIDEO_EP_OUT raises once a new image has
   been received completely and is ready to be dequeued.
   2. A signal registered on VIDEO_EP_ALL raises with the vsync interrupt

[uLipe]

It seems this PR is related to: #78499

If so the first commit does not belong to this one, could you please remove this commit, from the history and link the PR above to this as a depencency to get merged?

[uLipe]

@epc-ake , I don't know if you are aware of, but you can run the compliance checks locally in your workspace before pushing and catch compliance checks without having to wait long times until the CI executes,

Inside of your zephyr folder, AFTER, commiting just do:

$ ./scripts/ci/check_compliance.py

I hope this help you to save some time making the CI happy :)

[LucasTambor]

should we check data->is_streaming here?

[josuah]

As noted by @uLipe: depends on this PR:

• drivers: video: Add VIDEO_BUF_STARTED to poll signals #78499

[josuah]

I am "requesting changes" but maybe I simply did not understand this hardware...

The Video Endpoints (enum video_endpoint_id) allow to address the individual logical stream or physical port within a same device.

For instance, a video/frame encoder would have 2 ports:

📥 one input stream uncompressed (selected by the wildcard VIDEO_EP_IN or VIDEO_EP_ALL)

📤 one output stream compressed (selected by the wildcard VIDEO_EP_OUT or VIDEO_EP_ALL)

There can also be some form of multiplexing into logical streams if the same device is used for multiple "channels" of some kind:

📥 input channel 1 with its endpoint number 0x01 (selected by passing 0x01 or VIDEO_EP_IN or VIDEO_EP_ALL)
📥 input channel 2 with its endpoint number 0x02 (selected by passing 0x02 or VIDEO_EP_IN or VIDEO_EP_ALL)

📤 output channel 1 with its endpoint number 0x11 (selected by passing 0x11 or VIDEO_EP_OUT or VIDEO_EP_ALL)
📤 output channel 2 with its endpoint number 0x12 (selected by passing 0x12 or VIDEO_EP_OUT or VIDEO_EP_ALL)

These numbers 0x01, 0x02, 0x11, 0x12, are here just examples, vendor might have their own encoding...

There can be separate physical input ports numbered likewise 1, 2, 3, 4... And these endpoints are there to reflect the diversity of what exists on hardware.

Conclusion: Given how I understood this endpoint API, I have the impression that there need to be only one signal: dat->signal, used for both the IRQ or the regular signals

Does that make sense? Did you have some different vision?

[epc-ake]

@epc-ake , I don't know if you are aware of, but you can run the compliance checks locally in your workspace before pushing and catch compliance checks without having to wait long times until the CI executes,

Inside of your zephyr folder, AFTER, commiting just do:

$ ./scripts/ci/check_compliance.py

I hope this help you to save some time making the CI happy :)

Thanks, I'm aware of the tool but I were not able to run it on my system...
It reports fatal: cannot use Perl-compatible regexes when not compiled with USE_LIBPCRE
which seems to come from a call to git grep -I -h --perl-regexp '^\s*(?:menu)?config\s*([A-Z0-9_]+)\s*(?:#|$)
Did not figure out how to resolve this jet...

[epc-ake]

Conclusion: Given how I understood this endpoint API, I have the impression that there need to be only one signal: dat->signal, used for both the IRQ or the regular signals

There are two reasons I defined two signals.

1. In a live stream, VIDEO_BUF_STARTED is raised very close to VIDEO_BUF_DONE (only some us between them). Which in my tests resulted in missing the VIDEO_BUF_DONE within the application if they are raised on the same signal.
2. I need the possibility to poll for the vsync signal ("VIDEO_BUF_STARTED") from within my sensor driver. So I need two signal to prevent multiple threads from pollin on the same one...

I actually don't like the Idea to polling on a signal inside the sensor driver. But with the current API, this is the only way I see to inform the sensor about the vsync interrupt.

[josuah]

So it keeps the video driver signatures, but changes the video driver semantics.
If I understood right, this makes this driver incompatible with the other drivers, who will use enum video_endpoint_id for what it is documented.

Maybe the fix is to modify the API to enable the kind of performance you'd need?

I tried to brainstorm the data path here but did not have time to put it in practice yet.

Glad to allocate time to build (discussions and code) the API you need to keep a same API for all drivers!

[josuah]

I think I understood better the meaning given to VIDEO_EP_OUT and VIDEO_EP_ALL in this PR...

Rather than being wildcards, they are used as signal categories. Which sounds much like a valid interpretation of the same API. In this interpretation, all drivers are still able to "categorize" their signal using these "endpoints categories".

The endpoints used in Video4Zephyr seems like a mapping to devicetree port { endpoint@0 { remote-endpoint = <&phandle>; }; };: addressing which hardware port/stream the buffer should be enqueued to, rather than some form of "tags". This was recently modified to accept actual endpoint numbers too although no driver has multiple ports/streams yet.

[epc-ake]

I tried to make use of this PR in my sensor driver now and I came to the conclusion that it is not optimal to use the set_signal API to inform the sensor driver about events on other drivers...
I had a brief look at v4l2 and they have an API for notifying a subdevice (aka sensor driver)
@josuah I wonder if we could also add e.g. a notify_event() function to the v4z API ?

[josuah]

I suspect Linux introduced subdevices because they had userland-facing devices and "underground" devices living below the kernel, connected through the devicetree. V4L2 devices and subdevices have different APIs.

In Zephyr, there is no subdevices, as for instance, cameras like Arducam Mega will combine both functions of moving the data (device) and controlling an image sensor (subdevice). Related: #73013 (review)

Though, the current model is that for signals and buffers, the application does the glue across devices, Srather than the sensor talking to each other directly, and the API is built for a single k_poll() for all signals. Could this work for this situation? Was it where the performance bottleneck was hit?

https://static.linaro.org/connect/san19/presentations/san19-503.pdf

[josuah]

In practice: I am wondering if regarding this...

I need the possibility to poll for the vsync signal ("VIDEO_BUF_STARTED") from within my sensor driver. So I need two signal to prevent multiple threads from polling on the same one...

...it was possible to move the k_poll() call from the sensor thread back to main() application, where the application would call video_set_ctrl(sensor_dev, ...).

Zephyr driver API calls are lightweight, no syscall overhead involved (unless Zephyr userspace is involved), so this could allow removing one thread? I might lack the full picture to understand the requirement to having a thread in the sensor itself.

[epc-ake]

...it was possible to move the k_poll() call from the sensor thread back to main() application, where the application would call video_set_ctrl(sensor_dev, ...).

Sure, I just hoped I can hide this in the sensor driver since its needed to operate the camera and its not something the application should much care about.

[josuah]

not something the application should much care about

Maybe the difference here is that there is no video subsystem which could cover all this extra piping.
Subsystems help with moving the complexity away from drivers, who already need to deal with complex hardware.
I am very interested in having the video API a bit more pipeline-oriented (one of initial V4Z goal by Loic Poulain), but incremental API changes might be needed for this, and this discussion greatly help with this!

[epc-ake]

I removed the vsync signal generation from this PR again.
Now this PR just completes the current video API.
I will make a new PR draft where I experiment with an additional Video API function to achieve what I need.
Discussions over there are very welcome.

[loicpoulain]

There is no concept of subdevices all capture (e.g DSI) and control (e.g sensor) devices are all video devices. However in practice a capture device has a reference to the sensor device so that it can propagate format, and controls (this should be ideally represented via remote-endpoint in dts).

For the VSYNC stuff, look like what you need at the end is per-buffer sensor configuration, so maybe we lack some possibilities to attach additional metadata/config to the video buffer struct, which would allow the video 'subsystem' or capture device to do the magic (getting the buffer, reading new config, wait for vsync, applying it to sensor...).

[epc-ake]

Can someone tell me how to silence the remaining warning without renaming the argument?
All other drivers and the video API also call this argument signal.

[josuah]

It looks like it is indeed listed as a reserved function, which helps in case mixing sources with the POSIX C API.
https://github.com/zephyrproject-rtos/zephyr/blob/main/scripts/coccinelle/symbols.txt#L177

How about switching to sig instead? The other sources would need to be switched away from signal too.

[josuah]

Some minor change related to error handling.
The rest seems good to go.

[kartben]

Ping @sylvioalves