[{"id":13944,"web_url":"https://patchwork.libcamera.org/comment/13944/","msgid":"<20201127110510.j6fcbn6lpjaiazwb@uno.localdomain>","date":"2020-11-27T11:05:10","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: rkisp1:\n\tUse SOF event to warn about late parameters","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Nov 23, 2020 at 11:12:33PM +0100, Niklas Söderlund wrote:\n> In the Timeline approach the idea was to delay queuing buffers to the\n> device until the IPA had a chance to prepare the parameters buffer. A\n> check was still added to warn if the IPA queued buffers before the\n> parameters buffer was filled in.\n>\n> This check happened much sooner then needed as the parameter buffers\n> does not have to be ready when the buffer is queued but just before its\n> consumed. As the pipeline now has true knowledge of each SOF we can move\n> the check there and remove the delaying of queuing of buffers.\n>\n> This change really speeds up the IPA reactions as the delays used in the\n> Timeline where approximated while with this change they are driven by\n> events reported by the device.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 78 ++++++++----------------\n> 1 file changed, 24 insertions(+), 54 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index c3c4b5a65e3d9afe..3662a53ac4a43fcd 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -40,7 +40,6 @@ namespace libcamera {\n> LOG_DEFINE_CATEGORY(RkISP1)\n>\n> class PipelineHandlerRkISP1;\n> -class RkISP1ActionQueueBuffers;\n> class RkISP1CameraData;\n>\n> enum RkISP1ActionType {\n> @@ -203,7 +202,6 @@ private:\n> \t\t\tPipelineHandler::cameraData(camera));\n> \t}\n>\n> -\tfriend RkISP1ActionQueueBuffers;\n> \tfriend RkISP1CameraData;\n> \tfriend RkISP1Frames;\n>\n> @@ -214,6 +212,7 @@ private:\n> \tvoid bufferReady(FrameBuffer *buffer);\n> \tvoid paramReady(FrameBuffer *buffer);\n> \tvoid statReady(FrameBuffer *buffer);\n> +\tvoid frameStart(uint32_t sequence);\n>\n> \tint allocateBuffers(Camera *camera);\n> \tint freeBuffers(Camera *camera);\n> @@ -348,53 +347,6 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> \treturn nullptr;\n> }\n>\n> -class RkISP1ActionQueueBuffers : public FrameAction\n> -{\n> -public:\n> -\tRkISP1ActionQueueBuffers(unsigned int frame, RkISP1CameraData *data,\n> -\t\t\t\t PipelineHandlerRkISP1 *pipe)\n> -\t\t: FrameAction(frame, QueueBuffers), data_(data), pipe_(pipe)\n> -\t{\n> -\t}\n> -\n> -protected:\n> -\tvoid run() override\n> -\t{\n> -\t\tRkISP1FrameInfo *info = data_->frameInfo_.find(frame());\n> -\t\tif (!info)\n> -\t\t\tLOG(RkISP1, Fatal) << \"Frame not known\";\n> -\n> -\t\t/*\n> -\t\t * \\todo: If parameters are not filled a better method to handle\n> -\t\t * the situation than queuing a buffer with unknown content\n> -\t\t * should be used.\n> -\t\t *\n> -\t\t * It seems excessive to keep an internal zeroed scratch\n> -\t\t * parameters buffer around as this should not happen unless the\n> -\t\t * devices is under too much load. Perhaps failing the request\n> -\t\t * and returning it to the application with an error code is\n> -\t\t * better than queue it to hardware?\n> -\t\t */\n> -\t\tif (!info->paramFilled)\n> -\t\t\tLOG(RkISP1, Error)\n> -\t\t\t\t<< \"Parameters not ready on time for frame \"\n> -\t\t\t\t<< frame();\n> -\n> -\t\tpipe_->param_->queueBuffer(info->paramBuffer);\n> -\t\tpipe_->stat_->queueBuffer(info->statBuffer);\n> -\n> -\t\tif (info->mainPathBuffer)\n> -\t\t\tpipe_->mainPath_.queueBuffer(info->mainPathBuffer);\n> -\n> -\t\tif (info->selfPathBuffer)\n> -\t\t\tpipe_->selfPath_.queueBuffer(info->selfPathBuffer);\n> -\t}\n> -\n> -private:\n> -\tRkISP1CameraData *data_;\n> -\tPipelineHandlerRkISP1 *pipe_;\n> -};\n> -\n> int RkISP1CameraData::loadIPA()\n> {\n> \tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> @@ -958,9 +910,14 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> \top.controls = { request->controls() };\n> \tdata->ipa_->processEvent(op);\n>\n> -\tdata->timeline_.scheduleAction(std::make_unique(data->frame_,\n> -\t\t\t\t\t\t\t\t\t\t data,\n> -\t\t\t\t\t\t\t\t\t\t this));\n> +\tparam_->queueBuffer(info->paramBuffer);\n> +\tstat_->queueBuffer(info->statBuffer);\n> +\n> +\tif (info->mainPathBuffer)\n> +\t\tmainPath_.queueBuffer(info->mainPathBuffer);\n> +\n> +\tif (info->selfPathBuffer)\n> +\t\tselfPath_.queueBuffer(info->selfPathBuffer);\n>\n> \tdata->frame_++;\n>\n> @@ -1098,6 +1055,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> \tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> +\tisp_->frameStart.connect(this, &PipelineHandlerRkISP1::frameStart);\n> \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>\n> \t/*\n> @@ -1177,8 +1135,6 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> \tif (!info)\n> \t\treturn;\n>\n> -\tdata->timeline_.bufferReady(buffer);\n> -\n> \tif (data->frame_ <= buffer->metadata().sequence)\n> \t\tdata->frame_ = buffer->metadata().sequence + 1;\n>\n> @@ -1188,6 +1144,20 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> \tdata->ipa_->processEvent(op);\n> }\n>\n> +void PipelineHandlerRkISP1::frameStart(uint32_t sequence)\n> +{\n> +\tif (!activeCamera_)\n> +\t\treturn;\n> +\n> +\tRkISP1CameraData *data = cameraData(activeCamera_);\n> +\n> +\tRkISP1FrameInfo *info = data->frameInfo_.find(sequence);\n> +\tif (!info || !info->paramFilled)\n> +\t\tLOG(RkISP1, Info)\n> +\t\t\t<< \"Parameters not ready on time for frame \"\n> +\t\t\t<< sequence;\n> +}\n> +\n\nThis surely is my lack of understanding, but shouldn't we worry about\nthis a param buffer queue time ?\n\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)\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 C68A2BE176\n\tfor ;\n\tFri, 27 Nov 2020 11:05:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E7AA6346E;\n\tFri, 27 Nov 2020 12:05:07 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 259D0632EE\n\tfor ;\n\tFri, 27 Nov 2020 12:05:05 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 632E52001F;\n\tFri, 27 Nov 2020 11:05:03 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 27 Nov 2020 12:05:10 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201127110510.j6fcbn6lpjaiazwb@uno.localdomain>","References":"<20201123221234.485933-1-niklas.soderlund@ragnatech.se>\n\t<20201123221234.485933-8-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201123221234.485933-8-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: rkisp1:\n\tUse SOF event to warn about late parameters","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":14072,"web_url":"https://patchwork.libcamera.org/comment/14072/","msgid":"<20201205020832.GS4109@pendragon.ideasonboard.com>","date":"2020-12-05T02:08:32","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: rkisp1:\n\tUse SOF event to warn about late parameters","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nReviewed-by: Laurent Pinchart \n\nOn Fri, Nov 27, 2020 at 12:05:10PM +0100, Jacopo Mondi wrote:\n> On Mon, Nov 23, 2020 at 11:12:33PM +0100, Niklas Söderlund wrote:\n> > In the Timeline approach the idea was to delay queuing buffers to the\n> > device until the IPA had a chance to prepare the parameters buffer. A\n> > check was still added to warn if the IPA queued buffers before the\n> > parameters buffer was filled in.\n> >\n> > This check happened much sooner then needed as the parameter buffers\n> > does not have to be ready when the buffer is queued but just before its\n\ns/its/it's/\n\n> > consumed. As the pipeline now has true knowledge of each SOF we can move\n> > the check there and remove the delaying of queuing of buffers.\n> >\n> > This change really speeds up the IPA reactions as the delays used in the\n> > Timeline where approximated while with this change they are driven by\n> > events reported by the device.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 78 ++++++++----------------\n> > 1 file changed, 24 insertions(+), 54 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index c3c4b5a65e3d9afe..3662a53ac4a43fcd 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -40,7 +40,6 @@ namespace libcamera {\n> > LOG_DEFINE_CATEGORY(RkISP1)\n> >\n> > class PipelineHandlerRkISP1;\n> > -class RkISP1ActionQueueBuffers;\n> > class RkISP1CameraData;\n> >\n> > enum RkISP1ActionType {\n> > @@ -203,7 +202,6 @@ private:\n> > \t\t\tPipelineHandler::cameraData(camera));\n> > \t}\n> >\n> > -\tfriend RkISP1ActionQueueBuffers;\n> > \tfriend RkISP1CameraData;\n> > \tfriend RkISP1Frames;\n> >\n> > @@ -214,6 +212,7 @@ private:\n> > \tvoid bufferReady(FrameBuffer *buffer);\n> > \tvoid paramReady(FrameBuffer *buffer);\n> > \tvoid statReady(FrameBuffer *buffer);\n> > +\tvoid frameStart(uint32_t sequence);\n> >\n> > \tint allocateBuffers(Camera *camera);\n> > \tint freeBuffers(Camera *camera);\n> > @@ -348,53 +347,6 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> > \treturn nullptr;\n> > }\n> >\n> > -class RkISP1ActionQueueBuffers : public FrameAction\n> > -{\n> > -public:\n> > -\tRkISP1ActionQueueBuffers(unsigned int frame, RkISP1CameraData *data,\n> > -\t\t\t\t PipelineHandlerRkISP1 *pipe)\n> > -\t\t: FrameAction(frame, QueueBuffers), data_(data), pipe_(pipe)\n> > -\t{\n> > -\t}\n> > -\n> > -protected:\n> > -\tvoid run() override\n> > -\t{\n> > -\t\tRkISP1FrameInfo *info = data_->frameInfo_.find(frame());\n> > -\t\tif (!info)\n> > -\t\t\tLOG(RkISP1, Fatal) << \"Frame not known\";\n> > -\n> > -\t\t/*\n> > -\t\t * \\todo: If parameters are not filled a better method to handle\n> > -\t\t * the situation than queuing a buffer with unknown content\n> > -\t\t * should be used.\n> > -\t\t *\n> > -\t\t * It seems excessive to keep an internal zeroed scratch\n> > -\t\t * parameters buffer around as this should not happen unless the\n> > -\t\t * devices is under too much load. Perhaps failing the request\n> > -\t\t * and returning it to the application with an error code is\n> > -\t\t * better than queue it to hardware?\n> > -\t\t */\n> > -\t\tif (!info->paramFilled)\n> > -\t\t\tLOG(RkISP1, Error)\n> > -\t\t\t\t<< \"Parameters not ready on time for frame \"\n> > -\t\t\t\t<< frame();\n> > -\n> > -\t\tpipe_->param_->queueBuffer(info->paramBuffer);\n> > -\t\tpipe_->stat_->queueBuffer(info->statBuffer);\n> > -\n> > -\t\tif (info->mainPathBuffer)\n> > -\t\t\tpipe_->mainPath_.queueBuffer(info->mainPathBuffer);\n> > -\n> > -\t\tif (info->selfPathBuffer)\n> > -\t\t\tpipe_->selfPath_.queueBuffer(info->selfPathBuffer);\n> > -\t}\n> > -\n> > -private:\n> > -\tRkISP1CameraData *data_;\n> > -\tPipelineHandlerRkISP1 *pipe_;\n> > -};\n> > -\n> > int RkISP1CameraData::loadIPA()\n> > {\n> > \tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> > @@ -958,9 +910,14 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > \top.controls = { request->controls() };\n> > \tdata->ipa_->processEvent(op);\n> >\n> > -\tdata->timeline_.scheduleAction(std::make_unique(data->frame_,\n> > -\t\t\t\t\t\t\t\t\t\t data,\n> > -\t\t\t\t\t\t\t\t\t\t this));\n> > +\tparam_->queueBuffer(info->paramBuffer);\n> > +\tstat_->queueBuffer(info->statBuffer);\n> > +\n> > +\tif (info->mainPathBuffer)\n> > +\t\tmainPath_.queueBuffer(info->mainPathBuffer);\n> > +\n> > +\tif (info->selfPathBuffer)\n> > +\t\tselfPath_.queueBuffer(info->selfPathBuffer);\n\nYou're queuing the buffers to the ISP, including the ISP parameters,\nbefore the IPA gets a change to even take into account the parameters\nfor this request to compute the corresponding ISP parameters. Do we\nreally want to do that ?\n\n> >\n> > \tdata->frame_++;\n> >\n> > @@ -1098,6 +1055,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > \tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > +\tisp_->frameStart.connect(this, &PipelineHandlerRkISP1::frameStart);\n> > \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> >\n> > \t/*\n> > @@ -1177,8 +1135,6 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > \tif (!info)\n> > \t\treturn;\n> >\n> > -\tdata->timeline_.bufferReady(buffer);\n> > -\n> > \tif (data->frame_ <= buffer->metadata().sequence)\n> > \t\tdata->frame_ = buffer->metadata().sequence + 1;\n> >\n> > @@ -1188,6 +1144,20 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > \tdata->ipa_->processEvent(op);\n> > }\n> >\n> > +void PipelineHandlerRkISP1::frameStart(uint32_t sequence)\n> > +{\n> > +\tif (!activeCamera_)\n> > +\t\treturn;\n> > +\n> > +\tRkISP1CameraData *data = cameraData(activeCamera_);\n> > +\n> > +\tRkISP1FrameInfo *info = data->frameInfo_.find(sequence);\n> > +\tif (!info || !info->paramFilled)\n> > +\t\tLOG(RkISP1, Info)\n> > +\t\t\t<< \"Parameters not ready on time for frame \"\n> > +\t\t\t<< sequence;\n> > +}\n> > +\n> \n> This surely is my lack of understanding, but shouldn't we worry about\n> this a param buffer queue time ?\n> \n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)\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 ED730BE177\n\tfor ;\n\tSat, 5 Dec 2020 02:08:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7ECAB635DE;\n\tSat, 5 Dec 2020 03:08:36 +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 673D1634C6\n\tfor ;\n\tSat, 5 Dec 2020 03:08:34 +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 D47442A4;\n\tSat, 5 Dec 2020 03:08:33 +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=\"g78l75Ts\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607134114;\n\tbh=XhpWTorSlMdGSgAthcAudGQwYsCf8RH3pxZ+AgDF53U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=g78l75TsTd5QhB19W0buF8eMsIkmCRqNI26Y7wVg6KxQjMdd+s9YE4whYZuwIGtUz\n\tqzwsPUwUcSDOyZE5A4DjqzWJ56mWI005AlMIz4mEOQVFQrelg3AlbEOpt0RrGmqwrE\n\tt8IAsJJlNaTef0NWaOJsGbHlEOaN3tKScJi2Sw6I=","Date":"Sat, 5 Dec 2020 04:08:32 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201205020832.GS4109@pendragon.ideasonboard.com>","References":"<20201123221234.485933-1-niklas.soderlund@ragnatech.se>\n\t<20201123221234.485933-8-niklas.soderlund@ragnatech.se>\n\t<20201127110510.j6fcbn6lpjaiazwb@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201127110510.j6fcbn6lpjaiazwb@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: rkisp1:\n\tUse SOF event to warn about late parameters","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":14223,"web_url":"https://patchwork.libcamera.org/comment/14223/","msgid":"<20201211163235.GA724824@oden.dyn.berto.se>","date":"2020-12-11T16:32:35","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: rkisp1:\n\tUse SOF event to warn about late parameters","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent and Jacopo,\n\nThanks for your feedback. I think you both ask the same thing but in two \ndifferent locations in the file so I reply only at one ;-)\n\nOn 2020-12-05 04:08:32 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Reviewed-by: Laurent Pinchart \n> \n> On Fri, Nov 27, 2020 at 12:05:10PM +0100, Jacopo Mondi wrote:\n> > On Mon, Nov 23, 2020 at 11:12:33PM +0100, Niklas Söderlund wrote:\n> > > In the Timeline approach the idea was to delay queuing buffers to the\n> > > device until the IPA had a chance to prepare the parameters buffer. A\n> > > check was still added to warn if the IPA queued buffers before the\n> > > parameters buffer was filled in.\n> > >\n> > > This check happened much sooner then needed as the parameter buffers\n> > > does not have to be ready when the buffer is queued but just before its\n> \n> s/its/it's/\n> \n> > > consumed. As the pipeline now has true knowledge of each SOF we can move\n> > > the check there and remove the delaying of queuing of buffers.\n> > >\n> > > This change really speeds up the IPA reactions as the delays used in the\n> > > Timeline where approximated while with this change they are driven by\n> > > events reported by the device.\n> > >\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 78 ++++++++----------------\n> > > 1 file changed, 24 insertions(+), 54 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index c3c4b5a65e3d9afe..3662a53ac4a43fcd 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -40,7 +40,6 @@ namespace libcamera {\n> > > LOG_DEFINE_CATEGORY(RkISP1)\n> > >\n> > > class PipelineHandlerRkISP1;\n> > > -class RkISP1ActionQueueBuffers;\n> > > class RkISP1CameraData;\n> > >\n> > > enum RkISP1ActionType {\n> > > @@ -203,7 +202,6 @@ private:\n> > > \t\t\tPipelineHandler::cameraData(camera));\n> > > \t}\n> > >\n> > > -\tfriend RkISP1ActionQueueBuffers;\n> > > \tfriend RkISP1CameraData;\n> > > \tfriend RkISP1Frames;\n> > >\n> > > @@ -214,6 +212,7 @@ private:\n> > > \tvoid bufferReady(FrameBuffer *buffer);\n> > > \tvoid paramReady(FrameBuffer *buffer);\n> > > \tvoid statReady(FrameBuffer *buffer);\n> > > +\tvoid frameStart(uint32_t sequence);\n> > >\n> > > \tint allocateBuffers(Camera *camera);\n> > > \tint freeBuffers(Camera *camera);\n> > > @@ -348,53 +347,6 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> > > \treturn nullptr;\n> > > }\n> > >\n> > > -class RkISP1ActionQueueBuffers : public FrameAction\n> > > -{\n> > > -public:\n> > > -\tRkISP1ActionQueueBuffers(unsigned int frame, RkISP1CameraData *data,\n> > > -\t\t\t\t PipelineHandlerRkISP1 *pipe)\n> > > -\t\t: FrameAction(frame, QueueBuffers), data_(data), pipe_(pipe)\n> > > -\t{\n> > > -\t}\n> > > -\n> > > -protected:\n> > > -\tvoid run() override\n> > > -\t{\n> > > -\t\tRkISP1FrameInfo *info = data_->frameInfo_.find(frame());\n> > > -\t\tif (!info)\n> > > -\t\t\tLOG(RkISP1, Fatal) << \"Frame not known\";\n> > > -\n> > > -\t\t/*\n> > > -\t\t * \\todo: If parameters are not filled a better method to handle\n> > > -\t\t * the situation than queuing a buffer with unknown content\n> > > -\t\t * should be used.\n> > > -\t\t *\n> > > -\t\t * It seems excessive to keep an internal zeroed scratch\n> > > -\t\t * parameters buffer around as this should not happen unless the\n> > > -\t\t * devices is under too much load. Perhaps failing the request\n> > > -\t\t * and returning it to the application with an error code is\n> > > -\t\t * better than queue it to hardware?\n> > > -\t\t */\n> > > -\t\tif (!info->paramFilled)\n> > > -\t\t\tLOG(RkISP1, Error)\n> > > -\t\t\t\t<< \"Parameters not ready on time for frame \"\n> > > -\t\t\t\t<< frame();\n> > > -\n> > > -\t\tpipe_->param_->queueBuffer(info->paramBuffer);\n> > > -\t\tpipe_->stat_->queueBuffer(info->statBuffer);\n> > > -\n> > > -\t\tif (info->mainPathBuffer)\n> > > -\t\t\tpipe_->mainPath_.queueBuffer(info->mainPathBuffer);\n> > > -\n> > > -\t\tif (info->selfPathBuffer)\n> > > -\t\t\tpipe_->selfPath_.queueBuffer(info->selfPathBuffer);\n> > > -\t}\n> > > -\n> > > -private:\n> > > -\tRkISP1CameraData *data_;\n> > > -\tPipelineHandlerRkISP1 *pipe_;\n> > > -};\n> > > -\n> > > int RkISP1CameraData::loadIPA()\n> > > {\n> > > \tipa_ = IPAManager::createIPA(pipe_, 1, 1);\n> > > @@ -958,9 +910,14 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > > \top.controls = { request->controls() };\n> > > \tdata->ipa_->processEvent(op);\n> > >\n> > > -\tdata->timeline_.scheduleAction(std::make_unique(data->frame_,\n> > > -\t\t\t\t\t\t\t\t\t\t data,\n> > > -\t\t\t\t\t\t\t\t\t\t this));\n> > > +\tparam_->queueBuffer(info->paramBuffer);\n> > > +\tstat_->queueBuffer(info->statBuffer);\n> > > +\n> > > +\tif (info->mainPathBuffer)\n> > > +\t\tmainPath_.queueBuffer(info->mainPathBuffer);\n> > > +\n> > > +\tif (info->selfPathBuffer)\n> > > +\t\tselfPath_.queueBuffer(info->selfPathBuffer);\n> \n> You're queuing the buffers to the ISP, including the ISP parameters,\n> before the IPA gets a change to even take into account the parameters\n> for this request to compute the corresponding ISP parameters. Do we\n> really want to do that ?\n\nI'm not sure if this is what we want to do in the end, but as I \nunderstand it this is how we agreed to do it when this was discussed \nlast time and is what the Timeline solution this replaces tried to do.\n\nThe rational we had back then was that we need to queue the buffers to \nthe video device as soon as possible as to not stall the pipeline. We \nalso know the IPA may fill in the buffer up until the point in time \nwhere it's consumed by the ISP hardware. In this solution we \"buy\" the \nIPA more time to fill the param buffer then we would if we blocked for \nIPA feedback.\n\nIn this pipeline we judge that a param buffer is consume by the hardware \nwhen we see the corresponding SOE. If we at that time have not had \nfeedback form the IPA we print the warning Jacopo comments on below.\n\nI'm not committed to this mode of operation but if we wish to change it \nit think we should do so before or after this series. What do you guys \nthink?\n\n> \n> > >\n> > > \tdata->frame_++;\n> > >\n> > > @@ -1098,6 +1055,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > \tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > \tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > > +\tisp_->frameStart.connect(this, &PipelineHandlerRkISP1::frameStart);\n> > > \tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> > >\n> > > \t/*\n> > > @@ -1177,8 +1135,6 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > > \tif (!info)\n> > > \t\treturn;\n> > >\n> > > -\tdata->timeline_.bufferReady(buffer);\n> > > -\n> > > \tif (data->frame_ <= buffer->metadata().sequence)\n> > > \t\tdata->frame_ = buffer->metadata().sequence + 1;\n> > >\n> > > @@ -1188,6 +1144,20 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > > \tdata->ipa_->processEvent(op);\n> > > }\n> > >\n> > > +void PipelineHandlerRkISP1::frameStart(uint32_t sequence)\n> > > +{\n> > > +\tif (!activeCamera_)\n> > > +\t\treturn;\n> > > +\n> > > +\tRkISP1CameraData *data = cameraData(activeCamera_);\n> > > +\n> > > +\tRkISP1FrameInfo *info = data->frameInfo_.find(sequence);\n> > > +\tif (!info || !info->paramFilled)\n> > > +\t\tLOG(RkISP1, Info)\n> > > +\t\t\t<< \"Parameters not ready on time for frame \"\n> > > +\t\t\t<< sequence;\n> > > +}\n> > > +\n> > \n> > This surely is my lack of understanding, but shouldn't we worry about\n> > this a param buffer queue time ?\n> > \n> > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)\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 8591ABD808\n\tfor ;\n\tFri, 11 Dec 2020 16:32:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 106C067FA0;\n\tFri, 11 Dec 2020 17:32:40 +0100 (CET)","from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DC97C67F06\n\tfor ;\n\tFri, 11 Dec 2020 17:32:38 +0100 (CET)","by mail-lj1-x243.google.com with SMTP id s11so11583173ljp.4\n\tfor ;\n\tFri, 11 Dec 2020 08:32:38 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tb22sm936596lfp.233.2020.12.11.08.32.36\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 11 Dec 2020 08:32:36 -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=\"l/luDG1Q\"; 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=yFUu9vlWblk8AZdEOEaFHwEQWYr8Fazn0VR6Bp7otGE=;\n\tb=l/luDG1QKs3KVqM1W2r4SADiWrZ3ZsGmOaPe4oPwV+cuh45BUb03HWd/btn1aSuMiN\n\tXq2/PMQk9YNu/k9QHaBpV702PqCcEVL/XPIXKV9PmIYFDEN1Gd0K64BTLY9XEHjpEwY3\n\t/r0DV20UDqBCCSBCfuuCHrEmCndtMeZQk2lQp57WUik1zgnKr3SDyAaybUDrJ5JJ/Td5\n\tXTnxZMgqSUP5KL7KgWBvESB9Wepfw3X7mL/RBZx1V6aMoDwfcr9/d83IrQsAkzzQK+IX\n\tftRJcFA8qz2lnQYm0peS6J2bj40Wp6coFBz+eJfNkHAUwui0xXjWme/omQACyddwHyr/\n\tMNnA==","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=yFUu9vlWblk8AZdEOEaFHwEQWYr8Fazn0VR6Bp7otGE=;\n\tb=Ef0GLmH+FyiPAHh74joQsaa6l0lKtPMvZPsEWWay6WPJ4zmfmR7mn3zhWxh9enni5j\n\tmRpyefOmN33Iqqxbx2uDppO5hV/735I7vyQ4vb0PQA+aHlbNNdj76TApSkM571FxVGHW\n\tHztBRgAg1UAgx5fZYAitZnOOX9h4mhIokltf5Etjz8tXT2dj1uEcB7dvs1hxvbArTY1k\n\tLRwf1G0PYLouupPIUWsV/sPFtXNL/ELpeG/rEv+DtUxlaO166nhnk0kvIM21lNBklogU\n\tHehEqRJ+lP4sFuuho5lfzxMxKXVVBRAb55S1tQFlPS2Bh4O95g1Dmdr845x6A2+eZK39\n\thlew==","X-Gm-Message-State":"AOAM5334/JrU7fsIBggA0OFoezu3lSam1sRyLi9Yhp03SHUR+Aq2MLaM\n\tutbyVA7/UJ1sncw/w1HAH24G4HHc/eK7OQ==","X-Google-Smtp-Source":"ABdhPJyhZCwYxKkREJ6XgNUSrTrdDoYtWWb9td6TOTTpHd5L3EWtmKYoZya3qYGfopNfeR1/PfVM6Q==","X-Received":"by 2002:a2e:6f17:: with SMTP id\n\tk23mr5398888ljc.411.1607704358122; \n\tFri, 11 Dec 2020 08:32:38 -0800 (PST)","Date":"Fri, 11 Dec 2020 17:32:35 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"<20201211163235.GA724824@oden.dyn.berto.se>","References":"<20201123221234.485933-1-niklas.soderlund@ragnatech.se>\n\t<20201123221234.485933-8-niklas.soderlund@ragnatech.se>\n\t<20201127110510.j6fcbn6lpjaiazwb@uno.localdomain>\n\t<20201205020832.GS4109@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201205020832.GS4109@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: rkisp1:\n\tUse SOF event to warn about late parameters","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\" "}}]