[{"id":15019,"web_url":"https://patchwork.libcamera.org/comment/15019/","msgid":"","date":"2021-02-04T23:47:03","subject":"Re: [libcamera-devel] [PATCH v4 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 Fri, Feb 05, 2021 at 12:26:13AM +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\nReviewed-by: Laurent Pinchart \n\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> * Changes since v3\n> - Fix error path in queueRequestDevice().\n> - Simplify error logic in cio2BufferReady().\n> - Handle canceled stat buffers correctly.\n> - Hand request parameters to IPA separatly from asking it to fill the\n> parameters buffer.\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 158 ++++++++++++++++++++-------\n> 1 file changed, 120 insertions(+), 38 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 092db6389833a481..c3763507c40a5d53 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> @@ -721,27 +732,20 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> \t */\n> \tFrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);\n> \tFrameBuffer *rawBuffer = data->cio2_.queueBuffer(request, reqRawBuffer);\n> -\tif (!rawBuffer)\n> +\tif (!rawBuffer) {\n> +\t\tdata->frameInfos_.remove(info);\n> \t\treturn -ENOMEM;\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> -\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> +\tinfo->rawBuffer = rawBuffer;\n> +\n> +\tIPAOperationData op;\n> +\top.operation = IPU3_IPA_EVENT_PROCESS_CONTROLS;\n> +\top.data = { info->id };\n> +\top.controls = { request->controls() };\n> +\tdata->ipa_->processEvent(op);\n> +\n> +\treturn 0;\n> }\n> \n> bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> @@ -1007,6 +1011,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 +1047,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 +1056,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> +\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> +\t\tbreak;\n> +\t}\n> \tdefault:\n> \t\tLOG(IPU3, Error) << \"Unknown action \" << action.operation;\n> \t\tbreak;\n> @@ -1068,11 +1111,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 +1125,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 +1142,59 @@ 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> +\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\tpipe_->completeBuffer(request, it.second);\n> +\n> +\t\tframeInfos_.remove(info);\n> +\t\tpipe_->completeRequest(request);\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (request->findBuffer(&rawStream_))\n> +\t\tpipe_->completeBuffer(request, buffer);\n> +\n> +\tIPAOperationData op;\n> +\top.operation = IPU3_IPA_EVENT_FILL_PARAMS;\n> +\top.data = { info->id, info->paramBuffer->cookie() };\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> +\tIPU3Frames::Info *info = frameInfos_.find(buffer);\n> +\tif (!info)\n> +\t\treturn;\n> +\n> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> +\t\tinfo->metadataProcessed = true;\n> +\t\tif (frameInfos_.tryComplete(info))\n> +\t\t\tpipe_->completeRequest(buffer->request());\n> +\t\treturn;\n> \t}\n> \n> -\timgu_->input_->queueBuffer(buffer);\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 AA854BD162\n\tfor ;\n\tThu, 4 Feb 2021 23:47:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 78246614AA;\n\tFri, 5 Feb 2021 00:47:27 +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 77AF06149B\n\tfor ;\n\tFri, 5 Feb 2021 00:47:26 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0B4CB31A;\n\tFri, 5 Feb 2021 00:47:25 +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=\"bOE70c0Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612482446;\n\tbh=NBCsxbUd2+Esa2PUGyUoWu3cPoBr3+TKUHQKIwMUx7c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bOE70c0YVObPQLe/ghcCpIRZ7TTj7b56kjcnQ0MijxHbyPqWEPT2iBgfQ1GkFBmQL\n\t2rbYwezK30SHvTheS50Wpbk786RVmr0CAmpcsy7CBQfP6u83qQsDhe8cxrjzaveKC+\n\txdsBRa8+752Ep0H7blLRgxwPKtQqBbMhdZijK8Bw=","Date":"Fri, 5 Feb 2021 01:47:03 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210204232613.494121-1-niklas.soderlund@ragnatech.se>\n\t<20210204232613.494121-12-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210204232613.494121-12-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v4 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\" "}}]