[{"id":21205,"web_url":"https://patchwork.libcamera.org/comment/21205/","msgid":"","date":"2021-11-24T23:16:12","subject":"Re: [libcamera-devel] [PATCH v3 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 Tue, Nov 23, 2021 at 01:10:40PM +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> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 88 +++++++++++--------\n> 1 file changed, 52 insertions(+), 36 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 12fd38061241..c5034480820e 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 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> @@ -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,65 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> \n> bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> {\n> +\tMediaDevice *unicamDevice, *ispDevice;\n> +\n> \tDeviceMatch unicam(\"unicam\");\n> -\tDeviceMatch isp(\"bcm2835-isp\");\n> +\tunicamDevice = acquireMediaDevice(enumerator, unicam);\n\nYou can declare the variable here. Same for ispDevice below.\n\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> +\tispDevice = 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> @@ -1046,7 +1059,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> @@ -1054,23 +1067,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> @@ -1091,12 +1104,12 @@ bool PipelineHandlerRPi::registerCamera()\n> \n> \tfor (auto stream : data->streams_) {\n> \t\tif (stream->dev()->open())\n> -\t\t\treturn false;\n> +\t\t\tcontinue;\n\nIs this correct ? If some of the devices can't be opened, shouldn't it\nbe a fatal error ?\n\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 +1171,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> @@ -1178,7 +1191,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 DD033BF415\n\tfor ;\n\tWed, 24 Nov 2021 23:16:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5EDB560231;\n\tThu, 25 Nov 2021 00:16:37 +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 82DCE60228\n\tfor ;\n\tThu, 25 Nov 2021 00:16:35 +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 009A92C5;\n\tThu, 25 Nov 2021 00:16:34 +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=\"m8b7FGnZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637795795;\n\tbh=U0yOUXXQVea8/xo+XznmabLV4aZul2/pVD6qVVfEEB0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m8b7FGnZgDPu0q/vDnob/uLohbhv398n5S/2W1q8TzciI1oYNYLOHujnd/1H7gNcH\n\tbgksk34af7ZwfEm7QO9PjEgmlELBh/nqTEtLU5NVrEU9G9r5eVhrn1dJxnYJDR8/dp\n\tjmZuufMajlfhA2NqmTu3bG7/ld0BntXrYQzem0mg=","Date":"Thu, 25 Nov 2021 01:16:12 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211123131040.1542581-1-naush@raspberrypi.com>\n\t<20211123131040.1542581-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211123131040.1542581-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 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":21219,"web_url":"https://patchwork.libcamera.org/comment/21219/","msgid":"","date":"2021-11-25T09:49:31","subject":"Re: [libcamera-devel] [PATCH v3 2/2] pipeline: raspberrypi: Tidy\n\tthe camera enumeration and registration logic","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Wed, 24 Nov 2021 at 23:16, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, Nov 23, 2021 at 01:10:40PM +0000, Naushir Patuck wrote:\n> > When acquiring the media device, it is not necessary to match all entity\n> names,\n> > so remove it. Aditionally, we do not need to keep the MediaEntity\n> pointers for\n> > the Unicam and ISP devices stored within the PipelineHandlerRPi class.\n> Instead\n> > these can be stored locally in PipelineHandlerRPi::match().\n> >\n> > PipelineHandlerRPi::registerCamera() now returns an int error code\n> instead of a\n> > boolean for pass/fail.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 88 +++++++++++--------\n> > 1 file changed, 52 insertions(+), 36 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 12fd38061241..c5034480820e 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 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,\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,65 @@ int PipelineHandlerRPi::queueRequestDevice(Camera\n> *camera, Request *request)\n> >\n> > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > {\n> > + MediaDevice *unicamDevice, *ispDevice;\n> > +\n> > DeviceMatch unicam(\"unicam\");\n> > - DeviceMatch isp(\"bcm2835-isp\");\n> > + unicamDevice = acquireMediaDevice(enumerator, unicam);\n>\n> You can declare the variable here. Same for ispDevice below.\n>\n\nAck.\n\n\n>\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> > + 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\n> *isp)\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-image\");\n> > + MediaEntity *ispOutput0 =\n> isp->getEntityByName(\"bcm2835-isp0-output0\");\n> > + MediaEntity *ispCapture1 =\n> isp->getEntityByName(\"bcm2835-isp0-capture1\");\n> > + MediaEntity *ispCapture2 =\n> isp->getEntityByName(\"bcm2835-isp0-capture2\");\n> > + MediaEntity *ispCapture3 =\n> isp->getEntityByName(\"bcm2835-isp0-capture3\");\n> > +\n> > + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 ||\n> !ispCapture3)\n> > + return -ENOENT;\n> >\n> > /* Locate and open the unicam video streams. */\n> > - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> unicam_->getEntityByName(\"unicam-image\"));\n> > + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> unicamImage);\n> >\n> > /* An embedded data node will not be present if the sensor does\n> not support it. */\n> > - MediaEntity *embeddedEntity =\n> unicam_->getEntityByName(\"unicam-embedded\");\n> > - if (embeddedEntity) {\n> > - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n> Embedded\", embeddedEntity);\n> > + MediaEntity *unicamEmbedded =\n> unicam->getEntityByName(\"unicam-embedded\");\n> > + if (unicamEmbedded) {\n> > + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n> Embedded\", unicamEmbedded);\n> >\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> >\n> &RPiCameraData::unicamBufferDequeue);\n> > }\n> >\n> > /* Tag the ISP input stream as an import stream. */\n> > - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\",\n> isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> > - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> > - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> > - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> > + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0,\n> 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(),\n> &RPiCameraData::frameStarted);\n> > @@ -1046,7 +1059,7 @@ bool PipelineHandlerRPi::registerCamera()\n> > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(),\n> &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_ =\n> std::make_unique(entity);\n> > break;\n> > @@ -1054,23 +1067,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\n> CamHelper for embedded data usage!\";\n> > sensorConfig.sensorMetadata = false;\n> > - if (embeddedEntity)\n> > + if (unicamEmbedded)\n> >\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > }\n> >\n> > @@ -1091,12 +1104,12 @@ bool PipelineHandlerRPi::registerCamera()\n> >\n> > for (auto stream : data->streams_) {\n> > if (stream->dev()->open())\n> > - return false;\n> > + continue;\n>\n> Is this correct ? If some of the devices can't be opened, shouldn't it\n> be a fatal error ?\n>\n\nGood catch, yes it should!\nI'll fix this up for the next version.\n\nRegards,\nNaush\n\n\n\n>\n> > }\n> >\n> > if\n> (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> > LOG(RPI, Error) << \"Unicam driver does not use the\n> MediaController, please update your kernel!\";\n> > - return false;\n> > + return -EINVAL;\n> > }\n> >\n> > /*\n> > @@ -1158,7 +1171,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> > @@ -1178,7 +1191,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> --\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 AD010BDB13\n\tfor ;\n\tThu, 25 Nov 2021 09:49:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E15FF6038B;\n\tThu, 25 Nov 2021 10:49:48 +0100 (CET)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE7B260231\n\tfor ;\n\tThu, 25 Nov 2021 10:49:47 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id u22so11359990lju.7\n\tfor ;\n\tThu, 25 Nov 2021 01:49:47 -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=\"XgFN+6L5\"; 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=FrTRdBaDdHjZ3/4QuIPWP/EHksVPnLe7ikZsE12tprs=;\n\tb=XgFN+6L5iU8pe2xxmBHc3Ecf81spaVwsguekqfSE4eIo+6/jH1gVtzShqLbWzu4oFb\n\tC0KAdsnA4Qv9WOyyh530P0uVXGKSGs+ASpyDGLkbKQeJBmZVf/0UOzHP81gc2FIwEs2C\n\t5KbrxhwEXpgnsvCxO5Gh5ZBpgxfHvwp7qMn9ltnOiOQnFyH9dsf5WbyI9+BJ4DwRnyWF\n\tgL+SXuIs0bA7mpYIZ9vgYlWk80QtM3AnihHzDnaf4JN5f9a8yqeSJWNxNgmN6wpthaJe\n\tqsRmnsmjeRkhu6VvAA1L3irBKugsmnWK5Te4lC4z4QTLmbp9EkYIwc+UrEHrUwTOEmnH\n\t9ALw==","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=FrTRdBaDdHjZ3/4QuIPWP/EHksVPnLe7ikZsE12tprs=;\n\tb=dUYGug4t67/hdfi7in+zvzMlZNajEH3JK7EzMjx1uhak9v2pv3HX0R0D7Zefp2+Il0\n\tdZk6uQOwYLRfatdkER7NJs/xzZ3nJQauKi1EQqMnbsBpogwnYRkcZtpoaIUkNwip3hW0\n\t3R3d5JHpyUnrNhi06/DN4bWVPyGG6mO7jMQCHddm+SxEORqrW3Cm81yPapUEMaIInKiz\n\t6Apaj/6I80bvrLiHF9RcFsPw44+R4W6laV29F95IQ6Z+Rw0Mq7uqB8/Mvqz1BEUho0zQ\n\thFlhu+CVdDpvwGayJNYHm2ETA/LFIQeAGYxanRurcKwD/+APqoTX1W0NwLl6WSdBXt7J\n\tlCLw==","X-Gm-Message-State":"AOAM533RUhEbbSBrcnx3kqouAtLi6+Y9d9kfxJrrgW2SRS5R9OnLTmvS\n\t3K747ruPVboXFiR52zPS7CBdhBuea1WRjQqfdDB5zIpjaZyOOg==","X-Google-Smtp-Source":"ABdhPJzgLJ/fZfghn0lnlUAj31uenLUxgnuPtERREofS5NUC0wdERUjAS734tpL/FjKoGKyiLCxrcY4Ab0YVqgorZVQ=","X-Received":"by 2002:a2e:a22a:: with SMTP id\n\ti10mr23849184ljm.16.1637833787084; \n\tThu, 25 Nov 2021 01:49:47 -0800 (PST)","MIME-Version":"1.0","References":"<20211123131040.1542581-1-naush@raspberrypi.com>\n\t<20211123131040.1542581-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Thu, 25 Nov 2021 09:49:31 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000014a6a705d199e5a1\"","Subject":"Re: [libcamera-devel] [PATCH v3 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":21220,"web_url":"https://patchwork.libcamera.org/comment/21220/","msgid":"<163783427929.3059017.13757621402257965719@Monstersaurus>","date":"2021-11-25T09:57:59","subject":"Re: [libcamera-devel] [PATCH v3 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-25 09:49:31)\n> Hi Laurent,\n> \n> Thank you for your feedback.\n> \n> On Wed, 24 Nov 2021 at 23:16, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n> \n> > Hi Naush,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Nov 23, 2021 at 01:10:40PM +0000, Naushir Patuck wrote:\n> > > When acquiring the media device, it is not necessary to match all entity\n> > names,\n> > > so remove it. Aditionally, we do not need to keep the MediaEntity\n> > pointers for\n> > > the Unicam and ISP devices stored within the PipelineHandlerRPi class.\n> > Instead\n> > > these can be stored locally in PipelineHandlerRPi::match().\n> > >\n> > > PipelineHandlerRPi::registerCamera() now returns an int error code\n> > instead of a\n> > > boolean for pass/fail.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 88 +++++++++++--------\n> > > 1 file changed, 52 insertions(+), 36 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 12fd38061241..c5034480820e 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 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,\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,65 @@ int PipelineHandlerRPi::queueRequestDevice(Camera\n> > *camera, Request *request)\n> > >\n> > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > {\n> > > + MediaDevice *unicamDevice, *ispDevice;\n> > > +\n> > > DeviceMatch unicam(\"unicam\");\n> > > - DeviceMatch isp(\"bcm2835-isp\");\n> > > + unicamDevice = acquireMediaDevice(enumerator, unicam);\n> >\n> > You can declare the variable here. Same for ispDevice below.\n> >\n> \n> Ack.\n> \n> \n> >\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> > > + 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\n> > *isp)\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-image\");\n> > > + MediaEntity *ispOutput0 =\n> > isp->getEntityByName(\"bcm2835-isp0-output0\");\n> > > + MediaEntity *ispCapture1 =\n> > isp->getEntityByName(\"bcm2835-isp0-capture1\");\n> > > + MediaEntity *ispCapture2 =\n> > isp->getEntityByName(\"bcm2835-isp0-capture2\");\n> > > + MediaEntity *ispCapture3 =\n> > isp->getEntityByName(\"bcm2835-isp0-capture3\");\n> > > +\n> > > + if (!unicamImage || !ispOutput0 || !ispCapture1 || !ispCapture2 ||\n> > !ispCapture3)\n> > > + return -ENOENT;\n> > >\n> > > /* Locate and open the unicam video streams. */\n> > > - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> > unicam_->getEntityByName(\"unicam-image\"));\n> > > + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> > unicamImage);\n> > >\n> > > /* An embedded data node will not be present if the sensor does\n> > not support it. */\n> > > - MediaEntity *embeddedEntity =\n> > unicam_->getEntityByName(\"unicam-embedded\");\n> > > - if (embeddedEntity) {\n> > > - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n> > Embedded\", embeddedEntity);\n> > > + MediaEntity *unicamEmbedded =\n> > unicam->getEntityByName(\"unicam-embedded\");\n> > > + if (unicamEmbedded) {\n> > > + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n> > Embedded\", unicamEmbedded);\n> > >\n> > data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> > >\n> > &RPiCameraData::unicamBufferDequeue);\n> > > }\n> > >\n> > > /* Tag the ISP input stream as an import stream. */\n> > > - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\",\n> > isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> > > - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n> > isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> > > - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n> > isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> > > - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\",\n> > isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> > > + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0,\n> > 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(),\n> > &RPiCameraData::frameStarted);\n> > > @@ -1046,7 +1059,7 @@ bool PipelineHandlerRPi::registerCamera()\n> > > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(),\n> > &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_ =\n> > std::make_unique(entity);\n> > > break;\n> > > @@ -1054,23 +1067,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\n> > CamHelper for embedded data usage!\";\n> > > sensorConfig.sensorMetadata = false;\n> > > - if (embeddedEntity)\n> > > + if (unicamEmbedded)\n> > >\n> > data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > > }\n> > >\n> > > @@ -1091,12 +1104,12 @@ bool PipelineHandlerRPi::registerCamera()\n> > >\n> > > for (auto stream : data->streams_) {\n> > > if (stream->dev()->open())\n> > > - return false;\n> > > + continue;\n> >\n> > Is this correct ? If some of the devices can't be opened, shouldn't it\n> > be a fatal error ?\n> >\n> \n\nBy 'fatal' I presume you mean - will prevent registration of this\ncamera, not 'will prevent libcamera from continuing' ?\n\nIt could be that the 'other' camera works fine, and that's what the user\nwants to use ?\n\nBut it should certainly be loud about the error in opening...\n\n> Good catch, yes it should!\n> I'll fix this up for the next version.\n> \n> Regards,\n> Naush\n> \n> \n> \n> >\n> > > }\n> > >\n> > > if\n> > (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> > > LOG(RPI, Error) << \"Unicam driver does not use the\n> > MediaController, please update your kernel!\";\n> > > - return false;\n> > > + return -EINVAL;\n> > > }\n> > >\n> > > /*\n> > > @@ -1158,7 +1171,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> > > @@ -1178,7 +1191,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> > --\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 5E7B3BF415\n\tfor ;\n\tThu, 25 Nov 2021 09:58:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93B3060233;\n\tThu, 25 Nov 2021 10:58:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A605260228\n\tfor ;\n\tThu, 25 Nov 2021 10:58:02 +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 4AB38881;\n\tThu, 25 Nov 2021 10:58: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=\"lgrjIUmn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637834282;\n\tbh=UYXH083vIcUw66RITcWO4FBc494vC6KXM0fXihiyx+I=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=lgrjIUmnMijIOBawpelYQKGfrizNllDYVtXRm5KUxExoWPNkxNGDaMa6G/hWeBVIs\n\t9yU1kgVz1PnkjLr85PM2fWjQKFC8BXbeXFIwSPZNJA8xshqAUmmxaMwLCbx/uTIRgv\n\t7iB6Z/LW+hW7tsnpoiLN7Ea7ijxsLYnU/oPbe4WU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211123131040.1542581-1-naush@raspberrypi.com>\n\t<20211123131040.1542581-3-naush@raspberrypi.com>\n\t\n\t","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tNaushir Patuck ","Date":"Thu, 25 Nov 2021 09:57:59 +0000","Message-ID":"<163783427929.3059017.13757621402257965719@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 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":21221,"web_url":"https://patchwork.libcamera.org/comment/21221/","msgid":"","date":"2021-11-25T10:02:30","subject":"Re: [libcamera-devel] [PATCH v3 2/2] pipeline: raspberrypi: Tidy\n\tthe camera enumeration and registration logic","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Thu, 25 Nov 2021 at 09:58, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Naushir Patuck (2021-11-25 09:49:31)\n> > Hi Laurent,\n> >\n> > Thank you for your feedback.\n> >\n> > On Wed, 24 Nov 2021 at 23:16, Laurent Pinchart <\n> > laurent.pinchart@ideasonboard.com> wrote:\n> >\n> > > Hi Naush,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Tue, Nov 23, 2021 at 01:10:40PM +0000, Naushir Patuck wrote:\n> > > > When acquiring the media device, it is not necessary to match all\n> entity\n> > > names,\n> > > > so remove it. Aditionally, we do not need to keep the MediaEntity\n> > > pointers for\n> > > > the Unicam and ISP devices stored within the PipelineHandlerRPi\n> class.\n> > > Instead\n> > > > these can be stored locally in PipelineHandlerRPi::match().\n> > > >\n> > > > PipelineHandlerRPi::registerCamera() now returns an int error code\n> > > instead of a\n> > > > boolean for pass/fail.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 88\n> +++++++++++--------\n> > > > 1 file changed, 52 insertions(+), 36 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 12fd38061241..c5034480820e 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 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,\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,65 @@ int\n> PipelineHandlerRPi::queueRequestDevice(Camera\n> > > *camera, Request *request)\n> > > >\n> > > > bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > {\n> > > > + MediaDevice *unicamDevice, *ispDevice;\n> > > > +\n> > > > DeviceMatch unicam(\"unicam\");\n> > > > - DeviceMatch isp(\"bcm2835-isp\");\n> > > > + unicamDevice = acquireMediaDevice(enumerator, unicam);\n> > >\n> > > You can declare the variable here. Same for ispDevice below.\n> > >\n> >\n> > Ack.\n> >\n> >\n> > >\n> > > >\n> > > > - unicam.add(\"unicam-image\");\n> > > > + if (!unicamDevice) {\n> > > > + LOG(RPI, Debug) << \"Unable to acquire a Unicam\n> 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> > > > + 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: \" <<\n> ret;\n> > > > return false;\n> > > > + }\n> > > >\n> > > > - return registerCamera();\n> > > > + return true;\n> > > > }\n> > > >\n> > > > -bool PipelineHandlerRPi::registerCamera()\n> > > > +int PipelineHandlerRPi::registerCamera(MediaDevice *unicam,\n> MediaDevice\n> > > *isp)\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-image\");\n> > > > + MediaEntity *ispOutput0 =\n> > > isp->getEntityByName(\"bcm2835-isp0-output0\");\n> > > > + MediaEntity *ispCapture1 =\n> > > isp->getEntityByName(\"bcm2835-isp0-capture1\");\n> > > > + MediaEntity *ispCapture2 =\n> > > isp->getEntityByName(\"bcm2835-isp0-capture2\");\n> > > > + MediaEntity *ispCapture3 =\n> > > isp->getEntityByName(\"bcm2835-isp0-capture3\");\n> > > > +\n> > > > + if (!unicamImage || !ispOutput0 || !ispCapture1 ||\n> !ispCapture2 ||\n> > > !ispCapture3)\n> > > > + return -ENOENT;\n> > > >\n> > > > /* Locate and open the unicam video streams. */\n> > > > - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> > > unicam_->getEntityByName(\"unicam-image\"));\n> > > > + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> > > unicamImage);\n> > > >\n> > > > /* An embedded data node will not be present if the sensor does\n> > > not support it. */\n> > > > - MediaEntity *embeddedEntity =\n> > > unicam_->getEntityByName(\"unicam-embedded\");\n> > > > - if (embeddedEntity) {\n> > > > - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n> > > Embedded\", embeddedEntity);\n> > > > + MediaEntity *unicamEmbedded =\n> > > unicam->getEntityByName(\"unicam-embedded\");\n> > > > + if (unicamEmbedded) {\n> > > > + data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n> > > Embedded\", unicamEmbedded);\n> > > >\n> > > data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n> > > >\n> > > &RPiCameraData::unicamBufferDequeue);\n> > > > }\n> > > >\n> > > > /* Tag the ISP input stream as an import stream. */\n> > > > - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\",\n> > > isp_->getEntityByName(\"bcm2835-isp0-output0\"), true);\n> > > > - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n> > > isp_->getEntityByName(\"bcm2835-isp0-capture1\"));\n> > > > - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n> > > isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> > > > - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\",\n> > > isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> > > > + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0,\n> > > true);\n> > > > + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\",\n> ispCapture1);\n> > > > + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n> ispCapture2);\n> > > > + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> > > >\n> > > > /* Wire up all the buffer connections. */\n> > > >\n> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(),\n> > > &RPiCameraData::frameStarted);\n> > > > @@ -1046,7 +1059,7 @@ bool PipelineHandlerRPi::registerCamera()\n> > > > data->isp_[Isp::Stats].dev()->bufferReady.connect(data.get(),\n> > > &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_ =\n> > > std::make_unique(entity);\n> > > > break;\n> > > > @@ -1054,23 +1067,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\n> 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\n> > > CamHelper for embedded data usage!\";\n> > > > sensorConfig.sensorMetadata = false;\n> > > > - if (embeddedEntity)\n> > > > + if (unicamEmbedded)\n> > > >\n> > > data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > > > }\n> > > >\n> > > > @@ -1091,12 +1104,12 @@ bool PipelineHandlerRPi::registerCamera()\n> > > >\n> > > > for (auto stream : data->streams_) {\n> > > > if (stream->dev()->open())\n> > > > - return false;\n> > > > + continue;\n> > >\n> > > Is this correct ? If some of the devices can't be opened, shouldn't it\n> > > be a fatal error ?\n> > >\n> >\n>\n> By 'fatal' I presume you mean - will prevent registration of this\n> camera, not 'will prevent libcamera from continuing' ?\n>\n> It could be that the 'other' camera works fine, and that's what the user\n> wants to use ?\n>\n> But it should certainly be loud about the error in opening...\n>\n\nAn open() failure here ought to return early and cause the registration to\nfail. But other cameras might work fine, so it's not \"fatal\" in that\nrespect.\n\n\n\n>\n> > Good catch, yes it should!\n> > I'll fix this up for the next version.\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > >\n> > > > }\n> > > >\n> > > > if\n> > > (!data->unicam_[Unicam::Image].dev()->caps().hasMediaController()) {\n> > > > LOG(RPI, Error) << \"Unicam driver does not use the\n> > > MediaController, please update your kernel!\";\n> > > > - return false;\n> > > > + return -EINVAL;\n> > > > }\n> > > >\n> > > > /*\n> > > > @@ -1158,7 +1171,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> > > > @@ -1178,7 +1191,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> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\n> > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7E251BDB13\n\tfor ;\n\tThu, 25 Nov 2021 10:02:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D42C460376;\n\tThu, 25 Nov 2021 11:02:47 +0100 (CET)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 37DD360228\n\tfor ;\n\tThu, 25 Nov 2021 11:02:47 +0100 (CET)","by mail-lf1-x12e.google.com with SMTP id l22so14922779lfg.7\n\tfor ;\n\tThu, 25 Nov 2021 02:02:47 -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=\"Apnr7JgK\"; 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=xV2e4N+U2yfQLKaGBIqHTTUA8/yJLi0bfj/k/hhfMeE=;\n\tb=Apnr7JgKjedQ2xX39o1j4EWVeCpJtTKK+/6koVL2xEUyQY24Hj1TWK6015oKRL/Cax\n\tOSUZDRbOVrP9SaIFDAhB3TdQk/l5nUPu1SbGDLuNcpNItZuHUL+Yc5sHz1kyyVtdVTk9\n\t2XCR5qSYD6Dc/e0piG9Bo4Vig+9EiiD8QLsy7UtIvYOmdgU37kV8xKZZNF6MkECJFZ6t\n\tKB2bvCchCTQpBSoYU60YxB5XR6sM6jbnKNnAWjlWX9tGA7CnPDwVh2cwfcHonknhND8I\n\tScpvRQGBfSAxCncZkYvOfAqBaAFlYSoWq2fBJDW2leHkJebAm6g4gdNJ20x/OEV4coDh\n\tNDww==","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=xV2e4N+U2yfQLKaGBIqHTTUA8/yJLi0bfj/k/hhfMeE=;\n\tb=OHFK/sMdrDdutFP00lRTIWBVjMEW+CEHeKQWPnJ4yBSHxpRlcAg0HTQ1PTC0hdErQi\n\t0xnzNmXso+abzTRBHjeQsaaPJKZJjeWO7XIQy9W0F3fUiQ73fOMbiLWDZlgF0xV+LJjV\n\tFp/bGlNYoltRoO14PnoZI+TOaoeWxrPjtbEZY0IB20iUcZx0VAtK/xGESNyT34I8bih5\n\tTGm5CGVk9IIq1Mw9QdkX3CAX5b/r9+uwlNhO3/mMUHXbkz3WCL6gjFXLgL1ApmoCLOyU\n\tOZbgLe18MirlCC/eTIn62VHAvcf+taMyzOXlNCoYVdeQb+jkDansImxC1ZP30KGnHmjs\n\tLysg==","X-Gm-Message-State":"AOAM532ib3nEcHldu6UMFZxxyre7hgKImAcOU7koUQXw3HQMsJI8RTjx\n\tMtbhybDr+v4WoS+WAwx4X39lkhn3yNQYEW4aQSFQBA==","X-Google-Smtp-Source":"ABdhPJxMw5/aAo4Cc/jDllELUJmZUy0wdr2+rU7AvHjX1coBX6Oy+CMyeQ7xbvDP5cp2Ikdaj0AOYthUYOpV+GgNdUo=","X-Received":"by 2002:a05:6512:150f:: with SMTP id\n\tbq15mr23284562lfb.687.1637834566372; \n\tThu, 25 Nov 2021 02:02:46 -0800 (PST)","MIME-Version":"1.0","References":"<20211123131040.1542581-1-naush@raspberrypi.com>\n\t<20211123131040.1542581-3-naush@raspberrypi.com>\n\t\n\t\n\t<163783427929.3059017.13757621402257965719@Monstersaurus>","In-Reply-To":"<163783427929.3059017.13757621402257965719@Monstersaurus>","From":"Naushir Patuck ","Date":"Thu, 25 Nov 2021 10:02:30 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"00000000000087a08b05d19a1303\"","Subject":"Re: [libcamera-devel] [PATCH v3 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\" "}}]