Message ID | 20230529085003.47207-5-tomi.valkeinen@ideasonboard.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v5,1/7] v4l2-ctl: Add routing and streams support | expand |
On 29/05/2023 10:50, Tomi Valkeinen wrote: > Add a very simple test for > VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING. > > We can't (at least at the moment) really know here what kind of routings > the driver would accept, but we can test a VIDIOC_SUBDEV_G_ROUTING call, > followed by a VIDIOC_SUBDEV_S_ROUTING call with the routing we > received. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > utils/v4l2-compliance/v4l2-compliance.cpp | 12 ++++++++++++ > utils/v4l2-compliance/v4l2-compliance.h | 1 + > utils/v4l2-compliance/v4l2-test-subdevs.cpp | 16 ++++++++++++++++ > 3 files changed, 29 insertions(+) > > diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp > index d7c10482..f082f569 100644 > --- a/utils/v4l2-compliance/v4l2-compliance.cpp > +++ b/utils/v4l2-compliance/v4l2-compliance.cpp > @@ -1249,6 +1249,18 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ > node.is_passthrough_subdev = has_source && has_sink; > > if (has_routes) { > + printf("Sub-Device routing ioctls:\n"); > + > + for (unsigned which = V4L2_SUBDEV_FORMAT_TRY; > + which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) { > + > + printf("\ttest %s VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: %s\n", > + which ? "Active" : "Try", > + ok(testSubDevRouting(&node, which))); > + } > + > + printf("\n"); > + > for (unsigned which = V4L2_SUBDEV_FORMAT_TRY; > which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) { > > diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h > index 0cd43980..35b2274b 100644 > --- a/utils/v4l2-compliance/v4l2-compliance.h > +++ b/utils/v4l2-compliance/v4l2-compliance.h > @@ -375,6 +375,7 @@ int testSubDevEnum(struct node *node, unsigned which, unsigned pad, unsigned str > int testSubDevFormat(struct node *node, unsigned which, unsigned pad, unsigned stream); > int testSubDevSelection(struct node *node, unsigned which, unsigned pad, unsigned stream); > int testSubDevFrameInterval(struct node *node, unsigned pad, unsigned stream); > +int testSubDevRouting(struct node *node, unsigned which); > > // Buffer ioctl tests > int testReqBufs(struct node *node); > diff --git a/utils/v4l2-compliance/v4l2-test-subdevs.cpp b/utils/v4l2-compliance/v4l2-test-subdevs.cpp > index 07192bda..962d9244 100644 > --- a/utils/v4l2-compliance/v4l2-test-subdevs.cpp > +++ b/utils/v4l2-compliance/v4l2-test-subdevs.cpp > @@ -551,3 +551,19 @@ int testSubDevSelection(struct node *node, unsigned which, unsigned pad, unsigne > > return have_sel ? 0 : ENOTTY; > } > + > +int testSubDevRouting(struct node *node, unsigned which) > +{ > + struct v4l2_subdev_routing routing = {}; > + struct v4l2_subdev_route routes[256] = {}; NUM_ROUTES_MAX > + > + routing.which = which; > + routing.routes = (__u64)&routes; > + routing.num_routes = 256; NUM_ROUTES_MAX Regards, Hans > + > + fail_on_test(doioctl(node, VIDIOC_SUBDEV_G_ROUTING, &routing)); > + > + fail_on_test(doioctl(node, VIDIOC_SUBDEV_S_ROUTING, &routing)); > + > + return 0; > +}
On 07/06/2023 13:35, Hans Verkuil wrote: > On 29/05/2023 10:50, Tomi Valkeinen wrote: >> Add a very simple test for >> VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING. >> >> We can't (at least at the moment) really know here what kind of routings >> the driver would accept, but we can test a VIDIOC_SUBDEV_G_ROUTING call, >> followed by a VIDIOC_SUBDEV_S_ROUTING call with the routing we >> received. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >> --- >> utils/v4l2-compliance/v4l2-compliance.cpp | 12 ++++++++++++ >> utils/v4l2-compliance/v4l2-compliance.h | 1 + >> utils/v4l2-compliance/v4l2-test-subdevs.cpp | 16 ++++++++++++++++ >> 3 files changed, 29 insertions(+) >> >> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp >> index d7c10482..f082f569 100644 >> --- a/utils/v4l2-compliance/v4l2-compliance.cpp >> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp >> @@ -1249,6 +1249,18 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ >> node.is_passthrough_subdev = has_source && has_sink; >> >> if (has_routes) { >> + printf("Sub-Device routing ioctls:\n"); >> + >> + for (unsigned which = V4L2_SUBDEV_FORMAT_TRY; >> + which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) { >> + >> + printf("\ttest %s VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: %s\n", >> + which ? "Active" : "Try", >> + ok(testSubDevRouting(&node, which))); >> + } >> + >> + printf("\n"); >> + >> for (unsigned which = V4L2_SUBDEV_FORMAT_TRY; >> which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) { >> >> diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h >> index 0cd43980..35b2274b 100644 >> --- a/utils/v4l2-compliance/v4l2-compliance.h >> +++ b/utils/v4l2-compliance/v4l2-compliance.h >> @@ -375,6 +375,7 @@ int testSubDevEnum(struct node *node, unsigned which, unsigned pad, unsigned str >> int testSubDevFormat(struct node *node, unsigned which, unsigned pad, unsigned stream); >> int testSubDevSelection(struct node *node, unsigned which, unsigned pad, unsigned stream); >> int testSubDevFrameInterval(struct node *node, unsigned pad, unsigned stream); >> +int testSubDevRouting(struct node *node, unsigned which); >> >> // Buffer ioctl tests >> int testReqBufs(struct node *node); >> diff --git a/utils/v4l2-compliance/v4l2-test-subdevs.cpp b/utils/v4l2-compliance/v4l2-test-subdevs.cpp >> index 07192bda..962d9244 100644 >> --- a/utils/v4l2-compliance/v4l2-test-subdevs.cpp >> +++ b/utils/v4l2-compliance/v4l2-test-subdevs.cpp >> @@ -551,3 +551,19 @@ int testSubDevSelection(struct node *node, unsigned which, unsigned pad, unsigne >> >> return have_sel ? 0 : ENOTTY; >> } >> + >> +int testSubDevRouting(struct node *node, unsigned which) >> +{ >> + struct v4l2_subdev_routing routing = {}; >> + struct v4l2_subdev_route routes[256] = {}; > > NUM_ROUTES_MAX > >> + >> + routing.which = which; >> + routing.routes = (__u64)&routes; >> + routing.num_routes = 256; > > NUM_ROUTES_MAX Actually, you should also test the corner cases of NUM_ROUTES_MAX + 1 (that should fail, right?) and setting num_routes to 0 and check that ENOSPC is returned and num_routes is updated. Also verify that 'reserved' is zeroed (i.e. set it to 0xff here, then check for 0 after the ioctl). Regards, Hans > > Regards, > > Hans > >> + >> + fail_on_test(doioctl(node, VIDIOC_SUBDEV_G_ROUTING, &routing)); >> + >> + fail_on_test(doioctl(node, VIDIOC_SUBDEV_S_ROUTING, &routing)); >> + >> + return 0; >> +} >
On 07/06/2023 13:48, Hans Verkuil wrote: > On 07/06/2023 13:35, Hans Verkuil wrote: >> On 29/05/2023 10:50, Tomi Valkeinen wrote: >>> Add a very simple test for >>> VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING. >>> >>> We can't (at least at the moment) really know here what kind of routings >>> the driver would accept, but we can test a VIDIOC_SUBDEV_G_ROUTING call, >>> followed by a VIDIOC_SUBDEV_S_ROUTING call with the routing we >>> received. >>> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >>> --- >>> utils/v4l2-compliance/v4l2-compliance.cpp | 12 ++++++++++++ >>> utils/v4l2-compliance/v4l2-compliance.h | 1 + >>> utils/v4l2-compliance/v4l2-test-subdevs.cpp | 16 ++++++++++++++++ >>> 3 files changed, 29 insertions(+) >>> >>> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp >>> index d7c10482..f082f569 100644 >>> --- a/utils/v4l2-compliance/v4l2-compliance.cpp >>> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp >>> @@ -1249,6 +1249,18 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ >>> node.is_passthrough_subdev = has_source && has_sink; >>> >>> if (has_routes) { >>> + printf("Sub-Device routing ioctls:\n"); >>> + >>> + for (unsigned which = V4L2_SUBDEV_FORMAT_TRY; >>> + which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) { >>> + >>> + printf("\ttest %s VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: %s\n", >>> + which ? "Active" : "Try", >>> + ok(testSubDevRouting(&node, which))); >>> + } >>> + >>> + printf("\n"); >>> + >>> for (unsigned which = V4L2_SUBDEV_FORMAT_TRY; >>> which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) { >>> >>> diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h >>> index 0cd43980..35b2274b 100644 >>> --- a/utils/v4l2-compliance/v4l2-compliance.h >>> +++ b/utils/v4l2-compliance/v4l2-compliance.h >>> @@ -375,6 +375,7 @@ int testSubDevEnum(struct node *node, unsigned which, unsigned pad, unsigned str >>> int testSubDevFormat(struct node *node, unsigned which, unsigned pad, unsigned stream); >>> int testSubDevSelection(struct node *node, unsigned which, unsigned pad, unsigned stream); >>> int testSubDevFrameInterval(struct node *node, unsigned pad, unsigned stream); >>> +int testSubDevRouting(struct node *node, unsigned which); >>> >>> // Buffer ioctl tests >>> int testReqBufs(struct node *node); >>> diff --git a/utils/v4l2-compliance/v4l2-test-subdevs.cpp b/utils/v4l2-compliance/v4l2-test-subdevs.cpp >>> index 07192bda..962d9244 100644 >>> --- a/utils/v4l2-compliance/v4l2-test-subdevs.cpp >>> +++ b/utils/v4l2-compliance/v4l2-test-subdevs.cpp >>> @@ -551,3 +551,19 @@ int testSubDevSelection(struct node *node, unsigned which, unsigned pad, unsigne >>> >>> return have_sel ? 0 : ENOTTY; >>> } >>> + >>> +int testSubDevRouting(struct node *node, unsigned which) >>> +{ >>> + struct v4l2_subdev_routing routing = {}; >>> + struct v4l2_subdev_route routes[256] = {}; >> >> NUM_ROUTES_MAX >> >>> + >>> + routing.which = which; >>> + routing.routes = (__u64)&routes; >>> + routing.num_routes = 256; >> >> NUM_ROUTES_MAX > > Actually, you should also test the corner cases of NUM_ROUTES_MAX + 1 > (that should fail, right?) and setting num_routes to 0 and check that > ENOSPC is returned and num_routes is updated. > > Also verify that 'reserved' is zeroed (i.e. set it to 0xff here, then > check for 0 after the ioctl). I assume also that if num_routes is set to 256, then G_ROUTING is called, num_routes is updated to the actual number of routes? The spec does not actually state that. And what should happen when num_routes is set to 0 and S_ROUTING is called? Would that clear all routes? Or is that an error? And can G_ROUTING actually return num_routes == 0 as well if there are no routes defined? Additional checks you can do is to verify that all sink/source pads are valid and that 'flags' is valid. Another test is setting a pad or stream to an invalid value and verify that EINVAL is returned. Note that the spec says that E2BIG is returned for S_ROUTING, but it is returned for G_ROUTING as well, that should be updated. Regards, Hans > > Regards, > > Hans > >> >> Regards, >> >> Hans >> >>> + >>> + fail_on_test(doioctl(node, VIDIOC_SUBDEV_G_ROUTING, &routing)); >>> + >>> + fail_on_test(doioctl(node, VIDIOC_SUBDEV_S_ROUTING, &routing)); >>> + >>> + return 0; >>> +} >> >
On 07/06/2023 14:57, Hans Verkuil wrote: > On 07/06/2023 13:48, Hans Verkuil wrote: >> On 07/06/2023 13:35, Hans Verkuil wrote: >>> On 29/05/2023 10:50, Tomi Valkeinen wrote: >>>> Add a very simple test for >>>> VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING. >>>> >>>> We can't (at least at the moment) really know here what kind of routings >>>> the driver would accept, but we can test a VIDIOC_SUBDEV_G_ROUTING call, >>>> followed by a VIDIOC_SUBDEV_S_ROUTING call with the routing we >>>> received. >>>> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >>>> --- >>>> utils/v4l2-compliance/v4l2-compliance.cpp | 12 ++++++++++++ >>>> utils/v4l2-compliance/v4l2-compliance.h | 1 + >>>> utils/v4l2-compliance/v4l2-test-subdevs.cpp | 16 ++++++++++++++++ >>>> 3 files changed, 29 insertions(+) >>>> >>>> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp >>>> index d7c10482..f082f569 100644 >>>> --- a/utils/v4l2-compliance/v4l2-compliance.cpp >>>> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp >>>> @@ -1249,6 +1249,18 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ >>>> node.is_passthrough_subdev = has_source && has_sink; >>>> >>>> if (has_routes) { >>>> + printf("Sub-Device routing ioctls:\n"); >>>> + >>>> + for (unsigned which = V4L2_SUBDEV_FORMAT_TRY; >>>> + which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) { >>>> + >>>> + printf("\ttest %s VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: %s\n", >>>> + which ? "Active" : "Try", >>>> + ok(testSubDevRouting(&node, which))); >>>> + } >>>> + >>>> + printf("\n"); >>>> + >>>> for (unsigned which = V4L2_SUBDEV_FORMAT_TRY; >>>> which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) { >>>> >>>> diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h >>>> index 0cd43980..35b2274b 100644 >>>> --- a/utils/v4l2-compliance/v4l2-compliance.h >>>> +++ b/utils/v4l2-compliance/v4l2-compliance.h >>>> @@ -375,6 +375,7 @@ int testSubDevEnum(struct node *node, unsigned which, unsigned pad, unsigned str >>>> int testSubDevFormat(struct node *node, unsigned which, unsigned pad, unsigned stream); >>>> int testSubDevSelection(struct node *node, unsigned which, unsigned pad, unsigned stream); >>>> int testSubDevFrameInterval(struct node *node, unsigned pad, unsigned stream); >>>> +int testSubDevRouting(struct node *node, unsigned which); >>>> >>>> // Buffer ioctl tests >>>> int testReqBufs(struct node *node); >>>> diff --git a/utils/v4l2-compliance/v4l2-test-subdevs.cpp b/utils/v4l2-compliance/v4l2-test-subdevs.cpp >>>> index 07192bda..962d9244 100644 >>>> --- a/utils/v4l2-compliance/v4l2-test-subdevs.cpp >>>> +++ b/utils/v4l2-compliance/v4l2-test-subdevs.cpp >>>> @@ -551,3 +551,19 @@ int testSubDevSelection(struct node *node, unsigned which, unsigned pad, unsigne >>>> >>>> return have_sel ? 0 : ENOTTY; >>>> } >>>> + >>>> +int testSubDevRouting(struct node *node, unsigned which) >>>> +{ >>>> + struct v4l2_subdev_routing routing = {}; >>>> + struct v4l2_subdev_route routes[256] = {}; >>> >>> NUM_ROUTES_MAX >>> >>>> + >>>> + routing.which = which; >>>> + routing.routes = (__u64)&routes; >>>> + routing.num_routes = 256; >>> >>> NUM_ROUTES_MAX Yes, a common NUM_ROUTES_MAX in utils/common/v4l2-info.h makes sense. >> Actually, you should also test the corner cases of NUM_ROUTES_MAX + 1 Yes, that fails, but I don't think it's a good test as the maximum number of routes is not part of the uAPI, it's just an arbitrary sanity limit. >> (that should fail, right?) and setting num_routes to 0 and check that >> ENOSPC is returned and num_routes is updated. Yes, I'll add that. However, if the subdev has 0 routes, the ioctl can succeed and num_routes is 0. I'll check that too. >> Also verify that 'reserved' is zeroed (i.e. set it to 0xff here, then >> check for 0 after the ioctl). Ok. > I assume also that if num_routes is set to 256, then G_ROUTING is called, > num_routes is updated to the actual number of routes? The spec does not > actually state that. Hmm, indeed, the doc doesn't say that num_routes is updated on success. I need to add that. > And what should happen when num_routes is set to 0 and S_ROUTING is called? > Would that clear all routes? Or is that an error? And can G_ROUTING actually > return num_routes == 0 as well if there are no routes defined? Yes, it would clear the routes, and G_ROUTING would return num_routes == 0. However, this is really up to the driver, and it can return an error for S_ROUTING with num_routes == 0. > Additional checks you can do is to verify that all sink/source pads are valid > and that 'flags' is valid. Ok. But if I understand the code right, I can only do that when the user is testing a media device, not if the user is testing a single subdevice. I can do this part of the test when node->pads is non-NULL. > Another test is setting a pad or stream to an invalid value and verify that EINVAL > is returned. There are no invalid stream values, but I can test for invalid pads. However, here I again need a media device as above. > Note that the spec says that E2BIG is returned for S_ROUTING, but it is returned > for G_ROUTING as well, that should be updated. For the 256+ case? As I mentioned above, it's an arbitrary "hidden" limit, which can be increased in the kernel when needed. So, in theory, G_ROUTING should accept any num_routes value, but to avoid large allocations in the kernel side, it's at the moment limited to 256. Maybe a better kernel side implementation would be to silently limit the kernel side arrays to 256, even if the userspace provides a larger array. Tomi
diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp index d7c10482..f082f569 100644 --- a/utils/v4l2-compliance/v4l2-compliance.cpp +++ b/utils/v4l2-compliance/v4l2-compliance.cpp @@ -1249,6 +1249,18 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ node.is_passthrough_subdev = has_source && has_sink; if (has_routes) { + printf("Sub-Device routing ioctls:\n"); + + for (unsigned which = V4L2_SUBDEV_FORMAT_TRY; + which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) { + + printf("\ttest %s VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: %s\n", + which ? "Active" : "Try", + ok(testSubDevRouting(&node, which))); + } + + printf("\n"); + for (unsigned which = V4L2_SUBDEV_FORMAT_TRY; which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) { diff --git a/utils/v4l2-compliance/v4l2-compliance.h b/utils/v4l2-compliance/v4l2-compliance.h index 0cd43980..35b2274b 100644 --- a/utils/v4l2-compliance/v4l2-compliance.h +++ b/utils/v4l2-compliance/v4l2-compliance.h @@ -375,6 +375,7 @@ int testSubDevEnum(struct node *node, unsigned which, unsigned pad, unsigned str int testSubDevFormat(struct node *node, unsigned which, unsigned pad, unsigned stream); int testSubDevSelection(struct node *node, unsigned which, unsigned pad, unsigned stream); int testSubDevFrameInterval(struct node *node, unsigned pad, unsigned stream); +int testSubDevRouting(struct node *node, unsigned which); // Buffer ioctl tests int testReqBufs(struct node *node); diff --git a/utils/v4l2-compliance/v4l2-test-subdevs.cpp b/utils/v4l2-compliance/v4l2-test-subdevs.cpp index 07192bda..962d9244 100644 --- a/utils/v4l2-compliance/v4l2-test-subdevs.cpp +++ b/utils/v4l2-compliance/v4l2-test-subdevs.cpp @@ -551,3 +551,19 @@ int testSubDevSelection(struct node *node, unsigned which, unsigned pad, unsigne return have_sel ? 0 : ENOTTY; } + +int testSubDevRouting(struct node *node, unsigned which) +{ + struct v4l2_subdev_routing routing = {}; + struct v4l2_subdev_route routes[256] = {}; + + routing.which = which; + routing.routes = (__u64)&routes; + routing.num_routes = 256; + + fail_on_test(doioctl(node, VIDIOC_SUBDEV_G_ROUTING, &routing)); + + fail_on_test(doioctl(node, VIDIOC_SUBDEV_S_ROUTING, &routing)); + + return 0; +}