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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux