From: Ricardo Ribalda <ribalda@chromium.org>
To: Hans Verkuil <hverkuil@xs4all.nl>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Hans de Goede <hdegoede@redhat.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
Ricardo Ribalda <ribalda@chromium.org>
Subject: [PATCH v2] media: uvcvideo: Filter hw errors while enumerating controls
Date: Thu, 19 Dec 2024 15:33:29 +0000 [thread overview]
Message-ID: <20241219-uvc-eaccess-v2-1-bf6520c8b86d@chromium.org> (raw)
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 and introduces a warning in dmesg so we can
have a trace of what has happened.
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 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 | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 4e58476d305e..43ddc5bb02db 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1307,17 +1307,24 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
s32 val;
int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
- if (ret < 0)
- return ret;
- 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\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;
+
+ if (ret < 0) {
+ dev_warn_ratelimited(&chain->dev->udev->dev,
+ "UVC non compliance: Error %d populating cache of control %x\n",
+ ret, mapping->id);
+ }
}
if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) {
---
base-commit: 40ed9e9b2808beeb835bd0ed971fb364c285d39c
change-id: 20241213-uvc-eaccess-755cc061a360
Best regards,
--
Ricardo Ribalda <ribalda@chromium.org>
next reply other threads:[~2024-12-19 15:33 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-19 15:33 Ricardo Ribalda [this message]
2025-01-06 15:01 ` Hans Verkuil
2025-01-06 16:20 ` Ricardo Ribalda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241219-uvc-eaccess-v2-1-bf6520c8b86d@chromium.org \
--to=ribalda@chromium.org \
--cc=hdegoede@redhat.com \
--cc=hverkuil@xs4all.nl \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®