[{"id":28979,"web_url":"https://patchwork.libcamera.org/comment/28979/","msgid":"<171050041743.1011926.146866331932751931@ping.linuxembedded.co.uk>","date":"2024-03-15T11:00:17","subject":"Re: [PATCH v2 09/14] libcamera: v4l2_subdevice: Add\n\tV4L2Subdevice::Route structure","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-03-15 00:16:08)\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 <laurent.pinchart@ideasonboard.com>\n> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n\nI got to the bottom without finding anything to comment on. So I guess\nthat's a \n\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n> ---\n> Changes since combined RFC:\n> \n> - Fix compilation in imx8-isi pipeline handler\n> - Fix indentation in comments\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/imx8-isi/imx8-isi.cpp |  14 +--\n>  src/libcamera/pipeline/simple/simple.cpp     |   8 +-\n>  src/libcamera/v4l2_subdevice.cpp             | 108 +++++++++++++++++--\n>  4 files changed, 123 insertions(+), 26 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>                 unsigned int stream;\n>         };\n>  \n> -       using Routing = std::vector<struct v4l2_subdev_route>;\n> +       struct Route {\n> +               Route()\n> +                       : flags(0)\n> +               {\n> +               }\n> +\n> +               Route(const Stream &snk, const Stream &src, uint32_t f)\n> +                       : sink(snk), source(src), flags(f)\n> +               {\n> +               }\n> +\n> +               Stream sink;\n> +               Stream source;\n> +               uint32_t flags;\n> +       };\n> +\n> +       using Routing = std::vector<Route>;\n>  \n>         explicit V4L2Subdevice(const MediaEntity *entity);\n>         ~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/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> index a3782aea2ba9..63082cea7e56 100644\n> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> @@ -827,16 +827,10 @@ int PipelineHandlerISI::configure(Camera *camera, CameraConfiguration *c)\n>         unsigned int xbarFirstSource = crossbar_->entity()->pads().size() / 2 + 1;\n>  \n>         for (const auto &[idx, config] : utils::enumerate(*c)) {\n> -               struct v4l2_subdev_route route = {\n> -                       .sink_pad = data->xbarSink_,\n> -                       .sink_stream = 0,\n> -                       .source_pad = static_cast<uint32_t>(xbarFirstSource + idx),\n> -                       .source_stream = 0,\n> -                       .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,\n> -                       .reserved = {}\n> -               };\n> -\n> -               routing.push_back(route);\n> +               uint32_t sourcePad = xbarFirstSource + idx;\n> +               routing.emplace_back(V4L2Subdevice::Stream{ data->xbarSink_, 0 },\n> +                                    V4L2Subdevice::Stream{ sourcePad, 0 },\n> +                                    V4L2_SUBDEV_ROUTE_FL_ACTIVE);\n>         }\n>  \n>         int ret = crossbar_->setRouting(&routing, V4L2Subdevice::ActiveFormat);\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<const MediaPad *> SimpleCameraData::routedSourcePads(MediaPad *sink)\n>  \n>         std::vector<const MediaPad *> pads;\n>  \n> -       for (const struct v4l2_subdev_route &route : routing) {\n> -               if (sink->index() != route.sink_pad ||\n> +       for (const V4L2Subdevice::Route &route : routing) {\n> +               if (sink->index() != route.sink.pad ||\n>                     !(route.flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))\n>                         continue;\n>  \n> -               const MediaPad *pad = entity->getPadByIndex(route.source_pad);\n> +               const MediaPad *pad = entity->getPadByIndex(route.source.pad);\n>                 if (!pad) {\n>                         LOG(SimplePipeline, Warning)\n>                                 << \"Entity \" << entity->name()\n>                                 << \" has invalid route source pad \"\n> -                               << route.source_pad;\n> +                               << route.source.pad;\n>                 }\n>  \n>                 pads.push_back(pad);\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index deef681e0d3a..1076b7006b0b 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -898,6 +898,53 @@ std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n>         return 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> + * output stream\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> +       out << route.sink << \" -> \" << route.source\n> +           << \" (\" << utils::hex(route.flags) << \")\";\n> +\n> +       return out;\n> +}\n> +\n>  /**\n>   * \\typedef V4L2Subdevice::Routing\n>   * \\brief V4L2 subdevice routing table\n> @@ -907,7 +954,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n>  \n>  /**\n>   * \\brief Insert a text representation of a V4L2Subdevice::Routing into an\n> - *     output stream\n> + * output stream\n>   * \\param[in] out The output stream\n>   * \\param[in] routing The V4L2Subdevice::Routing\n>   * \\return The output stream \\a out\n> @@ -915,10 +962,7 @@ std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n>  std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Routing &routing)\n>  {\n>         for (const auto &[i, route] : utils::enumerate(routing)) {\n> -               out << \"[\" << i << \"] \"\n> -                   << route.sink_pad << \"/\" << route.sink_stream << \" -> \"\n> -                   << route.source_pad << \"/\" << route.source_stream\n> -                   << \" (\" << utils::hex(route.flags) << \")\";\n> +               out << \"[\" << i << \"] \" << route;\n>                 if (i != routing.size() - 1)\n>                         out << \", \";\n>         }\n> @@ -1272,6 +1316,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> +                    const struct v4l2_subdev_route &kroute)\n> +{\n> +       route.sink.pad = kroute.sink_pad;\n> +       route.sink.stream = kroute.sink_stream;\n> +       route.source.pad = kroute.source_pad;\n> +       route.source.stream = kroute.source_stream;\n> +       route.flags = kroute.flags;\n> +}\n> +\n> +void routeToKernel(const V4L2Subdevice::Route &route,\n> +                  struct v4l2_subdev_route &kroute)\n> +{\n> +       kroute.sink_pad = route.sink.pad;\n> +       kroute.sink_stream = route.sink.stream;\n> +       kroute.source_pad = route.source.pad;\n> +       kroute.source_stream = route.source.stream;\n> +       kroute.flags = route.flags;\n> +}\n> +\n> +} /* namespace */\n> +\n>  /**\n>   * \\brief Retrieve the subdevice's internal routing table\n>   * \\param[out] routing The routing table\n> @@ -1282,6 +1350,8 @@ int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n>   */\n>  int V4L2Subdevice::getRouting(Routing *routing, Whence whence)\n>  {\n> +       routing->clear();\n> +\n>         if (!caps_.hasStreams())\n>                 return 0;\n>  \n> @@ -1300,8 +1370,8 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)\n>                 return ret;\n>         }\n>  \n> -       routing->resize(rt.num_routes);\n> -       rt.routes = reinterpret_cast<uintptr_t>(routing->data());\n> +       std::vector<struct v4l2_subdev_route> routes{ rt.num_routes };\n> +       rt.routes = reinterpret_cast<uintptr_t>(routes.data());\n>  \n>         ret = ioctl(VIDIOC_SUBDEV_G_ROUTING, &rt);\n>         if (ret) {\n> @@ -1310,11 +1380,16 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)\n>                 return ret;\n>         }\n>  \n> -       if (rt.num_routes != routing->size()) {\n> +       if (rt.num_routes != routes.size()) {\n>                 LOG(V4L2, Error) << \"Invalid number of routes\";\n>                 return -EINVAL;\n>         }\n>  \n> +       routing->resize(rt.num_routes);\n> +\n> +       for (const auto &[i, route] : utils::enumerate(routes))\n> +               routeFromKernel((*routing)[i], route);\n> +\n>         return 0;\n>  }\n>  \n> @@ -1332,13 +1407,20 @@ int V4L2Subdevice::getRouting(Routing *routing, Whence whence)\n>   */\n>  int V4L2Subdevice::setRouting(Routing *routing, Whence whence)\n>  {\n> -       if (!caps_.hasStreams())\n> +       if (!caps_.hasStreams()) {\n> +               routing->clear();\n>                 return 0;\n> +       }\n> +\n> +       std::vector<struct v4l2_subdev_route> routes{ routing->size() };\n> +\n> +       for (const auto &[i, route] : utils::enumerate(*routing))\n> +               routeToKernel(route, routes[i]);\n>  \n>         struct v4l2_subdev_routing rt = {};\n>         rt.which = whence;\n> -       rt.num_routes = routing->size();\n> -       rt.routes = reinterpret_cast<uintptr_t>(routing->data());\n> +       rt.num_routes = routes.size();\n> +       rt.routes = reinterpret_cast<uintptr_t>(routes.data());\n>  \n>         int ret = ioctl(VIDIOC_SUBDEV_S_ROUTING, &rt);\n>         if (ret) {\n> @@ -1346,8 +1428,12 @@ int V4L2Subdevice::setRouting(Routing *routing, Whence whence)\n>                 return ret;\n>         }\n>  \n> +       routes.resize(rt.num_routes);\n>         routing->resize(rt.num_routes);\n>  \n> +       for (const auto &[i, route] : utils::enumerate(routes))\n> +               routeFromKernel((*routing)[i], route);\n> +\n>         return 0;\n>  }\n>  \n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 E149BBD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Mar 2024 11:00:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C21462C8D;\n\tFri, 15 Mar 2024 12:00:21 +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 0A8D661C65\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Mar 2024 12:00:20 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 33AF5B3;\n\tFri, 15 Mar 2024 11:59:56 +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=\"vKk7d+0C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710500396;\n\tbh=9xYEw6/OfZPCwlA+fWAW3ePa2LTXFenPH8L2ujiDQvw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=vKk7d+0CmXhLmbJCeSU9JCq9cNwpZAFVDNxccB865Y8qOcv0uMCGgX/B4p/oR7YIR\n\t1Evnc7oB6bmKso+w0fWsZE8Ib+dSec+KPfqre/Ti3C0VWypVumrV1sZAO3393VCuom\n\tnygqeH6m/baiJasOT16PXa5hn6parG6mEchVj7E4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240315001613.2033-10-laurent.pinchart@ideasonboard.com>","References":"<20240315001613.2033-1-laurent.pinchart@ideasonboard.com>\n\t<20240315001613.2033-10-laurent.pinchart@ideasonboard.com>","Subject":"Re: [PATCH v2 09/14] libcamera: v4l2_subdevice: Add\n\tV4L2Subdevice::Route structure","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 15 Mar 2024 11:00:17 +0000","Message-ID":"<171050041743.1011926.146866331932751931@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]