[{"id":28853,"web_url":"https://patchwork.libcamera.org/comment/28853/","msgid":"","date":"2024-03-05T08:55:58","subject":"Re: [PATCH/RFC 09/32] libcamera: v4l2_subdevice: Add\n\tV4L2Subdevice::Route structure","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Fri, Mar 01, 2024 at 11:20:58PM +0200, Laurent Pinchart wrote:\n> The V4L2Subdevice class deals with streams in two places:\n>\n> - In routing tables, streams as expressed as a pad number and a stream\n> number in a v4l2_subdev_route instance.\n> - In the format and selection get and set functions, streams as\n> expressed using the Stream structure, which binds the pad number and\n> stream number.\n>\n> Expressing streams in different ways requires pipeline handlers and\n> other helpers to convert between the two representations. This isn't\n> much of an issue yet as libcamera has little stream-aware code, but it\n> is expected to increasingly become a burden.\n>\n> To simplify the API, introduce a V4L2Subdevice::Route structure that\n> mimicks the kernel v4l2_subdev_route structure but represents streams as\n> V4L2Subdevice::Stream instances. This will improve seamless integration\n> of routes, formats and selection rectangles.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v1:\n>\n> - Clear routing at the beginning of getRouting() and setRouting()\n> ---\n> include/libcamera/internal/v4l2_subdevice.h | 19 +++-\n> src/libcamera/pipeline/simple/simple.cpp | 8 +-\n> src/libcamera/v4l2_subdevice.cpp | 106 ++++++++++++++++++--\n> 3 files changed, 118 insertions(+), 15 deletions(-)\n>\n> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> index 2939dc2411c6..01ed4c2fc397 100644\n> --- a/include/libcamera/internal/v4l2_subdevice.h\n> +++ b/include/libcamera/internal/v4l2_subdevice.h\n> @@ -95,7 +95,23 @@ public:\n> \t\tunsigned int stream;\n> \t};\n>\n> -\tusing Routing = std::vector;\n> +\tstruct Route {\n> +\t\tRoute()\n> +\t\t\t: flags(0)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tRoute(const Stream &snk, const Stream &src, uint32_t f)\n> +\t\t\t: sink(snk), source(src), flags(f)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tStream sink;\n> +\t\tStream source;\n> +\t\tuint32_t flags;\n> +\t};\n> +\n> +\tusing Routing = std::vector;\n>\n> \texplicit V4L2Subdevice(const MediaEntity *entity);\n> \t~V4L2Subdevice();\n> @@ -174,6 +190,7 @@ static inline bool operator!=(const V4L2Subdevice::Stream &lhs,\n> }\n>\n> std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);\n> +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Route &route);\n> std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Routing &routing);\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index feea26fd124f..01f2a97798ba 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -852,17 +852,17 @@ std::vector SimpleCameraData::routedSourcePads(MediaPad *sink)\n>\n> \tstd::vector pads;\n>\n> -\tfor (const struct v4l2_subdev_route &route : routing) {\n> -\t\tif (sink->index() != route.sink_pad ||\n> +\tfor (const V4L2Subdevice::Route &route : routing) {\n> +\t\tif (sink->index() != route.sink.pad ||\n> \t\t !(route.flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))\n> \t\t\tcontinue;\n>\n> -\t\tconst MediaPad *pad = entity->getPadByIndex(route.source_pad);\n> +\t\tconst MediaPad *pad = entity->getPadByIndex(route.source.pad);\n> \t\tif (!pad) {\n> \t\t\tLOG(SimplePipeline, Warning)\n> \t\t\t\t<< \"Entity \" << entity->name()\n> \t\t\t\t<< \" has invalid route source pad \"\n> -\t\t\t\t<< route.source_pad;\n> +\t\t\t\t<< route.source.pad;\n> \t\t}\n>\n> \t\tpads.push_back(pad);\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 5dedfde4107f..3653f57a7d10 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -870,6 +870,53 @@ std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n> \treturn out;\n> }\n>\n> +/**\n> + * \\class V4L2Subdevice::Route\n> + * \\brief V4L2 subdevice routing table entry\n> + *\n> + * This class models a route in the subdevice routing table. It is similar to\n> + * the v4l2_subdev_route structure, but uses the V4L2Subdevice::Stream class\n> + * for easier usage with the V4L2Subdevice stream-aware functions.\n> + *\n> + * \\var V4L2Subdevice::Route::sink\n> + * \\brief The sink stream of the route\n> + *\n> + * \\var V4L2Subdevice::Route::source\n> + * \\brief The source stream of the route\n> + *\n> + * \\var V4L2Subdevice::Route::flags\n> + * \\brief The route flags (V4L2_SUBDEV_ROUTE_FL_*)\n> + */\n> +\n> +/**\n> + * \\fn V4L2Subdevice::Route::Route()\n> + * \\brief Construct a Route with default streams\n> + */\n> +\n> +/**\n> + * \\fn V4L2Subdevice::Route::Route(const Stream &sink, const Stream &source,\n> + * uint32_t flags)\n> + * \\brief Construct a Route from \\a sink to \\a source\n> + * \\param[in] sink The sink stream\n> + * \\param[in] source The source stream\n> + * \\param[in] flags The route flags\n> + */\n> +\n> +/**\n> + * \\brief Insert a text representation of a V4L2Subdevice::Route into an\n> + *\toutput stream\n\nStill not sure if this is intentional.\n\n> + * \\param[in] out The output stream\n> + * \\param[in] route The V4L2Subdevice::Route\n> + * \\return The output stream \\a out\n> + */\n> +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Route &route)\n> +{\n> +\tout << route.sink << \" -> \" << route.source\n> +\t << \" (\" << utils::hex(route.flags) << \")\";\n> +\n> +\treturn out;\n> +}\n> +\n> /**\n> * \\typedef V4L2Subdevice::Routing\n> * \\brief V4L2 subdevice routing table\n> @@ -887,10 +934,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n> std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Routing &routing)\n> {\n> \tfor (const auto &[i, route] : utils::enumerate(routing)) {\n> -\t\tout << \"[\" << i << \"] \"\n> -\t\t << route.sink_pad << \"/\" << route.sink_stream << \" -> \"\n> -\t\t << route.source_pad << \"/\" << route.source_stream\n> -\t\t << \" (\" << utils::hex(route.flags) << \")\";\n> +\t\tout << \"[\" << i << \"] \" << route;\n> \t\tif (i != routing.size() - 1)\n> \t\t\tout << \", \";\n> \t}\n> @@ -1244,6 +1288,30 @@ int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> * \\return 0 on success or a negative error code otherwise\n> */\n>\n> +namespace {\n> +\n> +void routeFromKernel(V4L2Subdevice::Route &route,\n> +\t\t const struct v4l2_subdev_route &kroute)\n> +{\n> +\troute.sink.pad = kroute.sink_pad;\n> +\troute.sink.stream = kroute.sink_stream;\n> +\troute.source.pad = kroute.source_pad;\n> +\troute.source.stream = kroute.source_stream;\n> +\troute.flags = kroute.flags;\n> +}\n> +\n> +void routeToKernel(const V4L2Subdevice::Route &route,\n> +\t\t struct v4l2_subdev_route &kroute)\n> +{\n> +\tkroute.sink_pad = route.sink.pad;\n> +\tkroute.sink_stream = route.sink.stream;\n> +\tkroute.source_pad = route.source.pad;\n> +\tkroute.source_stream = route.source.stream;\n> +\tkroute.flags = route.flags;\n> +}\n\nStatic helpers always feel a bit meh to me.. have you considered\n\n Route::fromKernelRoute(const struct v4l2_subdev_route &kroute);\n Route::toKernelRoute(struct v4l2_subdev_route *kroute);\n\nand decided static helpers are better ?\n\n> +\n> +} /* namespace */\n> +\n> /**\n> * \\brief Retrieve the subdevice's internal routing table\n> * \\param[out] routing The routing table\n> @@ -1254,6 +1322,8 @@ int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> */\n> int V4L2Subdevice::getRouting(Routing *routing, Whence whence)\n> {\n> +\trouting->clear();\n> +\n> \tif (!caps_.hasStreams())\n> \t\treturn 0;\n>\n> @@ -1272,8 +1342,8 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)\n> \t\treturn ret;\n> \t}\n>\n> -\trouting->resize(rt.num_routes);\n> -\trt.routes = reinterpret_cast(routing->data());\n> +\tstd::vector routes{ rt.num_routes };\n> +\trt.routes = reinterpret_cast(routes.data());\n>\n> \tret = ioctl(VIDIOC_SUBDEV_G_ROUTING, &rt);\n> \tif (ret) {\n> @@ -1282,11 +1352,16 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)\n> \t\treturn ret;\n> \t}\n>\n> -\tif (rt.num_routes != routing->size()) {\n> +\tif (rt.num_routes != routes.size()) {\n> \t\tLOG(V4L2, Error) << \"Invalid number of routes\";\n> \t\treturn -EINVAL;\n> \t}\n>\n> +\trouting->resize(rt.num_routes);\n> +\n> +\tfor (const auto &[i, route] : utils::enumerate(routes))\n> +\t\trouteFromKernel((*routing)[i], route);\n> +\n> \treturn 0;\n> }\n>\n> @@ -1304,13 +1379,20 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)\n> */\n> int V4L2Subdevice::setRouting(Routing *routing, Whence whence)\n> {\n> -\tif (!caps_.hasStreams())\n> +\tif (!caps_.hasStreams()) {\n> +\t\trouting->clear();\n> \t\treturn 0;\n> +\t}\n> +\n> +\tstd::vector routes{ routing->size() };\n> +\n> +\tfor (const auto &[i, route] : utils::enumerate(*routing))\n> +\t\trouteToKernel(route, routes[i]);\n>\n> \tstruct v4l2_subdev_routing rt = {};\n> \trt.which = whence;\n> -\trt.num_routes = routing->size();\n> -\trt.routes = reinterpret_cast(routing->data());\n> +\trt.num_routes = routes.size();\n> +\trt.routes = reinterpret_cast(routes.data());\n>\n> \tint ret = ioctl(VIDIOC_SUBDEV_S_ROUTING, &rt);\n> \tif (ret) {\n> @@ -1318,8 +1400,12 @@ int V4L2Subdevice::setRouting(Routing *routing, Whence whence)\n> \t\treturn ret;\n> \t}\n>\n> +\troutes.resize(rt.num_routes);\n> \trouting->resize(rt.num_routes);\n>\n> +\tfor (const auto &[i, route] : utils::enumerate(routes))\n> +\t\trouteFromKernel((*routing)[i], route);\n> +\n> \treturn 0;\n\nWith or without the static helpers\nReviewed-by: Jacopo Mondi \n\n> }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C9525C326B\n\tfor ;\n\tTue, 5 Mar 2024 08:56:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2397562868;\n\tTue, 5 Mar 2024 09:56:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 98EE062865\n\tfor ;\n\tTue, 5 Mar 2024 09:56:01 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8B6BECC8;\n\tTue, 5 Mar 2024 09:55:44 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uATti3cO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709628944;\n\tbh=vMocGj0yWGQg49Bl04BaH8XX9djWU03RfKerhz+Y2/I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uATti3cOa0gzx+UFY/Mv5CPlR0gXY2VbyoxTSfppCeepMAHSRzlNfAVFSjljoMDNy\n\taDg1BCYFtF9VAr7Rf/YTv9jyYHXVZP87ENweE8NYqRMfXGhbujv3nBrieqZhTh5FRq\n\t5u1h5Y9oLZ3K+SfmQNZnZIyC/wFmv/U1fAH6uyN0=","Date":"Tue, 5 Mar 2024 09:55:58 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Subject":"Re: [PATCH/RFC 09/32] libcamera: v4l2_subdevice: Add\n\tV4L2Subdevice::Route structure","Message-ID":"","References":"<20240301212121.9072-1-laurent.pinchart@ideasonboard.com>\n\t<20240301212121.9072-10-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240301212121.9072-10-laurent.pinchart@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, Sakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28857,"web_url":"https://patchwork.libcamera.org/comment/28857/","msgid":"<20240305092122.GI12503@pendragon.ideasonboard.com>","date":"2024-03-05T09:21:22","subject":"Re: [PATCH/RFC 09/32] libcamera: v4l2_subdevice: Add\n\tV4L2Subdevice::Route structure","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Mar 05, 2024 at 09:55:58AM +0100, Jacopo Mondi wrote:\n> Hi Laurent\n> \n> On Fri, Mar 01, 2024 at 11:20:58PM +0200, Laurent Pinchart wrote:\n> > The V4L2Subdevice class deals with streams in two places:\n> >\n> > - In routing tables, streams as expressed as a pad number and a stream\n> > number in a v4l2_subdev_route instance.\n> > - In the format and selection get and set functions, streams as\n> > expressed using the Stream structure, which binds the pad number and\n> > stream number.\n> >\n> > Expressing streams in different ways requires pipeline handlers and\n> > other helpers to convert between the two representations. This isn't\n> > much of an issue yet as libcamera has little stream-aware code, but it\n> > is expected to increasingly become a burden.\n> >\n> > To simplify the API, introduce a V4L2Subdevice::Route structure that\n> > mimicks the kernel v4l2_subdev_route structure but represents streams as\n> > V4L2Subdevice::Stream instances. This will improve seamless integration\n> > of routes, formats and selection rectangles.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v1:\n> >\n> > - Clear routing at the beginning of getRouting() and setRouting()\n> > ---\n> > include/libcamera/internal/v4l2_subdevice.h | 19 +++-\n> > src/libcamera/pipeline/simple/simple.cpp | 8 +-\n> > src/libcamera/v4l2_subdevice.cpp | 106 ++++++++++++++++++--\n> > 3 files changed, 118 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> > index 2939dc2411c6..01ed4c2fc397 100644\n> > --- a/include/libcamera/internal/v4l2_subdevice.h\n> > +++ b/include/libcamera/internal/v4l2_subdevice.h\n> > @@ -95,7 +95,23 @@ public:\n> > \t\tunsigned int stream;\n> > \t};\n> >\n> > -\tusing Routing = std::vector;\n> > +\tstruct Route {\n> > +\t\tRoute()\n> > +\t\t\t: flags(0)\n> > +\t\t{\n> > +\t\t}\n> > +\n> > +\t\tRoute(const Stream &snk, const Stream &src, uint32_t f)\n> > +\t\t\t: sink(snk), source(src), flags(f)\n> > +\t\t{\n> > +\t\t}\n> > +\n> > +\t\tStream sink;\n> > +\t\tStream source;\n> > +\t\tuint32_t flags;\n> > +\t};\n> > +\n> > +\tusing Routing = std::vector;\n> >\n> > \texplicit V4L2Subdevice(const MediaEntity *entity);\n> > \t~V4L2Subdevice();\n> > @@ -174,6 +190,7 @@ static inline bool operator!=(const V4L2Subdevice::Stream &lhs,\n> > }\n> >\n> > std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);\n> > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Route &route);\n> > std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Routing &routing);\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index feea26fd124f..01f2a97798ba 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -852,17 +852,17 @@ std::vector SimpleCameraData::routedSourcePads(MediaPad *sink)\n> >\n> > \tstd::vector pads;\n> >\n> > -\tfor (const struct v4l2_subdev_route &route : routing) {\n> > -\t\tif (sink->index() != route.sink_pad ||\n> > +\tfor (const V4L2Subdevice::Route &route : routing) {\n> > +\t\tif (sink->index() != route.sink.pad ||\n> > \t\t !(route.flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))\n> > \t\t\tcontinue;\n> >\n> > -\t\tconst MediaPad *pad = entity->getPadByIndex(route.source_pad);\n> > +\t\tconst MediaPad *pad = entity->getPadByIndex(route.source.pad);\n> > \t\tif (!pad) {\n> > \t\t\tLOG(SimplePipeline, Warning)\n> > \t\t\t\t<< \"Entity \" << entity->name()\n> > \t\t\t\t<< \" has invalid route source pad \"\n> > -\t\t\t\t<< route.source_pad;\n> > +\t\t\t\t<< route.source.pad;\n> > \t\t}\n> >\n> > \t\tpads.push_back(pad);\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 5dedfde4107f..3653f57a7d10 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -870,6 +870,53 @@ std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n> > \treturn out;\n> > }\n> >\n> > +/**\n> > + * \\class V4L2Subdevice::Route\n> > + * \\brief V4L2 subdevice routing table entry\n> > + *\n> > + * This class models a route in the subdevice routing table. It is similar to\n> > + * the v4l2_subdev_route structure, but uses the V4L2Subdevice::Stream class\n> > + * for easier usage with the V4L2Subdevice stream-aware functions.\n> > + *\n> > + * \\var V4L2Subdevice::Route::sink\n> > + * \\brief The sink stream of the route\n> > + *\n> > + * \\var V4L2Subdevice::Route::source\n> > + * \\brief The source stream of the route\n> > + *\n> > + * \\var V4L2Subdevice::Route::flags\n> > + * \\brief The route flags (V4L2_SUBDEV_ROUTE_FL_*)\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Subdevice::Route::Route()\n> > + * \\brief Construct a Route with default streams\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Subdevice::Route::Route(const Stream &sink, const Stream &source,\n> > + * uint32_t flags)\n> > + * \\brief Construct a Route from \\a sink to \\a source\n> > + * \\param[in] sink The sink stream\n> > + * \\param[in] source The source stream\n> > + * \\param[in] flags The route flags\n> > + */\n> > +\n> > +/**\n> > + * \\brief Insert a text representation of a V4L2Subdevice::Route into an\n> > + *\toutput stream\n> \n> Still not sure if this is intentional.\n> \n> > + * \\param[in] out The output stream\n> > + * \\param[in] route The V4L2Subdevice::Route\n> > + * \\return The output stream \\a out\n> > + */\n> > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Route &route)\n> > +{\n> > +\tout << route.sink << \" -> \" << route.source\n> > +\t << \" (\" << utils::hex(route.flags) << \")\";\n> > +\n> > +\treturn out;\n> > +}\n> > +\n> > /**\n> > * \\typedef V4L2Subdevice::Routing\n> > * \\brief V4L2 subdevice routing table\n> > @@ -887,10 +934,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n> > std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Routing &routing)\n> > {\n> > \tfor (const auto &[i, route] : utils::enumerate(routing)) {\n> > -\t\tout << \"[\" << i << \"] \"\n> > -\t\t << route.sink_pad << \"/\" << route.sink_stream << \" -> \"\n> > -\t\t << route.source_pad << \"/\" << route.source_stream\n> > -\t\t << \" (\" << utils::hex(route.flags) << \")\";\n> > +\t\tout << \"[\" << i << \"] \" << route;\n> > \t\tif (i != routing.size() - 1)\n> > \t\t\tout << \", \";\n> > \t}\n> > @@ -1244,6 +1288,30 @@ int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> >\n> > +namespace {\n> > +\n> > +void routeFromKernel(V4L2Subdevice::Route &route,\n> > +\t\t const struct v4l2_subdev_route &kroute)\n> > +{\n> > +\troute.sink.pad = kroute.sink_pad;\n> > +\troute.sink.stream = kroute.sink_stream;\n> > +\troute.source.pad = kroute.source_pad;\n> > +\troute.source.stream = kroute.source_stream;\n> > +\troute.flags = kroute.flags;\n> > +}\n> > +\n> > +void routeToKernel(const V4L2Subdevice::Route &route,\n> > +\t\t struct v4l2_subdev_route &kroute)\n> > +{\n> > +\tkroute.sink_pad = route.sink.pad;\n> > +\tkroute.sink_stream = route.sink.stream;\n> > +\tkroute.source_pad = route.source.pad;\n> > +\tkroute.source_stream = route.source.stream;\n> > +\tkroute.flags = route.flags;\n> > +}\n> \n> Static helpers always feel a bit meh to me.. have you considered\n> \n> Route::fromKernelRoute(const struct v4l2_subdev_route &kroute);\n> Route::toKernelRoute(struct v4l2_subdev_route *kroute);\n> \n> and decided static helpers are better ?\n\nThe idea is to make those internal, I don't think they need to be\nexposed in the API. The whole point of the Route structure is to avoid\nexposing v4l2_subdev_route :-)\n\n> > +\n> > +} /* namespace */\n> > +\n> > /**\n> > * \\brief Retrieve the subdevice's internal routing table\n> > * \\param[out] routing The routing table\n> > @@ -1254,6 +1322,8 @@ int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > */\n> > int V4L2Subdevice::getRouting(Routing *routing, Whence whence)\n> > {\n> > +\trouting->clear();\n> > +\n> > \tif (!caps_.hasStreams())\n> > \t\treturn 0;\n> >\n> > @@ -1272,8 +1342,8 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)\n> > \t\treturn ret;\n> > \t}\n> >\n> > -\trouting->resize(rt.num_routes);\n> > -\trt.routes = reinterpret_cast(routing->data());\n> > +\tstd::vector routes{ rt.num_routes };\n> > +\trt.routes = reinterpret_cast(routes.data());\n> >\n> > \tret = ioctl(VIDIOC_SUBDEV_G_ROUTING, &rt);\n> > \tif (ret) {\n> > @@ -1282,11 +1352,16 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)\n> > \t\treturn ret;\n> > \t}\n> >\n> > -\tif (rt.num_routes != routing->size()) {\n> > +\tif (rt.num_routes != routes.size()) {\n> > \t\tLOG(V4L2, Error) << \"Invalid number of routes\";\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n> > +\trouting->resize(rt.num_routes);\n> > +\n> > +\tfor (const auto &[i, route] : utils::enumerate(routes))\n> > +\t\trouteFromKernel((*routing)[i], route);\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -1304,13 +1379,20 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)\n> > */\n> > int V4L2Subdevice::setRouting(Routing *routing, Whence whence)\n> > {\n> > -\tif (!caps_.hasStreams())\n> > +\tif (!caps_.hasStreams()) {\n> > +\t\trouting->clear();\n> > \t\treturn 0;\n> > +\t}\n> > +\n> > +\tstd::vector routes{ routing->size() };\n> > +\n> > +\tfor (const auto &[i, route] : utils::enumerate(*routing))\n> > +\t\trouteToKernel(route, routes[i]);\n> >\n> > \tstruct v4l2_subdev_routing rt = {};\n> > \trt.which = whence;\n> > -\trt.num_routes = routing->size();\n> > -\trt.routes = reinterpret_cast(routing->data());\n> > +\trt.num_routes = routes.size();\n> > +\trt.routes = reinterpret_cast(routes.data());\n> >\n> > \tint ret = ioctl(VIDIOC_SUBDEV_S_ROUTING, &rt);\n> > \tif (ret) {\n> > @@ -1318,8 +1400,12 @@ int V4L2Subdevice::setRouting(Routing *routing, Whence whence)\n> > \t\treturn ret;\n> > \t}\n> >\n> > +\troutes.resize(rt.num_routes);\n> > \trouting->resize(rt.num_routes);\n> >\n> > +\tfor (const auto &[i, route] : utils::enumerate(routes))\n> > +\t\trouteFromKernel((*routing)[i], route);\n> > +\n> > \treturn 0;\n> \n> With or without the static helpers\n> Reviewed-by: Jacopo Mondi \n> \n> > }\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1ECF9BD160\n\tfor ;\n\tTue, 5 Mar 2024 09:21:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67A4A62872;\n\tTue, 5 Mar 2024 10:21:22 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1FF4B62865\n\tfor ;\n\tTue, 5 Mar 2024 10:21:21 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E97A01225;\n\tTue, 5 Mar 2024 10:21:03 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qBqiHm84\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709630464;\n\tbh=bt4VfX9n67l2tRG2BlSfikSVBXkegOjci2TO/mjr3uw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qBqiHm84GeXcbiRjU4wi6NFD+cUB00UnAVQY2o1JZh6806erP215NUQ8u9DVc26qe\n\t1iYAL9ARGtcAjIMRwUVWCI2Pc6juj1RoTLS2pCliP3KEBLsDpsg9C5wJqPKwg+R1hC\n\trhM0lu4bLLUBF6dsvtCbN25moPRBK/2fYcVp1/tM=","Date":"Tue, 5 Mar 2024 11:21:22 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Subject":"Re: [PATCH/RFC 09/32] libcamera: v4l2_subdevice: Add\n\tV4L2Subdevice::Route structure","Message-ID":"<20240305092122.GI12503@pendragon.ideasonboard.com>","References":"<20240301212121.9072-1-laurent.pinchart@ideasonboard.com>\n\t<20240301212121.9072-10-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, Sakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]