[v4l-utils,v2,3/7] v4l2-compliance: Support the changed routing API
Commit Message
Set len_routes of struct v4l2_subdev_routing. ENOSPC error code is no
longer used, i.e. having room for fewer routes than exist in the
configuration is not considered an error anymore.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
utils/v4l2-compliance/v4l2-compliance.cpp | 12 +++++++-----
utils/v4l2-compliance/v4l2-test-subdevs.cpp | 19 +++++++++++--------
2 files changed, 18 insertions(+), 13 deletions(-)
Comments
On 24/04/2024 17:22, Laurent Pinchart wrote:
> Set len_routes of struct v4l2_subdev_routing. ENOSPC error code is no
> longer used, i.e. having room for fewer routes than exist in the
> configuration is not considered an error anymore.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> utils/v4l2-compliance/v4l2-compliance.cpp | 12 +++++++-----
> utils/v4l2-compliance/v4l2-test-subdevs.cpp | 19 +++++++++++--------
> 2 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp
> index fd7e7d76e214..144f961842c6 100644
> --- a/utils/v4l2-compliance/v4l2-compliance.cpp
> +++ b/utils/v4l2-compliance/v4l2-compliance.cpp
> @@ -1272,15 +1272,17 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_
>
> for (unsigned which = V4L2_SUBDEV_FORMAT_TRY;
> which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) {
> + struct v4l2_subdev_routing &routing = sd_routing[which];
>
> - sd_routing[which].which = which;
> - sd_routing[which].routes = (uintptr_t)sd_routes[which];
> - sd_routing[which].num_routes = NUM_ROUTES_MAX;
> + routing.which = which;
> + routing.routes = (uintptr_t)sd_routes[which];
> + routing.len_routes = NUM_ROUTES_MAX;
> + routing.num_routes = 0;
>
> - ret = doioctl(&node, VIDIOC_SUBDEV_G_ROUTING, &sd_routing[which]);
> + ret = doioctl(&node, VIDIOC_SUBDEV_G_ROUTING, &routing);
> if (ret) {
> fail("VIDIOC_SUBDEV_G_ROUTING: failed to get routing\n");
> - sd_routing[which].num_routes = 0;
> + routing.num_routes = 0;
> }
> }
> }
> diff --git a/utils/v4l2-compliance/v4l2-test-subdevs.cpp b/utils/v4l2-compliance/v4l2-test-subdevs.cpp
> index da304a8caa8a..41eaf77112f0 100644
> --- a/utils/v4l2-compliance/v4l2-test-subdevs.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-subdevs.cpp
> @@ -587,17 +587,15 @@ int testSubDevRouting(struct node *node, unsigned which)
>
> routing.which = which;
> routing.routes = (uintptr_t)&routes;
> + routing.len_routes = 0;
> 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.
> - */
> + /* First test that G_ROUTING returns success even when len_routes is 0. */
>
> 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(ret);
> + fail_on_test(routing.num_routes > NUM_ROUTES_MAX);
> fail_on_test(check_0(routing.reserved, sizeof(routing.reserved)));
>
> if (!routing.num_routes)
> @@ -609,7 +607,8 @@ int testSubDevRouting(struct node *node, unsigned which)
> */
>
> uint32_t num_routes = routing.num_routes;
> - routing.num_routes = num_routes + 1;
> + routing.len_routes = NUM_ROUTES_MAX;
> + routing.num_routes = 0;
> fail_on_test(doioctl(node, VIDIOC_SUBDEV_G_ROUTING, &routing));
> fail_on_test(routing.num_routes != num_routes);
>
> @@ -633,10 +632,14 @@ int testSubDevRouting(struct node *node, unsigned which)
> }
> }
>
> - /* Set the same routes back, which should always succeed. */
> + /*
> + * Set the same routes back, which should always succeed and report the
> + * same number of routes.
> + */
>
> memset(routing.reserved, 0xff, sizeof(routing.reserved));
> fail_on_test(doioctl(node, VIDIOC_SUBDEV_S_ROUTING, &routing));
> + fail_on_test(routing.num_routes != num_routes);
> fail_on_test(check_0(routing.reserved, sizeof(routing.reserved)));
I think this needs another test: if S_ROUTING is called with num_routes > len_routes,
then EINVAL must be returned. This is an important test to make sure the core handles
this correctly and won't be using more than len_routes routes.
Regards,
Hans
>
> /* Test setting invalid pads */
Hi Hans,
On Wed, Apr 24, 2024 at 05:50:56PM +0200, Hans Verkuil wrote:
> On 24/04/2024 17:22, Laurent Pinchart wrote:
> > Set len_routes of struct v4l2_subdev_routing. ENOSPC error code is no
> > longer used, i.e. having room for fewer routes than exist in the
> > configuration is not considered an error anymore.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > utils/v4l2-compliance/v4l2-compliance.cpp | 12 +++++++-----
> > utils/v4l2-compliance/v4l2-test-subdevs.cpp | 19 +++++++++++--------
> > 2 files changed, 18 insertions(+), 13 deletions(-)
> >
> > diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp
> > index fd7e7d76e214..144f961842c6 100644
> > --- a/utils/v4l2-compliance/v4l2-compliance.cpp
> > +++ b/utils/v4l2-compliance/v4l2-compliance.cpp
> > @@ -1272,15 +1272,17 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_
> >
> > for (unsigned which = V4L2_SUBDEV_FORMAT_TRY;
> > which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) {
> > + struct v4l2_subdev_routing &routing = sd_routing[which];
> >
> > - sd_routing[which].which = which;
> > - sd_routing[which].routes = (uintptr_t)sd_routes[which];
> > - sd_routing[which].num_routes = NUM_ROUTES_MAX;
> > + routing.which = which;
> > + routing.routes = (uintptr_t)sd_routes[which];
> > + routing.len_routes = NUM_ROUTES_MAX;
> > + routing.num_routes = 0;
> >
> > - ret = doioctl(&node, VIDIOC_SUBDEV_G_ROUTING, &sd_routing[which]);
> > + ret = doioctl(&node, VIDIOC_SUBDEV_G_ROUTING, &routing);
> > if (ret) {
> > fail("VIDIOC_SUBDEV_G_ROUTING: failed to get routing\n");
> > - sd_routing[which].num_routes = 0;
> > + routing.num_routes = 0;
> > }
> > }
> > }
> > diff --git a/utils/v4l2-compliance/v4l2-test-subdevs.cpp b/utils/v4l2-compliance/v4l2-test-subdevs.cpp
> > index da304a8caa8a..41eaf77112f0 100644
> > --- a/utils/v4l2-compliance/v4l2-test-subdevs.cpp
> > +++ b/utils/v4l2-compliance/v4l2-test-subdevs.cpp
> > @@ -587,17 +587,15 @@ int testSubDevRouting(struct node *node, unsigned which)
> >
> > routing.which = which;
> > routing.routes = (uintptr_t)&routes;
> > + routing.len_routes = 0;
> > 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.
> > - */
> > + /* First test that G_ROUTING returns success even when len_routes is 0. */
> >
> > 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(ret);
> > + fail_on_test(routing.num_routes > NUM_ROUTES_MAX);
> > fail_on_test(check_0(routing.reserved, sizeof(routing.reserved)));
> >
> > if (!routing.num_routes)
> > @@ -609,7 +607,8 @@ int testSubDevRouting(struct node *node, unsigned which)
> > */
> >
> > uint32_t num_routes = routing.num_routes;
> > - routing.num_routes = num_routes + 1;
> > + routing.len_routes = NUM_ROUTES_MAX;
> > + routing.num_routes = 0;
> > fail_on_test(doioctl(node, VIDIOC_SUBDEV_G_ROUTING, &routing));
> > fail_on_test(routing.num_routes != num_routes);
> >
> > @@ -633,10 +632,14 @@ int testSubDevRouting(struct node *node, unsigned which)
> > }
> > }
> >
> > - /* Set the same routes back, which should always succeed. */
> > + /*
> > + * Set the same routes back, which should always succeed and report the
> > + * same number of routes.
> > + */
> >
> > memset(routing.reserved, 0xff, sizeof(routing.reserved));
> > fail_on_test(doioctl(node, VIDIOC_SUBDEV_S_ROUTING, &routing));
> > + fail_on_test(routing.num_routes != num_routes);
> > fail_on_test(check_0(routing.reserved, sizeof(routing.reserved)));
>
> I think this needs another test: if S_ROUTING is called with num_routes > len_routes,
> then EINVAL must be returned. This is an important test to make sure the core handles
> this correctly and won't be using more than len_routes routes.
I'll add the test now, that should be easy. I'll push an update to my
branch but won't repost the patches yet, to let reviewers a chance to
check the series.
> >
> > /* Test setting invalid pads */
@@ -1272,15 +1272,17 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_
for (unsigned which = V4L2_SUBDEV_FORMAT_TRY;
which <= V4L2_SUBDEV_FORMAT_ACTIVE; which++) {
+ struct v4l2_subdev_routing &routing = sd_routing[which];
- sd_routing[which].which = which;
- sd_routing[which].routes = (uintptr_t)sd_routes[which];
- sd_routing[which].num_routes = NUM_ROUTES_MAX;
+ routing.which = which;
+ routing.routes = (uintptr_t)sd_routes[which];
+ routing.len_routes = NUM_ROUTES_MAX;
+ routing.num_routes = 0;
- ret = doioctl(&node, VIDIOC_SUBDEV_G_ROUTING, &sd_routing[which]);
+ ret = doioctl(&node, VIDIOC_SUBDEV_G_ROUTING, &routing);
if (ret) {
fail("VIDIOC_SUBDEV_G_ROUTING: failed to get routing\n");
- sd_routing[which].num_routes = 0;
+ routing.num_routes = 0;
}
}
}
@@ -587,17 +587,15 @@ int testSubDevRouting(struct node *node, unsigned which)
routing.which = which;
routing.routes = (uintptr_t)&routes;
+ routing.len_routes = 0;
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.
- */
+ /* First test that G_ROUTING returns success even when len_routes is 0. */
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(ret);
+ fail_on_test(routing.num_routes > NUM_ROUTES_MAX);
fail_on_test(check_0(routing.reserved, sizeof(routing.reserved)));
if (!routing.num_routes)
@@ -609,7 +607,8 @@ int testSubDevRouting(struct node *node, unsigned which)
*/
uint32_t num_routes = routing.num_routes;
- routing.num_routes = num_routes + 1;
+ routing.len_routes = NUM_ROUTES_MAX;
+ routing.num_routes = 0;
fail_on_test(doioctl(node, VIDIOC_SUBDEV_G_ROUTING, &routing));
fail_on_test(routing.num_routes != num_routes);
@@ -633,10 +632,14 @@ int testSubDevRouting(struct node *node, unsigned which)
}
}
- /* Set the same routes back, which should always succeed. */
+ /*
+ * Set the same routes back, which should always succeed and report the
+ * same number of routes.
+ */
memset(routing.reserved, 0xff, sizeof(routing.reserved));
fail_on_test(doioctl(node, VIDIOC_SUBDEV_S_ROUTING, &routing));
+ fail_on_test(routing.num_routes != num_routes);
fail_on_test(check_0(routing.reserved, sizeof(routing.reserved)));
/* Test setting invalid pads */