[{"id":13943,"web_url":"https://patchwork.libcamera.org/comment/13943/","msgid":"<20201127102518.h4sybeze52pua3gf@uno.localdomain>","date":"2020-11-27T10:25:18","subject":"Re: [libcamera-devel] [PATCH v3 6/8] libcamera: pipeline: rkisp1:\n\tUse CameraSensor and delayed controls","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:32PM +0100, Niklas Söderlund wrote:\n> Instead of setting controls using the RkISP1 local Timeline helper use\n> the DelayedControls interface provided by the CameraSensor. The result\n> are the same, the controls are applied with a delay.\n\ns/are/is\n\n>\n> The values of the delays are however different between the two methods.\n> The values used in the Timeline solution was chosen after some\n\ns/was/were/\n\n> experimentation and the values used in DelayedControls are taken from a\n> generic sensor. None of the two are a perfect match as the delays can be\n> different for different sensors used with the pipeline.\n\nHow are we going to quantify those dealays when filling in the sensor\ndatabase ?\n\n>\n> However using the interface provided by CameraSensor we prepare for the\n> future where sensor specific delays will provided by the CameraSensor\n> and used without any change in the pipeline.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++---------------\n> 1 file changed, 13 insertions(+), 22 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 6e74a49abfda1b55..c3c4b5a65e3d9afe 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -121,8 +121,9 @@ class RkISP1CameraData : public CameraData\n> public:\n> \tRkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> \t\t\t RkISP1SelfPath *selfPath)\n> -\t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> -\t\t frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath)\n> +\t\t: CameraData(pipe), sensor_(nullptr), delayedCtrls_(nullptr),\n> +\t\t frame_(0), frameInfo_(pipe), mainPath_(mainPath),\n> +\t\t selfPath_(selfPath)\n> \t{\n> \t}\n>\n> @@ -136,6 +137,7 @@ public:\n> \tStream mainPathStream_;\n> \tStream selfPathStream_;\n> \tCameraSensor *sensor_;\n> +\tDelayedControls *delayedCtrls_;\n> \tunsigned int frame_;\n> \tstd::vector ipaBuffers_;\n> \tRkISP1Frames frameInfo_;\n> @@ -346,23 +348,6 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> \treturn nullptr;\n> }\n>\n> -class RkISP1ActionSetSensor : public FrameAction\n> -{\n> -public:\n> -\tRkISP1ActionSetSensor(unsigned int frame, CameraSensor *sensor, const ControlList &controls)\n> -\t\t: FrameAction(frame, SetSensor), sensor_(sensor), controls_(controls) {}\n> -\n> -protected:\n> -\tvoid run() override\n> -\t{\n> -\t\tsensor_->setControls(&controls_);\n> -\t}\n> -\n> -private:\n> -\tCameraSensor *sensor_;\n> -\tControlList controls_;\n> -};\n> -\n> class RkISP1ActionQueueBuffers : public FrameAction\n> {\n> public:\n> @@ -430,9 +415,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,\n> \tswitch (action.operation) {\n> \tcase RKISP1_IPA_ACTION_V4L2_SET: {\n> \t\tconst ControlList &controls = action.controls[0];\n> -\t\ttimeline_.scheduleAction(std::make_unique(frame,\n> -\t\t\t\t\t\t\t\t\t\t sensor_,\n> -\t\t\t\t\t\t\t\t\t\t controls));\n> +\t\tdelayedCtrls_->push(controls);\n\nI'm not sure I like this. As I've said I think this sits half-way\nbetween a 1-to-1 replacement of StaggeredCtrl and an half-backed\nenhancement.\n\nWhat I mean is:\n1) Delays and which controls are delayed is a decision of the\nCameraSensor. The pipeline and the IPA has no say on this with the\ncurrent interface.\n2) Here we unconditionally push controls received from the IPA, which\nimplies it has to know which one are delayed (otherwise the push()\nhere fails).\n\nAs suggested in the previous patch I would backtrack and mimic what\nStaggeredCtrls did, so let the pipeline initialize delays and which\ncontrols are delayed, as in the end the IPA has to know that\nanyway.\n\nGoing forward I agree CameraSensor will have to handle this, but we\nneed to find a better way to do so to decouple IPA and sensor specific\nrequirements (or decide we can't and make the IPA sensor-aware, but I\nthink nobody wants this).\n\n> \t\tbreak;\n> \t}\n> \tcase RKISP1_IPA_ACTION_PARAM_FILLED: {\n> @@ -906,6 +889,8 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> \t\t};\n> \t}\n>\n> +\tisp_->setFrameStartEnabled(true);\n> +\n> \tactiveCamera_ = camera;\n>\n> \t/* Inform IPA of stream configuration and sensor controls. */\n> @@ -933,6 +918,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> \tRkISP1CameraData *data = cameraData(camera);\n> \tint ret;\n>\n> +\tisp_->setFrameStartEnabled(false);\n> +\n> \tselfPath_.stop();\n> \tmainPath_.stop();\n>\n> @@ -1051,6 +1038,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \t/* Initialize the camera properties. */\n> \tdata->properties_ = data->sensor_->properties();\n>\n> +\tdata->delayedCtrls_ = data->sensor_->delayedContols();\n> +\tisp_->frameStart.connect(data->delayedCtrls_,\n> +\t\t\t\t &DelayedControls::frameStart);\n> +\n> \tret = data->loadIPA();\n> \tif (ret)\n> \t\treturn ret;\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 057B3BE08A\n\tfor ;\n\tFri, 27 Nov 2020 10:25:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65C236346E;\n\tFri, 27 Nov 2020 11:25:14 +0100 (CET)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D90A06345C\n\tfor ;\n\tFri, 27 Nov 2020 11:25:12 +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 relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 1DF6DFF814;\n\tFri, 27 Nov 2020 10:25:11 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 27 Nov 2020 11:25:18 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201127102518.h4sybeze52pua3gf@uno.localdomain>","References":"<20201123221234.485933-1-niklas.soderlund@ragnatech.se>\n\t<20201123221234.485933-7-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201123221234.485933-7-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 6/8] libcamera: pipeline: rkisp1:\n\tUse CameraSensor and delayed controls","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":14071,"web_url":"https://patchwork.libcamera.org/comment/14071/","msgid":"","date":"2020-12-05T02:05:56","subject":"Re: [libcamera-devel] [PATCH v3 6/8] libcamera: pipeline: rkisp1:\n\tUse CameraSensor and delayed controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Nov 27, 2020 at 11:25:18AM +0100, Jacopo Mondi wrote:\n> On Mon, Nov 23, 2020 at 11:12:32PM +0100, Niklas Söderlund wrote:\n> > Instead of setting controls using the RkISP1 local Timeline helper use\n> > the DelayedControls interface provided by the CameraSensor. The result\n> > are the same, the controls are applied with a delay.\n> \n> s/are/is\n> \n> > The values of the delays are however different between the two methods.\n> > The values used in the Timeline solution was chosen after some\n> \n> s/was/were/\n> \n> > experimentation and the values used in DelayedControls are taken from a\n> > generic sensor. None of the two are a perfect match as the delays can be\n> > different for different sensors used with the pipeline.\n> \n> How are we going to quantify those dealays when filling in the sensor\n> database ?\n\nThis is information that the sensor documentation should provide. It can\nalso be measured experimentally if needed, by changing controls\ndrastically and checking after how many frames the new value takes\neffect.\n\n> > However using the interface provided by CameraSensor we prepare for the\n> > future where sensor specific delays will provided by the CameraSensor\n> > and used without any change in the pipeline.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++---------------\n> > 1 file changed, 13 insertions(+), 22 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 6e74a49abfda1b55..c3c4b5a65e3d9afe 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -121,8 +121,9 @@ class RkISP1CameraData : public CameraData\n> > public:\n> > \tRkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> > \t\t\t RkISP1SelfPath *selfPath)\n> > -\t\t: CameraData(pipe), sensor_(nullptr), frame_(0),\n> > -\t\t frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath)\n> > +\t\t: CameraData(pipe), sensor_(nullptr), delayedCtrls_(nullptr),\n> > +\t\t frame_(0), frameInfo_(pipe), mainPath_(mainPath),\n> > +\t\t selfPath_(selfPath)\n> > \t{\n> > \t}\n> >\n> > @@ -136,6 +137,7 @@ public:\n> > \tStream mainPathStream_;\n> > \tStream selfPathStream_;\n> > \tCameraSensor *sensor_;\n> > +\tDelayedControls *delayedCtrls_;\n> > \tunsigned int frame_;\n> > \tstd::vector ipaBuffers_;\n> > \tRkISP1Frames frameInfo_;\n> > @@ -346,23 +348,6 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> > \treturn nullptr;\n> > }\n> >\n> > -class RkISP1ActionSetSensor : public FrameAction\n> > -{\n> > -public:\n> > -\tRkISP1ActionSetSensor(unsigned int frame, CameraSensor *sensor, const ControlList &controls)\n> > -\t\t: FrameAction(frame, SetSensor), sensor_(sensor), controls_(controls) {}\n> > -\n> > -protected:\n> > -\tvoid run() override\n> > -\t{\n> > -\t\tsensor_->setControls(&controls_);\n> > -\t}\n> > -\n> > -private:\n> > -\tCameraSensor *sensor_;\n> > -\tControlList controls_;\n> > -};\n> > -\n> > class RkISP1ActionQueueBuffers : public FrameAction\n> > {\n> > public:\n> > @@ -430,9 +415,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,\n> > \tswitch (action.operation) {\n> > \tcase RKISP1_IPA_ACTION_V4L2_SET: {\n> > \t\tconst ControlList &controls = action.controls[0];\n> > -\t\ttimeline_.scheduleAction(std::make_unique(frame,\n> > -\t\t\t\t\t\t\t\t\t\t sensor_,\n> > -\t\t\t\t\t\t\t\t\t\t controls));\n> > +\t\tdelayedCtrls_->push(controls);\n\nThere's an undocumented assumption in this series, that IPA will only\ninclude controls whose value changes in the sensor control list. The\nDelayedControls class doesn't skip setting controls whose values hasn't\nchanged. I think that's fine, as setting a control to the same value may\nhave side effects, so it's probably best to let the IPA decide about\nwhat controls to set. And now that I've written this, I wonder if\nthere's really a use case for not optimizing this in the DelayedControls\nclass. In any case, we should document the assumption, regardless of\nwhether we avoid unneeded writes in DelayedControls, or expect the IPA\nto do so (in which case the IPA may need to return empty control lists\nwhen no value changes).\n\n> I'm not sure I like this. As I've said I think this sits half-way\n> between a 1-to-1 replacement of StaggeredCtrl and an half-backed\n> enhancement.\n> \n> What I mean is:\n> 1) Delays and which controls are delayed is a decision of the\n> CameraSensor. The pipeline and the IPA has no say on this with the\n> current interface.\n> 2) Here we unconditionally push controls received from the IPA, which\n> implies it has to know which one are delayed (otherwise the push()\n> here fails).\n> \n> As suggested in the previous patch I would backtrack and mimic what\n> StaggeredCtrls did, so let the pipeline initialize delays and which\n> controls are delayed, as in the end the IPA has to know that\n> anyway.\n> \n> Going forward I agree CameraSensor will have to handle this, but we\n> need to find a better way to do so to decouple IPA and sensor specific\n> requirements (or decide we can't and make the IPA sensor-aware, but I\n> think nobody wants this).\n\nI agree, as commented in the review of 5/8. I think we can move this to\nthe CameraSensor class without too much trouble, but that will still\nrequire a bit of work, and I don't want to delay this series more than\nrequired.\n\n> > \t\tbreak;\n> > \t}\n> > \tcase RKISP1_IPA_ACTION_PARAM_FILLED: {\n> > @@ -906,6 +889,8 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n> > \t\t};\n> > \t}\n> >\n> > +\tisp_->setFrameStartEnabled(true);\n> > +\n> > \tactiveCamera_ = camera;\n> >\n> > \t/* Inform IPA of stream configuration and sensor controls. */\n> > @@ -933,6 +918,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> > \tRkISP1CameraData *data = cameraData(camera);\n> > \tint ret;\n> >\n> > +\tisp_->setFrameStartEnabled(false);\n> > +\n> > \tselfPath_.stop();\n> > \tmainPath_.stop();\n> >\n> > @@ -1051,6 +1038,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > \t/* Initialize the camera properties. */\n> > \tdata->properties_ = data->sensor_->properties();\n> >\n> > +\tdata->delayedCtrls_ = data->sensor_->delayedContols();\n> > +\tisp_->frameStart.connect(data->delayedCtrls_,\n> > +\t\t\t\t &DelayedControls::frameStart);\n> > +\n> > \tret = data->loadIPA();\n> > \tif (ret)\n> > \t\treturn ret;","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 5ED9CBE177\n\tfor ;\n\tSat, 5 Dec 2020 02:06:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE6A0635EF;\n\tSat, 5 Dec 2020 03:05:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 173C3634C6\n\tfor ;\n\tSat, 5 Dec 2020 03:05:59 +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 7BD502A4;\n\tSat, 5 Dec 2020 03:05:58 +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=\"qP/KrWAw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607133958;\n\tbh=ClkCoQIE5KS9xa7kZqfz3pnVH90jWiDDrCjGlxEedgk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qP/KrWAwatmC9jtqTiJYqAAaQMBDbUhYxdJnAgm/TneMkFzSOfNQQ5b+Ua0ipkK3Q\n\t2pQVER9kJs1wRTeNWJCMlmdzLFd5OQGzv9bj5dz8jQKTMBdKCWQgA9qcZp0Qwqpfzm\n\trsxivEExv2Bmp39REwA2ELjcbQGRcUNXne9uMoT8=","Date":"Sat, 5 Dec 2020 04:05:56 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20201123221234.485933-1-niklas.soderlund@ragnatech.se>\n\t<20201123221234.485933-7-niklas.soderlund@ragnatech.se>\n\t<20201127102518.h4sybeze52pua3gf@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201127102518.h4sybeze52pua3gf@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 6/8] libcamera: pipeline: rkisp1:\n\tUse CameraSensor and delayed controls","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\" "}}]