| Message ID | 20210317120812.292261-1-ribalda@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] v4l2-compliance: Let uvcvideo return -EACCES | expand |
On 17/03/2021 13:08, Ricardo Ribalda wrote: > Setting a control while inactive is meant to work, but it might > not be actually written to the hardware until control becomes active. > > v4l2-compliance should allow -EACCES as an error code, but only for > the uvcdriver when an attempt is made to set inactive controls. > > The control framework is able to handle this case more elegantly: > it will remember the last set inactive value, and when the control > becomes active it will update the hardware. But that's really hard > to model in uvc. > > Suggested-by: Hans Verkuil <hverkuil@xs4all.nl> > Signed-off-by: Ricardo Ribalda <ricardo@ribalda.com> > --- > > Changelog v2: > - Also fix MENU and INTEGER_MENU > > utils/v4l2-compliance/v4l2-compliance.cpp | 2 ++ > utils/v4l2-compliance/v4l2-compliance.h | 1 + > utils/v4l2-compliance/v4l2-test-controls.cpp | 23 +++++++++++++++----- > 3 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp > index 9f71332c..1c21197b 100644 > --- a/utils/v4l2-compliance/v4l2-compliance.cpp > +++ b/utils/v4l2-compliance/v4l2-compliance.cpp > @@ -84,6 +84,7 @@ bool show_colors; > bool exit_on_fail; > bool exit_on_warn; > bool is_vivid; > +bool is_uvcvideo; > int media_fd = -1; > unsigned warnings; > > @@ -958,6 +959,7 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ > if (node.is_v4l2()) { > doioctl(&node, VIDIOC_QUERYCAP, &vcap); > driver = reinterpret_cast<const char *>(vcap.driver); > + is_uvcvideo = driver == "uvcvideo"; > is_vivid = driver == "vivid"; > if (is_vivid) > node.bus_info = reinterpret_cast<const char *>(vcap.bus_info); > diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h > index 4d5c3a5c..db4790a6 100644 > --- a/utils/v4l2-compliance/v4l2-compliance.h > +++ b/utils/v4l2-compliance/v4l2-compliance.h > @@ -50,6 +50,7 @@ extern bool no_progress; > extern bool exit_on_fail; > extern bool exit_on_warn; > extern bool is_vivid; // We're testing the vivid driver > +extern bool is_uvcvideo; // We're testing the uvc driver > extern int kernel_version; > extern int media_fd; > extern unsigned warnings; > diff --git a/utils/v4l2-compliance/v4l2-test-controls.cpp b/utils/v4l2-compliance/v4l2-test-controls.cpp > index 4be2f61c..511a76a5 100644 > --- a/utils/v4l2-compliance/v4l2-test-controls.cpp > +++ b/utils/v4l2-compliance/v4l2-test-controls.cpp > @@ -485,6 +485,8 @@ int testSimpleControls(struct node *node) > } else if (ret == EILSEQ) { > warn("s_ctrl returned EILSEQ\n"); > ret = 0; > + } else if (ret == EACCES && is_uvcvideo) { > + ret = 0; > } else if (ret) { > return fail("s_ctrl returned an error (%d)\n", ret); > } > @@ -498,7 +500,8 @@ int testSimpleControls(struct node *node) > ctrl.id = qctrl.id; > ctrl.value = qctrl.minimum - 1; > ret = doioctl(node, VIDIOC_S_CTRL, &ctrl); > - if (ret && ret != EIO && ret != EILSEQ && ret != ERANGE) > + if (ret && ret != EIO && ret != EILSEQ && ret != ERANGE && > + !(ret == EACCES && is_uvcvideo)) > return fail("invalid minimum range check\n"); > if (!ret && checkSimpleCtrl(ctrl, qctrl)) > return fail("invalid control %08x\n", qctrl.id); > @@ -508,7 +511,8 @@ int testSimpleControls(struct node *node) > ctrl.id = qctrl.id; > ctrl.value = qctrl.maximum + 1; > ret = doioctl(node, VIDIOC_S_CTRL, &ctrl); > - if (ret && ret != EIO && ret != EILSEQ && ret != ERANGE) > + if (ret && ret != EIO && ret != EILSEQ && ret != ERANGE && > + !(ret == EACCES && is_uvcvideo)) > return fail("invalid maximum range check\n"); > if (!ret && checkSimpleCtrl(ctrl, qctrl)) > return fail("invalid control %08x\n", qctrl.id); You missed updating this: For the 'if (qctrl.step > 1 && qctrl.maximum > qctrl.minimum) {' section an EACCES check is also needed (it fails there for my Logitech webcam). So: if (ret == ERANGE) warn("%s: returns ERANGE for in-range, but non-step-multiple value\n", qctrl.name); else if (ret && ret != EIO && ret != EILSEQ) return fail("returns error for in-range, but non-step-multiple value\n"); should be: if (ret == ERANGE) warn("%s: returns ERANGE for in-range, but non-step-multiple value\n", qctrl.name); else if (ret && ret != EIO && ret != EILSEQ && !(ret == EACCES && is_uvcvideo)) return fail("returns error for in-range, but non-step-multiple value\n"); Regards, Hans > @@ -539,6 +543,8 @@ int testSimpleControls(struct node *node) > ctrl.id = qctrl.id; > ctrl.value = i; > ret = doioctl(node, VIDIOC_S_CTRL, &ctrl); > + if (valid && ret == EACCES && is_uvcvideo) > + continue; > if (valid && ret) > return fail("could not set valid menu item %d\n", i); > if (!valid && !ret) > @@ -551,15 +557,18 @@ int testSimpleControls(struct node *node) > ctrl.id = qctrl.id; > ctrl.value = qctrl.minimum; > ret = doioctl(node, VIDIOC_S_CTRL, &ctrl); > - if (ret && ret != EIO && ret != EILSEQ) > + if (ret && ret != EIO && ret != EILSEQ && > + !(ret == EACCES && is_uvcvideo)) > return fail("could not set minimum value\n"); > ctrl.value = qctrl.maximum; > ret = doioctl(node, VIDIOC_S_CTRL, &ctrl); > - if (ret && ret != EIO && ret != EILSEQ) > + if (ret && ret != EIO && ret != EILSEQ && > + !(ret == EACCES && is_uvcvideo)) > return fail("could not set maximum value\n"); > ctrl.value = qctrl.default_value; > ret = doioctl(node, VIDIOC_S_CTRL, &ctrl); > - if (ret && ret != EIO && ret != EILSEQ) > + if (ret && ret != EIO && ret != EILSEQ && > + !(ret == EACCES && is_uvcvideo)) > return fail("could not set default value\n"); > } > } > @@ -731,6 +740,8 @@ int testExtendedControls(struct node *node) > } else if (ret == EILSEQ) { > warn("s_ext_ctrls returned EILSEQ\n"); > ret = 0; > + } else if (ret == EACCES && is_uvcvideo) { > + ret = 0; > } > if (ret) > return fail("s_ext_ctrls returned an error (%d)\n", ret); > @@ -806,6 +817,8 @@ int testExtendedControls(struct node *node) > } else if (ret == EILSEQ) { > warn("s_ext_ctrls returned EILSEQ\n"); > ret = 0; > + } else if (ret == EACCES && is_uvcvideo) { > + ret = 0; > } > if (ret) > return fail("could not set all controls\n"); >
diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp index 9f71332c..1c21197b 100644 --- a/utils/v4l2-compliance/v4l2-compliance.cpp +++ b/utils/v4l2-compliance/v4l2-compliance.cpp @@ -84,6 +84,7 @@ bool show_colors; bool exit_on_fail; bool exit_on_warn; bool is_vivid; +bool is_uvcvideo; int media_fd = -1; unsigned warnings; @@ -958,6 +959,7 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ if (node.is_v4l2()) { doioctl(&node, VIDIOC_QUERYCAP, &vcap); driver = reinterpret_cast<const char *>(vcap.driver); + is_uvcvideo = driver == "uvcvideo"; is_vivid = driver == "vivid"; if (is_vivid) node.bus_info = reinterpret_cast<const char *>(vcap.bus_info); diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h index 4d5c3a5c..db4790a6 100644 --- a/utils/v4l2-compliance/v4l2-compliance.h +++ b/utils/v4l2-compliance/v4l2-compliance.h @@ -50,6 +50,7 @@ extern bool no_progress; extern bool exit_on_fail; extern bool exit_on_warn; extern bool is_vivid; // We're testing the vivid driver +extern bool is_uvcvideo; // We're testing the uvc driver extern int kernel_version; extern int media_fd; extern unsigned warnings; diff --git a/utils/v4l2-compliance/v4l2-test-controls.cpp b/utils/v4l2-compliance/v4l2-test-controls.cpp index 4be2f61c..511a76a5 100644 --- a/utils/v4l2-compliance/v4l2-test-controls.cpp +++ b/utils/v4l2-compliance/v4l2-test-controls.cpp @@ -485,6 +485,8 @@ int testSimpleControls(struct node *node) } else if (ret == EILSEQ) { warn("s_ctrl returned EILSEQ\n"); ret = 0; + } else if (ret == EACCES && is_uvcvideo) { + ret = 0; } else if (ret) { return fail("s_ctrl returned an error (%d)\n", ret); } @@ -498,7 +500,8 @@ int testSimpleControls(struct node *node) ctrl.id = qctrl.id; ctrl.value = qctrl.minimum - 1; ret = doioctl(node, VIDIOC_S_CTRL, &ctrl); - if (ret && ret != EIO && ret != EILSEQ && ret != ERANGE) + if (ret && ret != EIO && ret != EILSEQ && ret != ERANGE && + !(ret == EACCES && is_uvcvideo)) return fail("invalid minimum range check\n"); if (!ret && checkSimpleCtrl(ctrl, qctrl)) return fail("invalid control %08x\n", qctrl.id); @@ -508,7 +511,8 @@ int testSimpleControls(struct node *node) ctrl.id = qctrl.id; ctrl.value = qctrl.maximum + 1; ret = doioctl(node, VIDIOC_S_CTRL, &ctrl); - if (ret && ret != EIO && ret != EILSEQ && ret != ERANGE) + if (ret && ret != EIO && ret != EILSEQ && ret != ERANGE && + !(ret == EACCES && is_uvcvideo)) return fail("invalid maximum range check\n"); if (!ret && checkSimpleCtrl(ctrl, qctrl)) return fail("invalid control %08x\n", qctrl.id); @@ -539,6 +543,8 @@ int testSimpleControls(struct node *node) ctrl.id = qctrl.id; ctrl.value = i; ret = doioctl(node, VIDIOC_S_CTRL, &ctrl); + if (valid && ret == EACCES && is_uvcvideo) + continue; if (valid && ret) return fail("could not set valid menu item %d\n", i); if (!valid && !ret) @@ -551,15 +557,18 @@ int testSimpleControls(struct node *node) ctrl.id = qctrl.id; ctrl.value = qctrl.minimum; ret = doioctl(node, VIDIOC_S_CTRL, &ctrl); - if (ret && ret != EIO && ret != EILSEQ) + if (ret && ret != EIO && ret != EILSEQ && + !(ret == EACCES && is_uvcvideo)) return fail("could not set minimum value\n"); ctrl.value = qctrl.maximum; ret = doioctl(node, VIDIOC_S_CTRL, &ctrl); - if (ret && ret != EIO && ret != EILSEQ) + if (ret && ret != EIO && ret != EILSEQ && + !(ret == EACCES && is_uvcvideo)) return fail("could not set maximum value\n"); ctrl.value = qctrl.default_value; ret = doioctl(node, VIDIOC_S_CTRL, &ctrl); - if (ret && ret != EIO && ret != EILSEQ) + if (ret && ret != EIO && ret != EILSEQ && + !(ret == EACCES && is_uvcvideo)) return fail("could not set default value\n"); } } @@ -731,6 +740,8 @@ int testExtendedControls(struct node *node) } else if (ret == EILSEQ) { warn("s_ext_ctrls returned EILSEQ\n"); ret = 0; + } else if (ret == EACCES && is_uvcvideo) { + ret = 0; } if (ret) return fail("s_ext_ctrls returned an error (%d)\n", ret); @@ -806,6 +817,8 @@ int testExtendedControls(struct node *node) } else if (ret == EILSEQ) { warn("s_ext_ctrls returned EILSEQ\n"); ret = 0; + } else if (ret == EACCES && is_uvcvideo) { + ret = 0; } if (ret) return fail("could not set all controls\n");
Setting a control while inactive is meant to work, but it might not be actually written to the hardware until control becomes active. v4l2-compliance should allow -EACCES as an error code, but only for the uvcdriver when an attempt is made to set inactive controls. The control framework is able to handle this case more elegantly: it will remember the last set inactive value, and when the control becomes active it will update the hardware. But that's really hard to model in uvc. Suggested-by: Hans Verkuil <hverkuil@xs4all.nl> Signed-off-by: Ricardo Ribalda <ricardo@ribalda.com> --- Changelog v2: - Also fix MENU and INTEGER_MENU utils/v4l2-compliance/v4l2-compliance.cpp | 2 ++ utils/v4l2-compliance/v4l2-compliance.h | 1 + utils/v4l2-compliance/v4l2-test-controls.cpp | 23 +++++++++++++++----- 3 files changed, 21 insertions(+), 5 deletions(-)