[{"id":21285,"web_url":"https://patchwork.libcamera.org/comment/21285/","msgid":"","date":"2021-11-26T16:09:01","subject":"Re: [libcamera-devel] [PATCH v4 2/2] pipeline: raspberrypi: Tidy\n\tthe camera enumeration and registration logic","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 Fri, Nov 26, 2021 at 03:35:38PM +0000, Naushir Patuck wrote:\n> When acquiring the media device, it is not necessary to match all entity names,\n> so remove it. Aditionally, we do not need to keep the MediaEntity pointers for\n> the Unicam and ISP devices stored within the PipelineHandlerRPi class. Instead\n> these can be stored locally in PipelineHandlerRPi::match().\n> \n> PipelineHandlerRPi::registerCamera() now returns an int error code instead of a\n> boolean for pass/fail.\n> \n> Signed-off-by: Naushir Patuck \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 89 +++++++++++--------\n> 1 file changed, 52 insertions(+), 37 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 5a6dfa4e8b2a..e31fa3b23859 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -314,14 +314,11 @@ private:\n> \t\treturn static_cast(camera->_d());\n> \t}\n> \n> -\tbool registerCamera();\n> +\tint registerCamera(MediaDevice *unicam, MediaDevice *isp);\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> @@ -512,7 +509,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> @@ -997,48 +994,62 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> {\n> \tDeviceMatch unicam(\"unicam\");\n> -\tDeviceMatch isp(\"bcm2835-isp\");\n> +\tMediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> \n> -\tunicam.add(\"unicam-image\");\n> +\tif (!unicamDevice) {\n> +\t\tLOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> +\t\treturn false;\n> +\t}\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> +\tDeviceMatch isp(\"bcm2835-isp\");\n> +\tMediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> \n> -\tunicam_ = acquireMediaDevice(enumerator, unicam);\n> -\tif (!unicam_)\n> +\tif (!ispDevice) {\n> +\t\tLOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> \t\treturn false;\n> +\t}\n> \n> -\tisp_ = acquireMediaDevice(enumerator, isp);\n> -\tif (!isp_)\n> +\tint ret = registerCamera(unicamDevice, ispDevice);\n> +\tif (ret) {\n> +\t\tLOG(RPI, Error) << \"Failed to register camera: \" << ret;\n> \t\treturn false;\n> +\t}\n> \n> -\treturn registerCamera();\n> +\treturn true;\n> }\n> \n> -bool PipelineHandlerRPi::registerCamera()\n> +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)\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-image\");\n> +\tMediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp0-output0\");\n> +\tMediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp0-capture1\");\n> +\tMediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp0-capture2\");\n> +\tMediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp0-capture3\");\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-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> @@ -1049,7 +1060,7 @@ bool PipelineHandlerRPi::registerCamera()\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> @@ -1057,23 +1068,23 @@ bool PipelineHandlerRPi::registerCamera()\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> @@ -1093,13 +1104,14 @@ bool PipelineHandlerRPi::registerCamera()\n> \t\tdata->streams_.push_back(&stream);\n> \n> \tfor (auto stream : data->streams_) {\n> -\t\tif (stream->dev()->open())\n> -\t\t\treturn false;\n> +\t\tint ret = stream->dev()->open();\n> +\t\tif (ret)\n> +\t\t\treturn ret;\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> @@ -1161,7 +1173,7 @@ bool PipelineHandlerRPi::registerCamera()\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> @@ -1181,7 +1193,10 @@ bool PipelineHandlerRPi::registerCamera()\n> \t\tCamera::create(std::move(data), id, streams);\n> \tPipelineHandler::registerCamera(std::move(camera));\n> \n> -\treturn true;\n> +\tLOG(RPI, Info) << \"Registered camera \" << id\n> +\t\t << \" to Unicam device \" << unicam->deviceNode()\n> +\t\t << \" and ISP device \" << isp->deviceNode();\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 3B268BDB13\n\tfor ;\n\tFri, 26 Nov 2021 16:09:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 463DD604FD;\n\tFri, 26 Nov 2021 17:09:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F9A96011E\n\tfor ;\n\tFri, 26 Nov 2021 17:09:26 +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 7097F340;\n\tFri, 26 Nov 2021 17:09:25 +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=\"Nx10OCxo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637942965;\n\tbh=ztUlOkjxWK80F8oOTzP3tH4HiYNZ7sx8lFpSlTrddlE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Nx10OCxotGjX+3eX1ocEipzWS0newg5OWjQ4fCEgaNHTblFFUhFr2yEnuS2w8Ru0O\n\t4Zb0/xrUsjqgz84bAgJWkZ7mH5/hMDSV33hcK6ETKeL+5sdBLqtdC/QyFVfKDTsNpb\n\tXchmIE28Iygihh9ehuyqQvNVw92FzKoSgCkZWG5k=","Date":"Fri, 26 Nov 2021 18:09:01 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211126153538.2594702-1-naush@raspberrypi.com>\n\t<20211126153538.2594702-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211126153538.2594702-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 2/2] pipeline: raspberrypi: Tidy\n\tthe camera enumeration and registration logic","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":21331,"web_url":"https://patchwork.libcamera.org/comment/21331/","msgid":"<163819685594.3059017.16574409222680268760@Monstersaurus>","date":"2021-11-29T14:40:55","subject":"Re: [libcamera-devel] [PATCH v4 2/2] pipeline: raspberrypi: Tidy\n\tthe camera enumeration and registration logic","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2021-11-26 15:35:38)\n> When acquiring the media device, it is not necessary to match all entity names,\n> so remove it. Aditionally, we do not need to keep the MediaEntity pointers for\n> the Unicam and ISP devices stored within the PipelineHandlerRPi class. Instead\n> these can be stored locally in PipelineHandlerRPi::match().\n> \n> PipelineHandlerRPi::registerCamera() now returns an int error code instead of a\n> boolean for pass/fail.\n\nThis looks good. I think it's clearer even for a single camera too, and\nit will be nice to see when we get multiple camera's running through.\n\nI know of the StereoPi use case already, and I guess there will be\nothers with the CM4 using this.\n\n\nReviewed-by: Kieran Bingham \n\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 89 +++++++++++--------\n> 1 file changed, 52 insertions(+), 37 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 5a6dfa4e8b2a..e31fa3b23859 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -314,14 +314,11 @@ private:\n> return static_cast(camera->_d());\n> }\n> \n> - bool registerCamera();\n> + int registerCamera(MediaDevice *unicam, MediaDevice *isp);\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> @@ -512,7 +509,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> @@ -997,48 +994,62 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> {\n> DeviceMatch unicam(\"unicam\");\n> - DeviceMatch isp(\"bcm2835-isp\");\n> + MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> \n> - unicam.add(\"unicam-image\");\n> + if (!unicamDevice) {\n> + LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> + return false;\n> + }\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> + DeviceMatch isp(\"bcm2835-isp\");\n> + MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> \n> - unicam_ = acquireMediaDevice(enumerator, unicam);\n> - if (!unicam_)\n> + if (!ispDevice) {\n> + LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> return false;\n> + }\n> \n> - isp_ = acquireMediaDevice(enumerator, isp);\n> - if (!isp_)\n> + int ret = registerCamera(unicamDevice, ispDevice);\n> + if (ret) {\n> + LOG(RPI, Error) << \"Failed to register camera: \" << ret;\n> return false;\n> + }\n> \n> - return registerCamera();\n> + return true;\n> }\n> \n> -bool PipelineHandlerRPi::registerCamera()\n> +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)\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-image\");\n> + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp0-output0\");\n> + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp0-capture1\");\n> + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp0-capture2\");\n> + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp0-capture3\");\n> +\n> + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)\n> + return -ENOENT;\n> \n> /* Locate and open the unicam video streams. */\n> - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicam_->getEntityByName(\"unicam-image\"));\n> + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicamImage);\n> \n> /* An embedded data node will not be present if the sensor does not support it. */\n> - MediaEntity *embeddedEntity = unicam_->getEntityByName(\"unicam-embedded\");\n> - if (embeddedEntity) {\n> - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", embeddedEntity);\n> + MediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam-embedded\");\n> + if (unicamEmbedded) {\n> + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> &RPiCameraData::unicamBufferDequeue);\n> }\n> \n> /* Tag the ISP input stream as an import stream. */\n> - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, true);\n> + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1);\n> + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> \n> /* Wire up all the buffer connections. */\n> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n> @@ -1049,7 +1060,7 @@ bool PipelineHandlerRPi::registerCamera()\n> data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);\n> \n> /* Identify the sensor. */\n> - for (MediaEntity *entity : unicam_->entities()) {\n> + for (MediaEntity *entity : unicam->entities()) {\n> if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> data->sensor_ = std::make_unique(entity);\n> break;\n> @@ -1057,23 +1068,23 @@ bool PipelineHandlerRPi::registerCamera()\n> }\n> \n> if (!data->sensor_)\n> - return false;\n> + return -EINVAL;\n> \n> if (data->sensor_->init())\n> - return false;\n> + return -EINVAL;\n> \n> data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> \n> ipa::RPi::SensorConfig sensorConfig;\n> if (data->loadIPA(&sensorConfig)) {\n> LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> - return false;\n> + return -EINVAL;\n> }\n> \n> - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n> + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> LOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> sensorConfig.sensorMetadata = false;\n> - if (embeddedEntity)\n> + if (unicamEmbedded)\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> }\n> \n> @@ -1093,13 +1104,14 @@ bool PipelineHandlerRPi::registerCamera()\n> data->streams_.push_back(&stream);\n> \n> for (auto stream : data->streams_) {\n> - if (stream->dev()->open())\n> - return false;\n> + int ret = stream->dev()->open();\n> + if (ret)\n> + return ret;\n> }\n> \n> if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> LOG(RPI, Error) << \"Unicam driver does not use the MediaController, please update your kernel!\";\n> - return false;\n> + return -EINVAL;\n> }\n> \n> /*\n> @@ -1161,7 +1173,7 @@ bool PipelineHandlerRPi::registerCamera()\n> \n> if (!bayerFormat.isValid()) {\n> LOG(RPI, Error) << \"No Bayer format found\";\n> - return false;\n> + return -EINVAL;\n> }\n> data->nativeBayerOrder_ = bayerFormat.order;\n> \n> @@ -1181,7 +1193,10 @@ bool PipelineHandlerRPi::registerCamera()\n> Camera::create(std::move(data), id, streams);\n> PipelineHandler::registerCamera(std::move(camera));\n> \n> - return true;\n> + LOG(RPI, Info) << \"Registered camera \" << id\n> + << \" to Unicam device \" << unicam->deviceNode()\n> + << \" and ISP device \" << isp->deviceNode();\n> + return 0;\n> }\n> \n> int PipelineHandlerRPi::queueAllBuffers(Camera *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 99F59BDB13\n\tfor ;\n\tMon, 29 Nov 2021 14:41:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D96CB605A6;\n\tMon, 29 Nov 2021 15:40:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8EFB260592\n\tfor ;\n\tMon, 29 Nov 2021 15:40:58 +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 200582A5;\n\tMon, 29 Nov 2021 15:40:58 +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=\"DgAHegIF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638196858;\n\tbh=UqHnYUqPfWilJ5S8Hnwf/5ceqgxu5v2K/L5U2x7+iYQ=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=DgAHegIF01d91pjOYE8lieV7QeTf1DTSYz738w7ORYDBy/g0ziXdMWQa4d/f75UKa\n\tFJx1Vudkqe1CUQjcGOoTd7C0UQF770cKFWhVkYfpZm8QO6ecgJKNgV6l9nig0LLj6H\n\tRr2ECOstguR7CsrgUgBh1lWJScxbeMmxtyhGY6vA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211126153538.2594702-2-naush@raspberrypi.com>","References":"<20211126153538.2594702-1-naush@raspberrypi.com>\n\t<20211126153538.2594702-2-naush@raspberrypi.com>","From":"Kieran Bingham ","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 29 Nov 2021 14:40:55 +0000","Message-ID":"<163819685594.3059017.16574409222680268760@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 2/2] pipeline: raspberrypi: Tidy\n\tthe camera enumeration and registration logic","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":21332,"web_url":"https://patchwork.libcamera.org/comment/21332/","msgid":"","date":"2021-11-29T14:51:08","subject":"Re: [libcamera-devel] [PATCH v4 2/2] pipeline: raspberrypi: Tidy\n\tthe camera enumeration and registration logic","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"On Mon, 29 Nov 2021 at 14:41, Kieran Bingham\n wrote:\n>\n> Quoting Naushir Patuck (2021-11-26 15:35:38)\n> > When acquiring the media device, it is not necessary to match all entity names,\n> > so remove it. Aditionally, we do not need to keep the MediaEntity pointers for\n> > the Unicam and ISP devices stored within the PipelineHandlerRPi class. Instead\n> > these can be stored locally in PipelineHandlerRPi::match().\n> >\n> > PipelineHandlerRPi::registerCamera() now returns an int error code instead of a\n> > boolean for pass/fail.\n>\n> This looks good. I think it's clearer even for a single camera too, and\n> it will be nice to see when we get multiple camera's running through.\n>\n> I know of the StereoPi use case already, and I guess there will be\n> others with the CM4 using this.\n\nThis is two simultaneous but independent cameras.\nStereoscopic requires a bit more work to be able to combine the output\nof two ISP passes into one buffer for side-by-side, or top-bottom.\nI suspect there will be a load of discussion required for how the\nlibcamera frameworks wish to support stereoscopic, and how IPAs are\nexpected to handle stereo.\n\n> Reviewed-by: Kieran Bingham \n>\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 89 +++++++++++--------\n> > 1 file changed, 52 insertions(+), 37 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 5a6dfa4e8b2a..e31fa3b23859 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -314,14 +314,11 @@ private:\n> > return static_cast(camera->_d());\n> > }\n> >\n> > - bool registerCamera();\n> > + int registerCamera(MediaDevice *unicam, MediaDevice *isp);\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> > @@ -512,7 +509,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> > @@ -997,48 +994,62 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > {\n> > DeviceMatch unicam(\"unicam\");\n> > - DeviceMatch isp(\"bcm2835-isp\");\n> > + MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> >\n> > - unicam.add(\"unicam-image\");\n> > + if (!unicamDevice) {\n> > + LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> > + return false;\n> > + }\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> > + DeviceMatch isp(\"bcm2835-isp\");\n> > + MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> >\n> > - unicam_ = acquireMediaDevice(enumerator, unicam);\n> > - if (!unicam_)\n> > + if (!ispDevice) {\n> > + LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> > return false;\n> > + }\n> >\n> > - isp_ = acquireMediaDevice(enumerator, isp);\n> > - if (!isp_)\n> > + int ret = registerCamera(unicamDevice, ispDevice);\n> > + if (ret) {\n> > + LOG(RPI, Error) << \"Failed to register camera: \" << ret;\n> > return false;\n> > + }\n> >\n> > - return registerCamera();\n> > + return true;\n> > }\n> >\n> > -bool PipelineHandlerRPi::registerCamera()\n> > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)\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-image\");\n> > + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp0-output0\");\n> > + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp0-capture1\");\n> > + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp0-capture2\");\n> > + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp0-capture3\");\n> > +\n> > + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)\n> > + return -ENOENT;\n> >\n> > /* Locate and open the unicam video streams. */\n> > - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicam_->getEntityByName(\"unicam-image\"));\n> > + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicamImage);\n> >\n> > /* An embedded data node will not be present if the sensor does not support it. */\n> > - MediaEntity *embeddedEntity = unicam_->getEntityByName(\"unicam-embedded\");\n> > - if (embeddedEntity) {\n> > - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", embeddedEntity);\n> > + MediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam-embedded\");\n> > + if (unicamEmbedded) {\n> > + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> > data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> > &RPiCameraData::unicamBufferDequeue);\n> > }\n> >\n> > /* Tag the ISP input stream as an import stream. */\n> > - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> > - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> > - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> > - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> > + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, true);\n> > + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1);\n> > + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> > + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> >\n> > /* Wire up all the buffer connections. */\n> > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n> > @@ -1049,7 +1060,7 @@ bool PipelineHandlerRPi::registerCamera()\n> > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);\n> >\n> > /* Identify the sensor. */\n> > - for (MediaEntity *entity : unicam_->entities()) {\n> > + for (MediaEntity *entity : unicam->entities()) {\n> > if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> > data->sensor_ = std::make_unique(entity);\n> > break;\n> > @@ -1057,23 +1068,23 @@ bool PipelineHandlerRPi::registerCamera()\n> > }\n> >\n> > if (!data->sensor_)\n> > - return false;\n> > + return -EINVAL;\n> >\n> > if (data->sensor_->init())\n> > - return false;\n> > + return -EINVAL;\n> >\n> > data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> >\n> > ipa::RPi::SensorConfig sensorConfig;\n> > if (data->loadIPA(&sensorConfig)) {\n> > LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > - return false;\n> > + return -EINVAL;\n> > }\n> >\n> > - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n> > + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> > LOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> > sensorConfig.sensorMetadata = false;\n> > - if (embeddedEntity)\n> > + if (unicamEmbedded)\n> > data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > }\n> >\n> > @@ -1093,13 +1104,14 @@ bool PipelineHandlerRPi::registerCamera()\n> > data->streams_.push_back(&stream);\n> >\n> > for (auto stream : data->streams_) {\n> > - if (stream->dev()->open())\n> > - return false;\n> > + int ret = stream->dev()->open();\n> > + if (ret)\n> > + return ret;\n> > }\n> >\n> > if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> > LOG(RPI, Error) << \"Unicam driver does not use the MediaController, please update your kernel!\";\n> > - return false;\n> > + return -EINVAL;\n> > }\n> >\n> > /*\n> > @@ -1161,7 +1173,7 @@ bool PipelineHandlerRPi::registerCamera()\n> >\n> > if (!bayerFormat.isValid()) {\n> > LOG(RPI, Error) << \"No Bayer format found\";\n> > - return false;\n> > + return -EINVAL;\n> > }\n> > data->nativeBayerOrder_ = bayerFormat.order;\n> >\n> > @@ -1181,7 +1193,10 @@ bool PipelineHandlerRPi::registerCamera()\n> > Camera::create(std::move(data), id, streams);\n> > PipelineHandler::registerCamera(std::move(camera));\n> >\n> > - return true;\n> > + LOG(RPI, Info) << \"Registered camera \" << id\n> > + << \" to Unicam device \" << unicam->deviceNode()\n> > + << \" and ISP device \" << isp->deviceNode();\n> > + return 0;\n> > }\n> >\n> > int PipelineHandlerRPi::queueAllBuffers(Camera *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 DAD3BBF415\n\tfor ;\n\tMon, 29 Nov 2021 14:51:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE9B2605A4;\n\tMon, 29 Nov 2021 15:51:30 +0100 (CET)","from mail-wm1-x32b.google.com (mail-wm1-x32b.google.com\n\t[IPv6:2a00:1450:4864:20::32b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 845E960592\n\tfor ;\n\tMon, 29 Nov 2021 15:51:28 +0100 (CET)","by mail-wm1-x32b.google.com with SMTP id i12so14866783wmq.4\n\tfor ;\n\tMon, 29 Nov 2021 06:51:28 -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=\"POUpYDhk\"; 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=NabvRGUZ6h7QT3yshnjCeMpudHog7oLPrXED0FZW6eM=;\n\tb=POUpYDhkk+sMuUL4x8kpLFA1rMBDWrDPt6MXaKs7glbzQtiJ3e0/vypcc4NJfX2WZA\n\tgJQeG06quR98qSkNVkd4X0IBDnV1AYKkgFY+eq9BsGDtEeuwOVkL80ZxjC0lb/Mm4Yw7\n\tUbaVuDPKwIQhGF0GP8KHpIyKl8KycZ4p858i9WIQya4G5K6uFfbqvAwKsyM8HZc6OXzg\n\t8j6CZRcrRkTo4NgkbBhlc0yMybv3FDteZp9p2a9ddqjpqS2FCzF8SnIOx5hDe5zcOVx8\n\t81CeydwnqPde475xIKhqSXZTUOQSehIfAQwsjkQ8AUj0MrLwzA1vb/zTQ2SM+5DZuSmM\n\tsVAw==","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=NabvRGUZ6h7QT3yshnjCeMpudHog7oLPrXED0FZW6eM=;\n\tb=UNM3nisK/FiiJehP6qBxzXB/TrUPxcogbTxw7p+4ekqD1bUY1TfF+xKbtrVKOOuOu3\n\t14gIZdAEVzaoUxkIEtGGb0sv1hxhvzTb2iNUpr4CERqRl04n1fkE5n67cBZpUMuTQJRJ\n\tNVhamWt7Zo9kSXMVO5nXbrh8ExhW4rMvwpGaesp3JIGAQAx2id2hrkFGAGtqDURUKpRh\n\taJhNTp/HEo8T5SZzTjhc8MlZDOn022w/eFdLw+vcbdNaMkyJwUnpCFkH0WMstLluTy/g\n\t2E6GT6DrwT8esU5zGsvAt/u9t3zGGIiy7w3c+Ow0B5Ulg07qzzT/nOmiLVlv7Ft1eR7f\n\tIlnQ==","X-Gm-Message-State":"AOAM533e/PTUWGRg4X64azxdRc8juWesrp6mLfmpphuxJ2fd51JArJx5\n\tbwvY9JbgaenjuskElo18fYWl3bqaSWhl2ifwl7R9wQ==","X-Google-Smtp-Source":"ABdhPJzC1FOIgz/NuQdUwDP9NBQ+XcsKjcDvwZrBj+7oEPI4CEXGNUjvmfq7b538JPKBNgKKOnVDWOed++hgmCIyx84=","X-Received":"by 2002:a05:600c:358a:: with SMTP id\n\tp10mr36274360wmq.180.1638197488048; \n\tMon, 29 Nov 2021 06:51:28 -0800 (PST)","MIME-Version":"1.0","References":"<20211126153538.2594702-1-naush@raspberrypi.com>\n\t<20211126153538.2594702-2-naush@raspberrypi.com>\n\t<163819685594.3059017.16574409222680268760@Monstersaurus>","In-Reply-To":"<163819685594.3059017.16574409222680268760@Monstersaurus>","From":"Dave Stevenson ","Date":"Mon, 29 Nov 2021 14:51:08 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v4 2/2] pipeline: raspberrypi: Tidy\n\tthe camera enumeration and registration logic","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21378,"web_url":"https://patchwork.libcamera.org/comment/21378/","msgid":"<163822771788.3059017.7494283935318830362@Monstersaurus>","date":"2021-11-29T23:15:17","subject":"Re: [libcamera-devel] [PATCH v4 2/2] pipeline: raspberrypi: Tidy\n\tthe camera enumeration and registration logic","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Dave Stevenson (2021-11-29 14:51:08)\n> On Mon, 29 Nov 2021 at 14:41, Kieran Bingham\n> wrote:\n> >\n> > Quoting Naushir Patuck (2021-11-26 15:35:38)\n> > > When acquiring the media device, it is not necessary to match all entity names,\n> > > so remove it. Aditionally, we do not need to keep the MediaEntity pointers for\n> > > the Unicam and ISP devices stored within the PipelineHandlerRPi class. Instead\n> > > these can be stored locally in PipelineHandlerRPi::match().\n> > >\n> > > PipelineHandlerRPi::registerCamera() now returns an int error code instead of a\n> > > boolean for pass/fail.\n> >\n> > This looks good. I think it's clearer even for a single camera too, and\n> > it will be nice to see when we get multiple camera's running through.\n> >\n> > I know of the StereoPi use case already, and I guess there will be\n> > others with the CM4 using this.\n> \n> This is two simultaneous but independent cameras.\n> Stereoscopic requires a bit more work to be able to combine the output\n> of two ISP passes into one buffer for side-by-side, or top-bottom.\n> I suspect there will be a load of discussion required for how the\n> libcamera frameworks wish to support stereoscopic, and how IPAs are\n> expected to handle stereo.\n\nAh, yes I can see keeping them in sync will take more work than just\ncapturing the same frames and having frame-sync between them. We'll have\nto make sure the IPA's either communicate, or are consistent between the\ntwo cameras.\n\nWell, that's for later, this series is merged, so at least we can\ncapture from both cameras.\n\nThanks.\n\nKieran\n\n\n> \n> > Reviewed-by: Kieran Bingham \n> >\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 89 +++++++++++--------\n> > > 1 file changed, 52 insertions(+), 37 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 5a6dfa4e8b2a..e31fa3b23859 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -314,14 +314,11 @@ private:\n> > > return static_cast(camera->_d());\n> > > }\n> > >\n> > > - bool registerCamera();\n> > > + int registerCamera(MediaDevice *unicam, MediaDevice *isp);\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> > > @@ -512,7 +509,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> > > @@ -997,48 +994,62 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > {\n> > > DeviceMatch unicam(\"unicam\");\n> > > - DeviceMatch isp(\"bcm2835-isp\");\n> > > + MediaDevice *unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > >\n> > > - unicam.add(\"unicam-image\");\n> > > + if (!unicamDevice) {\n> > > + LOG(RPI, Debug) << \"Unable to acquire a Unicam instance\";\n> > > + return false;\n> > > + }\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> > > + DeviceMatch isp(\"bcm2835-isp\");\n> > > + MediaDevice *ispDevice = acquireMediaDevice(enumerator, isp);\n> > >\n> > > - unicam_ = acquireMediaDevice(enumerator, unicam);\n> > > - if (!unicam_)\n> > > + if (!ispDevice) {\n> > > + LOG(RPI, Debug) << \"Unable to acquire ISP instance\";\n> > > return false;\n> > > + }\n> > >\n> > > - isp_ = acquireMediaDevice(enumerator, isp);\n> > > - if (!isp_)\n> > > + int ret = registerCamera(unicamDevice, ispDevice);\n> > > + if (ret) {\n> > > + LOG(RPI, Error) << \"Failed to register camera: \" << ret;\n> > > return false;\n> > > + }\n> > >\n> > > - return registerCamera();\n> > > + return true;\n> > > }\n> > >\n> > > -bool PipelineHandlerRPi::registerCamera()\n> > > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp)\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-image\");\n> > > + MediaEntity *ispOutput0 = isp->getEntityByName(\"bcm2835-isp0-output0\");\n> > > + MediaEntity *ispCapture1 = isp->getEntityByName(\"bcm2835-isp0-capture1\");\n> > > + MediaEntity *ispCapture2 = isp->getEntityByName(\"bcm2835-isp0-capture2\");\n> > > + MediaEntity *ispCapture3 = isp->getEntityByName(\"bcm2835-isp0-capture3\");\n> > > +\n> > > + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 || !ispCapture3)\n> > > + return -ENOENT;\n> > >\n> > > /* Locate and open the unicam video streams. */\n> > > - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicam_->getEntityByName(\"unicam-image\"));\n> > > + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicamImage);\n> > >\n> > > /* An embedded data node will not be present if the sensor does not support it. */\n> > > - MediaEntity *embeddedEntity = unicam_->getEntityByName(\"unicam-embedded\");\n> > > - if (embeddedEntity) {\n> > > - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", embeddedEntity);\n> > > + MediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam-embedded\");\n> > > + if (unicamEmbedded) {\n> > > + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n> > > data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> > > &RPiCameraData::unicamBufferDequeue);\n> > > }\n> > >\n> > > /* Tag the ISP input stream as an import stream. */\n> > > - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> > > - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> > > - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> > > - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> > > + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, true);\n> > > + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1);\n> > > + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> > > + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> > >\n> > > /* Wire up all the buffer connections. */\n> > > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n> > > @@ -1049,7 +1060,7 @@ bool PipelineHandlerRPi::registerCamera()\n> > > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispOutputDequeue);\n> > >\n> > > /* Identify the sensor. */\n> > > - for (MediaEntity *entity : unicam_->entities()) {\n> > > + for (MediaEntity *entity : unicam->entities()) {\n> > > if (entity->function() == MEDIA_ENT_F_CAM_SENSOR) {\n> > > data->sensor_ = std::make_unique(entity);\n> > > break;\n> > > @@ -1057,23 +1068,23 @@ bool PipelineHandlerRPi::registerCamera()\n> > > }\n> > >\n> > > if (!data->sensor_)\n> > > - return false;\n> > > + return -EINVAL;\n> > >\n> > > if (data->sensor_->init())\n> > > - return false;\n> > > + return -EINVAL;\n> > >\n> > > data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > >\n> > > ipa::RPi::SensorConfig sensorConfig;\n> > > if (data->loadIPA(&sensorConfig)) {\n> > > LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > > - return false;\n> > > + return -EINVAL;\n> > > }\n> > >\n> > > - if (sensorConfig.sensorMetadata ^ !!embeddedEntity) {\n> > > + if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> > > LOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> > > sensorConfig.sensorMetadata = false;\n> > > - if (embeddedEntity)\n> > > + if (unicamEmbedded)\n> > > data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > > }\n> > >\n> > > @@ -1093,13 +1104,14 @@ bool PipelineHandlerRPi::registerCamera()\n> > > data->streams_.push_back(&stream);\n> > >\n> > > for (auto stream : data->streams_) {\n> > > - if (stream->dev()->open())\n> > > - return false;\n> > > + int ret = stream->dev()->open();\n> > > + if (ret)\n> > > + return ret;\n> > > }\n> > >\n> > > if (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> > > LOG(RPI, Error) << \"Unicam driver does not use the MediaController, please update your kernel!\";\n> > > - return false;\n> > > + return -EINVAL;\n> > > }\n> > >\n> > > /*\n> > > @@ -1161,7 +1173,7 @@ bool PipelineHandlerRPi::registerCamera()\n> > >\n> > > if (!bayerFormat.isValid()) {\n> > > LOG(RPI, Error) << \"No Bayer format found\";\n> > > - return false;\n> > > + return -EINVAL;\n> > > }\n> > > data->nativeBayerOrder_ = bayerFormat.order;\n> > >\n> > > @@ -1181,7 +1193,10 @@ bool PipelineHandlerRPi::registerCamera()\n> > > Camera::create(std::move(data), id, streams);\n> > > PipelineHandler::registerCamera(std::move(camera));\n> > >\n> > > - return true;\n> > > + LOG(RPI, Info) << \"Registered camera \" << id\n> > > + << \" to Unicam device \" << unicam->deviceNode()\n> > > + << \" and ISP device \" << isp->deviceNode();\n> > > + return 0;\n> > > }\n> > >\n> > > int PipelineHandlerRPi::queueAllBuffers(Camera *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 89D97BDB13\n\tfor ;\n\tMon, 29 Nov 2021 23:15:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A86FC605A4;\n\tTue, 30 Nov 2021 00:15:22 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DF6E6604FC\n\tfor ;\n\tTue, 30 Nov 2021 00:15:20 +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 7814AD97;\n\tTue, 30 Nov 2021 00:15:20 +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=\"XHBp2UgA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638227720;\n\tbh=Ya7ftrgYhSgwPVASuTIn6FKOl2X9sdyLzoS5abzobaw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=XHBp2UgA94EHcXcWaeKfO3G/HJC89zlFlq/YiwOIKJMfnBJukVCZKbncSdZZsZnFW\n\tJet5WJOXokhHrhFms/uLQZblwTrU3FoEBzW3qSFjI8o9gWdMQ0s2Pof9Qtb+AKNy1n\n\t5EGch3fT7lX5UWKUhEaaSMojImsfEVUdlEKdGM6E=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211126153538.2594702-1-naush@raspberrypi.com>\n\t<20211126153538.2594702-2-naush@raspberrypi.com>\n\t<163819685594.3059017.16574409222680268760@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Dave Stevenson ","Date":"Mon, 29 Nov 2021 23:15:17 +0000","Message-ID":"<163822771788.3059017.7494283935318830362@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 2/2] pipeline: raspberrypi: Tidy\n\tthe camera enumeration and registration logic","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\" "}}]