[{"id":21097,"web_url":"https://patchwork.libcamera.org/comment/21097/","msgid":"","date":"2021-11-22T13:00:40","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\n(CC'ing Sakari)\n\nThank you for the patch.\n\nOn Mon, Nov 22, 2021 at 12:34:27PM +0000, Naushir Patuck wrote:\n> Expand the pipeline handler camera registration to correctly handle multiple\n> cameras attached to the platform. For example, Raspberry Pi Compute Module\n> platforms have two camera connectors, and this change would allow the user to\n> select either of the two cameras to run.\n> \n> There are associated kernel driver changes for both Unicam and the ISP needed\n> to correctly advertise multiple media devices and nodes for multi-camera usage:\n> \n> https://github.com/raspberrypi/linux/pull/4140\n> https://github.com/raspberrypi/linux/pull/4709\n> \n> However, this change is backward compatible with kernel builds that do not have\n> these changes for standard single camera usage.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 113 ++++++++++++------\n> 1 file changed, 74 insertions(+), 39 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 9aa7e9eef5e7..3f9e15514ed9 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -311,14 +311,11 @@ private:\n> \t\treturn static_cast(camera->_d());\n> \t}\n> \n> -\tbool registerCameras();\n> +\tint registerCameras(MediaDevice *unicam, MediaDevice *isp, const std::string &deviceId);\n\nGiven that a pipeline handler instance doesn't register multiple cameras\nanymore in v2, should this be called registerCamera() ?\n\n> \tint queueAllBuffers(Camera *camera);\n> \tint prepareBuffers(Camera *camera);\n> \tvoid freeBuffers(Camera *camera);\n> \tvoid mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);\n> -\n> -\tMediaDevice *unicam_;\n> -\tMediaDevice *isp_;\n> };\n> \n> RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)\n> @@ -509,7 +506,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> }\n> \n> PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n> -\t: PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)\n> +\t: PipelineHandler(manager)\n> {\n> }\n> \n> @@ -993,49 +990,85 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> \n> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> {\n> -\tDeviceMatch unicam(\"unicam\");\n> -\tDeviceMatch isp(\"bcm2835-isp\");\n> +\tMediaDevice *unicamDevice, *ispDevice;\n> +\tstd::string deviceId;\n> \n> -\tunicam.add(\"unicam-image\");\n> +\t/*\n> +\t * String of indexes to append to the entity names when searching for\n> +\t * the Unican media devices. The first string is empty (un-indexed) to\n> +\t * to maintain backward compatability with old versions of the Unicam\n> +\t * kernel driver that did not advertise instance indexes.\n> +\t */\n> +\tfor (const std::string &id : { \"\", \"0\", \"1\" }) {\n> +\t\tDeviceMatch unicam(\"unicam\");\n> +\t\tunicam.add(\"unicam\" + id + \"-image\");\n> +\t\tunicamDevice = acquireMediaDevice(enumerator, unicam);\n> \n> -\tisp.add(\"bcm2835-isp0-output0\"); /* Input */\n> -\tisp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n> -\tisp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n> -\tisp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n> +\t\tif (unicamDevice) {\n> +\t\t\tdeviceId = id == \"1\" ? \"1\" : \"0\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n\nWouldn't it be better to extend DeviceMatch to accept regexps ?\nhttps://en.cppreference.com/w/cpp/header/regex should make that easy.\n\n> \n> -\tunicam_ = acquireMediaDevice(enumerator, unicam);\n> -\tif (!unicam_)\n> +\tif (!unicamDevice) {\n> +\t\tLOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> \t\treturn false;\n> +\t}\n> \n> -\tisp_ = acquireMediaDevice(enumerator, isp);\n> -\tif (!isp_)\n> +\tDeviceMatch isp(\"bcm2835-isp\");\n> +\tisp.add(\"bcm2835-isp\" + deviceId + \"-output0\"); /* Input */\n> +\tisp.add(\"bcm2835-isp\" + deviceId + \"-capture1\"); /* Output 0 */\n> +\tisp.add(\"bcm2835-isp\" + deviceId + \"-capture2\"); /* Output 1 */\n> +\tisp.add(\"bcm2835-isp\" + deviceId + \"-capture3\"); /* Stats */\n\nUnless you need to match the unicam and ISP instances, you could here\ndrop the isp.add() calls as matching on the device name should be\nenough. Unless there are bcm2835-isp instances that don't have the\nneeded nodes ?\n\n> +\tispDevice = acquireMediaDevice(enumerator, isp);\n> +\n> +\tif (!ispDevice) {\n> +\t\tLOG(RPI, Error) << \"Unable to acquire ISP instance \" << deviceId;\n> \t\treturn false;\n> +\t}\n> +\n> +\tint ret = registerCameras(unicamDevice, ispDevice, deviceId);\n> +\tif (ret) {\n> +\t\tLOG(RPI, Error) << \"Failed to register camera: \" << ret;\n> +\t\treturn false;\n> +\t}\n> \n> -\treturn registerCameras();\n> +\treturn true;\n> }\n> \n> -bool PipelineHandlerRPi::registerCameras()\n> +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> +\t\t\t\t\tconst std::string &deviceId)\n> {\n> \tstd::unique_ptr data = std::make_unique(this);\n> +\n> \tif (!data->dmaHeap_.isValid())\n> -\t\treturn false;\n> +\t\treturn -ENOMEM;\n> +\n> +\tMediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" + deviceId + \"-image\");\n> +\tMediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-output0\");\n> +\tMediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture1\");\n> +\tMediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture2\");\n> +\tMediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture3\");\n\nI'm wondering if wildcards would be nice here too, but maybe a simpler\noption would be to not include the device ID in the entity names in the\ndriver ? We have a bus info field that the kernel reports for the media\ndevice, to differentiate between multiple instances of the same device.\nI suppose we are missing guidelines on the kernel side regarding entity\nnaming. Sakari, any opinion on this ?\n\n> +\n> +\tif (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)\n> +\t\treturn -ENOENT;\n> \n> \t/* Locate and open the unicam video streams. */\n> -\tdata->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicam_->getEntityByName(\"unicam-image\"));\n> +\tdata->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicamImage);\n> \n> \t/* An embedded data node will not be present if the sensor does not support it. */\n> -\tMediaEntity *embeddedEntity = unicam_->getEntityByName(\"unicam-embedded\");\n> -\tif (embeddedEntity) {\n> -\t\tdata->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", embeddedEntity);\n> +\tMediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam\" + deviceId + \"-embedded\");\n> +\tif (unicamEmbedded) {\n> +\t\tdata->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> \t\tdata->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> \t\t\t\t\t\t\t\t\t &RPiCameraData::unicamBufferDequeue);\n> \t}\n> \n> \t/* Tag the ISP input stream as an import stream. */\n> -\tdata->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> -\tdata->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> -\tdata->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> -\tdata->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> +\tdata->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, true);\n> +\tdata->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1);\n> +\tdata->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> +\tdata->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> \n> \t/* Wire up all the buffer connections. */\n> \tdata->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n> @@ -1046,7 +1079,7 @@ bool PipelineHandlerRPi::registerCameras()\n> \tdata->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);\n> \n> \t/* Identify the sensor. */\n> -\tfor (MediaEntity *entity : unicam_->entities()) {\n> +\tfor (MediaEntity *entity : unicam->entities()) {\n> \t\tif (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> \t\t\tdata->sensor_ = std::make_unique(entity);\n> \t\t\tbreak;\n> @@ -1054,23 +1087,23 @@ bool PipelineHandlerRPi::registerCameras()\n> \t}\n> \n> \tif (!data->sensor_)\n> -\t\treturn false;\n> +\t\treturn -EINVAL;\n> \n> \tif (data->sensor_->init())\n> -\t\treturn false;\n> +\t\treturn -EINVAL;\n> \n> \tdata->sensorFormats_ = populateSensorFormats(data->sensor_);\n> \n> \tipa::RPi::SensorConfig sensorConfig;\n> \tif (data->loadIPA(&sensorConfig)) {\n> \t\tLOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> -\t\treturn false;\n> +\t\treturn -EINVAL;\n> \t}\n> \n> -\tif (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n> +\tif (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> \t\tLOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> \t\tsensorConfig.sensorMetadata = false;\n> -\t\tif (embeddedEntity)\n> +\t\tif (unicamEmbedded)\n> \t\t\tdata->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> \t}\n> \n> @@ -1091,12 +1124,12 @@ bool PipelineHandlerRPi::registerCameras()\n> \n> \tfor (auto stream : data->streams_) {\n> \t\tif (stream->dev()->open())\n> -\t\t\treturn false;\n> +\t\t\tcontinue;\n> \t}\n> \n> \tif (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> \t\tLOG(RPI, Error) << \"Unicam driver does not use the MediaController, please update your kernel!\";\n> -\t\treturn false;\n> +\t\treturn -EINVAL;\n> \t}\n> \n> \t/*\n> @@ -1158,7 +1191,7 @@ bool PipelineHandlerRPi::registerCameras()\n> \n> \tif (!bayerFormat.isValid()) {\n> \t\tLOG(RPI, Error) << \"No Bayer format found\";\n> -\t\treturn false;\n> +\t\treturn -EINVAL;\n> \t}\n> \tdata->nativeBayerOrder_ = bayerFormat.order;\n> \n> @@ -1173,12 +1206,14 @@ bool PipelineHandlerRPi::registerCameras()\n> \tstreams.insert(&data->isp_[Isp::Output1]);\n> \n> \t/* Create and register the camera. */\n> -\tconst std::string &id = data->sensor_->id();\n> +\tconst std::string &cameraId = data->sensor_->id();\n> \tstd::shared_ptr camera =\n> -\t\tCamera::create(std::move(data), id, streams);\n> +\t\tCamera::create(std::move(data), cameraId, streams);\n> \tregisterCamera(std::move(camera));\n> \n> -\treturn true;\n> +\tLOG(RPI, Info) << \"Registered camera \" << cameraId\n> +\t\t << \" to instance \\\"\" << deviceId << \"\\\"\";\n> +\treturn 0;\n> }\n> \n> int PipelineHandlerRPi::queueAllBuffers(Camera *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 D771BBF415\n\tfor ;\n\tMon, 22 Nov 2021 13:01:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 334276036F;\n\tMon, 22 Nov 2021 14:01:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9739E60230\n\tfor ;\n\tMon, 22 Nov 2021 14:01:03 +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 0AB1914C3;\n\tMon, 22 Nov 2021 14:01:02 +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=\"I8Mlva5L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637586063;\n\tbh=MYIFFBkFNMHMZ0gvtrsN/b/QOI0N6CjbBcjR0Kak4rc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=I8Mlva5LJA524OCe2ojnHqhtUZ/Mg6w+BGLyfeFEOMLQjQojASNIFKMIQxORWeoHa\n\tz7x1CxKVhyK64Ymn0BPWx2tyEHlDGIe64IdSLIp6PsldIIjrHeJivZZ/kcoGODNWf+\n\tVRjnfveBrfy4i0yt56xU/kTlWK8nG5U3+hlrFSwQ=","Date":"Mon, 22 Nov 2021 15:00:40 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211122123427.808484-1-naush@raspberrypi.com>\n\t<20211122123427.808484-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211122123427.808484-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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@lists.libcamera.org, Sakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21099,"web_url":"https://patchwork.libcamera.org/comment/21099/","msgid":"","date":"2021-11-22T13:56:50","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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 the feedback. I apologize for the possibly very obvious\nquestions\nthat follow below - but this enumeration business is a bit unclear to me :-)\n\nOn Mon, 22 Nov 2021 at 13:01, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> (CC'ing Sakari)\n>\n> Thank you for the patch.\n>\n> On Mon, Nov 22, 2021 at 12:34:27PM +0000, Naushir Patuck wrote:\n> > Expand the pipeline handler camera registration to correctly handle\n> multiple\n> > cameras attached to the platform. For example, Raspberry Pi Compute\n> Module\n> > platforms have two camera connectors, and this change would allow the\n> user to\n> > select either of the two cameras to run.\n> >\n> > There are associated kernel driver changes for both Unicam and the ISP\n> needed\n> > to correctly advertise multiple media devices and nodes for multi-camera\n> usage:\n> >\n> > https://github.com/raspberrypi/linux/pull/4140\n> > https://github.com/raspberrypi/linux/pull/4709\n> >\n> > However, this change is backward compatible with kernel builds that do\n> not have\n> > these changes for standard single camera usage.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 113 ++++++++++++------\n> > 1 file changed, 74 insertions(+), 39 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 9aa7e9eef5e7..3f9e15514ed9 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -311,14 +311,11 @@ private:\n> > return static_cast(camera->_d());\n> > }\n> >\n> > - bool registerCameras();\n> > + int registerCameras(MediaDevice *unicam, MediaDevice *isp, const\n> std::string &deviceId);\n>\n> Given that a pipeline handler instance doesn't register multiple cameras\n> anymore in v2, should this be called registerCamera() ?\n>\n\nYes!\n\n\n>\n> > int queueAllBuffers(Camera *camera);\n> > int prepareBuffers(Camera *camera);\n> > void freeBuffers(Camera *camera);\n> > void mapBuffers(Camera *camera, const RPi::BufferMap &buffers,\n> unsigned int mask);\n> > -\n> > - MediaDevice *unicam_;\n> > - MediaDevice *isp_;\n> > };\n> >\n> > RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData\n> *data)\n> > @@ -509,7 +506,7 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validate()\n> > }\n> >\n> > PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n> > - : PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)\n> > + : PipelineHandler(manager)\n> > {\n> > }\n> >\n> > @@ -993,49 +990,85 @@ int PipelineHandlerRPi::queueRequestDevice(Camera\n> *camera, Request *request)\n> >\n> > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > {\n> > - DeviceMatch unicam(\"unicam\");\n> > - DeviceMatch isp(\"bcm2835-isp\");\n> > + MediaDevice *unicamDevice, *ispDevice;\n> > + std::string deviceId;\n> >\n> > - unicam.add(\"unicam-image\");\n> > + /*\n> > + * String of indexes to append to the entity names when searching\n> for\n> > + * the Unican media devices. The first string is empty\n> (un-indexed) to\n> > + * to maintain backward compatability with old versions of the\n> Unicam\n> > + * kernel driver that did not advertise instance indexes.\n> > + */\n> > + for (const std::string &id : { \"\", \"0\", \"1\" }) {\n> > + DeviceMatch unicam(\"unicam\");\n> > + unicam.add(\"unicam\" + id + \"-image\");\n> > + unicamDevice = acquireMediaDevice(enumerator, unicam);\n> >\n> > - isp.add(\"bcm2835-isp0-output0\"); /* Input */\n> > - isp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n> > - isp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n> > - isp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n> > + if (unicamDevice) {\n> > + deviceId = id == \"1\" ? \"1\" : \"0\";\n> > + break;\n> > + }\n> > + }\n>\n> Wouldn't it be better to extend DeviceMatch to accept regexps ?\n> https://en.cppreference.com/w/cpp/header/regex should make that easy.\n>\n\nI'm not sure - see below.\n\n\n>\n> >\n> > - unicam_ = acquireMediaDevice(enumerator, unicam);\n> > - if (!unicam_)\n> > + if (!unicamDevice) {\n> > + LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> > return false;\n> > + }\n> >\n> > - isp_ = acquireMediaDevice(enumerator, isp);\n> > - if (!isp_)\n> > + DeviceMatch isp(\"bcm2835-isp\");\n> > + isp.add(\"bcm2835-isp\" + deviceId + \"-output0\"); /* Input */\n> > + isp.add(\"bcm2835-isp\" + deviceId + \"-capture1\"); /* Output 0 */\n> > + isp.add(\"bcm2835-isp\" + deviceId + \"-capture2\"); /* Output 1 */\n> > + isp.add(\"bcm2835-isp\" + deviceId + \"-capture3\"); /* Stats */\n>\n> Unless you need to match the unicam and ISP instances, you could here\n> drop the isp.add() calls as matching on the device name should be\n> enough. Unless there are bcm2835-isp instances that don't have the\n> needed nodes ?\n>\n\nI've set things up so that the 2 instances of unicam have the same device\nname (\"unicam\"). To distinguish the different instances, the pad entity\nnames\nhave an index in the string. Ditto for the ISP driver.\n\nIf I were to remove the isp.add() calls, then DeviceEnumerator would only\never find\nthe first match to \"bcm2825-isp\" device and I would never be able to access\nthe\nsecond instance, is that correct? This loops back to the previous\nquestion, and\nwhy I am not sure if regexps in DeviceMatch would make any difference.\n\n\n>\n> > + ispDevice = acquireMediaDevice(enumerator, isp);\n> > +\n> > + if (!ispDevice) {\n> > + LOG(RPI, Error) << \"Unable to acquire ISP instance \" <<\n> deviceId;\n> > return false;\n> > + }\n> > +\n> > + int ret = registerCameras(unicamDevice, ispDevice, deviceId);\n> > + if (ret) {\n> > + LOG(RPI, Error) << \"Failed to register camera: \" << ret;\n> > + return false;\n> > + }\n> >\n> > - return registerCameras();\n> > + return true;\n> > }\n> >\n> > -bool PipelineHandlerRPi::registerCameras()\n> > +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam,\n> MediaDevice *isp,\n> > + const std::string &deviceId)\n> > {\n> > std::unique_ptr data =\n> std::make_unique(this);\n> > +\n> > if (!data->dmaHeap_.isValid())\n> > - return false;\n> > + return -ENOMEM;\n> > +\n> > + MediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" +\n> deviceId + \"-image\");\n> > + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" +\n> deviceId + \"-output0\");\n> > + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\" +\n> deviceId + \"-capture1\");\n> > + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\" +\n> deviceId + \"-capture2\");\n> > + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\" +\n> deviceId + \"-capture3\");\n>\n> I'm wondering if wildcards would be nice here too, but maybe a simpler\n> option would be to not include the device ID in the entity names in the\n> driver ? We have a bus info field that the kernel reports for the media\n> device, to differentiate between multiple instances of the same device.\n> I suppose we are missing guidelines on the kernel side regarding entity\n> naming. Sakari, any opinion on this ?\n>\n\nSame problem, if all instances have the same driver name and entity names,\nI can\nnever acquire the second instance, as the first instance will always return\na match.\n\nAgain, I am sure my interpretation of the API is not fully correct, so\nthese issues may\nnot actually exist :-) So please do correct me where I went wrong.\n\nFor clarity, this is how the 2 instances of unicam advertises themselves:\n\npi@cm4:~ $ media-ctl -d /dev/media0 -p\nMedia controller API version 5.10.78\n\nMedia device information\n------------------------\ndriver unicam\nmodel unicam\nserial\nbus info platform:fe800000.csi\nhw revision 0x0\ndriver version 5.10.78\n\nDevice topology\n- entity 1: imx219 0-0010 (2 pads, 2 links)\n type V4L2 subdev subtype Sensor flags 0\n device node name /dev/v4l-subdev0\npad0: Source\n[fmt:SRGGB10_1X10/1640x1232 field:none colorspace:raw xfer:none ycbcr:601\nquantization:full-range\ncrop.bounds:(8,8)/3280x2464\ncrop:(8,8)/3280x2464]\n-> \"unicam1-image\":0 [ENABLED,IMMUTABLE]\npad1: Source\n[fmt:unknown/16384x1 field:none\ncrop.bounds:(8,8)/3280x2464\ncrop:(8,8)/3280x2464]\n-> \"unicam1-embedded\":0 [ENABLED,IMMUTABLE]\n\n- entity 4: unicam1-image (1 pad, 1 link)\n type Node subtype V4L flags 1\n device node name /dev/video0\npad0: Sink\n<- \"imx219 0-0010\":0 [ENABLED,IMMUTABLE]\n\n- entity 10: unicam1-embedded (1 pad, 1 link)\n type Node subtype V4L flags 0\n device node name /dev/video1\npad0: Sink\n<- \"imx219 0-0010\":1 [ENABLED,IMMUTABLE]\n\npi@cm4:~ $ media-ctl -d /dev/media1 -p\nMedia controller API version 5.10.78\n\nMedia device information\n------------------------\ndriver unicam\nmodel unicam\nserial\nbus info platform:fe801000.csi\nhw revision 0x0\ndriver version 5.10.78\n\nDevice topology\n- entity 1: imx219 10-0010 (2 pads, 2 links)\n type V4L2 subdev subtype Sensor flags 0\n device node name /dev/v4l-subdev1\npad0: Source\n[fmt:SRGGB10_1X10/1640x1232 field:none colorspace:raw xfer:none ycbcr:601\nquantization:full-range\ncrop.bounds:(8,8)/3280x2464\ncrop:(8,8)/3280x2464]\n-> \"unicam0-image\":0 [ENABLED,IMMUTABLE]\npad1: Source\n[fmt:unknown/16384x1 field:none\ncrop.bounds:(8,8)/3280x2464\ncrop:(8,8)/3280x2464]\n-> \"unicam0-embedded\":0 [ENABLED,IMMUTABLE]\n\n- entity 4: unicam0-image (1 pad, 1 link)\n type Node subtype V4L flags 1\n device node name /dev/video2\npad0: Sink\n<- \"imx219 10-0010\":0 [ENABLED,IMMUTABLE]\n\n- entity 10: unicam0-embedded (1 pad, 1 link)\n type Node subtype V4L flags 0\n device node name /dev/video3\npad0: Sink\n<- \"imx219 10-0010\":1 [ENABLED,IMMUTABLE]","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 56D93BF415\n\tfor ;\n\tMon, 22 Nov 2021 13:57:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C39A6033C;\n\tMon, 22 Nov 2021 14:57:08 +0100 (CET)","from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0673060230\n\tfor ;\n\tMon, 22 Nov 2021 14:57:07 +0100 (CET)","by mail-lf1-x12d.google.com with SMTP id bi37so81145888lfb.5\n\tfor ;\n\tMon, 22 Nov 2021 05:57:06 -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=\"KnjiJIC9\"; 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=JGQnWiTRuwFqIpQ2lhR5ZVBLQVh46+wMYBDYWGj42kA=;\n\tb=KnjiJIC9BnCmlbWE4vlPnm4TdMqHfWnoZAgX/fhSiUGdt5EdEBPxLQ1E+hAYMHjPsp\n\txGsgkdSK7VqMGvXlwk+phYwy+Yc0R62R5SgL2aWP4f9FdaP0XitZgG1JwcyvJ1zm8Nb5\n\t3Q4xczM7Ot8HHB6ikE1W2KVkffQ2TaMvASOgQM8qbfcZoY/EqWwIcghIh23GPGtJHZMo\n\t5WUXPXdjS56Vy67xoiStZys0E7wQMgAUmLODHykZsf4kJmyY+n95awpAZlcgwghJgbC7\n\td0E+UMlgWn6HUSjnTu1tS+z85O1dptpAXTYNZ6uJYx8ISCEGgPA6FeEdlr5WHm05FxxL\n\tb20Q==","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=JGQnWiTRuwFqIpQ2lhR5ZVBLQVh46+wMYBDYWGj42kA=;\n\tb=05T/S/j4h2hPh7PHM7OwWNJ+K4qc8SxRTtyGkEdU0F3+dulNi/V1Yx/x70Pk+dwyxF\n\tsuQtXEjN37AWk0JjOrbjAFOPTNixKoZ9nP2PqLtlzwxqMLDBoIO6n+qm+TqW292qpsTh\n\tiUHYVUdz9Ue9swn/MC5detcW0X0zQiMptkvW5SSg0wOLXhMP6Ri2vX+92lqvonyiEUPb\n\tHmVqy75swnGcbqplmF1j60hzBgy4/eTxukmsZKwP1Xsj/fzVAkunQpsAjcNCGmtQoFwG\n\tYPHbfKgo7uSEMJ4TPhyPLdADFOlxBV5/NkOlsI2DLrFphyj7/lMwhoopgv6UIdzwxO12\n\tY2uA==","X-Gm-Message-State":"AOAM532IQvRpiuHd22pfL0o8FBznEk4pdEShdwbJBQM/RjvuK3gGoiiC\n\tdaMc8hPadwiXdKLNB5ncZrKu0YgVfsHpMAGUisl19Q==","X-Google-Smtp-Source":"ABdhPJyFQbAQLmcouKrjpWv6Yznp0FuphIntWFyiQF1t5t+TYiHtzNgVezU53zQNKaGaTTBHqR8js9VwxKE3YkoH8fE=","X-Received":"by 2002:a19:6754:: with SMTP id\n\te20mr54562201lfj.122.1637589426068; \n\tMon, 22 Nov 2021 05:57:06 -0800 (PST)","MIME-Version":"1.0","References":"<20211122123427.808484-1-naush@raspberrypi.com>\n\t<20211122123427.808484-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Mon, 22 Nov 2021 13:56:50 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000077a2205d16100d3\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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 ,\n\tSakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21106,"web_url":"https://patchwork.libcamera.org/comment/21106/","msgid":"","date":"2021-11-23T00:09:29","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Mon, Nov 22, 2021 at 01:56:50PM +0000, Naushir Patuck wrote:\n> Hi Laurent,\n> \n> Thank you for the feedback. I apologize for the possibly very obvious questions\n> that follow below - but this enumeration business is a bit unclear to me :-)\n\nI never accept apologies for such questions, because there's nothing to\napologize for :-)\n\n> On Mon, 22 Nov 2021 at 13:01, Laurent Pinchart wrote:\n> > On Mon, Nov 22, 2021 at 12:34:27PM +0000, Naushir Patuck wrote:\n> > > Expand the pipeline handler camera registration to correctly handle multiple\n> > > cameras attached to the platform. For example, Raspberry Pi Compute Module\n> > > platforms have two camera connectors, and this change would allow the user to\n> > > select either of the two cameras to run.\n> > >\n> > > There are associated kernel driver changes for both Unicam and the ISP needed\n> > > to correctly advertise multiple media devices and nodes for multi-camera usage:\n> > >\n> > > https://github.com/raspberrypi/linux/pull/4140\n> > > https://github.com/raspberrypi/linux/pull/4709\n> > >\n> > > However, this change is backward compatible with kernel builds that do not have\n> > > these changes for standard single camera usage.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 113 ++++++++++++------\n> > > 1 file changed, 74 insertions(+), 39 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 9aa7e9eef5e7..3f9e15514ed9 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -311,14 +311,11 @@ private:\n> > > return static_cast(camera->_d());\n> > > }\n> > >\n> > > - bool registerCameras();\n> > > + int registerCameras(MediaDevice *unicam, MediaDevice *isp, const std::string &deviceId);\n> >\n> > Given that a pipeline handler instance doesn't register multiple cameras\n> > anymore in v2, should this be called registerCamera() ?\n> \n> Yes!\n> \n> > > int queueAllBuffers(Camera *camera);\n> > > int prepareBuffers(Camera *camera);\n> > > void freeBuffers(Camera *camera);\n> > > void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);\n> > > -\n> > > - MediaDevice *unicam_;\n> > > - MediaDevice *isp_;\n> > > };\n> > >\n> > > RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData *data)\n> > > @@ -509,7 +506,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > > }\n> > >\n> > > PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n> > > - : PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)\n> > > + : PipelineHandler(manager)\n> > > {\n> > > }\n> > >\n> > > @@ -993,49 +990,85 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > >\n> > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > {\n> > > - DeviceMatch unicam(\"unicam\");\n> > > - DeviceMatch isp(\"bcm2835-isp\");\n> > > + MediaDevice *unicamDevice, *ispDevice;\n> > > + std::string deviceId;\n> > >\n> > > - unicam.add(\"unicam-image\");\n> > > + /*\n> > > + * String of indexes to append to the entity names when searching for\n> > > + * the Unican media devices. The first string is empty (un-indexed) to\n> > > + * to maintain backward compatability with old versions of the Unicam\n> > > + * kernel driver that did not advertise instance indexes.\n> > > + */\n> > > + for (const std::string &id : { \"\", \"0\", \"1\" }) {\n> > > + DeviceMatch unicam(\"unicam\");\n> > > + unicam.add(\"unicam\" + id + \"-image\");\n> > > + unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > >\n> > > - isp.add(\"bcm2835-isp0-output0\"); /* Input */\n> > > - isp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n> > > - isp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n> > > - isp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n> > > + if (unicamDevice) {\n> > > + deviceId = id == \"1\" ? \"1\" : \"0\";\n> > > + break;\n> > > + }\n> > > + }\n> >\n> > Wouldn't it be better to extend DeviceMatch to accept regexps ?\n> > https://en.cppreference.com/w/cpp/header/regex should make that easy.\n> \n> I'm not sure - see below.\n> \n> > > - unicam_ = acquireMediaDevice(enumerator, unicam);\n> > > - if (!unicam_)\n> > > + if (!unicamDevice) {\n> > > + LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> > > return false;\n> > > + }\n> > >\n> > > - isp_ = acquireMediaDevice(enumerator, isp);\n> > > - if (!isp_)\n> > > + DeviceMatch isp(\"bcm2835-isp\");\n> > > + isp.add(\"bcm2835-isp\" + deviceId + \"-output0\"); /* Input */\n> > > + isp.add(\"bcm2835-isp\" + deviceId + \"-capture1\"); /* Output 0 */\n> > > + isp.add(\"bcm2835-isp\" + deviceId + \"-capture2\"); /* Output 1 */\n> > > + isp.add(\"bcm2835-isp\" + deviceId + \"-capture3\"); /* Stats */\n> >\n> > Unless you need to match the unicam and ISP instances, you could here\n> > drop the isp.add() calls as matching on the device name should be\n> > enough. Unless there are bcm2835-isp instances that don't have the\n> > needed nodes ?\n> \n> I've set things up so that the 2 instances of unicam have the same device\n> name (\"unicam\"). To distinguish the different instances, the pad entity names\n> have an index in the string. Ditto for the ISP driver.\n\nThe media controller API doesn't report a device name for a\nmedia_device, but a driver name, a model name, and bus information. The\ndriver name should be identical for all instances. The model name can\ndifferentiate different models (for instance if the Unicam IP core\nexisted in different versions in different SoCs), but should be\nidentical for different instances of the same device. The bus\ninformation is the field that tells instances apart.\n\n> If I were to remove the isp.add() calls, then DeviceEnumerator would only ever find\n> the first match to \"bcm2825-isp\" device and I would never be able to access the\n> second instance, is that correct? This loops back to the previous question, and\n> why I am not sure if regexps in DeviceMatch would make any difference.\n\nOnce a pipeline handler acquires a media device, it is removed from the\npool of available devices. The next call to match() will call\nacquireMediaDevice() with the same driver name (\"bcm2825-isp\"), which\nwill then return the second ISP instance as the first one will have been\nacquired already.\n\nThis mechanism allows pipeline handlers to not have to differentiate\nbetween identical instances of the same device, when there's no hardware\nconstraint that requires otherwise (for instance if the Unicam and ISP\ninstances had to be paired exactly, you would be able to use the media\ndevice acquisition mechanism without caring about the instance ID for\nUnicam, but you would then have to acquire a specific ISP instance).\n\n> > > + ispDevice = acquireMediaDevice(enumerator, isp);\n> > > +\n> > > + if (!ispDevice) {\n> > > + LOG(RPI, Error) << \"Unable to acquire ISP instance \" << deviceId;\n> > > return false;\n> > > + }\n> > > +\n> > > + int ret = registerCameras(unicamDevice, ispDevice, deviceId);\n> > > + if (ret) {\n> > > + LOG(RPI, Error) << \"Failed to register camera: \" << ret;\n> > > + return false;\n> > > + }\n> > >\n> > > - return registerCameras();\n> > > + return true;\n> > > }\n> > >\n> > > -bool PipelineHandlerRPi::registerCameras()\n> > > +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> > > + const std::string &deviceId)\n> > > {\n> > > std::unique_ptr data = std::make_unique(this);\n> > > +\n> > > if (!data->dmaHeap_.isValid())\n> > > - return false;\n> > > + return -ENOMEM;\n> > > +\n> > > + MediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" + deviceId + \"-image\");\n> > > + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-output0\");\n> > > + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture1\");\n> > > + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture2\");\n> > > + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture3\");\n> >\n> > I'm wondering if wildcards would be nice here too, but maybe a simpler\n> > option would be to not include the device ID in the entity names in the\n> > driver ? We have a bus info field that the kernel reports for the media\n> > device, to differentiate between multiple instances of the same device.\n> > I suppose we are missing guidelines on the kernel side regarding entity\n> > naming. Sakari, any opinion on this ?\n> \n> Same problem, if all instances have the same driver name and entity names, I can\n> never acquire the second instance, as the first instance will always return a match.\n> \n> Again, I am sure my interpretation of the API is not fully correct, so these issues may\n> not actually exist :-) So please do correct me where I went wrong.\n> \n> For clarity, this is how the 2 instances of unicam advertises themselves:\n> \n> pi@cm4:~ $ media-ctl -d /dev/media0 -p\n> Media controller API version 5.10.78\n> \n> Media device information\n> ------------------------\n> driver unicam\n> model unicam\n> serial\n> bus info platform:fe800000.csi\n> hw revision 0x0\n> driver version 5.10.78\n> \n> Device topology\n> - entity 1: imx219 0-0010 (2 pads, 2 links)\n> type V4L2 subdev subtype Sensor flags 0\n> device node name /dev/v4l-subdev0\n> pad0: Source\n> [fmt:SRGGB10_1X10/1640x1232 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464]\n> -> \"unicam1-image\":0 [ENABLED,IMMUTABLE]\n> pad1: Source\n> [fmt:unknown/16384x1 field:none crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464]\n> -> \"unicam1-embedded\":0 [ENABLED,IMMUTABLE]\n> \n> - entity 4: unicam1-image (1 pad, 1 link)\n> type Node subtype V4L flags 1\n> device node name /dev/video0\n> pad0: Sink\n> <- \"imx219 0-0010\":0 [ENABLED,IMMUTABLE]\n> \n> - entity 10: unicam1-embedded (1 pad, 1 link)\n> type Node subtype V4L flags 0\n> device node name /dev/video1\n> pad0: Sink\n> <- \"imx219 0-0010\":1 [ENABLED,IMMUTABLE]\n> \n> pi@cm4:~ $ media-ctl -d /dev/media1 -p\n> Media controller API version 5.10.78\n> \n> Media device information\n> ------------------------\n> driver unicam\n> model unicam\n> serial\n> bus info platform:fe801000.csi\n> hw revision 0x0\n> driver version 5.10.78\n> \n> Device topology\n> - entity 1: imx219 10-0010 (2 pads, 2 links)\n> type V4L2 subdev subtype Sensor flags 0\n> device node name /dev/v4l-subdev1\n> pad0: Source\n> [fmt:SRGGB10_1X10/1640x1232 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464]\n> -> \"unicam0-image\":0 [ENABLED,IMMUTABLE]\n> pad1: Source\n> [fmt:unknown/16384x1 field:none crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464]\n> -> \"unicam0-embedded\":0 [ENABLED,IMMUTABLE]\n> \n> - entity 4: unicam0-image (1 pad, 1 link)\n> type Node subtype V4L flags 1\n> device node name /dev/video2\n> pad0: Sink\n> <- \"imx219 10-0010\":0 [ENABLED,IMMUTABLE]\n> \n> - entity 10: unicam0-embedded (1 pad, 1 link)\n> type Node subtype V4L flags 0\n> device node name /dev/video3\n> pad0: Sink\n> <- \"imx219 10-0010\":1 [ENABLED,IMMUTABLE]","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 92FD9BDB13\n\tfor ;\n\tTue, 23 Nov 2021 00:09:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D67056038A;\n\tTue, 23 Nov 2021 01:09:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D4DC60233\n\tfor ;\n\tTue, 23 Nov 2021 01:09:53 +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 AD6F3A1B;\n\tTue, 23 Nov 2021 01:09:52 +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=\"hgHFqXTN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637626192;\n\tbh=wlJJ3Qb+ux0ipHNt9NvQ3tk2dNhWjIJ5e28yEgsQTBg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hgHFqXTNGM6yDCu8Tw4riSPMhTAmJAieeZRrTN0r4Lofz0yYJHBk5BZq2fuZwO5mv\n\t1Ff/pz3df0CyFgXnKM06yfZ3wJ7uglLZmmnz1EcwwFL0rmHY7yH/XpvzDXZGvBdWWm\n\tgitKF9U5L7d4uNZhq03f+HQkkYywTVHIjlL3iScg=","Date":"Tue, 23 Nov 2021 02:09:29 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211122123427.808484-1-naush@raspberrypi.com>\n\t<20211122123427.808484-3-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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 ,\n\tSakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21113,"web_url":"https://patchwork.libcamera.org/comment/21113/","msgid":"","date":"2021-11-23T09:43:01","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 23 Nov 2021 at 00:09, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Nov 22, 2021 at 01:56:50PM +0000, Naushir Patuck wrote:\n> > Hi Laurent,\n> >\n> > Thank you for the feedback. I apologize for the possibly very obvious\n> questions\n> > that follow below - but this enumeration business is a bit unclear to me\n> :-)\n>\n> I never accept apologies for such questions, because there's nothing to\n> apologize for :-)\n>\n> > On Mon, 22 Nov 2021 at 13:01, Laurent Pinchart wrote:\n> > > On Mon, Nov 22, 2021 at 12:34:27PM +0000, Naushir Patuck wrote:\n> > > > Expand the pipeline handler camera registration to correctly handle\n> multiple\n> > > > cameras attached to the platform. For example, Raspberry Pi Compute\n> Module\n> > > > platforms have two camera connectors, and this change would allow\n> the user to\n> > > > select either of the two cameras to run.\n> > > >\n> > > > There are associated kernel driver changes for both Unicam and the\n> ISP needed\n> > > > to correctly advertise multiple media devices and nodes for\n> multi-camera usage:\n> > > >\n> > > > https://github.com/raspberrypi/linux/pull/4140\n> > > > https://github.com/raspberrypi/linux/pull/4709\n> > > >\n> > > > However, this change is backward compatible with kernel builds that\n> do not have\n> > > > these changes for standard single camera usage.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 113\n> ++++++++++++------\n> > > > 1 file changed, 74 insertions(+), 39 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 9aa7e9eef5e7..3f9e15514ed9 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -311,14 +311,11 @@ private:\n> > > > return static_cast(camera->_d());\n> > > > }\n> > > >\n> > > > - bool registerCameras();\n> > > > + int registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> const std::string &deviceId);\n> > >\n> > > Given that a pipeline handler instance doesn't register multiple\n> cameras\n> > > anymore in v2, should this be called registerCamera() ?\n> >\n> > Yes!\n> >\n> > > > int queueAllBuffers(Camera *camera);\n> > > > int prepareBuffers(Camera *camera);\n> > > > void freeBuffers(Camera *camera);\n> > > > void mapBuffers(Camera *camera, const RPi::BufferMap &buffers,\n> unsigned int mask);\n> > > > -\n> > > > - MediaDevice *unicam_;\n> > > > - MediaDevice *isp_;\n> > > > };\n> > > >\n> > > > RPiCameraConfiguration::RPiCameraConfiguration(const RPiCameraData\n> *data)\n> > > > @@ -509,7 +506,7 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validate()\n> > > > }\n> > > >\n> > > > PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)\n> > > > - : PipelineHandler(manager), unicam_(nullptr), isp_(nullptr)\n> > > > + : PipelineHandler(manager)\n> > > > {\n> > > > }\n> > > >\n> > > > @@ -993,49 +990,85 @@ int\n> PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > >\n> > > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > {\n> > > > - DeviceMatch unicam(\"unicam\");\n> > > > - DeviceMatch isp(\"bcm2835-isp\");\n> > > > + MediaDevice *unicamDevice, *ispDevice;\n> > > > + std::string deviceId;\n> > > >\n> > > > - unicam.add(\"unicam-image\");\n> > > > + /*\n> > > > + * String of indexes to append to the entity names when\n> searching for\n> > > > + * the Unican media devices. The first string is empty\n> (un-indexed) to\n> > > > + * to maintain backward compatability with old versions of the\n> Unicam\n> > > > + * kernel driver that did not advertise instance indexes.\n> > > > + */\n> > > > + for (const std::string &id : { \"\", \"0\", \"1\" }) {\n> > > > + DeviceMatch unicam(\"unicam\");\n> > > > + unicam.add(\"unicam\" + id + \"-image\");\n> > > > + unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > > >\n> > > > - isp.add(\"bcm2835-isp0-output0\"); /* Input */\n> > > > - isp.add(\"bcm2835-isp0-capture1\"); /* Output 0 */\n> > > > - isp.add(\"bcm2835-isp0-capture2\"); /* Output 1 */\n> > > > - isp.add(\"bcm2835-isp0-capture3\"); /* Stats */\n> > > > + if (unicamDevice) {\n> > > > + deviceId = id == \"1\" ? \"1\" : \"0\";\n> > > > + break;\n> > > > + }\n> > > > + }\n> > >\n> > > Wouldn't it be better to extend DeviceMatch to accept regexps ?\n> > > https://en.cppreference.com/w/cpp/header/regex should make that easy.\n> >\n> > I'm not sure - see below.\n> >\n> > > > - unicam_ = acquireMediaDevice(enumerator, unicam);\n> > > > - if (!unicam_)\n> > > > + if (!unicamDevice) {\n> > > > + LOG(RPI, Debug) << \"Unable to acquire a Unicam\n> instance\";\n> > > > return false;\n> > > > + }\n> > > >\n> > > > - isp_ = acquireMediaDevice(enumerator, isp);\n> > > > - if (!isp_)\n> > > > + DeviceMatch isp(\"bcm2835-isp\");\n> > > > + isp.add(\"bcm2835-isp\" + deviceId + \"-output0\"); /* Input */\n> > > > + isp.add(\"bcm2835-isp\" + deviceId + \"-capture1\"); /* Output 0 */\n> > > > + isp.add(\"bcm2835-isp\" + deviceId + \"-capture2\"); /* Output 1 */\n> > > > + isp.add(\"bcm2835-isp\" + deviceId + \"-capture3\"); /* Stats */\n> > >\n> > > Unless you need to match the unicam and ISP instances, you could here\n> > > drop the isp.add() calls as matching on the device name should be\n> > > enough. Unless there are bcm2835-isp instances that don't have the\n> > > needed nodes ?\n> >\n> > I've set things up so that the 2 instances of unicam have the same device\n> > name (\"unicam\"). To distinguish the different instances, the pad entity\n> names\n> > have an index in the string. Ditto for the ISP driver.\n>\n> The media controller API doesn't report a device name for a\n> media_device, but a driver name, a model name, and bus information. The\n> driver name should be identical for all instances. The model name can\n> differentiate different models (for instance if the Unicam IP core\n> existed in different versions in different SoCs), but should be\n> identical for different instances of the same device. The bus\n> information is the field that tells instances apart.\n>\n> > If I were to remove the isp.add() calls, then DeviceEnumerator would\n> only ever find\n> > the first match to \"bcm2825-isp\" device and I would never be able to\n> access the\n> > second instance, is that correct? This loops back to the previous\n> question, and\n> > why I am not sure if regexps in DeviceMatch would make any difference.\n>\n> Once a pipeline handler acquires a media device, it is removed from the\n> pool of available devices. The next call to match() will call\n> acquireMediaDevice() with the same driver name (\"bcm2825-isp\"), which\n> will then return the second ISP instance as the first one will have been\n> acquired already.\n>\n> This mechanism allows pipeline handlers to not have to differentiate\n> between identical instances of the same device, when there's no hardware\n> constraint that requires otherwise (for instance if the Unicam and ISP\n> instances had to be paired exactly, you would be able to use the media\n> device acquisition mechanism without caring about the instance ID for\n> Unicam, but you would then have to acquire a specific ISP instance).\n>\n\nThank you! That certainly clarifies things for me. It also makes it\npossible\nto simplify my enumeration code significantly if I keep the device driver\nname\nand entity names the same for all instances. Will post an update with those\nchanges soon.\n\nNaush\n\n\n>\n> > > > + ispDevice = acquireMediaDevice(enumerator, isp);\n> > > > +\n> > > > + if (!ispDevice) {\n> > > > + LOG(RPI, Error) << \"Unable to acquire ISP instance \"\n> << deviceId;\n> > > > return false;\n> > > > + }\n> > > > +\n> > > > + int ret = registerCameras(unicamDevice, ispDevice, deviceId);\n> > > > + if (ret) {\n> > > > + LOG(RPI, Error) << \"Failed to register camera: \" <<\n> ret;\n> > > > + return false;\n> > > > + }\n> > > >\n> > > > - return registerCameras();\n> > > > + return true;\n> > > > }\n> > > >\n> > > > -bool PipelineHandlerRPi::registerCameras()\n> > > > +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam,\n> MediaDevice *isp,\n> > > > + const std::string &deviceId)\n> > > > {\n> > > > std::unique_ptr data =\n> std::make_unique(this);\n> > > > +\n> > > > if (!data->dmaHeap_.isValid())\n> > > > - return false;\n> > > > + return -ENOMEM;\n> > > > +\n> > > > + MediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" +\n> deviceId + \"-image\");\n> > > > + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" +\n> deviceId + \"-output0\");\n> > > > + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\"\n> + deviceId + \"-capture1\");\n> > > > + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\"\n> + deviceId + \"-capture2\");\n> > > > + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\"\n> + deviceId + \"-capture3\");\n> > >\n> > > I'm wondering if wildcards would be nice here too, but maybe a simpler\n> > > option would be to not include the device ID in the entity names in the\n> > > driver ? We have a bus info field that the kernel reports for the media\n> > > device, to differentiate between multiple instances of the same device.\n> > > I suppose we are missing guidelines on the kernel side regarding entity\n> > > naming. Sakari, any opinion on this ?\n> >\n> > Same problem, if all instances have the same driver name and entity\n> names, I can\n> > never acquire the second instance, as the first instance will always\n> return a match.\n> >\n> > Again, I am sure my interpretation of the API is not fully correct, so\n> these issues may\n> > not actually exist :-) So please do correct me where I went wrong.\n> >\n> > For clarity, this is how the 2 instances of unicam advertises themselves:\n> >\n> > pi@cm4:~ $ media-ctl -d /dev/media0 -p\n> > Media controller API version 5.10.78\n> >\n> > Media device information\n> > ------------------------\n> > driver unicam\n> > model unicam\n> > serial\n> > bus info platform:fe800000.csi\n> > hw revision 0x0\n> > driver version 5.10.78\n> >\n> > Device topology\n> > - entity 1: imx219 0-0010 (2 pads, 2 links)\n> > type V4L2 subdev subtype Sensor flags 0\n> > device node name /dev/v4l-subdev0\n> > pad0: Source\n> > [fmt:SRGGB10_1X10/1640x1232 field:none colorspace:raw\n> xfer:none ycbcr:601 quantization:full-range crop.bounds:(8,8)/3280x2464\n> crop:(8,8)/3280x2464]\n> > -> \"unicam1-image\":0 [ENABLED,IMMUTABLE]\n> > pad1: Source\n> > [fmt:unknown/16384x1 field:none\n> crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464]\n> > -> \"unicam1-embedded\":0 [ENABLED,IMMUTABLE]\n> >\n> > - entity 4: unicam1-image (1 pad, 1 link)\n> > type Node subtype V4L flags 1\n> > device node name /dev/video0\n> > pad0: Sink\n> > <- \"imx219 0-0010\":0 [ENABLED,IMMUTABLE]\n> >\n> > - entity 10: unicam1-embedded (1 pad, 1 link)\n> > type Node subtype V4L flags 0\n> > device node name /dev/video1\n> > pad0: Sink\n> > <- \"imx219 0-0010\":1 [ENABLED,IMMUTABLE]\n> >\n> > pi@cm4:~ $ media-ctl -d /dev/media1 -p\n> > Media controller API version 5.10.78\n> >\n> > Media device information\n> > ------------------------\n> > driver unicam\n> > model unicam\n> > serial\n> > bus info platform:fe801000.csi\n> > hw revision 0x0\n> > driver version 5.10.78\n> >\n> > Device topology\n> > - entity 1: imx219 10-0010 (2 pads, 2 links)\n> > type V4L2 subdev subtype Sensor flags 0\n> > device node name /dev/v4l-subdev1\n> > pad0: Source\n> > [fmt:SRGGB10_1X10/1640x1232 field:none colorspace:raw\n> xfer:none ycbcr:601 quantization:full-range crop.bounds:(8,8)/3280x2464\n> crop:(8,8)/3280x2464]\n> > -> \"unicam0-image\":0 [ENABLED,IMMUTABLE]\n> > pad1: Source\n> > [fmt:unknown/16384x1 field:none\n> crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464]\n> > -> \"unicam0-embedded\":0 [ENABLED,IMMUTABLE]\n> >\n> > - entity 4: unicam0-image (1 pad, 1 link)\n> > type Node subtype V4L flags 1\n> > device node name /dev/video2\n> > pad0: Sink\n> > <- \"imx219 10-0010\":0 [ENABLED,IMMUTABLE]\n> >\n> > - entity 10: unicam0-embedded (1 pad, 1 link)\n> > type Node subtype V4L flags 0\n> > device node name /dev/video3\n> > pad0: Sink\n> > <- \"imx219 10-0010\":1 [ENABLED,IMMUTABLE]\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 1E895BF415\n\tfor ;\n\tTue, 23 Nov 2021 09:43:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 386E26039C;\n\tTue, 23 Nov 2021 10:43:20 +0100 (CET)","from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com\n\t[IPv6:2a00:1450:4864:20::12f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 139CE6036F\n\tfor ;\n\tTue, 23 Nov 2021 10:43:19 +0100 (CET)","by mail-lf1-x12f.google.com with SMTP id bi37so89569916lfb.5\n\tfor ;\n\tTue, 23 Nov 2021 01:43:19 -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=\"pJi8HBMz\"; 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=p/B2ue/HjOTtkoR6Os4w/U0sCaHvfa0s7q71kcTRCTE=;\n\tb=pJi8HBMzvyg3ZvVuIuR6hWzMKN6ez2epD5q1U9P64XlyjR4wKty33xSBKtV/1pe5YO\n\t9/vk4CIHo1M4Q/Hx9OYgX1YMED1mdOeYGHbcbIZOrxAd4eFtEKLRwLi4NSg6v6OYqYCz\n\tU1GIPI5hEMBoG3Epe4lY6xEtNhA9Bh1/TLl+aW+j1ep9QnPI3q8f+r52lbBQTCh6bKCo\n\t3oNS1Webj7WPfiDd0DteoSwLJgetU3YcBPgxyJbPdmYg28LmQpp8+etY982728rkrnu2\n\tCn7NPFb3MQE0Xoty7UIksr12eFZhftpE/U2raCoRT2NR2vRQUshoJRiA/t235QVBh8fv\n\tHUiA==","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=p/B2ue/HjOTtkoR6Os4w/U0sCaHvfa0s7q71kcTRCTE=;\n\tb=SeWfgdxPjV4G7RlLMAvSCVE+W4qK6gKUv4JNJAOzrXoIRsuvNA564OJCk/Z3CGdlxI\n\tcYAyEDikzB9wNdDJCC60H3iP9geIAgUytJGr+RnNnNwvvsNPr7klpmNNUkkth4pgiKwz\n\tsvIEGwv7L/fH2T9/ZjHMKGRqRReNezFwiMbM/IDC98D2Au4+B1IOTwtDcN2+9I68S7lx\n\tb8nKVCUtARxlA99QobsjU27Z4OSjLl/5axbL3BlZoKgNpq1ZvTrDGyfSfB1lCJ+fL88V\n\tsX3Aa+Oz6VcQp1KPFWZS1dtd1ZnMEg0RlaDNUGQiaqRgwORindsUYEW5pwHDZ9c8yDve\n\tlzsw==","X-Gm-Message-State":"AOAM532r6QquKkRSmqHYQjfHBlmzXteig/GXZ2fO8sqXkM1LSmphajtJ\n\te/bbAANtdJLduwb3/IlTDlgCsFUD3peEwCunjefQZA==","X-Google-Smtp-Source":"ABdhPJxC1oE/nILwNulEr2zxPx/q8/RoRd7T7rR3n5QMGzkZu7LXVu9tY0QrYPLkh4c18a4qciwHxNuZlKYz2+0Cx6s=","X-Received":"by 2002:a05:6512:39c4:: with SMTP id\n\tk4mr3381138lfu.79.1637660598261; \n\tTue, 23 Nov 2021 01:43:18 -0800 (PST)","MIME-Version":"1.0","References":"<20211122123427.808484-1-naush@raspberrypi.com>\n\t<20211122123427.808484-3-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 23 Nov 2021 09:43:01 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000038ee2705d1719257\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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 ,\n\tSakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21157,"web_url":"https://patchwork.libcamera.org/comment/21157/","msgid":"","date":"2021-11-23T16:28:56","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":32,"url":"https://patchwork.libcamera.org/api/people/32/","name":"Sakari Ailus","email":"sakari.ailus@iki.fi"},"content":"Hi Laurent, Naushir,\n\nOn Mon, Nov 22, 2021 at 03:00:40PM +0200, Laurent Pinchart wrote:\n> > -bool PipelineHandlerRPi::registerCameras()\n> > +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> > +\t\t\t\t\tconst std::string &deviceId)\n> > {\n> > \tstd::unique_ptr data = std::make_unique(this);\n> > +\n> > \tif (!data->dmaHeap_.isValid())\n> > -\t\treturn false;\n> > +\t\treturn -ENOMEM;\n> > +\n> > +\tMediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" + deviceId + \"-image\");\n> > +\tMediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-output0\");\n> > +\tMediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture1\");\n> > +\tMediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture2\");\n> > +\tMediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture3\");\n> \n> I'm wondering if wildcards would be nice here too, but maybe a simpler\n> option would be to not include the device ID in the entity names in the\n> driver ? We have a bus info field that the kernel reports for the media\n> device, to differentiate between multiple instances of the same device.\n> I suppose we are missing guidelines on the kernel side regarding entity\n> naming. Sakari, any opinion on this ?\n\nI think it'd be best to be able to match multiple fields, not just entity\nname. But I guess a regex goes a long way already.\n\nThere are some (unwritten?) rules for naming devices and they mostly work.\n\nWhat's deviceId here? Would you need that as these are on separate MC\ndevices? (I suppose it's up to the driver thought.)","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 ED71FBF415\n\tfor ;\n\tTue, 23 Nov 2021 16:29:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DFE966036F;\n\tTue, 23 Nov 2021 17:28:59 +0100 (CET)","from lahtoruutu.iki.fi (lahtoruutu.iki.fi [IPv6:2a0b:5c81:1c1::37])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 05AD260121\n\tfor ;\n\tTue, 23 Nov 2021 17:28:57 +0100 (CET)","from hillosipuli.retiisi.eu\n\t(dkvn5pty0gzs3nltj987t-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:4457:9640:1e2d:1f75:a607:ef37])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (2048 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: sailus)\n\tby lahtoruutu.iki.fi (Postfix) with ESMTPSA id C21441B00056;\n\tTue, 23 Nov 2021 18:28:56 +0200 (EET)","from valkosipuli.retiisi.eu (valkosipuli.localdomain [192.168.4.2])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256) (No client certificate requested)\n\tby hillosipuli.retiisi.eu (Postfix) with ESMTPS id 4DD28634C90;\n\tTue, 23 Nov 2021 18:28:56 +0200 (EET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=iki.fi header.i=@iki.fi header.b=\"nyLgGO8J\";\n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=lahtoruutu;\n\tt=1637684936;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=pNy6BOHpUanEbyX/ceDmGyszTtQCgrbLpC+WVj6xyns=;\n\tb=nyLgGO8Jvj584JacSLzylavJJpUNAtLJ4S+zHxSdH/pXACs7LqriiKUmFnhlDIZ0vtwk3H\n\tlKiE/rskI/y5K6qezeoAL0hvfUOMBp2LKBhwCIhzV3oxAj0BdRuvojmviseRA4r/qHlCmj\n\t0B31MCrVuUL3g+5QMawjjcuAF4NcZ2oCcggKr3uIlT6TKYJ6pYOy2RXB/NYLw4KXf23TBQ\n\t35ma+s2vy3jMeKJr+CG+Zmzfcf6m6RDFQggnt4mciH7Gi8HS+078SsQRBiUuEBLsfEAMse\n\t5vdU0K0TnD9MM3l8c9OZGFPjJGRGpMFk1c21LiSrhgSkOzpISpxy3neKQTkIHw==","Date":"Tue, 23 Nov 2021 18:28:56 +0200","From":"Sakari Ailus ","To":"Laurent Pinchart ","Message-ID":"","References":"<20211122123427.808484-1-naush@raspberrypi.com>\n\t<20211122123427.808484-3-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","ARC-Seal":"i=1; s=lahtoruutu; d=iki.fi; t=1637684936; a=rsa-sha256; cv=none; \n\tb=XPfI+uXU/rKBJcEyO9yWHGzyjvnt2s5nYhbVOJYQXkRtACN0BxgH9i3ibx62PSiAnYCkPC\n\tmWkhqshH0cuMiGJBGeNa9LvEMRjZ1xU6//E0KSQDJ+2gsvPLOEijW3iisZxx//lurFhu0V\n\tlPtWzwa2TFydG8dzZX0QPdCyxY1BDNEYoth+IHnV/dsWJHvRIjsmM9TGLBTCGBbDy21Obf\n\t5HifVq96c07RealjYGslupVlaPW4v9nDIN8QIJZ7yt7QGZvqahUJUSRmUeeabk9jkW77pi\n\tI773zY8R16tdqlSqFHB60dQK1ejxNNkFTG2FmJqHF7uPLS+/gHVkv11/tmsVSA==","ARC-Authentication-Results":"i=1; ORIGINATING;\n\tauth=pass smtp.auth=sailus smtp.mailfrom=sakari.ailus@iki.fi","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi;\n\ts=lahtoruutu; t=1637684936;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=pNy6BOHpUanEbyX/ceDmGyszTtQCgrbLpC+WVj6xyns=;\n\tb=odovNH7raUZOSgjbPUuHG8JEhct/3uPSYqr9mGcAqlCDvvx6mEGkI+CFILFCKpm9r2ppCh\n\tJUcPXlC4SkGhp0vWQ5Q263JCsZ35TrnlHQZOh9RgkMWo6hBochy7yULMHg6Ma9HZGBGEr7\n\tzTTnphHhsbaG/NzxfiWFt0lRSEXF8juTicgRhjabkylkAPZvGJRmcfMY+zynYI4JzvZWXw\n\tEIZ15vFE/XeyahbEWVgqjuNuKFmBU6BRTvKxUzWAJGtK+GtdcRJkTwPrGTnoHdcnX5pn2o\n\t1Xdc8sI5qmRsEhDy3xMW1Xt6Oue/2K+F3GNbz/Jh0x3tzzfDA5yo79isIuUyHg==","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21159,"web_url":"https://patchwork.libcamera.org/comment/21159/","msgid":"","date":"2021-11-23T16:57:54","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Sakari,\n\nOn Tue, Nov 23, 2021 at 06:28:56PM +0200, Sakari Ailus wrote:\n> Hi Laurent, Naushir,\n> \n> On Mon, Nov 22, 2021 at 03:00:40PM +0200, Laurent Pinchart wrote:\n> > > -bool PipelineHandlerRPi::registerCameras()\n> > > +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> > > +\t\t\t\t\tconst std::string &deviceId)\n> > > {\n> > > \tstd::unique_ptr data = std::make_unique(this);\n> > > +\n> > > \tif (!data->dmaHeap_.isValid())\n> > > -\t\treturn false;\n> > > +\t\treturn -ENOMEM;\n> > > +\n> > > +\tMediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" + deviceId + \"-image\");\n> > > +\tMediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-output0\");\n> > > +\tMediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture1\");\n> > > +\tMediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture2\");\n> > > +\tMediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture3\");\n> > \n> > I'm wondering if wildcards would be nice here too, but maybe a simpler\n> > option would be to not include the device ID in the entity names in the\n> > driver ? We have a bus info field that the kernel reports for the media\n> > device, to differentiate between multiple instances of the same device.\n> > I suppose we are missing guidelines on the kernel side regarding entity\n> > naming. Sakari, any opinion on this ?\n> \n> I think it'd be best to be able to match multiple fields, not just entity\n> name. But I guess a regex goes a long way already.\n> \n> There are some (unwritten?) rules for naming devices and they mostly work.\n\nThis is the part I'd like your opinion on. I can read written rules\nmyself, but have harder access to the ones that are only in your brain\n:-)\n\nFrom an MC point of view, my understanding is that the driver and model\nnames should not be instance-specific (the model can vary between\ndifferent SoCs that integrate different versions/models of the same IP,\nbut shouldn't differ between multiple instances of the same IP in a\ngiven SoC, at least if those instances are identical). For entity names,\nwe have a (probably unwritten) requirement for all entities in a given\nmedia device to have different names. What's your opinion about entity\nnames when we have different media devices that correspond to multiple\ninstances of the same IP core ? For instance, with two completely\nindependent CSI-2 receivers, exposed as two separate media graphs, would\nyou name the CSI-2 RX subdevs differently or identically ? If named\ndifferently, how would they differ ?\n\n> What's deviceId here? Would you need that as these are on separate MC\n> devices? (I suppose it's up to the driver thought.)\n\nIn this specific case it's an index (equal to 0 or 1) that identifies a\nparticular instance of the ISP (there's a single ISP at the hardware\nlevel, exposed as two virtual instances by the firmware).","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 93E6CBF415\n\tfor ;\n\tTue, 23 Nov 2021 16:58:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D925160230;\n\tTue, 23 Nov 2021 17:58:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F35860121\n\tfor ;\n\tTue, 23 Nov 2021 17:58:17 +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 E6D18A1B;\n\tTue, 23 Nov 2021 17:58:16 +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=\"gjsw8M7m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637686697;\n\tbh=A9MXJSs/nQhGaB3aYhKwUatjvupJbuenca0Br/B2BKM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gjsw8M7mon2N4VOCuZhOrpQQ6YUMq6+0vEM0/ACb1IVLXJBBUN4KTT0MhSqSSiVFK\n\tXaZpY69cmhjwOm+f7jP+2vjSR/21to2jwA0cBvecaxj/LTdhprQqDwj29mHdoe08QQ\n\tZxetp8vSTBbJn/EgG4lbwA5iLZOcYvakg/7EPs9M=","Date":"Tue, 23 Nov 2021 18:57:54 +0200","From":"Laurent Pinchart ","To":"Sakari Ailus ","Message-ID":"","References":"<20211122123427.808484-1-naush@raspberrypi.com>\n\t<20211122123427.808484-3-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21181,"web_url":"https://patchwork.libcamera.org/comment/21181/","msgid":"","date":"2021-11-24T06:53:48","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":32,"url":"https://patchwork.libcamera.org/api/people/32/","name":"Sakari Ailus","email":"sakari.ailus@iki.fi"},"content":"Hi Laurent,\n\nOn Tue, Nov 23, 2021 at 06:57:54PM +0200, Laurent Pinchart wrote:\n> Hi Sakari,\n> \n> On Tue, Nov 23, 2021 at 06:28:56PM +0200, Sakari Ailus wrote:\n> > Hi Laurent, Naushir,\n> > \n> > On Mon, Nov 22, 2021 at 03:00:40PM +0200, Laurent Pinchart wrote:\n> > > > -bool PipelineHandlerRPi::registerCameras()\n> > > > +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam, MediaDevice *isp,\n> > > > +\t\t\t\t\tconst std::string &deviceId)\n> > > > {\n> > > > \tstd::unique_ptr data = std::make_unique(this);\n> > > > +\n> > > > \tif (!data->dmaHeap_.isValid())\n> > > > -\t\treturn false;\n> > > > +\t\treturn -ENOMEM;\n> > > > +\n> > > > +\tMediaEntity *unicamImage = unicam->getEntityByName(\"unicam\" + deviceId + \"-image\");\n> > > > +\tMediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-output0\");\n> > > > +\tMediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture1\");\n> > > > +\tMediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture2\");\n> > > > +\tMediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture3\");\n> > > \n> > > I'm wondering if wildcards would be nice here too, but maybe a simpler\n> > > option would be to not include the device ID in the entity names in the\n> > > driver ? We have a bus info field that the kernel reports for the media\n> > > device, to differentiate between multiple instances of the same device.\n> > > I suppose we are missing guidelines on the kernel side regarding entity\n> > > naming. Sakari, any opinion on this ?\n> > \n> > I think it'd be best to be able to match multiple fields, not just entity\n> > name. But I guess a regex goes a long way already.\n> > \n> > There are some (unwritten?) rules for naming devices and they mostly work.\n> \n> This is the part I'd like your opinion on. I can read written rules\n> myself, but have harder access to the ones that are only in your brain\n> :-)\n> \n> From an MC point of view, my understanding is that the driver and model\n> names should not be instance-specific (the model can vary between\n> different SoCs that integrate different versions/models of the same IP,\n> but shouldn't differ between multiple instances of the same IP in a\n> given SoC, at least if those instances are identical). For entity names,\n\nAgreed.\n\n> we have a (probably unwritten) requirement for all entities in a given\n> media device to have different names. What's your opinion about entity\n> names when we have different media devices that correspond to multiple\n> instances of the same IP core ? For instance, with two completely\n> independent CSI-2 receivers, exposed as two separate media graphs, would\n> you name the CSI-2 RX subdevs differently or identically ? If named\n> differently, how would they differ ?\n\nI don't think there's any benefit from having globally unique media entity\nnames, they simply need to be unique within a media device.\n\nCamera sensor entities have been named as:\n\n\tsensor name [subdev name if more than one ]i2c-address\n\nI guess that works fine as-is if you're targetting for a given system but\nthat is seldom the case anymore. Still it should be possible to recognise\ndifferent sensors from this if you know what you're looking for. Regular\nexpressions should help here.\n\n> > What's deviceId here? Would you need that as these are on separate MC\n> > devices? (I suppose it's up to the driver thought.)\n> \n> In this specific case it's an index (equal to 0 or 1) that identifies a\n> particular instance of the ISP (there's a single ISP at the hardware\n> level, exposed as two virtual instances by the firmware).\n\nI suppose the two still are present in the same media device?","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 7477EBF415\n\tfor ;\n\tWed, 24 Nov 2021 06:53:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C7FE36022F;\n\tWed, 24 Nov 2021 07:53:51 +0100 (CET)","from lahtoruutu.iki.fi (lahtoruutu.iki.fi [IPv6:2a0b:5c81:1c1::37])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F0AEB60121\n\tfor ;\n\tWed, 24 Nov 2021 07:53:49 +0100 (CET)","from hillosipuli.retiisi.eu (89-27-103-169.bb.dnainternet.fi\n\t[89.27.103.169])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (2048 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: sailus)\n\tby lahtoruutu.iki.fi (Postfix) with ESMTPSA id 0310A1B0008C;\n\tWed, 24 Nov 2021 08:53:49 +0200 (EET)","from valkosipuli.retiisi.eu (valkosipuli.localdomain [192.168.4.2])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256) (No client certificate requested)\n\tby hillosipuli.retiisi.eu (Postfix) with ESMTPS id 8FE80634C90;\n\tWed, 24 Nov 2021 08:53:48 +0200 (EET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=iki.fi header.i=@iki.fi header.b=\"l+Jl6AuU\";\n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=lahtoruutu;\n\tt=1637736829;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=vUa7hTHJOJz5C8njsGPvASEPEiO3/ss8sDlSxFsi5OQ=;\n\tb=l+Jl6AuUpxg2oKA8jB7T/XUuBLGCpleRkWaeOURuwxb+4tX/eoixhAFW3azVuzZD5S2PD5\n\tuc/+R9DlpDnLIeLyqNviFGbAP2ITT+QH7qLDBqfKuoILZA337lEHrVPkUkObE2uvfxZBzn\n\tqQP9acQRtqQ52WEbJ92QbwSE3UexhfmO+nuCiOS8NhFGaDn2xlREo6UiQXHx5pBDdLGyEw\n\tIvBqXHZFA/959qUw+H+MVeYw524iDqq/Uq3iMuSBB8wF1ISmq49j5FF1E1ZrxBvGstFAPD\n\tQNlxtQjKy5xUUX3+hvTHuTJS9jNgKgB2pihFmttun/NRBLspJBp/cuLpRicUqA==","Date":"Wed, 24 Nov 2021 08:53:48 +0200","From":"Sakari Ailus ","To":"Laurent Pinchart ","Message-ID":"","References":"<20211122123427.808484-1-naush@raspberrypi.com>\n\t<20211122123427.808484-3-naush@raspberrypi.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","ARC-Seal":"i=1; s=lahtoruutu; d=iki.fi; t=1637736829; a=rsa-sha256; cv=none; \n\tb=UX+ffP0i045FWSfMq5oF2SI+4gd5W4ihPjNCkcpO8qPgvoO6pXRXwazzDFFbGaZXmEaXT+\n\th6hOpZpdD0j6iMpvkyS8iaUeUQuHaJP+XuW4JaU0PsPBSNI9bZiT+dA6M5rrp2/6ZybxhE\n\t6QIhHekTnpL5P5UhUK7lm5RsRZslqbQweOl+cdWCL1omu4jM7xYkUJFDtWXFH1WfRj+ryp\n\tXCP/iRzy95D0YBQSZleyS66gsZ7vpkG+M4BjHVRinzlzNSQoyB5D7uK9r8YbBzWaCdoKEK\n\tp98ZeMRymGSRzJWHbk0J248herPGSorGANaQE5mszmUcdsTPz8pM+9c9GXC/bQ==","ARC-Authentication-Results":"i=1; ORIGINATING;\n\tauth=pass smtp.auth=sailus smtp.mailfrom=sakari.ailus@iki.fi","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi;\n\ts=lahtoruutu; t=1637736829;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=vUa7hTHJOJz5C8njsGPvASEPEiO3/ss8sDlSxFsi5OQ=;\n\tb=kER1YlH9Lcs8J0CIgeC60mS+HPA8plv/GUurKydVnYBZHbmr/JMq5xNia5cSGa20DguenC\n\tnqLeJPj4wPkrGNh2/cmXN0qtk9HTHIWELEUfqf3ucheZ08Xdzc0Qn8fD1Vbu9rskz6zFpB\n\t8Div/7U6DlxHo4MhI3Nh6uT7p7ukqeWOmTUYlf+H6ahFi/S0eJpxSg8aYJxm3E5OIsrHga\n\tfkX3zpYvWfCW8Xy+zc2puT6Z97Xa57B3bkbt37dj2+olJcpudoCnv43RI1t0OwHMNPYeOr\n\t5prR1fpu9yRDiZZtuLhq7Z/+kLii11CsoMChPX0gGErSOMVriMCjVr1BlIOfaw==","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21184,"web_url":"https://patchwork.libcamera.org/comment/21184/","msgid":"","date":"2021-11-24T08:45:51","subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Sakari and Laurent,\n\nOn Wed, 24 Nov 2021 at 06:53, Sakari Ailus wrote:\n\n> Hi Laurent,\n>\n> On Tue, Nov 23, 2021 at 06:57:54PM +0200, Laurent Pinchart wrote:\n> > Hi Sakari,\n> >\n> > On Tue, Nov 23, 2021 at 06:28:56PM +0200, Sakari Ailus wrote:\n> > > Hi Laurent, Naushir,\n> > >\n> > > On Mon, Nov 22, 2021 at 03:00:40PM +0200, Laurent Pinchart wrote:\n> > > > > -bool PipelineHandlerRPi::registerCameras()\n> > > > > +int PipelineHandlerRPi::registerCameras(MediaDevice *unicam,\n> MediaDevice *isp,\n> > > > > + const std::string\n> &deviceId)\n> > > > > {\n> > > > > std::unique_ptr data =\n> std::make_unique(this);\n> > > > > +\n> > > > > if (!data->dmaHeap_.isValid())\n> > > > > - return false;\n> > > > > + return -ENOMEM;\n> > > > > +\n> > > > > + MediaEntity *unicamImage =\n> unicam->getEntityByName(\"unicam\" + deviceId + \"-image\");\n> > > > > + MediaEntity *ispOutput0 =\n> isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-output0\");\n> > > > > + MediaEntity *ispCapture1 =\n> isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture1\");\n> > > > > + MediaEntity *ispCapture2 =\n> isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture2\");\n> > > > > + MediaEntity *ispCapture3 =\n> isp->getEntityByName(\"bcm2835-isp\" + deviceId + \"-capture3\");\n> > > >\n> > > > I'm wondering if wildcards would be nice here too, but maybe a\n> simpler\n> > > > option would be to not include the device ID in the entity names in\n> the\n> > > > driver ? We have a bus info field that the kernel reports for the\n> media\n> > > > device, to differentiate between multiple instances of the same\n> device.\n> > > > I suppose we are missing guidelines on the kernel side regarding\n> entity\n> > > > naming. Sakari, any opinion on this ?\n> > >\n> > > I think it'd be best to be able to match multiple fields, not just\n> entity\n> > > name. But I guess a regex goes a long way already.\n> > >\n> > > There are some (unwritten?) rules for naming devices and they mostly\n> work.\n> >\n> > This is the part I'd like your opinion on. I can read written rules\n> > myself, but have harder access to the ones that are only in your brain\n> > :-)\n> >\n> > From an MC point of view, my understanding is that the driver and model\n> > names should not be instance-specific (the model can vary between\n> > different SoCs that integrate different versions/models of the same IP,\n> > but shouldn't differ between multiple instances of the same IP in a\n> > given SoC, at least if those instances are identical). For entity names,\n>\n> Agreed.\n>\n> > we have a (probably unwritten) requirement for all entities in a given\n> > media device to have different names. What's your opinion about entity\n> > names when we have different media devices that correspond to multiple\n> > instances of the same IP core ? For instance, with two completely\n> > independent CSI-2 receivers, exposed as two separate media graphs, would\n> > you name the CSI-2 RX subdevs differently or identically ? If named\n> > differently, how would they differ ?\n>\n> I don't think there's any benefit from having globally unique media entity\n> names, they simply need to be unique within a media device.\n>\n> Camera sensor entities have been named as:\n>\n> sensor name [subdev name if more than one ]i2c-address\n>\n> I guess that works fine as-is if you're targetting for a given system but\n> that is seldom the case anymore. Still it should be possible to recognise\n> different sensors from this if you know what you're looking for. Regular\n> expressions should help here.\n>\n> > > What's deviceId here? Would you need that as these are on separate MC\n> > > devices? (I suppose it's up to the driver thought.)\n> >\n> > In this specific case it's an index (equal to 0 or 1) that identifies a\n> > particular instance of the ISP (there's a single ISP at the hardware\n> > level, exposed as two virtual instances by the firmware).\n>\n> I suppose the two still are present in the same media device?\n>\n\nThanks for all the feedback!\n\nWe have no problem duplicating driver and model names as well as entity\nnames across multiple instances of the hardware (i.e. unindexed). In many\nways\nthis makes my life easier during enumeration in libcamera.\n\nThe confusion (and subsequent discussion) started from my assumption that\nthe names needed to be different in order to enumerate the different\ninstances\nof the devices through libcamera. Laurent clarified that this is not the\ncase, and\nI've got it working now.\n\nSo if there are no objections, we will keep driver/entity names the same\nacross\nall instances of the unicam and bcm2835_isp driver.\n\nRegards,\nNaush\n\n\n>\n> --\n> Kind regards,\n>\n> Sakari Ailus\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 E5568BDB13\n\tfor ;\n\tWed, 24 Nov 2021 08:46:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E8696036F;\n\tWed, 24 Nov 2021 09:46:10 +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 538D96022F\n\tfor ;\n\tWed, 24 Nov 2021 09:46:08 +0100 (CET)","by mail-lf1-x130.google.com with SMTP id t26so5192004lfk.9\n\tfor ;\n\tWed, 24 Nov 2021 00:46:08 -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=\"NM+FsFhV\"; 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=gpLMXIk00PNgpQaGkDn5edckkzNyE8FCDqqSCdX270w=;\n\tb=NM+FsFhVko4nAAbXpHIFoVYi3a6NFtmmDeyB/sYu0ZaMwwara7vzqx6WwpNdPSJAG9\n\tfdI8sgHAe33ByoVfDvjM+A6rb3faqWFdPht/ugFk8eb1dC0ZlgnM1h6JbBgHn2S+mqnV\n\t+cvcVDZu7VPUS1n+RFqoXvdgZwJ2lCSmxrh3JnKruVex80TO17j03f0q5CpdACX21rFi\n\tW6Dunxetp+7jdki2mssJPtr9IB6U8ju+sg3c180z2aS7eSmF4uzswZPe4KpMUwGZRSBP\n\ts9VyGvbiJmeRRSRBPB+MKQ6sCtwpVVDHjor/oCl7Z8P1lAhPL8HKXv+wsFZURdcd/PlI\n\tw9Og==","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=gpLMXIk00PNgpQaGkDn5edckkzNyE8FCDqqSCdX270w=;\n\tb=DifKzGVpAx48rqFK6eQC4Et9bBsZuGqPfRdSkR+gza5ntzHLGtwS5xZbOwUH4qpEV6\n\tTKAa8lgThKS+s4adm/Q5sCeuTBOVn7mXGabnrQxgtpqgw0NJ2IA6ilNLLzLW4Dx/QhFg\n\tvBSIX2gykcP7QNkAyJP4nt1JRTlKNOtBKE4Sb4voN0Sj21DrW2AHvCkyp5CeNLK/p0GF\n\t6n8Zq4dvpENzV7zhkAJr7vEzZ00S8W3OHC17KbnBjvrOqF1gDBhC/pY2Ot3KnMJe+gtF\n\tE/9WYdOz52VHdAYRx700QNXH8eP8RNghxjCFyb7jozAep4kTAo2CIy6hXNLUMqUzTs13\n\telDw==","X-Gm-Message-State":"AOAM531edpssNvWqYBErjMXRQs5c4sPQKfzFXPkFwQdew9RIko4liE6M\n\tAcXEIaQwYvPhHo9Hd2bu9gCZabXyQYCI3ilIC1iVbQ==","X-Google-Smtp-Source":"ABdhPJylbkTWnkHnawUoff9aZBAUdCy05lhylTlCKQ7nfm75NewLt8Qmn+in4fDpXtwKJ20xMfnlQP6U8xgwfYSsttY=","X-Received":"by 2002:a05:6512:39c4:: with SMTP id\n\tk4mr12536297lfu.79.1637743567381; \n\tWed, 24 Nov 2021 00:46:07 -0800 (PST)","MIME-Version":"1.0","References":"<20211122123427.808484-1-naush@raspberrypi.com>\n\t<20211122123427.808484-3-naush@raspberrypi.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 24 Nov 2021 08:45:51 +0000","Message-ID":"","To":"Sakari Ailus ","Content-Type":"multipart/alternative; boundary=\"00000000000091372b05d184e31f\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] pipeline: raspberrypi: Allow\n\tregistration of multiple cameras","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\" "}}]