[{"id":21674,"web_url":"https://patchwork.libcamera.org/comment/21674/","msgid":"<163896642310.1970692.7932799544696435231@Monstersaurus>","date":"2021-12-08T12:27:03","subject":"Re: [libcamera-devel] [PATCH v1 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-01 11:57:11)\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 | 60 +++++++++++++++++++\n> 1 file changed, 60 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 811160490f5c..2512d0746d4a 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -224,6 +225,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 the sink pad of the entity.\n> + */\n> + std::vector, const MediaPad *>> videoDevices;\n\nVideo devices makes me think this is a store of unicams, but really it's\na store of the subdevices and their associated pad/links.\n\nBut ... it feels like that's just a naming conflict, because they are\nthe \"video devices\" - it's just a clash with the V4L2VideoDevice which\nis ... separate/different.\n\nI started thinking maybe sensorDevices would make more sense, but that's\nnot right either - so perhaps bridgeDevices or just subdevices?\n\nAnyway, that's just bikeshedding a name so it shouldn't matter too much.\n\n\n> \n> /* DMAHEAP allocation helper. */\n> RPi::DmaHeap dmaHeap_;\n> @@ -315,6 +321,7 @@ private:\n> }\n> \n> int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);\n> + void enumerateVideoDevices(RPiCameraData *data, const MediaPad *sinkPad);\n> int queueAllBuffers(Camera *camera);\n> int prepareBuffers(Camera *camera);\n> void freeBuffers(Camera *camera);\n> @@ -848,6 +855,24 @@ 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, pad] : data->videoDevices) {\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 to this camera, and setup the pad format. */\n> + data->sensor_->entity()->pads()[0]->links()[0]->setEnabled(true);\n\ndoes this some how need to 'walk' up the graph to the unicam enabling\nall links on the way up? (only because you've called it a cascade?)\n\n\n> + ret = device->setFormat(pad->index(), &sensorFormat);\n> + LOG(RPI, Info) << \"Configured media link on device \" << device->entity()->name()\n> + << \" at pad \" << pad->index();\n> + }\n> +\n> return ret;\n> }\n> \n> @@ -1078,6 +1103,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> + MediaPad *sensorSinkPad = sensorEntity->getPadByIndex(0)->links()[0]->sink();\n> + enumerateVideoDevices(data.get(), sensorSinkPad);\n> +\n> data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> \n> ipa::RPi::SensorConfig sensorConfig;\n> @@ -1204,6 +1236,34 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> return 0;\n> }\n> \n> +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data, const MediaPad *sinkPad)\n> +{\n> + const MediaEntity *entity = sinkPad->entity();\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->videoDevices.emplace_back(std::make_unique(entity), sinkPad);\n> + data->videoDevices.back().first->open();\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 (const MediaLink *l : pad->links())\n> + enumerateVideoDevices(data, l->sink());\n\nNice, so it's turtles all the way down ... ;-)\n\nBikeshedding of a name aside, my only query is with the enabling of the\nlinks on the way back up from the sensorEntity now that it supports\ncascading.\n\nI don't mind if that's added later when hardware is available to test,\nbut if my interpretation is right that it would need to do it there,\nplease add a todo.\n\nReviewed-by: Kieran Bingham \n\n--\nKieran\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 25697BF415\n\tfor ;\n\tWed, 8 Dec 2021 12:27:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67FB06086A;\n\tWed, 8 Dec 2021 13:27:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 15F4B60225\n\tfor ;\n\tWed, 8 Dec 2021 13:27:06 +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 B91208BB;\n\tWed, 8 Dec 2021 13:27:05 +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=\"WDoK5qVE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638966425;\n\tbh=XGkFxjMiKDCFET9UHpfB8bTE5jhFfW2iwrxAANsHxrY=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=WDoK5qVEBoqryaus9d3eLnX3QJMjJq7auAaoDSxS3puMgIbAaNfVX2TfVK7y9cYaZ\n\ttle9PirpCXoq6sqHn4XLIf3tyssrOK+4+xiQWj/5EaEUGSbPNmcDqr8yQBW3zkBQg7\n\t/4nyHEDA4MFdD+hC2ANtPI7Ks4ft0Q6aYcibrYZA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211201115711.1017822-3-naush@raspberrypi.com>","References":"<20211201115711.1017822-1-naush@raspberrypi.com>\n\t<20211201115711.1017822-3-naush@raspberrypi.com>","From":"Kieran Bingham ","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 08 Dec 2021 12:27:03 +0000","Message-ID":"<163896642310.1970692.7932799544696435231@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 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":21676,"web_url":"https://patchwork.libcamera.org/comment/21676/","msgid":"","date":"2021-12-08T12:43:44","subject":"Re: [libcamera-devel] [PATCH v1 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\nThank you for your feedback!\n\nOn Wed, 8 Dec 2021 at 12:27, Kieran Bingham \nwrote:\n\n> Quoting Naushir Patuck (2021-12-01 11:57:11)\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 | 60 +++++++++++++++++++\n> > 1 file changed, 60 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 811160490f5c..2512d0746d4a 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -11,6 +11,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> >\n> > #include \n> > #include \n> > @@ -224,6 +225,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 the sink pad of the entity.\n> > + */\n> > + std::vector, const\n> MediaPad *>> videoDevices;\n>\n> Video devices makes me think this is a store of unicams, but really it's\n> a store of the subdevices and their associated pad/links.\n>\n> But ... it feels like that's just a naming conflict, because they are\n> the \"video devices\" - it's just a clash with the V4L2VideoDevice which\n> is ... separate/different.\n>\n> I started thinking maybe sensorDevices would make more sense, but that's\n> not right either - so perhaps bridgeDevices or just subdevices?\n>\n\nI did have the same thought train when deciding on a name here :-)\nIt was originally called muxDevices, but that would not capture the\nfact that bridge devices are also accounted for. Perhaps bridgeDevices\nis generic enough to encompass both? Anyone else have suggestions?\n\n\n>\n> Anyway, that's just bikeshedding a name so it shouldn't matter too much.\n>\n>\n> >\n> > /* DMAHEAP allocation helper. */\n> > RPi::DmaHeap dmaHeap_;\n> > @@ -315,6 +321,7 @@ private:\n> > }\n> >\n> > int registerCamera(MediaDevice *unicam, MediaDevice *isp,\n> MediaEntity *sensorEntity);\n> > + void enumerateVideoDevices(RPiCameraData *data, const MediaPad\n> *sinkPad);\n> > int queueAllBuffers(Camera *camera);\n> > int prepareBuffers(Camera *camera);\n> > void freeBuffers(Camera *camera);\n> > @@ -848,6 +855,24 @@ 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, pad] : data->videoDevices) {\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 to this camera, and setup the\n> pad format. */\n> > +\n> data->sensor_->entity()->pads()[0]->links()[0]->setEnabled(true);\n>\n> does this some how need to 'walk' up the graph to the unicam enabling\n> all links on the way up? (only because you've called it a cascade?)\n>\n\nYes, you are right. I do need to enable the links on all entity pads up.\nI will rework this loop to do exactly that in the next revision!\n\nRegards,\nNaush\n\n\n\n>\n>\n> > + ret = device->setFormat(pad->index(), &sensorFormat);\n> > + LOG(RPI, Info) << \"Configured media link on device \" <<\n> device->entity()->name()\n> > + << \" at pad \" << pad->index();\n> > + }\n> > +\n> > return ret;\n> > }\n> >\n> > @@ -1078,6 +1103,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> > + MediaPad *sensorSinkPad =\n> sensorEntity->getPadByIndex(0)->links()[0]->sink();\n> > + enumerateVideoDevices(data.get(), sensorSinkPad);\n> > +\n> > data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> >\n> > ipa::RPi::SensorConfig sensorConfig;\n> > @@ -1204,6 +1236,34 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > return 0;\n> > }\n> >\n> > +void PipelineHandlerRPi::enumerateVideoDevices(RPiCameraData *data,\n> const MediaPad *sinkPad)\n> > +{\n> > + const MediaEntity *entity = sinkPad->entity();\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->videoDevices.emplace_back(std::make_unique(entity),\n> sinkPad);\n> > + data->videoDevices.back().first->open();\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 (const MediaLink *l : pad->links())\n> > + enumerateVideoDevices(data, l->sink());\n>\n> Nice, so it's turtles all the way down ... ;-)\n>\n> Bikeshedding of a name aside, my only query is with the enabling of the\n> links on the way back up from the sensorEntity now that it supports\n> cascading.\n>\n> I don't mind if that's added later when hardware is available to test,\n> but if my interpretation is right that it would need to do it there,\n> please add a todo.\n>\n> Reviewed-by: Kieran Bingham \n>\n> --\n> Kieran\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 E75FABF415\n\tfor ;\n\tWed, 8 Dec 2021 12:44:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D8E36087A;\n\tWed, 8 Dec 2021 13:44:02 +0100 (CET)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B35E60225\n\tfor ;\n\tWed, 8 Dec 2021 13:44:01 +0100 (CET)","by mail-lj1-x235.google.com with SMTP id b19so1911125ljr.12\n\tfor ;\n\tWed, 08 Dec 2021 04:44:01 -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=\"eUjknbAq\"; 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=TEP9lZI9L5DgpCZW16MLhc5n/UK1/9j9XYVNCxFQs6I=;\n\tb=eUjknbAqTnrYdvOAJOdBc4mDD3em1bjSm3JxaPlhQcvFezn13A1cytFDdSWysKwYsQ\n\tQIylGZfQ0heeVKICPOTsf3STL7lp5Zuc6dzQEtVMJFy4vy3NF6bOq6YjngpLApLYP6Dj\n\tuHGjzg3Z6BOaZtgtdF45TKjGucrBIxAqpYiW9lY2O+VBvm5u+H6WsYl/39E5tR8a5P8w\n\tP69F6EIodZ3u/x6j+lU4rxZIDibSeiVtkr+B1IM2An8qJasjekwk18NUVWsWJT1qlmKx\n\tJwZaq3S4EaNvMdSCNDue1GJbX28rGRkNmTbp6YmOC2ndKxRV7S31IoNQpQCN0CG0Mz3e\n\tbiwA==","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=TEP9lZI9L5DgpCZW16MLhc5n/UK1/9j9XYVNCxFQs6I=;\n\tb=uCuvme9GIdLc60SS9T2ii9lQTGfsNckDAlLhL6durEwdPzRTVlMUsS0PemEGNHyDSR\n\t8XPYcVusqckCVFrRa1fk9G4SCTcdB7UFPtbHWWsFZSAfyZwjX74m/uZU3wn5bV8F3K+i\n\t/4dJSxxUn+JiIFTOMiw85n3cGT8607V484IfbebtN87FM1YetRs0+X+l762UnvUcU/yn\n\t0k2rhifhuj5sMbXSP+E2nvAOtfH93ppjwgL3JvSKlpLHcv1El9ODyOhSXHewBO7fKIg7\n\tV1IKm2TG3wrzkqC7LZw7TuSv3/5ngBSlbtE+T6oA15dVQTnglwTaKwkOwnvLEPRPnmAl\n\tIpaQ==","X-Gm-Message-State":"AOAM533gnQV52J/kNlrD1Ds/1hXUsRGl+gj/pl2HqVXCpGfoYaNa/UPg\n\t7fWAYi/GIhc45CgLNGJsR7xXQxf78a+SJg3fZT0xyA==","X-Google-Smtp-Source":"ABdhPJzly/ifInemwW2SE9HdK8rF1YbB5li9jKUNTUyLLYVltDaGaw+4PkmZmvoNYoTs9N5mcBLWSwBhOeDSHU8i3io=","X-Received":"by 2002:a2e:908b:: with SMTP id\n\tl11mr49821032ljg.208.1638967440791; \n\tWed, 08 Dec 2021 04:44:00 -0800 (PST)","MIME-Version":"1.0","References":"<20211201115711.1017822-1-naush@raspberrypi.com>\n\t<20211201115711.1017822-3-naush@raspberrypi.com>\n\t<163896642310.1970692.7932799544696435231@Monstersaurus>","In-Reply-To":"<163896642310.1970692.7932799544696435231@Monstersaurus>","From":"Naushir Patuck ","Date":"Wed, 8 Dec 2021 12:43:44 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"0000000000001b6f2105d2a1d88b\"","Subject":"Re: [libcamera-devel] [PATCH v1 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\" "}}]