drivers/media/usb/uvc/uvc_ctrl.c | 53 ++++++++++++++++++++++++++++++++++------ drivers/media/usb/uvc/uvcvideo.h | 2 ++ 2 files changed, 47 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. These
error can be triggered every time, sometimes, or when other controls do
not have the "right value". 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:
- Retries for an extra attempt to read the control, to avoid spurious
errors. More attempts do not seem to produce better results in the
tested hardware.
- 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.
- Makes sure we keep returning V4L2_CTRL_FLAG_DISABLED for all the next
attempts to query that control (other operations have the same
functionality as now).
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 later 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
User Controls
brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
white_balance_automatic 0x0098090c (bool) : default=1 value=1
gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
With this patch:
$ v4l2-ctl --list-ctrls
User Controls
brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
white_balance_automatic 0x0098090c (bool) : default=1 value=1
gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
sharpness 0x0098091b (int) : min=0 max=7 step=1 default=3 value=3
backlight_compensation 0x0098091c (int) : min=0 max=2 step=1 default=1 value=1
[ 32.777643] usb 3-6: UVC non compliance: permanently disabling control 98091b (Sharpness), due to error -5
Camera Controls
auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
exposure_time_absolute 0x009a0902 (int) : min=2 max=1250 step=1 default=156 value=156 flags=inactive
exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0
privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only
region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload
region_of_interest_auto_ctrls 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
Emulating error with:
+ if (cs == UVC_PU_SHARPNESS_CONTROL && query == UVC_GET_MAX) {
+ printk(KERN_ERR "%s:%d %s\n", __FILE__, __LINE__, __func__);
+ return -EIO;
+ }
In uvc_query_ctrl()
---
Changes in v7:
- Only retry on -EIO (Thanks Hans).
- Add comment for retry (Thanks Hans).
- Rebase patch
- Check master_map->disabled before reading the master control.
- Link to v6: https://lore.kernel.org/r/20250310-uvc-eaccess-v6-1-bf4562f7cabd@chromium.org
Changes in v6:
- Keep returning V4L2_CTRL_FLAG_DISABLED in future control queries.
- Link to v5: https://lore.kernel.org/r/20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org
Changes in v5:
- Explain the retry in the commit message (Thanks Laurent).
- Link to v4: https://lore.kernel.org/r/20250111-uvc-eaccess-v4-1-c7759bfd1bd4@chromium.org
Changes in v4:
- Display control name (Thanks Hans)
- Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org
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 | 53 ++++++++++++++++++++++++++++++++++------
drivers/media/usb/uvc/uvcvideo.h | 2 ++
2 files changed, 47 insertions(+), 8 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index cbf19aa1d82374a08cf79b6a6787fa348b83523a..b41fed364d54aefd7da72c47197cf9d9e3c1b176 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1483,14 +1483,28 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
return ~0;
}
+/*
+ * Maximum retry count to avoid spurious errors with controls. Increase this
+ * value does no seem to produce better results in the tested hardware.
+ */
+#define MAX_QUERY_RETRIES 2
+
static int __uvc_queryctrl_boundaries(struct uvc_video_chain *chain,
struct uvc_control *ctrl,
struct uvc_control_mapping *mapping,
struct v4l2_query_ext_ctrl *v4l2_ctrl)
{
if (!ctrl->cached) {
- int ret = uvc_ctrl_populate_cache(chain, ctrl);
- if (ret < 0)
+ unsigned int retries;
+ int ret;
+
+ for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
+ ret = uvc_ctrl_populate_cache(chain, ctrl);
+ if (ret != -EIO)
+ break;
+ }
+
+ if (ret)
return ret;
}
@@ -1567,6 +1581,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
{
struct uvc_control_mapping *master_map = NULL;
struct uvc_control *master_ctrl = NULL;
+ int ret;
memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
v4l2_ctrl->id = mapping->id;
@@ -1587,18 +1602,29 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
__uvc_find_control(ctrl->entity, mapping->master_id,
&master_map, &master_ctrl, 0, 0);
if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
+ unsigned int retries;
s32 val;
int ret;
if (WARN_ON(uvc_ctrl_mapping_is_compound(master_map)))
return -EIO;
- ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
- if (ret < 0)
- return ret;
+ for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
+ ret = __uvc_ctrl_get(chain, master_ctrl, master_map,
+ &val);
+ if (ret != -EIO)
+ break;
+ }
- if (val != mapping->master_manual)
- v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
+ 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;
+ }
}
v4l2_ctrl->elem_size = uvc_mapping_v4l2_size(mapping);
@@ -1613,7 +1639,18 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
return 0;
}
- return __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
+ ret = __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
+ if (ret && !mapping->disabled) {
+ dev_warn(&chain->dev->udev->dev,
+ "UVC non compliance: permanently disabling control %x (%s), due to error %d\n",
+ mapping->id, uvc_map_get_name(mapping), ret);
+ mapping->disabled = true;
+ }
+
+ if (mapping->disabled)
+ v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED;
+
+ return 0;
}
int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..8e3753896d42baddcc2192057e8c5786ddd79fa8 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -134,6 +134,8 @@ struct uvc_control_mapping {
s32 master_manual;
u32 slave_ids[2];
+ bool disabled;
+
const struct uvc_control_mapping *(*filter_mapping)
(struct uvc_video_chain *chain,
struct uvc_control *ctrl);
---
base-commit: 4e82c87058f45e79eeaa4d5bcc3b38dd3dce7209
change-id: 20250403-uvc-eaccess-8f3666151830
Best regards,
--
Ricardo Ribalda <ribalda@chromium.org>Hi Ricardo,
Some more comments:
On 03/04/2025 14:59, 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. These
> error can be triggered every time, sometimes, or when other controls do
> not have the "right value". 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:
> - Retries for an extra attempt to read the control, to avoid spurious
> errors. More attempts do not seem to produce better results in the
> tested hardware.
> - 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.
> - Makes sure we keep returning V4L2_CTRL_FLAG_DISABLED for all the next
> attempts to query that control (other operations have the same
> functionality as now).
>
> 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 later 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
> User Controls
>
> brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
> contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
> saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
> hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
> white_balance_automatic 0x0098090c (bool) : default=1 value=1
> gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
> power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
> white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
>
> With this patch:
> $ v4l2-ctl --list-ctrls
>
> User Controls
>
> brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
> contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
> saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
> hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
> white_balance_automatic 0x0098090c (bool) : default=1 value=1
> gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
> power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
> white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
> sharpness 0x0098091b (int) : min=0 max=7 step=1 default=3 value=3
> backlight_compensation 0x0098091c (int) : min=0 max=2 step=1 default=1 value=1
> [ 32.777643] usb 3-6: UVC non compliance: permanently disabling control 98091b (Sharpness), due to error -5
>
> Camera Controls
>
> auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
> exposure_time_absolute 0x009a0902 (int) : min=2 max=1250 step=1 default=156 value=156 flags=inactive
> exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0
> privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only
> region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload
> region_of_interest_auto_ctrls 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
When you post a v8, remember to update the v4l2-ctl --list-ctrls output.
>
> Emulating error with:
> + if (cs == UVC_PU_SHARPNESS_CONTROL && query == UVC_GET_MAX) {
> + printk(KERN_ERR "%s:%d %s\n", __FILE__, __LINE__, __func__);
> + return -EIO;
> + }
> In uvc_query_ctrl()
> ---
> Changes in v7:
> - Only retry on -EIO (Thanks Hans).
> - Add comment for retry (Thanks Hans).
> - Rebase patch
> - Check master_map->disabled before reading the master control.
> - Link to v6: https://lore.kernel.org/r/20250310-uvc-eaccess-v6-1-bf4562f7cabd@chromium.org
>
> Changes in v6:
> - Keep returning V4L2_CTRL_FLAG_DISABLED in future control queries.
> - Link to v5: https://lore.kernel.org/r/20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org
>
> Changes in v5:
> - Explain the retry in the commit message (Thanks Laurent).
> - Link to v4: https://lore.kernel.org/r/20250111-uvc-eaccess-v4-1-c7759bfd1bd4@chromium.org
>
> Changes in v4:
> - Display control name (Thanks Hans)
> - Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org
>
> 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 | 53 ++++++++++++++++++++++++++++++++++------
> drivers/media/usb/uvc/uvcvideo.h | 2 ++
> 2 files changed, 47 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index cbf19aa1d82374a08cf79b6a6787fa348b83523a..b41fed364d54aefd7da72c47197cf9d9e3c1b176 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1483,14 +1483,28 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
> return ~0;
> }
>
> +/*
> + * Maximum retry count to avoid spurious errors with controls. Increase this
Increase -> Increasing
> + * value does no seem to produce better results in the tested hardware.
> + */
> +#define MAX_QUERY_RETRIES 2
> +
> static int __uvc_queryctrl_boundaries(struct uvc_video_chain *chain,
> struct uvc_control *ctrl,
> struct uvc_control_mapping *mapping,
> struct v4l2_query_ext_ctrl *v4l2_ctrl)
> {
> if (!ctrl->cached) {
> - int ret = uvc_ctrl_populate_cache(chain, ctrl);
> - if (ret < 0)
> + unsigned int retries;
> + int ret;
> +
> + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
> + ret = uvc_ctrl_populate_cache(chain, ctrl);
> + if (ret != -EIO)
> + break;
> + }
> +
> + if (ret)
> return ret;
> }
>
> @@ -1567,6 +1581,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> {
> struct uvc_control_mapping *master_map = NULL;
> struct uvc_control *master_ctrl = NULL;
> + int ret;
>
> memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
> v4l2_ctrl->id = mapping->id;
> @@ -1587,18 +1602,29 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> __uvc_find_control(ctrl->entity, mapping->master_id,
> &master_map, &master_ctrl, 0, 0);
> if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> + unsigned int retries;
> s32 val;
> int ret;
>
> if (WARN_ON(uvc_ctrl_mapping_is_compound(master_map)))
> return -EIO;
>
> - ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> - if (ret < 0)
> - return ret;
> + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map,
> + &val);
I'd write this:
if (ret < 0 && ret != -EIO)
return ret;
Without that you'll see the non compliance warning below, but that's not
what you want for non-EIO errors.
> + if (ret != -EIO)
> + break;
> + }
>
> - if (val != mapping->master_manual)
> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> + 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;
> + }
> }
>
> v4l2_ctrl->elem_size = uvc_mapping_v4l2_size(mapping);
> @@ -1613,7 +1639,18 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> return 0;
> }
>
> - return __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
> + ret = __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
> + if (ret && !mapping->disabled) {
> + dev_warn(&chain->dev->udev->dev,
> + "UVC non compliance: permanently disabling control %x (%s), due to error %d\n",
> + mapping->id, uvc_map_get_name(mapping), ret);
> + mapping->disabled = true;
> + }
> +
> + if (mapping->disabled)
> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED;
> +
> + return 0;
> }
>
> int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..8e3753896d42baddcc2192057e8c5786ddd79fa8 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -134,6 +134,8 @@ struct uvc_control_mapping {
> s32 master_manual;
> u32 slave_ids[2];
>
> + bool disabled;
> +
> const struct uvc_control_mapping *(*filter_mapping)
> (struct uvc_video_chain *chain,
> struct uvc_control *ctrl);
>
> ---
> base-commit: 4e82c87058f45e79eeaa4d5bcc3b38dd3dce7209
> change-id: 20250403-uvc-eaccess-8f3666151830
>
> Best regards,
Regards,
Hans
Hi Hans
On Mon, 7 Apr 2025 at 16:02, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Ricardo,
>
> Some more comments:
>
> On 03/04/2025 14:59, 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. These
> > error can be triggered every time, sometimes, or when other controls do
> > not have the "right value". 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:
> > - Retries for an extra attempt to read the control, to avoid spurious
> > errors. More attempts do not seem to produce better results in the
> > tested hardware.
> > - 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.
> > - Makes sure we keep returning V4L2_CTRL_FLAG_DISABLED for all the next
> > attempts to query that control (other operations have the same
> > functionality as now).
> >
> > 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 later 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
> > User Controls
> >
> > brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
> > contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
> > saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
> > hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
> > white_balance_automatic 0x0098090c (bool) : default=1 value=1
> > gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
> > power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
> > white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
> >
> > With this patch:
> > $ v4l2-ctl --list-ctrls
> >
> > User Controls
> >
> > brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
> > contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
> > saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
> > hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
> > white_balance_automatic 0x0098090c (bool) : default=1 value=1
> > gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
> > power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
> > white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
> > sharpness 0x0098091b (int) : min=0 max=7 step=1 default=3 value=3
> > backlight_compensation 0x0098091c (int) : min=0 max=2 step=1 default=1 value=1
> > [ 32.777643] usb 3-6: UVC non compliance: permanently disabling control 98091b (Sharpness), due to error -5
> >
> > Camera Controls
> >
> > auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
> > exposure_time_absolute 0x009a0902 (int) : min=2 max=1250 step=1 default=156 value=156 flags=inactive
> > exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0
> > privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only
> > region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload
> > region_of_interest_auto_ctrls 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
>
> When you post a v8, remember to update the v4l2-ctl --list-ctrls output.
will do. Please note that the output is after --- so it will not be in
git log. Let me know if I shall promote it.
>
> >
> > Emulating error with:
> > + if (cs == UVC_PU_SHARPNESS_CONTROL && query == UVC_GET_MAX) {
> > + printk(KERN_ERR "%s:%d %s\n", __FILE__, __LINE__, __func__);
> > + return -EIO;
> > + }
> > In uvc_query_ctrl()
> > ---
> > Changes in v7:
> > - Only retry on -EIO (Thanks Hans).
> > - Add comment for retry (Thanks Hans).
> > - Rebase patch
> > - Check master_map->disabled before reading the master control.
> > - Link to v6: https://lore.kernel.org/r/20250310-uvc-eaccess-v6-1-bf4562f7cabd@chromium.org
> >
> > Changes in v6:
> > - Keep returning V4L2_CTRL_FLAG_DISABLED in future control queries.
> > - Link to v5: https://lore.kernel.org/r/20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org
> >
> > Changes in v5:
> > - Explain the retry in the commit message (Thanks Laurent).
> > - Link to v4: https://lore.kernel.org/r/20250111-uvc-eaccess-v4-1-c7759bfd1bd4@chromium.org
> >
> > Changes in v4:
> > - Display control name (Thanks Hans)
> > - Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org
> >
> > 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 | 53 ++++++++++++++++++++++++++++++++++------
> > drivers/media/usb/uvc/uvcvideo.h | 2 ++
> > 2 files changed, 47 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index cbf19aa1d82374a08cf79b6a6787fa348b83523a..b41fed364d54aefd7da72c47197cf9d9e3c1b176 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1483,14 +1483,28 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
> > return ~0;
> > }
> >
> > +/*
> > + * Maximum retry count to avoid spurious errors with controls. Increase this
>
> Increase -> Increasing
>
> > + * value does no seem to produce better results in the tested hardware.
> > + */
> > +#define MAX_QUERY_RETRIES 2
> > +
> > static int __uvc_queryctrl_boundaries(struct uvc_video_chain *chain,
> > struct uvc_control *ctrl,
> > struct uvc_control_mapping *mapping,
> > struct v4l2_query_ext_ctrl *v4l2_ctrl)
> > {
> > if (!ctrl->cached) {
> > - int ret = uvc_ctrl_populate_cache(chain, ctrl);
> > - if (ret < 0)
> > + unsigned int retries;
> > + int ret;
> > +
> > + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
> > + ret = uvc_ctrl_populate_cache(chain, ctrl);
> > + if (ret != -EIO)
> > + break;
> > + }
> > +
> > + if (ret)
> > return ret;
> > }
> >
> > @@ -1567,6 +1581,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > {
> > struct uvc_control_mapping *master_map = NULL;
> > struct uvc_control *master_ctrl = NULL;
> > + int ret;
> >
> > memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
> > v4l2_ctrl->id = mapping->id;
> > @@ -1587,18 +1602,29 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > __uvc_find_control(ctrl->entity, mapping->master_id,
> > &master_map, &master_ctrl, 0, 0);
> > if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> > + unsigned int retries;
> > s32 val;
> > int ret;
> >
> > if (WARN_ON(uvc_ctrl_mapping_is_compound(master_map)))
> > return -EIO;
> >
> > - ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> > - if (ret < 0)
> > - return ret;
> > + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
> > + ret = __uvc_ctrl_get(chain, master_ctrl, master_map,
> > + &val);
>
> I'd write this:
>
> if (ret < 0 && ret != -EIO)
> return ret;
>
> Without that you'll see the non compliance warning below, but that's not
> what you want for non-EIO errors.
I am not sure about this one. If there is an error reading a master
control you probably want to continue. Semantically it is similar to
not finding the control or having invalid flags. And in those two
cases we move on.
Also, I think there is some value for the warning. It is counter
intuitive that query_ctrl for control X fails due to an error in
control Y.
Let me know what do you think.
>
> > + if (ret != -EIO)
> > + break;
> > + }
> >
> > - if (val != mapping->master_manual)
> > - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> > + 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;
> > + }
> > }
> >
> > v4l2_ctrl->elem_size = uvc_mapping_v4l2_size(mapping);
> > @@ -1613,7 +1639,18 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > return 0;
> > }
> >
> > - return __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
> > + ret = __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
> > + if (ret && !mapping->disabled) {
> > + dev_warn(&chain->dev->udev->dev,
> > + "UVC non compliance: permanently disabling control %x (%s), due to error %d\n",
> > + mapping->id, uvc_map_get_name(mapping), ret);
> > + mapping->disabled = true;
> > + }
> > +
> > + if (mapping->disabled)
> > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED;
> > +
> > + return 0;
> > }
> >
> > int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..8e3753896d42baddcc2192057e8c5786ddd79fa8 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -134,6 +134,8 @@ struct uvc_control_mapping {
> > s32 master_manual;
> > u32 slave_ids[2];
> >
> > + bool disabled;
> > +
> > const struct uvc_control_mapping *(*filter_mapping)
> > (struct uvc_video_chain *chain,
> > struct uvc_control *ctrl);
> >
> > ---
> > base-commit: 4e82c87058f45e79eeaa4d5bcc3b38dd3dce7209
> > change-id: 20250403-uvc-eaccess-8f3666151830
> >
> > Best regards,
>
> Regards,
>
> Hans
--
Ricardo Ribalda
Hi Ricardo,
Apologies, this got snowed under.
On 07/04/2025 16:39, Ricardo Ribalda wrote:
> Hi Hans
>
> On Mon, 7 Apr 2025 at 16:02, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Hi Ricardo,
>>
>> Some more comments:
>>
>> On 03/04/2025 14:59, 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. These
>>> error can be triggered every time, sometimes, or when other controls do
>>> not have the "right value". 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:
>>> - Retries for an extra attempt to read the control, to avoid spurious
>>> errors. More attempts do not seem to produce better results in the
>>> tested hardware.
>>> - 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.
>>> - Makes sure we keep returning V4L2_CTRL_FLAG_DISABLED for all the next
>>> attempts to query that control (other operations have the same
>>> functionality as now).
>>>
>>> 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 later 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
>>> User Controls
>>>
>>> brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
>>> contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
>>> saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
>>> hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
>>> white_balance_automatic 0x0098090c (bool) : default=1 value=1
>>> gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
>>> power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
>>> white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
>>>
>>> With this patch:
>>> $ v4l2-ctl --list-ctrls
>>>
>>> User Controls
>>>
>>> brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
>>> contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
>>> saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
>>> hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
>>> white_balance_automatic 0x0098090c (bool) : default=1 value=1
>>> gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
>>> power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
>>> white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
>>> sharpness 0x0098091b (int) : min=0 max=7 step=1 default=3 value=3
>>> backlight_compensation 0x0098091c (int) : min=0 max=2 step=1 default=1 value=1
>>> [ 32.777643] usb 3-6: UVC non compliance: permanently disabling control 98091b (Sharpness), due to error -5
>>>
>>> Camera Controls
>>>
>>> auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
>>> exposure_time_absolute 0x009a0902 (int) : min=2 max=1250 step=1 default=156 value=156 flags=inactive
>>> exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0
>>> privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only
>>> region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload
>>> region_of_interest_auto_ctrls 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
>>
>> When you post a v8, remember to update the v4l2-ctl --list-ctrls output.
>
> will do. Please note that the output is after --- so it will not be in
> git log. Let me know if I shall promote it.
It's fine after the ---.
>
>>
>>>
>>> Emulating error with:
>>> + if (cs == UVC_PU_SHARPNESS_CONTROL && query == UVC_GET_MAX) {
>>> + printk(KERN_ERR "%s:%d %s\n", __FILE__, __LINE__, __func__);
>>> + return -EIO;
>>> + }
>>> In uvc_query_ctrl()
>>> ---
>>> Changes in v7:
>>> - Only retry on -EIO (Thanks Hans).
>>> - Add comment for retry (Thanks Hans).
>>> - Rebase patch
>>> - Check master_map->disabled before reading the master control.
>>> - Link to v6: https://lore.kernel.org/r/20250310-uvc-eaccess-v6-1-bf4562f7cabd@chromium.org
>>>
>>> Changes in v6:
>>> - Keep returning V4L2_CTRL_FLAG_DISABLED in future control queries.
>>> - Link to v5: https://lore.kernel.org/r/20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org
>>>
>>> Changes in v5:
>>> - Explain the retry in the commit message (Thanks Laurent).
>>> - Link to v4: https://lore.kernel.org/r/20250111-uvc-eaccess-v4-1-c7759bfd1bd4@chromium.org
>>>
>>> Changes in v4:
>>> - Display control name (Thanks Hans)
>>> - Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org
>>>
>>> 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 | 53 ++++++++++++++++++++++++++++++++++------
>>> drivers/media/usb/uvc/uvcvideo.h | 2 ++
>>> 2 files changed, 47 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>>> index cbf19aa1d82374a08cf79b6a6787fa348b83523a..b41fed364d54aefd7da72c47197cf9d9e3c1b176 100644
>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>>> @@ -1483,14 +1483,28 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
>>> return ~0;
>>> }
>>>
>>> +/*
>>> + * Maximum retry count to avoid spurious errors with controls. Increase this
>>
>> Increase -> Increasing
>>
>>> + * value does no seem to produce better results in the tested hardware.
>>> + */
>>> +#define MAX_QUERY_RETRIES 2
>>> +
>>> static int __uvc_queryctrl_boundaries(struct uvc_video_chain *chain,
>>> struct uvc_control *ctrl,
>>> struct uvc_control_mapping *mapping,
>>> struct v4l2_query_ext_ctrl *v4l2_ctrl)
>>> {
>>> if (!ctrl->cached) {
>>> - int ret = uvc_ctrl_populate_cache(chain, ctrl);
>>> - if (ret < 0)
>>> + unsigned int retries;
>>> + int ret;
>>> +
>>> + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
>>> + ret = uvc_ctrl_populate_cache(chain, ctrl);
>>> + if (ret != -EIO)
>>> + break;
>>> + }
>>> +
>>> + if (ret)
>>> return ret;
>>> }
>>>
>>> @@ -1567,6 +1581,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>> {
>>> struct uvc_control_mapping *master_map = NULL;
>>> struct uvc_control *master_ctrl = NULL;
>>> + int ret;
>>>
>>> memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
>>> v4l2_ctrl->id = mapping->id;
>>> @@ -1587,18 +1602,29 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>> __uvc_find_control(ctrl->entity, mapping->master_id,
>>> &master_map, &master_ctrl, 0, 0);
>>> if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
>>> + unsigned int retries;
>>> s32 val;
>>> int ret;
>>>
>>> if (WARN_ON(uvc_ctrl_mapping_is_compound(master_map)))
>>> return -EIO;
>>>
>>> - ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
>>> - if (ret < 0)
>>> - return ret;
>>> + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
>>> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map,
>>> + &val);
>>
>> I'd write this:
>>
>> if (ret < 0 && ret != -EIO)
>> return ret;
>>
>> Without that you'll see the non compliance warning below, but that's not
>> what you want for non-EIO errors.
>
> I am not sure about this one. If there is an error reading a master
> control you probably want to continue. Semantically it is similar to
> not finding the control or having invalid flags. And in those two
> cases we move on.
> Also, I think there is some value for the warning. It is counter
> intuitive that query_ctrl for control X fails due to an error in
> control Y.
In the original code you just return the error as well, without a
warning.
I think for non-EIO errors you should just return. It can always be
changed later, for now I think we should stick close to the original
code.
It's really a matter of taste rather than right or wrong code.
Regards,
Hans
>
> Let me know what do you think.
>
>>
>>> + if (ret != -EIO)
>>> + break;
>>> + }
>>>
>>> - if (val != mapping->master_manual)
>>> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>>> + 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;
>>> + }
>>> }
>>>
>>> v4l2_ctrl->elem_size = uvc_mapping_v4l2_size(mapping);
>>> @@ -1613,7 +1639,18 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>> return 0;
>>> }
>>>
>>> - return __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
>>> + ret = __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
>>> + if (ret && !mapping->disabled) {
>>> + dev_warn(&chain->dev->udev->dev,
>>> + "UVC non compliance: permanently disabling control %x (%s), due to error %d\n",
>>> + mapping->id, uvc_map_get_name(mapping), ret);
>>> + mapping->disabled = true;
>>> + }
>>> +
>>> + if (mapping->disabled)
>>> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED;
>>> +
>>> + return 0;
>>> }
>>>
>>> int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..8e3753896d42baddcc2192057e8c5786ddd79fa8 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -134,6 +134,8 @@ struct uvc_control_mapping {
>>> s32 master_manual;
>>> u32 slave_ids[2];
>>>
>>> + bool disabled;
>>> +
>>> const struct uvc_control_mapping *(*filter_mapping)
>>> (struct uvc_video_chain *chain,
>>> struct uvc_control *ctrl);
>>>
>>> ---
>>> base-commit: 4e82c87058f45e79eeaa4d5bcc3b38dd3dce7209
>>> change-id: 20250403-uvc-eaccess-8f3666151830
>>>
>>> Best regards,
>>
>> Regards,
>>
>> Hans
>
>
>
Hi,
On 3-Apr-25 14:59, 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. These
> error can be triggered every time, sometimes, or when other controls do
> not have the "right value". 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:
> - Retries for an extra attempt to read the control, to avoid spurious
> errors. More attempts do not seem to produce better results in the
> tested hardware.
> - 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.
> - Makes sure we keep returning V4L2_CTRL_FLAG_DISABLED for all the next
> attempts to query that control (other operations have the same
> functionality as now).
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Hans Verkuil, are you happy with this patch now that Ricardo has provided
updated v4l2-ctl output ?
Regards,
Hans
> ---
> 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 later 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
> User Controls
>
> brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
> contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
> saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
> hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
> white_balance_automatic 0x0098090c (bool) : default=1 value=1
> gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
> power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
> white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
>
> With this patch:
> $ v4l2-ctl --list-ctrls
>
> User Controls
>
> brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
> contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
> saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
> hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
> white_balance_automatic 0x0098090c (bool) : default=1 value=1
> gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
> power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
> white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
> sharpness 0x0098091b (int) : min=0 max=7 step=1 default=3 value=3
> backlight_compensation 0x0098091c (int) : min=0 max=2 step=1 default=1 value=1
> [ 32.777643] usb 3-6: UVC non compliance: permanently disabling control 98091b (Sharpness), due to error -5
>
> Camera Controls
>
> auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
> exposure_time_absolute 0x009a0902 (int) : min=2 max=1250 step=1 default=156 value=156 flags=inactive
> exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0
> privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only
> region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload
> region_of_interest_auto_ctrls 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
>
> Emulating error with:
> + if (cs == UVC_PU_SHARPNESS_CONTROL && query == UVC_GET_MAX) {
> + printk(KERN_ERR "%s:%d %s\n", __FILE__, __LINE__, __func__);
> + return -EIO;
> + }
> In uvc_query_ctrl()
> ---
> Changes in v7:
> - Only retry on -EIO (Thanks Hans).
> - Add comment for retry (Thanks Hans).
> - Rebase patch
> - Check master_map->disabled before reading the master control.
> - Link to v6: https://lore.kernel.org/r/20250310-uvc-eaccess-v6-1-bf4562f7cabd@chromium.org
>
> Changes in v6:
> - Keep returning V4L2_CTRL_FLAG_DISABLED in future control queries.
> - Link to v5: https://lore.kernel.org/r/20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org
>
> Changes in v5:
> - Explain the retry in the commit message (Thanks Laurent).
> - Link to v4: https://lore.kernel.org/r/20250111-uvc-eaccess-v4-1-c7759bfd1bd4@chromium.org
>
> Changes in v4:
> - Display control name (Thanks Hans)
> - Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org
>
> 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 | 53 ++++++++++++++++++++++++++++++++++------
> drivers/media/usb/uvc/uvcvideo.h | 2 ++
> 2 files changed, 47 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index cbf19aa1d82374a08cf79b6a6787fa348b83523a..b41fed364d54aefd7da72c47197cf9d9e3c1b176 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1483,14 +1483,28 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
> return ~0;
> }
>
> +/*
> + * Maximum retry count to avoid spurious errors with controls. Increase this
> + * value does no seem to produce better results in the tested hardware.
> + */
> +#define MAX_QUERY_RETRIES 2
> +
> static int __uvc_queryctrl_boundaries(struct uvc_video_chain *chain,
> struct uvc_control *ctrl,
> struct uvc_control_mapping *mapping,
> struct v4l2_query_ext_ctrl *v4l2_ctrl)
> {
> if (!ctrl->cached) {
> - int ret = uvc_ctrl_populate_cache(chain, ctrl);
> - if (ret < 0)
> + unsigned int retries;
> + int ret;
> +
> + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
> + ret = uvc_ctrl_populate_cache(chain, ctrl);
> + if (ret != -EIO)
> + break;
> + }
> +
> + if (ret)
> return ret;
> }
>
> @@ -1567,6 +1581,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> {
> struct uvc_control_mapping *master_map = NULL;
> struct uvc_control *master_ctrl = NULL;
> + int ret;
>
> memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
> v4l2_ctrl->id = mapping->id;
> @@ -1587,18 +1602,29 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> __uvc_find_control(ctrl->entity, mapping->master_id,
> &master_map, &master_ctrl, 0, 0);
> if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> + unsigned int retries;
> s32 val;
> int ret;
>
> if (WARN_ON(uvc_ctrl_mapping_is_compound(master_map)))
> return -EIO;
>
> - ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> - if (ret < 0)
> - return ret;
> + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map,
> + &val);
> + if (ret != -EIO)
> + break;
> + }
>
> - if (val != mapping->master_manual)
> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> + 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;
> + }
> }
>
> v4l2_ctrl->elem_size = uvc_mapping_v4l2_size(mapping);
> @@ -1613,7 +1639,18 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> return 0;
> }
>
> - return __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
> + ret = __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
> + if (ret && !mapping->disabled) {
> + dev_warn(&chain->dev->udev->dev,
> + "UVC non compliance: permanently disabling control %x (%s), due to error %d\n",
> + mapping->id, uvc_map_get_name(mapping), ret);
> + mapping->disabled = true;
> + }
> +
> + if (mapping->disabled)
> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED;
> +
> + return 0;
> }
>
> int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..8e3753896d42baddcc2192057e8c5786ddd79fa8 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -134,6 +134,8 @@ struct uvc_control_mapping {
> s32 master_manual;
> u32 slave_ids[2];
>
> + bool disabled;
> +
> const struct uvc_control_mapping *(*filter_mapping)
> (struct uvc_video_chain *chain,
> struct uvc_control *ctrl);
>
> ---
> base-commit: 4e82c87058f45e79eeaa4d5bcc3b38dd3dce7209
> change-id: 20250403-uvc-eaccess-8f3666151830
>
> Best regards,
On 03/04/2025 14:59, 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. These
> error can be triggered every time, sometimes, or when other controls do
> not have the "right value". 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:
> - Retries for an extra attempt to read the control, to avoid spurious
> errors. More attempts do not seem to produce better results in the
> tested hardware.
> - 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.
> - Makes sure we keep returning V4L2_CTRL_FLAG_DISABLED for all the next
> attempts to query that control (other operations have the same
> functionality as now).
>
> 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 later 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
> User Controls
>
> brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
> contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
> saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
> hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
> white_balance_automatic 0x0098090c (bool) : default=1 value=1
> gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
> power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
> white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
>
> With this patch:
> $ v4l2-ctl --list-ctrls
>
> User Controls
>
> brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
> contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
> saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
> hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
> white_balance_automatic 0x0098090c (bool) : default=1 value=1
> gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
> power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
> white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
> sharpness 0x0098091b (int) : min=0 max=7 step=1 default=3 value=3
> backlight_compensation 0x0098091c (int) : min=0 max=2 step=1 default=1 value=1
> [ 32.777643] usb 3-6: UVC non compliance: permanently disabling control 98091b (Sharpness), due to error -5
So why isn't 'flags=disabled' listed with the sharpness control above?
Looking at the v4l2-ctl code I think it should just skip the 'sharpness' control and
not list it at all. I'm actually not so sure that is the right thing to do.
So try with this patch for v4l2-ctl:
diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
index 5b953cbd..41739294 100644
--- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
+++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
@@ -824,8 +824,6 @@ static int print_control(int fd, struct v4l2_query_ext_ctrl &qctrl, bool show_me
memset(&ctrl, 0, sizeof(ctrl));
memset(&ext_ctrl, 0, sizeof(ext_ctrl));
memset(&ctrls, 0, sizeof(ctrls));
- if (qctrl.flags & V4L2_CTRL_FLAG_DISABLED)
- return 1;
if (qctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS) {
printf("\n%s\n\n", qctrl.name);
return 1;
Regards,
Hans
>
> Camera Controls
>
> auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
> exposure_time_absolute 0x009a0902 (int) : min=2 max=1250 step=1 default=156 value=156 flags=inactive
> exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0
> privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only
> region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload
> region_of_interest_auto_ctrls 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
>
> Emulating error with:
> + if (cs == UVC_PU_SHARPNESS_CONTROL && query == UVC_GET_MAX) {
> + printk(KERN_ERR "%s:%d %s\n", __FILE__, __LINE__, __func__);
> + return -EIO;
> + }
> In uvc_query_ctrl()
> ---
> Changes in v7:
> - Only retry on -EIO (Thanks Hans).
> - Add comment for retry (Thanks Hans).
> - Rebase patch
> - Check master_map->disabled before reading the master control.
> - Link to v6: https://lore.kernel.org/r/20250310-uvc-eaccess-v6-1-bf4562f7cabd@chromium.org
>
> Changes in v6:
> - Keep returning V4L2_CTRL_FLAG_DISABLED in future control queries.
> - Link to v5: https://lore.kernel.org/r/20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org
>
> Changes in v5:
> - Explain the retry in the commit message (Thanks Laurent).
> - Link to v4: https://lore.kernel.org/r/20250111-uvc-eaccess-v4-1-c7759bfd1bd4@chromium.org
>
> Changes in v4:
> - Display control name (Thanks Hans)
> - Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org
>
> 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 | 53 ++++++++++++++++++++++++++++++++++------
> drivers/media/usb/uvc/uvcvideo.h | 2 ++
> 2 files changed, 47 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index cbf19aa1d82374a08cf79b6a6787fa348b83523a..b41fed364d54aefd7da72c47197cf9d9e3c1b176 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1483,14 +1483,28 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
> return ~0;
> }
>
> +/*
> + * Maximum retry count to avoid spurious errors with controls. Increase this
> + * value does no seem to produce better results in the tested hardware.
> + */
> +#define MAX_QUERY_RETRIES 2
> +
> static int __uvc_queryctrl_boundaries(struct uvc_video_chain *chain,
> struct uvc_control *ctrl,
> struct uvc_control_mapping *mapping,
> struct v4l2_query_ext_ctrl *v4l2_ctrl)
> {
> if (!ctrl->cached) {
> - int ret = uvc_ctrl_populate_cache(chain, ctrl);
> - if (ret < 0)
> + unsigned int retries;
> + int ret;
> +
> + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
> + ret = uvc_ctrl_populate_cache(chain, ctrl);
> + if (ret != -EIO)
> + break;
> + }
> +
> + if (ret)
> return ret;
> }
>
> @@ -1567,6 +1581,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> {
> struct uvc_control_mapping *master_map = NULL;
> struct uvc_control *master_ctrl = NULL;
> + int ret;
>
> memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
> v4l2_ctrl->id = mapping->id;
> @@ -1587,18 +1602,29 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> __uvc_find_control(ctrl->entity, mapping->master_id,
> &master_map, &master_ctrl, 0, 0);
> if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> + unsigned int retries;
> s32 val;
> int ret;
>
> if (WARN_ON(uvc_ctrl_mapping_is_compound(master_map)))
> return -EIO;
>
> - ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> - if (ret < 0)
> - return ret;
> + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map,
> + &val);
> + if (ret != -EIO)
> + break;
> + }
>
> - if (val != mapping->master_manual)
> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> + 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;
> + }
> }
>
> v4l2_ctrl->elem_size = uvc_mapping_v4l2_size(mapping);
> @@ -1613,7 +1639,18 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> return 0;
> }
>
> - return __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
> + ret = __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
> + if (ret && !mapping->disabled) {
> + dev_warn(&chain->dev->udev->dev,
> + "UVC non compliance: permanently disabling control %x (%s), due to error %d\n",
> + mapping->id, uvc_map_get_name(mapping), ret);
> + mapping->disabled = true;
> + }
> +
> + if (mapping->disabled)
> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED;
> +
> + return 0;
> }
>
> int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..8e3753896d42baddcc2192057e8c5786ddd79fa8 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -134,6 +134,8 @@ struct uvc_control_mapping {
> s32 master_manual;
> u32 slave_ids[2];
>
> + bool disabled;
> +
> const struct uvc_control_mapping *(*filter_mapping)
> (struct uvc_video_chain *chain,
> struct uvc_control *ctrl);
>
> ---
> base-commit: 4e82c87058f45e79eeaa4d5bcc3b38dd3dce7209
> change-id: 20250403-uvc-eaccess-8f3666151830
>
> Best regards,
On Thu, 3 Apr 2025 at 15:07, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 03/04/2025 14:59, 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. These
> > error can be triggered every time, sometimes, or when other controls do
> > not have the "right value". 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:
> > - Retries for an extra attempt to read the control, to avoid spurious
> > errors. More attempts do not seem to produce better results in the
> > tested hardware.
> > - 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.
> > - Makes sure we keep returning V4L2_CTRL_FLAG_DISABLED for all the next
> > attempts to query that control (other operations have the same
> > functionality as now).
> >
> > 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 later 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
> > User Controls
> >
> > brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
> > contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
> > saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
> > hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
> > white_balance_automatic 0x0098090c (bool) : default=1 value=1
> > gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
> > power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
> > white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
> >
> > With this patch:
> > $ v4l2-ctl --list-ctrls
> >
> > User Controls
> >
> > brightness 0x00980900 (int) : min=0 max=255 step=1 default=128 value=128
> > contrast 0x00980901 (int) : min=0 max=100 step=1 default=32 value=32
> > saturation 0x00980902 (int) : min=0 max=100 step=1 default=64 value=64
> > hue 0x00980903 (int) : min=-180 max=180 step=1 default=0 value=0
> > white_balance_automatic 0x0098090c (bool) : default=1 value=1
> > gamma 0x00980910 (int) : min=90 max=150 step=1 default=120 value=120
> > power_line_frequency 0x00980918 (menu) : min=0 max=2 default=2 value=2 (60 Hz)
> > white_balance_temperature 0x0098091a (int) : min=2800 max=6500 step=1 default=4600 value=4600 flags=inactive
> > sharpness 0x0098091b (int) : min=0 max=7 step=1 default=3 value=3
> > backlight_compensation 0x0098091c (int) : min=0 max=2 step=1 default=1 value=1
> > [ 32.777643] usb 3-6: UVC non compliance: permanently disabling control 98091b (Sharpness), due to error -5
>
> So why isn't 'flags=disabled' listed with the sharpness control above?
Because I am unable to copy/paste. Sorry about that
xol-rev8 ~ # v4l2-ctl -l
User Controls
brightness 0x00980900 (int) : min=0 max=255
step=1 default=128 value=128
contrast 0x00980901 (int) : min=0 max=100
step=1 default=32 value=32
saturation 0x00980902 (int) : min=0 max=100
step=1 default=64 value=64
hue 0x00980903 (int) : min=-180 max=180
step=1 default=0 value=0
white_balance_automatic 0x0098090c (bool) : default=1 value=1
gamma 0x00980910 (int) : min=90 max=150
step=1 default=120 value=120
power_line_frequency 0x00980918 (menu) : min=0 max=2
default=2 value=2 (60 Hz)
white_balance_temperature 0x0098091a (int) : min=2800
max=6500 step=1 default=4600 value=4600 flags=inactive
backlight_compensation 0x0098091c (int) : min=0 max=2
step=1 default=1 value=1
Camera Controls
auto_exposure 0x009a0901 (menu) : min=0 max=3
default=3 value=3 (Aperture Priority Mode)
exposure_time_absolute 0x009a0902 (int) : min=2 max=1250
step=1 default=156 value=156 flags=inactive
exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=1
privacy 0x009a0910 (bool) : default=0
value=0 flags=read-only
region_of_interest_rectangle 0x009a1901 (unknown): type=107
value=unsupported payload type flags=has-payload
region_of_interest_auto_ctrls 0x009a1902 (bitmask): max=0x00000001
default=0x00000001 value=1
>
> Looking at the v4l2-ctl code I think it should just skip the 'sharpness' control and
> not list it at all. I'm actually not so sure that is the right thing to do.
>
> So try with this patch for v4l2-ctl:
>
> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> index 5b953cbd..41739294 100644
> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> @@ -824,8 +824,6 @@ static int print_control(int fd, struct v4l2_query_ext_ctrl &qctrl, bool show_me
> memset(&ctrl, 0, sizeof(ctrl));
> memset(&ext_ctrl, 0, sizeof(ext_ctrl));
> memset(&ctrls, 0, sizeof(ctrls));
> - if (qctrl.flags & V4L2_CTRL_FLAG_DISABLED)
> - return 1;
> if (qctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS) {
> printf("\n%s\n\n", qctrl.name);
> return 1;
>
> Regards,
User Controls
brightness 0x00980900 (int) : min=0 max=255
step=1 default=128 value=128
contrast 0x00980901 (int) : min=0 max=100
step=1 default=32 value=32
saturation 0x00980902 (int) : min=0 max=100
step=1 default=64 value=64
hue 0x00980903 (int) : min=-180 max=180
step=1 default=0 value=0
white_balance_automatic 0x0098090c (bool) : default=1 value=1
gamma 0x00980910 (int) : min=90 max=150
step=1 default=120 value=120
power_line_frequency 0x00980918 (menu) : min=0 max=2
default=2 value=2 (60 Hz)
white_balance_temperature 0x0098091a (int) : min=2800
max=6500 step=1 default=4600 value=4600 flags=inactive
sharpness 0x0098091b (int) : min=0 max=0
step=0 default=0 value=3 flags=disabled
backlight_compensation 0x0098091c (int) : min=0 max=2
step=1 default=1 value=1
Camera Controls
auto_exposure 0x009a0901 (menu) : min=0 max=3
default=3 value=3 (Aperture Priority Mode)
exposure_time_absolute 0x009a0902 (int) : min=2 max=1250
step=1 default=156 value=156 flags=inactive
exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=1
privacy 0x009a0910 (bool) : default=0
value=0 flags=read-only
region_of_interest_rectangle 0x009a1901 (rect) :
value=(0,0)/1924x1084 flags=has-payload
region_of_interest_auto_ctrls 0x009a1902 (bitmask): max=0x00000001
default=0x00000001 value=1
>
> Hans
>
> >
> > Camera Controls
> >
> > auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
> > exposure_time_absolute 0x009a0902 (int) : min=2 max=1250 step=1 default=156 value=156 flags=inactive
> > exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0
> > privacy 0x009a0910 (bool) : default=0 value=0 flags=read-only
> > region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload
> > region_of_interest_auto_ctrls 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
> >
> > Emulating error with:
> > + if (cs == UVC_PU_SHARPNESS_CONTROL && query == UVC_GET_MAX) {
> > + printk(KERN_ERR "%s:%d %s\n", __FILE__, __LINE__, __func__);
> > + return -EIO;
> > + }
> > In uvc_query_ctrl()
> > ---
> > Changes in v7:
> > - Only retry on -EIO (Thanks Hans).
> > - Add comment for retry (Thanks Hans).
> > - Rebase patch
> > - Check master_map->disabled before reading the master control.
> > - Link to v6: https://lore.kernel.org/r/20250310-uvc-eaccess-v6-1-bf4562f7cabd@chromium.org
> >
> > Changes in v6:
> > - Keep returning V4L2_CTRL_FLAG_DISABLED in future control queries.
> > - Link to v5: https://lore.kernel.org/r/20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org
> >
> > Changes in v5:
> > - Explain the retry in the commit message (Thanks Laurent).
> > - Link to v4: https://lore.kernel.org/r/20250111-uvc-eaccess-v4-1-c7759bfd1bd4@chromium.org
> >
> > Changes in v4:
> > - Display control name (Thanks Hans)
> > - Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org
> >
> > 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 | 53 ++++++++++++++++++++++++++++++++++------
> > drivers/media/usb/uvc/uvcvideo.h | 2 ++
> > 2 files changed, 47 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index cbf19aa1d82374a08cf79b6a6787fa348b83523a..b41fed364d54aefd7da72c47197cf9d9e3c1b176 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1483,14 +1483,28 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
> > return ~0;
> > }
> >
> > +/*
> > + * Maximum retry count to avoid spurious errors with controls. Increase this
> > + * value does no seem to produce better results in the tested hardware.
> > + */
> > +#define MAX_QUERY_RETRIES 2
> > +
> > static int __uvc_queryctrl_boundaries(struct uvc_video_chain *chain,
> > struct uvc_control *ctrl,
> > struct uvc_control_mapping *mapping,
> > struct v4l2_query_ext_ctrl *v4l2_ctrl)
> > {
> > if (!ctrl->cached) {
> > - int ret = uvc_ctrl_populate_cache(chain, ctrl);
> > - if (ret < 0)
> > + unsigned int retries;
> > + int ret;
> > +
> > + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
> > + ret = uvc_ctrl_populate_cache(chain, ctrl);
> > + if (ret != -EIO)
> > + break;
> > + }
> > +
> > + if (ret)
> > return ret;
> > }
> >
> > @@ -1567,6 +1581,7 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > {
> > struct uvc_control_mapping *master_map = NULL;
> > struct uvc_control *master_ctrl = NULL;
> > + int ret;
> >
> > memset(v4l2_ctrl, 0, sizeof(*v4l2_ctrl));
> > v4l2_ctrl->id = mapping->id;
> > @@ -1587,18 +1602,29 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > __uvc_find_control(ctrl->entity, mapping->master_id,
> > &master_map, &master_ctrl, 0, 0);
> > if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> > + unsigned int retries;
> > s32 val;
> > int ret;
> >
> > if (WARN_ON(uvc_ctrl_mapping_is_compound(master_map)))
> > return -EIO;
> >
> > - ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> > - if (ret < 0)
> > - return ret;
> > + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
> > + ret = __uvc_ctrl_get(chain, master_ctrl, master_map,
> > + &val);
> > + if (ret != -EIO)
> > + break;
> > + }
> >
> > - if (val != mapping->master_manual)
> > - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> > + 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;
> > + }
> > }
> >
> > v4l2_ctrl->elem_size = uvc_mapping_v4l2_size(mapping);
> > @@ -1613,7 +1639,18 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > return 0;
> > }
> >
> > - return __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
> > + ret = __uvc_queryctrl_boundaries(chain, ctrl, mapping, v4l2_ctrl);
> > + if (ret && !mapping->disabled) {
> > + dev_warn(&chain->dev->udev->dev,
> > + "UVC non compliance: permanently disabling control %x (%s), due to error %d\n",
> > + mapping->id, uvc_map_get_name(mapping), ret);
> > + mapping->disabled = true;
> > + }
> > +
> > + if (mapping->disabled)
> > + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED;
> > +
> > + return 0;
> > }
> >
> > int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..8e3753896d42baddcc2192057e8c5786ddd79fa8 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -134,6 +134,8 @@ struct uvc_control_mapping {
> > s32 master_manual;
> > u32 slave_ids[2];
> >
> > + bool disabled;
> > +
> > const struct uvc_control_mapping *(*filter_mapping)
> > (struct uvc_video_chain *chain,
> > struct uvc_control *ctrl);
> >
> > ---
> > base-commit: 4e82c87058f45e79eeaa4d5bcc3b38dd3dce7209
> > change-id: 20250403-uvc-eaccess-8f3666151830
> >
> > Best regards,
>
--
Ricardo Ribalda
© 2016 - 2026 Red Hat, Inc.