| Message ID | 20210326095840.364424-1-ribalda@chromium.org |
|---|---|
| Headers | show |
| Series | uvcvideo: Fix v4l2-compliance errors | expand |
On 26/03/2021 10:58, Ricardo Ribalda wrote: > The uvc driver relies on the camera firmware to keep the control states > and therefore is not capable of changing an inactive control. > > Allow returning -EACESS in those cases. -EACCES > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst | 5 +++++ > Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst | 5 +++++ > 2 files changed, 10 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst > index 4f1bed53fad5..8c0a203385c2 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst > @@ -95,3 +95,8 @@ EBUSY > > EACCES > Attempt to set a read-only control or to get a write-only control. > + > + Or if there is an attempt to set an inactive control and the driver is > + not capable of keeping the new value until the control is active again. keeping: 'caching' or 'storing' are better words, I think. > + This is the case for drivers that do not use the standard control > + framework and rely purely on the hardware to keep the controls' state. I would drop that last sentence. It is not relevant information to the users of the API. > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > index b9c62affbb5a..bb7de7a25241 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > @@ -438,3 +438,8 @@ EACCES > > Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the > device does not support requests. > + > + Or if there is an attempt to set an inactive control and the driver is > + not capable of keeping the new value until the control is active again. > + This is the case for drivers that do not use the standard control > + framework and rely purely on the hardware to keep the controls' state. Same comments as above. > Regards, Hans
On 26/03/2021 10:58, Ricardo Ribalda wrote: > Convert the error into a debug message, so they are still valid for > debugging but do not fill dmesg. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/usb/uvc/uvc_video.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index ea2903dc3252..b63c073ec30e 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -76,7 +76,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > if (likely(ret == size)) > return 0; > > - dev_err(&dev->udev->dev, > + dev_dbg(&dev->udev->dev, > "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > uvc_query_name(query), cs, unit, ret, size); > >
Hi Hans Thanks for your review! On Sat, Mar 27, 2021 at 12:19 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 26/03/2021 10:58, Ricardo Ribalda wrote: > > The uvc driver relies on the camera firmware to keep the control states > > and therefore is not capable of changing an inactive control. > > > > Allow returning -EACESS in those cases. > > -EACCES This british people that like to have a lot of double consonants :) I have updated the series at: https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/log/?h=uvc-compliance-v10 Will not post until there is more feedback to avoid spamming the list. Thanks :) > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst | 5 +++++ > > Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst | 5 +++++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst > > index 4f1bed53fad5..8c0a203385c2 100644 > > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst > > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst > > @@ -95,3 +95,8 @@ EBUSY > > > > EACCES > > Attempt to set a read-only control or to get a write-only control. > > + > > + Or if there is an attempt to set an inactive control and the driver is > > + not capable of keeping the new value until the control is active again. > > keeping: 'caching' or 'storing' are better words, I think. > > > + This is the case for drivers that do not use the standard control > > + framework and rely purely on the hardware to keep the controls' state. > > I would drop that last sentence. It is not relevant information to the users of > the API. > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > > index b9c62affbb5a..bb7de7a25241 100644 > > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > > @@ -438,3 +438,8 @@ EACCES > > > > Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the > > device does not support requests. > > + > > + Or if there is an attempt to set an inactive control and the driver is > > + not capable of keeping the new value until the control is active again. > > + This is the case for drivers that do not use the standard control > > + framework and rely purely on the hardware to keep the controls' state. > > Same comments as above. > > > > > Regards, > > Hans -- Ricardo Ribalda
On 27/03/2021 13:01, Ricardo Ribalda wrote: > Hi Hans > > Thanks for your review! > > On Sat, Mar 27, 2021 at 12:19 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> On 26/03/2021 10:58, Ricardo Ribalda wrote: >>> The uvc driver relies on the camera firmware to keep the control states >>> and therefore is not capable of changing an inactive control. >>> >>> Allow returning -EACESS in those cases. >> >> -EACCES > > This british people that like to have a lot of double consonants :) > > I have updated the series at: > > https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/log/?h=uvc-compliance-v10 For v10: Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Thanks! Hans > > Will not post until there is more feedback to avoid spamming the list. > > Thanks :) > >> >>> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >>> --- >>> Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst | 5 +++++ >>> Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst | 5 +++++ >>> 2 files changed, 10 insertions(+) >>> >>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst >>> index 4f1bed53fad5..8c0a203385c2 100644 >>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst >>> @@ -95,3 +95,8 @@ EBUSY >>> >>> EACCES >>> Attempt to set a read-only control or to get a write-only control. >>> + >>> + Or if there is an attempt to set an inactive control and the driver is >>> + not capable of keeping the new value until the control is active again. >> >> keeping: 'caching' or 'storing' are better words, I think. >> >>> + This is the case for drivers that do not use the standard control >>> + framework and rely purely on the hardware to keep the controls' state. >> >> I would drop that last sentence. It is not relevant information to the users of >> the API. >> >>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst >>> index b9c62affbb5a..bb7de7a25241 100644 >>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst >>> @@ -438,3 +438,8 @@ EACCES >>> >>> Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the >>> device does not support requests. >>> + >>> + Or if there is an attempt to set an inactive control and the driver is >>> + not capable of keeping the new value until the control is active again. >>> + This is the case for drivers that do not use the standard control >>> + framework and rely purely on the hardware to keep the controls' state. >> >> Same comments as above. >> >>> >> >> Regards, >> >> Hans > > >
Hi everyone, On Fri, Mar 26, 2021 at 6:58 PM Ricardo Ribalda <ribalda@chromium.org> wrote: > > *v4l2-compliance -m /dev/media0 -a -f > Total for uvcvideo device /dev/media0: 8, Succeeded: 6, Failed: 2, Warnings: 0 > Total for uvcvideo device /dev/video0: 54, Succeeded: 50, Failed: 4, Warnings: 2 > Total for uvcvideo device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0 > Grand Total for uvcvideo device /dev/media0: 108, Succeeded: 102, > Failed: 6, Warnings: 2 > > After fixing all of them we go down to: > > Total for uvcvideo device /dev/media0: 8, Succeeded: 8, Failed: 0, Warnings: 0 > Total for uvcvideo device /dev/video0: 54, Succeeded: 54, Failed: 0, Warnings: 0 > Total for uvcvideo device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0 > Grand Total for uvcvideo device /dev/media0: 108, Succeeded: 108, > Failed: 0, Warnings: 0 > > YES, NO MORE WARNINGS :) > > Note that we depend on: > > https://patchwork.linuxtv.org/project/linux-media/patch/20210317143453.483470-1-ribalda@chromium.org/ > > With Hans patch we can also pass v4l2-compliance -s. > > Changelog from v8 (Thanks to Hans) > - 3 patches from Hans > - Add Reviewed-by > > Hans Verkuil (4): > uvcvideo: uvc_ctrl_is_accessible: check for INACTIVE > uvcvideo: improve error handling in uvc_query_ctrl() > uvcvideo: don't spam the log in uvc_ctrl_restore_values() > uvc: use vb2 ioctl and fop helpers > > Ricardo Ribalda (18): > media: v4l2-ioctl: Fix check_ext_ctrls > media: pvrusb2: Do not check for V4L2_CTRL_WHICH_DEF_VAL > media: uvcvideo: Do not check for V4L2_CTRL_WHICH_DEF_VAL > media: v4l2-ioctl: S_CTRL output the right value > media: uvcvideo: Remove s_ctrl and g_ctrl > media: uvcvideo: Set capability in s_param > media: uvcvideo: Return -EIO for control errors > media: uvcvideo: refactor __uvc_ctrl_add_mapping > media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS > media: uvcvideo: Use dev->name for querycap() > media: uvcvideo: Set unique vdev name based in type > media: uvcvideo: Increase the size of UVC_METADATA_BUF_SIZE > media: uvcvideo: Use control names from framework > media: uvcvideo: Check controls flags before accessing them > media: uvcvideo: Set error_idx during ctrl_commit errors > media: uvcvideo: Return -EACCES to inactive controls > media: docs: Document the behaviour of uvcdriver > media: uvcvideo: Downgrade control error messages > > .../userspace-api/media/v4l/vidioc-g-ctrl.rst | 5 + > .../media/v4l/vidioc-g-ext-ctrls.rst | 5 + > drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 4 - > drivers/media/usb/uvc/uvc_ctrl.c | 343 +++++++++++---- > drivers/media/usb/uvc/uvc_driver.c | 22 +- > drivers/media/usb/uvc/uvc_metadata.c | 10 +- > drivers/media/usb/uvc/uvc_queue.c | 143 ------- > drivers/media/usb/uvc/uvc_v4l2.c | 389 +++--------------- > drivers/media/usb/uvc/uvc_video.c | 51 ++- > drivers/media/usb/uvc/uvcvideo.h | 54 +-- > drivers/media/v4l2-core/v4l2-ioctl.c | 67 +-- > 11 files changed, 444 insertions(+), 649 deletions(-) > > -- > 2.31.0.291.g576ba9dcdaf-goog > Any comments on this? Could we have this merged? Thanks, Tomasz
Hi Ricardo, Thank you for the patch. On Sat, Mar 27, 2021 at 01:01:05PM +0100, Ricardo Ribalda wrote: > On Sat, Mar 27, 2021 at 12:19 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 26/03/2021 10:58, Ricardo Ribalda wrote: > > > The uvc driver relies on the camera firmware to keep the control states > > > and therefore is not capable of changing an inactive control. > > > > > > Allow returning -EACESS in those cases. > > > > -EACCES > > This british people that like to have a lot of double consonants :) > > I have updated the series at: > > https://git.kernel.org/pub/scm/linux/kernel/git/ribalda/linux.git/log/?h=uvc-compliance-v10 > > Will not post until there is more feedback to avoid spamming the list. s/uvcdriver/uvcvideo driver/ in the subject line. For the version in that branch, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst | 5 +++++ > > > Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst | 5 +++++ > > > 2 files changed, 10 insertions(+) > > > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst > > > index 4f1bed53fad5..8c0a203385c2 100644 > > > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ctrl.rst > > > @@ -95,3 +95,8 @@ EBUSY > > > > > > EACCES > > > Attempt to set a read-only control or to get a write-only control. > > > + > > > + Or if there is an attempt to set an inactive control and the driver is > > > + not capable of keeping the new value until the control is active again. > > > > keeping: 'caching' or 'storing' are better words, I think. > > > > > + This is the case for drivers that do not use the standard control > > > + framework and rely purely on the hardware to keep the controls' state. > > > > I would drop that last sentence. It is not relevant information to the users of > > the API. > > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > > > index b9c62affbb5a..bb7de7a25241 100644 > > > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst > > > @@ -438,3 +438,8 @@ EACCES > > > > > > Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the > > > device does not support requests. > > > + > > > + Or if there is an attempt to set an inactive control and the driver is > > > + not capable of keeping the new value until the control is active again. > > > + This is the case for drivers that do not use the standard control > > > + framework and rely purely on the hardware to keep the controls' state. > > > > Same comments as above. -- Regards, Laurent Pinchart