[{"id":15162,"web_url":"https://patchwork.libcamera.org/comment/15162/","msgid":"<20210213035621.GB66270@pyrite.rasen.tech>","date":"2021-02-13T03:56:21","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1:\n\tconfigure IPA from configure method instead of start method","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Dafna,\n\nThank you for the patch.\n\nOn Fri, Feb 12, 2021 at 08:05:57PM +0100, Dafna Hirschfeld wrote:\n> Currently the call to the configure method of rkisp1 IPA\n> is called upon the 'start' of the pipeline. This should be done\n> in the 'configure' method instead.\n> \n> Signed-off-by: Dafna Hirschfeld \n\nReviewed-by: Paul Elder \n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++------------\n> 1 file changed, 31 insertions(+), 32 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index e85979a7..f15efa9f 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -607,11 +607,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t<< \"ISP output pad configured with \" << format.toString()\n> \t\t<< \" crop \" << rect.toString();\n> \n> +\tstd::map streamConfig;\n> +\n> \tfor (const StreamConfiguration &cfg : *config) {\n> -\t\tif (cfg.stream() == &data->mainPathStream_)\n> +\t\tif (cfg.stream() == &data->mainPathStream_) {\n> \t\t\tret = mainPath_.configure(cfg, format);\n> -\t\telse\n> +\t\t\tif (data->mainPath_->isEnabled())\n> +\t\t\t\tstreamConfig[0] = {\n> +\t\t\t\t\t.pixelFormat = cfg.pixelFormat,\n> +\t\t\t\t\t.size = cfg.size,\n> +\t\t\t\t};\n> +\t\t} else {\n> \t\t\tret = selfPath_.configure(cfg, format);\n> +\t\t\tif (data->selfPath_->isEnabled())\n> +\t\t\t\tstreamConfig[1] = {\n> +\t\t\t\t\t.pixelFormat = cfg.pixelFormat,\n> +\t\t\t\t\t.size = cfg.size,\n> +\t\t\t\t};\n> +\t\t}\n> \n> \t\tif (ret)\n> \t\t\treturn ret;\n> @@ -629,6 +642,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\t/* Inform IPA of stream configuration and sensor controls. */\n> +\tCameraSensorInfo sensorInfo = {};\n> +\tret = data->sensor_->sensorInfo(&sensorInfo);\n> +\tif (ret) {\n> +\t\t/* \\todo Turn this in an hard failure. */\n> +\t\tLOG(RkISP1, Warning) << \"Camera sensor information not available\";\n> +\t\tsensorInfo = {};\n> +\t\tret = 0;\n> +\t}\n> +\n> +\tstd::map entityControls;\n> +\tentityControls.emplace(0, data->sensor_->controls());\n> +\n> +\tIPAOperationData ipaConfig;\n> +\tdata->ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, nullptr);\n> +\n> \treturn 0;\n> }\n> \n> @@ -759,8 +788,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> \t\treturn ret;\n> \t}\n> \n> -\tstd::map streamConfig;\n> -\n> \tif (data->mainPath_->isEnabled()) {\n> \t\tret = mainPath_.start();\n> \t\tif (ret) {\n> @@ -770,11 +797,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> \t\t\tfreeBuffers(camera);\n> \t\t\treturn ret;\n> \t\t}\n> -\n> -\t\tstreamConfig[0] = {\n> -\t\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n> -\t\t\t.size = data->mainPathStream_.configuration().size,\n> -\t\t};\n> \t}\n> \n> \tif (data->selfPath_->isEnabled()) {\n> @@ -787,34 +809,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> \t\t\tfreeBuffers(camera);\n> \t\t\treturn ret;\n> \t\t}\n> -\n> -\t\tstreamConfig[1] = {\n> -\t\t\t.pixelFormat = data->selfPathStream_.configuration().pixelFormat,\n> -\t\t\t.size = data->selfPathStream_.configuration().size,\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> -\tCameraSensorInfo sensorInfo = {};\n> -\tret = data->sensor_->sensorInfo(&sensorInfo);\n> -\tif (ret) {\n> -\t\t/* \\todo Turn this in an hard failure. */\n> -\t\tLOG(RkISP1, Warning) << \"Camera sensor information not available\";\n> -\t\tsensorInfo = {};\n> -\t\tret = 0;\n> -\t}\n> -\n> -\tstd::map entityControls;\n> -\tentityControls.emplace(0, data->sensor_->controls());\n> -\n> -\tIPAOperationData ipaConfig;\n> -\tdata->ipa_->configure(sensorInfo, streamConfig, entityControls,\n> -\t\t\t ipaConfig, nullptr);\n> -\n> \treturn ret;\n> }\n> \n> -- \n> 2.17.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 1A275BD162\n\tfor ;\n\tSat, 13 Feb 2021 03:56:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9CE736379A;\n\tSat, 13 Feb 2021 04:56:33 +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 8EA23601B5\n\tfor ;\n\tSat, 13 Feb 2021 04:56:32 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 01159564;\n\tSat, 13 Feb 2021 04:56:29 +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=\"FS+k8wv/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613188592;\n\tbh=NrxrMMXgIsM+4TWP38MU/xe4NXAsaMYGxTqM8fZ/EIY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FS+k8wv/JOJD51aJWZVjH/rWJgFJ2Wjnvpnya4Z8hAWZAaIUPXlN5bRDQal2r78sH\n\tqTiLH6B02oysPC+ox64hZ/v2IEoQHyjkC1yDUT7vVjlWPcrU5ohFiAoA6huCiqPowF\n\tzIl+iGBSTZRLarIQNeO0Y1AORUwQL5erU4H+N83A=","Date":"Sat, 13 Feb 2021 12:56:21 +0900","From":"paul.elder@ideasonboard.com","To":"Dafna Hirschfeld ","Message-ID":"<20210213035621.GB66270@pyrite.rasen.tech>","References":"<20210212190557.13905-1-dafna.hirschfeld@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210212190557.13905-1-dafna.hirschfeld@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1:\n\tconfigure IPA from configure method instead of start method","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, kernel@collabora.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15163,"web_url":"https://patchwork.libcamera.org/comment/15163/","msgid":"<20210213091217.g2h7hpgpljwhmz2o@basti-TUXEDO-Book-XA1510>","date":"2021-02-13T09:12:17","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1:\n\tconfigure IPA from configure method instead of start method","submitter":{"id":78,"url":"https://patchwork.libcamera.org/api/people/78/","name":"Sebastian Fricke","email":"sebastian.fricke@posteo.net"},"content":"Hey Dafna,\n\nThank you for the patch.\n\nOn 12.02.2021 20:05, Dafna Hirschfeld wrote:\n>Currently the call to the configure method of rkisp1 IPA\n>is called upon the 'start' of the pipeline. This should be done\n>in the 'configure' method instead.\n\nI assume that the reason for this change is consistency right? I believe\nthe raspberry Pi has the most mature pipeline and IPA implementation and\nit makes sense to have some kind of consistent pattern.\n\nIn order to find out why it is necessary to configure the IPA in\nconfigure instead of start, I first looked into `src/cam/capture.cpp`,\nwhere both methods are called. In between the configure and the start\ncall, the buffers are allocated and the initial set of requests is\ncreated.\nAdditionally, I have compared both the RkISP1 and Raspberry Pi\nimplementation of the IPA configure method. Currently, the RkISP1 IPA\ndoesn't do a lot so it is difficult to decide, which parts might be added.\nThe Raspberry Pi currently does the following actions that are missing in\nthe RkISP1:\n- Setup and validate the ISP controls\n- Setup metadata controls\n- Load device specific information from a helper\n- Load the lens shading table\n- Prepare the algorithms by setting the camera mode\n- Load the tuning file for the sensor\n\nWhat they both do is initialize the analog gain and exposure controls.\n(With the difference that RkISP1 sets both to their respective minimum\nand Raspberry Pi provides default values).\n\nThis means, that the different order of the IPA configure method can\nonly have an impact on the actions happening in between the two states\nconfigured and start (allocation of buffers and creation of the inital\nrequests). So, the current advantage/disadvantage is that the controls\nare reset to their minimum for the initial set of requests as well.\n\n From my point of view this seems to be a good preparation for the\nfollowing changes to the RkISP1 IPA, even though it currently doesn't\nseem to do to much. Have I missed anything else that makes this order of\nactions essential?\n\nBesides that little analysis I have tested the patch by building it and\nperforming a test capture. Both works without problems.\n\nGreetings,\nSebastian\n\nI fixed a small typo below that existed before as well.\n\n>\n>Signed-off-by: Dafna Hirschfeld \n>---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++------------\n> 1 file changed, 31 insertions(+), 32 deletions(-)\n>\n>diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>index e85979a7..f15efa9f 100644\n>--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>@@ -607,11 +607,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t<< \"ISP output pad configured with \" << format.toString()\n> \t\t<< \" crop \" << rect.toString();\n>\n>+\tstd::map streamConfig;\n>+\n> \tfor (const StreamConfiguration &cfg : *config) {\n>-\t\tif (cfg.stream() == &data->mainPathStream_)\n>+\t\tif (cfg.stream() == &data->mainPathStream_) {\n> \t\t\tret = mainPath_.configure(cfg, format);\n>-\t\telse\n>+\t\t\tif (data->mainPath_->isEnabled())\n>+\t\t\t\tstreamConfig[0] = {\n>+\t\t\t\t\t.pixelFormat = cfg.pixelFormat,\n>+\t\t\t\t\t.size = cfg.size,\n>+\t\t\t\t};\n>+\t\t} else {\n> \t\t\tret = selfPath_.configure(cfg, format);\n>+\t\t\tif (data->selfPath_->isEnabled())\n>+\t\t\t\tstreamConfig[1] = {\n>+\t\t\t\t\t.pixelFormat = cfg.pixelFormat,\n>+\t\t\t\t\t.size = cfg.size,\n>+\t\t\t\t};\n>+\t\t}\n>\n> \t\tif (ret)\n> \t\t\treturn ret;\n>@@ -629,6 +642,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tif (ret)\n> \t\treturn ret;\n>\n>+\t/* Inform IPA of stream configuration and sensor controls. */\n>+\tCameraSensorInfo sensorInfo = {};\n>+\tret = data->sensor_->sensorInfo(&sensorInfo);\n>+\tif (ret) {\n>+\t\t/* \\todo Turn this in an hard failure. */\n\ns/Turn this in an hard failure./Turn this into a hard failure./\n(or maybe: Make this a hard failure)\n\n>+\t\tLOG(RkISP1, Warning) << \"Camera sensor information not available\";\n>+\t\tsensorInfo = {};\n>+\t\tret = 0;\n>+\t}\n>+\n>+\tstd::map entityControls;\n>+\tentityControls.emplace(0, data->sensor_->controls());\n>+\n>+\tIPAOperationData ipaConfig;\n>+\tdata->ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, nullptr);\n>+\n> \treturn 0;\n> }\n>\n>@@ -759,8 +788,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> \t\treturn ret;\n> \t}\n>\n>-\tstd::map streamConfig;\n>-\n> \tif (data->mainPath_->isEnabled()) {\n> \t\tret = mainPath_.start();\n> \t\tif (ret) {\n>@@ -770,11 +797,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> \t\t\tfreeBuffers(camera);\n> \t\t\treturn ret;\n> \t\t}\n>-\n>-\t\tstreamConfig[0] = {\n>-\t\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n>-\t\t\t.size = data->mainPathStream_.configuration().size,\n>-\t\t};\n> \t}\n>\n> \tif (data->selfPath_->isEnabled()) {\n>@@ -787,34 +809,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> \t\t\tfreeBuffers(camera);\n> \t\t\treturn ret;\n> \t\t}\n>-\n>-\t\tstreamConfig[1] = {\n>-\t\t\t.pixelFormat = data->selfPathStream_.configuration().pixelFormat,\n>-\t\t\t.size = data->selfPathStream_.configuration().size,\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>-\tCameraSensorInfo sensorInfo = {};\n>-\tret = data->sensor_->sensorInfo(&sensorInfo);\n>-\tif (ret) {\n>-\t\t/* \\todo Turn this in an hard failure. */\n>-\t\tLOG(RkISP1, Warning) << \"Camera sensor information not available\";\n>-\t\tsensorInfo = {};\n>-\t\tret = 0;\n>-\t}\n>-\n>-\tstd::map entityControls;\n>-\tentityControls.emplace(0, data->sensor_->controls());\n>-\n>-\tIPAOperationData ipaConfig;\n>-\tdata->ipa_->configure(sensorInfo, streamConfig, entityControls,\n>-\t\t\t ipaConfig, nullptr);\n>-\n> \treturn ret;\n> }\n>\n>-- \n>2.17.1\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 4AF88BD162\n\tfor ;\n\tSat, 13 Feb 2021 09:12:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0023637A3;\n\tSat, 13 Feb 2021 10:12:20 +0100 (CET)","from mout02.posteo.de (mout02.posteo.de [185.67.36.66])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A393260D37\n\tfor ;\n\tSat, 13 Feb 2021 10:12:19 +0100 (CET)","from submission (posteo.de [89.146.220.130]) \n\tby mout02.posteo.de (Postfix) with ESMTPS id 268C92400FC\n\tfor ;\n\tSat, 13 Feb 2021 10:12:19 +0100 (CET)","from customer (localhost [127.0.0.1])\n\tby submission (posteo.de) with ESMTPSA id 4Dd4Pt2nYWz9rxc;\n\tSat, 13 Feb 2021 10:12:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=posteo.net header.i=@posteo.net\n\theader.b=\"kW9nf59q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017;\n\tt=1613207539; bh=3SRbIRilhr7TFfx6KmaOgZAxRd5TfNzs/Twk6yguLBQ=;\n\th=Date:From:To:Cc:Subject:From;\n\tb=kW9nf59qqB4wef14GJYw5sg+9UcrWmRI6BdpM+7VJrQupi9/uV9uE0HS9zkN7K8He\n\t1gPb01Db+Kgxh/mWF5zk93QmYF01ZkKiy0ulTiU8ydzO46rX2rHPwRd+Wk8EApOsAX\n\t/p/Ldzxfe8hHzPnOkglG6bKHl3lAJRW5tV+XS2YKt7TTLsRkI/a58Rk5KDPPfqol+d\n\t7zTWEF2DnrVn1kPb7n8DbPxBhfiKEbGzeOQbNRY0BcuNRfEXTfBmW5MkpjfAqxAdv7\n\tQAGVtwCVEcT/+1ZRTL75lVFZiaB1r6MBgcb40NBuWaHiC3N1n/QhW73A5q3pAEOPWN\n\tQl4EcqxOOQRcA==","Date":"Sat, 13 Feb 2021 10:12:17 +0100","From":"Sebastian Fricke ","To":"Dafna Hirschfeld ","Message-ID":"<20210213091217.g2h7hpgpljwhmz2o@basti-TUXEDO-Book-XA1510>","References":"<20210212190557.13905-1-dafna.hirschfeld@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210212190557.13905-1-dafna.hirschfeld@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1:\n\tconfigure IPA from configure method instead of start method","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, kernel@collabora.com","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15167,"web_url":"https://patchwork.libcamera.org/comment/15167/","msgid":"","date":"2021-02-15T03:42:13","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1:\n\tconfigure IPA from configure method instead of start method","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dafna,\n\nThank you for the patch.\n\nOn Fri, Feb 12, 2021 at 08:05:57PM +0100, Dafna Hirschfeld wrote:\n> Currently the call to the configure method of rkisp1 IPA\n> is called upon the 'start' of the pipeline. This should be done\n> in the 'configure' method instead.\n> \n> Signed-off-by: Dafna Hirschfeld \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++------------\n> 1 file changed, 31 insertions(+), 32 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index e85979a7..f15efa9f 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -607,11 +607,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t<< \"ISP output pad configured with \" << format.toString()\n> \t\t<< \" crop \" << rect.toString();\n> \n> +\tstd::map streamConfig;\n> +\n> \tfor (const StreamConfiguration &cfg : *config) {\n> -\t\tif (cfg.stream() == &data->mainPathStream_)\n> +\t\tif (cfg.stream() == &data->mainPathStream_) {\n> \t\t\tret = mainPath_.configure(cfg, format);\n> -\t\telse\n> +\t\t\tif (data->mainPath_->isEnabled())\n\nI think you can drop this check. isEnabled() will return true if\nPipelineHandlerRkISP1::initLinks() has enabled the path with\nsetEnabled(), which is done when cfg.stream() == &data->mainPathStream_.\n\nThis being said, I think there's a bug in the pipeline handler as links\nare never reset, but that's out of scope for this patch.\n\n> +\t\t\t\tstreamConfig[0] = {\n> +\t\t\t\t\t.pixelFormat = cfg.pixelFormat,\n> +\t\t\t\t\t.size = cfg.size,\n> +\t\t\t\t};\n> +\t\t} else {\n> \t\t\tret = selfPath_.configure(cfg, format);\n> +\t\t\tif (data->selfPath_->isEnabled())\n\nSame here.\n\n> +\t\t\t\tstreamConfig[1] = {\n> +\t\t\t\t\t.pixelFormat = cfg.pixelFormat,\n> +\t\t\t\t\t.size = cfg.size,\n> +\t\t\t\t};\n> +\t\t}\n> \n> \t\tif (ret)\n> \t\t\treturn ret;\n> @@ -629,6 +642,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\t/* Inform IPA of stream configuration and sensor controls. */\n> +\tCameraSensorInfo sensorInfo = {};\n> +\tret = data->sensor_->sensorInfo(&sensorInfo);\n> +\tif (ret) {\n> +\t\t/* \\todo Turn this in an hard failure. */\n> +\t\tLOG(RkISP1, Warning) << \"Camera sensor information not available\";\n> +\t\tsensorInfo = {};\n> +\t\tret = 0;\n> +\t}\n> +\n> +\tstd::map entityControls;\n> +\tentityControls.emplace(0, data->sensor_->controls());\n> +\n> +\tIPAOperationData ipaConfig;\n> +\tdata->ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, nullptr);\n\nThis could be wrapped to shorten the line.\n\nWith these issues addressed,\n\nReviewed-by: Laurent Pinchart \n\n> +\n> \treturn 0;\n> }\n> \n> @@ -759,8 +788,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> \t\treturn ret;\n> \t}\n> \n> -\tstd::map streamConfig;\n> -\n> \tif (data->mainPath_->isEnabled()) {\n> \t\tret = mainPath_.start();\n> \t\tif (ret) {\n> @@ -770,11 +797,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> \t\t\tfreeBuffers(camera);\n> \t\t\treturn ret;\n> \t\t}\n> -\n> -\t\tstreamConfig[0] = {\n> -\t\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n> -\t\t\t.size = data->mainPathStream_.configuration().size,\n> -\t\t};\n> \t}\n> \n> \tif (data->selfPath_->isEnabled()) {\n> @@ -787,34 +809,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> \t\t\tfreeBuffers(camera);\n> \t\t\treturn ret;\n> \t\t}\n> -\n> -\t\tstreamConfig[1] = {\n> -\t\t\t.pixelFormat = data->selfPathStream_.configuration().pixelFormat,\n> -\t\t\t.size = data->selfPathStream_.configuration().size,\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> -\tCameraSensorInfo sensorInfo = {};\n> -\tret = data->sensor_->sensorInfo(&sensorInfo);\n> -\tif (ret) {\n> -\t\t/* \\todo Turn this in an hard failure. */\n> -\t\tLOG(RkISP1, Warning) << \"Camera sensor information not available\";\n> -\t\tsensorInfo = {};\n> -\t\tret = 0;\n> -\t}\n> -\n> -\tstd::map entityControls;\n> -\tentityControls.emplace(0, data->sensor_->controls());\n> -\n> -\tIPAOperationData ipaConfig;\n> -\tdata->ipa_->configure(sensorInfo, streamConfig, entityControls,\n> -\t\t\t ipaConfig, nullptr);\n> -\n> \treturn ret;\n> }\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 2C8FCBD160\n\tfor ;\n\tMon, 15 Feb 2021 03:42:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B56AF637AB;\n\tMon, 15 Feb 2021 04:42:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6062A602F9\n\tfor ;\n\tMon, 15 Feb 2021 04:42:39 +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 B6070743;\n\tMon, 15 Feb 2021 04:42:38 +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=\"dnv5Wxce\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613360558;\n\tbh=538m2h34Q015Rr5wRXXHOEP+LvBoM4Wh4KyY3tx7ESc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dnv5Wxce6tmA1jJhToQio20iSVT2F57m4ZhOrSu646zKzGM6Mo8JVXseYyxVgwqX1\n\tmqWr7ucuqKgnreFF6wTGu1dRHjnUmFJbQoWwlpmHJCncJSKS6BECRXVS9+9xu50mzI\n\t218rJHHGTeoQ8JrMA8mvr6xyPNmi3GJUG/UIiOvE=","Date":"Mon, 15 Feb 2021 05:42:13 +0200","From":"Laurent Pinchart ","To":"Dafna Hirschfeld ","Message-ID":"","References":"<20210212190557.13905-1-dafna.hirschfeld@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210212190557.13905-1-dafna.hirschfeld@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1:\n\tconfigure IPA from configure method instead of start method","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, kernel@collabora.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15169,"web_url":"https://patchwork.libcamera.org/comment/15169/","msgid":"","date":"2021-02-15T03:59:31","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1:\n\tconfigure IPA from configure method instead of start method","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Sebastian,\n\nOn Sat, Feb 13, 2021 at 10:12:17AM +0100, Sebastian Fricke wrote:\n> On 12.02.2021 20:05, Dafna Hirschfeld wrote:\n> > Currently the call to the configure method of rkisp1 IPA\n> > is called upon the 'start' of the pipeline. This should be done\n> > in the 'configure' method instead.\n> \n> I assume that the reason for this change is consistency right? I believe\n> the raspberry Pi has the most mature pipeline and IPA implementation and\n> it makes sense to have some kind of consistent pattern.\n> \n> In order to find out why it is necessary to configure the IPA in\n> configure instead of start, I first looked into `src/cam/capture.cpp`,\n> where both methods are called. In between the configure and the start\n> call, the buffers are allocated and the initial set of requests is\n> created.\n> Additionally, I have compared both the RkISP1 and Raspberry Pi\n> implementation of the IPA configure method. Currently, the RkISP1 IPA\n> doesn't do a lot so it is difficult to decide, which parts might be added.\n> The Raspberry Pi currently does the following actions that are missing in\n> the RkISP1:\n> - Setup and validate the ISP controls\n> - Setup metadata controls\n> - Load device specific information from a helper\n> - Load the lens shading table\n> - Prepare the algorithms by setting the camera mode\n> - Load the tuning file for the sensor\n> \n> What they both do is initialize the analog gain and exposure controls.\n> (With the difference that RkISP1 sets both to their respective minimum\n> and Raspberry Pi provides default values).\n> \n> This means, that the different order of the IPA configure method can\n> only have an impact on the actions happening in between the two states\n> configured and start (allocation of buffers and creation of the inital\n> requests). So, the current advantage/disadvantage is that the controls\n> are reset to their minimum for the initial set of requests as well.\n\nNice and detailed analysis :-)\n\n> From my point of view this seems to be a good preparation for the\n> following changes to the RkISP1 IPA, even though it currently doesn't\n> seem to do to much. Have I missed anything else that makes this order of\n> actions essential?\n\nRight now, no, but with Paul's series that implements the IPA IPC\nprotocol, IPA functions called after start() must be asynchronous. The\nIPA configure() function is a synchronous call, so this won't work well.\nThis patch is in preparation for the IPA IPC (beside improving\nconsistency, which is usually good).\n\n> Besides that little analysis I have tested the patch by building it and\n> performing a test capture. Both works without problems.\n> \n> Greetings,\n> Sebastian\n> \n> I fixed a small typo below that existed before as well.\n> \n> > Signed-off-by: Dafna Hirschfeld \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++------------\n> > 1 file changed, 31 insertions(+), 32 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index e85979a7..f15efa9f 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -607,11 +607,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \t\t<< \"ISP output pad configured with \" << format.toString()\n> > \t\t<< \" crop \" << rect.toString();\n> > \n> > +\tstd::map streamConfig;\n> > +\n> > \tfor (const StreamConfiguration &cfg : *config) {\n> > -\t\tif (cfg.stream() == &data->mainPathStream_)\n> > +\t\tif (cfg.stream() == &data->mainPathStream_) {\n> > \t\t\tret = mainPath_.configure(cfg, format);\n> > -\t\telse\n> > +\t\t\tif (data->mainPath_->isEnabled())\n> > +\t\t\t\tstreamConfig[0] = {\n> > +\t\t\t\t\t.pixelFormat = cfg.pixelFormat,\n> > +\t\t\t\t\t.size = cfg.size,\n> > +\t\t\t\t};\n> > +\t\t} else {\n> > \t\t\tret = selfPath_.configure(cfg, format);\n> > +\t\t\tif (data->selfPath_->isEnabled())\n> > +\t\t\t\tstreamConfig[1] = {\n> > +\t\t\t\t\t.pixelFormat = cfg.pixelFormat,\n> > +\t\t\t\t\t.size = cfg.size,\n> > +\t\t\t\t};\n> > +\t\t}\n> > \n> > \t\tif (ret)\n> > \t\t\treturn ret;\n> > @@ -629,6 +642,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \tif (ret)\n> > \t\treturn ret;\n> > \n> > +\t/* Inform IPA of stream configuration and sensor controls. */\n> > +\tCameraSensorInfo sensorInfo = {};\n> > +\tret = data->sensor_->sensorInfo(&sensorInfo);\n> > +\tif (ret) {\n> > +\t\t/* \\todo Turn this in an hard failure. */\n> \n> s/Turn this in an hard failure./Turn this into a hard failure./\n> (or maybe: Make this a hard failure)\n> \n> > +\t\tLOG(RkISP1, Warning) << \"Camera sensor information not available\";\n> > +\t\tsensorInfo = {};\n> > +\t\tret = 0;\n> > +\t}\n> > +\n> > +\tstd::map entityControls;\n> > +\tentityControls.emplace(0, data->sensor_->controls());\n> > +\n> > +\tIPAOperationData ipaConfig;\n> > +\tdata->ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, nullptr);\n> > +\n> > \treturn 0;\n> > }\n> > \n> > @@ -759,8 +788,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> > \t\treturn ret;\n> > \t}\n> > \n> > -\tstd::map streamConfig;\n> > -\n> > \tif (data->mainPath_->isEnabled()) {\n> > \t\tret = mainPath_.start();\n> > \t\tif (ret) {\n> > @@ -770,11 +797,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> > \t\t\tfreeBuffers(camera);\n> > \t\t\treturn ret;\n> > \t\t}\n> > -\n> > -\t\tstreamConfig[0] = {\n> > -\t\t\t.pixelFormat = data->mainPathStream_.configuration().pixelFormat,\n> > -\t\t\t.size = data->mainPathStream_.configuration().size,\n> > -\t\t};\n> > \t}\n> > \n> > \tif (data->selfPath_->isEnabled()) {\n> > @@ -787,34 +809,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> > \t\t\tfreeBuffers(camera);\n> > \t\t\treturn ret;\n> > \t\t}\n> > -\n> > -\t\tstreamConfig[1] = {\n> > -\t\t\t.pixelFormat = data->selfPathStream_.configuration().pixelFormat,\n> > -\t\t\t.size = data->selfPathStream_.configuration().size,\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> > -\tCameraSensorInfo sensorInfo = {};\n> > -\tret = data->sensor_->sensorInfo(&sensorInfo);\n> > -\tif (ret) {\n> > -\t\t/* \\todo Turn this in an hard failure. */\n> > -\t\tLOG(RkISP1, Warning) << \"Camera sensor information not available\";\n> > -\t\tsensorInfo = {};\n> > -\t\tret = 0;\n> > -\t}\n> > -\n> > -\tstd::map entityControls;\n> > -\tentityControls.emplace(0, data->sensor_->controls());\n> > -\n> > -\tIPAOperationData ipaConfig;\n> > -\tdata->ipa_->configure(sensorInfo, streamConfig, entityControls,\n> > -\t\t\t ipaConfig, nullptr);\n> > -\n> > \treturn ret;\n> > }\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 5997DBD160\n\tfor ;\n\tMon, 15 Feb 2021 03:59:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC318637B7;\n\tMon, 15 Feb 2021 04:59:58 +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 89E49602F9\n\tfor ;\n\tMon, 15 Feb 2021 04:59:57 +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 ED7F1743;\n\tMon, 15 Feb 2021 04:59:56 +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=\"QFN1JBId\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613361597;\n\tbh=ZyQfPsIkMzjLYt/vV3iqTXnuCnjmnXB921FFP+SuuHU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QFN1JBIdfLpXTKk5yjijVIYvk1SlT8ksYi+do52nZdEVUn/dogwC5dTvqSa+1MQvs\n\tZ10Px7cjTG0EEGQxg3e9ub946hsp0IdqxPKPomeFFv36WXNHgsk9WrWS/m2cQF4cr3\n\thKJxNrDCdLpPJiYP+Jei36S6dUEgPj+6Jd4fbeHs=","Date":"Mon, 15 Feb 2021 05:59:31 +0200","From":"Laurent Pinchart ","To":"Sebastian Fricke ","Message-ID":"","References":"<20210212190557.13905-1-dafna.hirschfeld@collabora.com>\n\t<20210213091217.g2h7hpgpljwhmz2o@basti-TUXEDO-Book-XA1510>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210213091217.g2h7hpgpljwhmz2o@basti-TUXEDO-Book-XA1510>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1:\n\tconfigure IPA from configure method instead of start method","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, kernel@collabora.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]