On Fri May 2 07:48:28 2025 +0000, Ricardo Ribalda wrote:
> To implement VIDIOC_QUERYCTRL, we need to know the minimum, maximum,
> step and flags of the control. For some of the controls, this involves
> querying the actual hardware.
>
> Some non-compliant cameras produce errors when we query them. 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 for -EIO errors.
> - 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).
>
> Reviewed-by: Hans de Goede <[email protected]>
> Signed-off-by: Ricardo Ribalda <[email protected]>
> Link:
> https://lore.kernel.org/r/[email protected]
> Signed-off-by: Hans de Goede <[email protected]>
> Signed-off-by: Laurent Pinchart <[email protected]>
> Signed-off-by: Hans Verkuil <[email protected]>
Patch committed.
Thanks,
Hans Verkuil
drivers/media/usb/uvc/uvc_ctrl.c | 55 ++++++++++++++++++++++++++++++++++------
drivers/media/usb/uvc/uvcvideo.h | 2 ++
2 files changed, 49 insertions(+), 8 deletions(-)
---
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 44b6513c5264..f24272d483a2 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. Increasing 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,31 @@ 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)
+ break;
+ if (ret < 0 && ret != -EIO)
+ return ret;
+ }
- if (val != mapping->master_manual)
- v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
+ if (ret == -EIO) {
+ 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 +1641,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 b9f8eb62ba1d..11d6e3c2ebdf 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);
