| Message ID | 20210311221946.1319924-1-ribalda@chromium.org |
|---|---|
| Headers | show |
| Series | uvcvideo: Fix v4l2-compliance errors | expand |
Hi Ricardo, Thank you for the patch. On Thu, Mar 11, 2021 at 11:19:46PM +0100, Ricardo Ribalda wrote: > All the entities must have a unique name. > > Fixes v4l2-compliance: > Media Controller ioctls: > fail: v4l2-test-media.cpp(205): v2_entity_names_set.find(key) != v2_entity_names_set.end() > test MEDIA_IOC_G_TOPOLOGY: FAIL > fail: v4l2-test-media.cpp(394): num_data_links != num_links > test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_driver.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 30ef2a3110f7..47efa9a9be99 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -2199,7 +2199,10 @@ int uvc_register_video_device(struct uvc_device *dev, > break; > } > > - strscpy(vdev->name, dev->name, sizeof(vdev->name)); > + if (type == V4L2_BUF_TYPE_META_CAPTURE) > + strscpy(vdev->name, "Metadata Videodev", sizeof(vdev->name)); > + else > + strscpy(vdev->name, dev->name, sizeof(vdev->name)); A UVC device could contain multiple output terminals (either in the same chain or in different chains), which would still result in multiple entities having the same name. Could this be fixed at the same time ? You can use the unit ID of the output terminal to create unique names (and it would be nice if the video and metadata nodes has similar names, with "video" and "metadata" being the only difference between them). > > /* > * Set the driver data before calling video_register_device, otherwise
Hi Ricardo, Thank you for the patch. On Thu, Mar 11, 2021 at 11:19:44PM +0100, Ricardo Ribalda wrote: > According to the doc: > The, in hindsight quite poor, solution for that is to set error_idx to > count if the validation failed. > > Fixes v4l2-compliance: > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(645): invalid error index write only control > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> I'll hold off on this one until we conclude the discussion with Hans on v1. > --- > drivers/media/usb/uvc/uvc_v4l2.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 157310c0ca87..36eb48622d48 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -1073,7 +1073,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh, > ret = uvc_ctrl_get(chain, ctrl); > if (ret < 0) { > uvc_ctrl_rollback(handle); > - ctrls->error_idx = i; > + ctrls->error_idx = (ret == -EACCES) ? > + ctrls->count : i; > return ret; > } > }
On 11/03/2021 23:19, Ricardo Ribalda wrote: > Fixes v4l2-compliance: > > Format ioctls (Input 0): > warn: v4l2-test-formats.cpp(1339): S_PARM is supported but doesn't report V4L2_CAP_TIMEPERFRAME > fail: v4l2-test-formats.cpp(1241): node->has_frmintervals && !cap->capability > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Thanks! Hans > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_v4l2.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 252136cc885c..157310c0ca87 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -472,10 +472,13 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream, > uvc_simplify_fraction(&timeperframe.numerator, > &timeperframe.denominator, 8, 333); > > - if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) > + if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) { > parm->parm.capture.timeperframe = timeperframe; > - else > + parm->parm.capture.capability = V4L2_CAP_TIMEPERFRAME; > + } else { > parm->parm.output.timeperframe = timeperframe; > + parm->parm.output.capability = V4L2_CAP_TIMEPERFRAME; > + } > > return 0; > } >
On 11/03/2021 23:19, Ricardo Ribalda wrote: > According to the doc: > The, in hindsight quite poor, solution for that is to set error_idx to > count if the validation failed. I think this needs a bit more explanation. How about this: "If an error is found when validating the list of controls passed with VIDIOC_G_EXT_CTRLS, then error_idx shall be set to ctrls->count to indicate to userspace that no actual hardware was touched. It would have been much nicer of course if error_idx could point to the control index that failed the validation, but sadly that's not how the API was designed." > > Fixes v4l2-compliance: > Control ioctls (Input 0): > fail: v4l2-test-controls.cpp(645): invalid error index write only control > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> After improving the commit log you can add my: Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Thanks! Hans > --- > drivers/media/usb/uvc/uvc_v4l2.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 157310c0ca87..36eb48622d48 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -1073,7 +1073,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh, > ret = uvc_ctrl_get(chain, ctrl); > if (ret < 0) { > uvc_ctrl_rollback(handle); > - ctrls->error_idx = i; > + ctrls->error_idx = (ret == -EACCES) ? > + ctrls->count : i; > return ret; > } > } >
On 12/03/2021 00:38, Laurent Pinchart wrote: > Hi Ricardo, > > Thank you for the patch. > > On Thu, Mar 11, 2021 at 11:19:46PM +0100, Ricardo Ribalda wrote: >> All the entities must have a unique name. >> >> Fixes v4l2-compliance: >> Media Controller ioctls: >> fail: v4l2-test-media.cpp(205): v2_entity_names_set.find(key) != v2_entity_names_set.end() >> test MEDIA_IOC_G_TOPOLOGY: FAIL >> fail: v4l2-test-media.cpp(394): num_data_links != num_links >> test MEDIA_IOC_ENUM_ENTITIES/LINKS: FAIL >> >> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >> --- >> drivers/media/usb/uvc/uvc_driver.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c >> index 30ef2a3110f7..47efa9a9be99 100644 >> --- a/drivers/media/usb/uvc/uvc_driver.c >> +++ b/drivers/media/usb/uvc/uvc_driver.c >> @@ -2199,7 +2199,10 @@ int uvc_register_video_device(struct uvc_device *dev, >> break; >> } >> >> - strscpy(vdev->name, dev->name, sizeof(vdev->name)); >> + if (type == V4L2_BUF_TYPE_META_CAPTURE) >> + strscpy(vdev->name, "Metadata Videodev", sizeof(vdev->name)); >> + else >> + strscpy(vdev->name, dev->name, sizeof(vdev->name)); > > A UVC device could contain multiple output terminals (either in the same > chain or in different chains), which would still result in multiple > entities having the same name. Could this be fixed at the same time ? > You can use the unit ID of the output terminal to create unique names > (and it would be nice if the video and metadata nodes has similar names, > with "video" and "metadata" being the only difference between them). I agree with Laurent. How about using something like this for the videodevs: snprintf(vdev->name, sizeof(vdev->name), "Meta %s", dev->name); and: snprintf(vdev->name, sizeof(vdev->name), "Video %s", dev->name); Regards, Hans > >> >> /* >> * Set the driver data before calling video_register_device, otherwise >
Hi Hans, On Fri, Mar 12, 2021 at 08:14:13AM +0100, Hans Verkuil wrote: > On 11/03/2021 23:19, Ricardo Ribalda wrote: > > According to the doc: > > The, in hindsight quite poor, solution for that is to set error_idx to > > count if the validation failed. > > I think this needs a bit more explanation. How about this: > > "If an error is found when validating the list of controls passed with > VIDIOC_G_EXT_CTRLS, then error_idx shall be set to ctrls->count to indicate > to userspace that no actual hardware was touched. > > It would have been much nicer of course if error_idx could point to the > control index that failed the validation, but sadly that's not how the > API was designed." Here's what I commented on v1: The [V4L2 spec] states: "This check is done to avoid leaving the hardware in an inconsistent state due to easy-to-avoid problems. But it leads to another problem: the application needs to know whether an error came from the validation step (meaning that the hardware was not touched) or from an error during the actual reading from/writing to hardware." I'm not sure this is correct though. -EACCES is returned by __uvc_ctrl_get() when the control is found and is a write-only control. The uvc_ctrl_get() calls for the previous controls will have potentially touched the device to read the current control value if it wasn't cached already, to this contradicts the rationale from the specification. I understand the need for this when setting controls, but when reading them, it's more puzzling, as the interactions with the hardware to read the controls are not supposed to affect the hardware state in a way that applications should care about. It may be an issue in the V4L2 specification. > > > > Fixes v4l2-compliance: > > Control ioctls (Input 0): > > fail: v4l2-test-controls.cpp(645): invalid error index write only control > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > After improving the commit log you can add my: > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > > --- > > drivers/media/usb/uvc/uvc_v4l2.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > index 157310c0ca87..36eb48622d48 100644 > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > @@ -1073,7 +1073,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh, > > ret = uvc_ctrl_get(chain, ctrl); > > if (ret < 0) { > > uvc_ctrl_rollback(handle); > > - ctrls->error_idx = i; > > + ctrls->error_idx = (ret == -EACCES) ? > > + ctrls->count : i; > > return ret; > > } > > } -- Regards, Laurent Pinchart
In my computer I am getting this output for 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: 9 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: 9 We are still not compliant with v4l2-compliance -s: Streaming ioctls: test read/write: OK (Not Supported) test blocking wait: OK fail: v4l2-test-buffers.cpp(1265): node->streamon(q.g_type()) != EINVAL test MMAP (no poll): FAIL fail: v4l2-test-buffers.cpp(1265): node->streamon(q.g_type()) != EINVAL test MMAP (select): FAIL fail: v4l2-test-buffers.cpp(1265): node->streamon(q.g_type()) != EINVAL test MMAP (epoll): FAIL But fixing that will probably require a lot of changes in the driver that are already implemented in the vb2 helpers. It is better to continue Hans work on that: https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=uvc-4.19&id=a6a0a05f643521d29a4c1e551b0be73ce66b7108 Changelog v2 (Thanks to Hans and Laurent) - Reimplement the CTRL_CLASS as a patch on queryctl - Do not return -EIO for case 8 - Handle request bug and which_def multiclass in core Ricardo Ribalda (6): media: v4l2-ioctl: Fix check_ext_ctrls media: uvcvideo: Set capability in s_param media: uvcvideo: Return -EIO for control errors media: uvcvideo: set error_idx to count on EACCESS media: uvcvideo: Add support for V4L2_CTRL_TYPE_CTRL_CLASS media: uvcvideo: Set a different name for the metadata entity drivers/media/usb/uvc/uvc_ctrl.c | 90 ++++++++++++++++++++++++++++ drivers/media/usb/uvc/uvc_driver.c | 5 +- drivers/media/usb/uvc/uvc_v4l2.c | 10 +++- drivers/media/usb/uvc/uvc_video.c | 5 ++ drivers/media/usb/uvc/uvcvideo.h | 7 +++ drivers/media/v4l2-core/v4l2-ioctl.c | 4 +- 6 files changed, 116 insertions(+), 5 deletions(-)