On Thu, 19 Dec 2024 at 16:41, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > On Thu, Dec 19, 2024 at 04:35:37PM +0100, Ricardo Ribalda wrote: > > On Thu, 19 Dec 2024 at 15:41, Laurent Pinchart wrote: > > > On Thu, Dec 19, 2024 at 09:17:31AM +0100, Ricardo Ribalda wrote: > > > > On Thu, 19 Dec 2024 at 00:27, Laurent Pinchart wrote: > > > > > On Fri, Dec 13, 2024 at 11:21:02AM +0000, Ricardo Ribalda wrote: > > > > > > To implement VIDIOC_QUERYCTRL, we need to read from the hardware all the > > > > > > values that were not cached previously. If that read fails, we used to > > > > > > report back the error to the user. > > > > > > > > > > > > Unfortunately this does not play nice with userspace. When they are > > > > > > enumerating the contols, the only expect an error when there are no > > > > > > "next" control. > > > > > > > > > > > > This is probably a corner case, and could be handled in userspace, but > > > > > > both v4l2-ctl and yavta fail to enumerate all the controls if we return > > > > > > then -EIO during VIDIOC_QUERYCTRL. I suspect that there are tons of > > > > > > userspace apps handling this wrongly as well. > > > > > > > > > > > > This patch get around this issue by ignoring the hardware errors and > > > > > > always returning 0 if a control exists. > > > > > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> > > > > > > --- > > > > > > 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. > > > > > > > > > > Was it transient and random errors, or does the device always fail for > > > > > those controls ? > > > > > > > > For one of the devices the control is always failing (or I could not > > > > find a combination that made it work). > > > > > > > > For the other it was more or less random. > > > > > > Are there other controls that failed for that device ? And what > > > request(s) fail, is it only GET_CUR or also GET_MIN/GET_MAX/GET_RES ? > > > > It is a mix. > > > > > What's the approximate frequency of those random failures ? > > > > > > > > > This could be solved in userspace.. but I suspect that a lot of people > > > > > > has copied the implementation of v4l-utils or yavta. > > > > > > > > > > > > What do you think of this workaround? > > > > > > > > > > Pretending that the control could be queried is problematic. We'll > > > > > return invalid values to the user, I don't think that's a good idea. If > > > > > the problematic device always returns error for focus controls, we could > > > > > add a quirk, and extend the uvc_device_info structure to list the > > > > > controls to ignore. > > > > > > > > > > Another option would be to skip over controls that return -EIO within > > > > > the kernel, and mark those controls are broken. I think this could be > > > > > done transparently for userspace, the first time we try to populate the > > > > > cache for such controls, a -EIO error would mark the control as broken, > > > > > and from a userspace point of view it wouldn't be visible through as > > > > > ioctl. > > > > > > > > I see a couple of issues with this: > > > > - There are controls that fail randomly. > > > > - There are controls that fail based on the value of other controls > > > > (yeah, I know). > > > > > > I was fearing there would be random (or random-looking) failures, as > > > that can preclude marking the controls as broken and fully hiding them > > > from userspace :-( > > > > > > > - There are controls that do not implement RES, MIN, or MAX, but > > > > besides that, they are completely functional. > > > > In any of those cases we do not want to skip those controls. > > > > > > > > I am not against quirking specific cameras once we detect that they > > > > are broken... > > > > > > Hopefully there won't be too many of those, right ? Righhhht... ? > > > > So far I have identified 4 in a week, and I am not testing obscure > > camera modules.... > > Can you provide more information about those modules ? USB descriptors > maybe, and the list of controls that fail, and how they fail ? These are the ones I can share now: "13d3:5519": Focus value out of range focus_absolute 0x009a090a (int) : min=355 max=790 step=1 default=6 value=500 flags=inactive "3277:0003": Focus returns -EIO Focus Absolute and Focus, Automatic Continuous: return -EIO for at least one of get_ max/min/res "0408:302f": Error reading AutoExposure Flags UVC_GET_INFO returns invalid flags > > > > > but we still need a solution for every camera that > > > > allows enumerating all the controls, even if there is a non compliant > > > > control. > > > > > > I'm quite concerned by this. Faking success in QUERYCTRL when we don't > > > know the actual control min/max values is really bad. For transient > > > failures, we may work around with a retry. For devices that fail all the > > > time, we can blacklist the controls with quirks. For other devices... > > > you know, at some point we need to get the hardware shipped back to the > > > manufacturer. > > > > What I have seen is that a simple retry will always fail. In some > > modules you can "fix" the control by changing the value of other > > controls or changing the format. > > > > If we look at the big picture, this is not worse than a control that > > returns invalid min/max/res. We already have plenty of those and in > > those cases we return 0... > > In my opinion an QUERYCTRL with invalid values is much better than > > hidding valid controls. > > > > I would like to support as much hardware as possible, even the broken one. > > > > I believe that we should either apply a patch like this : > > https://lore.kernel.org/linux-media/20241219-uvc-eaccess-v2-1-bf6520c8b86d@xxxxxxxxxxxx/ > > > > or we should copy the content of v4l2_queryctrl to userspace when an > > error happens, and modify v4l2-ctl and yavta. I do not know what Hans > > thinks about this. > > Let's see what Hans thinks. > > > > > Now that I look into the patch... I think I have to remove the > > > > can_fail argument and let it always remove invalid values. > > > > > > > > > > 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 > > > > > > > > > > > > -- > > > > > > --- > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 31 ++++++++++++++++++++++--------- > > > > > > 1 file changed, 22 insertions(+), 9 deletions(-) > > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > > > > index 4fe26e82e3d1..a11048c9a105 100644 > > > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > > > > @@ -1283,7 +1283,8 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl, > > > > > > static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > > > > > struct uvc_control *ctrl, > > > > > > struct uvc_control_mapping *mapping, > > > > > > - struct v4l2_queryctrl *v4l2_ctrl) > > > > > > + struct v4l2_queryctrl *v4l2_ctrl, > > > > > > + bool can_fail) > > > > > > { > > > > > > struct uvc_control_mapping *master_map = NULL; > > > > > > struct uvc_control *master_ctrl = NULL; > > > > > > @@ -1307,17 +1308,28 @@ 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); > > > > > > + if (can_fail) > > > > > > + return ret; > > > > > > + } 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 (can_fail) > > > > > > + return ret; > > > > > > + } > > > > > > } > > > > > > > > > > > > if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) { > > > > > > @@ -1420,7 +1432,8 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, > > > > > > goto done; > > > > > > } > > > > > > > > > > > > - ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl); > > > > > > + ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl, > > > > > > + !(v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL)); > > > > > > done: > > > > > > mutex_unlock(&chain->ctrl_mutex); > > > > > > return ret; > > > > > > @@ -1513,7 +1526,7 @@ static void uvc_ctrl_fill_event(struct uvc_video_chain *chain, > > > > > > { > > > > > > struct v4l2_queryctrl v4l2_ctrl; > > > > > > > > > > > > - __uvc_query_v4l2_ctrl(chain, ctrl, mapping, &v4l2_ctrl); > > > > > > + __uvc_query_v4l2_ctrl(chain, ctrl, mapping, &v4l2_ctrl, true); > > > > > > > > > > > > memset(ev, 0, sizeof(*ev)); > > > > > > ev->type = V4L2_EVENT_CTRL; > > > > > > > > > > > > --- > > > > > > base-commit: a7f5b36b34824415c28875d615c49a3cf5070615 > > > > > > change-id: 20241213-uvc-eaccess-755cc061a360 > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda
