| Message ID | 20240402000033.4007-7-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support for the generic line-based metadata support | expand |
On 02/04/2024 03:00, 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 2cf979096bd0..72fe0bd778ab 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 = (__u64)sd_routes[which]; > - sd_routing[which].num_routes = NUM_ROUTES_MAX; > + routing.which = which; > + routing.routes = (__u64)sd_routes[which]; > + routing.len_routes = NUM_ROUTES_MAX; > + routing.num_routes = 0; Not part of this series, but something I noticed while testing. Our routing code is broken for 32-bit userspace: we convert the routes pointer to u64, which ends up sign extending the address. Interestingly gcc gives "cast from pointer to integer of different size [-Wpointer-to-int-cast]", but g++ doesn't, and "command-line option ‘-Wpointer-to-int-cast’ is valid for C/ObjC but not for C++". So we don't get any warnings for the bugs. With a quick look I found these places to fix. Will you add a patch to your series, or should I send this as a separate one? diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp index 72fe0bd7..a969b69c 100644 --- a/utils/v4l2-compliance/v4l2-compliance.cpp +++ b/utils/v4l2-compliance/v4l2-compliance.cpp @@ -1275,7 +1275,7 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ struct v4l2_subdev_routing &routing = sd_routing[which]; routing.which = which; - routing.routes = (__u64)sd_routes[which]; + routing.routes = (__u64)(uintptr_t)sd_routes[which]; routing.len_routes = NUM_ROUTES_MAX; routing.num_routes = 0; @@ -1307,7 +1307,7 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ routes = sd_routes[which]; } else { dummy_routing.num_routes = 1; - dummy_routing.routes = (__u64)&dummy_routes; + dummy_routing.routes = (__u64)(uintptr_t)&dummy_routes; dummy_routes[0].source_pad = pad; dummy_routes[0].source_stream = 0; dummy_routes[0].sink_pad = pad; diff --git a/utils/v4l2-compliance/v4l2-test-subdevs.cpp b/utils/v4l2-compliance/v4l2-test-subdevs.cpp index e26d09ed..21d7776a 100644 --- a/utils/v4l2-compliance/v4l2-test-subdevs.cpp +++ b/utils/v4l2-compliance/v4l2-test-subdevs.cpp @@ -587,7 +587,7 @@ int testSubDevRouting(struct node *node, unsigned which) int ret; routing.which = which; - routing.routes = (__u64)&routes; + routing.routes = (__u64)(uintptr_t)&routes; routing.len_routes = 0; routing.num_routes = 0; memset(routing.reserved, 0xff, sizeof(routing.reserved)); Tomi
Moi, On Tue, Apr 02, 2024 at 01:54:00PM +0300, Tomi Valkeinen wrote: > On 02/04/2024 03:00, 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 2cf979096bd0..72fe0bd778ab 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 = (__u64)sd_routes[which]; > > - sd_routing[which].num_routes = NUM_ROUTES_MAX; > > + routing.which = which; > > + routing.routes = (__u64)sd_routes[which]; > > + routing.len_routes = NUM_ROUTES_MAX; > > + routing.num_routes = 0; > > Not part of this series, but something I noticed while testing. Our > routing code is broken for 32-bit userspace: we convert the routes > pointer to u64, which ends up sign extending the address. > > Interestingly gcc gives "cast from pointer to integer of different size > [-Wpointer-to-int-cast]", but g++ doesn't, and "command-line option > ‘-Wpointer-to-int-cast’ is valid for C/ObjC but not for C++". So we don't > get any warnings for the bugs. Nice find! > > With a quick look I found these places to fix. Will you add a patch to > your series, or should I send this as a separate one? > > diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp > index 72fe0bd7..a969b69c 100644 > --- a/utils/v4l2-compliance/v4l2-compliance.cpp > +++ b/utils/v4l2-compliance/v4l2-compliance.cpp > @@ -1275,7 +1275,7 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ > struct v4l2_subdev_routing &routing = sd_routing[which]; > routing.which = which; > - routing.routes = (__u64)sd_routes[which]; > + routing.routes = (__u64)(uintptr_t)sd_routes[which]; I don't think you need the second cast anymore as it's already an integer. Same for the rest. > routing.len_routes = NUM_ROUTES_MAX; > routing.num_routes = 0; > @@ -1307,7 +1307,7 @@ void testNode(struct node &node, struct node &node_m2m_cap, struct node &expbuf_ > routes = sd_routes[which]; > } else { > dummy_routing.num_routes = 1; > - dummy_routing.routes = (__u64)&dummy_routes; > + dummy_routing.routes = (__u64)(uintptr_t)&dummy_routes; > dummy_routes[0].source_pad = pad; > dummy_routes[0].source_stream = 0; > dummy_routes[0].sink_pad = pad; > diff --git a/utils/v4l2-compliance/v4l2-test-subdevs.cpp b/utils/v4l2-compliance/v4l2-test-subdevs.cpp > index e26d09ed..21d7776a 100644 > --- a/utils/v4l2-compliance/v4l2-test-subdevs.cpp > +++ b/utils/v4l2-compliance/v4l2-test-subdevs.cpp > @@ -587,7 +587,7 @@ int testSubDevRouting(struct node *node, unsigned which) > int ret; > routing.which = which; > - routing.routes = (__u64)&routes; > + routing.routes = (__u64)(uintptr_t)&routes; > routing.len_routes = 0; > routing.num_routes = 0; > memset(routing.reserved, 0xff, sizeof(routing.reserved)); > > > Tomi >
diff --git a/utils/v4l2-compliance/v4l2-compliance.cpp b/utils/v4l2-compliance/v4l2-compliance.cpp index 2cf979096bd0..72fe0bd778ab 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 = (__u64)sd_routes[which]; - sd_routing[which].num_routes = NUM_ROUTES_MAX; + routing.which = which; + routing.routes = (__u64)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 ebca1b94f5c0..714857021fc6 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 = (__u64)&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 */
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(-)