[{"id":24273,"web_url":"https://patchwork.libcamera.org/comment/24273/","msgid":"<20220801161603.4wl4anfvk46jnlcr@uno.localdomain>","date":"2022-08-01T16:16:03","subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: simple:\n\tWalk pipeline using subdev internal routing","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Mon, Aug 01, 2022 at 03:05:41AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> From: Phi-Bang Nguyen \n>\n> When traversing the media graph to discover a pipeline from the camera\n> sensor to a video node, all sink-to-source paths inside subdevs are\n> considered. This can lead to invalid paths being followed, when a subdev\n> has restrictions on its internal routing.\n>\n> The V4L2 API supports exposing subdev internal routing to userspace.\n> Make use if this feature, when implemented by a subdev, to restrict the\n\nMake use \"of\"\n\n> internal paths to the currently active routes. If a subdev doesn't\n> implement the internal routing operations, all source pads are\n> considered, as done today.\n>\n> This change is needed to properly support multiple sensors with devices\n> such as the NXP i.MX8 ISI or the MediaTek i350 and i500 SENINF. Support\n> for changing routes dynamically will be added later when required.\n>\n> Signed-off-by: Phi-Bang Nguyen \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 73 ++++++++++++++++++++++--\n> 1 file changed, 67 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 4bde9caa7254..2a8811183907 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -100,8 +100,14 @@ LOG_DEFINE_CATEGORY(SimplePipeline)\n> *\n> * During the breadth-first search, the pipeline is traversed from entity to\n> * entity, by following media graph links from source to sink, starting at the\n> - * camera sensor. When reaching an entity (on its sink side), all its source\n> - * pads are considered to continue the graph traversal.\n> + * camera sensor.\n> + *\n> + * When reaching an entity (on its sink side), if the entity is a V4L2 subdev\n> + * that supports the streams API, the subdev internal routes are followed to\n> + * find the connected source pads. Otherwise all of the entity's source pads\n> + * are considered to continue the graph traversal. The pipeline handler\n> + * currently considers the default internal routes only and doesn't attempt to\n> + * setup custom routes. This can be extended if needed.\n> *\n> * The shortest path between the camera sensor and a video node is stored in\n> * SimpleCameraData::entities_ as a list of SimpleCameraData::Entity structures,\n> @@ -261,6 +267,7 @@ public:\n>\n> private:\n> \tvoid tryPipeline(unsigned int code, const Size &size);\n> +\tstatic std::vector routedSourcePads(MediaPad *sink);\n>\n> \tvoid converterInputDone(FrameBuffer *buffer);\n> \tvoid converterOutputDone(FrameBuffer *buffer);\n> @@ -387,12 +394,29 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,\n> \t\t\tbreak;\n> \t\t}\n>\n> -\t\t/* The actual breadth-first search algorithm. */\n> \t\tvisited.insert(entity);\n> -\t\tfor (MediaPad *pad : entity->pads()) {\n> -\t\t\tif (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> -\t\t\t\tcontinue;\n>\n> +\t\t/*\n> +\t\t * Add direct downstream entities to the search queue. If the\n> +\t\t * current entity supports the subdev internal routing API,\n> +\t\t * restrict the search to downstream entities reachable through\n> +\t\t * active routes.\n> +\t\t */\n> +\n\nIs the emtpy line necessary ?\n\n> +\t\tstd::vector pads;\n> +\n> +\t\tif (sinkPad)\n> +\t\t\tpads = routedSourcePads(sinkPad);\n> +\n> +\t\tif (pads.empty()) {\n\npads is empty also in case of errors, like failures to open the\nsubdev.\n\nIsn't it better to consider pads.emtpy and error condition, and handle\nthe !sinkPad cases in the \"routedSourcePads()\" function ?\n\n\n std::vector<> pads = connectedSources(entity, sinkPad);\n if (pads.empty()) {\n LOG(Error) ...\n return -EINVAL;\n }\n\n}\n\nstd::vector<> SimpleCameraData::nextSources(entity)\n{\n vector<> pads;\n\n for (pad : entity->pads()) {\n if (pad->flags & SINK)\n continue;\n\n pads.push_back(pad);\n }\n\n return pads;\n}\n\nstd::vector<> SimpleCameraData::connectedSources(entity, sinkPad)\n{\n if (!sinkPad)\n return nextSources(entity);\n\n ret = subdev->open();\n if (ret)\n return {};\n\n ret = subdev->getRouting();\n if (!routing)\n return nextSources(entity);\n\n ....\n}\n\n> +\t\t\tfor (const MediaPad *pad : entity->pads()) {\n> +\t\t\t\tif (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\tpads.push_back(pad);\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tfor (const MediaPad *pad : pads) {\n> \t\t\tfor (MediaLink *link : pad->links()) {\n> \t\t\t\tMediaEntity *next = link->sink()->entity();\n> \t\t\t\tif (visited.find(next) == visited.end()) {\n> @@ -782,6 +806,43 @@ void SimpleCameraData::converterOutputDone(FrameBuffer *buffer)\n> \t\tpipe->completeRequest(request);\n> }\n>\n> +/* Retrieve all source pads connected to a sink pad through active routes. */\n> +std::vector SimpleCameraData::routedSourcePads(MediaPad *sink)\n> +{\n> +\tMediaEntity *entity = sink->entity();\n> +\tstd::unique_ptr subdev =\n> +\t\tstd::make_unique(entity);\n> +\n> +\tint ret = subdev->open();\n> +\tif (ret < 0)\n> +\t\treturn {};\n> +\n> +\tV4L2Subdevice::Routing routing = {};\n> +\tret = subdev->getRouting(&routing, V4L2Subdevice::ActiveFormat);\n> +\tif (ret < 0)\n> +\t\treturn {};\n> +\n> +\tstd::vector pads;\n> +\n> +\tfor (const struct v4l2_subdev_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\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}\n> +\n> +\t\tpads.push_back(pad);\n> +\t}\n> +\n> +\treturn pads;\n> +}\n> +\n> /* -----------------------------------------------------------------------------\n> * Camera Configuration\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 000D4BE173\n\tfor ;\n\tMon, 1 Aug 2022 16:16:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4BEEB63312;\n\tMon, 1 Aug 2022 18:16:07 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3244E603E8\n\tfor ;\n\tMon, 1 Aug 2022 18:16:06 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 3A49B1C000D;\n\tMon, 1 Aug 2022 16:16:04 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659370567;\n\tbh=3E6NTK2H4JLkKQlGojJ6yckMMYUDuVMIf0A33ymSErM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=yh9BA+lN5E66575//5C5zdRMUae9UwqIYL5kq5P3RwRCvLQfQMJESpyJ35c75akkr\n\tP+XzD35JFVGuwFjTHqFBQXuCb5sAUG1yt8+8hU9FOz53QlJiFJ6rLOA/vTF8AxNqR5\n\t1kDHjk5VruzykqbLOlvr7cn2zeU5jqlVkxPiszTeriF3N4kgTLSk3i2xQ9IWRtlV8z\n\ttJA2B1wX33yFadZMpZOX/mTYi4UBJcu5NZWpxIAlzkzkKSi7AZ3UPk2ojZ/IfWqQqq\n\ts/9+5Wz/8ylD/5OAtI6NlflVwdNlxzpNZvOVDevpi0CD63xKrvsMNMDJ9LGrfZpMEx\n\th4dLA6mTRfODQ==","Date":"Mon, 1 Aug 2022 18:16:03 +0200","To":"Laurent Pinchart ","Message-ID":"<20220801161603.4wl4anfvk46jnlcr@uno.localdomain>","References":"<20220801000543.3501-1-laurent.pinchart@ideasonboard.com>\n\t<20220801000543.3501-12-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220801000543.3501-12-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: simple:\n\tWalk pipeline using subdev internal routing","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24279,"web_url":"https://patchwork.libcamera.org/comment/24279/","msgid":"","date":"2022-08-01T20:36:19","subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: simple:\n\tWalk pipeline using subdev internal routing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Aug 01, 2022 at 06:16:03PM +0200, Jacopo Mondi wrote:\n> On Mon, Aug 01, 2022 at 03:05:41AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > From: Phi-Bang Nguyen \n> >\n> > When traversing the media graph to discover a pipeline from the camera\n> > sensor to a video node, all sink-to-source paths inside subdevs are\n> > considered. This can lead to invalid paths being followed, when a subdev\n> > has restrictions on its internal routing.\n> >\n> > The V4L2 API supports exposing subdev internal routing to userspace.\n> > Make use if this feature, when implemented by a subdev, to restrict the\n> \n> Make use \"of\"\n> \n> > internal paths to the currently active routes. If a subdev doesn't\n> > implement the internal routing operations, all source pads are\n> > considered, as done today.\n> >\n> > This change is needed to properly support multiple sensors with devices\n> > such as the NXP i.MX8 ISI or the MediaTek i350 and i500 SENINF. Support\n> > for changing routes dynamically will be added later when required.\n> >\n> > Signed-off-by: Phi-Bang Nguyen \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/pipeline/simple/simple.cpp | 73 ++++++++++++++++++++++--\n> > 1 file changed, 67 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 4bde9caa7254..2a8811183907 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -100,8 +100,14 @@ LOG_DEFINE_CATEGORY(SimplePipeline)\n> > *\n> > * During the breadth-first search, the pipeline is traversed from entity to\n> > * entity, by following media graph links from source to sink, starting at the\n> > - * camera sensor. When reaching an entity (on its sink side), all its source\n> > - * pads are considered to continue the graph traversal.\n> > + * camera sensor.\n> > + *\n> > + * When reaching an entity (on its sink side), if the entity is a V4L2 subdev\n> > + * that supports the streams API, the subdev internal routes are followed to\n> > + * find the connected source pads. Otherwise all of the entity's source pads\n> > + * are considered to continue the graph traversal. The pipeline handler\n> > + * currently considers the default internal routes only and doesn't attempt to\n> > + * setup custom routes. This can be extended if needed.\n> > *\n> > * The shortest path between the camera sensor and a video node is stored in\n> > * SimpleCameraData::entities_ as a list of SimpleCameraData::Entity structures,\n> > @@ -261,6 +267,7 @@ public:\n> >\n> > private:\n> > \tvoid tryPipeline(unsigned int code, const Size &size);\n> > +\tstatic std::vector routedSourcePads(MediaPad *sink);\n> >\n> > \tvoid converterInputDone(FrameBuffer *buffer);\n> > \tvoid converterOutputDone(FrameBuffer *buffer);\n> > @@ -387,12 +394,29 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,\n> > \t\t\tbreak;\n> > \t\t}\n> >\n> > -\t\t/* The actual breadth-first search algorithm. */\n> > \t\tvisited.insert(entity);\n> > -\t\tfor (MediaPad *pad : entity->pads()) {\n> > -\t\t\tif (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > -\t\t\t\tcontinue;\n> >\n> > +\t\t/*\n> > +\t\t * Add direct downstream entities to the search queue. If the\n> > +\t\t * current entity supports the subdev internal routing API,\n> > +\t\t * restrict the search to downstream entities reachable through\n> > +\t\t * active routes.\n> > +\t\t */\n> > +\n> \n> Is the emtpy line necessary ?\n\nThe code seems to still run fine without it ;-) I'll drop it.\n\n> > +\t\tstd::vector pads;\n> > +\n> > +\t\tif (sinkPad)\n> > +\t\t\tpads = routedSourcePads(sinkPad);\n> > +\n> > +\t\tif (pads.empty()) {\n> \n> pads is empty also in case of errors, like failures to open the\n> subdev.\n\nThat's right. That's actually the only failure unrelated to routing (the\nother failure would come from subdev->getRouting(), which would just\nindicate that the API isn't supported). We could handle the open failure\nhere, but the subdevs are all opened in SimplePipelineHandler::match()\njust after constructing the SimpleCameraData instances, so we'll also\ncatch errors there. Failures to open subdevs are really not supposed to\nhappen, so as long as we handle them correctly somewhere, that's good\nenough for me I think.\n\n> Isn't it better to consider pads.emtpy and error condition, and handle\n> the !sinkPad cases in the \"routedSourcePads()\" function ?\n\nThese are two different issues. The sinkPad check is only meant to skip\nthe routedSourcePads() call for the camera sensor entity (that's the\nonly one pushed to the queue with a nullptr for the MediaPad pointer).\nThen, pads can be empty in error cases indeed, but also if the entity\ndoesn't support the routing API (not all entities do, and certainly not\non kernels that don't have the stream series applied :-)). In that case\nthe code defaults to exploring all the source pads of the entity, like\nit did before this patch.\n\nAnd now that I've written that, I see it matches the code you wrote\nbelow :-)\n\n> \n> \n> std::vector<> pads = connectedSources(entity, sinkPad);\n> if (pads.empty()) {\n> LOG(Error) ...\n> return -EINVAL;\n> }\n> \n> }\n> \n> std::vector<> SimpleCameraData::nextSources(entity)\n> {\n> vector<> pads;\n> \n> for (pad : entity->pads()) {\n> if (pad->flags & SINK)\n> continue;\n> \n> pads.push_back(pad);\n> }\n> \n> return pads;\n> }\n> \n> std::vector<> SimpleCameraData::connectedSources(entity, sinkPad)\n> {\n> if (!sinkPad)\n> return nextSources(entity);\n> \n> ret = subdev->open();\n> if (ret)\n> return {};\n> \n> ret = subdev->getRouting();\n> if (!routing)\n> return nextSources(entity);\n> \n> ....\n> }\n\nI was going to push back a bit because I'm lazy, but that looks cleaner\n:-) I'll give it a try.\n\n> > +\t\t\tfor (const MediaPad *pad : entity->pads()) {\n> > +\t\t\t\tif (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > +\t\t\t\t\tcontinue;\n> > +\t\t\t\tpads.push_back(pad);\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tfor (const MediaPad *pad : pads) {\n> > \t\t\tfor (MediaLink *link : pad->links()) {\n> > \t\t\t\tMediaEntity *next = link->sink()->entity();\n> > \t\t\t\tif (visited.find(next) == visited.end()) {\n> > @@ -782,6 +806,43 @@ void SimpleCameraData::converterOutputDone(FrameBuffer *buffer)\n> > \t\tpipe->completeRequest(request);\n> > }\n> >\n> > +/* Retrieve all source pads connected to a sink pad through active routes. */\n> > +std::vector SimpleCameraData::routedSourcePads(MediaPad *sink)\n> > +{\n> > +\tMediaEntity *entity = sink->entity();\n> > +\tstd::unique_ptr subdev =\n> > +\t\tstd::make_unique(entity);\n> > +\n> > +\tint ret = subdev->open();\n> > +\tif (ret < 0)\n> > +\t\treturn {};\n> > +\n> > +\tV4L2Subdevice::Routing routing = {};\n> > +\tret = subdev->getRouting(&routing, V4L2Subdevice::ActiveFormat);\n> > +\tif (ret < 0)\n> > +\t\treturn {};\n> > +\n> > +\tstd::vector pads;\n> > +\n> > +\tfor (const struct v4l2_subdev_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\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}\n> > +\n> > +\t\tpads.push_back(pad);\n> > +\t}\n> > +\n> > +\treturn pads;\n> > +}\n> > +\n> > /* -----------------------------------------------------------------------------\n> > * Camera Configuration\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 B42A4C3275\n\tfor ;\n\tMon, 1 Aug 2022 20:36:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0959C63312;\n\tMon, 1 Aug 2022 22:36:25 +0200 (CEST)","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 36B5D603E8\n\tfor ;\n\tMon, 1 Aug 2022 22:36:24 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AB6EA2F3;\n\tMon, 1 Aug 2022 22:36:23 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659386185;\n\tbh=SYiE9WdKMzzxrOD5ngFx5dRDTCD6d6fgGCZfUWUlRAA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=xQplTLJU3D+SJwz3rhxArCeiLs/5ae5I85Wzcl7z/CbFdwva3ecKTmYKNOtYQ49Pg\n\tkqYjvGFj5StDFtWfUrL2dbnvNwJ9yX4cvLU8WzHgtNRtozG+gJFy0NEuVIDwy3oW7B\n\tvSwwBRpf0BxRDUekGyBAgBov6LvKkcZweLXiCvGGsz/yUv0gO4EkqoHD2kUx5QkIgk\n\tQFLD65j9hSg2Pk4HRAE2vl75kE+2qm1mJSBn08Teo6LMU8vzb2WuunN3gPfSbgEObp\n\tU/FgdpHOES7Yvs90bg78VNxSj12epX7ds5ZVQnbgL65+30/1IOL/HJyEaXtHZGBw0E\n\t4jczxn4xRmh1w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659386183;\n\tbh=SYiE9WdKMzzxrOD5ngFx5dRDTCD6d6fgGCZfUWUlRAA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TwOW8o7Vq10rY+OK/QI0FZliwdib+iSzDpceDByhZcKlNeZQc3tvhJRoWiQKfP4Fe\n\t430eFndujx3MGfysJczTL8GA1ARxtTLla0OxuA+WpapWiZJO2fxf9hD+XMwWM1WDUz\n\tNIeePuokpw+A3WVd+IfMmfirfeaoSe9TjVO3T/q8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TwOW8o7V\"; dkim-atps=neutral","Date":"Mon, 1 Aug 2022 23:36:19 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220801000543.3501-1-laurent.pinchart@ideasonboard.com>\n\t<20220801000543.3501-12-laurent.pinchart@ideasonboard.com>\n\t<20220801161603.4wl4anfvk46jnlcr@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220801161603.4wl4anfvk46jnlcr@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: simple:\n\tWalk pipeline using subdev internal routing","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24280,"web_url":"https://patchwork.libcamera.org/comment/24280/","msgid":"","date":"2022-08-01T20:46:23","subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: simple:\n\tWalk pipeline using subdev internal routing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Aug 01, 2022 at 11:36:19PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> On Mon, Aug 01, 2022 at 06:16:03PM +0200, Jacopo Mondi wrote:\n> > On Mon, Aug 01, 2022 at 03:05:41AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > From: Phi-Bang Nguyen \n> > >\n> > > When traversing the media graph to discover a pipeline from the camera\n> > > sensor to a video node, all sink-to-source paths inside subdevs are\n> > > considered. This can lead to invalid paths being followed, when a subdev\n> > > has restrictions on its internal routing.\n> > >\n> > > The V4L2 API supports exposing subdev internal routing to userspace.\n> > > Make use if this feature, when implemented by a subdev, to restrict the\n> > \n> > Make use \"of\"\n> > \n> > > internal paths to the currently active routes. If a subdev doesn't\n> > > implement the internal routing operations, all source pads are\n> > > considered, as done today.\n> > >\n> > > This change is needed to properly support multiple sensors with devices\n> > > such as the NXP i.MX8 ISI or the MediaTek i350 and i500 SENINF. Support\n> > > for changing routes dynamically will be added later when required.\n> > >\n> > > Signed-off-by: Phi-Bang Nguyen \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/pipeline/simple/simple.cpp | 73 ++++++++++++++++++++++--\n> > > 1 file changed, 67 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > index 4bde9caa7254..2a8811183907 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -100,8 +100,14 @@ LOG_DEFINE_CATEGORY(SimplePipeline)\n> > > *\n> > > * During the breadth-first search, the pipeline is traversed from entity to\n> > > * entity, by following media graph links from source to sink, starting at the\n> > > - * camera sensor. When reaching an entity (on its sink side), all its source\n> > > - * pads are considered to continue the graph traversal.\n> > > + * camera sensor.\n> > > + *\n> > > + * When reaching an entity (on its sink side), if the entity is a V4L2 subdev\n> > > + * that supports the streams API, the subdev internal routes are followed to\n> > > + * find the connected source pads. Otherwise all of the entity's source pads\n> > > + * are considered to continue the graph traversal. The pipeline handler\n> > > + * currently considers the default internal routes only and doesn't attempt to\n> > > + * setup custom routes. This can be extended if needed.\n> > > *\n> > > * The shortest path between the camera sensor and a video node is stored in\n> > > * SimpleCameraData::entities_ as a list of SimpleCameraData::Entity structures,\n> > > @@ -261,6 +267,7 @@ public:\n> > >\n> > > private:\n> > > \tvoid tryPipeline(unsigned int code, const Size &size);\n> > > +\tstatic std::vector routedSourcePads(MediaPad *sink);\n> > >\n> > > \tvoid converterInputDone(FrameBuffer *buffer);\n> > > \tvoid converterOutputDone(FrameBuffer *buffer);\n> > > @@ -387,12 +394,29 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,\n> > > \t\t\tbreak;\n> > > \t\t}\n> > >\n> > > -\t\t/* The actual breadth-first search algorithm. */\n> > > \t\tvisited.insert(entity);\n> > > -\t\tfor (MediaPad *pad : entity->pads()) {\n> > > -\t\t\tif (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > > -\t\t\t\tcontinue;\n> > >\n> > > +\t\t/*\n> > > +\t\t * Add direct downstream entities to the search queue. If the\n> > > +\t\t * current entity supports the subdev internal routing API,\n> > > +\t\t * restrict the search to downstream entities reachable through\n> > > +\t\t * active routes.\n> > > +\t\t */\n> > > +\n> > \n> > Is the emtpy line necessary ?\n> \n> The code seems to still run fine without it ;-) I'll drop it.\n> \n> > > +\t\tstd::vector pads;\n> > > +\n> > > +\t\tif (sinkPad)\n> > > +\t\t\tpads = routedSourcePads(sinkPad);\n> > > +\n> > > +\t\tif (pads.empty()) {\n> > \n> > pads is empty also in case of errors, like failures to open the\n> > subdev.\n> \n> That's right. That's actually the only failure unrelated to routing (the\n> other failure would come from subdev->getRouting(), which would just\n> indicate that the API isn't supported). We could handle the open failure\n> here, but the subdevs are all opened in SimplePipelineHandler::match()\n> just after constructing the SimpleCameraData instances, so we'll also\n> catch errors there. Failures to open subdevs are really not supposed to\n> happen, so as long as we handle them correctly somewhere, that's good\n> enough for me I think.\n> \n> > Isn't it better to consider pads.emtpy and error condition, and handle\n> > the !sinkPad cases in the \"routedSourcePads()\" function ?\n> \n> These are two different issues. The sinkPad check is only meant to skip\n> the routedSourcePads() call for the camera sensor entity (that's the\n> only one pushed to the queue with a nullptr for the MediaPad pointer).\n> Then, pads can be empty in error cases indeed, but also if the entity\n> doesn't support the routing API (not all entities do, and certainly not\n> on kernels that don't have the stream series applied :-)). In that case\n> the code defaults to exploring all the source pads of the entity, like\n> it did before this patch.\n> \n> And now that I've written that, I see it matches the code you wrote\n> below :-)\n> \n> > \n> > \n> > std::vector<> pads = connectedSources(entity, sinkPad);\n> > if (pads.empty()) {\n> > LOG(Error) ...\n> > return -EINVAL;\n> > }\n> > \n> > }\n> > \n> > std::vector<> SimpleCameraData::nextSources(entity)\n> > {\n> > vector<> pads;\n> > \n> > for (pad : entity->pads()) {\n> > if (pad->flags & SINK)\n> > continue;\n> > \n> > pads.push_back(pad);\n> > }\n> > \n> > return pads;\n> > }\n> > \n> > std::vector<> SimpleCameraData::connectedSources(entity, sinkPad)\n> > {\n> > if (!sinkPad)\n> > return nextSources(entity);\n> > \n> > ret = subdev->open();\n> > if (ret)\n> > return {};\n> > \n> > ret = subdev->getRouting();\n> > if (!routing)\n> > return nextSources(entity);\n> > \n> > ....\n> > }\n> \n> I was going to push back a bit because I'm lazy, but that looks cleaner\n> :-) I'll give it a try.\n\nThat's actually not going to work nicely. If for any reason there's no\nconnected source pad, we shouldn't fail, but keep walking the graph\nthrough all the other links to find a suitable path.\n\nWould you be OK keeping this patch as-is ?\n\n> > > +\t\t\tfor (const MediaPad *pad : entity->pads()) {\n> > > +\t\t\t\tif (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > > +\t\t\t\t\tcontinue;\n> > > +\t\t\t\tpads.push_back(pad);\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\n> > > +\t\tfor (const MediaPad *pad : pads) {\n> > > \t\t\tfor (MediaLink *link : pad->links()) {\n> > > \t\t\t\tMediaEntity *next = link->sink()->entity();\n> > > \t\t\t\tif (visited.find(next) == visited.end()) {\n> > > @@ -782,6 +806,43 @@ void SimpleCameraData::converterOutputDone(FrameBuffer *buffer)\n> > > \t\tpipe->completeRequest(request);\n> > > }\n> > >\n> > > +/* Retrieve all source pads connected to a sink pad through active routes. */\n> > > +std::vector SimpleCameraData::routedSourcePads(MediaPad *sink)\n> > > +{\n> > > +\tMediaEntity *entity = sink->entity();\n> > > +\tstd::unique_ptr subdev =\n> > > +\t\tstd::make_unique(entity);\n> > > +\n> > > +\tint ret = subdev->open();\n> > > +\tif (ret < 0)\n> > > +\t\treturn {};\n> > > +\n> > > +\tV4L2Subdevice::Routing routing = {};\n> > > +\tret = subdev->getRouting(&routing, V4L2Subdevice::ActiveFormat);\n> > > +\tif (ret < 0)\n> > > +\t\treturn {};\n> > > +\n> > > +\tstd::vector pads;\n> > > +\n> > > +\tfor (const struct v4l2_subdev_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\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}\n> > > +\n> > > +\t\tpads.push_back(pad);\n> > > +\t}\n> > > +\n> > > +\treturn pads;\n> > > +}\n> > > +\n> > > /* -----------------------------------------------------------------------------\n> > > * Camera Configuration\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 083F3BE173\n\tfor ;\n\tMon, 1 Aug 2022 20:46:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 88AFE63312;\n\tMon, 1 Aug 2022 22:46:29 +0200 (CEST)","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 87E6B603E8\n\tfor ;\n\tMon, 1 Aug 2022 22:46:28 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F15582F3;\n\tMon, 1 Aug 2022 22:46:27 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659386789;\n\tbh=RWGOPg9msDvRzkI9FHzrzlD06LVTb6B9xdzTuzuQNyU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=JT364n7/l3o6QrhW4aelK+hCyXot/f+Jckvt8/s/NKfApGd8bIW2c79EtVLlnewA8\n\tJRMZSLk5i/77qdSt4IuLYgsqqt9XOqNyr2OKQ+20D9HaCFLtTev0kKBuQmZpjbYJLf\n\ta/hIsuWKbuF4VATedeo5wO5EWGMnZ4nqioOLXAhjLzPU/zgVWqjXeyUNaLsAWG42kz\n\tVjrx0+UzKYJQcndnzcLrTlkXHhlj/EiGDqlH7Qac1W5l+T7n0mY7KO+60GUTxCBBP/\n\te9IR4xuTFNhFZOE0Zuywi+0KnXFMv/c+X+WIv+26AW02A3UXtxZkueNhV/PnDt9UOl\n\tO0Vhko3xx//XQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659386788;\n\tbh=RWGOPg9msDvRzkI9FHzrzlD06LVTb6B9xdzTuzuQNyU=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=pI2om8sBKc0nxB8VTgW1y2ghUQ/5Hbh2KP0J3qscK0Faj4LPbuVjLu1JeLqXc6mdp\n\tIU2BTI2WL7MHezIMpkGLnMa1JwS+YKg5bBZvTuj9CttgX8RGFLiiJwstWtjQU9qd1H\n\tF2P/RRmvP15eBDE9rV4bWQO1m+I9me5YSh88CiUA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"pI2om8sB\"; dkim-atps=neutral","Date":"Mon, 1 Aug 2022 23:46:23 +0300","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"","References":"<20220801000543.3501-1-laurent.pinchart@ideasonboard.com>\n\t<20220801000543.3501-12-laurent.pinchart@ideasonboard.com>\n\t<20220801161603.4wl4anfvk46jnlcr@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: simple:\n\tWalk pipeline using subdev internal routing","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24287,"web_url":"https://patchwork.libcamera.org/comment/24287/","msgid":"<20220802074830.ztmfxchxi4dcd2vq@uno.localdomain>","date":"2022-08-02T07:48:30","subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: simple:\n\tWalk pipeline using subdev internal routing","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Aug 01, 2022 at 11:46:23PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Aug 01, 2022 at 11:36:19PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > On Mon, Aug 01, 2022 at 06:16:03PM +0200, Jacopo Mondi wrote:\n> > > On Mon, Aug 01, 2022 at 03:05:41AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > From: Phi-Bang Nguyen \n> > > >\n> > > > When traversing the media graph to discover a pipeline from the camera\n> > > > sensor to a video node, all sink-to-source paths inside subdevs are\n> > > > considered. This can lead to invalid paths being followed, when a subdev\n> > > > has restrictions on its internal routing.\n> > > >\n> > > > The V4L2 API supports exposing subdev internal routing to userspace.\n> > > > Make use if this feature, when implemented by a subdev, to restrict the\n> > >\n> > > Make use \"of\"\n> > >\n> > > > internal paths to the currently active routes. If a subdev doesn't\n> > > > implement the internal routing operations, all source pads are\n> > > > considered, as done today.\n> > > >\n> > > > This change is needed to properly support multiple sensors with devices\n> > > > such as the NXP i.MX8 ISI or the MediaTek i350 and i500 SENINF. Support\n> > > > for changing routes dynamically will be added later when required.\n> > > >\n> > > > Signed-off-by: Phi-Bang Nguyen \n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/libcamera/pipeline/simple/simple.cpp | 73 ++++++++++++++++++++++--\n> > > > 1 file changed, 67 insertions(+), 6 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > > index 4bde9caa7254..2a8811183907 100644\n> > > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > > @@ -100,8 +100,14 @@ LOG_DEFINE_CATEGORY(SimplePipeline)\n> > > > *\n> > > > * During the breadth-first search, the pipeline is traversed from entity to\n> > > > * entity, by following media graph links from source to sink, starting at the\n> > > > - * camera sensor. When reaching an entity (on its sink side), all its source\n> > > > - * pads are considered to continue the graph traversal.\n> > > > + * camera sensor.\n> > > > + *\n> > > > + * When reaching an entity (on its sink side), if the entity is a V4L2 subdev\n> > > > + * that supports the streams API, the subdev internal routes are followed to\n> > > > + * find the connected source pads. Otherwise all of the entity's source pads\n> > > > + * are considered to continue the graph traversal. The pipeline handler\n> > > > + * currently considers the default internal routes only and doesn't attempt to\n> > > > + * setup custom routes. This can be extended if needed.\n> > > > *\n> > > > * The shortest path between the camera sensor and a video node is stored in\n> > > > * SimpleCameraData::entities_ as a list of SimpleCameraData::Entity structures,\n> > > > @@ -261,6 +267,7 @@ public:\n> > > >\n> > > > private:\n> > > > \tvoid tryPipeline(unsigned int code, const Size &size);\n> > > > +\tstatic std::vector routedSourcePads(MediaPad *sink);\n> > > >\n> > > > \tvoid converterInputDone(FrameBuffer *buffer);\n> > > > \tvoid converterOutputDone(FrameBuffer *buffer);\n> > > > @@ -387,12 +394,29 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,\n> > > > \t\t\tbreak;\n> > > > \t\t}\n> > > >\n> > > > -\t\t/* The actual breadth-first search algorithm. */\n> > > > \t\tvisited.insert(entity);\n> > > > -\t\tfor (MediaPad *pad : entity->pads()) {\n> > > > -\t\t\tif (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > > > -\t\t\t\tcontinue;\n> > > >\n> > > > +\t\t/*\n> > > > +\t\t * Add direct downstream entities to the search queue. If the\n> > > > +\t\t * current entity supports the subdev internal routing API,\n> > > > +\t\t * restrict the search to downstream entities reachable through\n> > > > +\t\t * active routes.\n> > > > +\t\t */\n> > > > +\n> > >\n> > > Is the emtpy line necessary ?\n> >\n> > The code seems to still run fine without it ;-) I'll drop it.\n> >\n> > > > +\t\tstd::vector pads;\n> > > > +\n> > > > +\t\tif (sinkPad)\n> > > > +\t\t\tpads = routedSourcePads(sinkPad);\n> > > > +\n> > > > +\t\tif (pads.empty()) {\n> > >\n> > > pads is empty also in case of errors, like failures to open the\n> > > subdev.\n> >\n> > That's right. That's actually the only failure unrelated to routing (the\n> > other failure would come from subdev->getRouting(), which would just\n> > indicate that the API isn't supported). We could handle the open failure\n> > here, but the subdevs are all opened in SimplePipelineHandler::match()\n> > just after constructing the SimpleCameraData instances, so we'll also\n> > catch errors there. Failures to open subdevs are really not supposed to\n> > happen, so as long as we handle them correctly somewhere, that's good\n> > enough for me I think.\n> >\n> > > Isn't it better to consider pads.emtpy and error condition, and handle\n> > > the !sinkPad cases in the \"routedSourcePads()\" function ?\n> >\n> > These are two different issues. The sinkPad check is only meant to skip\n> > the routedSourcePads() call for the camera sensor entity (that's the\n> > only one pushed to the queue with a nullptr for the MediaPad pointer).\n> > Then, pads can be empty in error cases indeed, but also if the entity\n> > doesn't support the routing API (not all entities do, and certainly not\n> > on kernels that don't have the stream series applied :-)). In that case\n> > the code defaults to exploring all the source pads of the entity, like\n> > it did before this patch.\n> >\n> > And now that I've written that, I see it matches the code you wrote\n> > below :-)\n> >\n> > >\n> > >\n> > > std::vector<> pads = connectedSources(entity, sinkPad);\n> > > if (pads.empty()) {\n> > > LOG(Error) ...\n> > > return -EINVAL;\n> > > }\n> > >\n> > > }\n> > >\n> > > std::vector<> SimpleCameraData::nextSources(entity)\n> > > {\n> > > vector<> pads;\n> > >\n> > > for (pad : entity->pads()) {\n> > > if (pad->flags & SINK)\n> > > continue;\n> > >\n> > > pads.push_back(pad);\n> > > }\n> > >\n> > > return pads;\n> > > }\n> > >\n> > > std::vector<> SimpleCameraData::connectedSources(entity, sinkPad)\n> > > {\n> > > if (!sinkPad)\n> > > return nextSources(entity);\n> > >\n> > > ret = subdev->open();\n> > > if (ret)\n> > > return {};\n> > >\n> > > ret = subdev->getRouting();\n> > > if (!routing)\n> > > return nextSources(entity);\n> > >\n> > > ....\n> > > }\n> >\n> > I was going to push back a bit because I'm lazy, but that looks cleaner\n> > :-) I'll give it a try.\n>\n> That's actually not going to work nicely. If for any reason there's no\n> connected source pad, we shouldn't fail, but keep walking the graph\n> through all the other links to find a suitable path.\n>\n\nconnected or routed ?\nAnyway, I see your point, it would add yet another case to handle and\nthe function would grow a bit too much ?\n\n> Would you be OK keeping this patch as-is ?\n>\n\nOK\nReviewed-by: Jacopo Mondi \n\n> > > > +\t\t\tfor (const MediaPad *pad : entity->pads()) {\n> > > > +\t\t\t\tif (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > > > +\t\t\t\t\tcontinue;\n> > > > +\t\t\t\tpads.push_back(pad);\n> > > > +\t\t\t}\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tfor (const MediaPad *pad : pads) {\n> > > > \t\t\tfor (MediaLink *link : pad->links()) {\n> > > > \t\t\t\tMediaEntity *next = link->sink()->entity();\n> > > > \t\t\t\tif (visited.find(next) == visited.end()) {\n> > > > @@ -782,6 +806,43 @@ void SimpleCameraData::converterOutputDone(FrameBuffer *buffer)\n> > > > \t\tpipe->completeRequest(request);\n> > > > }\n> > > >\n> > > > +/* Retrieve all source pads connected to a sink pad through active routes. */\n> > > > +std::vector SimpleCameraData::routedSourcePads(MediaPad *sink)\n> > > > +{\n> > > > +\tMediaEntity *entity = sink->entity();\n> > > > +\tstd::unique_ptr subdev =\n> > > > +\t\tstd::make_unique(entity);\n> > > > +\n> > > > +\tint ret = subdev->open();\n> > > > +\tif (ret < 0)\n> > > > +\t\treturn {};\n> > > > +\n> > > > +\tV4L2Subdevice::Routing routing = {};\n> > > > +\tret = subdev->getRouting(&routing, V4L2Subdevice::ActiveFormat);\n> > > > +\tif (ret < 0)\n> > > > +\t\treturn {};\n> > > > +\n> > > > +\tstd::vector pads;\n> > > > +\n> > > > +\tfor (const struct v4l2_subdev_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\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}\n> > > > +\n> > > > +\t\tpads.push_back(pad);\n> > > > +\t}\n> > > > +\n> > > > +\treturn pads;\n> > > > +}\n> > > > +\n> > > > /* -----------------------------------------------------------------------------\n> > > > * Camera Configuration\n> > > > */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 D0702C3275\n\tfor ;\n\tTue, 2 Aug 2022 07:48:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 489DC6330F;\n\tTue, 2 Aug 2022 09:48:35 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 629AF6330D\n\tfor ;\n\tTue, 2 Aug 2022 09:48:33 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 5804E240006;\n\tTue, 2 Aug 2022 07:48:32 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659426515;\n\tbh=zmIi8r8x7OnSW1DcA9Sbn+2OGHNbx6rPJlxlmhUHqLQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ZjQZIoy6lxtFyoG0NjfWExU+JBmx/ilrhfWYjlkrqIpGtYPLD7JRdWVsnws0X35Bx\n\t6FEhYisZw8FYNQnwR5gVhQps0PHeS2YXD6DACfwV83hb4Oa+M6lPmxaUEMGzKMtMyd\n\tzqx/0hAM+j3EOVr8mxkeyhx8dJ6qCTVZP8kfTw9c8lYd+s7ufT53PvdLlTx18nmfMd\n\tN8x1OnX6WqDaxiw3iEcmbnl6h9SjTIU062l8PRX2upgFsTlKsdU2a05dRqQliBtyU+\n\tEXiBIf1XNzuhB3pIRdraUFPfe+x9zW9g8nMXjrvZXHpMo/mPJxuJ+LjMv8yti4j73p\n\tPDUB/jswJ96FQ==","Date":"Tue, 2 Aug 2022 09:48:30 +0200","To":"Laurent Pinchart ","Message-ID":"<20220802074830.ztmfxchxi4dcd2vq@uno.localdomain>","References":"<20220801000543.3501-1-laurent.pinchart@ideasonboard.com>\n\t<20220801000543.3501-12-laurent.pinchart@ideasonboard.com>\n\t<20220801161603.4wl4anfvk46jnlcr@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: simple:\n\tWalk pipeline using subdev internal routing","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24294,"web_url":"https://patchwork.libcamera.org/comment/24294/","msgid":"","date":"2022-08-02T10:24:17","subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: simple:\n\tWalk pipeline using subdev internal routing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Aug 02, 2022 at 09:48:30AM +0200, Jacopo Mondi wrote:\n> On Mon, Aug 01, 2022 at 11:46:23PM +0300, Laurent Pinchart wrote:\n> > On Mon, Aug 01, 2022 at 11:36:19PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > On Mon, Aug 01, 2022 at 06:16:03PM +0200, Jacopo Mondi wrote:\n> > > > On Mon, Aug 01, 2022 at 03:05:41AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > > > > From: Phi-Bang Nguyen \n> > > > >\n> > > > > When traversing the media graph to discover a pipeline from the camera\n> > > > > sensor to a video node, all sink-to-source paths inside subdevs are\n> > > > > considered. This can lead to invalid paths being followed, when a subdev\n> > > > > has restrictions on its internal routing.\n> > > > >\n> > > > > The V4L2 API supports exposing subdev internal routing to userspace.\n> > > > > Make use if this feature, when implemented by a subdev, to restrict the\n> > > >\n> > > > Make use \"of\"\n> > > >\n> > > > > internal paths to the currently active routes. If a subdev doesn't\n> > > > > implement the internal routing operations, all source pads are\n> > > > > considered, as done today.\n> > > > >\n> > > > > This change is needed to properly support multiple sensors with devices\n> > > > > such as the NXP i.MX8 ISI or the MediaTek i350 and i500 SENINF. Support\n> > > > > for changing routes dynamically will be added later when required.\n> > > > >\n> > > > > Signed-off-by: Phi-Bang Nguyen \n> > > > > Signed-off-by: Laurent Pinchart \n> > > > > ---\n> > > > > src/libcamera/pipeline/simple/simple.cpp | 73 ++++++++++++++++++++++--\n> > > > > 1 file changed, 67 insertions(+), 6 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > > > index 4bde9caa7254..2a8811183907 100644\n> > > > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > > > @@ -100,8 +100,14 @@ LOG_DEFINE_CATEGORY(SimplePipeline)\n> > > > > *\n> > > > > * During the breadth-first search, the pipeline is traversed from entity to\n> > > > > * entity, by following media graph links from source to sink, starting at the\n> > > > > - * camera sensor. When reaching an entity (on its sink side), all its source\n> > > > > - * pads are considered to continue the graph traversal.\n> > > > > + * camera sensor.\n> > > > > + *\n> > > > > + * When reaching an entity (on its sink side), if the entity is a V4L2 subdev\n> > > > > + * that supports the streams API, the subdev internal routes are followed to\n> > > > > + * find the connected source pads. Otherwise all of the entity's source pads\n> > > > > + * are considered to continue the graph traversal. The pipeline handler\n> > > > > + * currently considers the default internal routes only and doesn't attempt to\n> > > > > + * setup custom routes. This can be extended if needed.\n> > > > > *\n> > > > > * The shortest path between the camera sensor and a video node is stored in\n> > > > > * SimpleCameraData::entities_ as a list of SimpleCameraData::Entity structures,\n> > > > > @@ -261,6 +267,7 @@ public:\n> > > > >\n> > > > > private:\n> > > > > \tvoid tryPipeline(unsigned int code, const Size &size);\n> > > > > +\tstatic std::vector routedSourcePads(MediaPad *sink);\n> > > > >\n> > > > > \tvoid converterInputDone(FrameBuffer *buffer);\n> > > > > \tvoid converterOutputDone(FrameBuffer *buffer);\n> > > > > @@ -387,12 +394,29 @@ SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,\n> > > > > \t\t\tbreak;\n> > > > > \t\t}\n> > > > >\n> > > > > -\t\t/* The actual breadth-first search algorithm. */\n> > > > > \t\tvisited.insert(entity);\n> > > > > -\t\tfor (MediaPad *pad : entity->pads()) {\n> > > > > -\t\t\tif (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > > > > -\t\t\t\tcontinue;\n> > > > >\n> > > > > +\t\t/*\n> > > > > +\t\t * Add direct downstream entities to the search queue. If the\n> > > > > +\t\t * current entity supports the subdev internal routing API,\n> > > > > +\t\t * restrict the search to downstream entities reachable through\n> > > > > +\t\t * active routes.\n> > > > > +\t\t */\n> > > > > +\n> > > >\n> > > > Is the emtpy line necessary ?\n> > >\n> > > The code seems to still run fine without it ;-) I'll drop it.\n> > >\n> > > > > +\t\tstd::vector pads;\n> > > > > +\n> > > > > +\t\tif (sinkPad)\n> > > > > +\t\t\tpads = routedSourcePads(sinkPad);\n> > > > > +\n> > > > > +\t\tif (pads.empty()) {\n> > > >\n> > > > pads is empty also in case of errors, like failures to open the\n> > > > subdev.\n> > >\n> > > That's right. That's actually the only failure unrelated to routing (the\n> > > other failure would come from subdev->getRouting(), which would just\n> > > indicate that the API isn't supported). We could handle the open failure\n> > > here, but the subdevs are all opened in SimplePipelineHandler::match()\n> > > just after constructing the SimpleCameraData instances, so we'll also\n> > > catch errors there. Failures to open subdevs are really not supposed to\n> > > happen, so as long as we handle them correctly somewhere, that's good\n> > > enough for me I think.\n> > >\n> > > > Isn't it better to consider pads.emtpy and error condition, and handle\n> > > > the !sinkPad cases in the \"routedSourcePads()\" function ?\n> > >\n> > > These are two different issues. The sinkPad check is only meant to skip\n> > > the routedSourcePads() call for the camera sensor entity (that's the\n> > > only one pushed to the queue with a nullptr for the MediaPad pointer).\n> > > Then, pads can be empty in error cases indeed, but also if the entity\n> > > doesn't support the routing API (not all entities do, and certainly not\n> > > on kernels that don't have the stream series applied :-)). In that case\n> > > the code defaults to exploring all the source pads of the entity, like\n> > > it did before this patch.\n> > >\n> > > And now that I've written that, I see it matches the code you wrote\n> > > below :-)\n> > >\n> > > >\n> > > >\n> > > > std::vector<> pads = connectedSources(entity, sinkPad);\n> > > > if (pads.empty()) {\n> > > > LOG(Error) ...\n> > > > return -EINVAL;\n> > > > }\n> > > >\n> > > > }\n> > > >\n> > > > std::vector<> SimpleCameraData::nextSources(entity)\n> > > > {\n> > > > vector<> pads;\n> > > >\n> > > > for (pad : entity->pads()) {\n> > > > if (pad->flags & SINK)\n> > > > continue;\n> > > >\n> > > > pads.push_back(pad);\n> > > > }\n> > > >\n> > > > return pads;\n> > > > }\n> > > >\n> > > > std::vector<> SimpleCameraData::connectedSources(entity, sinkPad)\n> > > > {\n> > > > if (!sinkPad)\n> > > > return nextSources(entity);\n> > > >\n> > > > ret = subdev->open();\n> > > > if (ret)\n> > > > return {};\n> > > >\n> > > > ret = subdev->getRouting();\n> > > > if (!routing)\n> > > > return nextSources(entity);\n> > > >\n> > > > ....\n> > > > }\n> > >\n> > > I was going to push back a bit because I'm lazy, but that looks cleaner\n> > > :-) I'll give it a try.\n> >\n> > That's actually not going to work nicely. If for any reason there's no\n> > connected source pad, we shouldn't fail, but keep walking the graph\n> > through all the other links to find a suitable path.\n> \n> connected or routed ?\n\nI meant routed, sorry.\n\n> Anyway, I see your point, it would add yet another case to handle and\n> the function would grow a bit too much ?\n\nMy point was that when there's no connected pad, the function can only\nreturn an empty vector, and that's not an error. If we wanted to add\nerror handling here, we would need to return both an error code and a\nvector (with std::tuple, std::variant, ...).\n\n> > Would you be OK keeping this patch as-is ?\n> \n> OK\n> Reviewed-by: Jacopo Mondi \n> \n> > > > > +\t\t\tfor (const MediaPad *pad : entity->pads()) {\n> > > > > +\t\t\t\tif (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > > > > +\t\t\t\t\tcontinue;\n> > > > > +\t\t\t\tpads.push_back(pad);\n> > > > > +\t\t\t}\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tfor (const MediaPad *pad : pads) {\n> > > > > \t\t\tfor (MediaLink *link : pad->links()) {\n> > > > > \t\t\t\tMediaEntity *next = link->sink()->entity();\n> > > > > \t\t\t\tif (visited.find(next) == visited.end()) {\n> > > > > @@ -782,6 +806,43 @@ void SimpleCameraData::converterOutputDone(FrameBuffer *buffer)\n> > > > > \t\tpipe->completeRequest(request);\n> > > > > }\n> > > > >\n> > > > > +/* Retrieve all source pads connected to a sink pad through active routes. */\n> > > > > +std::vector SimpleCameraData::routedSourcePads(MediaPad *sink)\n> > > > > +{\n> > > > > +\tMediaEntity *entity = sink->entity();\n> > > > > +\tstd::unique_ptr subdev =\n> > > > > +\t\tstd::make_unique(entity);\n> > > > > +\n> > > > > +\tint ret = subdev->open();\n> > > > > +\tif (ret < 0)\n> > > > > +\t\treturn {};\n> > > > > +\n> > > > > +\tV4L2Subdevice::Routing routing = {};\n> > > > > +\tret = subdev->getRouting(&routing, V4L2Subdevice::ActiveFormat);\n> > > > > +\tif (ret < 0)\n> > > > > +\t\treturn {};\n> > > > > +\n> > > > > +\tstd::vector pads;\n> > > > > +\n> > > > > +\tfor (const struct v4l2_subdev_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\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}\n> > > > > +\n> > > > > +\t\tpads.push_back(pad);\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn pads;\n> > > > > +}\n> > > > > +\n> > > > > /* -----------------------------------------------------------------------------\n> > > > > * Camera Configuration\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 54883BE173\n\tfor ;\n\tTue, 2 Aug 2022 10:24:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C922D63314;\n\tTue, 2 Aug 2022 12:24:23 +0200 (CEST)","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 50C6C603E7\n\tfor ;\n\tTue, 2 Aug 2022 12:24:22 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BF8C025B;\n\tTue, 2 Aug 2022 12:24:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659435863;\n\tbh=RM6/cM8NPCkzCt+EikL9YvbtFixk4FP/kJ/FpSs1I9c=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=fpkr8jXS9hVRuGvMylpPvHoG4tE1Mc3sdzOCSsTyPlMASnnCXfXqIehkIUm433Hgp\n\tQjgf7UMcxS25jTzDHaQCJ3165601lP5AjL3pIaPfubqKSX0X6yEwfHp8Tlvq8EWPUc\n\tMc4FbZaYZ2Ing0PRRZ8yYGvefS58a4eshwR8Axc5QoZlPX+kQ6Kgli57+2LN7n+uEY\n\th5F3gXjPYQyf61dySBpSFAhnSviDCmv8cLNqaIdxI3XMeeZkl1QgjMFZZZzb4VKAJA\n\thhtRyy/BqaCJ6xml54F158vx+W/W1zZQUWJQ0J+ea3VLgSkySlQnd4oa0grMRHrK/k\n\t8IhjyUaKbBOHw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659435861;\n\tbh=RM6/cM8NPCkzCt+EikL9YvbtFixk4FP/kJ/FpSs1I9c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jk+yWtQLE3o6yYyAtvpKg7E0JLXmi2kqQxBWuz+mG5om0fmjsMpPV4yj1d0tpfqDM\n\t7XN/tbxCR51BZxYJnLp3y+4u10Qw6Xx4WY7Q57HaxfPwZIuQgMmomc12XLB1da5i1J\n\tHWaW5RWPmHTbXrkKosVMDfvfsgCn3KVw9j7HJ6J8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jk+yWtQL\"; dkim-atps=neutral","Date":"Tue, 2 Aug 2022 13:24:17 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220801000543.3501-1-laurent.pinchart@ideasonboard.com>\n\t<20220801000543.3501-12-laurent.pinchart@ideasonboard.com>\n\t<20220801161603.4wl4anfvk46jnlcr@uno.localdomain>\n\t\n\t\n\t<20220802074830.ztmfxchxi4dcd2vq@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220802074830.ztmfxchxi4dcd2vq@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 11/13] libcamera: pipeline: simple:\n\tWalk pipeline using subdev internal routing","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]