[{"id":14470,"web_url":"https://patchwork.libcamera.org/comment/14470/","msgid":"<20210107162817.zogxas47odaanurk@uno.localdomain>","date":"2021-01-07T16:28:17","subject":"Re: [libcamera-devel] [PATCH v2 11/11] libcamera: ipu3: Share\n\tparameter and statistic buffers with IPA","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Tue, Dec 29, 2020 at 05:03:18PM +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> src/libcamera/pipeline/ipu3/ipu3.cpp | 115 +++++++++++++++++++++++++--\n> 1 file changed, 107 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 141066c528890c8e..6cd1879a76b195bf 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> @@ -60,6 +61,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> @@ -69,6 +72,7 @@ public:\n> \tStream rawStream_;\n>\n> \tstd::unique_ptr delayedCtrls_;\n> +\tstd::unique_ptr frameInfos_;\n>\n> private:\n> \tvoid actOnIpa(unsigned int id, const IPAOperationData &op);\n> @@ -602,6 +606,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> @@ -609,6 +615,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> @@ -720,6 +728,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> \tIPU3CameraData *data = cameraData(camera);\n> \tint error = 0;\n>\n> +\tIPU3Frames::Info *info = data->frameInfos_->create(request);\n> +\tif (!info)\n> +\t\treturn -ENOENT;\n> +\n> \t/*\n> \t * Queue a buffer on the CIO2, using the raw stream buffer provided in\n> \t * the request, if any, or a CIO2 internal buffer otherwise.\n> @@ -729,7 +741,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> \tif (!rawBuffer)\n> \t\treturn -ENOMEM;\n>\n> +\tinfo->rawBuffer = rawBuffer;\n> +\n> \t/* Queue all buffers from the request aimed for the ImgU. */\n> +\tbool onlyRaw = true;\n> \tfor (auto it : request->buffers()) {\n> \t\tconst Stream *stream = it.first;\n> \t\tFrameBuffer *buffer = it.second;\n> @@ -744,6 +759,23 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n>\n> \t\tif (ret < 0)\n> \t\t\terror = ret;\n> +\n> +\t\tonlyRaw = false;\n> +\t}\n> +\n> +\t/* If request only contains a raw buffer do not involve IPA. */\n> +\tif (onlyRaw) {\n> +\t\tinfo->paramDequeued = true;\n> +\t\tinfo->metadataProcessed = true;\n> +\t} else {\n> +\t\tIPAOperationData op;\n> +\t\top.operation = IPU3_IPA_EVENT_FILL_PARAMS;\n> +\t\top.data = { info->id, info->paramBuffer->cookie() };\n> +\t\top.controls = { request->controls() };\n\nI'm a bit skeptical about this one. I would leave controls out of the\nIPA interface for the moment. We have towards the API a mixed\nv4l2/libcamera controls interface which might require a second\nthought, and not all the controls have to be sent to the IPA. I would\ndrop this for the moment.\n\n> +\t\tdata->ipa_->processEvent(op);\n> +\n> +\t\tdata->imgu_->param_->queueBuffer(info->paramBuffer);\n> +\t\tdata->imgu_->stat_->queueBuffer(info->statBuffer);\n> \t}\n>\n> \treturn error;\n> @@ -893,6 +925,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> @@ -922,10 +958,12 @@ int IPU3CameraData::loadIPA()\n>\n> \tipa_->init(IPASettings{});\n>\n> +\tframeInfos_ = std::make_unique();\n> +\n> \treturn 0;\n> }\n>\n> -void IPU3CameraData::actOnIpa([[maybe_unused]] unsigned int id,\n> +void IPU3CameraData::actOnIpa(unsigned int id,\n> \t\t\t const IPAOperationData &action)\n> {\n> \tswitch (action.operation) {\n> @@ -934,6 +972,27 @@ void IPU3CameraData::actOnIpa([[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\tinfo->paramFilled = true;\n> +\t\tbreak;\n> +\t}\n> +\tcase IPU3_IPA_ACTION_METADATA_READY: {\n\nThe corresponding action when notifying IPA about tha availability of\nthe metadata buffer is called IPU3_IPA_EVENT_STAT_READY. The ipa uses\nthe terms 'parseStatistics'. Can we stabilize on one name, being it\nmetadata, STATS or Statistics ?\n\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> @@ -954,13 +1013,16 @@ 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> -\tpipe_->completeRequest(request);\n> +\n> +\tif (frameInfos_->tryComplete(info))\n> +\t\tpipe_->completeRequest(request);\n> }\n>\n> /**\n> @@ -976,6 +1038,10 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> \t\treturn;\n>\n> +\tIPU3Frames::Info *info = frameInfos_->find(buffer);\n> +\tif (!info)\n> +\t\treturn;\n> +\n> \tRequest *request = buffer->request();\n>\n> \t/*\n> @@ -983,17 +1049,50 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\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\nI would be kept it. We're familiar with the fact completeBuffer()\nreturn 1 if the request is complete, but it's not intuitive and\n'isComplete' helps clarifying it.\n\n> +\t\tif (pipe_->completeBuffer(request, buffer)) {\n> \t\t\trequest->metadata().set(controls::draft::PipelineDepth, 2);\n> -\t\t\tpipe_->completeRequest(request);\n> +\t\t\tif (frameInfos_->tryComplete(info))\n> +\t\t\t\tpipe_->completeRequest(request);\n> \t\t\treturn;\n> \t\t}\n> \t}\n>\n> +\tif (!info->paramFilled)\n> +\t\tLOG(IPU3, Info)\n> +\t\t\t<< \"Parameters not ready on time for id \" << info->id;\n\nCan this happen ?\n\nCurrently it's not super clear to me how parameters are expected to be\nelaborated by the IPA, we currently send the IPU3_IPA_EVENT_FILL_PARAMS\nin queueRequestDevice(), which immediately returns the\nIPU3_IPA_ACTION_PARAM_FILLED to the pipeline (and a set of sensor\ncontrols immediately after).\n\nIsn't actually the parsing of the metadata produced by a capture,\ncombined with the user controls, that should generate the new set of\nparameters ?\n\nIt does not make any difference here actually as they're empty.\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\n> \timgu_->input_->queueBuffer(buffer);\n> }\n>\n> +void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> +{\n> +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> +\t\treturn;\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> +\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> } /* namespace libcamera */\n> --\n> 2.29.2\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 EAE92C0F1A\n\tfor ;\n\tThu, 7 Jan 2021 16:28:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7CA036345C;\n\tThu, 7 Jan 2021 17:28:04 +0100 (CET)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7ECDA62013\n\tfor ;\n\tThu, 7 Jan 2021 17:28:03 +0100 (CET)","from uno.localdomain (mob-2-41-226-44.net.vodafone.it\n\t[2.41.226.44]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 865BF60018;\n\tThu, 7 Jan 2021 16:28:02 +0000 (UTC)"],"X-Originating-IP":"2.41.226.44","Date":"Thu, 7 Jan 2021 17:28:17 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20210107162817.zogxas47odaanurk@uno.localdomain>","References":"<20201229160318.77536-1-niklas.soderlund@ragnatech.se>\n\t<20201229160318.77536-12-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201229160318.77536-12-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 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":14499,"web_url":"https://patchwork.libcamera.org/comment/14499/","msgid":"","date":"2021-01-10T20:59:57","subject":"Re: [libcamera-devel] [PATCH v2 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, Jan 07, 2021 at 05:28:17PM +0100, Jacopo Mondi wrote:\n> On Tue, Dec 29, 2020 at 05:03:18PM +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> > src/libcamera/pipeline/ipu3/ipu3.cpp | 115 +++++++++++++++++++++++++--\n> > 1 file changed, 107 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 141066c528890c8e..6cd1879a76b195bf 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> > @@ -60,6 +61,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> > @@ -69,6 +72,7 @@ public:\n> > \tStream rawStream_;\n> >\n> > \tstd::unique_ptr delayedCtrls_;\n> > +\tstd::unique_ptr frameInfos_;\n\nCan this be embedded instead of allocated dynamically ?\n\n> >\n> > private:\n> > \tvoid actOnIpa(unsigned int id, const IPAOperationData &op);\n> > @@ -602,6 +606,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> > @@ -609,6 +615,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> > @@ -720,6 +728,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > \tIPU3CameraData *data = cameraData(camera);\n> > \tint error = 0;\n> >\n> > +\tIPU3Frames::Info *info = data->frameInfos_->create(request);\n> > +\tif (!info)\n> > +\t\treturn -ENOENT;\n\n-ENOBUFS ?\n\n> > +\n> > \t/*\n> > \t * Queue a buffer on the CIO2, using the raw stream buffer provided in\n> > \t * the request, if any, or a CIO2 internal buffer otherwise.\n> > @@ -729,7 +741,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > \tif (!rawBuffer)\n> > \t\treturn -ENOMEM;\n> >\n> > +\tinfo->rawBuffer = rawBuffer;\n> > +\n> > \t/* Queue all buffers from the request aimed for the ImgU. */\n> > +\tbool onlyRaw = true;\n> > \tfor (auto it : request->buffers()) {\n> > \t\tconst Stream *stream = it.first;\n> > \t\tFrameBuffer *buffer = it.second;\n> > @@ -744,6 +759,23 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> >\n> > \t\tif (ret < 0)\n> > \t\t\terror = ret;\n> > +\n> > +\t\tonlyRaw = false;\n> > +\t}\n> > +\n> > +\t/* If request only contains a raw buffer do not involve IPA. */\n> > +\tif (onlyRaw) {\n> > +\t\tinfo->paramDequeued = true;\n> > +\t\tinfo->metadataProcessed = true;\n\nI don't think that's right. Even if we only capture raw frames, the IPA\nhas to run or all the algorithms will stop.\n\n> > +\t} else {\n> > +\t\tIPAOperationData op;\n> > +\t\top.operation = IPU3_IPA_EVENT_FILL_PARAMS;\n> > +\t\top.data = { info->id, info->paramBuffer->cookie() };\n> > +\t\top.controls = { request->controls() };\n> \n> I'm a bit skeptical about this one. I would leave controls out of the\n> IPA interface for the moment. We have towards the API a mixed\n> v4l2/libcamera controls interface which might require a second\n> thought, and not all the controls have to be sent to the IPA. I would\n> drop this for the moment.\n\nI'm not sure to agree here. The controls that are received as part of\nthe request are all libcamera controls, and the IPA needs them. The\nseparate set of controls for the sensor is currently using V4L2 controls\nand that will change, but it's separate from the controls stored in the\nrequest.\n\n> > +\t\tdata->ipa_->processEvent(op);\n> > +\n> > +\t\tdata->imgu_->param_->queueBuffer(info->paramBuffer);\n\nThat's not right. The processEvent() call is not synchronous, the\nparameters buffer will be filled later, you're queuing an empty buffer\nhere.\n\nFurthermore, even if processEvent() was synchronous, you would end up\nrequesting ImgU parameters too early if the application has a large\nqueue of requests.\n\nControls need to be sent to the IPA as soon as the request is received,\nbut ISP parameters should be requested only when we get close to needing\nthem.\n\n> > +\t\tdata->imgu_->stat_->queueBuffer(info->statBuffer);\n> > \t}\n> >\n> > \treturn error;\n> > @@ -893,6 +925,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> > @@ -922,10 +958,12 @@ int IPU3CameraData::loadIPA()\n> >\n> > \tipa_->init(IPASettings{});\n> >\n> > +\tframeInfos_ = std::make_unique();\n> > +\n> > \treturn 0;\n> > }\n> >\n> > -void IPU3CameraData::actOnIpa([[maybe_unused]] unsigned int id,\n> > +void IPU3CameraData::actOnIpa(unsigned int id,\n> > \t\t\t const IPAOperationData &action)\n> > {\n> > \tswitch (action.operation) {\n> > @@ -934,6 +972,27 @@ void IPU3CameraData::actOnIpa([[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\tinfo->paramFilled = true;\n> > +\t\tbreak;\n> > +\t}\n> > +\tcase IPU3_IPA_ACTION_METADATA_READY: {\n> \n> The corresponding action when notifying IPA about tha availability of\n> the metadata buffer is called IPU3_IPA_EVENT_STAT_READY. The ipa uses\n> the terms 'parseStatistics'. Can we stabilize on one name, being it\n> metadata, STATS or Statistics ?\n\nIsn't \"statistics\" (or \"stats\") used for statistics computed by the ImgU\nand sent by the pipeline handler to the IPA, and \"metadata\" used for\nrequest metadata computed by the IPA and sent by the IPA to the pipeline\nhandler ?\n\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> > @@ -954,13 +1013,16 @@ 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> > -\tpipe_->completeRequest(request);\n> > +\n> > +\tif (frameInfos_->tryComplete(info))\n> > +\t\tpipe_->completeRequest(request);\n> > }\n> >\n> > /**\n> > @@ -976,6 +1038,10 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> > \t\treturn;\n> >\n> > +\tIPU3Frames::Info *info = frameInfos_->find(buffer);\n> > +\tif (!info)\n> > +\t\treturn;\n> > +\n> > \tRequest *request = buffer->request();\n> >\n> > \t/*\n> > @@ -983,17 +1049,50 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\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> \n> I would be kept it. We're familiar with the fact completeBuffer()\n> return 1 if the request is complete, but it's not intuitive and\n> 'isComplete' helps clarifying it.\n\nDo you think we should add\n\n\tenum class RequestStatus {\n\t\tRequestPending,\n\t\tRequestComplete,\n\t};\n\nto the PipelineHandler class, and use it as the return type for\ncompleteBuffer() ? This could would then become\n\n\t\tif (pipe_->completeBuffer(request, buffer) == RequestComplete) {\n\nIt's more explicit, at the cost of being longer.\n\n> > +\t\tif (pipe_->completeBuffer(request, buffer)) {\n> > \t\t\trequest->metadata().set(controls::draft::PipelineDepth, 2);\n> > -\t\t\tpipe_->completeRequest(request);\n> > +\t\t\tif (frameInfos_->tryComplete(info))\n> > +\t\t\t\tpipe_->completeRequest(request);\n> > \t\t\treturn;\n> > \t\t}\n> > \t}\n> >\n> > +\tif (!info->paramFilled)\n> > +\t\tLOG(IPU3, Info)\n> > +\t\t\t<< \"Parameters not ready on time for id \" << info->id;\n> \n> Can this happen ?\n> \n> Currently it's not super clear to me how parameters are expected to be\n> elaborated by the IPA, we currently send the IPU3_IPA_EVENT_FILL_PARAMS\n> in queueRequestDevice(), which immediately returns the\n> IPU3_IPA_ACTION_PARAM_FILLED to the pipeline (and a set of sensor\n> controls immediately after).\n> \n> Isn't actually the parsing of the metadata produced by a capture,\n> combined with the user controls, that should generate the new set of\n> parameters ?\n> \n> It does not make any difference here actually as they're empty.\n> \n> Reviewed-by: Jacopo Mondi \n> \n> > +\n> > \timgu_->input_->queueBuffer(buffer);\n> > }\n> >\n> > +void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> > +{\n> > +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> > +\t\treturn;\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> > +\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> > } /* namespace libcamera */","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 8E34FBD80C\n\tfor ;\n\tSun, 10 Jan 2021 21:00:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 022BE68074;\n\tSun, 10 Jan 2021 22:00:13 +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 EB2CE60523\n\tfor ;\n\tSun, 10 Jan 2021 22:00:11 +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 4DB77DA;\n\tSun, 10 Jan 2021 22:00:11 +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=\"Yx4AKyRY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1610312411;\n\tbh=43IoS8hYYJeZSPZSmK4AXf5HanSWBnL+rAKNeCUQ1Qc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Yx4AKyRY8hVZKnrVn6HFzYAMXBYkEeSqp593DgI6pCUHhaForCjlh/jVD04JJX9Sv\n\tMb3KtvuEjdlLi73hakdjHzLqv5+5oWjyEw/9AFsteMhStp9kJQbthbRhszHSH8HVC0\n\taNtuaJvRk75kJymuHFJIUB6uTfOcCQnJM6voyQHY=","Date":"Sun, 10 Jan 2021 22:59:57 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20201229160318.77536-1-niklas.soderlund@ragnatech.se>\n\t<20201229160318.77536-12-niklas.soderlund@ragnatech.se>\n\t<20210107162817.zogxas47odaanurk@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210107162817.zogxas47odaanurk@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 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":14927,"web_url":"https://patchwork.libcamera.org/comment/14927/","msgid":"","date":"2021-02-03T15:04:43","subject":"Re: [libcamera-devel] [PATCH v2 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 Jacopo and Laurent,\n\nThanks for your feedback.\n\nOn 2021-01-10 22:59:57 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Thu, Jan 07, 2021 at 05:28:17PM +0100, Jacopo Mondi wrote:\n> > On Tue, Dec 29, 2020 at 05:03:18PM +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> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 115 +++++++++++++++++++++++++--\n> > > 1 file changed, 107 insertions(+), 8 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 141066c528890c8e..6cd1879a76b195bf 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> > > @@ -60,6 +61,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> > > @@ -69,6 +72,7 @@ public:\n> > > \tStream rawStream_;\n> > >\n> > > \tstd::unique_ptr delayedCtrls_;\n> > > +\tstd::unique_ptr frameInfos_;\n> \n> Can this be embedded instead of allocated dynamically ?\n\nGood point, yes it can.\n\n> \n> > >\n> > > private:\n> > > \tvoid actOnIpa(unsigned int id, const IPAOperationData &op);\n> > > @@ -602,6 +606,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> > > @@ -609,6 +615,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> > > @@ -720,6 +728,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > > \tIPU3CameraData *data = cameraData(camera);\n> > > \tint error = 0;\n> > >\n> > > +\tIPU3Frames::Info *info = data->frameInfos_->create(request);\n> > > +\tif (!info)\n> > > +\t\treturn -ENOENT;\n> \n> -ENOBUFS ?\n\nSure.\n\n> \n> > > +\n> > > \t/*\n> > > \t * Queue a buffer on the CIO2, using the raw stream buffer provided in\n> > > \t * the request, if any, or a CIO2 internal buffer otherwise.\n> > > @@ -729,7 +741,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > > \tif (!rawBuffer)\n> > > \t\treturn -ENOMEM;\n> > >\n> > > +\tinfo->rawBuffer = rawBuffer;\n> > > +\n> > > \t/* Queue all buffers from the request aimed for the ImgU. */\n> > > +\tbool onlyRaw = true;\n> > > \tfor (auto it : request->buffers()) {\n> > > \t\tconst Stream *stream = it.first;\n> > > \t\tFrameBuffer *buffer = it.second;\n> > > @@ -744,6 +759,23 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> > >\n> > > \t\tif (ret < 0)\n> > > \t\t\terror = ret;\n> > > +\n> > > +\t\tonlyRaw = false;\n> > > +\t}\n> > > +\n> > > +\t/* If request only contains a raw buffer do not involve IPA. */\n> > > +\tif (onlyRaw) {\n> > > +\t\tinfo->paramDequeued = true;\n> > > +\t\tinfo->metadataProcessed = true;\n> \n> I don't think that's right. Even if we only capture raw frames, the IPA\n> has to run or all the algorithms will stop.\n\nUnderstood this whole logic will need to be reworked for v2 anyway.\n\n> \n> > > +\t} else {\n> > > +\t\tIPAOperationData op;\n> > > +\t\top.operation = IPU3_IPA_EVENT_FILL_PARAMS;\n> > > +\t\top.data = { info->id, info->paramBuffer->cookie() };\n> > > +\t\top.controls = { request->controls() };\n> > \n> > I'm a bit skeptical about this one. I would leave controls out of the\n> > IPA interface for the moment. We have towards the API a mixed\n> > v4l2/libcamera controls interface which might require a second\n> > thought, and not all the controls have to be sent to the IPA. I would\n> > drop this for the moment.\n> \n> I'm not sure to agree here. The controls that are received as part of\n> the request are all libcamera controls, and the IPA needs them. The\n> separate set of controls for the sensor is currently using V4L2 controls\n> and that will change, but it's separate from the controls stored in the\n> request.\n> \n> > > +\t\tdata->ipa_->processEvent(op);\n> > > +\n> > > +\t\tdata->imgu_->param_->queueBuffer(info->paramBuffer);\n> \n> That's not right. The processEvent() call is not synchronous, the\n> parameters buffer will be filled later, you're queuing an empty buffer\n> here.\n> \n> Furthermore, even if processEvent() was synchronous, you would end up\n> requesting ImgU parameters too early if the application has a large\n> queue of requests.\n> \n> Controls need to be sent to the IPA as soon as the request is received,\n> but ISP parameters should be requested only when we get close to needing\n> them.\n\nAs discussed offline this will all be reworked for next version. It will \nnot get us all the way but a good step forward in the right direction.\n\n> \n> > > +\t\tdata->imgu_->stat_->queueBuffer(info->statBuffer);\n> > > \t}\n> > >\n> > > \treturn error;\n> > > @@ -893,6 +925,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> > > @@ -922,10 +958,12 @@ int IPU3CameraData::loadIPA()\n> > >\n> > > \tipa_->init(IPASettings{});\n> > >\n> > > +\tframeInfos_ = std::make_unique();\n> > > +\n> > > \treturn 0;\n> > > }\n> > >\n> > > -void IPU3CameraData::actOnIpa([[maybe_unused]] unsigned int id,\n> > > +void IPU3CameraData::actOnIpa(unsigned int id,\n> > > \t\t\t const IPAOperationData &action)\n> > > {\n> > > \tswitch (action.operation) {\n> > > @@ -934,6 +972,27 @@ void IPU3CameraData::actOnIpa([[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\tinfo->paramFilled = true;\n> > > +\t\tbreak;\n> > > +\t}\n> > > +\tcase IPU3_IPA_ACTION_METADATA_READY: {\n> > \n> > The corresponding action when notifying IPA about tha availability of\n> > the metadata buffer is called IPU3_IPA_EVENT_STAT_READY. The ipa uses\n> > the terms 'parseStatistics'. Can we stabilize on one name, being it\n> > metadata, STATS or Statistics ?\n> \n> Isn't \"statistics\" (or \"stats\") used for statistics computed by the ImgU\n> and sent by the pipeline handler to the IPA, and \"metadata\" used for\n> request metadata computed by the IPA and sent by the IPA to the pipeline\n> handler ?\n\nYes.\n\n> \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> > > @@ -954,13 +1013,16 @@ 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> > > -\tpipe_->completeRequest(request);\n> > > +\n> > > +\tif (frameInfos_->tryComplete(info))\n> > > +\t\tpipe_->completeRequest(request);\n> > > }\n> > >\n> > > /**\n> > > @@ -976,6 +1038,10 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> > > \t\treturn;\n> > >\n> > > +\tIPU3Frames::Info *info = frameInfos_->find(buffer);\n> > > +\tif (!info)\n> > > +\t\treturn;\n> > > +\n> > > \tRequest *request = buffer->request();\n> > >\n> > > \t/*\n> > > @@ -983,17 +1049,50 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\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> > \n> > I would be kept it. We're familiar with the fact completeBuffer()\n> > return 1 if the request is complete, but it's not intuitive and\n> > 'isComplete' helps clarifying it.\n> \n> Do you think we should add\n> \n> \tenum class RequestStatus {\n> \t\tRequestPending,\n> \t\tRequestComplete,\n> \t};\n> \n> to the PipelineHandler class, and use it as the return type for\n> completeBuffer() ? This could would then become\n> \n> \t\tif (pipe_->completeBuffer(request, buffer) == RequestComplete) {\n> \n> It's more explicit, at the cost of being longer.\n\nI don't like the class and to no surprise I prefer what is done in this \npatch ;-) I will settle on the middle ground and restore the isComplete \nso we can bikeshed about this in a different series.\n\n> \n> > > +\t\tif (pipe_->completeBuffer(request, buffer)) {\n> > > \t\t\trequest->metadata().set(controls::draft::PipelineDepth, 2);\n> > > -\t\t\tpipe_->completeRequest(request);\n> > > +\t\t\tif (frameInfos_->tryComplete(info))\n> > > +\t\t\t\tpipe_->completeRequest(request);\n> > > \t\t\treturn;\n> > > \t\t}\n> > > \t}\n> > >\n> > > +\tif (!info->paramFilled)\n> > > +\t\tLOG(IPU3, Info)\n> > > +\t\t\t<< \"Parameters not ready on time for id \" << info->id;\n> > \n> > Can this happen ?\n\nYes, if the IPA is to slow signaling it has processed the param buffer. \nThis race between the IPA and V4L2 will however be reworked for next \nversion so hopefully this can't happen after v3 :-)\n\n> > \n> > Currently it's not super clear to me how parameters are expected to be\n> > elaborated by the IPA, we currently send the IPU3_IPA_EVENT_FILL_PARAMS\n> > in queueRequestDevice(), which immediately returns the\n> > IPU3_IPA_ACTION_PARAM_FILLED to the pipeline (and a set of sensor\n> > controls immediately after).\n> > \n> > Isn't actually the parsing of the metadata produced by a capture,\n> > combined with the user controls, that should generate the new set of\n> > parameters ?\n> > \n> > It does not make any difference here actually as they're empty.\n> > \n> > Reviewed-by: Jacopo Mondi \n\nThank you. As I need to rework much of this patch I will not be able to \ncollect your tag.\n\n> > \n> > > +\n> > > \timgu_->input_->queueBuffer(buffer);\n> > > }\n> > >\n> > > +void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)\n> > > +{\n> > > +\tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> > > +\t\treturn;\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> > > +\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> > > } /* namespace libcamera */\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 50BA2BD160\n\tfor ;\n\tWed, 3 Feb 2021 15:04:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D12C16844C;\n\tWed, 3 Feb 2021 16:04:47 +0100 (CET)","from mail-ej1-x636.google.com (mail-ej1-x636.google.com\n\t[IPv6:2a00:1450:4864:20::636])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48C2C683FE\n\tfor ;\n\tWed, 3 Feb 2021 16:04:46 +0100 (CET)","by mail-ej1-x636.google.com with SMTP id bl23so36287137ejb.5\n\tfor ;\n\tWed, 03 Feb 2021 07:04:46 -0800 (PST)","from localhost (p4fca2458.dip0.t-ipconnect.de. [79.202.36.88])\n\tby smtp.gmail.com with ESMTPSA id\n\tl17sm1122886ejc.60.2021.02.03.07.04.43\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 03 Feb 2021 07:04:44 -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=\"If45MZau\"; 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=9u54NJDAkIfR7zwPTming29ZRDOBRjnCLyJzun1wtIQ=;\n\tb=If45MZaul4zpe16zv3zrJv04Ljxs5a+EcCtxgb/+kxs4If7Hx8IrUsDXFFV6jSLY8X\n\tHSmTt+sESUTxRCB3w7bUh2Lxr36BmG3GTcgDPHYDv0Mjh4xFDi1u9D6BmozqGzmGOhPQ\n\tdsPYKSoTaZ7nggxcb2UTmARg1xj0V8bJZdQ0B1wP4NzDlLCfUg/TxSbo97nAYuVNYGOT\n\te4gc1/7dh9VHMfWZhr7lGCYxjj1k3c5w28IWGbBNkfFte8nQkSm0MxrI4Uis6WtSqtnX\n\t7Ub2K+g92/uGwh9Hr54bYAbV4FyilsQIckUpZgkvpL9GMgN+/2WkOByolSMTdVg6sdDT\n\tAz8Q==","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=9u54NJDAkIfR7zwPTming29ZRDOBRjnCLyJzun1wtIQ=;\n\tb=h536IgLeZ8aeCIuB7vR9NOTu3Q78pz628rKJyD88Mv7KS5qKt4I3k9fNCRGCdyrH3W\n\teX2ZCQLqYpY4r3jsHFlGS4XhBwhihPRZb7q2zZHCE1nB+XDrPyISQW6tZnevMYyyPhdP\n\tMZ2B+Mrb5W0N4o04aA8NB9CQwfS9RmvOO2wFYhKUIAPxOOQI+on5HusyKgY+ExwLxxf+\n\tGdwdIDaU62mJ2n7xawafd8h0tzTqfavTy8c10zsxfzr1bpixjdCQzAQt6l6ECVPjX65y\n\tkdKyab17Ct1yVr72rPj/LuFy2fCLh0Kuc4J9I15WxT7Y5Aa/G7kRi94G+CJQhUICAUP+\n\trEvg==","X-Gm-Message-State":"AOAM533Knmx/Xd8/FUw1TYdE0vsfmqFXgMWxlb8JdLKRXo7zjv0qtB/o\n\taIzaI4Me/BGmEVEsdBl4TDvWiQ==","X-Google-Smtp-Source":"ABdhPJxWXIxE1+C7uNpmDM7EdgYzqnra1JL5yha/z6xuuPY8yyXy9Y0Lj30lz8/B6Z+jXGgJKaMsQg==","X-Received":"by 2002:a17:906:1f45:: with SMTP id\n\td5mr3717494ejk.76.1612364685092; \n\tWed, 03 Feb 2021 07:04:45 -0800 (PST)","Date":"Wed, 3 Feb 2021 16:04:43 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ,\n\tJacopo Mondi ","Message-ID":"","References":"<20201229160318.77536-1-niklas.soderlund@ragnatech.se>\n\t<20201229160318.77536-12-niklas.soderlund@ragnatech.se>\n\t<20210107162817.zogxas47odaanurk@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 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\" "}}]