Re: [PATCH v2 2/6] media: uvcvideo: Import standard controls from uvcdynctrl

[Hans de Goede <hansg@kernel.org>]

Hi,

On 9-Dec-25 7:28 AM, Ricardo Ribalda wrote:
> Hi Hans
> 
> 
> On Mon, 8 Dec 2025 at 20:12, Hans de Goede <hansg@kernel.org> wrote:
>>
>> Hi,
>>
>> On 8-Dec-25 12:02 PM, Hans de Goede wrote:
>>> Hi Ricardo,
>>>
>>> Thank you very much for doing this, this has been on my own TODO list for
>>> a long time, so it is great to finally see this happen.
>>>
>>> On 19-Nov-25 8:37 PM, Ricardo Ribalda wrote:
>>>> The uvcdynctrl tool from libwebcam:
>>>> https://sourceforge.net/projects/libwebcam/
>>>> maps proprietary controls into v4l2 controls using the UVCIOC_CTRL_MAP
>>>> ioctl.
>>>>
>>>> The tool has not been updated for 10+ years now, and there is no reason
>>>> for the UVC driver to not do the mapping by itself.
>>>>
>>>> This patch adds the mappings from the uvcdynctrl into the driver. Hopefully
>>>> this effort can help in deprecating the UVCIOC_CTRL_MAP ioctl.
>>>
>>> ...
>>>
>>> Question what happens if uvcdynctrl is run after applying this patch ?
>>
>> Answering my own question here, we will hit:
>>
>> drivers/media/usb/uvc/uvc_ctrl.c: 3166:
>>
>>         list_for_each_entry(map, &ctrl->info.mappings, list) {
>>                 if (mapping->id == map->id) {
>>                         uvc_dbg(dev, CONTROL,
>>                                 "Can't add mapping '%s', control id 0x%08x already exists\n",
>>                                 uvc_map_get_name(mapping), mapping->id);
>>                         ret = -EEXIST;
>>                         goto done;
>>                 }
>>         }
>>
>> So uvcdynctrl will see an EEXIST error. I think we need to add an -EEXIST check
>> to uvc_ctrl_add_mapping() )or uvc_ioctl_xu_ctrl_map() which is the only caller of
>> uvc_ctrl_add_mapping()) and if -EEXIST is returned do a uvc_warn_once() that duplicate
>> mappings are being ignored and return 0 instead of -EEXIST to avoid breaking existing
> 
> uvcdynctrl seems to handle this kind of error ok:
> 
> https://sourceforge.net/p/libwebcam/code/ci/master/tree/libwebcam/dynctrl.c#l1215
> 
> while(node_mapping) {
>   CResult ret = process_mapping(node_mapping, ctx);
>   if(ctx->info) {
>     if(ret)
>       ctx->info->stats.mappings.successful++;
>     else
>       ctx->info->stats.mappings.failed++;
>   }
>   node_mapping = xml_get_next_sibling_by_name(node_mapping, "mapping");
> }
> 
> https://sourceforge.net/p/libwebcam/code/ci/master/tree/libwebcam/dynctrl.c#l1199
> if(v4l2_ret != 0
> #ifdef DYNCTRL_IGNORE_EEXIST_AFTER_PASS1
> && (ctx->pass == 1 || errno != EEXIST)
> #endif
> )
> 
> https://sourceforge.net/p/libwebcam/code/ci/master/tree/libwebcam/libwebcam.h#l69
> /// Ignore EEXIST errors for the UVCIOC_CTRL_ADD and UVCIOC_CTRL_MAP ioctls for
> /// all but the first device. This is required if the driver uses
> global controls
> /// instead of per-device controls.
> #define DYNCTRL_IGNORE_EEXIST_AFTER_PASS1
> 
> 
> (BTW, I think the last comment ^^^ is wrong, it should be. Ignore
> EEXIST errors, or errors for second passes.
> But I might need a coffee :P)
> 
> 
> Looking at the debian codesearch:
> https://codesearch.debian.net/search?q=UVCIOC_CTRL_MAP+-path%3Aioctl.rs+-path%3Auvc_v4l2.c+-file%3Auvcvideo.rst+-file%3Auvc_ctrl.c+-file%3Auvcvideo.h&literal=1
> 
> the only occurrence that I am no sure if it will properly handle -EEXIST is:
> https://sources.debian.org/src/chromium/143.0.7499.40-1/chrome/browser/ash/chromebox_for_meetings/xu_camera/xu_camera_service.cc?hl=400#L400
> But that is ash-> ChromeOS browser. I can ask the code owner to fix it
> if needed.
> 
> 
> I'd rather not add the quirk that you are proposing if possible. I
> would expect that any/all the userspace handles -EEXIST because the
> uvc control state outlives the userspace.
> 
> 
> Let me know what you think.

Based on what you wrote above I think that going with -EEXIST is ok for
now, we can always revisit if we get complaints about this.

Regards,

Hans