drivers: video: bugfixes for CID queries #91228

josuah · 2025-06-07T14:55:11Z

This groups two small bugfixes for the controls.

One programming typo on the shell.
One slightly more subtile bug with the way controls are searched across all devices.

Before: the search is in breadth first, and in depth after: some controls are skipped.

After: the search is in depth first, and in breadth after: exhaustive search but controls are not skipped.

(dots = controls, lines = search path)
[EDIT: it should have been dev1, dev2, dev3, dev4 rather than dev1, dev1, dev1, dev1]

ngphibang · 2025-06-07T19:09:46Z

Thank you for the bug fix in the searching strategy of video_query_ctrl() ! LGTM except the commit message should be rewritten a bit. IMO, the use of BFS or DFS terminology in this context is somewhat loose: if we consider the vdev is a node in the tree, then the old code is actually DFS.

drivers: video: shell: switch to breadth to depth search
Fix video_query_ctrl skipping IDs of source devices while searching.
For instance, a top level device with a low-ID and a high-ID would
skip all the mid-IDs bottom-level devices. Fix it by also searching
all the source devices for the next lowest ID across all devices.

Suggestion:

drivers: video: Fix skipping CIDs in video_query_ctrl
In video_query_ctrl(), controls are enumerated in ascending order within each device before moving to the next. This causes it to skip valid controls that exist in other devices but have lower IDs than those already checked. Fix this by searching CIDs globally across all devices of the pipeline.

The other two commits seems not related to the fix could be better put in another PR but not a problem to me.

ngphibang · 2025-06-07T19:15:04Z

BTW, I am curious about the usecase that we have more than one devices having controls in a pipeline that help you to detect the bug ?

In video_query_ctrl(), controls are enumerated in ascending order within each device before moving to the next. This causes it to skip valid controls that exist in other devices but have lower IDs than those already checked. Fix this by searching CIDs globally across all devices of the pipeline. Signed-off-by: Josuah Demangeon <me@josuah.net>

sonarqubecloud · 2025-06-07T19:59:31Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

josuah · 2025-06-07T20:02:40Z

Force-push:

Remove the shell-related controls, moved to #91239
Reword the commit following this suggestion

Much more clear, I could not word it properly so I made a drawing... ^_^'

BTW, I am curious about the usecase that we have more than one devices having controls in a pipeline that help you to detect the bug ?

I was testing the controls for IMX219 on the tinyCLUXN33

uart:~$ video ctrl
  uvc0                 imx219@10            csi@b2000400
  debayer0             stats@b2000000       uvcmanager@b4000000

Very small drivers used just for control APIs, all the I/O is on the hardware.

The UVC implemntation queries the top level device: uvcmanager.

So I wanted to check how the controls were looking like at each level to see if they were properly propagated, and the Zephyr shell use your APIs and that works well:

uart:~$ video ctrl uvcmanager@b4000000
I:                       brightness 0x00980900 (int)    (flags=0x00) : min=0 max=1023 step=1 default=40 value=40
I:                         exposure 0x00980911 (int)    (flags=0x00) : min=0 max=65535 step=1 default=1000 value=1000
I:                             gain 0x00980913 (int)    (flags=0x00) : min=0 max=4095 step=1 default=256 value=256
I:                    analogue_gain 0x009e0903 (int)    (flags=0x00) : min=0 max=255 step=1 default=0 value=0
I:                     test_pattern 0x009f0903 (menu)   (flags=0x00) : min=0 max=1 step=1 default=0 value=0
I:                                  0: None
I:                                  1: Incrementing Number
uart:~$ video ctrl imx219@10
I:                       brightness 0x00980900 (int)    (flags=0x00) : min=0 max=1023 step=1 default=40 value=40
I:                         exposure 0x00980911 (int)    (flags=0x00) : min=0 max=65535 step=1 default=1000 value=1000
I:                             gain 0x00980913 (int)    (flags=0x00) : min=0 max=4095 step=1 default=256 value=256
I:                    analogue_gain 0x009e0903 (int)    (flags=0x00) : min=0 max=255 step=1 default=0 value=0
I:                     test_pattern 0x009f0903 (menu)   (flags=0x00) : min=0 max=9 step=1 default=0 value=3
I:                                  0: Off
I:                                  1: Solid color
I:                                  2: 100% color bars
I:                                  3: Fade to grey color bar
I:                                  4: PN9
I:                                  5: 16 split color bar
I:                                  6: 16 split inverted color bar
I:                                  7: Column counter
I:                                  8: Inverted column counter
I:                                  9: PN31
uart:~$

