| Message ID | 20241213-uvc-eaccess-v1-1-62e0b4fcc634@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Filter hw errors while enumerating controls | expand |
On Fri, 13 Dec 2024 at 12:21, Ricardo Ribalda <ribalda@chromium.org> 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. Actually it CANNOT be handled in userspace. If we return anything different than 0, the structure is not copied to userspace: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/v4l2-core/v4l2-ioctl.c#n2929 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/v4l2-core/v4l2-ioctl.c#n3490
Hi Ricardo, Thank you for the patch. 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@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. Was it transient and random errors, or does the device always fail for those controls ? > 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. > 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
On Wed, Dec 18, 2024 at 03:38:34PM +0100, Ricardo Ribalda wrote: > On Fri, 13 Dec 2024 at 12:21, Ricardo Ribalda <ribalda@chromium.org> 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. > > Actually it CANNOT be handled in userspace. > > If we return anything different than 0, the structure is not copied to > userspace: That could be fixed, we do copy data back to userspace in case of failure for some ioctls. I don't think that would be needed though, I believe we can either mark controls as broken in the uvcvideo driver through quirks, or in a dynamic fashion. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/v4l2-core/v4l2-ioctl.c#n2929 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/v4l2-core/v4l2-ioctl.c#n3490
On Thu, 19 Dec 2024 at 00:27, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > 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@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. > > 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. > > > 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). - 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... but we still need a solution for every camera that allows enumerating all the controls, even if there is a non compliant control. 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
On Thu, 19 Dec 2024 at 00:28, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Wed, Dec 18, 2024 at 03:38:34PM +0100, Ricardo Ribalda wrote: > > On Fri, 13 Dec 2024 at 12:21, Ricardo Ribalda <ribalda@chromium.org> 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. > > > > Actually it CANNOT be handled in userspace. > > > > If we return anything different than 0, the structure is not copied to > > userspace: > > That could be fixed, we do copy data back to userspace in case of > failure for some ioctls. I don't think that would be needed though, I > believe we can either mark controls as broken in the uvcvideo driver > through quirks, or in a dynamic fashion. I'd rather not introduce more differences between uvc and the rest of the drivers. the ctrl framework only seems to return -EINVAL or 0. > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/v4l2-core/v4l2-ioctl.c#n2929 > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/v4l2-core/v4l2-ioctl.c#n3490 > > -- > Regards, > > Laurent Pinchart
On Thu, Dec 19, 2024 at 09:18:30AM +0100, Ricardo Ribalda wrote: > On Thu, 19 Dec 2024 at 00:28, Laurent Pinchart wrote: > > On Wed, Dec 18, 2024 at 03:38:34PM +0100, Ricardo Ribalda wrote: > > > On Fri, 13 Dec 2024 at 12:21, 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. > > > > > > Actually it CANNOT be handled in userspace. > > > > > > If we return anything different than 0, the structure is not copied to > > > userspace: > > > > That could be fixed, we do copy data back to userspace in case of > > failure for some ioctls. I don't think that would be needed though, I > > believe we can either mark controls as broken in the uvcvideo driver > > through quirks, or in a dynamic fashion. > > I'd rather not introduce more differences between uvc and the rest of > the drivers. > > the ctrl framework only seems to return -EINVAL or 0. True, but that's likely because there's no driver today other than uvcvideo that needs to interogate the hardware in a way that can fail to implement QUERYCTRL. I wish we didn't have to, but UVC is special from that point of view. Anyway, as mentioned in a separate answer in this mail thread, we may not need this if we hide the problematic controls from applications. > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/v4l2-core/v4l2-ioctl.c#n2929 > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/v4l2-core/v4l2-ioctl.c#n3490
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@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. > > > > 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 ? 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... ? > 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. > 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
On Thu, 19 Dec 2024 at 15:41, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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@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. > > > > > > 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.... > > > 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@chromium.org/ 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. Regards! > > > 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
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@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. > > > > > > > > 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 ? > > > 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@chromium.org/ > > 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
On Thu, 19 Dec 2024 at 16:41, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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@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. > > > > > > > > > > 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@chromium.org/ > > > > 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
Hi, On 19-Dec-24 4:53 PM, Ricardo Ribalda wrote: > On Thu, 19 Dec 2024 at 16:41, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> 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@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. >>>>>> >>>>>> 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 Hmm this one looks like min and default are swapped ? So I guess this one needs a quirk which checks if default < min and in that case swaps them (the check is to avoid swapping with fixed fw). If these are built into chromebooks how about doing a fwupdate for the camera instead ? > "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 Regards, Hans
On Thu, 19 Dec 2024 at 17:05, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 19-Dec-24 4:53 PM, Ricardo Ribalda wrote: > > On Thu, 19 Dec 2024 at 16:41, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> 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@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. > >>>>>> > >>>>>> 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 > > Hmm this one looks like min and default are swapped ? > > So I guess this one needs a quirk which checks if default < min > and in that case swaps them (the check is to avoid swapping > with fixed fw). If these are built into chromebooks how about > doing a fwupdate for the camera instead ? We do fwupdate whenever possible. But some modules are not updateable. They either: lack DFU, or the flash is read-only, or the update process has a non acceptable fail rate. We aim to detect compliance errors early in the development process. V4L2-compliance now (almost) works with the uvcvideo driver. And that helps a lot :) I plan to add quirks for the cameras that I can test. But we still need a solution for all the external cameras and modules that are not in the lab. > > > "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 > > Regards, > > Hans > > >
Hi, On 19-Dec-24 5:18 PM, Ricardo Ribalda wrote: > On Thu, 19 Dec 2024 at 17:05, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 19-Dec-24 4:53 PM, Ricardo Ribalda wrote: >>> On Thu, 19 Dec 2024 at 16:41, Laurent Pinchart >>> <laurent.pinchart@ideasonboard.com> 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@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. >>>>>>>> >>>>>>>> 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 >> >> Hmm this one looks like min and default are swapped ? >> >> So I guess this one needs a quirk which checks if default < min >> and in that case swaps them (the check is to avoid swapping >> with fixed fw). If these are built into chromebooks how about >> doing a fwupdate for the camera instead ? > > We do fwupdate whenever possible. But some modules are not updateable. > They either: lack DFU, or the flash is read-only, or the update > process has a non acceptable fail rate. Ok, I was just wondering if we could avoid having to add a quirk for this model. > We aim to detect compliance errors early in the development process. > V4L2-compliance now (almost) works with the uvcvideo driver. And that > helps a lot :) > > I plan to add quirks for the cameras that I can test. But we still > need a solution for all the external cameras and modules that are not > in the lab. I agree we need some solution for this, especially the broken controls hiding others is a problem which needs some workaround. I'm not sure what that workaround should look like though. Just returning 0 as your v2 patch does seems less then ideal. Lets continue discussing this after the Christmas break. Regards, Hans >>> "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 >> >> Regards, >> >> Hans >> >> >> > >
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;
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@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. 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? 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(-) --- base-commit: a7f5b36b34824415c28875d615c49a3cf5070615 change-id: 20241213-uvc-eaccess-755cc061a360 Best regards,