[{"id":15464,"web_url":"https://patchwork.libcamera.org/comment/15464/","msgid":"","date":"2021-03-04T01:26:02","subject":"Re: [libcamera-devel] [PATCH v4 3/4] pipeline: raspberrypi: Only\n\tenable embedded stream when available","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, Mar 02, 2021 at 03:11:10PM +0000, Naushir Patuck wrote:\n> The pipeline handler would enable and use the Unicam embedded data stream\n> even if the sensor did not support it. This was to allow a means to\n> pass exposure and gain values for the frame to the IPA in a synchronised\n> way.\n> \n> The recent changes to get the pipeline handler to pass a ControlList\n> with exposure and gain values means this is no longer required. Disable\n> the use of the embedded data stream when a sensor does not support it.\n> \n> This change also removes the mappedEmbeddedBuffers_ map as it is no\n> longer used.\n> \n> Signed-off-by: Naushir Patuck \n> Tested-by: David Plowman \n\nReviewed-by: Laurent Pinchart \n\nWith the assumption you'll move this to init() time once mojo will\nsupport it :-)\n\n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 114 +++++++++++-------\n> 1 file changed, 72 insertions(+), 42 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index d057241b9c76..5ae2551957f8 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -177,12 +177,6 @@ public:\n> \t/* Stores the ids of the buffers mapped in the IPA. */\n> \tstd::unordered_set ipaBuffers_;\n> \n> -\t/*\n> -\t * Map of (internal) mmaped embedded data buffers, to avoid having to\n> -\t * map/unmap on every frame.\n> -\t */\n> -\tstd::map mappedEmbeddedBuffers_;\n> -\n> \t/* DMAHEAP allocation helper. */\n> \tRPi::DmaHeap dmaHeap_;\n> \tFileDescriptor lsTable_;\n> @@ -636,14 +630,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \n> \t\tif (isRaw(cfg.pixelFormat)) {\n> \t\t\tcfg.setStream(&data->unicam_[Unicam::Image]);\n> -\t\t\t/*\n> -\t\t\t * We must set both Unicam streams as external, even\n> -\t\t\t * though the application may only request RAW frames.\n> -\t\t\t * This is because we match timestamps on both streams\n> -\t\t\t * to synchronise buffers.\n> -\t\t\t */\n> \t\t\tdata->unicam_[Unicam::Image].setExternal(true);\n> -\t\t\tdata->unicam_[Unicam::Embedded].setExternal(true);\n> \t\t\tcontinue;\n> \t\t}\n> \n> @@ -715,17 +702,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\treturn ret;\n> \t}\n> \n> -\t/* Unicam embedded data output format. */\n> -\tformat = {};\n> -\tformat.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);\n> -\tLOG(RPI, Debug) << \"Setting embedded data format.\";\n> -\tret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format);\n> -\tif (ret) {\n> -\t\tLOG(RPI, Error) << \"Failed to set format on Unicam embedded: \"\n> -\t\t\t\t<< format.toString();\n> -\t\treturn ret;\n> -\t}\n> -\n> \t/* Figure out the smallest selection the ISP will allow. */\n> \tRectangle testCrop(0, 0, 1, 1);\n> \tdata->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);\n> @@ -742,6 +718,41 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \tif (ret)\n> \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> \n> +\t/*\n> +\t * The IPA will set data->sensorMetadata_ to true if embedded data is\n> +\t * supported on this sensor. If so, open the Unicam embedded data\n> +\t * node and configure the output format.\n> +\t */\n> +\tif (data->sensorMetadata_) {\n> +\t\tformat = {};\n> +\t\tformat.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);\n> +\t\tLOG(RPI, Debug) << \"Setting embedded data format.\";\n> +\t\tdata->unicam_[Unicam::Embedded].dev()->open();\n> +\t\tret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format);\n> +\t\tif (ret) {\n> +\t\t\tLOG(RPI, Error) << \"Failed to set format on Unicam embedded: \"\n> +\t\t\t\t\t<< format.toString();\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * If a RAW/Bayer stream has been requested by the application,\n> +\t\t * we must set both Unicam streams as external, even though the\n> +\t\t * application may only request RAW frames. This is because we\n> +\t\t * match timestamps on both streams to synchronise buffers.\n> +\t\t */\n> +\t\tif (rawStream)\n> +\t\t\tdata->unicam_[Unicam::Embedded].setExternal(true);\n> +\t} else {\n> +\t\t/*\n> +\t\t * No embedded data present, so we do not want to iterate over\n> +\t\t * the embedded data stream when starting and stopping.\n> +\t\t */\n> +\t\tdata->streams_.erase(std::remove(data->streams_.begin(), data->streams_.end(),\n> +\t\t\t\t\t\t &data->unicam_[Unicam::Embedded]),\n> +\t\t\t\t data->streams_.end());\n> +\t}\n> +\n> \t/*\n> \t * Update the ScalerCropMaximum to the correct value for this camera mode.\n> \t * For us, it's the same as the \"analogue crop\".\n> @@ -949,10 +960,16 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> \tfor (auto &stream : data->isp_)\n> \t\tdata->streams_.push_back(&stream);\n> \n> -\t/* Open all Unicam and ISP streams. */\n> +\t/*\n> +\t * Open all Unicam and ISP streams. The exception is the embedded data\n> +\t * stream, which only gets opened if the IPA reports that the sensor\n> +\t * supports embedded data. This happens in RPiCameraData::configureIPA().\n> +\t */\n> \tfor (auto const stream : data->streams_) {\n> -\t\tif (stream->dev()->open())\n> -\t\t\treturn false;\n> +\t\tif (stream != &data->unicam_[Unicam::Embedded]) {\n> +\t\t\tif (stream->dev()->open())\n> +\t\t\t\treturn false;\n> +\t\t}\n> \t}\n> \n> \t/* Wire up all the buffer connections. */\n> @@ -1109,19 +1126,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \t\t\treturn ret;\n> \t}\n> \n> -\tif (!data->sensorMetadata_) {\n> -\t\tfor (auto const &it : data->unicam_[Unicam::Embedded].getBuffers()) {\n> -\t\t\tMappedFrameBuffer fb(it.second, PROT_READ | PROT_WRITE);\n> -\t\t\tdata->mappedEmbeddedBuffers_.emplace(it.first, std::move(fb));\n> -\t\t}\n> -\t}\n> -\n> \t/*\n> \t * Pass the stats and embedded data buffers to the IPA. No other\n> \t * buffers need to be passed.\n> \t */\n> \tmapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), ipa::RPi::MaskStats);\n> -\tmapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), ipa::RPi::MaskEmbeddedData);\n> +\tif (data->sensorMetadata_)\n> +\t\tmapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(),\n> +\t\t\t ipa::RPi::MaskEmbeddedData);\n> \n> \treturn 0;\n> }\n> @@ -1154,7 +1166,6 @@ void PipelineHandlerRPi::freeBuffers(Camera *camera)\n> \tstd::vector ipaBuffers(data->ipaBuffers_.begin(), data->ipaBuffers_.end());\n> \tdata->ipa_->unmapBuffers(ipaBuffers);\n> \tdata->ipaBuffers_.clear();\n> -\tdata->mappedEmbeddedBuffers_.clear();\n> \n> \tfor (auto const stream : data->streams_)\n> \t\tstream->releaseBuffers();\n> @@ -1652,7 +1663,7 @@ void RPiCameraData::tryRunPipeline()\n> \n> \t/* If any of our request or buffer queues are empty, we cannot proceed. */\n> \tif (state_ != State::Idle || requestQueue_.empty() ||\n> -\t bayerQueue_.empty() || embeddedQueue_.empty())\n> +\t bayerQueue_.empty() || (embeddedQueue_.empty() && sensorMetadata_))\n> \t\treturn;\n> \n> \tif (!findMatchingBuffers(bayerFrame, embeddedBuffer))\n> @@ -1675,17 +1686,24 @@ void RPiCameraData::tryRunPipeline()\n> \tstate_ = State::Busy;\n> \n> \tunsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);\n> -\tunsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\n> \n> \tLOG(RPI, Debug) << \"Signalling signalIspPrepare:\"\n> -\t\t\t<< \" Bayer buffer id: \" << bayerId\n> -\t\t\t<< \" Embedded buffer id: \" << embeddedId;\n> +\t\t\t<< \" Bayer buffer id: \" << bayerId;\n> \n> \tipa::RPi::ISPConfig ispPrepare;\n> -\tispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData | embeddedId;\n> \tispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId;\n> -\tispPrepare.embeddedBufferPresent = sensorMetadata_;\n> \tispPrepare.controls = std::move(bayerFrame.controls);\n> +\n> +\tif (embeddedBuffer) {\n> +\t\tunsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\n> +\n> +\t\tispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData | embeddedId;\n> +\t\tispPrepare.embeddedBufferPresent = true;\n> +\n> +\t\tLOG(RPI, Debug) << \"Signalling signalIspPrepare:\"\n> +\t\t\t\t<< \" Bayer buffer id: \" << embeddedId;\n> +\t}\n> +\n> \tipa_->signalIspPrepare(ispPrepare);\n> }\n> \n> @@ -1727,6 +1745,18 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em\n> \n> \t\t\tLOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n> \n> +\t\t\tif (!sensorMetadata_) {\n> +\t\t\t\t/*\n> +\t\t\t\t * If there is no sensor metadata, simply return the\n> +\t\t\t\t * first bayer frame in the queue.\n> +\t\t\t\t */\n> +\t\t\t\tLOG(RPI, Debug) << \"Returning bayer frame without a match\";\n> +\t\t\t\tbayerFrame = std::move(bayerQueue_.front());\n> +\t\t\t\tbayerQueue_.pop();\n> +\t\t\t\tembeddedBuffer = nullptr;\n> +\t\t\t\treturn true;\n> +\t\t\t}\n> +\n> \t\t\tif (!embeddedQueue_.empty()) {\n> \t\t\t\t/*\n> \t\t\t\t * Not found a matching embedded buffer for the bayer buffer in","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 5FB43BD80C\n\tfor ;\n\tThu, 4 Mar 2021 01:26:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D530068A92;\n\tThu, 4 Mar 2021 02:26:32 +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 E192760106\n\tfor ;\n\tThu, 4 Mar 2021 02:26:31 +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 5462427A;\n\tThu, 4 Mar 2021 02:26:31 +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=\"qL0LAMRm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614821191;\n\tbh=MA5juVdi+8xfMsZ3dZi8HzV89SG8NxFkLe0zdHc6GU8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qL0LAMRmDIEzjO0PAIv/jKLSbHh8JbZjYQB2ZKAtB1qWHd/YLsMUFnkXzx22SDq7F\n\t79Py6aMy6k++wQ4LeVFmra9qcHy0DtwFXGjG/tfSZYJGJFglETi0BkWtURDlY9VHrJ\n\t/HwD+kWYLHku4pY+8wPiO4VFRyFqAKoiPgKEw4eA=","Date":"Thu, 4 Mar 2021 03:26:02 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210302151111.212591-1-naush@raspberrypi.com>\n\t<20210302151111.212591-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210302151111.212591-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 3/4] pipeline: raspberrypi: Only\n\tenable embedded stream when available","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]