Here we see the different test patterns depending on where we query in the chain, as expected... Handy!

ngphibang · 2025-06-08T18:24:41Z

If I understand well, your uvcmanager and imx219 both expose the test_pattern control. Although I don't know why the uvcmanager has a test pattern control on its own but this scenario is rather common in practice, i.e. the bridge driver and the sensor driver both have a test pattern module.

So, in the scenario where parent and child devices expose the same controls, I think we need to show all the controls of each device when we query the parent device.

I made another version as below where we show all the controls of a device before moving to the next device in the hierachy (same as the old code) but fixing the skipped CIDs by resetting the cq->id and keeping track of the vdev (by memorizing a vdev in the query structure).

This scenario also reveals the question "How do we set/get a control when it presents in multiple hierachical devices ?"
I think we need to expose all the video devices in the tree to user so that this can be done.

I made a draft version to illustrate this here : #91265

What do you think ?

int video_query_ctrl(const struct device *dev, struct video_ctrl_query *cq)
 {
        int ret;
-       struct video_device *vdev;
        struct video_ctrl *ctrl = NULL;
 
        __ASSERT_NO_MSG(dev != NULL);
        __ASSERT_NO_MSG(cq != NULL);
 
        if (cq->id & VIDEO_CTRL_FLAG_NEXT_CTRL) {
-               vdev = video_find_vdev(dev);
                cq->id &= ~VIDEO_CTRL_FLAG_NEXT_CTRL;
-               while (vdev) {
-                       SYS_DLIST_FOR_EACH_CONTAINER(&vdev->ctrls, ctrl, node) {
-               vdev = video_find_vdev(dev);
                cq->id &= ~VIDEO_CTRL_FLAG_NEXT_CTRL;
-               while (vdev) {
-                       SYS_DLIST_FOR_EACH_CONTAINER(&vdev->ctrls, ctrl, node) {
+               if (cq->vdev == NULL) {
+                       cq->vdev = video_find_vdev(dev);
+               }
+
+               while (cq->vdev != NULL) {
+                       SYS_DLIST_FOR_EACH_CONTAINER(&cq->vdev->ctrls, ctrl, node) {
                                if (ctrl->id > cq->id) {
                                        goto fill_query;
                                }
                        }
-                       vdev = video_find_vdev(vdev->src_dev);
+                       cq->vdev = video_find_vdev(cq->vdev->src_dev);
+                       cq->id = 0;
                }
                return -ENOTSUP;
        }

josuah · 2025-06-08T19:51:24Z

I don't know why the uvcmanager has a test pattern control on its own

This is to have an image as low level as possible in the chain. Having a basic increasing counter pattern generator straight in the UVC handler makes the this difficult part a lot less complex.

in the scenario where parent and child devices expose the same controls

IIRC your original idea was that this would be a rare situation and the test pattern is seems like a non-problem as typically an user wanting to debug a system wants to enable a test pattern for a particular device, like down in the shell today.

How do we set/get a control when it presents in multiple hierarchical devices

It seems still possible to do by pointing at the lower-level device, so is still possible to do today: video_set_ctrl(top_level_dev, &ctrl); or video_set_ctrl(low_level_dev, &ctrl);:

In some cases, this will be the same control, if the control is only present on the low-level dev.
In some cases, thsi will be a different control, if the top-level control was occluding the low-level control.

Is this good enough?

we need to expose all the video devices in the tree to user so that this can be done

I will continue on #91265 to avoid going off-topic.

Thanks for the implementation!

ngphibang · 2025-06-08T20:25:28Z

IIRC your original idea was that this would be a rare situation and the test pattern is seems like a non-problem as typically an user wanting to debug a system wants to enable a test pattern for a particular device, like down in the shell today.

Yes, that's right. Apart from the test pattern, I don't see other controls that could be present in both bridge and sensor drivers. When enumerating the controls on the parent device, it currently enumerate all the controls of the parent and its children. So if a control is present on both, we should show it two times for consistency I think ?

josuah · 2025-06-08T21:48:04Z

if a control is present on both, we should show it two times for consistency I think

One interesting use-case: A video device with two image sensor as source, stacked vertically (video output twice taller).

https://github.com/tinyvision-ai-inc/tinyvision_zephyr_sdk/blob/5b7faecfa9c0ba9a465a79e80aa2909efc59fa5d/drivers/video/tvai_stacker.c#L274-L275

This means two VIDEO_DEVICE_DEFINE()! The way I thought of handling controls with the current APIs is to expose a control in the "stacker" driver, which would re-send the control queries to both sources.

This could be interesting to aggregate all controls... It can be confusing, though, that a video_query_ctrl() called on a device returns controls on unrelated devices...

Maybe this would be an easier model to understand once a high-level structure like "video pipeline" or "video interface" is introduced though. Then if "query controls of that interface" returns all controls of all devices of the pipeline behind that interface... it stays very logical. In preparation of that, it makes sense to prepare the implementation, which also fixes a bug.

josuah · 2025-06-08T21:51:13Z

Adding DNM flag to give time for discussion in case an alternative fix is chosen.
Should this bugfix be merged first to un-block the situation? I will remove the DNM if so.

josuah · 2025-06-08T21:52:59Z

Comment imported from #91265 (comment)

This sounds like a very good idea! However, I would consider switching VIDEO_DEVICE_DEFINE() to Z_DEVICE_DEFINE that I just discovered by searching now:

zephyr/include/zephyr/device.h

Lines 1270 to 1293 in 98ba754

    
           /** 
        
            * @brief Define a @ref device and all other required objects. 
        
            * 
        
            * This is the common macro used to define @ref device objects. It can be used 
        
            * to define both Devicetree and software devices. 
        
            * 
        
            * @param node_id Devicetree node id for the device (DT_INVALID_NODE if a 
        
            * software device). 
        
            * @param dev_id Device identifier (used to name the defined @ref device). 
        
            * @param name Name of the device. 
        
            * @param init_fn Device init function. 
        
            * @param flags Device flags. 
        
            * @param pm Reference to @ref pm_device_base associated with the device. 
        
            * (optional). 
        
            * @param data Reference to device data. 
        
            * @param config Reference to device config. 
        
            * @param level Initialization level. 
        
            * @param prio Initialization priority. 
        
            * @param api Reference to device API. 
        
            * @param state Reference to device state. 
        
            * @param ... Optional dependencies, manually specified. 
        
            */ 
        
           #define Z_DEVICE_DEFINE(node_id, dev_id, name, init_fn, deinit_fn, flags, pm,   \ 
        
           			data, config, level, prio, api, state, ...)             \

This permits to manually define a device and its dependencies with the ... argument for Optional dependencies, manually specified.

And then we can walk the tree of dependencies, including only the video devices (those manually "injected") with:

zephyr/include/zephyr/device.h

Lines 658 to 677 in 98ba754

    
           /** 
        
            * @brief Get the device handles for injected dependencies of this device. 
        
            * 
        
            * This function returns a pointer to an array of device handles. The length of 
        
            * the array is stored in the @p count parameter. 
        
            * 
        
            * The array contains a handle for each device that @p dev manually injected as 
        
            * a dependency, via providing extra arguments to Z_DEVICE_DEFINE. This does not 
        
            * include transitive dependencies; you must recursively determine those. 
        
            * 
        
            * @param dev the device for which injected dependencies are desired. 
        
            * @param count pointer to where this function should store the length of the 
        
            * returned array. No value is stored if the call returns a null pointer. The 
        
            * value may be set to zero if the device has no devicetree dependencies. 
        
            * 
        
            * @return a pointer to a sequence of @p *count device handles, or a null 
        
            * pointer if @p dev does not have any dependency data. 
        
            */ 
        
           static inline const device_handle_t * 
        
           device_injected_handles_get(const struct device *dev, size_t *count)

That way, it becomes possible to use native Zephyr APIs instead of video_print_dev_tree(), which sounds a lot like re-implementing devicetrees runtime lookups.

I can propose an implementation as soon as I am done with IMX219 (#88011) but feel free to also modify the current PR if you do not want to wait.

A shell command to print the tree of video devices would be some nice feature to encourage users to buy-in the complexity of the video APIs inherited from Linux.

ngphibang · 2025-06-08T22:00:10Z

One interesting use-case: A video device with two image sensor as source, stacked vertically (video output twice taller) ...

Yes, this is another usecase which is different than the one discussed here. This falls into the support of multiple-sensors at a time or a sensor at a time but can be switched or virtual channel categories.
Depending on the usecase, for example if 2 sensors at a time, we usually have 2 pipelines:
DMA1 -> bridge -> sensor1
DMA2 -> bridge -> sensor2
...

Here, the usecase is simpler where we have the bridge driver containing the same control with the sensor ...

This could be interesting to aggregate all controls...

My first implementation of the control framework is aggregating the controls as done in Linux but than I realized that it's more complicated and may require subdev, main dev and video dev concepts ...

ngphibang · 2025-06-13T23:25:26Z

This sounds like a very good idea! However, I would consider switching VIDEO_DEVICE_DEFINE() to Z_DEVICE_DEFINE that I just discovered by searching now:

Z_DEVICE_DEFINE is a low level macro and the back-end of DEVICE_DT_DEFINE so if you want to inject dependencies, you can still add them in the var_args of DEVICE_DT_DEFINE I think.

BTW, injecting the dependent devices to DEVICE_DT_DEFINE or Z_DEVICE_DEFINE is just like anotherway of setting the source_dev in the video_device structure. It can replace this action but cannot remove the use of VIDEO_DEVICE_DEFINE macro I think. This macro stores the video device in a registry (iterable section) so that we can get it (find video device) globally, outside of drivers.

Another point of injecting devices dependency is that it imposes constraint on the init order of the device and its dependencies what we want to avoid, e.g. a bridge driver (like CSI in rt1064) "depend on" the sensor dev but needs to be initialized before the sensor dev to provide the master clock to the sensor.

ngphibang · 2025-06-13T23:36:18Z

To focus only on fixing the skipped CIDS of the parent devices of video_query_ctrl(), when the same CID presents on the source and the bridge devices, the current implementation of this PR only shows the control of the bridge device and ignore the one of the source device while in #91265, it shows all controls of each device (a drawback is I had to print the device name, which can be long and the line can be broken, so not beautiful).

What do you think ?

josuah · 2025-06-14T10:14:07Z

Z_DEVICE_DEFINE is a low level macro and the back-end of DEVICE_DT_DEFINE

The __VA_ARGS__ is passed through the whole chain, so DEVICE_DT_DEFINE() this would allow injecting dependencies, yes you are right.

This macro stores the video device in a registry (iterable section)

There are also iterable sections for all devices, and is possible to test whether a device is having a video API or not. In theory it would be possible to use Zephyr native APIs, but that sounds a lot less straightforward to do so, agreed.

injecting devices dependency is that it imposes constraint on the init order of the device

Clearly a blocking point, injected dependencies cannot work for this reason.
This is a control chain that follows the flow of the data, not a dependency tree.

Thank you for describing the alternatives, this helps identifying the real need.

josuah · 2025-06-14T10:29:44Z

To focus only on fixing the skipped CIDS [...] in #91265, it shows all controls of each device

It seems like the best way to go. Maybe in the future users do not have to deal with the struct devices *dev directly unless dealing with the internals... Should a separate thread be opened to talk about turning the video libraries into a subsystem?

A drawback is I had to print the device name, which can be long and the line can be broken, so not beautiful

Since the controls are listed device by device, an alternative output could be:

- Supported controls for mipi_csi2rx@40810000:

           Test Pattern 0x009f0903 (menu)   (flags=0x00) : min=0 max=1 step=1 default=0 value=0
                          0: Disabled
                          1: 8-bars

- Supported controls for ov5640@3c:

             Brightness 0x00980900 (int)    (flags=0x00) : min=-15 max=15 step=1 default=0 value=0
               Contrast 0x00980901 (int)    (flags=0x00) : min=0 max=255 step=1 default=0 value=0
             Saturation 0x00980902 (int)    (flags=0x00) : min=0 max=255 step=1 default=64 value=64
                    Hue 0x00980903 (int)    (flags=0x00) : min=0 max=359 step=1 default=0 value=0
        Gain, Automatic 0x00980912 (bool)   (flags=0x10) : min=0 max=1 step=1 default=1 value=1
        Horizontal Flip 0x00980914 (bool)   (flags=0x00) : min=0 max=1 step=1 default=0 value=0
          Vertical Flip 0x00980915 (bool)   (flags=0x00) : min=0 max=1 step=1 default=0 value=0
   Power Line Frequency 0x00980918 (menu)   (flags=0x00) : min=0 max=3 step=1 default=1 value=1
                          0: Disabled
                          1: 50 Hz
                          2: 60 Hz
                          3: Auto
          Analogue Gain 0x009e0903 (int)    (flags=0x0c) : min=0 max=1023 step=1 default=0 value=16
             Pixel Rate 0x009f0902 (int64)  (flags=0x01) : min=24000000 max=96000000 step=1 default=48000000 value=48000000
           Test Pattern 0x009f0903 (menu)   (flags=0x00) : min=0 max=4 step=1 default=0 value=0
                          0: Disabled
                          1: Color bars
                          2: Color bars with rolling bar
                          3: Color squares
                          4: Color squares with rolling bar

ngphibang · 2025-06-17T08:37:54Z

Since the controls are listed device by device, an alternative output could be:

- Supported controls for mipi_csi2rx@40810000:

           Test Pattern 0x009f0903 (menu)   (flags=0x00) : min=0 max=1 step=1 default=0 value=0
                          0: Disabled
                          1: 8-bars

- Supported controls for ov5640@3c:

             Brightness 0x00980900 (int)    (flags=0x00) : min=-15 max=15 step=1 default=0 value=0

Yes, this is what I wanted to do but it's not easy at first quick implementation :-) (to know when we finish with a device, maybe need to use a temporary variable to compare the device's name ...). I will retry later.

ngphibang · 2025-06-18T08:04:00Z

Reworked #91265 so that video_ctrl_query keeps a device structure instead of a video_device structure. This way we do not need to expose video_device to public.
(I added your sign-off-by as a co-author of the commit)

josuah · 2025-06-23T20:19:49Z

This was superseded by #91265 which is merged. All good!

josuah marked this pull request as ready for review June 7, 2025 16:36

github-actions bot added the area: Video label Jun 7, 2025

github-actions bot requested review from avolmat-st, loicpoulain and ngphibang June 7, 2025 16:37

github-actions bot assigned ngphibang Jun 7, 2025

josuah force-pushed the pr-video-ctrl-fix branch from e7aeab7 to e643503 Compare June 7, 2025 19:52

josuah changed the title ~~drivers: video: bugfixes to get the controls to work on the shell~~ drivers: video: bugfixes for CID queries Jun 7, 2025

josuah mentioned this pull request Jun 7, 2025

drivers: video: shell: fix the name comparison #91239

Draft

ngphibang mentioned this pull request Jun 8, 2025

Fix video_query_ctrl skipped CIDs issue #91265

Merged

josuah added the DNM label Jun 8, 2025

josuah closed this Jun 23, 2025

drivers: video: bugfixes for CID queries #91228

drivers: video: bugfixes for CID queries #91228

Uh oh!

Conversation

josuah commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngphibang commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngphibang commented Jun 7, 2025

Uh oh!

sonarqubecloud bot commented Jun 7, 2025

Quality Gate passed

Uh oh!

josuah commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngphibang commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josuah commented Jun 8, 2025

Uh oh!

ngphibang commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josuah commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josuah commented Jun 8, 2025

Uh oh!

josuah commented Jun 8, 2025

Uh oh!

ngphibang commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngphibang commented Jun 13, 2025

Uh oh!

ngphibang commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josuah commented Jun 14, 2025

Uh oh!

josuah commented Jun 14, 2025

Uh oh!

ngphibang commented Jun 17, 2025

Uh oh!

ngphibang commented Jun 18, 2025

Uh oh!

josuah commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

josuah commented Jun 7, 2025 •

edited

Loading

ngphibang commented Jun 7, 2025 •

edited

Loading

josuah commented Jun 7, 2025 •

edited

Loading

ngphibang commented Jun 8, 2025 •

edited

Loading

ngphibang commented Jun 8, 2025 •

edited

Loading

josuah commented Jun 8, 2025 •

edited

Loading

ngphibang commented Jun 8, 2025 •

edited

Loading

ngphibang commented Jun 13, 2025 •

edited

Loading

josuah commented Jun 23, 2025 •

edited

Loading