On 03/03/2025 17:05, Ricardo Ribalda wrote: > On Mon, 3 Mar 2025 at 16:16, Laurent Pinchart > <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: >> >> On Mon, Feb 24, 2025 at 10:26:34AM +0100, Ricardo Ribalda wrote: >>> On Sun, 23 Feb 2025 at 18:03, Laurent Pinchart wrote: >>>> On Sat, Jan 11, 2025 at 09:57:21AM +0000, Ricardo Ribalda wrote: >>>>> To implement VIDIOC_QUERYCTRL, we need to know the minimum, maximum, >>>>> step and flags of the control. For some of the controls, this involves >>>>> querying the actual hardware. >>>>> >>>>> Some non-compliant cameras produce errors when we query them. Right now, >>>>> we populate that error to userspace. When an error happens, the v4l2 >>>>> framework does not copy the v4l2_queryctrl struct to userspace. Also, >>>>> userspace apps are not ready to handle any other error than -EINVAL. >>>>> >>>>> One of the main usecases of VIDIOC_QUERYCTRL is enumerating the controls >>>>> of a device. This is done using the V4L2_CTRL_FLAG_NEXT_CTRL flag. In >>>>> that usecase, a non-compliant control will make it almost impossible to >>>>> enumerate all controls of the device. >>>>> >>>>> A control with an invalid max/min/step/flags is better than non being >>>>> able to enumerate the rest of the controls. >>>>> >>>>> This patch makes VIDIOC_QUERYCTRL return 0 in all the error cases >>>>> different than -EINVAL, introduces a warning in dmesg so we can >>>>> have a trace of what has happened and sets the V4L2_CTRL_FLAG_DISABLED. >>>>> >>>>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> >>>>> --- >>>>> Hi 2*Hans and Laurent! >>>>> >>>>> I came around a device that was listing just a couple of controls when >>>>> it should be listing much more. >>>>> >>>>> Some debugging latter I found that the device was returning -EIO when >>>>> all the focal controls were read. >>>>> >>>>> Lots of good arguments in favor/against this patch in the v1. Please >>>>> check! >>>>> >>>>> Without this patch: >>>>> $ v4l2-ctl --list-ctrls >>>>> auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode) >>>>> exposure_time_absolute 0x009a0902 (int) : min=50 max=10000 step=1 default=166 value=166 flags=inactive >>>>> exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0 >>>>> region_of_interest_auto_control 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1 >>>>> >>>>> With this patch: >>>>> $ v4l2-ctl --list-ctrls >>>>> auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode) >>>>> exposure_time_absolute 0x009a0902 (int) : min=50 max=10000 step=1 default=166 value=166 flags=inactive >>>>> exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0 >>>>> error 5 getting ext_ctrl Focus, Absolute >>>>> error 5 getting ext_ctrl Focus, Automatic Continuous >>>>> region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload >>>>> region_of_interest_auto_control 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1 >>>>> -- >>>>> --- >>>>> Changes in v4: >>>>> - Display control name (Thanks Hans) >>>>> - Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@xxxxxxxxxxxx >>>>> >>>>> Changes in v3: >>>>> - Add a retry mechanism during error. >>>> >>>> This needs to be explained in the commit message, including when/why it >>>> helps, and why the retry count is 2. >>>> >>>>> - Set V4L2_CTRL_FLAG_DISABLED flag. >>>>> - Link to v2: https://lore.kernel.org/r/20241219-uvc-eaccess-v2-1-bf6520c8b86d@xxxxxxxxxxxx >>>>> >>>>> Changes in v2: >>>>> - Never return error, even if we are not enumerating the controls >>>>> - Improve commit message. >>>>> - Link to v1: https://lore.kernel.org/r/20241213-uvc-eaccess-v1-1-62e0b4fcc634@xxxxxxxxxxxx >>>>> --- >>>>> drivers/media/usb/uvc/uvc_ctrl.c | 43 ++++++++++++++++++++++++++++++++-------- >>>>> 1 file changed, 35 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c >>>>> index 4e58476d305e..9d7812e8572d 100644 >>>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c >>>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c >>>>> @@ -1280,6 +1280,8 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl, >>>>> return ~0; >>>>> } >>>>> >>>>> +#define MAX_QUERY_RETRIES 2 >>>>> + >>>>> static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, >>>>> struct uvc_control *ctrl, >>>>> struct uvc_control_mapping *mapping, >>>>> @@ -1305,19 +1307,44 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, >>>>> __uvc_find_control(ctrl->entity, mapping->master_id, >>>>> &master_map, &master_ctrl, 0); >>>>> if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) { >>>>> + unsigned int retries; >>>>> s32 val; >>>>> - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val); >>>>> - if (ret < 0) >>>>> - return ret; >>>>> + int ret; >>>>> >>>>> - if (val != mapping->master_manual) >>>>> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; >>>>> + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) { >>>>> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map, >>>>> + &val); >>>>> + if (ret >= 0) >>>>> + break; >>>>> + } >>>>> + >>>>> + if (ret < 0) { >>>>> + dev_warn_ratelimited(&chain->dev->udev->dev, >>>>> + "UVC non compliance: Error %d querying master control %x (%s)\n", >>>>> + ret, master_map->id, >>>>> + uvc_map_get_name(master_map)); >>>>> + } else if (val != mapping->master_manual) { >>>>> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE; >>>>> + } >>>>> } >>>>> >>>>> if (!ctrl->cached) { >>>>> - int ret = uvc_ctrl_populate_cache(chain, ctrl); >>>>> - if (ret < 0) >>>>> - return ret; >>>>> + unsigned int retries; >>>>> + int ret; >>>>> + >>>>> + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) { >>>>> + ret = uvc_ctrl_populate_cache(chain, ctrl); >>>>> + if (ret >= 0) >>>>> + break; >>>>> + } >>>>> + >>>>> + if (ret < 0) { >>>>> + dev_warn_ratelimited(&chain->dev->udev->dev, >>>>> + "UVC non compliance: Error %d populating cache of control %x (%s)\n", >>>>> + ret, mapping->id, >>>>> + uvc_map_get_name(mapping)); >>>>> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED; >>>> >>>> Can we make the control permanently disabled ? >>> >>> I'd rather not. In funky hardware the control might work with the >>> right combination of other controls. >> >> That makes the behaviour random and therefore very confusing for >> userspace. All of a sudden a control will start being available, even if >> it was marked as disabled during enumeration. > > Random weird hardware will have random behaviour. I think this is kind > of expected. > > Also there are probably lots of cameras in the field that cannot > enumerate properly but are used by custom apps. We are going to break > userspace if we enforce this. > > Hans V. What do you think? I agree with Laurent here. If this sometimes works, and sometimes it doesn't, then what is an application going to do with that? I would start with just permanently disabling this. If it turns out that for some hardware this works with some magic combination of other controls values, then that feels more like a quirk to me. One note, though: >>>>> + dev_warn_ratelimited(&chain->dev->udev->dev, >>>>> + "UVC non compliance: Error %d populating cache of control %x (%s)\n", >>>>> + ret, mapping->id, >>>>> + uvc_map_get_name(mapping)); >>>>> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED; That warning is too cryptic. You want to clearly state in the kernel log that this specific control is disabled due to UVC non compliance. E.g.: "UVC non-compliance: permanently disabling control %x (%s) due to error %d\n", Regards, Hans > >> >>>>> + } >>>>> } >>>>> >>>>> if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) { >>>>> >>>>> --- >>>>> base-commit: c5aa327e10b194884a9c9001a751f6e4703bc3e3 >>>>> change-id: 20241213-uvc-eaccess-755cc061a360 >> >> -- >> Regards, >> >> Laurent Pinchart > > >
