Re: [PATCH v6 5/8] v4l2-ctl/compliance: Add simple routing test

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

On 02/08/2023 16:54, Hans Verkuil wrote:
> On 20/07/2023 09:50, Tomi Valkeinen wrote:
> > Add a 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.
> >
> > Additionally, we can check that the returned pads and flags look fine,
> > and also that setting obviously invalid routing will fail.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> > ---
> >   utils/v4l2-compliance/v4l2-compliance.cpp   | 12 ++++
> >   utils/v4l2-compliance/v4l2-compliance.h     |  1 +
> >   utils/v4l2-compliance/v4l2-test-subdevs.cpp | 74 +++++++++++++++++++++
> >   3 files changed, 87 insertions(+)
> >
> > diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp
> > index e8359b2f..4b232314 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 5545b0dc..e59d67f7 100644
> > --- a/utils/v4l2-compliance/v4l2-test-subdevs.cpp
> > +++ b/utils/v4l2-compliance/v4l2-test-subdevs.cpp
> > @@ -551,3 +551,77 @@ int testSubDevSelection(struct node *node, unsigned which, unsigned pad, unsigne
> >   
> >   	return have_sel ? 0 : ENOTTY;
> >   }
> > +
> > +int testSubDevRouting(struct node *node, unsigned which)
> > +{
> > +	const uint32_t all_route_flags_mask = V4L2_SUBDEV_ROUTE_FL_ACTIVE;
> > +	struct v4l2_subdev_routing routing = {};
> > +	struct v4l2_subdev_route routes[NUM_ROUTES_MAX] = {};
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	routing.which = which;
> > +	routing.routes = (__u64)&routes;
> > +	routing.num_routes = 0;
> > +	memset(routing.reserved, 0xff, sizeof(routing.reserved));
> > +
> > +	/*
> > +	 * First test that G_ROUTING either returns success, or ENOSPC and
> > +	 * updates num_routes.
> > +	 */
> > +
> > +	ret = doioctl(node, VIDIOC_SUBDEV_G_ROUTING, &routing);
> > +	fail_on_test(ret && ret != ENOSPC);
> > +	fail_on_test(ret == ENOSPC && routing.num_routes == 0);
> > +	fail_on_test(check_0(routing.reserved, sizeof(routing.reserved)));
> > +
> > +	if (routing.num_routes)
> > +		return 0;
>
> Shouldn't this be 'if (!routing.num_routes)'?

Yes...

> > +
> > +	/* Then get the actual routes */
> > +
> > +	routing.num_routes = NUM_ROUTES_MAX;
> > +	fail_on_test(doioctl(node, VIDIOC_SUBDEV_G_ROUTING, &routing));
>
> I assume that num_routes is always updated to the actual number of routes, right?

If VIDIOC_SUBDEV_G_ROUTING succeeds, yes, num_routes is updated.

> That's not actually explained clearly in the spec. It says that if the app provided
> num_routes is too small, then it is updated, but it says nothing about what happens
> if the app provided value is too large.

Ok. I need to update the doc.

> Assuming I am right, then I would rewrite this code as follows:
>
> 	__u32 num_routes = routing.num_routes;
> 	routing.num_routes = num_routes + 1;
> 	fail_on_test(doioctl(node, VIDIOC_SUBDEV_G_ROUTING, &routing));
> 	fail_on_test(routing.num_routes != num_routes);

Yes, I think this looks fine.

Btw, you use __u32 above. Is there any style guide for these? I used 
uint32_t in this function for another variable, and I'd use it here too.

> > +
> > +	/* Check the validity of route pads and flags */
> > +
> > +	if (node->pads) {
> > +		for (i = 0; i < routing.num_routes; ++i) {
> > +			const struct v4l2_subdev_route *route = &routes[i];
> > +			const struct media_pad_desc *sink;
> > +			const struct media_pad_desc *source;
> > +
> > +			fail_on_test(route->sink_pad >= node->entity.pads);
> > +			fail_on_test(route->source_pad >= node->entity.pads);
> > +
> > +			sink = &node->pads[route->sink_pad];
> > +			source = &node->pads[route->source_pad];
> > +
> > +			fail_on_test(!(sink->flags & MEDIA_PAD_FL_SINK));
> > +			fail_on_test(!(source->flags & MEDIA_PAD_FL_SOURCE));
> > +			fail_on_test(route->flags & ~all_route_flags_mask);
> > +		}
> > +	}
> > +
> > +	/* Set the same routes back, which should always succeed. */
> > +
> > +	memset(routing.reserved, 0xff, sizeof(routing.reserved));
> > +	fail_on_test(doioctl(node, VIDIOC_SUBDEV_S_ROUTING, &routing));
> > +	fail_on_test(check_0(routing.reserved, sizeof(routing.reserved)));
> > +
> > +	/* Test setting invalid pads */
> > +
> > +	if (node->pads) {
> > +		for (i = 0; i < routing.num_routes; ++i) {
> > +			struct v4l2_subdev_route *route = &routes[i];
> > +
> > +			route->sink_pad = node->entity.pads + 1;
> > +		}
> > +
> > +		memset(routing.reserved, 0xff, sizeof(routing.reserved));
> > +		fail_on_test(doioctl(node, VIDIOC_SUBDEV_S_ROUTING, &routing) != EINVAL);
> > +		fail_on_test(check_0(routing.reserved, sizeof(routing.reserved)));

After fixing the num_routes check, I noticed that this one is broken 
too. If S_ROUTING fails early enough, the reserved field won't get 
cleared, so we can't have this check here.

> > +	}
> > +
> > +	return 0;
> > +}
>
> Regards,
>
> 	Hans