Message-ID: <CANiDSCv0iHPN1Fj0tHGqg11uQO9CSviouN6K8p_yoHmfgBN7Jg@mail.gmail.com>
Date: Mon, 6 Jan 2025 17:20:04 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Hans Verkuil <hverkuil@...all.nl>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, Hans de Goede <hdegoede@...hat.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: uvcvideo: Filter hw errors while enumerating controls

Hi Hans

On Mon, 6 Jan 2025 at 16:01, Hans Verkuil <hverkuil@...all.nl> wrote:
>
> Hi all,
>
> On 19/12/2024 16:33, 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 and introduces a warning in dmesg so we can
> > have a trace of what has happened.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.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
>
> How about setting the V4L2_CTRL_FLAG_DISABLED flag for problematic controls, but still return 0?

SGTM, Specially after reading the footnote:
https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/vidioc-queryctrl.html#id1
V4L2_CTRL_FLAG_DISABLED was intended for two purposes: Drivers can
skip predefined controls not supported by the hardware (although
returning EINVAL would do as well), or disable predefined and private
controls after hardware detection without the trouble of reordering
control arrays and indices (EINVAL cannot be used to skip private
controls because it would prematurely end the enumeration).


>
> That way you can still report it, and you can see what the field values are (if you managed to read them),
> but the control is disabled, i.e. it can't be used.
>
> But it is probably wise to try at least twice to get the value in case it is a transient problem.
>
> Note that I think that v4l2-compliance will complain about the use of that flag, since it
> really shouldn't be used in 'normal' drivers, but it can skip that check if is_uvcvideo is set.

I'd rather not quirk v4l2-compliance. Vendors are starting to use it
to validate their hardware. If v4l2-compliance  complains(ce), then
there is a bigger chance that they will fix it.


If there is no objection I will send a v3 with Hans V. suggestion

Thanks!

>
> Regards,
>
>         Hans
>
> >
> > --
> > ---
> > 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

Powered by blists - more mailing lists