[{"id":14262,"web_url":"https://patchwork.libcamera.org/comment/14262/","msgid":"<20201217141838.gkqvue4a7hxmfcob@uno.localdomain>","date":"2020-12-17T14:18:38","subject":"Re: [libcamera-devel] [PATCH v4 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 Tue, Dec 15, 2020 at 01:48:10AM +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 it's\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\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\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 513a60b04e5f2e21..3851d94e7b133356 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -41,7 +41,6 @@ namespace libcamera {\n> LOG_DEFINE_CATEGORY(RkISP1)\n>\n> class PipelineHandlerRkISP1;\n> -class RkISP1ActionQueueBuffers;\n> class RkISP1CameraData;\n>\n> enum RkISP1ActionType {\n> @@ -202,7 +201,6 @@ private:\n> \t\t\tPipelineHandler::cameraData(camera));\n> \t}\n>\n> -\tfriend RkISP1ActionQueueBuffers;\n> \tfriend RkISP1CameraData;\n> \tfriend RkISP1Frames;\n>\n> @@ -213,6 +211,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> @@ -347,53 +346,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> @@ -950,9 +902,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> @@ -1102,6 +1059,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> @@ -1181,8 +1139,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> @@ -1192,6 +1148,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> 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 29E5DC0F1A\n\tfor ;\n\tThu, 17 Dec 2020 14:18:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 067E861596;\n\tThu, 17 Dec 2020 15:18:30 +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 21B3F61591\n\tfor ;\n\tThu, 17 Dec 2020 15:18:29 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 3B98260014;\n\tThu, 17 Dec 2020 14:18:27 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 17 Dec 2020 15:18:38 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201217141838.gkqvue4a7hxmfcob@uno.localdomain>","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-8-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201215004811.602429-8-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v4 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":14492,"web_url":"https://patchwork.libcamera.org/comment/14492/","msgid":"","date":"2021-01-10T14:17:03","subject":"Re: [libcamera-devel] [PATCH v4 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\nThank you for the patch.\n\nOn Tue, Dec 15, 2020 at 01:48:10AM +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 it's\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 513a60b04e5f2e21..3851d94e7b133356 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -41,7 +41,6 @@ namespace libcamera {\n> LOG_DEFINE_CATEGORY(RkISP1)\n> \n> class PipelineHandlerRkISP1;\n> -class RkISP1ActionQueueBuffers;\n> class RkISP1CameraData;\n> \n> enum RkISP1ActionType {\n> @@ -202,7 +201,6 @@ private:\n> \t\t\tPipelineHandler::cameraData(camera));\n> \t}\n> \n> -\tfriend RkISP1ActionQueueBuffers;\n> \tfriend RkISP1CameraData;\n> \tfriend RkISP1Frames;\n> \n> @@ -213,6 +211,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> @@ -347,53 +346,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> @@ -950,9 +902,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\nUnless there's something I don't understand correctly, you're queuing\nthe parameters buffer (along with all the other buffers) immediately\nwhen the request is received, and then write the parameters to the\nbuffer at a later time. That violates the V4L2 API, as you're not\nsupposed to write to a queued buffer, and could even crash the kernel if\nthe parameters end up being written when the driver is processing the\nbuffer (that's an issue that will need to be fixed in the kernel, but\nwhen that will be the case, writing parameters to the buffer after\nqueuing it will definitely be broken).\n\nAs the rkisp1 pipeline handler is not the main target of this patch\nseries I'm not opposed to merging this to move forward with the IPU3,\nbut could you please fix this on top ASAP ?\n\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> @@ -1102,6 +1059,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> @@ -1181,8 +1139,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> @@ -1192,6 +1148,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> 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 CB463BD808\n\tfor ;\n\tSun, 10 Jan 2021 14:17:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3613568066;\n\tSun, 10 Jan 2021 15:17:19 +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 0164460523\n\tfor ;\n\tSun, 10 Jan 2021 15:17:17 +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 72DA3DA;\n\tSun, 10 Jan 2021 15:17:17 +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=\"EOH4Izfp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1610288237;\n\tbh=hgNYDA246pX7cUDp3W4f6cpDhBBwTeEn7WrkGBm33Q0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EOH4Izfptm74POCpEQqjd+4+KPenp9S4DoluTBTE2BMWUxwblnKDNCiks9UKNBgjH\n\tFgM7/otNRJWU0YC4WtKuH6vXfLLhycH8cbi0rtS20kNGiGFfMXS9o2eYXWik7aHKu/\n\tJZFEUBeZ0LqvsNn2KZKvjalGQcttenhWnF1KOPvI=","Date":"Sun, 10 Jan 2021 16:17:03 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-8-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201215004811.602429-8-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v4 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":14562,"web_url":"https://patchwork.libcamera.org/comment/14562/","msgid":"","date":"2021-01-16T10:34:28","subject":"Re: [libcamera-devel] [PATCH v4 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,\n\nThanks for your feedback.\n\nOn 2021-01-10 16:17:03 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Tue, Dec 15, 2020 at 01:48:10AM +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 it's\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 513a60b04e5f2e21..3851d94e7b133356 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -41,7 +41,6 @@ namespace libcamera {\n> > LOG_DEFINE_CATEGORY(RkISP1)\n> > \n> > class PipelineHandlerRkISP1;\n> > -class RkISP1ActionQueueBuffers;\n> > class RkISP1CameraData;\n> > \n> > enum RkISP1ActionType {\n> > @@ -202,7 +201,6 @@ private:\n> > \t\t\tPipelineHandler::cameraData(camera));\n> > \t}\n> > \n> > -\tfriend RkISP1ActionQueueBuffers;\n> > \tfriend RkISP1CameraData;\n> > \tfriend RkISP1Frames;\n> > \n> > @@ -213,6 +211,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> > @@ -347,53 +346,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> > @@ -950,9 +902,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> \n> Unless there's something I don't understand correctly, you're queuing\n> the parameters buffer (along with all the other buffers) immediately\n> when the request is received, and then write the parameters to the\n> buffer at a later time. That violates the V4L2 API, as you're not\n> supposed to write to a queued buffer, and could even crash the kernel if\n> the parameters end up being written when the driver is processing the\n> buffer (that's an issue that will need to be fixed in the kernel, but\n> when that will be the case, writing parameters to the buffer after\n> queuing it will definitely be broken).\n> \n\nYou understand the code correctly, the buffer is queued to the V4L2 \ndevice before the IPA (may) be done filling it with parameters. I \nunderstand this violates the V4L2 API and I have argued against this \ndesign in the past. Maybe this is another example of how my recollection \nof events are at odds with reality, if so I apologies. In my mind in our\nBologna face-to-face meeting this was discussed and sketched on a \nwhiteboard in our initial IPA design idea. The reasoning was to allow \nthe IPA more time to do its thing. All of my work in this area have\nbeen based on my recollection of the outcome of that meeting. In the \nfollowup series to this one, the IPU3 pipeline is integrated with an IPA \nwith the same pattern.\n\n> As the rkisp1 pipeline handler is not the main target of this patch\n> series I'm not opposed to merging this to move forward with the IPU3,\n> but could you please fix this on top ASAP ?\n\nI think the design is prone to races and would be more then happy to \nrework it into something slower but more deterministic. As this is a \nrather large change I would address this on top for RkISP1 as the \nbehavior is already in-tree. For IPU3 would you prefer I rework the \nseries that are on the ML or that we aim to merge that work with this \ndesign and fix it on-top?\n\n> \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> > @@ -1102,6 +1059,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> > @@ -1181,8 +1139,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> > @@ -1192,6 +1148,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> > 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 A5A75BD808\n\tfor ;\n\tSat, 16 Jan 2021 10:34:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 093CD68105;\n\tSat, 16 Jan 2021 11:34:32 +0100 (CET)","from mail-lf1-x132.google.com (mail-lf1-x132.google.com\n\t[IPv6:2a00:1450:4864:20::132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E3332680F9\n\tfor ;\n\tSat, 16 Jan 2021 11:34:30 +0100 (CET)","by mail-lf1-x132.google.com with SMTP id o19so17000527lfo.1\n\tfor ;\n\tSat, 16 Jan 2021 02:34:30 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ti27sm1106746lfo.213.2021.01.16.02.34.29\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 16 Jan 2021 02:34:29 -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=\"1aTU5tj3\"; 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=PJkEYvjsSaHpC7bL/iL1vPDkTofIzNjtRUL8uSvDZVA=;\n\tb=1aTU5tj3630R9Xg/Ox15t0c6uxoK0uNb+2fwRo5TKXuKh74N0yi/yf0Rubf2xDzC4Q\n\tKIeEgQmLZLvkhiSNcpmhp8hqhjqQaN+HkimG6drho1n7yJbe90QRVssrEDyfkj3gUa1t\n\tdzBXgJbbldmGsEDpYkPnZDEG05QMc8CIEx7PWeHmq2TPXJLs8IC4uiAziDHjRAFWnAfP\n\tiuwBfVDtKZU/6S1jCBbWBnTPlXma9bbF/iOtaKr4eIjQVCxX8yrQiswi0JHpSXQYSFd9\n\tPxMMNjDBKMOHeFqbIc1n3BfOzraQaOJFURaMrO0cmACBkID34pkzk/iakg26eENgfJzT\n\tqSwg==","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=PJkEYvjsSaHpC7bL/iL1vPDkTofIzNjtRUL8uSvDZVA=;\n\tb=SoSVzaLwuh1bBhWnLxI4EKJ4esOqC+x37am9OgE/OcFOc7kx3XmXsZSKKWq5UV4lOS\n\tnDPTB1d6NpL9ld2lZJMIQ3maGlE3JAaXp94MwZPXQzi5fTEYon9BzSKDyP00PYwW16XH\n\t03hoajZ50RYDZid/33ZWydt2OAiHM9LFcYlA0iYEV87JfwgUCoqBXXJUT27FuVX2yDtD\n\t3SJeEW3y3CGJU/C0APQii5EtxuOPmjOrfd+EJ97pW6g39Kh6Qe9oxU97pJ90YK1tProa\n\tp4Hc1XtVbIDGYF/hdk9qZwjjrrg4O0B4jMY3wi4sFQqJSfOb31NKI9/4QIbEOeV95Fty\n\tuH3Q==","X-Gm-Message-State":"AOAM533yjClUPKTR+Px3I9Rj+CeD6atiKvgJNNWINIuv/6nJUii5f6b+\n\tixcn72J7snTB1RODNXMCx/Wlqg==","X-Google-Smtp-Source":"ABdhPJzIXIeadQq2K42TTx7mmFF3HAeQpOl+vp0GPiQ8RLB/bhUx1BtbAGRwCZyAGqs1SI7quYfw5g==","X-Received":"by 2002:a19:e215:: with SMTP id\n\tz21mr7178443lfg.620.1610793270216; \n\tSat, 16 Jan 2021 02:34:30 -0800 (PST)","Date":"Sat, 16 Jan 2021 11:34:28 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-8-niklas.soderlund@ragnatech.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 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\" "}}]