* [PATCH] v4l2-compliance: Fix V4L2_CTRL_WHICH_DEF_VAL in multi_classes
@ 2021-03-10 21:24 Ricardo Ribalda
2021-03-11 12:56 ` Hans Verkuil
0 siblings, 1 reply; 5+ messages in thread
From: Ricardo Ribalda @ 2021-03-10 21:24 UTC (permalink / raw)
To: Hans Verkuil, linux-media; +Cc: Ricardo Ribalda
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(-)
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;
}
--
2.30.1.766.gb4fecdf3b7-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] v4l2-compliance: Fix V4L2_CTRL_WHICH_DEF_VAL in multi_classes
2021-03-10 21:24 [PATCH] v4l2-compliance: Fix V4L2_CTRL_WHICH_DEF_VAL in multi_classes Ricardo Ribalda
@ 2021-03-11 12:56 ` Hans Verkuil
2021-03-11 13:06 ` Ricardo Ribalda
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2021-03-11 12:56 UTC (permalink / raw)
To: Ricardo Ribalda, linux-media
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;
> }
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] v4l2-compliance: Fix V4L2_CTRL_WHICH_DEF_VAL in multi_classes
2021-03-11 12:56 ` Hans Verkuil
@ 2021-03-11 13:06 ` Ricardo Ribalda
2021-03-11 13:17 ` Hans Verkuil
0 siblings, 1 reply; 5+ messages in thread
From: Ricardo Ribalda @ 2021-03-11 13:06 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux Media Mailing List
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;
> > }
> >
> >
>
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] v4l2-compliance: Fix V4L2_CTRL_WHICH_DEF_VAL in multi_classes
2021-03-11 13:06 ` Ricardo Ribalda
@ 2021-03-11 13:17 ` Hans Verkuil
2021-03-11 13:19 ` Ricardo Ribalda
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2021-03-11 13:17 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Linux Media Mailing List
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;
>>> }
>>>
>>>
>>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] v4l2-compliance: Fix V4L2_CTRL_WHICH_DEF_VAL in multi_classes
2021-03-11 13:17 ` Hans Verkuil
@ 2021-03-11 13:19 ` Ricardo Ribalda
0 siblings, 0 replies; 5+ messages in thread
From: Ricardo Ribalda @ 2021-03-11 13:19 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux Media Mailing List
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;
> >>> }
> >>>
> >>>
> >>
> >
> >
>
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-11 13:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-10 21:24 [PATCH] v4l2-compliance: Fix V4L2_CTRL_WHICH_DEF_VAL in multi_classes Ricardo Ribalda
2021-03-11 12:56 ` Hans Verkuil
2021-03-11 13:06 ` Ricardo Ribalda
2021-03-11 13:17 ` Hans Verkuil
2021-03-11 13:19 ` Ricardo Ribalda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).