| Message ID | 20210310212450.1220416-1-ribalda@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v4l2-compliance: Fix V4L2_CTRL_WHICH_DEF_VAL in multi_classes | expand |
On 10/03/2021 22:24, Ricardo Ribalda wrote: > If there are multiple classes, the ioctl should fail. It shouldn't matter if there are multiple classes or not, it should work. What is the exact error you get with which driver? Regards, Hans > > Fixes: 0884b19adada ("v4l2-compliance: add test for V4L2_CTRL_WHICH_DEF_VAL") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > utils/v4l2-compliance/v4l2-test-controls.cpp | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/utils/v4l2-compliance/v4l2-test-controls.cpp b/utils/v4l2-compliance/v4l2-test-controls.cpp > index 9a68b7e847b0..79982bc15054 100644 > --- a/utils/v4l2-compliance/v4l2-test-controls.cpp > +++ b/utils/v4l2-compliance/v4l2-test-controls.cpp > @@ -793,7 +793,10 @@ int testExtendedControls(struct node *node) > ctrls.which = V4L2_CTRL_WHICH_DEF_VAL; > fail_on_test(!doioctl(node, VIDIOC_S_EXT_CTRLS, &ctrls)); > fail_on_test(!doioctl(node, VIDIOC_TRY_EXT_CTRLS, &ctrls)); > - fail_on_test(doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); > + if (multiple_classes) > + fail_on_test(!doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); > + else > + fail_on_test(doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); > return 0; > } > >
Hi Hans Thanks for your review! On Thu, Mar 11, 2021 at 1:57 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 10/03/2021 22:24, Ricardo Ribalda wrote: > > If there are multiple classes, the ioctl should fail. > > It shouldn't matter if there are multiple classes or not, it should > work. I believe this is the part of the kernel that is triggering the issue: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/v4l2-core/v4l2-ioctl.c#n929 I can send a patch if that is not the intended behaviour ;) /* 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) { c->error_idx = i; return 0; > > What is the exact error you get with which driver? I am trying to fix uvc compliance fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > Regards, Best regards! > > Hans > > > > > Fixes: 0884b19adada ("v4l2-compliance: add test for V4L2_CTRL_WHICH_DEF_VAL") > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > utils/v4l2-compliance/v4l2-test-controls.cpp | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/utils/v4l2-compliance/v4l2-test-controls.cpp b/utils/v4l2-compliance/v4l2-test-controls.cpp > > index 9a68b7e847b0..79982bc15054 100644 > > --- a/utils/v4l2-compliance/v4l2-test-controls.cpp > > +++ b/utils/v4l2-compliance/v4l2-test-controls.cpp > > @@ -793,7 +793,10 @@ int testExtendedControls(struct node *node) > > ctrls.which = V4L2_CTRL_WHICH_DEF_VAL; > > fail_on_test(!doioctl(node, VIDIOC_S_EXT_CTRLS, &ctrls)); > > fail_on_test(!doioctl(node, VIDIOC_TRY_EXT_CTRLS, &ctrls)); > > - fail_on_test(doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); > > + if (multiple_classes) > > + fail_on_test(!doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); > > + else > > + fail_on_test(doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); > > return 0; > > } > > > > >
On 11/03/2021 14:06, Ricardo Ribalda wrote: > Hi Hans > > Thanks for your review! > > > On Thu, Mar 11, 2021 at 1:57 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> On 10/03/2021 22:24, Ricardo Ribalda wrote: >>> If there are multiple classes, the ioctl should fail. >> >> It shouldn't matter if there are multiple classes or not, it should >> work. > > I believe this is the part of the kernel that is triggering the issue: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/v4l2-core/v4l2-ioctl.c#n929 > > I can send a patch if that is not the intended behaviour ;) > > /* 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) { > c->error_idx = i; > return 0; Ah, and this is only called for drivers that do not use the control framework. This is a bug, just before that for-loop it says: if (!c->which) return 1; This should be: if (!c->which || c->which == V4L2_CTRL_WHICH_DEF_VAL) return 1; if (c->which == V4L2_CTRL_WHICH_REQUEST_VAL) return 0; Regards, Hans > >> >> What is the exact error you get with which driver? > > I am trying to fix uvc compliance > > fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > >> >> Regards, > > Best regards! > >> >> Hans >> >>> >>> Fixes: 0884b19adada ("v4l2-compliance: add test for V4L2_CTRL_WHICH_DEF_VAL") >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >>> --- >>> utils/v4l2-compliance/v4l2-test-controls.cpp | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/utils/v4l2-compliance/v4l2-test-controls.cpp b/utils/v4l2-compliance/v4l2-test-controls.cpp >>> index 9a68b7e847b0..79982bc15054 100644 >>> --- a/utils/v4l2-compliance/v4l2-test-controls.cpp >>> +++ b/utils/v4l2-compliance/v4l2-test-controls.cpp >>> @@ -793,7 +793,10 @@ int testExtendedControls(struct node *node) >>> ctrls.which = V4L2_CTRL_WHICH_DEF_VAL; >>> fail_on_test(!doioctl(node, VIDIOC_S_EXT_CTRLS, &ctrls)); >>> fail_on_test(!doioctl(node, VIDIOC_TRY_EXT_CTRLS, &ctrls)); >>> - fail_on_test(doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); >>> + if (multiple_classes) >>> + fail_on_test(!doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); >>> + else >>> + fail_on_test(doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); >>> return 0; >>> } >>> >>> >> > >
On Thu, Mar 11, 2021 at 2:17 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 11/03/2021 14:06, Ricardo Ribalda wrote: > > Hi Hans > > > > Thanks for your review! > > > > > > On Thu, Mar 11, 2021 at 1:57 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >> > >> On 10/03/2021 22:24, Ricardo Ribalda wrote: > >>> If there are multiple classes, the ioctl should fail. > >> > >> It shouldn't matter if there are multiple classes or not, it should > >> work. > > > > I believe this is the part of the kernel that is triggering the issue: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/v4l2-core/v4l2-ioctl.c#n929 > > > > I can send a patch if that is not the intended behaviour ;) > > > > /* 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) { > > c->error_idx = i; > > return 0; > > Ah, and this is only called for drivers that do not use the control framework. > > This is a bug, just before that for-loop it says: > > if (!c->which) > return 1; > > This should be: > > if (!c->which || c->which == V4L2_CTRL_WHICH_DEF_VAL) > return 1; > if (c->which == V4L2_CTRL_WHICH_REQUEST_VAL) > return 0; Can I send a patch for that? > > Regards, > > Hans > > > > >> > >> What is the exact error you get with which driver? > > > > I am trying to fix uvc compliance > > > > fail: v4l2-test-controls.cpp(813): doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls) > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL > > > > > >> > >> Regards, > > > > Best regards! > > > >> > >> Hans > >> > >>> > >>> Fixes: 0884b19adada ("v4l2-compliance: add test for V4L2_CTRL_WHICH_DEF_VAL") > >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > >>> --- > >>> utils/v4l2-compliance/v4l2-test-controls.cpp | 5 ++++- > >>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/utils/v4l2-compliance/v4l2-test-controls.cpp b/utils/v4l2-compliance/v4l2-test-controls.cpp > >>> index 9a68b7e847b0..79982bc15054 100644 > >>> --- a/utils/v4l2-compliance/v4l2-test-controls.cpp > >>> +++ b/utils/v4l2-compliance/v4l2-test-controls.cpp > >>> @@ -793,7 +793,10 @@ int testExtendedControls(struct node *node) > >>> ctrls.which = V4L2_CTRL_WHICH_DEF_VAL; > >>> fail_on_test(!doioctl(node, VIDIOC_S_EXT_CTRLS, &ctrls)); > >>> fail_on_test(!doioctl(node, VIDIOC_TRY_EXT_CTRLS, &ctrls)); > >>> - fail_on_test(doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); > >>> + if (multiple_classes) > >>> + fail_on_test(!doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); > >>> + else > >>> + fail_on_test(doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); > >>> return 0; > >>> } > >>> > >>> > >> > > > > >
diff --git a/utils/v4l2-compliance/v4l2-test-controls.cpp b/utils/v4l2-compliance/v4l2-test-controls.cpp index 9a68b7e847b0..79982bc15054 100644 --- a/utils/v4l2-compliance/v4l2-test-controls.cpp +++ b/utils/v4l2-compliance/v4l2-test-controls.cpp @@ -793,7 +793,10 @@ int testExtendedControls(struct node *node) ctrls.which = V4L2_CTRL_WHICH_DEF_VAL; fail_on_test(!doioctl(node, VIDIOC_S_EXT_CTRLS, &ctrls)); fail_on_test(!doioctl(node, VIDIOC_TRY_EXT_CTRLS, &ctrls)); - fail_on_test(doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); + if (multiple_classes) + fail_on_test(!doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); + else + fail_on_test(doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls)); return 0; }
If there are multiple classes, the ioctl should fail. Fixes: 0884b19adada ("v4l2-compliance: add test for V4L2_CTRL_WHICH_DEF_VAL") Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- utils/v4l2-compliance/v4l2-test-controls.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)