[{"id":21689,"web_url":"https://patchwork.libcamera.org/comment/21689/","msgid":"<163904277369.2066819.17308357219840915893@Monstersaurus>","date":"2021-12-09T09:39:33","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2021-12-08 15:15:27)\n> This change will allow the pipeline handler to enumerate and control Video\n> Mux or Bridge devices that may be attached between sensors and a particular\n> Unicam instance. Cascaded mux or bridge devices are also handled.\n> \n> A new member function enumerateVideoDevices(), called from registerCamera(), is\n> used to identify and open all mux and bridge subdevices present in the\n> sensor -> Unicam link.\n> \n> Relevent links are enabled/disabled and pad formats correctly set in configure()\n> before the camera is started.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 78 +++++++++++++++++++\n> 1 file changed, 78 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 756878c70036..ca176ecb40ec 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -220,6 +221,11 @@ public:\n> std::vector streams_;\n> /* Stores the ids of the buffers mapped in the IPA. */\n> std::unordered_set ipaBuffers_;\n> + /*\n> + * Stores a cascade of Video Mux or Bridge devices between the sensor and\n> + * Unicam together with media link across the entities.\n> + */\n> + std::vector, MediaLink *>> bridgeDevices_;\n> \n> /* DMAHEAP allocation helper. */\n> RPi::DmaHeap dmaHeap_;\n> @@ -311,6 +317,7 @@ private:\n> }\n> \n> int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);\n> + void enumerateVideoDevices(RPiCameraData *data, MediaLink *link);\n> int queueAllBuffers(Camera *camera);\n> int prepareBuffers(Camera *camera);\n> void freeBuffers(Camera *camera);\n> @@ -868,6 +875,25 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> */\n> data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> \n> + /* Setup the Video Mux/Bridge entities. */\n> + for (auto &[device, link] : data->bridgeDevices_) {\n> + /* Start by disabling all the sink pad links on the devices in the cascade. */\n> + for (const MediaPad *p : device->entity()->pads()) {\n> + if (!(p->flags() & MEDIA_PAD_FL_SINK))\n> + continue;\n> +\n> + for (MediaLink *l : p->links())\n> + l->setEnabled(false);\n> + }\n> +\n> + /* Finally enable the link, and setup the pad format. */\n> + link->setEnabled(true);\n\nIsn't this going to enable /all/ bridge links in the cascade\nincorrectly?\n\n\n ┌──────────┐\n │ Unicam │\n └─────▲────┘\n │\n ┌───┴───┐\n │ Mux ◄───────┐\n └──▲────┘ │\n │ │\n ┌─────┴───┐ ┌───┴───┐\n │ Sensor1 │ │ Mux │◄──┐\n └─────────┘ └─▲─────┘ │\n │ │\n ┌───────┴─┐ ┌───┴─────┐\n │ Sensor2 │ │ Sensor3 │\n └─────────┘ └─────────┘\n\nIn that 'use case' we're now iterating over all bridges and enabling\ntheir link, I think which means we've just enabled both muxes,\nimmediately after disabling them - even for Sensor1?\n\nMaybe I've mis-understood it, but I thought there would be something\nthat would start at the desired sensor, and walk up the links to the\nunicam enabling those links (and only those) as it went up?\n\nThe walk only has to be done once too, so perhaps a per-camera(sensor)\nvector of links to iterate and enable?\n\nOr maybe I'm missing the separation between the bridgeDevices_ and the\nper-camera instances. But if that's already happening, I can't then see\nhow each camera data clears all the bridges used by other cameras...\n\n\n> + const MediaPad *srcPad = link->sink();\n> + ret = device->setFormat(srcPad->index(), &sensorFormat);\n\nThis assignment of ret is in a loop, so earlier failures are going to\nget ignored, unchecked.\n\n> + LOG(RPI, Info) << \"Configured media link on device \" << device->entity()->name()\n> + << \" at pad \" << srcPad->index();\n> + }\n> +\n> return ret;\n> }\n> \n> @@ -1098,6 +1124,13 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> if (data->sensor_->init())\n> return -EINVAL;\n> \n> + /*\n> + * Enumerate all the Video Mux/Bridge devices across the sensor -> unicam\n> + * link. There may be a cascade of devices in this link!\n> + */\n> + MediaLink *link = sensorEntity->getPadByIndex(0)->links()[0];\n> + enumerateVideoDevices(data.get(), link);\n> +\n> data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> \n> ipa::RPi::SensorConfig sensorConfig;\n> @@ -1224,6 +1257,51 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> return 0;\n> }\n> \n> +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data, MediaLink *link)\n> +{\n> + const MediaPad *sinkPad = link->sink();\n> + const MediaEntity *entity = sinkPad->entity();\n> + bool unicamFound = false;\n> +\n> + /* We only deal with Video Mux and Bridge devices in cascade. */\n> + if (entity->function() != MEDIA_ENT_F_VID_MUX &&\n> + entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)\n> + return;\n> +\n> + LOG(RPI, Info) << \"Found video mux device \" << entity->name()\n> + << \" linked to sink pad \" << sinkPad->index();\n> +\n> + data->bridgeDevices_.emplace_back(std::make_unique(entity), link);\n> + data->bridgeDevices_.back().first->open();\n\nGoing through the above, I can't help but wonder if the bridgeDevices_\nshould be stored as a single instance in the Pipeline handler (not each\nCameraData, we have one CameraData per camera right? if so, how does\ncamera3 disable all the links? does it know about every path?)\n\n\n> +\n> + for (const MediaPad *pad : entity->pads()) {\n> + /*\n> + * Iterate through all the sink pads down the cascade to find any\n> + * other Video Mux and Bridge devices.\n> + */\n> + if (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> + continue;\n> +\n> + for (MediaLink *l : pad->links()) {\n> + enumerateVideoDevices(data, l);\n> + if (l->sink()->entity()->name() == \"unicam-image\")\n> + unicamFound = true;\n> + }\n> + }\n> +\n> + /* This identifies the end of our entity enumeration recursion. */\n> + if (link->source()->entity()->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> + /*\n> + * If Unicam is not at the end of this cascade, we cannot configure\n> + * this topology automatically, so remove all entity references.\n> + */\n> + if (!unicamFound) {\n> + LOG(RPI, Warning) << \"Cannot automatically configure this MC topology!\";\n> + data->bridgeDevices_.clear();\n> + }\n> + }\n> +}\n> +\n> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> {\n> RPiCameraData *data = cameraData(camera);\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 DF513BDB13\n\tfor ;\n\tThu, 9 Dec 2021 09:39:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 254DB60822;\n\tThu, 9 Dec 2021 10:39:39 +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 1DE2C60224\n\tfor ;\n\tThu, 9 Dec 2021 10:39:37 +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 A92C8501;\n\tThu, 9 Dec 2021 10:39:36 +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=\"HEdMG9yd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639042776;\n\tbh=Nd4++odpQtOqhmotJj7UM8kkpzKJ1+0mZpVnX64ezG8=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=HEdMG9yd+OZGV41tXMNIkBXMxHaXCWHSTc9PlxvWuUwxX0VSLYG0VeIisGpVJtk0a\n\tNXFgQhhFFGfKXoweNVskGNaM5wNzQjjET2zDvHaHjNWSDhqS0YpU81jh/ELmujfHx5\n\tqHZnrWblCrQlFoWb6RSszpvdqv8gZYb6M+OfRucI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211208151527.1404715-3-naush@raspberrypi.com>","References":"<20211208151527.1404715-1-naush@raspberrypi.com>\n\t<20211208151527.1404715-3-naush@raspberrypi.com>","From":"Kieran Bingham ","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 09 Dec 2021 09:39:33 +0000","Message-ID":"<163904277369.2066819.17308357219840915893@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","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\" "}},{"id":21695,"web_url":"https://patchwork.libcamera.org/comment/21695/","msgid":"","date":"2021-12-09T10:07:43","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThanks for your feedback.\n\nOn Thu, 9 Dec 2021 at 09:39, Kieran Bingham \nwrote:\n\n> Quoting Naushir Patuck (2021-12-08 15:15:27)\n> > This change will allow the pipeline handler to enumerate and control\n> Video\n> > Mux or Bridge devices that may be attached between sensors and a\n> particular\n> > Unicam instance. Cascaded mux or bridge devices are also handled.\n> >\n> > A new member function enumerateVideoDevices(), called from\n> registerCamera(), is\n> > used to identify and open all mux and bridge subdevices present in the\n> > sensor -> Unicam link.\n> >\n> > Relevent links are enabled/disabled and pad formats correctly set in\n> configure()\n> > before the camera is started.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 78 +++++++++++++++++++\n> > 1 file changed, 78 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 756878c70036..ca176ecb40ec 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -12,6 +12,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> >\n> > #include \n> > #include \n> > @@ -220,6 +221,11 @@ public:\n> > std::vector streams_;\n> > /* Stores the ids of the buffers mapped in the IPA. */\n> > std::unordered_set ipaBuffers_;\n> > + /*\n> > + * Stores a cascade of Video Mux or Bridge devices between the\n> sensor and\n> > + * Unicam together with media link across the entities.\n> > + */\n> > + std::vector, MediaLink\n> *>> bridgeDevices_;\n> >\n> > /* DMAHEAP allocation helper. */\n> > RPi::DmaHeap dmaHeap_;\n> > @@ -311,6 +317,7 @@ private:\n> > }\n> >\n> > int registerCamera(MediaDevice *unicam, MediaDevice *isp,\n> MediaEntity *sensorEntity);\n> > + void enumerateVideoDevices(RPiCameraData *data, MediaLink *link);\n> > int queueAllBuffers(Camera *camera);\n> > int prepareBuffers(Camera *camera);\n> > void freeBuffers(Camera *camera);\n> > @@ -868,6 +875,25 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > */\n> > data->properties_.set(properties::ScalerCropMaximum,\n> data->sensorInfo_.analogCrop);\n> >\n> > + /* Setup the Video Mux/Bridge entities. */\n> > + for (auto &[device, link] : data->bridgeDevices_) {\n> > + /* Start by disabling all the sink pad links on the\n> devices in the cascade. */\n> > + for (const MediaPad *p : device->entity()->pads()) {\n> > + if (!(p->flags() & MEDIA_PAD_FL_SINK))\n> > + continue;\n> > +\n> > + for (MediaLink *l : p->links())\n> > + l->setEnabled(false);\n> > + }\n> > +\n> > + /* Finally enable the link, and setup the pad format. */\n> > + link->setEnabled(true);\n>\n> Isn't this going to enable /all/ bridge links in the cascade\n> incorrectly?\n>\n>\n> ┌──────────┐\n> │ Unicam │\n> └─────▲────┘\n> │\n> ┌───┴───┐\n> │ Mux ◄───────┐\n> └──▲────┘ │\n> │ │\n> ┌─────┴───┐ ┌───┴───┐\n> │ Sensor1 │ │ Mux │◄──┐\n> └─────────┘ └─▲─────┘ │\n> │ │\n> ┌───────┴─┐ ┌───┴─────┐\n> │ Sensor2 │ │ Sensor3 │\n> └─────────┘ └─────────┘\n>\n> In that 'use case' we're now iterating over all bridges and enabling\n> their link, I think which means we've just enabled both muxes,\n> immediately after disabling them - even for Sensor1?\n>\n> Maybe I've mis-understood it, but I thought there would be something\n> that would start at the desired sensor, and walk up the links to the\n> unicam enabling those links (and only those) as it went up?\n>\n> The walk only has to be done once too, so perhaps a per-camera(sensor)\n> vector of links to iterate and enable?\n>\n> Or maybe I'm missing the separation between the bridgeDevices_ and the\n> per-camera instances. But if that's already happening, I can't then see\n> how each camera data clears all the bridges used by other cameras...\n>\n\nLet me explain my intention, and you can then tell me if the code does\nwhat I think it does :-)\n\nEach sensor (Sensor{1,2,3}) will register its own Camera object, and\nin that object bridgeDevices_ will store the cascade of muxes between\nthe sensor and the Unicam port. So, for Sensor1, we store only 1 mux,\nand Sensor{2,3} will store both muxes. Together with the mux device,\nwe also store the entity to entity links.\n\nThe above code goes through those stored entities, first disabling *all*\nlinks on each device in the chain, and then selectively enabling\nthe specific links that are stored in bridgeDevices_ to link sensor\nto Unicam across all intermedia muxes. If Sensor1 is used, this\ndoes mean that the Sensor{2,3} -> Mux links might not change\nstate but the Mux to Mux link will be disabled. Similarly, if we are\ndriving\nSensor3, the Sensor{1,2} -> Mux link will be disabled.\n\n\n>\n> > + const MediaPad *srcPad = link->sink();\n> > + ret = device->setFormat(srcPad->index(), &sensorFormat);\n>\n> This assignment of ret is in a loop, so earlier failures are going to\n> get ignored, unchecked.\n>\n\nThis should not matter, as ret is only used in the loop to count successful\ncamera registrations. The return value from match() will be false only if\n0 cameras were registered.\n\n\n>\n> > + LOG(RPI, Info) << \"Configured media link on device \" <<\n> device->entity()->name()\n> > + << \" at pad \" << srcPad->index();\n> > + }\n> > +\n> > return ret;\n> > }\n> >\n> > @@ -1098,6 +1124,13 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > if (data->sensor_->init())\n> > return -EINVAL;\n> >\n> > + /*\n> > + * Enumerate all the Video Mux/Bridge devices across the sensor\n> -> unicam\n> > + * link. There may be a cascade of devices in this link!\n> > + */\n> > + MediaLink *link = sensorEntity->getPadByIndex(0)->links()[0];\n> > + enumerateVideoDevices(data.get(), link);\n> > +\n> > data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> >\n> > ipa::RPi::SensorConfig sensorConfig;\n> > @@ -1224,6 +1257,51 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > return 0;\n> > }\n> >\n> > +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data,\n> MediaLink *link)\n> > +{\n> > + const MediaPad *sinkPad = link->sink();\n> > + const MediaEntity *entity = sinkPad->entity();\n> > + bool unicamFound = false;\n> > +\n> > + /* We only deal with Video Mux and Bridge devices in cascade. */\n> > + if (entity->function() != MEDIA_ENT_F_VID_MUX &&\n> > + entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)\n> > + return;\n> > +\n> > + LOG(RPI, Info) << \"Found video mux device \" << entity->name()\n> > + << \" linked to sink pad \" << sinkPad->index();\n> > +\n> > +\n> data->bridgeDevices_.emplace_back(std::make_unique(entity),\n> link);\n> > + data->bridgeDevices_.back().first->open();\n>\n> Going through the above, I can't help but wonder if the bridgeDevices_\n> should be stored as a single instance in the Pipeline handler (not each\n> CameraData, we have one CameraData per camera right? if so, how does\n> camera3 disable all the links? does it know about every path?)\n>\n\nFrom my description earlier, bridgeDevices_ must be stored in the\nCameraData, as\nit lists the devices in the path between the sensor and Unicam. And this\nis unique\nper-sensor. Again, this does mean that if we are using Sensor1, links for\nSensor{2,3}\n-> Mux are not changed, but the Mux->Mux link will be disabled. Do you\nthink\nthat may not be appropriate leaving them enabled?\n\nPlease shout if this doesn't make sense, and something simpler might\nequally work :-)\n\nCheers,\nNaush\n\n\n\n>\n>\n> > +\n> > + for (const MediaPad *pad : entity->pads()) {\n> > + /*\n> > + * Iterate through all the sink pads down the cascade to\n> find any\n> > + * other Video Mux and Bridge devices.\n> > + */\n> > + if (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > + continue;\n> > +\n> > + for (MediaLink *l : pad->links()) {\n> > + enumerateVideoDevices(data, l);\n> > + if (l->sink()->entity()->name() ==\n> \"unicam-image\")\n> > + unicamFound = true;\n> > + }\n> > + }\n> > +\n> > + /* This identifies the end of our entity enumeration recursion.\n> */\n> > + if (link->source()->entity()->function() ==\n> MEDIA_ENT_F_CAM_SENSOR) {\n> > + /*\n> > + * If Unicam is not at the end of this cascade, we cannot\n> configure\n> > + * this topology automatically, so remove all entity\n> references.\n> > + */\n> > + if (!unicamFound) {\n> > + LOG(RPI, Warning) << \"Cannot automatically\n> configure this MC topology!\";\n> > + data->bridgeDevices_.clear();\n> > + }\n> > + }\n> > +}\n> > +\n> > int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > {\n> > RPiCameraData *data = cameraData(camera);\n> > --\n> > 2.25.1\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 D0861BDB13\n\tfor ;\n\tThu, 9 Dec 2021 10:08:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EFE4960882;\n\tThu, 9 Dec 2021 11:08:01 +0100 (CET)","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B1C2560224\n\tfor ;\n\tThu, 9 Dec 2021 11:07:59 +0100 (CET)","by mail-lj1-x22f.google.com with SMTP id p8so8177963ljo.5\n\tfor ;\n\tThu, 09 Dec 2021 02:07:59 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"lO/Ls6a6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=oqyV3zYEyThZ5c+yD6mQLNceEO4MCLjNxNOo0pxhc/E=;\n\tb=lO/Ls6a6yDgiBGXO5/R5/ZYBORHqX6eXtOrK1MxJeLjDbiDNRd1HC7bCYNBzQTRQ9a\n\tzgMMNDP6KexQx5zSorRvoDR7JHNRqwvFkZlPMiMYGVYzDDkgr+cLOzyP0eteD7Wb37KV\n\tZqQIt1DEZxIpIQnit8XC031wrCZmfgBJkDrB9GTttQvGQIkd92flkPp1jVYIBOAcF7Rm\n\ta+eJ3m51CaSSBg6SXqDop8n+Qi3EMWlON952bYLqwd1zHu2GBLF6ZVjcMMAzVnEDT1NA\n\tqBP5TQuezsQ+7UzOY3J3QeoAcNB+tmVAyADAD1a9eK5TjavebiUsZaxjeZGx+xRU9Cad\n\tCJXQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=oqyV3zYEyThZ5c+yD6mQLNceEO4MCLjNxNOo0pxhc/E=;\n\tb=VH1R3A/bMHTvBN2iTFRJWeXbJWbcmnYNTGp1i10iyWBWoEtWpOMWDIU4BCxjMRCEdA\n\tEFldmKyWS1TZxKMwwl8LjxgDHZr1pmXUHk9KCGbO48xP+SlaDnTrebcF6P/JZ54tb+d5\n\taJ4iKlIHDAHBL1e/g7HRFgBNl7BUpDk5rMoLK4ACnGXeYrY8F+jnsGiUxsoUuAIm2i4B\n\tESM2dm/kdrm3KQ45iGLVrZw2g8HUmivFvbfCDk/TzCO7iYyhXgg1nfSlWG1xArp2LUSG\n\tA/P25NtBK9s3U7OFXxGpDwmHl4N3RW76iIafVMqFpiNe9sNdKC6hWj3VaoVDe6eTAJiK\n\tn0tA==","X-Gm-Message-State":"AOAM5300kU8xCSsj/yJOqTliMdr2171r8BjMkudZTynRyXe7BU6h15Ut\n\tJRmMbWsQmozzP6dbOI2E7OkaOaDez6E6i4ukT0xC8g==","X-Google-Smtp-Source":"ABdhPJyPoXdKCEZXw5m5X6j6mHB4ThZlHXsqtHLo0hRdiLjTlEG+L0aHMzZ1zDC3nFOrJD8GkafDEIU7R2/YxQE55yY=","X-Received":"by 2002:a2e:a22a:: with SMTP id i10mr5403743ljm.16.1639044478947;\n\tThu, 09 Dec 2021 02:07:58 -0800 (PST)","MIME-Version":"1.0","References":"<20211208151527.1404715-1-naush@raspberrypi.com>\n\t<20211208151527.1404715-3-naush@raspberrypi.com>\n\t<163904277369.2066819.17308357219840915893@Monstersaurus>","In-Reply-To":"<163904277369.2066819.17308357219840915893@Monstersaurus>","From":"Naushir Patuck ","Date":"Thu, 9 Dec 2021 10:07:43 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000f06b1d05d2b3c79a\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21700,"web_url":"https://patchwork.libcamera.org/comment/21700/","msgid":"<163904552543.2066819.12592034208871016560@Monstersaurus>","date":"2021-12-09T10:25:25","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2021-12-09 10:07:43)\n> Hi Kieran,\n> \n> Thanks for your feedback.\n> \n> On Thu, 9 Dec 2021 at 09:39, Kieran Bingham \n> wrote:\n> \n> > Quoting Naushir Patuck (2021-12-08 15:15:27)\n> > > This change will allow the pipeline handler to enumerate and control\n> > Video\n> > > Mux or Bridge devices that may be attached between sensors and a\n> > particular\n> > > Unicam instance. Cascaded mux or bridge devices are also handled.\n> > >\n> > > A new member function enumerateVideoDevices(), called from\n> > registerCamera(), is\n> > > used to identify and open all mux and bridge subdevices present in the\n> > > sensor -> Unicam link.\n> > >\n> > > Relevent links are enabled/disabled and pad formats correctly set in\n> > configure()\n> > > before the camera is started.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 78 +++++++++++++++++++\n> > > 1 file changed, 78 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 756878c70036..ca176ecb40ec 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -12,6 +12,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > >\n> > > #include \n> > > #include \n> > > @@ -220,6 +221,11 @@ public:\n> > > std::vector streams_;\n> > > /* Stores the ids of the buffers mapped in the IPA. */\n> > > std::unordered_set ipaBuffers_;\n> > > + /*\n> > > + * Stores a cascade of Video Mux or Bridge devices between the\n> > sensor and\n> > > + * Unicam together with media link across the entities.\n> > > + */\n> > > + std::vector, MediaLink\n> > *>> bridgeDevices_;\n> > >\n> > > /* DMAHEAP allocation helper. */\n> > > RPi::DmaHeap dmaHeap_;\n> > > @@ -311,6 +317,7 @@ private:\n> > > }\n> > >\n> > > int registerCamera(MediaDevice *unicam, MediaDevice *isp,\n> > MediaEntity *sensorEntity);\n> > > + void enumerateVideoDevices(RPiCameraData *data, MediaLink *link);\n> > > int queueAllBuffers(Camera *camera);\n> > > int prepareBuffers(Camera *camera);\n> > > void freeBuffers(Camera *camera);\n> > > @@ -868,6 +875,25 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> > CameraConfiguration *config)\n> > > */\n> > > data->properties_.set(properties::ScalerCropMaximum,\n> > data->sensorInfo_.analogCrop);\n> > >\n> > > + /* Setup the Video Mux/Bridge entities. */\n> > > + for (auto &[device, link] : data->bridgeDevices_) {\n> > > + /* Start by disabling all the sink pad links on the\n> > devices in the cascade. */\n> > > + for (const MediaPad *p : device->entity()->pads()) {\n> > > + if (!(p->flags() & MEDIA_PAD_FL_SINK))\n> > > + continue;\n> > > +\n> > > + for (MediaLink *l : p->links())\n> > > + l->setEnabled(false);\n> > > + }\n> > > +\n> > > + /* Finally enable the link, and setup the pad format. */\n> > > + link->setEnabled(true);\n> >\n> > Isn't this going to enable /all/ bridge links in the cascade\n> > incorrectly?\n> >\n> >\n> > ┌──────────┐\n> > │ Unicam │\n> > └─────▲────┘\n> > │\n> > ┌───┴───┐\n> > │ Mux ◄───────┐\n> > └──▲────┘ │\n> > │ │\n> > ┌─────┴───┐ ┌───┴───┐\n> > │ Sensor1 │ │ Mux │◄──┐\n> > └─────────┘ └─▲─────┘ │\n> > │ │\n> > ┌───────┴─┐ ┌───┴─────┐\n> > │ Sensor2 │ │ Sensor3 │\n> > └─────────┘ └─────────┘\n> >\n> > In that 'use case' we're now iterating over all bridges and enabling\n> > their link, I think which means we've just enabled both muxes,\n> > immediately after disabling them - even for Sensor1?\n> >\n> > Maybe I've mis-understood it, but I thought there would be something\n> > that would start at the desired sensor, and walk up the links to the\n> > unicam enabling those links (and only those) as it went up?\n> >\n> > The walk only has to be done once too, so perhaps a per-camera(sensor)\n> > vector of links to iterate and enable?\n> >\n> > Or maybe I'm missing the separation between the bridgeDevices_ and the\n> > per-camera instances. But if that's already happening, I can't then see\n> > how each camera data clears all the bridges used by other cameras...\n> >\n> \n> Let me explain my intention, and you can then tell me if the code does\n> what I think it does :-)\n> \n> Each sensor (Sensor{1,2,3}) will register its own Camera object, and\n> in that object bridgeDevices_ will store the cascade of muxes between\n> the sensor and the Unicam port. So, for Sensor1, we store only 1 mux,\n> and Sensor{2,3} will store both muxes. Together with the mux device,\n> we also store the entity to entity links.\n> \n> The above code goes through those stored entities, first disabling *all*\n> links on each device in the chain, and then selectively enabling\n> the specific links that are stored in bridgeDevices_ to link sensor\n> to Unicam across all intermedia muxes. If Sensor1 is used, this\n> does mean that the Sensor{2,3} -> Mux links might not change\n> state but the Mux to Mux link will be disabled. Similarly, if we are\n> driving\n> Sensor3, the Sensor{1,2} -> Mux link will be disabled.\n\nI think I see the obvious part I missed ;-)\n\nIt doesn't matter what configuration Mux2 has as long as Mux1 only has\nSensor1 link enabled!\n\nIt might help to document that in the comments somehow somewhere?\nParticularly now that you've already written the explanation to me -\nperhaps it should be a block comment above enumerateVideoDevices.\n\nFeel free to take or add the ascii diagram, I think more pictures in\nour documentation are always helpful. It's really easy to make those\nblock diagrams in ascii art at https://asciiflow.com.\n\n\n> >\n> > > + const MediaPad *srcPad = link->sink();\n> > > + ret = device->setFormat(srcPad->index(), &sensorFormat);\n> >\n> > This assignment of ret is in a loop, so earlier failures are going to\n> > get ignored, unchecked.\n> >\n> \n> This should not matter, as ret is only used in the loop to count successful\n> camera registrations. The return value from match() will be false only if\n> 0 cameras were registered.\n\nSo should we not even assign it?\nDo we need to report a warning if it failed to set the format?\n\nBut aren't we here in configure() setting the format of the device to\npropogate the format?\n\nIf we fail to setFormat in that case, then I think we should be saying\nthe configure() call failed?\n\n\n\n> \n> \n> >\n> > > + LOG(RPI, Info) << \"Configured media link on device \" <<\n> > device->entity()->name()\n> > > + << \" at pad \" << srcPad->index();\n> > > + }\n> > > +\n> > > return ret;\n> > > }\n> > >\n> > > @@ -1098,6 +1124,13 @@ int\n> > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > if (data->sensor_->init())\n> > > return -EINVAL;\n> > >\n> > > + /*\n> > > + * Enumerate all the Video Mux/Bridge devices across the sensor\n> > -> unicam\n> > > + * link. There may be a cascade of devices in this link!\n> > > + */\n> > > + MediaLink *link = sensorEntity->getPadByIndex(0)->links()[0];\n> > > + enumerateVideoDevices(data.get(), link);\n> > > +\n> > > data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > >\n> > > ipa::RPi::SensorConfig sensorConfig;\n> > > @@ -1224,6 +1257,51 @@ int\n> > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > return 0;\n> > > }\n> > >\n> > > +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data,\n> > MediaLink *link)\n> > > +{\n> > > + const MediaPad *sinkPad = link->sink();\n> > > + const MediaEntity *entity = sinkPad->entity();\n> > > + bool unicamFound = false;\n> > > +\n> > > + /* We only deal with Video Mux and Bridge devices in cascade. */\n> > > + if (entity->function() != MEDIA_ENT_F_VID_MUX &&\n> > > + entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)\n> > > + return;\n> > > +\n> > > + LOG(RPI, Info) << \"Found video mux device \" << entity->name()\n> > > + << \" linked to sink pad \" << sinkPad->index();\n> > > +\n> > > +\n> > data->bridgeDevices_.emplace_back(std::make_unique(entity),\n> > link);\n> > > + data->bridgeDevices_.back().first->open();\n> >\n> > Going through the above, I can't help but wonder if the bridgeDevices_\n> > should be stored as a single instance in the Pipeline handler (not each\n> > CameraData, we have one CameraData per camera right? if so, how does\n> > camera3 disable all the links? does it know about every path?)\n> >\n> \n> From my description earlier, bridgeDevices_ must be stored in the\n> CameraData, as\n> it lists the devices in the path between the sensor and Unicam. And this\n> is unique\n> per-sensor. Again, this does mean that if we are using Sensor1, links for\n> Sensor{2,3}\n> -> Mux are not changed, but the Mux->Mux link will be disabled. Do you\n> think\n> that may not be appropriate leaving them enabled?\n\nI missed that, and now it's obvious. If the first mux is only pointing\nto Sensor1, then indeed it doesn't matter what mux2 is configured to\nlink to...\n\nSo I think that's ok.\n\n> \n> Please shout if this doesn't make sense, and something simpler might\n> equally work :-)\n\nIt's all fine until someone adds a crossbar perhaps that might make it\npossible to do more crazy things ;-)\n\nBut we don't need to worry about that now I dont' think.\n\nI think I'm still concerned about that ret during the loop in\nconfigure(). But with that resolved:\n\nReviewed-by: Kieran Bingham \n\n> Cheers,\n> Naush\n> \n> \n> \n> >\n> >\n> > > +\n> > > + for (const MediaPad *pad : entity->pads()) {\n> > > + /*\n> > > + * Iterate through all the sink pads down the cascade to\n> > find any\n> > > + * other Video Mux and Bridge devices.\n> > > + */\n> > > + if (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > > + continue;\n> > > +\n> > > + for (MediaLink *l : pad->links()) {\n> > > + enumerateVideoDevices(data, l);\n> > > + if (l->sink()->entity()->name() ==\n> > \"unicam-image\")\n> > > + unicamFound = true;\n> > > + }\n> > > + }\n> > > +\n> > > + /* This identifies the end of our entity enumeration recursion.\n> > */\n> > > + if (link->source()->entity()->function() ==\n> > MEDIA_ENT_F_CAM_SENSOR) {\n> > > + /*\n> > > + * If Unicam is not at the end of this cascade, we cannot\n> > configure\n> > > + * this topology automatically, so remove all entity\n> > references.\n> > > + */\n> > > + if (!unicamFound) {\n> > > + LOG(RPI, Warning) << \"Cannot automatically\n> > configure this MC topology!\";\n> > > + data->bridgeDevices_.clear();\n> > > + }\n> > > + }\n> > > +}\n> > > +\n> > > int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > > {\n> > > RPiCameraData *data = cameraData(camera);\n> > > --\n> > > 2.25.1\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 81409BDB13\n\tfor ;\n\tThu, 9 Dec 2021 10:25:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF05A60876;\n\tThu, 9 Dec 2021 11:25:29 +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 773FF60224\n\tfor ;\n\tThu, 9 Dec 2021 11:25:28 +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 182FB501;\n\tThu, 9 Dec 2021 11:25:28 +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=\"NwjQAo87\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639045528;\n\tbh=rezmAHdQb8iCIGRPomDM+AXEyAE/v/gG6jT8uAR7O1s=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=NwjQAo87hwaavfEKXxVF1u+JOzH6IrPbShtRqVFr6VHiKjzi1yRnH+yB2f1iFItIk\n\ttcmqcVafQxkexufXZDEwF+IftXiVnuVQ4VHglL1EU+dy0U9gPcroGLvXpBolyS9JpT\n\tODiW/EehVfUiQ5bfR6ncHaRYPoO5weEnMw9AWKzk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211208151527.1404715-1-naush@raspberrypi.com>\n\t<20211208151527.1404715-3-naush@raspberrypi.com>\n\t<163904277369.2066819.17308357219840915893@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Naushir Patuck ","Date":"Thu, 09 Dec 2021 10:25:25 +0000","Message-ID":"<163904552543.2066819.12592034208871016560@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21701,"web_url":"https://patchwork.libcamera.org/comment/21701/","msgid":"","date":"2021-12-09T10:29:21","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Thu, 9 Dec 2021 at 10:25, Kieran Bingham \nwrote:\n\n> Quoting Naushir Patuck (2021-12-09 10:07:43)\n> > Hi Kieran,\n> >\n> > Thanks for your feedback.\n> >\n> > On Thu, 9 Dec 2021 at 09:39, Kieran Bingham <\n> kieran.bingham@ideasonboard.com>\n> > wrote:\n> >\n> > > Quoting Naushir Patuck (2021-12-08 15:15:27)\n> > > > This change will allow the pipeline handler to enumerate and control\n> > > Video\n> > > > Mux or Bridge devices that may be attached between sensors and a\n> > > particular\n> > > > Unicam instance. Cascaded mux or bridge devices are also handled.\n> > > >\n> > > > A new member function enumerateVideoDevices(), called from\n> > > registerCamera(), is\n> > > > used to identify and open all mux and bridge subdevices present in\n> the\n> > > > sensor -> Unicam link.\n> > > >\n> > > > Relevent links are enabled/disabled and pad formats correctly set in\n> > > configure()\n> > > > before the camera is started.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 78\n> +++++++++++++++++++\n> > > > 1 file changed, 78 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 756878c70036..ca176ecb40ec 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -12,6 +12,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > >\n> > > > #include \n> > > > #include \n> > > > @@ -220,6 +221,11 @@ public:\n> > > > std::vector streams_;\n> > > > /* Stores the ids of the buffers mapped in the IPA. */\n> > > > std::unordered_set ipaBuffers_;\n> > > > + /*\n> > > > + * Stores a cascade of Video Mux or Bridge devices between\n> the\n> > > sensor and\n> > > > + * Unicam together with media link across the entities.\n> > > > + */\n> > > > + std::vector,\n> MediaLink\n> > > *>> bridgeDevices_;\n> > > >\n> > > > /* DMAHEAP allocation helper. */\n> > > > RPi::DmaHeap dmaHeap_;\n> > > > @@ -311,6 +317,7 @@ private:\n> > > > }\n> > > >\n> > > > int registerCamera(MediaDevice *unicam, MediaDevice *isp,\n> > > MediaEntity *sensorEntity);\n> > > > + void enumerateVideoDevices(RPiCameraData *data, MediaLink\n> *link);\n> > > > int queueAllBuffers(Camera *camera);\n> > > > int prepareBuffers(Camera *camera);\n> > > > void freeBuffers(Camera *camera);\n> > > > @@ -868,6 +875,25 @@ int PipelineHandlerRPi::configure(Camera\n> *camera,\n> > > CameraConfiguration *config)\n> > > > */\n> > > > data->properties_.set(properties::ScalerCropMaximum,\n> > > data->sensorInfo_.analogCrop);\n> > > >\n> > > > + /* Setup the Video Mux/Bridge entities. */\n> > > > + for (auto &[device, link] : data->bridgeDevices_) {\n> > > > + /* Start by disabling all the sink pad links on the\n> > > devices in the cascade. */\n> > > > + for (const MediaPad *p : device->entity()->pads()) {\n> > > > + if (!(p->flags() & MEDIA_PAD_FL_SINK))\n> > > > + continue;\n> > > > +\n> > > > + for (MediaLink *l : p->links())\n> > > > + l->setEnabled(false);\n> > > > + }\n> > > > +\n> > > > + /* Finally enable the link, and setup the pad\n> format. */\n> > > > + link->setEnabled(true);\n> > >\n> > > Isn't this going to enable /all/ bridge links in the cascade\n> > > incorrectly?\n> > >\n> > >\n> > > ┌──────────┐\n> > > │ Unicam │\n> > > └─────▲────┘\n> > > │\n> > > ┌───┴───┐\n> > > │ Mux ◄───────┐\n> > > └──▲────┘ │\n> > > │ │\n> > > ┌─────┴───┐ ┌───┴───┐\n> > > │ Sensor1 │ │ Mux │◄──┐\n> > > └─────────┘ └─▲─────┘ │\n> > > │ │\n> > > ┌───────┴─┐ ┌───┴─────┐\n> > > │ Sensor2 │ │ Sensor3 │\n> > > └─────────┘ └─────────┘\n> > >\n> > > In that 'use case' we're now iterating over all bridges and enabling\n> > > their link, I think which means we've just enabled both muxes,\n> > > immediately after disabling them - even for Sensor1?\n> > >\n> > > Maybe I've mis-understood it, but I thought there would be something\n> > > that would start at the desired sensor, and walk up the links to the\n> > > unicam enabling those links (and only those) as it went up?\n> > >\n> > > The walk only has to be done once too, so perhaps a per-camera(sensor)\n> > > vector of links to iterate and enable?\n> > >\n> > > Or maybe I'm missing the separation between the bridgeDevices_ and the\n> > > per-camera instances. But if that's already happening, I can't then see\n> > > how each camera data clears all the bridges used by other cameras...\n> > >\n> >\n> > Let me explain my intention, and you can then tell me if the code does\n> > what I think it does :-)\n> >\n> > Each sensor (Sensor{1,2,3}) will register its own Camera object, and\n> > in that object bridgeDevices_ will store the cascade of muxes between\n> > the sensor and the Unicam port. So, for Sensor1, we store only 1 mux,\n> > and Sensor{2,3} will store both muxes. Together with the mux device,\n> > we also store the entity to entity links.\n> >\n> > The above code goes through those stored entities, first disabling *all*\n> > links on each device in the chain, and then selectively enabling\n> > the specific links that are stored in bridgeDevices_ to link sensor\n> > to Unicam across all intermedia muxes. If Sensor1 is used, this\n> > does mean that the Sensor{2,3} -> Mux links might not change\n> > state but the Mux to Mux link will be disabled. Similarly, if we are\n> > driving\n> > Sensor3, the Sensor{1,2} -> Mux link will be disabled.\n>\n> I think I see the obvious part I missed ;-)\n>\n> It doesn't matter what configuration Mux2 has as long as Mux1 only has\n> Sensor1 link enabled!\n>\n> It might help to document that in the comments somehow somewhere?\n> Particularly now that you've already written the explanation to me -\n> perhaps it should be a block comment above enumerateVideoDevices.\n>\n> Feel free to take or add the ascii diagram, I think more pictures in\n> our documentation are always helpful. It's really easy to make those\n> block diagrams in ascii art at https://asciiflow.com.\n>\n\nSure, I'll add some comments through the code explaining it in more detail.\n\n\n>\n>\n> > >\n> > > > + const MediaPad *srcPad = link->sink();\n> > > > + ret = device->setFormat(srcPad->index(),\n> &sensorFormat);\n> > >\n> > > This assignment of ret is in a loop, so earlier failures are going to\n> > > get ignored, unchecked.\n> > >\n> >\n> > This should not matter, as ret is only used in the loop to count\n> successful\n> > camera registrations. The return value from match() will be false only\n> if\n> > 0 cameras were registered.\n>\n> So should we not even assign it?\n> Do we need to report a warning if it failed to set the format?\n>\n> But aren't we here in configure() setting the format of the device to\n> propogate the format?\n>\n> If we fail to setFormat in that case, then I think we should be saying\n> the configure() call failed?\n>\n\nOops, my mistake. I misread your original comment and what bit of\ncode it refers to! This ret value *is* used for the return path, and must\nbe accounted for correctly as it is set in a loop! I'll fix this in the next\nrevision.\n\n\n>\n>\n>\n> >\n> >\n> > >\n> > > > + LOG(RPI, Info) << \"Configured media link on device \"\n> <<\n> > > device->entity()->name()\n> > > > + << \" at pad \" << srcPad->index();\n> > > > + }\n> > > > +\n> > > > return ret;\n> > > > }\n> > > >\n> > > > @@ -1098,6 +1124,13 @@ int\n> > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice\n> *isp, Me\n> > > > if (data->sensor_->init())\n> > > > return -EINVAL;\n> > > >\n> > > > + /*\n> > > > + * Enumerate all the Video Mux/Bridge devices across the\n> sensor\n> > > -> unicam\n> > > > + * link. There may be a cascade of devices in this link!\n> > > > + */\n> > > > + MediaLink *link = sensorEntity->getPadByIndex(0)->links()[0];\n> > > > + enumerateVideoDevices(data.get(), link);\n> > > > +\n> > > > data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > > >\n> > > > ipa::RPi::SensorConfig sensorConfig;\n> > > > @@ -1224,6 +1257,51 @@ int\n> > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice\n> *isp, Me\n> > > > return 0;\n> > > > }\n> > > >\n> > > > +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data,\n> > > MediaLink *link)\n> > > > +{\n> > > > + const MediaPad *sinkPad = link->sink();\n> > > > + const MediaEntity *entity = sinkPad->entity();\n> > > > + bool unicamFound = false;\n> > > > +\n> > > > + /* We only deal with Video Mux and Bridge devices in\n> cascade. */\n> > > > + if (entity->function() != MEDIA_ENT_F_VID_MUX &&\n> > > > + entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)\n> > > > + return;\n> > > > +\n> > > > + LOG(RPI, Info) << \"Found video mux device \" << entity->name()\n> > > > + << \" linked to sink pad \" << sinkPad->index();\n> > > > +\n> > > > +\n> > >\n> data->bridgeDevices_.emplace_back(std::make_unique(entity),\n> > > link);\n> > > > + data->bridgeDevices_.back().first->open();\n> > >\n> > > Going through the above, I can't help but wonder if the bridgeDevices_\n> > > should be stored as a single instance in the Pipeline handler (not each\n> > > CameraData, we have one CameraData per camera right? if so, how does\n> > > camera3 disable all the links? does it know about every path?)\n> > >\n> >\n> > From my description earlier, bridgeDevices_ must be stored in the\n> > CameraData, as\n> > it lists the devices in the path between the sensor and Unicam. And this\n> > is unique\n> > per-sensor. Again, this does mean that if we are using Sensor1, links\n> for\n> > Sensor{2,3}\n> > -> Mux are not changed, but the Mux->Mux link will be disabled. Do you\n> > think\n> > that may not be appropriate leaving them enabled?\n>\n> I missed that, and now it's obvious. If the first mux is only pointing\n> to Sensor1, then indeed it doesn't matter what mux2 is configured to\n> link to...\n>\n> So I think that's ok.\n>\n> >\n> > Please shout if this doesn't make sense, and something simpler might\n> > equally work :-)\n>\n> It's all fine until someone adds a crossbar perhaps that might make it\n> possible to do more crazy things ;-)\n>\n> But we don't need to worry about that now I dont' think.\n>\n> I think I'm still concerned about that ret during the loop in\n> configure(). But with that resolved:\n>\n> Reviewed-by: Kieran Bingham \n>\n> > Cheers,\n> > Naush\n> >\n> >\n> >\n> > >\n> > >\n> > > > +\n> > > > + for (const MediaPad *pad : entity->pads()) {\n> > > > + /*\n> > > > + * Iterate through all the sink pads down the\n> cascade to\n> > > find any\n> > > > + * other Video Mux and Bridge devices.\n> > > > + */\n> > > > + if (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > > > + continue;\n> > > > +\n> > > > + for (MediaLink *l : pad->links()) {\n> > > > + enumerateVideoDevices(data, l);\n> > > > + if (l->sink()->entity()->name() ==\n> > > \"unicam-image\")\n> > > > + unicamFound = true;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + /* This identifies the end of our entity enumeration\n> recursion.\n> > > */\n> > > > + if (link->source()->entity()->function() ==\n> > > MEDIA_ENT_F_CAM_SENSOR) {\n> > > > + /*\n> > > > + * If Unicam is not at the end of this cascade, we\n> cannot\n> > > configure\n> > > > + * this topology automatically, so remove all entity\n> > > references.\n> > > > + */\n> > > > + if (!unicamFound) {\n> > > > + LOG(RPI, Warning) << \"Cannot automatically\n> > > configure this MC topology!\";\n> > > > + data->bridgeDevices_.clear();\n> > > > + }\n> > > > + }\n> > > > +}\n> > > > +\n> > > > int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > > > {\n> > > > RPiCameraData *data = cameraData(camera);\n> > > > --\n> > > > 2.25.1\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 CB2E4BF415\n\tfor ;\n\tThu, 9 Dec 2021 10:29:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B24B60882;\n\tThu, 9 Dec 2021 11:29:40 +0100 (CET)","from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com\n\t[IPv6:2a00:1450:4864:20::12b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7873460224\n\tfor ;\n\tThu, 9 Dec 2021 11:29:38 +0100 (CET)","by mail-lf1-x12b.google.com with SMTP id k37so11137088lfv.3\n\tfor ;\n\tThu, 09 Dec 2021 02:29:38 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"RHHAVi5e\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=bBVF6U/B20uuJ4VCYTFGUE7mKb6Qpbf+KZsP3CULFNQ=;\n\tb=RHHAVi5eRs1RdmAZWPzA4RMH8ji+tZeFFykvqRXrRd4uxLRddz3/3lSEwh5rIzqB2o\n\t4GctfmS5ZTtqxCDRE7isjkqTUYqF68YCJYa7XhY9NJ8VkFwtjCIb6RD+1rmBAclCZSHy\n\t3/3JXWQniOb+wCcrbIBuhIhYIyEwH00HK4qxDAFfObLeylFFZOeawqxHpjY0y7aMVI4q\n\tWiE0hqBsFDFB6XK09hik4vJDKWwJtbnDEph8Z51oGfFPgfitsd4AF0gXUxRSkr4yt7pW\n\t+tvlpuOv7ku0eWI7nZMZP/SX2OABdjpWOHuwagQmAArn47gY6T6iH/Fi8GmZxbRj2pzg\n\to9nw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=bBVF6U/B20uuJ4VCYTFGUE7mKb6Qpbf+KZsP3CULFNQ=;\n\tb=2CeVJQmW4Jmk48sRheLbYOZ1HaerCQun0J5hAOsE17oCMqV2I+sIwIxvVBx24RK0IR\n\tDqj5+CFmHTATWCR7jUfHO5I2H19lxKygpeIXjF1QFVx5oeGbh1+ot881IYdC8qghIB7Q\n\tY4HTDbQ2iUo07IlwIlot9/eAJKjxXt/ih31bfT6iLLg1hJP7/zzGlZs3AqiTQESN6fwt\n\t53fnN3ANTo4pMoRFQudFhkdkl8s1qJrVPmIeDYno7iE1tzDwVa5srk/l1Go/oZpMCxvw\n\tWrwYrYbAFW4Q21R6NQRQUry6etYFNTC8mUV+umiODhOxhkMqwWAdDDlOUQU8ydQloaj7\n\t5i5g==","X-Gm-Message-State":"AOAM531LvxMyVYkZ5bzdgQyVOsnZsl0G0umC4EXULAWa2owI2f4bX81T\n\tLO6j/kYNN5b1xvLVMBwc/adcxSvaUSoRMLa4tvDvoyv0KE1hgA==","X-Google-Smtp-Source":"ABdhPJxfaWuqMU6GQ2gD2uMWABp1lkMoV9QeKkNGQR2ef3+h4rbU12SAVNy3BFAqA3o8LFO073wLtg2zJfPiF8yDBxY=","X-Received":"by 2002:a19:6754:: with SMTP id\n\te20mr4854556lfj.122.1639045777634; \n\tThu, 09 Dec 2021 02:29:37 -0800 (PST)","MIME-Version":"1.0","References":"<20211208151527.1404715-1-naush@raspberrypi.com>\n\t<20211208151527.1404715-3-naush@raspberrypi.com>\n\t<163904277369.2066819.17308357219840915893@Monstersaurus>\n\t\n\t<163904552543.2066819.12592034208871016560@Monstersaurus>","In-Reply-To":"<163904552543.2066819.12592034208871016560@Monstersaurus>","From":"Naushir Patuck ","Date":"Thu, 9 Dec 2021 10:29:21 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"00000000000058cd1005d2b41554\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21730,"web_url":"https://patchwork.libcamera.org/comment/21730/","msgid":"","date":"2021-12-10T00:24:28","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Thu, Dec 09, 2021 at 10:29:21AM +0000, Naushir Patuck wrote:\n> On Thu, 9 Dec 2021 at 10:25, Kieran Bingham wrote:\n> > Quoting Naushir Patuck (2021-12-09 10:07:43)\n> > > On Thu, 9 Dec 2021 at 09:39, Kieran Bingham wrote:\n> > > > Quoting Naushir Patuck (2021-12-08 15:15:27)\n> > > > > This change will allow the pipeline handler to enumerate and control Video\n> > > > > Mux or Bridge devices that may be attached between sensors and a particular\n> > > > > Unicam instance. Cascaded mux or bridge devices are also handled.\n> > > > >\n> > > > > A new member function enumerateVideoDevices(), called from registerCamera(), is\n> > > > > used to identify and open all mux and bridge subdevices present in the\n> > > > > sensor -> Unicam link.\n> > > > >\n> > > > > Relevent links are enabled/disabled and pad formats correctly set in configure()\n> > > > > before the camera is started.\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > ---\n> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 78 +++++++++++++++++++\n> > > > > 1 file changed, 78 insertions(+)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index 756878c70036..ca176ecb40ec 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -12,6 +12,7 @@\n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > >\n> > > > > #include \n> > > > > #include \n> > > > > @@ -220,6 +221,11 @@ public:\n> > > > > std::vector streams_;\n> > > > > /* Stores the ids of the buffers mapped in the IPA. */\n> > > > > std::unordered_set ipaBuffers_;\n> > > > > + /*\n> > > > > + * Stores a cascade of Video Mux or Bridge devices between the sensor and\n> > > > > + * Unicam together with media link across the entities.\n> > > > > + */\n> > > > > + std::vector, MediaLink *>> bridgeDevices_;\n> > > > >\n> > > > > /* DMAHEAP allocation helper. */\n> > > > > RPi::DmaHeap dmaHeap_;\n> > > > > @@ -311,6 +317,7 @@ private:\n> > > > > }\n> > > > >\n> > > > > int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);\n> > > > > + void enumerateVideoDevices(RPiCameraData *data, MediaLink *link);\n> > > > > int queueAllBuffers(Camera *camera);\n> > > > > int prepareBuffers(Camera *camera);\n> > > > > void freeBuffers(Camera *camera);\n> > > > > @@ -868,6 +875,25 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > > */\n> > > > > data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> > > > >\n> > > > > + /* Setup the Video Mux/Bridge entities. */\n> > > > > + for (auto &[device, link] : data->bridgeDevices_) {\n> > > > > + /* Start by disabling all the sink pad links on the devices in the cascade. */\n> > > > > + for (const MediaPad *p : device->entity()->pads()) {\n> > > > > + if (!(p->flags() & MEDIA_PAD_FL_SINK))\n> > > > > + continue;\n> > > > > +\n> > > > > + for (MediaLink *l : p->links())\n> > > > > + l->setEnabled(false);\n\nAs an optimization, you could skip disabling the link that you will\nenable below.\n\n> > > > > + }\n> > > > > +\n> > > > > + /* Finally enable the link, and setup the pad format. */\n> > > > > + link->setEnabled(true);\n> > > >\n> > > > Isn't this going to enable /all/ bridge links in the cascade\n> > > > incorrectly?\n> > > >\n> > > >\n> > > > ┌──────────┐\n> > > > │ Unicam │\n> > > > └─────▲────┘\n> > > > │\n> > > > ┌───┴───┐\n> > > > │ Mux ◄───────┐\n> > > > └──▲────┘ │\n> > > > │ │\n> > > > ┌─────┴───┐ ┌───┴───┐\n> > > > │ Sensor1 │ │ Mux │◄──┐\n> > > > └─────────┘ └─▲─────┘ │\n> > > > │ │\n> > > > ┌───────┴─┐ ┌───┴─────┐\n> > > > │ Sensor2 │ │ Sensor3 │\n> > > > └─────────┘ └─────────┘\n> > > >\n> > > > In that 'use case' we're now iterating over all bridges and enabling\n> > > > their link, I think which means we've just enabled both muxes,\n> > > > immediately after disabling them - even for Sensor1?\n> > > >\n> > > > Maybe I've mis-understood it, but I thought there would be something\n> > > > that would start at the desired sensor, and walk up the links to the\n> > > > unicam enabling those links (and only those) as it went up?\n> > > >\n> > > > The walk only has to be done once too, so perhaps a per-camera(sensor)\n> > > > vector of links to iterate and enable?\n> > > >\n> > > > Or maybe I'm missing the separation between the bridgeDevices_ and the\n> > > > per-camera instances. But if that's already happening, I can't then see\n> > > > how each camera data clears all the bridges used by other cameras...\n> > >\n> > > Let me explain my intention, and you can then tell me if the code does\n> > > what I think it does :-)\n> > >\n> > > Each sensor (Sensor{1,2,3}) will register its own Camera object, and\n> > > in that object bridgeDevices_ will store the cascade of muxes between\n> > > the sensor and the Unicam port. So, for Sensor1, we store only 1 mux,\n> > > and Sensor{2,3} will store both muxes. Together with the mux device,\n> > > we also store the entity to entity links.\n> > >\n> > > The above code goes through those stored entities, first disabling *all*\n> > > links on each device in the chain, and then selectively enabling\n> > > the specific links that are stored in bridgeDevices_ to link sensor\n> > > to Unicam across all intermedia muxes. If Sensor1 is used, this\n> > > does mean that the Sensor{2,3} -> Mux links might not change\n> > > state but the Mux to Mux link will be disabled. Similarly, if we are driving\n> > > Sensor3, the Sensor{1,2} -> Mux link will be disabled.\n> >\n> > I think I see the obvious part I missed ;-)\n> >\n> > It doesn't matter what configuration Mux2 has as long as Mux1 only has\n> > Sensor1 link enabled!\n> >\n> > It might help to document that in the comments somehow somewhere?\n> > Particularly now that you've already written the explanation to me -\n> > perhaps it should be a block comment above enumerateVideoDevices.\n> >\n> > Feel free to take or add the ascii diagram, I think more pictures in\n> > our documentation are always helpful. It's really easy to make those\n> > block diagrams in ascii art at https://asciiflow.com.\n> \n> Sure, I'll add some comments through the code explaining it in more detail.\n> \n> > > > > + const MediaPad *srcPad = link->sink();\n> > > > > + ret = device->setFormat(srcPad->index(), &sensorFormat);\n> > > >\n> > > > This assignment of ret is in a loop, so earlier failures are going to\n> > > > get ignored, unchecked.\n> > >\n> > > This should not matter, as ret is only used in the loop to count successful\n> > > camera registrations. The return value from match() will be false only if\n> > > 0 cameras were registered.\n> >\n> > So should we not even assign it?\n> > Do we need to report a warning if it failed to set the format?\n> >\n> > But aren't we here in configure() setting the format of the device to\n> > propogate the format?\n> >\n> > If we fail to setFormat in that case, then I think we should be saying\n> > the configure() call failed?\n> \n> Oops, my mistake. I misread your original comment and what bit of\n> code it refers to! This ret value *is* used for the return path, and must\n> be accounted for correctly as it is set in a loop! I'll fix this in the next\n> revision.\n> \n> > > > > + LOG(RPI, Info) << \"Configured media link on device \" << device->entity()->name()\n> > > > > + << \" at pad \" << srcPad->index();\n> > > > > + }\n> > > > > +\n> > > > > return ret;\n> > > > > }\n> > > > >\n> > > > > @@ -1098,6 +1124,13 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > > > if (data->sensor_->init())\n> > > > > return -EINVAL;\n> > > > >\n> > > > > + /*\n> > > > > + * Enumerate all the Video Mux/Bridge devices across the sensor -> unicam\n> > > > > + * link. There may be a cascade of devices in this link!\n> > > > > + */\n> > > > > + MediaLink *link = sensorEntity->getPadByIndex(0)->links()[0];\n> > > > > + enumerateVideoDevices(data.get(), link);\n> > > > > +\n> > > > > data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > > > >\n> > > > > ipa::RPi::SensorConfig sensorConfig;\n> > > > > @@ -1224,6 +1257,51 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data, MediaLink *link)\n\nThis function accesses members of data but doesn't seem to access any\nmember of PipelineHandlerRPi. Could it be moved to RPiCameraData ?\n\n> > > > > +{\n> > > > > + const MediaPad *sinkPad = link->sink();\n> > > > > + const MediaEntity *entity = sinkPad->entity();\n> > > > > + bool unicamFound = false;\n> > > > > +\n> > > > > + /* We only deal with Video Mux and Bridge devices in cascade. */\n> > > > > + if (entity->function() != MEDIA_ENT_F_VID_MUX &&\n> > > > > + entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)\n> > > > > + return;\n> > > > > +\n> > > > > + LOG(RPI, Info) << \"Found video mux device \" << entity->name()\n> > > > > + << \" linked to sink pad \" << sinkPad->index();\n> > > > > +\n> > > > > + data->bridgeDevices_.emplace_back(std::make_unique(entity),link);\n> > > > > + data->bridgeDevices_.back().first->open();\n> > > >\n> > > > Going through the above, I can't help but wonder if the bridgeDevices_\n> > > > should be stored as a single instance in the Pipeline handler (not each\n> > > > CameraData, we have one CameraData per camera right? if so, how does\n> > > > camera3 disable all the links? does it know about every path?)\n> > >\n> > > From my description earlier, bridgeDevices_ must be stored in the CameraData, as\n> > > it lists the devices in the path between the sensor and Unicam. And this is unique\n> > > per-sensor. Again, this does mean that if we are using Sensor1, links for Sensor{2,3}\n> > > -> Mux are not changed, but the Mux->Mux link will be disabled. Do you think\n> > > that may not be appropriate leaving them enabled?\n\nI can't help but seeing lots of similarities with the simple pipeline\nhandler, which also performs dynamic discovery of pipelines (all the way\nto the video node in that case, while in your case it would be all the\nway to the Unicam device). I wonder if we could share code between the\ntwo ? This could benefit the Raspberry Pi pipeline handler by providing\na more generic version of format propagation through the pipeline\n(although that part may be more difficult to share).\n\nAn important difference between the two implementations is that the\nsimple pipeline handler opens the video node and subdevs once only,\nstoring them in the pipeline handler instance, and only stores pointers\nto those objects in the camera data.\n\n> > I missed that, and now it's obvious. If the first mux is only pointing\n> > to Sensor1, then indeed it doesn't matter what mux2 is configured to\n> > link to...\n> >\n> > So I think that's ok.\n> >\n> > > Please shout if this doesn't make sense, and something simpler might\n> > > equally work :-)\n> >\n> > It's all fine until someone adds a crossbar perhaps that might make it\n> > possible to do more crazy things ;-)\n> >\n> > But we don't need to worry about that now I dont' think.\n> >\n> > I think I'm still concerned about that ret during the loop in\n> > configure(). But with that resolved:\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > > > +\n> > > > > + for (const MediaPad *pad : entity->pads()) {\n> > > > > + /*\n> > > > > + * Iterate through all the sink pads down the cascade to find any\n> > > > > + * other Video Mux and Bridge devices.\n> > > > > + */\n> > > > > + if (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > > > > + continue;\n> > > > > +\n> > > > > + for (MediaLink *l : pad->links()) {\n> > > > > + enumerateVideoDevices(data, l);\n> > > > > + if (l->sink()->entity()->name() == \"unicam-image\")\n> > > > > + unicamFound = true;\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > > > + /* This identifies the end of our entity enumeration recursion. */\n> > > > > + if (link->source()->entity()->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> > > > > + /*\n> > > > > + * If Unicam is not at the end of this cascade, we cannot configure\n> > > > > + * this topology automatically, so remove all entity references.\n> > > > > + */\n> > > > > + if (!unicamFound) {\n> > > > > + LOG(RPI, Warning) << \"Cannot automatically configure this MC topology!\";\n> > > > > + data->bridgeDevices_.clear();\n> > > > > + }\n> > > > > + }\n> > > > > +}\n> > > > > +\n> > > > > int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > > > > {\n> > > > > RPiCameraData *data = cameraData(camera);","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 639CABDB13\n\tfor ;\n\tFri, 10 Dec 2021 00:25:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 732126086A;\n\tFri, 10 Dec 2021 01:25:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9802560113\n\tfor ;\n\tFri, 10 Dec 2021 01:24:58 +0100 (CET)","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 E75EE90E;\n\tFri, 10 Dec 2021 01:24:57 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rbMbBfLV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639095898;\n\tbh=AXkwwBfWAxFjTszgUHxYY0+6y+pIujjFcl1KjTREbC8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rbMbBfLVOtuN7LKQHxYeL0B0LZmPw6yzNtsjW0ZDFCLm6Nap+ZEIdUyXy+X0Mglzh\n\tSiwpHD//W1Y4y5H8l6uB77J6Adqhuf82jleiuCwmbHgISB3YkHKTkXL/6kzWLX7/wB\n\taaS2DEji0VSXsdR95mm/kSHDZqrP3BBmxTMaoEsc=","Date":"Fri, 10 Dec 2021 02:24:28 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211208151527.1404715-1-naush@raspberrypi.com>\n\t<20211208151527.1404715-3-naush@raspberrypi.com>\n\t<163904277369.2066819.17308357219840915893@Monstersaurus>\n\t\n\t<163904552543.2066819.12592034208871016560@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21757,"web_url":"https://patchwork.libcamera.org/comment/21757/","msgid":"","date":"2021-12-10T15:48:19","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Fri, 10 Dec 2021 at 00:24, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Thu, Dec 09, 2021 at 10:29:21AM +0000, Naushir Patuck wrote:\n> > On Thu, 9 Dec 2021 at 10:25, Kieran Bingham wrote:\n> > > Quoting Naushir Patuck (2021-12-09 10:07:43)\n> > > > On Thu, 9 Dec 2021 at 09:39, Kieran Bingham wrote:\n> > > > > Quoting Naushir Patuck (2021-12-08 15:15:27)\n> > > > > > This change will allow the pipeline handler to enumerate and\n> control Video\n> > > > > > Mux or Bridge devices that may be attached between sensors and a\n> particular\n> > > > > > Unicam instance. Cascaded mux or bridge devices are also handled.\n> > > > > >\n> > > > > > A new member function enumerateVideoDevices(), called from\n> registerCamera(), is\n> > > > > > used to identify and open all mux and bridge subdevices present\n> in the\n> > > > > > sensor -> Unicam link.\n> > > > > >\n> > > > > > Relevent links are enabled/disabled and pad formats correctly\n> set in configure()\n> > > > > > before the camera is started.\n> > > > > >\n> > > > > > Signed-off-by: Naushir Patuck \n> > > > > > ---\n> > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 78\n> +++++++++++++++++++\n> > > > > > 1 file changed, 78 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > index 756878c70036..ca176ecb40ec 100644\n> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > @@ -12,6 +12,7 @@\n> > > > > > #include \n> > > > > > #include \n> > > > > > #include \n> > > > > > +#include \n> > > > > >\n> > > > > > #include \n> > > > > > #include \n> > > > > > @@ -220,6 +221,11 @@ public:\n> > > > > > std::vector streams_;\n> > > > > > /* Stores the ids of the buffers mapped in the IPA. */\n> > > > > > std::unordered_set ipaBuffers_;\n> > > > > > + /*\n> > > > > > + * Stores a cascade of Video Mux or Bridge devices\n> between the sensor and\n> > > > > > + * Unicam together with media link across the entities.\n> > > > > > + */\n> > > > > > + std::vector,\n> MediaLink *>> bridgeDevices_;\n> > > > > >\n> > > > > > /* DMAHEAP allocation helper. */\n> > > > > > RPi::DmaHeap dmaHeap_;\n> > > > > > @@ -311,6 +317,7 @@ private:\n> > > > > > }\n> > > > > >\n> > > > > > int registerCamera(MediaDevice *unicam, MediaDevice\n> *isp, MediaEntity *sensorEntity);\n> > > > > > + void enumerateVideoDevices(RPiCameraData *data,\n> MediaLink *link);\n> > > > > > int queueAllBuffers(Camera *camera);\n> > > > > > int prepareBuffers(Camera *camera);\n> > > > > > void freeBuffers(Camera *camera);\n> > > > > > @@ -868,6 +875,25 @@ int PipelineHandlerRPi::configure(Camera\n> *camera, CameraConfiguration *config)\n> > > > > > */\n> > > > > > data->properties_.set(properties::ScalerCropMaximum,\n> data->sensorInfo_.analogCrop);\n> > > > > >\n> > > > > > + /* Setup the Video Mux/Bridge entities. */\n> > > > > > + for (auto &[device, link] : data->bridgeDevices_) {\n> > > > > > + /* Start by disabling all the sink pad links on\n> the devices in the cascade. */\n> > > > > > + for (const MediaPad *p :\n> device->entity()->pads()) {\n> > > > > > + if (!(p->flags() & MEDIA_PAD_FL_SINK))\n> > > > > > + continue;\n> > > > > > +\n> > > > > > + for (MediaLink *l : p->links())\n> > > > > > + l->setEnabled(false);\n>\n> As an optimization, you could skip disabling the link that you will\n> enable below.\n>\n\nAck.\n\n\n>\n> > > > > > + }\n> > > > > > +\n> > > > > > + /* Finally enable the link, and setup the pad\n> format. */\n> > > > > > + link->setEnabled(true);\n> > > > >\n> > > > > Isn't this going to enable /all/ bridge links in the cascade\n> > > > > incorrectly?\n> > > > >\n> > > > >\n> > > > > ┌──────────┐\n> > > > > │ Unicam │\n> > > > > └─────▲────┘\n> > > > > │\n> > > > > ┌───┴───┐\n> > > > > │ Mux ◄───────┐\n> > > > > └──▲────┘ │\n> > > > > │ │\n> > > > > ┌─────┴───┐ ┌───┴───┐\n> > > > > │ Sensor1 │ │ Mux │◄──┐\n> > > > > └─────────┘ └─▲─────┘ │\n> > > > > │ │\n> > > > > ┌───────┴─┐ ┌───┴─────┐\n> > > > > │ Sensor2 │ │ Sensor3 │\n> > > > > └─────────┘ └─────────┘\n> > > > >\n> > > > > In that 'use case' we're now iterating over all bridges and\n> enabling\n> > > > > their link, I think which means we've just enabled both muxes,\n> > > > > immediately after disabling them - even for Sensor1?\n> > > > >\n> > > > > Maybe I've mis-understood it, but I thought there would be\n> something\n> > > > > that would start at the desired sensor, and walk up the links to\n> the\n> > > > > unicam enabling those links (and only those) as it went up?\n> > > > >\n> > > > > The walk only has to be done once too, so perhaps a\n> per-camera(sensor)\n> > > > > vector of links to iterate and enable?\n> > > > >\n> > > > > Or maybe I'm missing the separation between the bridgeDevices_ and\n> the\n> > > > > per-camera instances. But if that's already happening, I can't\n> then see\n> > > > > how each camera data clears all the bridges used by other\n> cameras...\n> > > >\n> > > > Let me explain my intention, and you can then tell me if the code\n> does\n> > > > what I think it does :-)\n> > > >\n> > > > Each sensor (Sensor{1,2,3}) will register its own Camera object, and\n> > > > in that object bridgeDevices_ will store the cascade of muxes between\n> > > > the sensor and the Unicam port. So, for Sensor1, we store only 1 mux,\n> > > > and Sensor{2,3} will store both muxes. Together with the mux device,\n> > > > we also store the entity to entity links.\n> > > >\n> > > > The above code goes through those stored entities, first disabling\n> *all*\n> > > > links on each device in the chain, and then selectively enabling\n> > > > the specific links that are stored in bridgeDevices_ to link sensor\n> > > > to Unicam across all intermedia muxes. If Sensor1 is used, this\n> > > > does mean that the Sensor{2,3} -> Mux links might not change\n> > > > state but the Mux to Mux link will be disabled. Similarly, if we\n> are driving\n> > > > Sensor3, the Sensor{1,2} -> Mux link will be disabled.\n> > >\n> > > I think I see the obvious part I missed ;-)\n> > >\n> > > It doesn't matter what configuration Mux2 has as long as Mux1 only has\n> > > Sensor1 link enabled!\n> > >\n> > > It might help to document that in the comments somehow somewhere?\n> > > Particularly now that you've already written the explanation to me -\n> > > perhaps it should be a block comment above enumerateVideoDevices.\n> > >\n> > > Feel free to take or add the ascii diagram, I think more pictures in\n> > > our documentation are always helpful. It's really easy to make those\n> > > block diagrams in ascii art at https://asciiflow.com.\n> >\n> > Sure, I'll add some comments through the code explaining it in more\n> detail.\n> >\n> > > > > > + const MediaPad *srcPad = link->sink();\n> > > > > > + ret = device->setFormat(srcPad->index(),\n> &sensorFormat);\n> > > > >\n> > > > > This assignment of ret is in a loop, so earlier failures are going\n> to\n> > > > > get ignored, unchecked.\n> > > >\n> > > > This should not matter, as ret is only used in the loop to count\n> successful\n> > > > camera registrations. The return value from match() will be false\n> only if\n> > > > 0 cameras were registered.\n> > >\n> > > So should we not even assign it?\n> > > Do we need to report a warning if it failed to set the format?\n> > >\n> > > But aren't we here in configure() setting the format of the device to\n> > > propogate the format?\n> > >\n> > > If we fail to setFormat in that case, then I think we should be saying\n> > > the configure() call failed?\n> >\n> > Oops, my mistake. I misread your original comment and what bit of\n> > code it refers to! This ret value *is* used for the return path, and\n> must\n> > be accounted for correctly as it is set in a loop! I'll fix this in the\n> next\n> > revision.\n> >\n> > > > > > + LOG(RPI, Info) << \"Configured media link on\n> device \" << device->entity()->name()\n> > > > > > + << \" at pad \" << srcPad->index();\n> > > > > > + }\n> > > > > > +\n> > > > > > return ret;\n> > > > > > }\n> > > > > >\n> > > > > > @@ -1098,6 +1124,13 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > > > > if (data->sensor_->init())\n> > > > > > return -EINVAL;\n> > > > > >\n> > > > > > + /*\n> > > > > > + * Enumerate all the Video Mux/Bridge devices across the\n> sensor -> unicam\n> > > > > > + * link. There may be a cascade of devices in this link!\n> > > > > > + */\n> > > > > > + MediaLink *link =\n> sensorEntity->getPadByIndex(0)->links()[0];\n> > > > > > + enumerateVideoDevices(data.get(), link);\n> > > > > > +\n> > > > > > data->sensorFormats_ =\n> populateSensorFormats(data->sensor_);\n> > > > > >\n> > > > > > ipa::RPi::SensorConfig sensorConfig;\n> > > > > > @@ -1224,6 +1257,51 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > > > > > +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData\n> *data, MediaLink *link)\n>\n> This function accesses members of data but doesn't seem to access any\n> member of PipelineHandlerRPi. Could it be moved to RPiCameraData ?\n>\n\nYes, I can do that.\n\n\n>\n> > > > > > +{\n> > > > > > + const MediaPad *sinkPad = link->sink();\n> > > > > > + const MediaEntity *entity = sinkPad->entity();\n> > > > > > + bool unicamFound = false;\n> > > > > > +\n> > > > > > + /* We only deal with Video Mux and Bridge devices in\n> cascade. */\n> > > > > > + if (entity->function() != MEDIA_ENT_F_VID_MUX &&\n> > > > > > + entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)\n> > > > > > + return;\n> > > > > > +\n> > > > > > + LOG(RPI, Info) << \"Found video mux device \" <<\n> entity->name()\n> > > > > > + << \" linked to sink pad \" <<\n> sinkPad->index();\n> > > > > > +\n> > > > > > +\n> data->bridgeDevices_.emplace_back(std::make_unique(entity),link);\n> > > > > > + data->bridgeDevices_.back().first->open();\n> > > > >\n> > > > > Going through the above, I can't help but wonder if the\n> bridgeDevices_\n> > > > > should be stored as a single instance in the Pipeline handler (not\n> each\n> > > > > CameraData, we have one CameraData per camera right? if so, how\n> does\n> > > > > camera3 disable all the links? does it know about every path?)\n> > > >\n> > > > From my description earlier, bridgeDevices_ must be stored in the\n> CameraData, as\n> > > > it lists the devices in the path between the sensor and Unicam. And\n> this is unique\n> > > > per-sensor. Again, this does mean that if we are using Sensor1,\n> links for Sensor{2,3}\n> > > > -> Mux are not changed, but the Mux->Mux link will be disabled. Do\n> you think\n> > > > that may not be appropriate leaving them enabled?\n>\n> I can't help but seeing lots of similarities with the simple pipeline\n> handler, which also performs dynamic discovery of pipelines (all the way\n> to the video node in that case, while in your case it would be all the\n> way to the Unicam device). I wonder if we could share code between the\n> two ? This could benefit the Raspberry Pi pipeline handler by providing\n> a more generic version of format propagation through the pipeline\n> (although that part may be more difficult to share).\n>\n> An important difference between the two implementations is that the\n> simple pipeline handler opens the video node and subdevs once only,\n> storing them in the pipeline handler instance, and only stores pointers\n> to those objects in the camera data.\n>\n\nI've had a very brief look at the simple pipeline handler. I agree, there\nare\nsimilarities between the two. I see that the enumeration/discovery in the\nsimple pipeline handler is a fair bit more sophisticated in finding all\navailable\nentities, whereas the RPi case, I only care to look for a cascade of mux\nor bridge devices.\n\nWhat were your thoughts on trying to share functionality between these two\n(and indeed all) pipeline handlers? Perhaps one way would be to have some\ncommon code to create the media graph structure for any given media device?\nThis could include entities, pads and links represented in a common way for\npipeline handlers to use? Then the RPi enumeration code would simply look\nto find the devices it cares about. This is probably a bigger job than is\nrequired\nfor this change though.\n\nRegards,\nNaush\n\n\n> > > I missed that, and now it's obvious. If the first mux is only pointing\n> > > to Sensor1, then indeed it doesn't matter what mux2 is configured to\n> > > link to...\n> > >\n> > > So I think that's ok.\n> > >\n> > > > Please shout if this doesn't make sense, and something simpler might\n> > > > equally work :-)\n> > >\n> > > It's all fine until someone adds a crossbar perhaps that might make it\n> > > possible to do more crazy things ;-)\n> > >\n> > > But we don't need to worry about that now I dont' think.\n> > >\n> > > I think I'm still concerned about that ret during the loop in\n> > > configure(). But with that resolved:\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > > > +\n> > > > > > + for (const MediaPad *pad : entity->pads()) {\n> > > > > > + /*\n> > > > > > + * Iterate through all the sink pads down the\n> cascade to find any\n> > > > > > + * other Video Mux and Bridge devices.\n> > > > > > + */\n> > > > > > + if (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > > > > > + continue;\n> > > > > > +\n> > > > > > + for (MediaLink *l : pad->links()) {\n> > > > > > + enumerateVideoDevices(data, l);\n> > > > > > + if (l->sink()->entity()->name() ==\n> \"unicam-image\")\n> > > > > > + unicamFound = true;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + /* This identifies the end of our entity enumeration\n> recursion. */\n> > > > > > + if (link->source()->entity()->function() ==\n> MEDIA_ENT_F_CAM_SENSOR) {\n> > > > > > + /*\n> > > > > > + * If Unicam is not at the end of this cascade,\n> we cannot configure\n> > > > > > + * this topology automatically, so remove all\n> entity references.\n> > > > > > + */\n> > > > > > + if (!unicamFound) {\n> > > > > > + LOG(RPI, Warning) << \"Cannot\n> automatically configure this MC topology!\";\n> > > > > > + data->bridgeDevices_.clear();\n> > > > > > + }\n> > > > > > + }\n> > > > > > +}\n> > > > > > +\n> > > > > > int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > > > > > {\n> > > > > > RPiCameraData *data = cameraData(camera);\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 AC84ABF415\n\tfor ;\n\tFri, 10 Dec 2021 15:48:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86B3360897;\n\tFri, 10 Dec 2021 16:48:38 +0100 (CET)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 87E5360868\n\tfor ;\n\tFri, 10 Dec 2021 16:48:36 +0100 (CET)","by mail-lf1-x130.google.com with SMTP id l22so18800676lfg.7\n\tfor ;\n\tFri, 10 Dec 2021 07:48:36 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"b7VvFFfc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=LUhR80kkz/6qv5TzRP5jND9mCjJNQXZywkcblaDaDJs=;\n\tb=b7VvFFfcOVylMzix+mNM6H6DvIbIC9VkEwy5+fnvyoRbBTmMkhgI7lvmLqmWweOfmK\n\tgTtRQ1Z4fa0nkd65kz8kJDtT1iJvWAzQ7p3Y03aSj7Un2tDI+XeSrd22h1fr2HZ87RHT\n\toILFCt20oSsqq/sKkFfapM22j2mS5ckrPLJdECnuNbdAD+bcuhmSo4ezULJmD4MP3Bfk\n\t5wA1DSRNhfo/KTNGDlO//oUbRWIAD/ioXYrDjolAAKDp2mYTpZcvF2A0cWM/qmdYE+rc\n\tWiNK/K1YyjrSzcNjTwwSvATBwyucZuWC9ct1Q/ADXne+14mZFy33OMKzeCkz7W5V1spE\n\tfoIg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=LUhR80kkz/6qv5TzRP5jND9mCjJNQXZywkcblaDaDJs=;\n\tb=29+BI9f/uX5ptIz2p8rRkS6swEcQngsPI+GOW+q8e0hiNdVdTTym7hUR2Me03C6oJt\n\tYCUGp7J4qs8Wb05CQHQx++PZ1FKRr2IKuY1SbR3cAiqRoGKPsULgQuIShHtfE7WAIZwC\n\twLO0nT6YXgkZb9rfFjlPQikggp9fgHZl++otnwHTnF+UasxdE0rX4lzdWkK5bEx3TnEO\n\twzSqu8y5Cjox9kvVxiL6RXsREnxIDympvaZIAdbxUB7xhjYErgBWPaGsTWJS08ItkA3f\n\tffNR8S1fqFMMnxfRJ5LcK+oVsPEufgW6IDftJQTfegjn/L8CmhSV+/H6JBqYTRvzDDRq\n\te5Bw==","X-Gm-Message-State":"AOAM530b5a2ZgREkqKSU6wGlTklo8yn92mCmKc640Mf3QT+mIAaisCZC\n\tWiJKJDy9MrSzK25wujmvRdvosF+UKkCC2rrtqw5oWXkhdwmSMJVX","X-Google-Smtp-Source":"ABdhPJxOokcE0i6EdZOyzIzs9Kg1/2i7a4t9nRP9HlKR9/rPunk5j9km59QVL1kpzxsQR5Diz32TOvInlWtLn1p/ftM=","X-Received":"by 2002:a05:6512:39c4:: with SMTP id\n\tk4mr13059282lfu.79.1639151315433; \n\tFri, 10 Dec 2021 07:48:35 -0800 (PST)","MIME-Version":"1.0","References":"<20211208151527.1404715-1-naush@raspberrypi.com>\n\t<20211208151527.1404715-3-naush@raspberrypi.com>\n\t<163904277369.2066819.17308357219840915893@Monstersaurus>\n\t\n\t<163904552543.2066819.12592034208871016560@Monstersaurus>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 10 Dec 2021 15:48:19 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000e3cfc005d2cca718\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21765,"web_url":"https://patchwork.libcamera.org/comment/21765/","msgid":"","date":"2021-12-10T23:23:29","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Fri, Dec 10, 2021 at 03:48:19PM +0000, Naushir Patuck wrote:\n> On Fri, 10 Dec 2021 at 00:24, Laurent Pinchart wrote:\n> > On Thu, Dec 09, 2021 at 10:29:21AM +0000, Naushir Patuck wrote:\n> > > On Thu, 9 Dec 2021 at 10:25, Kieran Bingham wrote:\n> > > > Quoting Naushir Patuck (2021-12-09 10:07:43)\n> > > > > On Thu, 9 Dec 2021 at 09:39, Kieran Bingham wrote:\n> > > > > > Quoting Naushir Patuck (2021-12-08 15:15:27)\n> > > > > > > This change will allow the pipeline handler to enumerate and control Video\n> > > > > > > Mux or Bridge devices that may be attached between sensors and a particular\n> > > > > > > Unicam instance. Cascaded mux or bridge devices are also handled.\n> > > > > > >\n> > > > > > > A new member function enumerateVideoDevices(), called from registerCamera(), is\n> > > > > > > used to identify and open all mux and bridge subdevices present in the\n> > > > > > > sensor -> Unicam link.\n> > > > > > >\n> > > > > > > Relevent links are enabled/disabled and pad formats correctly set in configure()\n> > > > > > > before the camera is started.\n> > > > > > >\n> > > > > > > Signed-off-by: Naushir Patuck \n> > > > > > > ---\n> > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 78 +++++++++++++++++++\n> > > > > > > 1 file changed, 78 insertions(+)\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > index 756878c70036..ca176ecb40ec 100644\n> > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > @@ -12,6 +12,7 @@\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > +#include \n> > > > > > >\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > @@ -220,6 +221,11 @@ public:\n> > > > > > > std::vector streams_;\n> > > > > > > /* Stores the ids of the buffers mapped in the IPA. */\n> > > > > > > std::unordered_set ipaBuffers_;\n> > > > > > > + /*\n> > > > > > > + * Stores a cascade of Video Mux or Bridge devices between the sensor and\n> > > > > > > + * Unicam together with media link across the entities.\n> > > > > > > + */\n> > > > > > > + std::vector, MediaLink *>> bridgeDevices_;\n> > > > > > >\n> > > > > > > /* DMAHEAP allocation helper. */\n> > > > > > > RPi::DmaHeap dmaHeap_;\n> > > > > > > @@ -311,6 +317,7 @@ private:\n> > > > > > > }\n> > > > > > >\n> > > > > > > int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);\n> > > > > > > + void enumerateVideoDevices(RPiCameraData *data, MediaLink *link);\n> > > > > > > int queueAllBuffers(Camera *camera);\n> > > > > > > int prepareBuffers(Camera *camera);\n> > > > > > > void freeBuffers(Camera *camera);\n> > > > > > > @@ -868,6 +875,25 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > > > > */\n> > > > > > > data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> > > > > > >\n> > > > > > > + /* Setup the Video Mux/Bridge entities. */\n> > > > > > > + for (auto &[device, link] : data->bridgeDevices_) {\n> > > > > > > + /* Start by disabling all the sink pad links on the devices in the cascade. */\n> > > > > > > + for (const MediaPad *p : device->entity()->pads()) {\n> > > > > > > + if (!(p->flags() & MEDIA_PAD_FL_SINK))\n> > > > > > > + continue;\n> > > > > > > +\n> > > > > > > + for (MediaLink *l : p->links())\n> > > > > > > + l->setEnabled(false);\n> >\n> > As an optimization, you could skip disabling the link that you will\n> > enable below.\n> \n> Ack.\n> \n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + /* Finally enable the link, and setup the pad format. */\n> > > > > > > + link->setEnabled(true);\n> > > > > >\n> > > > > > Isn't this going to enable /all/ bridge links in the cascade\n> > > > > > incorrectly?\n> > > > > >\n> > > > > >\n> > > > > > ┌──────────┐\n> > > > > > │ Unicam │\n> > > > > > └─────▲────┘\n> > > > > > │\n> > > > > > ┌───┴───┐\n> > > > > > │ Mux ◄───────┐\n> > > > > > └──▲────┘ │\n> > > > > > │ │\n> > > > > > ┌─────┴───┐ ┌───┴───┐\n> > > > > > │ Sensor1 │ │ Mux │◄──┐\n> > > > > > └─────────┘ └─▲─────┘ │\n> > > > > > │ │\n> > > > > > ┌───────┴─┐ ┌───┴─────┐\n> > > > > > │ Sensor2 │ │ Sensor3 │\n> > > > > > └─────────┘ └─────────┘\n> > > > > >\n> > > > > > In that 'use case' we're now iterating over all bridges and enabling\n> > > > > > their link, I think which means we've just enabled both muxes,\n> > > > > > immediately after disabling them - even for Sensor1?\n> > > > > >\n> > > > > > Maybe I've mis-understood it, but I thought there would be something\n> > > > > > that would start at the desired sensor, and walk up the links to the\n> > > > > > unicam enabling those links (and only those) as it went up?\n> > > > > >\n> > > > > > The walk only has to be done once too, so perhaps a per-camera(sensor)\n> > > > > > vector of links to iterate and enable?\n> > > > > >\n> > > > > > Or maybe I'm missing the separation between the bridgeDevices_ and the\n> > > > > > per-camera instances. But if that's already happening, I can't then see\n> > > > > > how each camera data clears all the bridges used by other cameras...\n> > > > >\n> > > > > Let me explain my intention, and you can then tell me if the code does\n> > > > > what I think it does :-)\n> > > > >\n> > > > > Each sensor (Sensor{1,2,3}) will register its own Camera object, and\n> > > > > in that object bridgeDevices_ will store the cascade of muxes between\n> > > > > the sensor and the Unicam port. So, for Sensor1, we store only 1 mux,\n> > > > > and Sensor{2,3} will store both muxes. Together with the mux device,\n> > > > > we also store the entity to entity links.\n> > > > >\n> > > > > The above code goes through those stored entities, first disabling *all*\n> > > > > links on each device in the chain, and then selectively enabling\n> > > > > the specific links that are stored in bridgeDevices_ to link sensor\n> > > > > to Unicam across all intermedia muxes. If Sensor1 is used, this\n> > > > > does mean that the Sensor{2,3} -> Mux links might not change\n> > > > > state but the Mux to Mux link will be disabled. Similarly, if we are driving\n> > > > > Sensor3, the Sensor{1,2} -> Mux link will be disabled.\n> > > >\n> > > > I think I see the obvious part I missed ;-)\n> > > >\n> > > > It doesn't matter what configuration Mux2 has as long as Mux1 only has\n> > > > Sensor1 link enabled!\n> > > >\n> > > > It might help to document that in the comments somehow somewhere?\n> > > > Particularly now that you've already written the explanation to me -\n> > > > perhaps it should be a block comment above enumerateVideoDevices.\n> > > >\n> > > > Feel free to take or add the ascii diagram, I think more pictures in\n> > > > our documentation are always helpful. It's really easy to make those\n> > > > block diagrams in ascii art at https://asciiflow.com.\n> > >\n> > > Sure, I'll add some comments through the code explaining it in more detail.\n> > >\n> > > > > > > + const MediaPad *srcPad = link->sink();\n> > > > > > > + ret = device->setFormat(srcPad->index(), &sensorFormat);\n> > > > > >\n> > > > > > This assignment of ret is in a loop, so earlier failures are going to\n> > > > > > get ignored, unchecked.\n> > > > >\n> > > > > This should not matter, as ret is only used in the loop to count successful\n> > > > > camera registrations. The return value from match() will be false only if\n> > > > > 0 cameras were registered.\n> > > >\n> > > > So should we not even assign it?\n> > > > Do we need to report a warning if it failed to set the format?\n> > > >\n> > > > But aren't we here in configure() setting the format of the device to\n> > > > propogate the format?\n> > > >\n> > > > If we fail to setFormat in that case, then I think we should be saying\n> > > > the configure() call failed?\n> > >\n> > > Oops, my mistake. I misread your original comment and what bit of\n> > > code it refers to! This ret value *is* used for the return path, and must\n> > > be accounted for correctly as it is set in a loop! I'll fix this in the next\n> > > revision.\n> > >\n> > > > > > > + LOG(RPI, Info) << \"Configured media link on device \" << device->entity()->name()\n> > > > > > > + << \" at pad \" << srcPad->index();\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > return ret;\n> > > > > > > }\n> > > > > > >\n> > > > > > > @@ -1098,6 +1124,13 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > > > > > if (data->sensor_->init())\n> > > > > > > return -EINVAL;\n> > > > > > >\n> > > > > > > + /*\n> > > > > > > + * Enumerate all the Video Mux/Bridge devices across the sensor -> unicam\n> > > > > > > + * link. There may be a cascade of devices in this link!\n> > > > > > > + */\n> > > > > > > + MediaLink *link = sensorEntity->getPadByIndex(0)->links()[0];\n> > > > > > > + enumerateVideoDevices(data.get(), link);\n> > > > > > > +\n> > > > > > > data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > > > > > >\n> > > > > > > ipa::RPi::SensorConfig sensorConfig;\n> > > > > > > @@ -1224,6 +1257,51 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > > > > > return 0;\n> > > > > > > }\n> > > > > > >\n> > > > > > > +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data, MediaLink *link)\n> >\n> > This function accesses members of data but doesn't seem to access any\n> > member of PipelineHandlerRPi. Could it be moved to RPiCameraData ?\n> \n> Yes, I can do that.\n> \n> > > > > > > +{\n> > > > > > > + const MediaPad *sinkPad = link->sink();\n> > > > > > > + const MediaEntity *entity = sinkPad->entity();\n> > > > > > > + bool unicamFound = false;\n> > > > > > > +\n> > > > > > > + /* We only deal with Video Mux and Bridge devices in cascade. */\n> > > > > > > + if (entity->function() != MEDIA_ENT_F_VID_MUX &&\n> > > > > > > + entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)\n> > > > > > > + return;\n> > > > > > > +\n> > > > > > > + LOG(RPI, Info) << \"Found video mux device \" << entity->name()\n> > > > > > > + << \" linked to sink pad \" << sinkPad->index();\n> > > > > > > +\n> > > > > > > + data->bridgeDevices_.emplace_back(std::make_unique(entity),link);\n> > > > > > > + data->bridgeDevices_.back().first->open();\n> > > > > >\n> > > > > > Going through the above, I can't help but wonder if the bridgeDevices_\n> > > > > > should be stored as a single instance in the Pipeline handler (not each\n> > > > > > CameraData, we have one CameraData per camera right? if so, how does\n> > > > > > camera3 disable all the links? does it know about every path?)\n> > > > >\n> > > > > From my description earlier, bridgeDevices_ must be stored in the CameraData, as\n> > > > > it lists the devices in the path between the sensor and Unicam. And this is unique\n> > > > > per-sensor. Again, this does mean that if we are using Sensor1, links for Sensor{2,3}\n> > > > > -> Mux are not changed, but the Mux->Mux link will be disabled. Do you think\n> > > > > that may not be appropriate leaving them enabled?\n> >\n> > I can't help but seeing lots of similarities with the simple pipeline\n> > handler, which also performs dynamic discovery of pipelines (all the way\n> > to the video node in that case, while in your case it would be all the\n> > way to the Unicam device). I wonder if we could share code between the\n> > two ? This could benefit the Raspberry Pi pipeline handler by providing\n> > a more generic version of format propagation through the pipeline\n> > (although that part may be more difficult to share).\n> >\n> > An important difference between the two implementations is that the\n> > simple pipeline handler opens the video node and subdevs once only,\n> > storing them in the pipeline handler instance, and only stores pointers\n> > to those objects in the camera data.\n> \n> I've had a very brief look at the simple pipeline handler. I agree, there are\n> similarities between the two. I see that the enumeration/discovery in the\n> simple pipeline handler is a fair bit more sophisticated in finding all available\n> entities, whereas the RPi case, I only care to look for a cascade of mux\n> or bridge devices.\n> \n> What were your thoughts on trying to share functionality between these two\n> (and indeed all) pipeline handlers? Perhaps one way would be to have some\n> common code to create the media graph structure for any given media device?\n> This could include entities, pads and links represented in a common way for\n> pipeline handlers to use? Then the RPi enumeration code would simply look\n> to find the devices it cares about. This is probably a bigger job than is required\n> for this change though.\n\nThat's a very good question :-)\n\nI don't usually believe in designing helpers without at least a couple\nof use cases for them, that more often than not leads to bad designs.\nNow we have two examples of code that needs to parse a media graph, and\nI expect the exact same feature could be implemented by all pipeline\nhandlers.\n\nFollowing the design-from-use-cases philosophy, I think we should focus\non the need at hand. I think it would be able to share code that parses\na media graph to locate sensors and paths from them all the way up to a\ngiven entity (Unicam in your case, any video node in the simple pipeline\nhandler case), and store the paths in a shared data structure. Functions\nto then setup the links in the pipeline seem to also be good candidates\nfor a common helper.\n\nFormat configuration along the pipeline could also possibly be shared,\nbut that seems more difficult, as the needs appear to be a bit\ndifferent.\n\nI do agree this is a bit of yak shaving, as it's not strictly required\nto get this feature implemented, but we also have a growing technical\ndebt. I was thus wondering if you think you could have time to\nexperiment a little bit.\n\n> > > > I missed that, and now it's obvious. If the first mux is only pointing\n> > > > to Sensor1, then indeed it doesn't matter what mux2 is configured to\n> > > > link to...\n> > > >\n> > > > So I think that's ok.\n> > > >\n> > > > > Please shout if this doesn't make sense, and something simpler might\n> > > > > equally work :-)\n> > > >\n> > > > It's all fine until someone adds a crossbar perhaps that might make it\n> > > > possible to do more crazy things ;-)\n> > > >\n> > > > But we don't need to worry about that now I dont' think.\n> > > >\n> > > > I think I'm still concerned about that ret during the loop in\n> > > > configure(). But with that resolved:\n> > > >\n> > > > Reviewed-by: Kieran Bingham \n> > > >\n> > > > > > > +\n> > > > > > > + for (const MediaPad *pad : entity->pads()) {\n> > > > > > > + /*\n> > > > > > > + * Iterate through all the sink pads down the cascade to find any\n> > > > > > > + * other Video Mux and Bridge devices.\n> > > > > > > + */\n> > > > > > > + if (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > > > > > > + continue;\n> > > > > > > +\n> > > > > > > + for (MediaLink *l : pad->links()) {\n> > > > > > > + enumerateVideoDevices(data, l);\n> > > > > > > + if (l->sink()->entity()->name() == \"unicam-image\")\n> > > > > > > + unicamFound = true;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + /* This identifies the end of our entity enumeration recursion. */\n> > > > > > > + if (link->source()->entity()->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> > > > > > > + /*\n> > > > > > > + * If Unicam is not at the end of this cascade, we cannot configure\n> > > > > > > + * this topology automatically, so remove all entity references.\n> > > > > > > + */\n> > > > > > > + if (!unicamFound) {\n> > > > > > > + LOG(RPI, Warning) << \"Cannot automatically configure this MC topology!\";\n> > > > > > > + data->bridgeDevices_.clear();\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > > > > > > {\n> > > > > > > RPiCameraData *data = cameraData(camera);","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 2A7B4BF415\n\tfor ;\n\tFri, 10 Dec 2021 23:24:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D0AD6087E;\n\tSat, 11 Dec 2021 00:24:02 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D54860114\n\tfor ;\n\tSat, 11 Dec 2021 00:24:00 +0100 (CET)","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 E6799BBE;\n\tSat, 11 Dec 2021 00:23:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"RDL1G6ET\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639178640;\n\tbh=zF50ccv7/AbkmcGl2LaW5rFq0a6CpUUfmiN8G1s/1ko=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RDL1G6ETapzI2Qk4vz2DMbTwI3rc3L3jW+UIIKn31hD9hPgr8/Wmi5X0jjPDYd1+3\n\tjdzvB8uyeyUH0AynxHkBXs1xRAJn5lMLUR3HIXxtim1G1lJOXC3c6OJJGHKo7oSFet\n\tjmlZ/Pp8msHNGulJ7YglEuHXfXT1LEezO60Ghe48=","Date":"Sat, 11 Dec 2021 01:23:29 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211208151527.1404715-1-naush@raspberrypi.com>\n\t<20211208151527.1404715-3-naush@raspberrypi.com>\n\t<163904277369.2066819.17308357219840915893@Monstersaurus>\n\t\n\t<163904552543.2066819.12592034208871016560@Monstersaurus>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21774,"web_url":"https://patchwork.libcamera.org/comment/21774/","msgid":"","date":"2021-12-14T08:29:39","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Fri, 10 Dec 2021 at 23:24, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Fri, Dec 10, 2021 at 03:48:19PM +0000, Naushir Patuck wrote:\n> > On Fri, 10 Dec 2021 at 00:24, Laurent Pinchart wrote:\n> > > On Thu, Dec 09, 2021 at 10:29:21AM +0000, Naushir Patuck wrote:\n> > > > On Thu, 9 Dec 2021 at 10:25, Kieran Bingham wrote:\n> > > > > Quoting Naushir Patuck (2021-12-09 10:07:43)\n> > > > > > On Thu, 9 Dec 2021 at 09:39, Kieran Bingham wrote:\n> > > > > > > Quoting Naushir Patuck (2021-12-08 15:15:27)\n> > > > > > > > This change will allow the pipeline handler to enumerate and\n> control Video\n> > > > > > > > Mux or Bridge devices that may be attached between sensors\n> and a particular\n> > > > > > > > Unicam instance. Cascaded mux or bridge devices are also\n> handled.\n> > > > > > > >\n> > > > > > > > A new member function enumerateVideoDevices(), called from\n> registerCamera(), is\n> > > > > > > > used to identify and open all mux and bridge subdevices\n> present in the\n> > > > > > > > sensor -> Unicam link.\n> > > > > > > >\n> > > > > > > > Relevent links are enabled/disabled and pad formats\n> correctly set in configure()\n> > > > > > > > before the camera is started.\n> > > > > > > >\n> > > > > > > > Signed-off-by: Naushir Patuck \n> > > > > > > > ---\n> > > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 78\n> +++++++++++++++++++\n> > > > > > > > 1 file changed, 78 insertions(+)\n> > > > > > > >\n> > > > > > > > diff --git\n> a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > > index 756878c70036..ca176ecb40ec 100644\n> > > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > > @@ -12,6 +12,7 @@\n> > > > > > > > #include \n> > > > > > > > #include \n> > > > > > > > #include \n> > > > > > > > +#include \n> > > > > > > >\n> > > > > > > > #include \n> > > > > > > > #include \n> > > > > > > > @@ -220,6 +221,11 @@ public:\n> > > > > > > > std::vector streams_;\n> > > > > > > > /* Stores the ids of the buffers mapped in the IPA.\n> */\n> > > > > > > > std::unordered_set ipaBuffers_;\n> > > > > > > > + /*\n> > > > > > > > + * Stores a cascade of Video Mux or Bridge devices\n> between the sensor and\n> > > > > > > > + * Unicam together with media link across the\n> entities.\n> > > > > > > > + */\n> > > > > > > > +\n> std::vector, MediaLink *>>\n> bridgeDevices_;\n> > > > > > > >\n> > > > > > > > /* DMAHEAP allocation helper. */\n> > > > > > > > RPi::DmaHeap dmaHeap_;\n> > > > > > > > @@ -311,6 +317,7 @@ private:\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > int registerCamera(MediaDevice *unicam, MediaDevice\n> *isp, MediaEntity *sensorEntity);\n> > > > > > > > + void enumerateVideoDevices(RPiCameraData *data,\n> MediaLink *link);\n> > > > > > > > int queueAllBuffers(Camera *camera);\n> > > > > > > > int prepareBuffers(Camera *camera);\n> > > > > > > > void freeBuffers(Camera *camera);\n> > > > > > > > @@ -868,6 +875,25 @@ int\n> PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > > > > > */\n> > > > > > > > data->properties_.set(properties::ScalerCropMaximum,\n> data->sensorInfo_.analogCrop);\n> > > > > > > >\n> > > > > > > > + /* Setup the Video Mux/Bridge entities. */\n> > > > > > > > + for (auto &[device, link] : data->bridgeDevices_) {\n> > > > > > > > + /* Start by disabling all the sink pad links\n> on the devices in the cascade. */\n> > > > > > > > + for (const MediaPad *p :\n> device->entity()->pads()) {\n> > > > > > > > + if (!(p->flags() &\n> MEDIA_PAD_FL_SINK))\n> > > > > > > > + continue;\n> > > > > > > > +\n> > > > > > > > + for (MediaLink *l : p->links())\n> > > > > > > > + l->setEnabled(false);\n> > >\n> > > As an optimization, you could skip disabling the link that you will\n> > > enable below.\n> >\n> > Ack.\n> >\n> > > > > > > > + }\n> > > > > > > > +\n> > > > > > > > + /* Finally enable the link, and setup the\n> pad format. */\n> > > > > > > > + link->setEnabled(true);\n> > > > > > >\n> > > > > > > Isn't this going to enable /all/ bridge links in the cascade\n> > > > > > > incorrectly?\n> > > > > > >\n> > > > > > >\n> > > > > > > ┌──────────┐\n> > > > > > > │ Unicam │\n> > > > > > > └─────▲────┘\n> > > > > > > │\n> > > > > > > ┌───┴───┐\n> > > > > > > │ Mux ◄───────┐\n> > > > > > > └──▲────┘ │\n> > > > > > > │ │\n> > > > > > > ┌─────┴───┐ ┌───┴───┐\n> > > > > > > │ Sensor1 │ │ Mux │◄──┐\n> > > > > > > └─────────┘ └─▲─────┘ │\n> > > > > > > │ │\n> > > > > > > ┌───────┴─┐ ┌───┴─────┐\n> > > > > > > │ Sensor2 │ │ Sensor3 │\n> > > > > > > └─────────┘ └─────────┘\n> > > > > > >\n> > > > > > > In that 'use case' we're now iterating over all bridges and\n> enabling\n> > > > > > > their link, I think which means we've just enabled both muxes,\n> > > > > > > immediately after disabling them - even for Sensor1?\n> > > > > > >\n> > > > > > > Maybe I've mis-understood it, but I thought there would be\n> something\n> > > > > > > that would start at the desired sensor, and walk up the links\n> to the\n> > > > > > > unicam enabling those links (and only those) as it went up?\n> > > > > > >\n> > > > > > > The walk only has to be done once too, so perhaps a\n> per-camera(sensor)\n> > > > > > > vector of links to iterate and enable?\n> > > > > > >\n> > > > > > > Or maybe I'm missing the separation between the bridgeDevices_\n> and the\n> > > > > > > per-camera instances. But if that's already happening, I can't\n> then see\n> > > > > > > how each camera data clears all the bridges used by other\n> cameras...\n> > > > > >\n> > > > > > Let me explain my intention, and you can then tell me if the\n> code does\n> > > > > > what I think it does :-)\n> > > > > >\n> > > > > > Each sensor (Sensor{1,2,3}) will register its own Camera object,\n> and\n> > > > > > in that object bridgeDevices_ will store the cascade of muxes\n> between\n> > > > > > the sensor and the Unicam port. So, for Sensor1, we store only 1\n> mux,\n> > > > > > and Sensor{2,3} will store both muxes. Together with the mux\n> device,\n> > > > > > we also store the entity to entity links.\n> > > > > >\n> > > > > > The above code goes through those stored entities, first\n> disabling *all*\n> > > > > > links on each device in the chain, and then selectively enabling\n> > > > > > the specific links that are stored in bridgeDevices_ to link\n> sensor\n> > > > > > to Unicam across all intermedia muxes. If Sensor1 is used, this\n> > > > > > does mean that the Sensor{2,3} -> Mux links might not change\n> > > > > > state but the Mux to Mux link will be disabled. Similarly, if\n> we are driving\n> > > > > > Sensor3, the Sensor{1,2} -> Mux link will be disabled.\n> > > > >\n> > > > > I think I see the obvious part I missed ;-)\n> > > > >\n> > > > > It doesn't matter what configuration Mux2 has as long as Mux1 only\n> has\n> > > > > Sensor1 link enabled!\n> > > > >\n> > > > > It might help to document that in the comments somehow somewhere?\n> > > > > Particularly now that you've already written the explanation to me\n> -\n> > > > > perhaps it should be a block comment above enumerateVideoDevices.\n> > > > >\n> > > > > Feel free to take or add the ascii diagram, I think more pictures\n> in\n> > > > > our documentation are always helpful. It's really easy to make\n> those\n> > > > > block diagrams in ascii art at https://asciiflow.com.\n> > > >\n> > > > Sure, I'll add some comments through the code explaining it in more\n> detail.\n> > > >\n> > > > > > > > + const MediaPad *srcPad = link->sink();\n> > > > > > > > + ret = device->setFormat(srcPad->index(),\n> &sensorFormat);\n> > > > > > >\n> > > > > > > This assignment of ret is in a loop, so earlier failures are\n> going to\n> > > > > > > get ignored, unchecked.\n> > > > > >\n> > > > > > This should not matter, as ret is only used in the loop to count\n> successful\n> > > > > > camera registrations. The return value from match() will be\n> false only if\n> > > > > > 0 cameras were registered.\n> > > > >\n> > > > > So should we not even assign it?\n> > > > > Do we need to report a warning if it failed to set the format?\n> > > > >\n> > > > > But aren't we here in configure() setting the format of the device\n> to\n> > > > > propogate the format?\n> > > > >\n> > > > > If we fail to setFormat in that case, then I think we should be\n> saying\n> > > > > the configure() call failed?\n> > > >\n> > > > Oops, my mistake. I misread your original comment and what bit of\n> > > > code it refers to! This ret value *is* used for the return path,\n> and must\n> > > > be accounted for correctly as it is set in a loop! I'll fix this in\n> the next\n> > > > revision.\n> > > >\n> > > > > > > > + LOG(RPI, Info) << \"Configured media link on\n> device \" << device->entity()->name()\n> > > > > > > > + << \" at pad \" <<\n> srcPad->index();\n> > > > > > > > + }\n> > > > > > > > +\n> > > > > > > > return ret;\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > @@ -1098,6 +1124,13 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > > > > > > if (data->sensor_->init())\n> > > > > > > > return -EINVAL;\n> > > > > > > >\n> > > > > > > > + /*\n> > > > > > > > + * Enumerate all the Video Mux/Bridge devices across\n> the sensor -> unicam\n> > > > > > > > + * link. There may be a cascade of devices in this\n> link!\n> > > > > > > > + */\n> > > > > > > > + MediaLink *link =\n> sensorEntity->getPadByIndex(0)->links()[0];\n> > > > > > > > + enumerateVideoDevices(data.get(), link);\n> > > > > > > > +\n> > > > > > > > data->sensorFormats_ =\n> populateSensorFormats(data->sensor_);\n> > > > > > > >\n> > > > > > > > ipa::RPi::SensorConfig sensorConfig;\n> > > > > > > > @@ -1224,6 +1257,51 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > > > > > > return 0;\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > +void\n> PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data, MediaLink\n> *link)\n> > >\n> > > This function accesses members of data but doesn't seem to access any\n> > > member of PipelineHandlerRPi. Could it be moved to RPiCameraData ?\n> >\n> > Yes, I can do that.\n> >\n> > > > > > > > +{\n> > > > > > > > + const MediaPad *sinkPad = link->sink();\n> > > > > > > > + const MediaEntity *entity = sinkPad->entity();\n> > > > > > > > + bool unicamFound = false;\n> > > > > > > > +\n> > > > > > > > + /* We only deal with Video Mux and Bridge devices in\n> cascade. */\n> > > > > > > > + if (entity->function() != MEDIA_ENT_F_VID_MUX &&\n> > > > > > > > + entity->function() != MEDIA_ENT_F_VID_IF_BRIDGE)\n> > > > > > > > + return;\n> > > > > > > > +\n> > > > > > > > + LOG(RPI, Info) << \"Found video mux device \" <<\n> entity->name()\n> > > > > > > > + << \" linked to sink pad \" <<\n> sinkPad->index();\n> > > > > > > > +\n> > > > > > > > +\n> data->bridgeDevices_.emplace_back(std::make_unique(entity),link);\n> > > > > > > > + data->bridgeDevices_.back().first->open();\n> > > > > > >\n> > > > > > > Going through the above, I can't help but wonder if the\n> bridgeDevices_\n> > > > > > > should be stored as a single instance in the Pipeline handler\n> (not each\n> > > > > > > CameraData, we have one CameraData per camera right? if so,\n> how does\n> > > > > > > camera3 disable all the links? does it know about every path?)\n> > > > > >\n> > > > > > From my description earlier, bridgeDevices_ must be stored in\n> the CameraData, as\n> > > > > > it lists the devices in the path between the sensor and Unicam.\n> And this is unique\n> > > > > > per-sensor. Again, this does mean that if we are using Sensor1,\n> links for Sensor{2,3}\n> > > > > > -> Mux are not changed, but the Mux->Mux link will be disabled.\n> Do you think\n> > > > > > that may not be appropriate leaving them enabled?\n> > >\n> > > I can't help but seeing lots of similarities with the simple pipeline\n> > > handler, which also performs dynamic discovery of pipelines (all the\n> way\n> > > to the video node in that case, while in your case it would be all the\n> > > way to the Unicam device). I wonder if we could share code between the\n> > > two ? This could benefit the Raspberry Pi pipeline handler by providing\n> > > a more generic version of format propagation through the pipeline\n> > > (although that part may be more difficult to share).\n> > >\n> > > An important difference between the two implementations is that the\n> > > simple pipeline handler opens the video node and subdevs once only,\n> > > storing them in the pipeline handler instance, and only stores pointers\n> > > to those objects in the camera data.\n> >\n> > I've had a very brief look at the simple pipeline handler. I agree,\n> there are\n> > similarities between the two. I see that the enumeration/discovery in\n> the\n> > simple pipeline handler is a fair bit more sophisticated in finding all\n> available\n> > entities, whereas the RPi case, I only care to look for a cascade of mux\n> > or bridge devices.\n> >\n> > What were your thoughts on trying to share functionality between these\n> two\n> > (and indeed all) pipeline handlers? Perhaps one way would be to have\n> some\n> > common code to create the media graph structure for any given media\n> device?\n> > This could include entities, pads and links represented in a common way\n> for\n> > pipeline handlers to use? Then the RPi enumeration code would simply\n> look\n> > to find the devices it cares about. This is probably a bigger job than\n> is required\n> > for this change though.\n>\n> That's a very good question :-)\n>\n> I don't usually believe in designing helpers without at least a couple\n> of use cases for them, that more often than not leads to bad designs.\n> Now we have two examples of code that needs to parse a media graph, and\n> I expect the exact same feature could be implemented by all pipeline\n> handlers.\n>\n> Following the design-from-use-cases philosophy, I think we should focus\n> on the need at hand. I think it would be able to share code that parses\n> a media graph to locate sensors and paths from them all the way up to a\n> given entity (Unicam in your case, any video node in the simple pipeline\n> handler case), and store the paths in a shared data structure. Functions\n> to then setup the links in the pipeline seem to also be good candidates\n> for a common helper.\n>\n> Format configuration along the pipeline could also possibly be shared,\n> but that seems more difficult, as the needs appear to be a bit\n> different.\n>\n> I do agree this is a bit of yak shaving, as it's not strictly required\n> to get this feature implemented, but we also have a growing technical\n> debt. I was thus wondering if you think you could have time to\n> experiment a little bit.\n>\n\nSure, I'll see what I can come up with to have some general purpose\nhelpers dealing with media device enumeration/discovery. In the meantime,\nI'll post a new patch with the requested changes soon.\n\nRegard,\nNaush\n\n\n>\n> > > > > I missed that, and now it's obvious. If the first mux is only\n> pointing\n> > > > > to Sensor1, then indeed it doesn't matter what mux2 is configured\n> to\n> > > > > link to...\n> > > > >\n> > > > > So I think that's ok.\n> > > > >\n> > > > > > Please shout if this doesn't make sense, and something simpler\n> might\n> > > > > > equally work :-)\n> > > > >\n> > > > > It's all fine until someone adds a crossbar perhaps that might\n> make it\n> > > > > possible to do more crazy things ;-)\n> > > > >\n> > > > > But we don't need to worry about that now I dont' think.\n> > > > >\n> > > > > I think I'm still concerned about that ret during the loop in\n> > > > > configure(). But with that resolved:\n> > > > >\n> > > > > Reviewed-by: Kieran Bingham \n> > > > >\n> > > > > > > > +\n> > > > > > > > + for (const MediaPad *pad : entity->pads()) {\n> > > > > > > > + /*\n> > > > > > > > + * Iterate through all the sink pads down\n> the cascade to find any\n> > > > > > > > + * other Video Mux and Bridge devices.\n> > > > > > > > + */\n> > > > > > > > + if (!(pad->flags() & MEDIA_PAD_FL_SOURCE))\n> > > > > > > > + continue;\n> > > > > > > > +\n> > > > > > > > + for (MediaLink *l : pad->links()) {\n> > > > > > > > + enumerateVideoDevices(data, l);\n> > > > > > > > + if (l->sink()->entity()->name() ==\n> \"unicam-image\")\n> > > > > > > > + unicamFound = true;\n> > > > > > > > + }\n> > > > > > > > + }\n> > > > > > > > +\n> > > > > > > > + /* This identifies the end of our entity enumeration\n> recursion. */\n> > > > > > > > + if (link->source()->entity()->function() ==\n> MEDIA_ENT_F_CAM_SENSOR) {\n> > > > > > > > + /*\n> > > > > > > > + * If Unicam is not at the end of this\n> cascade, we cannot configure\n> > > > > > > > + * this topology automatically, so remove all\n> entity references.\n> > > > > > > > + */\n> > > > > > > > + if (!unicamFound) {\n> > > > > > > > + LOG(RPI, Warning) << \"Cannot\n> automatically configure this MC topology!\";\n> > > > > > > > + data->bridgeDevices_.clear();\n> > > > > > > > + }\n> > > > > > > > + }\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > > > > > > > {\n> > > > > > > > RPiCameraData *data = cameraData(camera);\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 1F0E8BF415\n\tfor ;\n\tTue, 14 Dec 2021 08:29:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D92A06089D;\n\tTue, 14 Dec 2021 09:29:57 +0100 (CET)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7BF4960117\n\tfor ;\n\tTue, 14 Dec 2021 09:29:56 +0100 (CET)","by mail-lj1-x236.google.com with SMTP id b19so25405059ljr.12\n\tfor ;\n\tTue, 14 Dec 2021 00:29:56 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"lDG4fIMm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=x1VIoUHvJYxEeZSHR32q+alsdTHyPfG+mws0RcbNHOM=;\n\tb=lDG4fIMmdD+VwzHdLIRB+IjtWWC3PT8yQxxV/2cZ2JKakrQYEc3watdG6oz/lgcFOC\n\t3l307x6oofxbupFdwUTJuN4vvo8jo02Gnsl+hFkLt1yg/hy1Gw4y4PIMZHNaS+CI8ZLo\n\t6dj5pFPLy2Y6XmEgllGXrQKlHOYaRH12HY3I/CJ1fy9Ub28jS1JWXha68U5l1un6EmZd\n\tqaO3TnTOwD4kuYuELRrzuFlzzsxWPOs3eY2bbbRZBA941td6wMbhrgoG9yqiDmEgRP+k\n\tYTvygLzO/XthoGyZ8hq/5gNruZA+fj+ENuMcMe3z1bd9sKhnQRUsDUQ/WZcYw7tnKruv\n\tiG1Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=x1VIoUHvJYxEeZSHR32q+alsdTHyPfG+mws0RcbNHOM=;\n\tb=4PGWuJsioLfBXF1EZ0v4LG/e2EEV0VtkztXiv0omCVB8tf6ThrgIjgvN3I/z/81+pN\n\tBioAKpT8zvhatTWMRETrBY9kDfo5GvACWNuxxk6PJUVaALthM8/ovK5S5G0V2l3ZxVNm\n\txxzI9uiEdhpXJBXA1U9aW0Y3bzZdH2B7dFyrdpR1D4o2oa8TjznpFCVHqgi6nG9zXXol\n\tx/xtTtPNfyh389iZ6Va7BGrAtGuqvz/YLW68SL80jcuos1RjPhJoBJfXpO3He0+JoFu3\n\tqs2qrOZfyU4S75ZjuV4urC3guJ0HZqymzrFAJVn6MyiKu0Mf/gTTQVYMA+tNQ1hIrr9L\n\tDLiA==","X-Gm-Message-State":"AOAM532SVaJ2+dsLF73d4LU3cfItYJBpmi7UnwyBa500+mfs0mcZK8/L\n\tkyvhRskSKDKRmYBNZ4piWVMXCUJ7wKiNXF7u8e0X2peFA18=","X-Google-Smtp-Source":"ABdhPJzebqcf1U0UsG8xjIYk11VmovUUxe6wVbGP+1eP1cOwl+J9SiCoe2fXIjU6eEnruAu9Wa2uvouJWADVrd5pq+Q=","X-Received":"by 2002:a2e:b0c5:: with SMTP id g5mr3487193ljl.381.1639470595494;\n\tTue, 14 Dec 2021 00:29:55 -0800 (PST)","MIME-Version":"1.0","References":"<20211208151527.1404715-1-naush@raspberrypi.com>\n\t<20211208151527.1404715-3-naush@raspberrypi.com>\n\t<163904277369.2066819.17308357219840915893@Monstersaurus>\n\t\n\t<163904552543.2066819.12592034208871016560@Monstersaurus>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 14 Dec 2021 08:29:39 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000076e7fa05d316fe94\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Add\n\tsupport for Video Mux and Bridge devices","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]