| Message ID | 20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] media: uvcvideo: Set V4L2_CTRL_FLAG_DISABLED during queryctrl errors | expand |
On 07/01/2025 16:23, 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@chromium.org> > --- > 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 v3: > - Add a retry mechanism during error. > - Set V4L2_CTRL_FLAG_DISABLED flag. > - Link to v2: https://lore.kernel.org/r/20241219-uvc-eaccess-v2-1-bf6520c8b86d@chromium.org > > 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@chromium.org > --- > drivers/media/usb/uvc/uvc_ctrl.c | 41 ++++++++++++++++++++++++++++++++-------- > 1 file changed, 33 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 4e58476d305e..d69b9efa74d0 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,42 @@ 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\n", > + ret, master_map->id); > + } 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\n", > + ret, mapping->id); Is ctrl->name available here? It's a pain to translate the control id to a specific control, if you log the name, then you immediately know which control it is. Ideally both the id and name are logged. Regards, Hans > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED; > + } > } > > if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) { > > --- > base-commit: c5aa327e10b194884a9c9001a751f6e4703bc3e3 > change-id: 20241213-uvc-eaccess-755cc061a360 > > Best regards,
Hi Hans On Tue, 7 Jan 2025 at 16:32, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On 07/01/2025 16:23, 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@chromium.org> > > --- > > 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 v3: > > - Add a retry mechanism during error. > > - Set V4L2_CTRL_FLAG_DISABLED flag. > > - Link to v2: https://lore.kernel.org/r/20241219-uvc-eaccess-v2-1-bf6520c8b86d@chromium.org > > > > 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@chromium.org > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 41 ++++++++++++++++++++++++++++++++-------- > > 1 file changed, 33 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index 4e58476d305e..d69b9efa74d0 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,42 @@ 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\n", > > + ret, master_map->id); > > + } 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\n", > > + ret, mapping->id); > > Is ctrl->name available here? It's a pain to translate the > control id to a specific control, if you log the name, then you > immediately know which control it is. Ideally both the id and name > are logged. Good point. I added this locally. Will resend after I get more feedback on the patch. diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index d69b9efa74d0..9d7812e8572d 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1320,8 +1320,9 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, if (ret < 0) { dev_warn_ratelimited(&chain->dev->udev->dev, - "UVC non compliance: Error %d querying master control %x\n", - ret, master_map->id); + "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; } @@ -1339,8 +1340,9 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, if (ret < 0) { dev_warn_ratelimited(&chain->dev->udev->dev, - "UVC non compliance: Error %d populating cache of control %x\n", - ret, mapping->id); + "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; > > Regards, > > Hans > > > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED; > > + } > > } > > > > if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) { > > > > --- > > base-commit: c5aa327e10b194884a9c9001a751f6e4703bc3e3 > > change-id: 20241213-uvc-eaccess-755cc061a360 > > > > Best regards, > -- Ricardo Ribalda
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4e58476d305e..d69b9efa74d0 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,42 @@ 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\n", + ret, master_map->id); + } 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\n", + ret, mapping->id); + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED; + } } if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) {
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@chromium.org> --- 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 v3: - Add a retry mechanism during error. - Set V4L2_CTRL_FLAG_DISABLED flag. - Link to v2: https://lore.kernel.org/r/20241219-uvc-eaccess-v2-1-bf6520c8b86d@chromium.org 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@chromium.org --- drivers/media/usb/uvc/uvc_ctrl.c | 41 ++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) --- base-commit: c5aa327e10b194884a9c9001a751f6e4703bc3e3 change-id: 20241213-uvc-eaccess-755cc061a360 Best regards,