[{"id":22080,"web_url":"https://patchwork.libcamera.org/comment/22080/","msgid":"<164367477725.115113.16564494473886510141@Monstersaurus>","date":"2022-02-01T00:19:37","subject":"Re: [libcamera-devel] [RFC v1 1/2] libcamera: media_device: Add\n\tenumerateMediaWalks() helper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2022-01-13 10:25:28)\n> Add a new helper function to the MediaDevice class which returns a vector of\n> all possible media device topologies starting at the sensor entity and walking\n> to an endpoint for the media device. Each of these topologies is called a\n> MediaWalk. For each entity in a MediaWalk, we store a MediaEntity pointer\n> together with the source and sink MediaLink pointers.\n> \n> As an example, consider the media graph below:\n> \n> +----------+\n> | CSI2 |\n> +-----^----+\n> |\n> +---+---+\n> | Mux1 <-------+\n> +--^----+ |\n> | |\n> +-----+---+ +---+---+\n> | Sensor1 | | Mux2 |<--+\n> +---------+ +-^-----+ |\n> | |\n> +-------+-+ +---+-----+\n> | Sensor2 | | Sensor3 |\n> +---------+ +---------+\n> \n> This would return three MediaDevice::MediaWalk structures looking like:\n> \n> 1)\n> +----------+\n> | CSI2 |\n> +-----^----+\n> |\n> +-----+----+\n> | Mux1 |\n> +-----^----+\n> |\n> +-----+----+\n> | Sensor1 |\n> +----------+\n> \n> 2)\n> +----------+\n> | CSI2 |\n> +-----^----+\n> |\n> +-----+----+\n> | Mux1 |\n> +-----^----+\n> |\n> +-----+----+\n> | Mux2 |\n> +-----^----+\n> |\n> +-----+----+\n> | Sensor2 |\n> +----------+\n> \n> 3)\n> +----------+\n> | CSI2 |\n> +-----^----+\n> |\n> +-----+----+\n> | Mux1 |\n> +-----^----+\n> |\n> +-----+----+\n> | Mux2 |\n> +-----^----+\n> |\n> +-----+----+\n> | Sensor3 |\n> +----------+\n> \n\nI think this fits and would be easier / clearer to see.\n\n 1) 2) 3)\n +----------+ +----------+ +----------+\n | CSI2 | | CSI2 | | CSI2 |\n +-----^----+ +-----^----+ +-----^----+\n | | |\n +-----+----+ +-----+----+ +-----+----+\n | Mux1 | | Mux1 | | Mux1 |\n +-----^----+ +-----^----+ +-----^----+\n | | |\n +-----+----+ +-----+----+ +-----+----+\n | Sensor1 | | Mux2 | | Mux2 |\n +----------+ +-----^----+ +-----^----+\n | |\n +-----+----+ +-----+----+\n | Sensor2 | | Sensor3 |\n +----------+ +----------+\n\n*formatted with https://asciiflow.com/#/\n\n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/internal/media_device.h | 12 ++\n> src/libcamera/media_device.cpp | 135 ++++++++++++++++++++++\n> 2 files changed, 147 insertions(+)\n> \n> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> index 6e2a63f38229..63fcbe423eb9 100644\n> --- a/include/libcamera/internal/media_device.h\n> +++ b/include/libcamera/internal/media_device.h\n> @@ -25,6 +25,13 @@ namespace libcamera {\n> class MediaDevice : protected Loggable\n> {\n> public:\n> + struct EntityParams {\n> + MediaEntity *entity;\n> + MediaLink *sinkLink;\n> + MediaLink *sourceLink;\n> + };\n> + using MediaWalk = std::vector;\n> +\n> MediaDevice(const std::string &deviceNode);\n> ~MediaDevice();\n> \n> @@ -47,6 +54,8 @@ public:\n> const std::vector &entities() const { return entities_; }\n> MediaEntity *getEntityByName(const std::string &name) const;\n> \n> + std::vector enumerateMediaWalks() const;\n> +\n> MediaLink *link(const std::string &sourceName, unsigned int sourceIdx,\n> const std::string &sinkName, unsigned int sinkIdx);\n> MediaLink *link(const MediaEntity *source, unsigned int sourceIdx,\n> @@ -77,6 +86,9 @@ private:\n> friend int MediaLink::setEnabled(bool enable);\n> int setupLink(const MediaLink *link, unsigned int flags);\n> \n> + void walkGraph(std::vector *mediaWalks, MediaWalk *walk,\n> + MediaEntity *entity, MediaLink *sinkLink) const;\n> +\n> std::string driver_;\n> std::string deviceNode_;\n> std::string model_;\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index 941f86c25f66..60b397e205a2 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -340,6 +340,105 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name) const\n> return nullptr;\n> }\n> \n> +/**\n> + * \\fn MediaDevice::enumerateMediaWalks()\n> + * \\brief Retrieve the list of all possible MediaWalks available to this device\n> + *\n> + * Enumerate the media graph and return all possible permutations of unique\n> + * sub-graphs where the first entity is a sensor device. These sub-graphs are\n> + * stored as a \\a MediaDevice::MediaWalk structure, where each element gives the\n> + * device entity and associated sink pad link. The first entity in this structure\n> + * is asensor device, with the sink pad link set to a nullptr.\n\n/asensor/a sensor/\n\n> + *\n> + * As an example, consider the media graph below:\n> + *\n> + * +----------+\n> + * | CSI2 |\n> + * +-----^----+\n> + * |\n> + * +---+---+\n> + * | Mux1 <-------+\n\nI'd probably fix this with a | before the <- too\n\n> + * +--^----+ |\n> + * | |\n> + * +-----+---+ +---+---+\n> + * | Sensor1 | | Mux2 |<--+\n> + * +---------+ +-^-----+ |\n> + * | |\n> + * +-------+-+ +---+-----+\n> + * | Sensor2 | | Sensor3 |\n> + * +---------+ +---------+\n> + *\n> + * This would return three \\a MediaDevice::MediaWalk structures looking like:\n> + *\n> + * 1)\n> + * +----------+\n> + * | CSI2 |\n> + * +-----^----+\n> + * |\n> + * +-----+----+\n> + * | Mux1 |\n> + * +-----^----+\n> + * |\n> + * +-----+----+\n> + * | Sensor1 |\n> + * +----------+\n> + *\n> + * 2)\n> + * +----------+\n> + * | CSI2 |\n> + * +-----^----+\n> + * |\n> + * +-----+----+\n> + * | Mux1 |\n> + * +-----^----+\n> + * |\n> + * +-----+----+\n> + * | Mux2 |\n> + * +-----^----+\n> + * |\n> + * +-----+----+\n> + * | Sensor2 |\n> + * +----------+\n> + *\n> + * 3)\n> + * +----------+\n> + * | CSI2 |\n> + * +-----^----+\n> + * |\n> + * +-----+----+\n> + * | Mux1 |\n> + * +-----^----+\n> + * |\n> + * +-----+----+\n> + * | Mux2 |\n> + * +-----^----+\n> + * |\n> + * +-----+----+\n> + * | Sensor3 |\n> + * +----------+\n\nAnd as with the commit message, those would be nicer if they were in\na horizontal row I think.\n\nThat's easy to fix with asciiflow, so here's a copy/paste to fix either\nnow or while applying.\n\n\n 1) 2) 3)\n +----------+ +----------+ +----------+\n | CSI2 | | CSI2 | | CSI2 |\n +-----^----+ +-----^----+ +-----^----+\n | | |\n +-----+----+ +-----+----+ +-----+----+\n | Mux1 | | Mux1 | | Mux1 |\n +-----^----+ +-----^----+ +-----^----+\n | | |\n +-----+----+ +-----+----+ +-----+----+\n | Sensor1 | | Mux2 | | Mux2 |\n +----------+ +-----^----+ +-----^----+\n | |\n +-----+----+ +-----+----+\n | Sensor2 | | Sensor3 |\n +----------+ +----------+\n\n\n\n> + *\n> + * \\return A vector of MediaWalk structures available\n> + */\n> +std::vector MediaDevice::enumerateMediaWalks() const\n> +{\n> + std::vector mediaWalks;\n> +\n> + for (MediaEntity *entity : entities_) {\n> + /* Only perform enumeration starting with sensor entities */\n> + if (entity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> + continue;\n> + /*\n> + * Start with an empty MediaWalk structure, and walk the graph\n> + * with the sensor entity at the top, and a nullptr as the sink\n> + * pad link.\n> + */\n> + mediaWalks.push_back({});\n> + walkGraph(&mediaWalks, &mediaWalks.back(), entity, nullptr);\n> + }\n> +\n> + return mediaWalks;\n> +}\n> +\n> /**\n> * \\brief Retrieve the MediaLink connecting two pads, identified by entity\n> * names and pad indexes\n> @@ -800,4 +899,40 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags)\n> return 0;\n> }\n> \n> +void MediaDevice::walkGraph(std::vector *mediaWalks, MediaWalk *walk,\n> + MediaEntity *entity, MediaLink *sinkLink) const\n> +{\n> + bool newWalk = false;\n> +\n> + walk->push_back({ entity, sinkLink, nullptr });\n> +\n> + for (const MediaPad *pad : entity->pads()) {\n> + /* We only walk down the graph, so ignore any sink pads */\n> + if (pad->flags() & MEDIA_PAD_FL_SINK)\n> + continue;\n> +\n> + for (MediaLink *nextLink : pad->links()) {\n> + MediaDevice::MediaWalk *nextWalk = walk;\n> + /*\n> + * If this is a new branch in the graph, create a new\n> + * MediaWalk structure in our list, and populate it with\n> + * a copy of the curret MediaWalk. Any subsequent recursion\n\n/curret/current/\n\nI think this looks useful, and I've already found myself having to write\nsimilar code else where so I beleive this will be reusable, and perhaps\nextended.\n\nI fear that in the future things may get more complex and end up\nrequiring specific helpers for specific entities that we match while\nwalking a graph. For example we've been working on devices like GMSL or\nFPDLink which handle serialising and deserialising mutliple devices\nthrough a single connection, or some paths may have more complexity that\nneed sovling. Maybe that will be with more support from the pipeline\nhandlers.\n\nBut ... I think those will be dealt with as we hit it.\n\nWith the typos fixed, and optionally making those graphs in a row ...\n\n\nReviewed-by: Kieran Bingham \n\n\n> + * into this graph branch will cause it to diverge.\n> + */\n> + if (newWalk) {\n> + mediaWalks->push_back(*walk);\n> + nextWalk = &mediaWalks->back();\n> + }\n> +\n> + /* Record the source pad link for the current entity */\n> + walk->back().sourceLink = nextLink;\n> +\n> + /* Recurse into the next entity for this source pad link */\n> + MediaEntity *nextEntity = nextLink->sink()->entity();\n> + walkGraph(mediaWalks, nextWalk, nextEntity, nextLink);\n> + newWalk = true;\n> + }\n> + }\n> +}\n> +\n> } /* namespace libcamera */\n> -- \n> 2.25.1\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 6CDC5BDCBF\n\tfor ;\n\tTue, 1 Feb 2022 00:19:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 675B7609C6;\n\tTue, 1 Feb 2022 01:19:41 +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 E6E25604F1\n\tfor ;\n\tTue, 1 Feb 2022 01:19:39 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 85C609DE;\n\tTue, 1 Feb 2022 01:19:39 +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=\"pSri+o54\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1643674779;\n\tbh=YjcvxWI0SV5kSoTdJYYD30HNNRGcTZrwFpRCOpXDXpk=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=pSri+o54PJ41dobrF3uYzEqT9lEC4CnaTnILGS+PAgDTFGTe+z0GFUhvPwExN1eap\n\tTy4voMg4DlwJga0ZIkJMz/JDjA4ooz2SarIQ6QJk1ZX54ZE+gR6h+0O9bslyA/tqGJ\n\tSGgXpf70enLlmcXC/ZvR7eKTasKyegN94RjzlPIc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220113102529.3163441-2-naush@raspberrypi.com>","References":"<20220113102529.3163441-1-naush@raspberrypi.com>\n\t<20220113102529.3163441-2-naush@raspberrypi.com>","From":"Kieran Bingham ","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 01 Feb 2022 00:19:37 +0000","Message-ID":"<164367477725.115113.16564494473886510141@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC v1 1/2] libcamera: media_device: Add\n\tenumerateMediaWalks() helper","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]