diff mbox series

[v4l-utils,v2,3/7] v4l2-compliance: Support the changed routing API

Message ID 20240424152230.31923-4-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series Support for the generic line-based metadata support | expand

Commit Message

Laurent Pinchart April 24, 2024, 3:22 p.m. UTC 
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

Hans Verkuil April 24, 2024, 3:50 p.m. UTC | #1 
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 */
Laurent Pinchart April 24, 2024, 4:07 p.m. UTC | #2 
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 */
diff mbox series

Patch

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)));
 
 	/* Test setting invalid pads */