[{"id":14981,"web_url":"https://patchwork.libcamera.org/comment/14981/","msgid":"","date":"2021-02-04T18:54:41","subject":"Re: [libcamera-devel] [PATCH v3 11/11] libcamera: ipu3: Share\n\tparameter and statistic buffers with IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Thu, Feb 04, 2021 at 05:29:43PM +0100, Niklas Söderlund wrote:\n> Use the IPU3Frames helper to share parameters and statistics buffers\n> with the IPA. Track which parameter and statistic buffer is used for\n> which request and make sure the parameter buffers is filled in by the\n> IPA before it's needed and that the statistic buffer is consumed and\n> meta data generated before completing the request.\n> \n> With this change the IPU3 pipeline is prepared to fully operate with an\n> IPA component.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> * Changes since v1\n> - Update commit message.\n> - s/frameInfo_/frameInfos_/\n> - Refactor away isComplete variable.\n> \n> * Changes since v2\n> - Emedd the IPU3Frames instance instead of allocating it.\n> - Use -ENOBUFS instead of _ENOENT in queueRequestDevice().\n> - Keep isComplete in cio2BufferReady()\n> - Rework the queue logic.\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 149 ++++++++++++++++++++-------\n> 1 file changed, 112 insertions(+), 37 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 092db6389833a481..b79db25050d2c1d6 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -29,6 +29,7 @@\n> #include \"libcamera/internal/v4l2_controls.h\"\n> \n> #include \"cio2.h\"\n> +#include \"frames.h\"\n> #include \"imgu.h\"\n> \n> namespace libcamera {\n> @@ -61,6 +62,8 @@ public:\n> \n> \tvoid imguOutputBufferReady(FrameBuffer *buffer);\n> \tvoid cio2BufferReady(FrameBuffer *buffer);\n> +\tvoid paramBufferReady(FrameBuffer *buffer);\n> +\tvoid statBufferReady(FrameBuffer *buffer);\n> \n> \tCIO2Device cio2_;\n> \tImgUDevice *imgu_;\n> @@ -71,6 +74,7 @@ public:\n> \n> \tuint32_t exposureTime_;\n> \tstd::unique_ptr delayedCtrls_;\n> +\tIPU3Frames frameInfos_;\n> \n> private:\n> \tvoid queueFrameAction(unsigned int id, const IPAOperationData &op);\n> @@ -609,6 +613,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> \n> \tdata->ipa_->mapBuffers(ipaBuffers_);\n> \n> +\tdata->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);\n> +\n> \treturn 0;\n> }\n> \n> @@ -616,6 +622,8 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> \n> +\tdata->frameInfos_.clear();\n> +\n> \tstd::vector ids;\n> \tfor (IPABuffer &ipabuf : ipaBuffers_)\n> \t\tids.push_back(ipabuf.id);\n> @@ -713,7 +721,10 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tint error = 0;\n> +\n> +\tIPU3Frames::Info *info = data->frameInfos_.create(request);\n> +\tif (!info)\n> +\t\treturn -ENOBUFS;\n> \n> \t/*\n> \t * Queue a buffer on the CIO2, using the raw stream buffer provided in\n> @@ -724,24 +735,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> \tif (!rawBuffer)\n> \t\treturn -ENOMEM;\n\nWhat will happen to info in this case ? As there will be no raw buffer\nassociated with it, won't it linger forever in a list ?\n\n> \n> -\t/* Queue all buffers from the request aimed for the ImgU. */\n> -\tfor (auto it : request->buffers()) {\n> -\t\tconst Stream *stream = it.first;\n> -\t\tFrameBuffer *buffer = it.second;\n> -\t\tint ret;\n> +\tinfo->rawBuffer = rawBuffer;\n> \n> -\t\tif (stream == &data->outStream_)\n> -\t\t\tret = data->imgu_->output_->queueBuffer(buffer);\n> -\t\telse if (stream == &data->vfStream_)\n> -\t\t\tret = data->imgu_->viewfinder_->queueBuffer(buffer);\n> -\t\telse\n> -\t\t\tcontinue;\n> -\n> -\t\tif (ret < 0)\n> -\t\t\terror = ret;\n> -\t}\n> -\n> -\treturn error;\n> +\treturn 0;\n> }\n> \n> bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> @@ -1007,6 +1003,10 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> \t\tdata->imgu_->viewfinder_->bufferReady.connect(data.get(),\n> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> +\t\tdata->imgu_->param_->bufferReady.connect(data.get(),\n> +\t\t\t\t\t&IPU3CameraData::paramBufferReady);\n> +\t\tdata->imgu_->stat_->bufferReady.connect(data.get(),\n> +\t\t\t\t\t&IPU3CameraData::statBufferReady);\n> \n> \t\t/* Create and register the Camera instance. */\n> \t\tstd::string cameraId = cio2->sensor()->id();\n> @@ -1039,7 +1039,7 @@ int IPU3CameraData::loadIPA()\n> \treturn 0;\n> }\n> \n> -void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,\n> +void IPU3CameraData::queueFrameAction(unsigned int id,\n> \t\t\t\t const IPAOperationData &action)\n> {\n> \tswitch (action.operation) {\n> @@ -1048,6 +1048,41 @@ void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,\n> \t\tdelayedCtrls_->push(controls);\n> \t\tbreak;\n> \t}\n> +\tcase IPU3_IPA_ACTION_PARAM_FILLED: {\n> +\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n> +\t\tif (!info)\n> +\t\t\tbreak;\n\nShould we warn in this case ? Maybe in IPU3Frames::info() itself to\navoid duplicating log calls ? I'm tempted to even assert.\n\n> +\n> +\t\t/* Queue all buffers from the request aimed for the ImgU. */\n> +\t\tfor (auto it : info->request->buffers()) {\n> +\t\t\tconst Stream *stream = it.first;\n> +\t\t\tFrameBuffer *outbuffer = it.second;\n> +\n> +\t\t\tif (stream == &outStream_)\n> +\t\t\t\timgu_->output_->queueBuffer(outbuffer);\n> +\t\t\telse if (stream == &vfStream_)\n> +\t\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n> +\t\t}\n> +\n> +\t\timgu_->param_->queueBuffer(info->paramBuffer);\n> +\t\timgu_->stat_->queueBuffer(info->statBuffer);\n> +\t\timgu_->input_->queueBuffer(info->rawBuffer);\n> +\n> +\t\tbreak;\n> +\t}\n> +\tcase IPU3_IPA_ACTION_METADATA_READY: {\n> +\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n> +\t\tif (!info)\n> +\t\t\tbreak;\n> +\n> +\t\tRequest *request = info->request;\n> +\t\trequest->metadata() = action.controls[0];\n> +\t\tinfo->metadataProcessed = true;\n> +\t\tif (frameInfos_.tryComplete(info))\n> +\t\t\tpipe_->completeRequest(request);\n\nMaybe we could pass the request to IPU3Frames, and handle completion\nautomatically internally, but that's for later.\n\n> +\n> +\t\tbreak;\n> +\t}\n> \tdefault:\n> \t\tLOG(IPU3, Error) << \"Unknown action \" << action.operation;\n> \t\tbreak;\n> @@ -1068,11 +1103,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> {\n> \tRequest *request = buffer->request();\n> \n> -\tif (!pipe_->completeBuffer(request, buffer))\n> -\t\t/* Request not completed yet, return here. */\n> +\tpipe_->completeBuffer(request, buffer);\n> +\n> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> +\tif (!info)\n> \t\treturn;\n> \n> -\t/* Mark the request as complete. */\n> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n> \t/* \\todo Move the ExposureTime control to the IPA. */\n> \trequest->metadata().set(controls::ExposureTime, exposureTime_);\n> @@ -1081,7 +1117,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> \t\tRectangle cropRegion = request->controls().get(controls::ScalerCrop);\n> \t\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n> \t}\n> -\tpipe_->completeRequest(request);\n> +\n> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> +\t\tinfo->metadataProcessed = true;\n> +\n> +\tif (frameInfos_.tryComplete(info))\n> +\t\tpipe_->completeRequest(request);\n> }\n> \n> /**\n> @@ -1093,26 +1134,60 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> */\n> void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> {\n> -\t/* \\todo Handle buffer failures when state is set to BufferError. */\n> -\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> +\tif (!info)\n> \t\treturn;\n> \n> \tRequest *request = buffer->request();\n> \n> -\t/*\n> -\t * If the request contains a buffer for the RAW stream only, complete it\n> -\t * now as there's no need for ImgU processing.\n> -\t */\n> -\tif (request->findBuffer(&rawStream_)) {\n> -\t\tbool isComplete = pipe_->completeBuffer(request, buffer);\n> -\t\tif (isComplete) {\n> -\t\t\trequest->metadata().set(controls::draft::PipelineDepth, 2);\n> -\t\t\tpipe_->completeRequest(request);\n> -\t\t\treturn;\n> -\t\t}\n> +\tif (request->findBuffer(&rawStream_))\n> +\t\tpipe_->completeBuffer(request, buffer);\n\nIf you moved this after the next if (...) { } block, you could...\n\n> +\n> +\t/* If the buffer is cancelled force a complete of the whole request. */\n> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> +\t\tfor (auto it : request->buffers())\n> +\t\t\tif (it.second != buffer)\n\n... drop this check.\n\n> +\t\t\t\tpipe_->completeBuffer(request, it.second);\n> +\n> +\t\tinfo->paramDequeued = true;\n> +\t\tinfo->metadataProcessed = true;\n> +\t\tASSERT(frameInfos_.tryComplete(info));\n> +\t\tpipe_->completeRequest(request);\n> +\n> +\t\treturn;\n> \t}\n> \n> -\timgu_->input_->queueBuffer(buffer);\n> +\tIPAOperationData op;\n> +\top.operation = IPU3_IPA_EVENT_FILL_PARAMS;\n> +\top.data = { info->id, info->paramBuffer->cookie() };\n> +\top.controls = { request->controls() };\n\nThis is the right time to call IPU3_IPA_EVENT_FILL_PARAMS, but the\nrequest controls need to be passed to the IPA earlier, right when the\nrequest is queued, as the IPA needs to look ahead in the request queue\nto compute parameters.\n\nThis is the main issue in this patch, the rest is fairly minor.\n\n> +\tipa_->processEvent(op);\n> +}\n> +\n> +void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> +{\n> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> +\tif (!info)\n> +\t\treturn;\n> +\n> +\tinfo->paramDequeued = true;\n> +\tif (frameInfos_.tryComplete(info))\n> +\t\tpipe_->completeRequest(buffer->request());\n> +}\n> +\n> +void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> +{\n> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> +\t\treturn;\n\nWhy do we need to check for the status for stats buffers but not for\nparams buffers ?\n\n> +\n> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> +\tif (!info)\n> +\t\treturn;\n> +\n> +\tIPAOperationData op;\n> +\top.operation = IPU3_IPA_EVENT_STAT_READY;\n> +\top.data = { info->id, info->statBuffer->cookie() };\n> +\tipa_->processEvent(op);\n> }\n> \n> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)","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 89A01BD162\n\tfor ;\n\tThu, 4 Feb 2021 18:55:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F044B6145B;\n\tThu, 4 Feb 2021 19:55:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BD87561430\n\tfor ;\n\tThu, 4 Feb 2021 19:55:04 +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 133D3510;\n\tThu, 4 Feb 2021 19:55:04 +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=\"ufX7DFS8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612464904;\n\tbh=nJHh8CXdHQgl83Y154MDKSGSB8oamGsDGcVcdskfPx4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ufX7DFS8GYd1wg61FY6XW6rxGphEdeUhfORsLoJWrf/V3IoMa5FZmEyVVfkyEWfbe\n\tGlB4U5bwZmBRi/L+c8hIp9VLKwHsN9upofWvkzjgEXn8niRjAy+vycljXvdZS3aQ/5\n\tvNbUNxeaeBMdgyFIpzX4Lbzk5EuHpB5nhH6LUh84=","Date":"Thu, 4 Feb 2021 20:54:41 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210204162943.268517-1-niklas.soderlund@ragnatech.se>\n\t<20210204162943.268517-12-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210204162943.268517-12-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 11/11] libcamera: ipu3: Share\n\tparameter and statistic buffers with IPA","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14994,"web_url":"https://patchwork.libcamera.org/comment/14994/","msgid":"<965096f3-d477-5324-e820-8092e4c40d5d@ideasonboard.com>","date":"2021-02-04T21:46:45","subject":"Re: [libcamera-devel] [PATCH v3 11/11] libcamera: ipu3: Share\n\tparameter and statistic buffers with IPA","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 04/02/2021 18:54, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Thu, Feb 04, 2021 at 05:29:43PM +0100, Niklas Söderlund wrote:\n>> Use the IPU3Frames helper to share parameters and statistics buffers\n>> with the IPA. Track which parameter and statistic buffer is used for\n>> which request and make sure the parameter buffers is filled in by the\n>> IPA before it's needed and that the statistic buffer is consumed and\n>> meta data generated before completing the request.\n>>\n>> With this change the IPU3 pipeline is prepared to fully operate with an\n>> IPA component.\n>>\n>> Signed-off-by: Niklas Söderlund \n>> ---\n>> * Changes since v1\n>> - Update commit message.\n>> - s/frameInfo_/frameInfos_/\n>> - Refactor away isComplete variable.\n>>\n>> * Changes since v2\n>> - Emedd the IPU3Frames instance instead of allocating it.\n>> - Use -ENOBUFS instead of _ENOENT in queueRequestDevice().\n>> - Keep isComplete in cio2BufferReady()\n>> - Rework the queue logic.\n>> ---\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 149 ++++++++++++++++++++-------\n>> 1 file changed, 112 insertions(+), 37 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 092db6389833a481..b79db25050d2c1d6 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -29,6 +29,7 @@\n>> #include \"libcamera/internal/v4l2_controls.h\"\n>> \n>> #include \"cio2.h\"\n>> +#include \"frames.h\"\n>> #include \"imgu.h\"\n>> \n>> namespace libcamera {\n>> @@ -61,6 +62,8 @@ public:\n>> \n>> \tvoid imguOutputBufferReady(FrameBuffer *buffer);\n>> \tvoid cio2BufferReady(FrameBuffer *buffer);\n>> +\tvoid paramBufferReady(FrameBuffer *buffer);\n>> +\tvoid statBufferReady(FrameBuffer *buffer);\n>> \n>> \tCIO2Device cio2_;\n>> \tImgUDevice *imgu_;\n>> @@ -71,6 +74,7 @@ public:\n>> \n>> \tuint32_t exposureTime_;\n>> \tstd::unique_ptr delayedCtrls_;\n>> +\tIPU3Frames frameInfos_;\n>> \n>> private:\n>> \tvoid queueFrameAction(unsigned int id, const IPAOperationData &op);\n>> @@ -609,6 +613,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n>> \n>> \tdata->ipa_->mapBuffers(ipaBuffers_);\n>> \n>> +\tdata->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);\n>> +\n>> \treturn 0;\n>> }\n>> \n>> @@ -616,6 +622,8 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n>> {\n>> \tIPU3CameraData *data = cameraData(camera);\n>> \n>> +\tdata->frameInfos_.clear();\n>> +\n>> \tstd::vector ids;\n>> \tfor (IPABuffer &ipabuf : ipaBuffers_)\n>> \t\tids.push_back(ipabuf.id);\n>> @@ -713,7 +721,10 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n>> int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n>> {\n>> \tIPU3CameraData *data = cameraData(camera);\n>> -\tint error = 0;\n>> +\n>> +\tIPU3Frames::Info *info = data->frameInfos_.create(request);\n>> +\tif (!info)\n>> +\t\treturn -ENOBUFS;\n>> \n>> \t/*\n>> \t * Queue a buffer on the CIO2, using the raw stream buffer provided in\n>> @@ -724,24 +735,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n>> \tif (!rawBuffer)\n>> \t\treturn -ENOMEM;\n> \n> What will happen to info in this case ? As there will be no raw buffer\n> associated with it, won't it linger forever in a list ?\n> \n>> \n>> -\t/* Queue all buffers from the request aimed for the ImgU. */\n>> -\tfor (auto it : request->buffers()) {\n>> -\t\tconst Stream *stream = it.first;\n>> -\t\tFrameBuffer *buffer = it.second;\n>> -\t\tint ret;\n>> +\tinfo->rawBuffer = rawBuffer;\n>> \n>> -\t\tif (stream == &data->outStream_)\n>> -\t\t\tret = data->imgu_->output_->queueBuffer(buffer);\n>> -\t\telse if (stream == &data->vfStream_)\n>> -\t\t\tret = data->imgu_->viewfinder_->queueBuffer(buffer);\n>> -\t\telse\n>> -\t\t\tcontinue;\n>> -\n>> -\t\tif (ret < 0)\n>> -\t\t\terror = ret;\n>> -\t}\n>> -\n>> -\treturn error;\n>> +\treturn 0;\n>> }\n>> \n>> bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n>> @@ -1007,6 +1003,10 @@ int PipelineHandlerIPU3::registerCameras()\n>> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n>> \t\tdata->imgu_->viewfinder_->bufferReady.connect(data.get(),\n>> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n>> +\t\tdata->imgu_->param_->bufferReady.connect(data.get(),\n>> +\t\t\t\t\t&IPU3CameraData::paramBufferReady);\n>> +\t\tdata->imgu_->stat_->bufferReady.connect(data.get(),\n>> +\t\t\t\t\t&IPU3CameraData::statBufferReady);\n>> \n>> \t\t/* Create and register the Camera instance. */\n>> \t\tstd::string cameraId = cio2->sensor()->id();\n>> @@ -1039,7 +1039,7 @@ int IPU3CameraData::loadIPA()\n>> \treturn 0;\n>> }\n>> \n>> -void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,\n>> +void IPU3CameraData::queueFrameAction(unsigned int id,\n>> \t\t\t\t const IPAOperationData &action)\n>> {\n>> \tswitch (action.operation) {\n>> @@ -1048,6 +1048,41 @@ void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,\n>> \t\tdelayedCtrls_->push(controls);\n>> \t\tbreak;\n>> \t}\n>> +\tcase IPU3_IPA_ACTION_PARAM_FILLED: {\n>> +\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n>> +\t\tif (!info)\n>> +\t\t\tbreak;\n> \n> Should we warn in this case ? Maybe in IPU3Frames::info() itself to\n> avoid duplicating log calls ? I'm tempted to even assert.\n> \n>> +\n>> +\t\t/* Queue all buffers from the request aimed for the ImgU. */\n>> +\t\tfor (auto it : info->request->buffers()) {\n>> +\t\t\tconst Stream *stream = it.first;\n>> +\t\t\tFrameBuffer *outbuffer = it.second;\n>> +\n>> +\t\t\tif (stream == &outStream_)\n>> +\t\t\t\timgu_->output_->queueBuffer(outbuffer);\n>> +\t\t\telse if (stream == &vfStream_)\n>> +\t\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n>> +\t\t}\n>> +\n>> +\t\timgu_->param_->queueBuffer(info->paramBuffer);\n>> +\t\timgu_->stat_->queueBuffer(info->statBuffer);\n>> +\t\timgu_->input_->queueBuffer(info->rawBuffer);\n>> +\n>> +\t\tbreak;\n>> +\t}\n>> +\tcase IPU3_IPA_ACTION_METADATA_READY: {\n>> +\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n>> +\t\tif (!info)\n>> +\t\t\tbreak;\n>> +\n>> +\t\tRequest *request = info->request;\n>> +\t\trequest->metadata() = action.controls[0];\n>> +\t\tinfo->metadataProcessed = true;\n>> +\t\tif (frameInfos_.tryComplete(info))\n>> +\t\t\tpipe_->completeRequest(request);\n> \n> Maybe we could pass the request to IPU3Frames, and handle completion\n> automatically internally, but that's for later.\n> \n>> +\n>> +\t\tbreak;\n>> +\t}\n>> \tdefault:\n>> \t\tLOG(IPU3, Error) << \"Unknown action \" << action.operation;\n>> \t\tbreak;\n>> @@ -1068,11 +1103,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>> {\n>> \tRequest *request = buffer->request();\n>> \n>> -\tif (!pipe_->completeBuffer(request, buffer))\n>> -\t\t/* Request not completed yet, return here. */\n>> +\tpipe_->completeBuffer(request, buffer);\n>> +\n>> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n>> +\tif (!info)\n>> \t\treturn;\n>> \n>> -\t/* Mark the request as complete. */\n>> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n>> \t/* \\todo Move the ExposureTime control to the IPA. */\n>> \trequest->metadata().set(controls::ExposureTime, exposureTime_);\n>> @@ -1081,7 +1117,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>> \t\tRectangle cropRegion = request->controls().get(controls::ScalerCrop);\n>> \t\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n>> \t}\n>> -\tpipe_->completeRequest(request);\n>> +\n>> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n>> +\t\tinfo->metadataProcessed = true;\n>> +\n>> +\tif (frameInfos_.tryComplete(info))\n>> +\t\tpipe_->completeRequest(request);\n>> }\n>> \n>> /**\n>> @@ -1093,26 +1134,60 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>> */\n>> void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>> {\n>> -\t/* \\todo Handle buffer failures when state is set to BufferError. */\n>> -\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n>> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n>> +\tif (!info)\n>> \t\treturn;\n>> \n>> \tRequest *request = buffer->request();\n>> \n>> -\t/*\n>> -\t * If the request contains a buffer for the RAW stream only, complete it\n>> -\t * now as there's no need for ImgU processing.\n>> -\t */\n>> -\tif (request->findBuffer(&rawStream_)) {\n>> -\t\tbool isComplete = pipe_->completeBuffer(request, buffer);\n>> -\t\tif (isComplete) {\n>> -\t\t\trequest->metadata().set(controls::draft::PipelineDepth, 2);\n>> -\t\t\tpipe_->completeRequest(request);\n>> -\t\t\treturn;\n>> -\t\t}\n>> +\tif (request->findBuffer(&rawStream_))\n>> +\t\tpipe_->completeBuffer(request, buffer);\n> \n> If you moved this after the next if (...) { } block, you could...\n> \n>> +\n>> +\t/* If the buffer is cancelled force a complete of the whole request. */\n>> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n>> +\t\tfor (auto it : request->buffers())\n>> +\t\t\tif (it.second != buffer)\n> \n> ... drop this check.\n> \n>> +\t\t\t\tpipe_->completeBuffer(request, it.second);\n>> +\n>> +\t\tinfo->paramDequeued = true;\n>> +\t\tinfo->metadataProcessed = true;\n>> +\t\tASSERT(frameInfos_.tryComplete(info));\n>> +\t\tpipe_->completeRequest(request);\n>> +\n>> +\t\treturn;\n>> \t}\n>> \n>> -\timgu_->input_->queueBuffer(buffer);\n>> +\tIPAOperationData op;\n>> +\top.operation = IPU3_IPA_EVENT_FILL_PARAMS;\n>> +\top.data = { info->id, info->paramBuffer->cookie() };\n>> +\top.controls = { request->controls() };\n> \n> This is the right time to call IPU3_IPA_EVENT_FILL_PARAMS, but the\n> request controls need to be passed to the IPA earlier, right when the\n> request is queued, as the IPA needs to look ahead in the request queue\n> to compute parameters.\n> \n> This is the main issue in this patch, the rest is fairly minor.\n\nSounds like we need to add another call to the IPA on queueRequest time.\n\nI'd be tempted to build that on top, as we're going to have to consider\nhow to handle when to run the IPA algorithms anyway.\n\nMy worry is running the algorithms, and then only getting the\nstatsistics back $QUEUE_LENGTH 'runs' later - but presumably that will\nbe handled by things like the sequence number, and the timestamps of the\ncompletions. We'll see as we dig further I think.\n\n\n> \n>> +\tipa_->processEvent(op);\n>> +}\n>> +\n>> +void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>> +{\n>> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n>> +\tif (!info)\n>> +\t\treturn;\n>> +\n>> +\tinfo->paramDequeued = true;\n>> +\tif (frameInfos_.tryComplete(info))\n>> +\t\tpipe_->completeRequest(buffer->request());\n>> +}\n>> +\n>> +void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>> +{\n>> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n>> +\t\treturn;\n> \n> Why do we need to check for the status for stats buffers but not for\n> params buffers ?\n\nAha! I bet we do :-) I started a rabbit-hole debugging where\ncompleteRequest() was being called with nullptrs at shutdown.\n\nI bet that was because the buffer->request() had been zeroed or was\nunset when the buffers are cancelled.\n\n\n> \n>> +\n>> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n>> +\tif (!info)\n>> +\t\treturn;\n>> +\n>> +\tIPAOperationData op;\n>> +\top.operation = IPU3_IPA_EVENT_STAT_READY;\n>> +\top.data = { info->id, info->statBuffer->cookie() };\n>> +\tipa_->processEvent(op);\n>> }\n>> \n>> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)\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 81572BD162\n\tfor ;\n\tThu, 4 Feb 2021 21:46:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A53861473;\n\tThu, 4 Feb 2021 22:46:50 +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 B1B3761430\n\tfor ;\n\tThu, 4 Feb 2021 22:46:48 +0100 (CET)","from [192.168.0.20]\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 1EF3345D;\n\tThu, 4 Feb 2021 22:46:48 +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=\"ZaP4izEy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612475208;\n\tbh=a+VWXS5IhbTtjUn5PPmdTdDuZ1Eid/AyNTv0Els+anY=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=ZaP4izEyz/R/4I6UsMVS40zWik+s+/gcRhbeaJOJwGGETsCU+XpeQjmBkmfC/dXrk\n\t3yGBJo0dhI+wNxs5tYv43IuZ8YQbVk4yd5IGQnE7GOwWSJGbmQ8erORWxIg9t1b7pJ\n\tB/YjdWDD7sly/l03tPM5AWSCOYi7Lc/AmApmQ1j0=","To":"Laurent Pinchart , =?utf-8?q?Niklas?=\n\t=?utf-8?q?_S=C3=B6derlund?= ","References":"<20210204162943.268517-1-niklas.soderlund@ragnatech.se>\n\t<20210204162943.268517-12-niklas.soderlund@ragnatech.se>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<965096f3-d477-5324-e820-8092e4c40d5d@ideasonboard.com>","Date":"Thu, 4 Feb 2021 21:46:45 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 11/11] libcamera: ipu3: Share\n\tparameter and statistic buffers with IPA","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15007,"web_url":"https://patchwork.libcamera.org/comment/15007/","msgid":"","date":"2021-02-04T22:13:52","subject":"Re: [libcamera-devel] [PATCH v3 11/11] libcamera: ipu3: Share\n\tparameter and statistic buffers with IPA","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2021-02-04 20:54:41 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Thu, Feb 04, 2021 at 05:29:43PM +0100, Niklas Söderlund wrote:\n> > Use the IPU3Frames helper to share parameters and statistics buffers\n> > with the IPA. Track which parameter and statistic buffer is used for\n> > which request and make sure the parameter buffers is filled in by the\n> > IPA before it's needed and that the statistic buffer is consumed and\n> > meta data generated before completing the request.\n> > \n> > With this change the IPU3 pipeline is prepared to fully operate with an\n> > IPA component.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > * Changes since v1\n> > - Update commit message.\n> > - s/frameInfo_/frameInfos_/\n> > - Refactor away isComplete variable.\n> > \n> > * Changes since v2\n> > - Emedd the IPU3Frames instance instead of allocating it.\n> > - Use -ENOBUFS instead of _ENOENT in queueRequestDevice().\n> > - Keep isComplete in cio2BufferReady()\n> > - Rework the queue logic.\n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 149 ++++++++++++++++++++-------\n> > 1 file changed, 112 insertions(+), 37 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 092db6389833a481..b79db25050d2c1d6 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -29,6 +29,7 @@\n> > #include \"libcamera/internal/v4l2_controls.h\"\n> > \n> > #include \"cio2.h\"\n> > +#include \"frames.h\"\n> > #include \"imgu.h\"\n> > \n> > namespace libcamera {\n> > @@ -61,6 +62,8 @@ public:\n> > \n> > \tvoid imguOutputBufferReady(FrameBuffer *buffer);\n> > \tvoid cio2BufferReady(FrameBuffer *buffer);\n> > +\tvoid paramBufferReady(FrameBuffer *buffer);\n> > +\tvoid statBufferReady(FrameBuffer *buffer);\n> > \n> > \tCIO2Device cio2_;\n> > \tImgUDevice *imgu_;\n> > @@ -71,6 +74,7 @@ public:\n> > \n> > \tuint32_t exposureTime_;\n> > \tstd::unique_ptr delayedCtrls_;\n> > +\tIPU3Frames frameInfos_;\n> > \n> > private:\n> > \tvoid queueFrameAction(unsigned int id, const IPAOperationData &op);\n> > @@ -609,6 +613,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> > \n> > \tdata->ipa_->mapBuffers(ipaBuffers_);\n> > \n> > +\tdata->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);\n> > +\n> > \treturn 0;\n> > }\n> > \n> > @@ -616,6 +622,8 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > \n> > +\tdata->frameInfos_.clear();\n> > +\n> > \tstd::vector ids;\n> > \tfor (IPABuffer &ipabuf : ipaBuffers_)\n> > \t\tids.push_back(ipabuf.id);\n> > @@ -713,7 +721,10 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > -\tint error = 0;\n> > +\n> > +\tIPU3Frames::Info *info = data->frameInfos_.create(request);\n> > +\tif (!info)\n> > +\t\treturn -ENOBUFS;\n> > \n> > \t/*\n> > \t * Queue a buffer on the CIO2, using the raw stream buffer provided in\n> > @@ -724,24 +735,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > \tif (!rawBuffer)\n> > \t\treturn -ENOMEM;\n> \n> What will happen to info in this case ? As there will be no raw buffer\n> associated with it, won't it linger forever in a list ?\n\nGood point. I will add a IPU3Frames::remove(Info *info); interface which \nmay be used to remove tracking of a request prematurely. It also makes \nthe case where the CIO2 buffer is canceled cleaner.\n\n> \n> > \n> > -\t/* Queue all buffers from the request aimed for the ImgU. */\n> > -\tfor (auto it : request->buffers()) {\n> > -\t\tconst Stream *stream = it.first;\n> > -\t\tFrameBuffer *buffer = it.second;\n> > -\t\tint ret;\n> > +\tinfo->rawBuffer = rawBuffer;\n> > \n> > -\t\tif (stream == &data->outStream_)\n> > -\t\t\tret = data->imgu_->output_->queueBuffer(buffer);\n> > -\t\telse if (stream == &data->vfStream_)\n> > -\t\t\tret = data->imgu_->viewfinder_->queueBuffer(buffer);\n> > -\t\telse\n> > -\t\t\tcontinue;\n> > -\n> > -\t\tif (ret < 0)\n> > -\t\t\terror = ret;\n> > -\t}\n> > -\n> > -\treturn error;\n> > +\treturn 0;\n> > }\n> > \n> > bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > @@ -1007,6 +1003,10 @@ int PipelineHandlerIPU3::registerCameras()\n> > \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> > \t\tdata->imgu_->viewfinder_->bufferReady.connect(data.get(),\n> > \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> > +\t\tdata->imgu_->param_->bufferReady.connect(data.get(),\n> > +\t\t\t\t\t&IPU3CameraData::paramBufferReady);\n> > +\t\tdata->imgu_->stat_->bufferReady.connect(data.get(),\n> > +\t\t\t\t\t&IPU3CameraData::statBufferReady);\n> > \n> > \t\t/* Create and register the Camera instance. */\n> > \t\tstd::string cameraId = cio2->sensor()->id();\n> > @@ -1039,7 +1039,7 @@ int IPU3CameraData::loadIPA()\n> > \treturn 0;\n> > }\n> > \n> > -void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,\n> > +void IPU3CameraData::queueFrameAction(unsigned int id,\n> > \t\t\t\t const IPAOperationData &action)\n> > {\n> > \tswitch (action.operation) {\n> > @@ -1048,6 +1048,41 @@ void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,\n> > \t\tdelayedCtrls_->push(controls);\n> > \t\tbreak;\n> > \t}\n> > +\tcase IPU3_IPA_ACTION_PARAM_FILLED: {\n> > +\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n> > +\t\tif (!info)\n> > +\t\t\tbreak;\n> \n> Should we warn in this case ? Maybe in IPU3Frames::info() itself to\n> avoid duplicating log calls ? I'm tempted to even assert.\n\nI will add LOG() to IPU3Frames::find() at the Error level. We can then \neasily turn them into Fatal if we think it better in the future.\n\n> \n> > +\n> > +\t\t/* Queue all buffers from the request aimed for the ImgU. */\n> > +\t\tfor (auto it : info->request->buffers()) {\n> > +\t\t\tconst Stream *stream = it.first;\n> > +\t\t\tFrameBuffer *outbuffer = it.second;\n> > +\n> > +\t\t\tif (stream == &outStream_)\n> > +\t\t\t\timgu_->output_->queueBuffer(outbuffer);\n> > +\t\t\telse if (stream == &vfStream_)\n> > +\t\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n> > +\t\t}\n> > +\n> > +\t\timgu_->param_->queueBuffer(info->paramBuffer);\n> > +\t\timgu_->stat_->queueBuffer(info->statBuffer);\n> > +\t\timgu_->input_->queueBuffer(info->rawBuffer);\n> > +\n> > +\t\tbreak;\n> > +\t}\n> > +\tcase IPU3_IPA_ACTION_METADATA_READY: {\n> > +\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n> > +\t\tif (!info)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tRequest *request = info->request;\n> > +\t\trequest->metadata() = action.controls[0];\n> > +\t\tinfo->metadataProcessed = true;\n> > +\t\tif (frameInfos_.tryComplete(info))\n> > +\t\t\tpipe_->completeRequest(request);\n> \n> Maybe we could pass the request to IPU3Frames, and handle completion\n> automatically internally, but that's for later.\n\nWe did that in earlier versions but it was disliked as a reference to \nthe pipeline handler would then need to be stored inside the IPU3Frames. \nI still think it's nicer then the code duplication you point out here. \nBut will keep it as is for now.\n\n> \n> > +\n> > +\t\tbreak;\n> > +\t}\n> > \tdefault:\n> > \t\tLOG(IPU3, Error) << \"Unknown action \" << action.operation;\n> > \t\tbreak;\n> > @@ -1068,11 +1103,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> > {\n> > \tRequest *request = buffer->request();\n> > \n> > -\tif (!pipe_->completeBuffer(request, buffer))\n> > -\t\t/* Request not completed yet, return here. */\n> > +\tpipe_->completeBuffer(request, buffer);\n> > +\n> > +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> > +\tif (!info)\n> > \t\treturn;\n> > \n> > -\t/* Mark the request as complete. */\n> > \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n> > \t/* \\todo Move the ExposureTime control to the IPA. */\n> > \trequest->metadata().set(controls::ExposureTime, exposureTime_);\n> > @@ -1081,7 +1117,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> > \t\tRectangle cropRegion = request->controls().get(controls::ScalerCrop);\n> > \t\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n> > \t}\n> > -\tpipe_->completeRequest(request);\n> > +\n> > +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> > +\t\tinfo->metadataProcessed = true;\n> > +\n> > +\tif (frameInfos_.tryComplete(info))\n> > +\t\tpipe_->completeRequest(request);\n> > }\n> > \n> > /**\n> > @@ -1093,26 +1134,60 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> > */\n> > void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > {\n> > -\t/* \\todo Handle buffer failures when state is set to BufferError. */\n> > -\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> > +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> > +\tif (!info)\n> > \t\treturn;\n> > \n> > \tRequest *request = buffer->request();\n> > \n> > -\t/*\n> > -\t * If the request contains a buffer for the RAW stream only, complete it\n> > -\t * now as there's no need for ImgU processing.\n> > -\t */\n> > -\tif (request->findBuffer(&rawStream_)) {\n> > -\t\tbool isComplete = pipe_->completeBuffer(request, buffer);\n> > -\t\tif (isComplete) {\n> > -\t\t\trequest->metadata().set(controls::draft::PipelineDepth, 2);\n> > -\t\t\tpipe_->completeRequest(request);\n> > -\t\t\treturn;\n> > -\t\t}\n> > +\tif (request->findBuffer(&rawStream_))\n> > +\t\tpipe_->completeBuffer(request, buffer);\n> \n> If you moved this after the next if (...) { } block, you could...\n> \n> > +\n> > +\t/* If the buffer is cancelled force a complete of the whole request. */\n> > +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > +\t\tfor (auto it : request->buffers())\n> > +\t\t\tif (it.second != buffer)\n> \n> ... drop this check.\n\nGood point.\n\n> \n> > +\t\t\t\tpipe_->completeBuffer(request, it.second);\n> > +\n> > +\t\tinfo->paramDequeued = true;\n> > +\t\tinfo->metadataProcessed = true;\n> > +\t\tASSERT(frameInfos_.tryComplete(info));\n> > +\t\tpipe_->completeRequest(request);\n> > +\n> > +\t\treturn;\n> > \t}\n> > \n> > -\timgu_->input_->queueBuffer(buffer);\n> > +\tIPAOperationData op;\n> > +\top.operation = IPU3_IPA_EVENT_FILL_PARAMS;\n> > +\top.data = { info->id, info->paramBuffer->cookie() };\n> > +\top.controls = { request->controls() };\n> \n> This is the right time to call IPU3_IPA_EVENT_FILL_PARAMS, but the\n> request controls need to be passed to the IPA earlier, right when the\n> request is queued, as the IPA needs to look ahead in the request queue\n> to compute parameters.\n> \n> This is the main issue in this patch, the rest is fairly minor.\n\nI see your point I will fix this for v4.\n\n> \n> > +\tipa_->processEvent(op);\n> > +}\n> > +\n> > +void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> > +{\n> > +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> > +\tif (!info)\n> > +\t\treturn;\n> > +\n> > +\tinfo->paramDequeued = true;\n> > +\tif (frameInfos_.tryComplete(info))\n> > +\t\tpipe_->completeRequest(buffer->request());\n> > +}\n> > +\n> > +void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > +{\n> > +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> > +\t\treturn;\n> \n> Why do we need to check for the status for stats buffers but not for\n> params buffers ?\n\nBecause there is no point in giving a cancelled statistics buffer to the \nIPA. While for the param all we really care about is marking the buffer \nfree and moving on. But this error path can't have been tested it should \nset info->metadataProcessed = true and try to complete the request. Will \nfix for v4.\n\n> \n> > +\n> > +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> > +\tif (!info)\n> > +\t\treturn;\n> > +\n> > +\tIPAOperationData op;\n> > +\top.operation = IPU3_IPA_EVENT_STAT_READY;\n> > +\top.data = { info->id, info->statBuffer->cookie() };\n> > +\tipa_->processEvent(op);\n> > }\n> > \n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 AE36FBD160\n\tfor ;\n\tThu, 4 Feb 2021 22:13:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C6B261489;\n\tThu, 4 Feb 2021 23:13:56 +0100 (CET)","from mail-lf1-x131.google.com (mail-lf1-x131.google.com\n\t[IPv6:2a00:1450:4864:20::131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EE57361430\n\tfor ;\n\tThu, 4 Feb 2021 23:13:54 +0100 (CET)","by mail-lf1-x131.google.com with SMTP id i187so6943463lfd.4\n\tfor ;\n\tThu, 04 Feb 2021 14:13:54 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tv10sm767221lfg.290.2021.02.04.14.13.53\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 04 Feb 2021 14:13:53 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"Nwd9l9We\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=tv+Z4zgG/tQPNHavwq2HBWDCMmRW1UkMqVtuMH0NDkI=;\n\tb=Nwd9l9WerXod1cwGVM9gsYjCxbimvoUwckguBGFXKt+Tqv661geTjrgk/n4tuuXlg5\n\tWCO6jM9fsOhquEGKi55u7NNHqPsG2NZpCKbX/1zYRMbEsekFOp1F5uDNpZqvHmjqaHXa\n\tqUWi57ZyOQRsJnyCZixA0OLPg3mecmjQI39IsHMBCZoJxs2NgCuzcymLQO+m4o+clSVg\n\tU4yoLSSw66mAnJIPFsIMGQUrTtq2THZSUIakJsdVvTG6lDLGKdLPimP2t/kL+CkQ8qhu\n\thkUtsLTpTiWTvCgl6YwbIB7qNbC8w8D2shZDbPRn393n9YfR/PE7JZGJLsW7yafoSwIU\n\tfhjw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=tv+Z4zgG/tQPNHavwq2HBWDCMmRW1UkMqVtuMH0NDkI=;\n\tb=CJcmbMq2IneQjTF0ogp7xfFgnpTsTGhcnCeYIlf2KFnRkoqTu8LYtCIVMOz/z5sL97\n\tvYuc2xc3lAOrGVrPvoWFu0qiDb1ckDqW8fDhxi5nF7XZAeKHN5M1hWXGgULg8xESZY+M\n\tLW+DDvjmgTjq3gOHGuOSeS/Z8NX2ZTUY0sRtEYko4lxvSfwCLPfyHnVBUTOENekJ9tOh\n\t3YaruGexYUZ8ccWob0+e4C612DmjLawpTSYc4s43Beqb3H+DEEWlpFyL3N84nTbFoJpK\n\tFfdbKY1o85lJ5phCUs6yuAPJEpsgHkglWG0EvOFEHe28V8Of19NAKelhqz6yRqwh8Zx+\n\tM34Q==","X-Gm-Message-State":"AOAM5309k6TGCCmDohjixz+b9lsx9dmw8i8dzLr9amMFZv3VXqDomRhM\n\tGIZ/3mfd+cpEjd5gahnDhTj6YvIPD5Cvned1","X-Google-Smtp-Source":"ABdhPJyfONVtoC+lZINqysB6F10E5ttTze3iIUMVdbieU5qkGD1Dfx8jYvNoqmWTVmUoPXLiGNEnew==","X-Received":"by 2002:a05:6512:10c5:: with SMTP id\n\tk5mr751541lfg.583.1612476834296; \n\tThu, 04 Feb 2021 14:13:54 -0800 (PST)","Date":"Thu, 4 Feb 2021 23:13:52 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20210204162943.268517-1-niklas.soderlund@ragnatech.se>\n\t<20210204162943.268517-12-niklas.soderlund@ragnatech.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 11/11] libcamera: ipu3: Share\n\tparameter and statistic buffers with IPA","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15024,"web_url":"https://patchwork.libcamera.org/comment/15024/","msgid":"<6f417b60-9dc3-173a-29a6-a7aaecffa06a@ideasonboard.com>","date":"2021-02-05T10:31:12","subject":"Re: [libcamera-devel] [PATCH v3 11/11] libcamera: ipu3: Share\n\tparameter and statistic buffers with IPA","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent/Niklas,\n\nOn 04/02/2021 22:13, Niklas Söderlund wrote:\n> Hi Laurent,\n> \n> Thanks for your feedback.\n> \n> On 2021-02-04 20:54:41 +0200, Laurent Pinchart wrote:\n>> Hi Niklas,\n>>\n>> Thank you for the patch.\n>>\n>> On Thu, Feb 04, 2021 at 05:29:43PM +0100, Niklas Söderlund wrote:\n>>> Use the IPU3Frames helper to share parameters and statistics buffers\n>>> with the IPA. Track which parameter and statistic buffer is used for\n>>> which request and make sure the parameter buffers is filled in by the\n>>> IPA before it's needed and that the statistic buffer is consumed and\n>>> meta data generated before completing the request.\n>>>\n>>> With this change the IPU3 pipeline is prepared to fully operate with an\n>>> IPA component.\n>>>\n>>> Signed-off-by: Niklas Söderlund \n>>> ---\n>>> * Changes since v1\n>>> - Update commit message.\n>>> - s/frameInfo_/frameInfos_/\n>>> - Refactor away isComplete variable.\n>>>\n>>> * Changes since v2\n>>> - Emedd the IPU3Frames instance instead of allocating it.\n>>> - Use -ENOBUFS instead of _ENOENT in queueRequestDevice().\n>>> - Keep isComplete in cio2BufferReady()\n>>> - Rework the queue logic.\n>>> ---\n>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 149 ++++++++++++++++++++-------\n>>> 1 file changed, 112 insertions(+), 37 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> index 092db6389833a481..b79db25050d2c1d6 100644\n>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> @@ -29,6 +29,7 @@\n>>> #include \"libcamera/internal/v4l2_controls.h\"\n>>> \n>>> #include \"cio2.h\"\n>>> +#include \"frames.h\"\n>>> #include \"imgu.h\"\n>>> \n>>> namespace libcamera {\n>>> @@ -61,6 +62,8 @@ public:\n>>> \n>>> \tvoid imguOutputBufferReady(FrameBuffer *buffer);\n>>> \tvoid cio2BufferReady(FrameBuffer *buffer);\n>>> +\tvoid paramBufferReady(FrameBuffer *buffer);\n>>> +\tvoid statBufferReady(FrameBuffer *buffer);\n>>> \n>>> \tCIO2Device cio2_;\n>>> \tImgUDevice *imgu_;\n>>> @@ -71,6 +74,7 @@ public:\n>>> \n>>> \tuint32_t exposureTime_;\n>>> \tstd::unique_ptr delayedCtrls_;\n>>> +\tIPU3Frames frameInfos_;\n>>> \n>>> private:\n>>> \tvoid queueFrameAction(unsigned int id, const IPAOperationData &op);\n>>> @@ -609,6 +613,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n>>> \n>>> \tdata->ipa_->mapBuffers(ipaBuffers_);\n>>> \n>>> +\tdata->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);\n>>> +\n>>> \treturn 0;\n>>> }\n>>> \n>>> @@ -616,6 +622,8 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n>>> {\n>>> \tIPU3CameraData *data = cameraData(camera);\n>>> \n>>> +\tdata->frameInfos_.clear();\n>>> +\n>>> \tstd::vector ids;\n>>> \tfor (IPABuffer &ipabuf : ipaBuffers_)\n>>> \t\tids.push_back(ipabuf.id);\n>>> @@ -713,7 +721,10 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n>>> int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n>>> {\n>>> \tIPU3CameraData *data = cameraData(camera);\n>>> -\tint error = 0;\n>>> +\n>>> +\tIPU3Frames::Info *info = data->frameInfos_.create(request);\n>>> +\tif (!info)\n>>> +\t\treturn -ENOBUFS;\n>>> \n>>> \t/*\n>>> \t * Queue a buffer on the CIO2, using the raw stream buffer provided in\n>>> @@ -724,24 +735,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n>>> \tif (!rawBuffer)\n>>> \t\treturn -ENOMEM;\n>>\n>> What will happen to info in this case ? As there will be no raw buffer\n>> associated with it, won't it linger forever in a list ?\n> \n> Good point. I will add a IPU3Frames::remove(Info *info); interface which \n> may be used to remove tracking of a request prematurely. It also makes \n> the case where the CIO2 buffer is canceled cleaner.\n> \n>>\n>>> \n>>> -\t/* Queue all buffers from the request aimed for the ImgU. */\n>>> -\tfor (auto it : request->buffers()) {\n>>> -\t\tconst Stream *stream = it.first;\n>>> -\t\tFrameBuffer *buffer = it.second;\n>>> -\t\tint ret;\n>>> +\tinfo->rawBuffer = rawBuffer;\n>>> \n>>> -\t\tif (stream == &data->outStream_)\n>>> -\t\t\tret = data->imgu_->output_->queueBuffer(buffer);\n>>> -\t\telse if (stream == &data->vfStream_)\n>>> -\t\t\tret = data->imgu_->viewfinder_->queueBuffer(buffer);\n>>> -\t\telse\n>>> -\t\t\tcontinue;\n>>> -\n>>> -\t\tif (ret < 0)\n>>> -\t\t\terror = ret;\n>>> -\t}\n>>> -\n>>> -\treturn error;\n>>> +\treturn 0;\n>>> }\n>>> \n>>> bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n>>> @@ -1007,6 +1003,10 @@ int PipelineHandlerIPU3::registerCameras()\n>>> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n>>> \t\tdata->imgu_->viewfinder_->bufferReady.connect(data.get(),\n>>> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n>>> +\t\tdata->imgu_->param_->bufferReady.connect(data.get(),\n>>> +\t\t\t\t\t&IPU3CameraData::paramBufferReady);\n>>> +\t\tdata->imgu_->stat_->bufferReady.connect(data.get(),\n>>> +\t\t\t\t\t&IPU3CameraData::statBufferReady);\n>>> \n>>> \t\t/* Create and register the Camera instance. */\n>>> \t\tstd::string cameraId = cio2->sensor()->id();\n>>> @@ -1039,7 +1039,7 @@ int IPU3CameraData::loadIPA()\n>>> \treturn 0;\n>>> }\n>>> \n>>> -void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,\n>>> +void IPU3CameraData::queueFrameAction(unsigned int id,\n>>> \t\t\t\t const IPAOperationData &action)\n>>> {\n>>> \tswitch (action.operation) {\n>>> @@ -1048,6 +1048,41 @@ void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,\n>>> \t\tdelayedCtrls_->push(controls);\n>>> \t\tbreak;\n>>> \t}\n>>> +\tcase IPU3_IPA_ACTION_PARAM_FILLED: {\n>>> +\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n>>> +\t\tif (!info)\n>>> +\t\t\tbreak;\n>>\n>> Should we warn in this case ? Maybe in IPU3Frames::info() itself to\n>> avoid duplicating log calls ? I'm tempted to even assert.\n> \n> I will add LOG() to IPU3Frames::find() at the Error level. We can then \n> easily turn them into Fatal if we think it better in the future.\n> \n>>\n>>> +\n>>> +\t\t/* Queue all buffers from the request aimed for the ImgU. */\n>>> +\t\tfor (auto it : info->request->buffers()) {\n>>> +\t\t\tconst Stream *stream = it.first;\n>>> +\t\t\tFrameBuffer *outbuffer = it.second;\n>>> +\n>>> +\t\t\tif (stream == &outStream_)\n>>> +\t\t\t\timgu_->output_->queueBuffer(outbuffer);\n>>> +\t\t\telse if (stream == &vfStream_)\n>>> +\t\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n>>> +\t\t}\n>>> +\n>>> +\t\timgu_->param_->queueBuffer(info->paramBuffer);\n>>> +\t\timgu_->stat_->queueBuffer(info->statBuffer);\n>>> +\t\timgu_->input_->queueBuffer(info->rawBuffer);\n>>> +\n>>> +\t\tbreak;\n>>> +\t}\n>>> +\tcase IPU3_IPA_ACTION_METADATA_READY: {\n>>> +\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n>>> +\t\tif (!info)\n>>> +\t\t\tbreak;\n>>> +\n>>> +\t\tRequest *request = info->request;\n>>> +\t\trequest->metadata() = action.controls[0];\n>>> +\t\tinfo->metadataProcessed = true;\n>>> +\t\tif (frameInfos_.tryComplete(info))\n>>> +\t\t\tpipe_->completeRequest(request);\n>>\n>> Maybe we could pass the request to IPU3Frames, and handle completion\n>> automatically internally, but that's for later.\n> \n> We did that in earlier versions but it was disliked as a reference to \n> the pipeline handler would then need to be stored inside the IPU3Frames. \n> I still think it's nicer then the code duplication you point out here. \n> But will keep it as is for now.\n\n\nHrm ... I think I've just realised the implications of this here.\n\nDo IPA's really have the power to 'complete requests' ? Shouldn't that\nbe a capability/responsibility restricted to the pipeline handlers?\n\nI can see that restricting would mean there would be some notification\nrequired back to the pipeline handler so that the IPA would be able to\nsay \"I'm finished with this object\", but I sort of expected that to be\nrequired anyway. Perhaps directly trying to complete the request is just\na shortcut on that then.\n\n\n--\nKieran\n\n>>\n>>> +\n>>> +\t\tbreak;\n>>> +\t}\n>>> \tdefault:\n>>> \t\tLOG(IPU3, Error) << \"Unknown action \" << action.operation;\n>>> \t\tbreak;\n>>> @@ -1068,11 +1103,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>>> {\n>>> \tRequest *request = buffer->request();\n>>> \n>>> -\tif (!pipe_->completeBuffer(request, buffer))\n>>> -\t\t/* Request not completed yet, return here. */\n>>> +\tpipe_->completeBuffer(request, buffer);\n>>> +\n>>> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n>>> +\tif (!info)\n>>> \t\treturn;\n>>> \n>>> -\t/* Mark the request as complete. */\n>>> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n>>> \t/* \\todo Move the ExposureTime control to the IPA. */\n>>> \trequest->metadata().set(controls::ExposureTime, exposureTime_);\n>>> @@ -1081,7 +1117,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>>> \t\tRectangle cropRegion = request->controls().get(controls::ScalerCrop);\n>>> \t\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n>>> \t}\n>>> -\tpipe_->completeRequest(request);\n>>> +\n>>> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n>>> +\t\tinfo->metadataProcessed = true;\n>>> +\n>>> +\tif (frameInfos_.tryComplete(info))\n>>> +\t\tpipe_->completeRequest(request);\n>>> }\n>>> \n>>> /**\n>>> @@ -1093,26 +1134,60 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>>> */\n>>> void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>>> {\n>>> -\t/* \\todo Handle buffer failures when state is set to BufferError. */\n>>> -\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n>>> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n>>> +\tif (!info)\n>>> \t\treturn;\n>>> \n>>> \tRequest *request = buffer->request();\n>>> \n>>> -\t/*\n>>> -\t * If the request contains a buffer for the RAW stream only, complete it\n>>> -\t * now as there's no need for ImgU processing.\n>>> -\t */\n>>> -\tif (request->findBuffer(&rawStream_)) {\n>>> -\t\tbool isComplete = pipe_->completeBuffer(request, buffer);\n>>> -\t\tif (isComplete) {\n>>> -\t\t\trequest->metadata().set(controls::draft::PipelineDepth, 2);\n>>> -\t\t\tpipe_->completeRequest(request);\n>>> -\t\t\treturn;\n>>> -\t\t}\n>>> +\tif (request->findBuffer(&rawStream_))\n>>> +\t\tpipe_->completeBuffer(request, buffer);\n>>\n>> If you moved this after the next if (...) { } block, you could...\n>>\n>>> +\n>>> +\t/* If the buffer is cancelled force a complete of the whole request. */\n>>> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n>>> +\t\tfor (auto it : request->buffers())\n>>> +\t\t\tif (it.second != buffer)\n>>\n>> ... drop this check.\n> \n> Good point.\n> \n>>\n>>> +\t\t\t\tpipe_->completeBuffer(request, it.second);\n>>> +\n>>> +\t\tinfo->paramDequeued = true;\n>>> +\t\tinfo->metadataProcessed = true;\n>>> +\t\tASSERT(frameInfos_.tryComplete(info));\n>>> +\t\tpipe_->completeRequest(request);\n>>> +\n>>> +\t\treturn;\n>>> \t}\n>>> \n>>> -\timgu_->input_->queueBuffer(buffer);\n>>> +\tIPAOperationData op;\n>>> +\top.operation = IPU3_IPA_EVENT_FILL_PARAMS;\n>>> +\top.data = { info->id, info->paramBuffer->cookie() };\n>>> +\top.controls = { request->controls() };\n>>\n>> This is the right time to call IPU3_IPA_EVENT_FILL_PARAMS, but the\n>> request controls need to be passed to the IPA earlier, right when the\n>> request is queued, as the IPA needs to look ahead in the request queue\n>> to compute parameters.\n>>\n>> This is the main issue in this patch, the rest is fairly minor.\n> \n> I see your point I will fix this for v4.\n> \n>>\n>>> +\tipa_->processEvent(op);\n>>> +}\n>>> +\n>>> +void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>>> +{\n>>> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n>>> +\tif (!info)\n>>> +\t\treturn;\n>>> +\n>>> +\tinfo->paramDequeued = true;\n>>> +\tif (frameInfos_.tryComplete(info))\n>>> +\t\tpipe_->completeRequest(buffer->request());\n>>> +}\n>>> +\n>>> +void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>>> +{\n>>> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n>>> +\t\treturn;\n>>\n>> Why do we need to check for the status for stats buffers but not for\n>> params buffers ?\n> \n> Because there is no point in giving a cancelled statistics buffer to the \n> IPA. While for the param all we really care about is marking the buffer \n> free and moving on. But this error path can't have been tested it should \n> set info->metadataProcessed = true and try to complete the request. Will \n> fix for v4.\n> \n>>\n>>> +\n>>> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n>>> +\tif (!info)\n>>> +\t\treturn;\n>>> +\n>>> +\tIPAOperationData op;\n>>> +\top.operation = IPU3_IPA_EVENT_STAT_READY;\n>>> +\top.data = { info->id, info->statBuffer->cookie() };\n>>> +\tipa_->processEvent(op);\n>>> }\n>>> \n>>> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)\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 8219BBD162\n\tfor ;\n\tFri, 5 Feb 2021 10:31:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B21A60106;\n\tFri, 5 Feb 2021 11:31:17 +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 3C21D60106\n\tfor ;\n\tFri, 5 Feb 2021 11:31:15 +0100 (CET)","from [192.168.0.20]\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 9AB29316;\n\tFri, 5 Feb 2021 11:31:14 +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=\"OTVgppko\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612521074;\n\tbh=/tWW4+LRXW5zsEEcW1NAek2HXXwb2bTE2RrOBPhKLes=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=OTVgppkoxqTJkdJwfmIlhU/NRvyGgIC9wNhg0FXQ6XvZIzjYvTAEj/CBRYAzF0oaY\n\t7K9Wj3/i8xaMA71K4kXwI8flVLFcids8DOyZdzmd+Kt++Ntzz0lRoB23A0DxMn6y+x\n\tBEMFbIloNsaW+b2sRRIl4nsYWktrDDgsesc90b8U=","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tLaurent Pinchart ","References":"<20210204162943.268517-1-niklas.soderlund@ragnatech.se>\n\t<20210204162943.268517-12-niklas.soderlund@ragnatech.se>\n\t\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<6f417b60-9dc3-173a-29a6-a7aaecffa06a@ideasonboard.com>","Date":"Fri, 5 Feb 2021 10:31:12 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 11/11] libcamera: ipu3: Share\n\tparameter and statistic buffers with IPA","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15025,"web_url":"https://patchwork.libcamera.org/comment/15025/","msgid":"<4088ff56-88f0-a998-fffc-e354b54d6b6f@ideasonboard.com>","date":"2021-02-05T10:34:17","subject":"Re: [libcamera-devel] [PATCH v3 11/11] libcamera: ipu3: Share\n\tparameter and statistic buffers with IPA","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hello Kieran\n\n\nOn 05/02/2021 10:31, Kieran Bingham wrote:\n> Hi Laurent/Niklas,\n> \n> On 04/02/2021 22:13, Niklas Söderlund wrote:\n>> Hi Laurent,\n>>\n>> Thanks for your feedback.\n>>\n>> On 2021-02-04 20:54:41 +0200, Laurent Pinchart wrote:\n>>> Hi Niklas,\n>>>\n>>> Thank you for the patch.\n>>>\n>>> On Thu, Feb 04, 2021 at 05:29:43PM +0100, Niklas Söderlund wrote:\n>>>> Use the IPU3Frames helper to share parameters and statistics buffers\n>>>> with the IPA. Track which parameter and statistic buffer is used for\n>>>> which request and make sure the parameter buffers is filled in by the\n>>>> IPA before it's needed and that the statistic buffer is consumed and\n>>>> meta data generated before completing the request.\n>>>>\n>>>> With this change the IPU3 pipeline is prepared to fully operate with an\n>>>> IPA component.\n>>>>\n>>>> Signed-off-by: Niklas Söderlund \n>>>> ---\n>>>> * Changes since v1\n>>>> - Update commit message.\n>>>> - s/frameInfo_/frameInfos_/\n>>>> - Refactor away isComplete variable.\n>>>>\n>>>> * Changes since v2\n>>>> - Emedd the IPU3Frames instance instead of allocating it.\n>>>> - Use -ENOBUFS instead of _ENOENT in queueRequestDevice().\n>>>> - Keep isComplete in cio2BufferReady()\n>>>> - Rework the queue logic.\n>>>> ---\n>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 149 ++++++++++++++++++++-------\n>>>> 1 file changed, 112 insertions(+), 37 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> index 092db6389833a481..b79db25050d2c1d6 100644\n>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> @@ -29,6 +29,7 @@\n>>>> #include \"libcamera/internal/v4l2_controls.h\"\n>>>> \n>>>> #include \"cio2.h\"\n>>>> +#include \"frames.h\"\n>>>> #include \"imgu.h\"\n>>>> \n>>>> namespace libcamera {\n>>>> @@ -61,6 +62,8 @@ public:\n>>>> \n>>>> \tvoid imguOutputBufferReady(FrameBuffer *buffer);\n>>>> \tvoid cio2BufferReady(FrameBuffer *buffer);\n>>>> +\tvoid paramBufferReady(FrameBuffer *buffer);\n>>>> +\tvoid statBufferReady(FrameBuffer *buffer);\n>>>> \n>>>> \tCIO2Device cio2_;\n>>>> \tImgUDevice *imgu_;\n>>>> @@ -71,6 +74,7 @@ public:\n>>>> \n>>>> \tuint32_t exposureTime_;\n>>>> \tstd::unique_ptr delayedCtrls_;\n>>>> +\tIPU3Frames frameInfos_;\n>>>> \n>>>> private:\n>>>> \tvoid queueFrameAction(unsigned int id, const IPAOperationData &op);\n>>>> @@ -609,6 +613,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n>>>> \n>>>> \tdata->ipa_->mapBuffers(ipaBuffers_);\n>>>> \n>>>> +\tdata->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);\n>>>> +\n>>>> \treturn 0;\n>>>> }\n>>>> \n>>>> @@ -616,6 +622,8 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n>>>> {\n>>>> \tIPU3CameraData *data = cameraData(camera);\n>>>> \n>>>> +\tdata->frameInfos_.clear();\n>>>> +\n>>>> \tstd::vector ids;\n>>>> \tfor (IPABuffer &ipabuf : ipaBuffers_)\n>>>> \t\tids.push_back(ipabuf.id);\n>>>> @@ -713,7 +721,10 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n>>>> int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n>>>> {\n>>>> \tIPU3CameraData *data = cameraData(camera);\n>>>> -\tint error = 0;\n>>>> +\n>>>> +\tIPU3Frames::Info *info = data->frameInfos_.create(request);\n>>>> +\tif (!info)\n>>>> +\t\treturn -ENOBUFS;\n>>>> \n>>>> \t/*\n>>>> \t * Queue a buffer on the CIO2, using the raw stream buffer provided in\n>>>> @@ -724,24 +735,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n>>>> \tif (!rawBuffer)\n>>>> \t\treturn -ENOMEM;\n>>>\n>>> What will happen to info in this case ? As there will be no raw buffer\n>>> associated with it, won't it linger forever in a list ?\n>>\n>> Good point. I will add a IPU3Frames::remove(Info *info); interface which \n>> may be used to remove tracking of a request prematurely. It also makes \n>> the case where the CIO2 buffer is canceled cleaner.\n>>\n>>>\n>>>> \n>>>> -\t/* Queue all buffers from the request aimed for the ImgU. */\n>>>> -\tfor (auto it : request->buffers()) {\n>>>> -\t\tconst Stream *stream = it.first;\n>>>> -\t\tFrameBuffer *buffer = it.second;\n>>>> -\t\tint ret;\n>>>> +\tinfo->rawBuffer = rawBuffer;\n>>>> \n>>>> -\t\tif (stream == &data->outStream_)\n>>>> -\t\t\tret = data->imgu_->output_->queueBuffer(buffer);\n>>>> -\t\telse if (stream == &data->vfStream_)\n>>>> -\t\t\tret = data->imgu_->viewfinder_->queueBuffer(buffer);\n>>>> -\t\telse\n>>>> -\t\t\tcontinue;\n>>>> -\n>>>> -\t\tif (ret < 0)\n>>>> -\t\t\terror = ret;\n>>>> -\t}\n>>>> -\n>>>> -\treturn error;\n>>>> +\treturn 0;\n>>>> }\n>>>> \n>>>> bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n>>>> @@ -1007,6 +1003,10 @@ int PipelineHandlerIPU3::registerCameras()\n>>>> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n>>>> \t\tdata->imgu_->viewfinder_->bufferReady.connect(data.get(),\n>>>> \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n>>>> +\t\tdata->imgu_->param_->bufferReady.connect(data.get(),\n>>>> +\t\t\t\t\t&IPU3CameraData::paramBufferReady);\n>>>> +\t\tdata->imgu_->stat_->bufferReady.connect(data.get(),\n>>>> +\t\t\t\t\t&IPU3CameraData::statBufferReady);\n>>>> \n>>>> \t\t/* Create and register the Camera instance. */\n>>>> \t\tstd::string cameraId = cio2->sensor()->id();\n>>>> @@ -1039,7 +1039,7 @@ int IPU3CameraData::loadIPA()\n>>>> \treturn 0;\n>>>> }\n>>>> \n>>>> -void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,\n>>>> +void IPU3CameraData::queueFrameAction(unsigned int id,\n>>>> \t\t\t\t const IPAOperationData &action)\n>>>> {\n>>>> \tswitch (action.operation) {\n>>>> @@ -1048,6 +1048,41 @@ void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,\n>>>> \t\tdelayedCtrls_->push(controls);\n>>>> \t\tbreak;\n>>>> \t}\n>>>> +\tcase IPU3_IPA_ACTION_PARAM_FILLED: {\n>>>> +\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n>>>> +\t\tif (!info)\n>>>> +\t\t\tbreak;\n>>>\n>>> Should we warn in this case ? Maybe in IPU3Frames::info() itself to\n>>> avoid duplicating log calls ? I'm tempted to even assert.\n>>\n>> I will add LOG() to IPU3Frames::find() at the Error level. We can then \n>> easily turn them into Fatal if we think it better in the future.\n>>\n>>>\n>>>> +\n>>>> +\t\t/* Queue all buffers from the request aimed for the ImgU. */\n>>>> +\t\tfor (auto it : info->request->buffers()) {\n>>>> +\t\t\tconst Stream *stream = it.first;\n>>>> +\t\t\tFrameBuffer *outbuffer = it.second;\n>>>> +\n>>>> +\t\t\tif (stream == &outStream_)\n>>>> +\t\t\t\timgu_->output_->queueBuffer(outbuffer);\n>>>> +\t\t\telse if (stream == &vfStream_)\n>>>> +\t\t\t\timgu_->viewfinder_->queueBuffer(outbuffer);\n>>>> +\t\t}\n>>>> +\n>>>> +\t\timgu_->param_->queueBuffer(info->paramBuffer);\n>>>> +\t\timgu_->stat_->queueBuffer(info->statBuffer);\n>>>> +\t\timgu_->input_->queueBuffer(info->rawBuffer);\n>>>> +\n>>>> +\t\tbreak;\n>>>> +\t}\n>>>> +\tcase IPU3_IPA_ACTION_METADATA_READY: {\n>>>> +\t\tIPU3Frames::Info *info = frameInfos_.find(id);\n>>>> +\t\tif (!info)\n>>>> +\t\t\tbreak;\n>>>> +\n>>>> +\t\tRequest *request = info->request;\n>>>> +\t\trequest->metadata() = action.controls[0];\n>>>> +\t\tinfo->metadataProcessed = true;\n>>>> +\t\tif (frameInfos_.tryComplete(info))\n>>>> +\t\t\tpipe_->completeRequest(request);\n>>>\n>>> Maybe we could pass the request to IPU3Frames, and handle completion\n>>> automatically internally, but that's for later.\n>>\n>> We did that in earlier versions but it was disliked as a reference to \n>> the pipeline handler would then need to be stored inside the IPU3Frames. \n>> I still think it's nicer then the code duplication you point out here. \n>> But will keep it as is for now.\n> \n> \n> Hrm ... I think I've just realised the implications of this here.\n> \n> Do IPA's really have the power to 'complete requests' ? Shouldn't that\n> be a capability/responsibility restricted to the pipeline handlers?\n> \n> I can see that restricting would mean there would be some notification\n> required back to the pipeline handler so that the IPA would be able to\n> say \"I'm finished with this object\", but I sort of expected that to be\n> required anyway. Perhaps directly trying to complete the request is just\n> a shortcut on that then.\n\n\nPlease consider the function that this code is in, and try to determine\nif this is in the PipelineHandler or the IPA - and you might find that\nyour entire concern is completely invalid ;-)\n\n\n\n--\nKieran\n\n> \n> \n> --\n> Kieran\n> \n>>>\n>>>> +\n>>>> +\t\tbreak;\n>>>> +\t}\n>>>> \tdefault:\n>>>> \t\tLOG(IPU3, Error) << \"Unknown action \" << action.operation;\n>>>> \t\tbreak;\n>>>> @@ -1068,11 +1103,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>>>> {\n>>>> \tRequest *request = buffer->request();\n>>>> \n>>>> -\tif (!pipe_->completeBuffer(request, buffer))\n>>>> -\t\t/* Request not completed yet, return here. */\n>>>> +\tpipe_->completeBuffer(request, buffer);\n>>>> +\n>>>> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n>>>> +\tif (!info)\n>>>> \t\treturn;\n>>>> \n>>>> -\t/* Mark the request as complete. */\n>>>> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n>>>> \t/* \\todo Move the ExposureTime control to the IPA. */\n>>>> \trequest->metadata().set(controls::ExposureTime, exposureTime_);\n>>>> @@ -1081,7 +1117,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>>>> \t\tRectangle cropRegion = request->controls().get(controls::ScalerCrop);\n>>>> \t\trequest->metadata().set(controls::ScalerCrop, cropRegion);\n>>>> \t}\n>>>> -\tpipe_->completeRequest(request);\n>>>> +\n>>>> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n>>>> +\t\tinfo->metadataProcessed = true;\n>>>> +\n>>>> +\tif (frameInfos_.tryComplete(info))\n>>>> +\t\tpipe_->completeRequest(request);\n>>>> }\n>>>> \n>>>> /**\n>>>> @@ -1093,26 +1134,60 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>>>> */\n>>>> void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>>>> {\n>>>> -\t/* \\todo Handle buffer failures when state is set to BufferError. */\n>>>> -\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n>>>> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n>>>> +\tif (!info)\n>>>> \t\treturn;\n>>>> \n>>>> \tRequest *request = buffer->request();\n>>>> \n>>>> -\t/*\n>>>> -\t * If the request contains a buffer for the RAW stream only, complete it\n>>>> -\t * now as there's no need for ImgU processing.\n>>>> -\t */\n>>>> -\tif (request->findBuffer(&rawStream_)) {\n>>>> -\t\tbool isComplete = pipe_->completeBuffer(request, buffer);\n>>>> -\t\tif (isComplete) {\n>>>> -\t\t\trequest->metadata().set(controls::draft::PipelineDepth, 2);\n>>>> -\t\t\tpipe_->completeRequest(request);\n>>>> -\t\t\treturn;\n>>>> -\t\t}\n>>>> +\tif (request->findBuffer(&rawStream_))\n>>>> +\t\tpipe_->completeBuffer(request, buffer);\n>>>\n>>> If you moved this after the next if (...) { } block, you could...\n>>>\n>>>> +\n>>>> +\t/* If the buffer is cancelled force a complete of the whole request. */\n>>>> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n>>>> +\t\tfor (auto it : request->buffers())\n>>>> +\t\t\tif (it.second != buffer)\n>>>\n>>> ... drop this check.\n>>\n>> Good point.\n>>\n>>>\n>>>> +\t\t\t\tpipe_->completeBuffer(request, it.second);\n>>>> +\n>>>> +\t\tinfo->paramDequeued = true;\n>>>> +\t\tinfo->metadataProcessed = true;\n>>>> +\t\tASSERT(frameInfos_.tryComplete(info));\n>>>> +\t\tpipe_->completeRequest(request);\n>>>> +\n>>>> +\t\treturn;\n>>>> \t}\n>>>> \n>>>> -\timgu_->input_->queueBuffer(buffer);\n>>>> +\tIPAOperationData op;\n>>>> +\top.operation = IPU3_IPA_EVENT_FILL_PARAMS;\n>>>> +\top.data = { info->id, info->paramBuffer->cookie() };\n>>>> +\top.controls = { request->controls() };\n>>>\n>>> This is the right time to call IPU3_IPA_EVENT_FILL_PARAMS, but the\n>>> request controls need to be passed to the IPA earlier, right when the\n>>> request is queued, as the IPA needs to look ahead in the request queue\n>>> to compute parameters.\n>>>\n>>> This is the main issue in this patch, the rest is fairly minor.\n>>\n>> I see your point I will fix this for v4.\n>>\n>>>\n>>>> +\tipa_->processEvent(op);\n>>>> +}\n>>>> +\n>>>> +void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n>>>> +{\n>>>> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n>>>> +\tif (!info)\n>>>> +\t\treturn;\n>>>> +\n>>>> +\tinfo->paramDequeued = true;\n>>>> +\tif (frameInfos_.tryComplete(info))\n>>>> +\t\tpipe_->completeRequest(buffer->request());\n>>>> +}\n>>>> +\n>>>> +void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>>>> +{\n>>>> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n>>>> +\t\treturn;\n>>>\n>>> Why do we need to check for the status for stats buffers but not for\n>>> params buffers ?\n>>\n>> Because there is no point in giving a cancelled statistics buffer to the \n>> IPA. While for the param all we really care about is marking the buffer \n>> free and moving on. But this error path can't have been tested it should \n>> set info->metadataProcessed = true and try to complete the request. Will \n>> fix for v4.\n>>\n>>>\n>>>> +\n>>>> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n>>>> +\tif (!info)\n>>>> +\t\treturn;\n>>>> +\n>>>> +\tIPAOperationData op;\n>>>> +\top.operation = IPU3_IPA_EVENT_STAT_READY;\n>>>> +\top.data = { info->id, info->statBuffer->cookie() };\n>>>> +\tipa_->processEvent(op);\n>>>> }\n>>>> \n>>>> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)\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 1A152BD160\n\tfor ;\n\tFri, 5 Feb 2021 10:34:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F0FA614B1;\n\tFri, 5 Feb 2021 11:34:20 +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 1549860106\n\tfor ;\n\tFri, 5 Feb 2021 11:34:20 +0100 (CET)","from [192.168.0.20]\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 73403316;\n\tFri, 5 Feb 2021 11:34:19 +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=\"BEBzQ6hk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612521259;\n\tbh=5040e7Bo8InDmLRv+iD1xu2bNvROw+VAyKqsUmyejvw=;\n\th=Reply-To:Subject:From:To:Cc:References:Date:In-Reply-To:From;\n\tb=BEBzQ6hkREj4BVa9ODEYxvgInWT2iUNMQPh2sMNzByc6rdpcQLBxnI8bgdsOwBeX8\n\tW3eG0rgr6W6B1SCqsv3JGyFllnVjJKoPNzaq2tPyH9sdfjr8wTOBdwgVOgyLtAdfOv\n\t4uTNqayK7Ns1llA6ADK26BHXb0j+buwPAJabOmeM=","From":"Kieran Bingham ","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tLaurent Pinchart ","References":"<20210204162943.268517-1-niklas.soderlund@ragnatech.se>\n\t<20210204162943.268517-12-niklas.soderlund@ragnatech.se>\n\t\n\t\n\t<6f417b60-9dc3-173a-29a6-a7aaecffa06a@ideasonboard.com>","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<4088ff56-88f0-a998-fffc-e354b54d6b6f@ideasonboard.com>","Date":"Fri, 5 Feb 2021 10:34:17 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<6f417b60-9dc3-173a-29a6-a7aaecffa06a@ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 11/11] libcamera: ipu3: Share\n\tparameter and statistic buffers with IPA","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]