[{"id":28653,"web_url":"https://patchwork.libcamera.org/comment/28653/","msgid":"","date":"2024-02-13T12:46:36","subject":"Re: [PATCH 3/5] libcamera: rkisp1: Switch IPA slots to use bufferId\n\tnot frame","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Dan\n\nOn Mon, Feb 12, 2024 at 03:35:30PM +0000, Daniel Scally wrote:\n> In preparation for removing the RkISP1FrameInfo class we need the\n> RkISP1 IPA to emit bufferIds rather than frame numbers for its slots\n> so that the pipeline handler can find the correct buffer properly.\n>\n> Switch the slots; reconfigure the pipeline handler to obtain the\n> correct buffer first before passing it to RkISP1FrameInfo::find()\n> as before; one of its overloads already works with FrameBuffer *.\n>\n> Signed-off-by: Daniel Scally \n> ---\n> include/libcamera/ipa/rkisp1.mojom | 4 +-\n> src/ipa/rkisp1/rkisp1.cpp | 4 +-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 59 ++++++++++++++++--------\n> 3 files changed, 44 insertions(+), 23 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> index 1009e970..4c097622 100644\n> --- a/include/libcamera/ipa/rkisp1.mojom\n> +++ b/include/libcamera/ipa/rkisp1.mojom\n> @@ -36,7 +36,7 @@ interface IPARkISP1Interface {\n> };\n>\n> interface IPARkISP1EventInterface {\n> -\tparamsBufferReady(uint32 frame);\n> +\tparamsBufferReady(uint32 bufferId);\n> \tsetSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> -\tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n> +\tmetadataReady(uint32 bufferId, libcamera.ControlList metadata);\n> };\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 6544c925..7e01a04d 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -337,7 +337,7 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n\nI guess this should be ok. The parameters and stats buffers are\ninitialized with a cookie\n\n\tfor (std::unique_ptr &buffer : paramBuffers_) {\n\t\tbuffer->setCookie(ipaBufferId++);\n\nwhich is used to identify them on the IPA buffer map\n\n\trkisp1_params_cfg *params =\n\t\treinterpret_cast(\n\t\t\tmappedBuffers_.at(bufferId).planes()[0].data());\n\nThere shouldn't be any risk of the ids colliding as we're constantly\ncycling on the same set of buffers and the ownership of the frame\nbuffer doesn't change compared to the previous version if I'm not\nmistaken\n\n> \tfor (auto const &algo : algorithms())\n> \t\talgo->prepare(context_, frame, frameContext, params);\n>\n> -\tparamsBufferReady.emit(frame);\n> +\tparamsBufferReady.emit(bufferId);\n> }\n>\n> void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n> @@ -370,7 +370,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n>\n> \tsetControls(frame);\n>\n> -\tmetadataReady.emit(frame, metadata);\n> +\tmetadataReady.emit(bufferId, metadata);\n> }\n>\n> void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 22e553fe..d4ed38a4 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -376,23 +376,34 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> \treturn 0;\n> }\n>\n> -void RkISP1CameraData::paramFilled(unsigned int frame)\n> +void RkISP1CameraData::paramFilled(unsigned int bufferId)\n> {\n> \tPipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();\n> -\tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> -\tif (!info)\n> -\t\treturn;\n>\n> -\tinfo->paramBuffer->_d()->metadata().planes()[0].bytesused =\n> -\t\tsizeof(struct rkisp1_params_cfg);\n> -\tpipe->param_->queueBuffer(info->paramBuffer);\n> -\tpipe->stat_->queueBuffer(info->statBuffer);\n> +\tfor (std::unique_ptr ¶mBuffer : pipe->paramBuffers_) {\n> +\t\tif (paramBuffer->cookie() != bufferId)\n> +\t\t\tcontinue;\n> +\n> +\t\tRkISP1FrameInfo *info = frameInfo_.find(paramBuffer.get());\n> +\t\tif (!info)\n> +\t\t\treturn;\n> +\n> +\t\tinfo->paramBuffer->_d()->metadata().planes()[0].bytesused =\n> +\t\t\tsizeof(struct rkisp1_params_cfg);\n> +\t\tpipe->param_->queueBuffer(info->paramBuffer);\n> +\t\tpipe->stat_->queueBuffer(info->statBuffer);\n>\n> -\tif (info->mainPathBuffer)\n> -\t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> +\t\tif (info->mainPathBuffer)\n> +\t\t\tmainPath_->queueBuffer(info->mainPathBuffer);\n> +\n> +\t\tif (selfPath_ && info->selfPathBuffer)\n> +\t\t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> +\n> +\t\treturn;\n> +\t}\n>\n> -\tif (selfPath_ && info->selfPathBuffer)\n> -\t\tselfPath_->queueBuffer(info->selfPathBuffer);\n> +\tLOG(RkISP1, Fatal) << \"Can't locate buffer from bufferId\";\n> +\treturn;\n\nNo need for return\n\n> }\n>\n> void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> @@ -401,16 +412,26 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> \tdelayedCtrls_->push(sensorControls);\n> }\n>\n> -void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> +void RkISP1CameraData::metadataReady(unsigned int bufferId,\n> +\t\t\t\t const ControlList &metadata)\n> {\n> -\tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> -\tif (!info)\n> -\t\treturn;\n> +\tfor (std::unique_ptr &statBuffer : pipe()->statBuffers_) {\n> +\t\tif (statBuffer->cookie() != bufferId)\n> +\t\t\tcontinue;\n> +\n> +\t\tRkISP1FrameInfo *info = frameInfo_.find(statBuffer.get());\n> +\t\tif (!info)\n> +\t\t\treturn;\n>\n> -\tinfo->request->metadata().merge(metadata);\n> -\tinfo->metadataProcessed = true;\n> +\t\tinfo->request->metadata().merge(metadata);\n> +\t\tinfo->metadataProcessed = true;\n> +\n> +\t\tpipe()->tryCompleteRequest(info);\n> +\t\treturn;\n> +\t}\n>\n> -\tpipe()->tryCompleteRequest(info);\n> +\tLOG(RkISP1, Fatal) << \"Can't locate buffer from bufferId\";\n> +\treturn;\n\nsame here\n\n> }\n>\n> /* -----------------------------------------------------------------------------\n> --\n> 2.34.1\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 3E41EC3257\n\tfor ;\n\tTue, 13 Feb 2024 12:46:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75E5662805;\n\tTue, 13 Feb 2024 13:46:40 +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 50EB261CB8\n\tfor ;\n\tTue, 13 Feb 2024 13:46:39 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CE114675;\n\tTue, 13 Feb 2024 13:46:36 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Tky3GTO/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1707828396;\n\tbh=otdKJ4jzL1T0vNgpl4qQ6Uz5Xg3s5nFm4ndpNBUsK0U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Tky3GTO/QQYl5aaOhUrTVTprfbW6ad9Kr22Jj1JAUkHpy5JEFZ8ObA6QnVNgZMpbl\n\tiCbWWf6FEPHewa7XXX7aB0PGfzxlboY7aCqLBKNfkUd+Hx/lSSKnh1ayZLciUEt02Q\n\tt2kZRm7IUX2rOnL7pVv1yI+JwCE+hF3ofj56qdE4=","Date":"Tue, 13 Feb 2024 13:46:36 +0100","From":"Jacopo Mondi ","To":"Daniel Scally ","Subject":"Re: [PATCH 3/5] libcamera: rkisp1: Switch IPA slots to use bufferId\n\tnot frame","Message-ID":"","References":"<20240212153532.179283-1-dan.scally@ideasonboard.com>\n\t<20240212153532.179283-4-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240212153532.179283-4-dan.scally@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28655,"web_url":"https://patchwork.libcamera.org/comment/28655/","msgid":"<170783037879.2629073.3787555610004583142@ping.linuxembedded.co.uk>","date":"2024-02-13T13:19:38","subject":"Re: [PATCH 3/5] libcamera: rkisp1: Switch IPA slots to use bufferId\n\tnot frame","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2024-02-13 12:46:36)\n> Hi Dan\n> \n> On Mon, Feb 12, 2024 at 03:35:30PM +0000, Daniel Scally wrote:\n> > In preparation for removing the RkISP1FrameInfo class we need the\n> > RkISP1 IPA to emit bufferIds rather than frame numbers for its slots\n> > so that the pipeline handler can find the correct buffer properly.\n> >\n> > Switch the slots; reconfigure the pipeline handler to obtain the\n> > correct buffer first before passing it to RkISP1FrameInfo::find()\n> > as before; one of its overloads already works with FrameBuffer *.\n> >\n> > Signed-off-by: Daniel Scally \n> > ---\n> > include/libcamera/ipa/rkisp1.mojom | 4 +-\n> > src/ipa/rkisp1/rkisp1.cpp | 4 +-\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 59 ++++++++++++++++--------\n> > 3 files changed, 44 insertions(+), 23 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > index 1009e970..4c097622 100644\n> > --- a/include/libcamera/ipa/rkisp1.mojom\n> > +++ b/include/libcamera/ipa/rkisp1.mojom\n> > @@ -36,7 +36,7 @@ interface IPARkISP1Interface {\n> > };\n> >\n> > interface IPARkISP1EventInterface {\n> > - paramsBufferReady(uint32 frame);\n> > + paramsBufferReady(uint32 bufferId);\n> > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > - metadataReady(uint32 frame, libcamera.ControlList metadata);\n> > + metadataReady(uint32 bufferId, libcamera.ControlList metadata);\n> > };\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 6544c925..7e01a04d 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -337,7 +337,7 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> \n> I guess this should be ok. The parameters and stats buffers are\n> initialized with a cookie\n> \n> for (std::unique_ptr &buffer : paramBuffers_) {\n> buffer->setCookie(ipaBufferId++);\n> \n> which is used to identify them on the IPA buffer map\n> \n> rkisp1_params_cfg *params =\n> reinterpret_cast(\n> mappedBuffers_.at(bufferId).planes()[0].data());\n> \n> There shouldn't be any risk of the ids colliding as we're constantly\n> cycling on the same set of buffers and the ownership of the frame\n> buffer doesn't change compared to the previous version if I'm not\n> mistaken\n> \n> > for (auto const &algo : algorithms())\n> > algo->prepare(context_, frame, frameContext, params);\n> >\n> > - paramsBufferReady.emit(frame);\n> > + paramsBufferReady.emit(bufferId);\n> > }\n> >\n> > void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n> > @@ -370,7 +370,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> >\n> > setControls(frame);\n> >\n> > - metadataReady.emit(frame, metadata);\n> > + metadataReady.emit(bufferId, metadata);\n> > }\n> >\n> > void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 22e553fe..d4ed38a4 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -376,23 +376,34 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > return 0;\n> > }\n> >\n> > -void RkISP1CameraData::paramFilled(unsigned int frame)\n> > +void RkISP1CameraData::paramFilled(unsigned int bufferId)\n> > {\n> > PipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();\n> > - RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > - if (!info)\n> > - return;\n> >\n> > - info->paramBuffer->_d()->metadata().planes()[0].bytesused =\n> > - sizeof(struct rkisp1_params_cfg);\n> > - pipe->param_->queueBuffer(info->paramBuffer);\n> > - pipe->stat_->queueBuffer(info->statBuffer);\n> > + for (std::unique_ptr ¶mBuffer : pipe->paramBuffers_) {\n> > + if (paramBuffer->cookie() != bufferId)\n> > + continue;\n> > +\n> > + RkISP1FrameInfo *info = frameInfo_.find(paramBuffer.get());\n> > + if (!info)\n> > + return;\n> > +\n> > + info->paramBuffer->_d()->metadata().planes()[0].bytesused =\n> > + sizeof(struct rkisp1_params_cfg);\n> > + pipe->param_->queueBuffer(info->paramBuffer);\n> > + pipe->stat_->queueBuffer(info->statBuffer);\n> >\n> > - if (info->mainPathBuffer)\n> > - mainPath_->queueBuffer(info->mainPathBuffer);\n> > + if (info->mainPathBuffer)\n> > + mainPath_->queueBuffer(info->mainPathBuffer);\n> > +\n> > + if (selfPath_ && info->selfPathBuffer)\n> > + selfPath_->queueBuffer(info->selfPathBuffer);\n> > +\n> > + return;\n> > + }\n> >\n> > - if (selfPath_ && info->selfPathBuffer)\n> > - selfPath_->queueBuffer(info->selfPathBuffer);\n> > + LOG(RkISP1, Fatal) << \"Can't locate buffer from bufferId\";\n\nThese Fatal's in the param/stats handling worry me a little though. Can\nwe be absolutely sure that there should be no reason we can get here?\n\nI.e. Can this happen if say we drop some frames at runtime?\n\n\n\n> > + return;\n> \n> No need for return\n> \n> > }\n> >\n> > void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> > @@ -401,16 +412,26 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> > delayedCtrls_->push(sensorControls);\n> > }\n> >\n> > -void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > +void RkISP1CameraData::metadataReady(unsigned int bufferId,\n> > + const ControlList &metadata)\n> > {\n> > - RkISP1FrameInfo *info = frameInfo_.find(frame);\n> > - if (!info)\n> > - return;\n> > + for (std::unique_ptr &statBuffer : pipe()->statBuffers_) {\n> > + if (statBuffer->cookie() != bufferId)\n> > + continue;\n> > +\n> > + RkISP1FrameInfo *info = frameInfo_.find(statBuffer.get());\n> > + if (!info)\n> > + return;\n> >\n> > - info->request->metadata().merge(metadata);\n> > - info->metadataProcessed = true;\n> > + info->request->metadata().merge(metadata);\n> > + info->metadataProcessed = true;\n> > +\n> > + pipe()->tryCompleteRequest(info);\n> > + return;\n> > + }\n> >\n> > - pipe()->tryCompleteRequest(info);\n> > + LOG(RkISP1, Fatal) << \"Can't locate buffer from bufferId\";\n> > + return;\n> \n> same here\n> \n> > }\n> >\n> > /* -----------------------------------------------------------------------------\n> > --\n> > 2.34.1\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 71350BDE17\n\tfor ;\n\tTue, 13 Feb 2024 13:19:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CDD9362805;\n\tTue, 13 Feb 2024 14:19:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19CFE61CB8\n\tfor ;\n\tTue, 13 Feb 2024 14:19:42 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A3C0283F;\n\tTue, 13 Feb 2024 14:19:39 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aTyEgFS6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1707830379;\n\tbh=YPUnGyQ6hkFU8bNsr4+L1LY5ibZOLjSrAEv63wv/QnQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=aTyEgFS6qmG7YKO9JxBE3q6qjp3O5pZW3bU9srvtuXzLHEC3cwDdqpfzrrzyPaZgy\n\t0FbUKVtU0YgGkqT/bruqGP1adT/AKiKoHsHumU1MROYTcuejgR1W4LWZEW/ggxDcRY\n\t0zxx6y6PXRf6Nihah65XgSmD1PePQBI1DBkCJZds=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240212153532.179283-1-dan.scally@ideasonboard.com>\n\t<20240212153532.179283-4-dan.scally@ideasonboard.com>\n\t","Subject":"Re: [PATCH 3/5] libcamera: rkisp1: Switch IPA slots to use bufferId\n\tnot frame","From":"Kieran Bingham ","To":"Daniel Scally ,\n\tJacopo Mondi ","Date":"Tue, 13 Feb 2024 13:19:38 +0000","Message-ID":"<170783037879.2629073.3787555610004583142@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]