drivers/media/usb/uvc/uvc_ctrl.c | 41 ++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-)
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);
+ 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 <ribalda@chromium.org>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
© 2016 - 2025 Red Hat, Inc.