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: 3
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: 3
With Hans patch we can also pass v4l2-compliance -s, but it is a WIP.
Changelog v3 (Thanks to Hans and Laurent)
- Return -EACCES on inactive controls
- Change unique name of the entities
- Increase metadata buffer size
Hans Verkuil (1):
uvc: use vb2 ioctl and fop helpers
Ricardo Ribalda (10):
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: 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: Return -EACCES to inactive controls
drivers/media/usb/uvc/uvc_ctrl.c | 154 +++++++++++++--
drivers/media/usb/uvc/uvc_driver.c | 22 ++-
drivers/media/usb/uvc/uvc_metadata.c | 8 +-
drivers/media/usb/uvc/uvc_queue.c | 131 -------------
drivers/media/usb/uvc/uvc_v4l2.c | 283 +++------------------------
drivers/media/usb/uvc/uvc_video.c | 5 +
drivers/media/usb/uvc/uvcvideo.h | 36 +---
drivers/media/v4l2-core/v4l2-ioctl.c | 25 ++-
8 files changed, 210 insertions(+), 454 deletions(-)
--
2.31.0.rc2.261.g7f71774620-goog
Drivers that do not use the ctrl-framework use this function instead.
- Return error when handling of REQUEST_VAL.
- Do not check for multiple classes when getting the DEF_VAL.
Fixes v4l2-compliance:
Control ioctls (Input 0):
fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)
test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
Cc: [email protected]
Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support")
Suggested-by: Hans Verkuil <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
drivers/media/v4l2-core/v4l2-ioctl.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 31d1342e61e8..9406e90ff805 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -917,15 +917,24 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)
for (i = 0; i < c->count; i++)
c->controls[i].reserved2[0] = 0;
- /* V4L2_CID_PRIVATE_BASE cannot be used as control class
- when using extended controls.
- Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL
- is it allowed for backwards compatibility.
- */
- if (!allow_priv && c->which == V4L2_CID_PRIVATE_BASE)
- return 0;
- if (!c->which)
+ switch (c->which) {
+ case V4L2_CID_PRIVATE_BASE:
+ /*
+ * V4L2_CID_PRIVATE_BASE cannot be used as control class
+ * when using extended controls.
+ * Only when passed in through VIDIOC_G_CTRL and VIDIOC_S_CTRL
+ * is it allowed for backwards compatibility.
+ */
+ if (!allow_priv)
+ return 0;
+ break;
+ case V4L2_CTRL_WHICH_DEF_VAL:
+ case V4L2_CTRL_WHICH_CUR_VAL:
return 1;
+ case V4L2_CTRL_WHICH_REQUEST_VAL:
+ return 0;
+ }
+
/* Check that all controls are from the same control class. */
for (i = 0; i < c->count; i++) {
if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) {
--
2.31.0.rc2.261.g7f71774620-goog
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 <[email protected]>
Reviewed-by: Hans Verkuil <[email protected]>
---
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;
}
}
--
2.31.0.rc2.261.g7f71774620-goog
Hans has discovered that in his test device, for the H264 format
bytesused goes up to about 570, for YUYV it will actually go up
to a bit over 5000 bytes, and for MJPG up to about 2706 bytes.
Credit-to: Hans Verkuil <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/usb/uvc/uvcvideo.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 1f17e4253673..91fc00ff311b 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -528,7 +528,7 @@ struct uvc_stats_stream {
unsigned int max_sof; /* Maximum STC.SOF value */
};
-#define UVC_METADATA_BUF_SIZE 1024
+#define UVC_METADATA_BUF_SIZE 10240
/**
* struct uvc_copy_op: Context structure to schedule asynchronous memcpy
--
2.31.0.rc2.261.g7f71774620-goog
Hi Ricardo, Laurent,
On 15/03/2021 18:36, Ricardo Ribalda wrote:
> Hans has discovered that in his test device, for the H264 format
> bytesused goes up to about 570, for YUYV it will actually go up
> to a bit over 5000 bytes, and for MJPG up to about 2706 bytes.
>
> Credit-to: Hans Verkuil <[email protected]>
> Signed-off-by: Ricardo Ribalda <[email protected]>
> ---
> drivers/media/usb/uvc/uvcvideo.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 1f17e4253673..91fc00ff311b 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -528,7 +528,7 @@ struct uvc_stats_stream {
> unsigned int max_sof; /* Maximum STC.SOF value */
> };
>
> -#define UVC_METADATA_BUF_SIZE 1024
> +#define UVC_METADATA_BUF_SIZE 10240
>
> /**
> * struct uvc_copy_op: Context structure to schedule asynchronous memcpy
>
I've been doing some tests here, and this is tricky.
I think the core bug is in uvc_video_decode_meta():
if (meta_buf->length - meta_buf->bytesused <
length + sizeof(meta->ns) + sizeof(meta->sof)) {
meta_buf->error = 1;
return;
}
This checks for a buffer overflow and by setting meta_buf->error to 1 it causes
the whole buffer to be discarded. But according to the V4L2_META_FMT_UVC docs here
https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/pixfmt-meta-uvc.html
it should "drop headers when the buffer is full", which suggests to me that
the 'meta_buf->error = 1;' is wrong and should be removed.
Looking at the number of headers I receive for various frame sizes and frame rates
when choosing YUYV as the pixelformat I see that the frame rate is the main input
to that: I get (very roughly) one header for every 150 microseconds. So that's
roughly 667 headers of 22 bytes for a 10 fps capture. The frame size has some
effect, but it is fairly small. This means that for slow captures (i.e. 2 fps)
you need about 75000 bytes, let's round it up to 102400.
I did tests with 1920x1080 at 5 fps for YUYV, H264 and MJPEG and saw the following:
Format Video Size Metadata Size
YUYV 4147200 29964
MJPG 130000 3608
H264 (P-frame) 70000 2600
H264 (I-frame) 150000 5500
The difference here is most likely due to YUYV being transmitted over time as
video lines arrive, so it is spread out over time, while H264 and MJPG are
bursty, i.e. the whole compressed frame is transferred in one go.
I think that 10240 is a good value for the metadata buffers since it is enough
for the worst-case for the compressed formats, and that if this is combined
with removing the 'meta_buf->error = 1;' it will also do its job for YUYV
even though data will be dropped, but that's better than not getting anything
at all.
Regards,
Hans
On 15/03/2021 18:36, Ricardo Ribalda wrote:
> 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 <[email protected]>
> Reviewed-by: Hans Verkuil <[email protected]>
> ---
> 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;
This isn't right.
For G_EXT_CTRLS error_idx should be set to either ctrls->count or the index of the
failing control depending on whether the hardware has been touched or not.
In the case of the 'if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {' the hardware
has not been touched, so there is should set error_idx to ctrls->count.
In the case where we obtain the actual values with uvc_ctrl_get() it must return
the index of the failing control.
According to the spec if VIDIOC_G_EXT_CTRLS returns an error and error_idx is equal
to the total count, then no hardware was touched, so it was during the validation
phase that some error was detected. If it returns an error and error_idx < count,
then the hardware was accessed and the contents of the controls at indices >= error_idx
is undefined.
So setting error_idx to i is the right thing to do, regardless of the EACCES test.
This does create a problem in v4l2-compliance, since it assumes that the control
framework is used, and that framework validates the control first in a separate step
before accessing the hardware. That's missing in uvc. I think v4l2-compliance should
be adjusted for uvcvideo since uvc isn't doing anything illegal by returning i here
since it really accessed hardware.
An alternative would be to introduce an initial validation phase in uvc_ioctl_g_ext_ctrls
as well, but I'm not sure that it worth the effort. It's quite difficult to get it really
right.
Relaxing the tests in v4l2-compliance for uvc is a better approach IMHO.
Or rewrite uvc to use the control framework :-) :-)
Regards,
Hans
> return ret;
> }
> }
>