[{"id":14061,"web_url":"https://patchwork.libcamera.org/comment/14061/","msgid":"<20201204161458.hpsdi4rozv7pzqdy@uno.localdomain>","date":"2020-12-04T16:14:58","subject":"Re: [libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi:\n\tPass controls to IPA on start","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Fri, Dec 04, 2020 at 03:31:21PM +0000, Naushir Patuck wrote:\n> Forward any controls passed into the pipeline handler to the IPA.\n> The IPA then sets up the Raspberry Pi controller with these settings\n> appropriately, and passes back any V4L2 sensor controls that need\n> to be applied.\n>\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Tested-by: David Plowman \nReviewed-by: Jacopo Mondi \n\nThanks\n j\n> ---\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 54 ++++++++++++-------\n> .../pipeline/raspberrypi/raspberrypi.cpp | 13 ++++-\n> 3 files changed, 47 insertions(+), 21 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 2b55dbfc..c91a14bd 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -21,6 +21,7 @@ enum ConfigParameters {\n> \tIPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> \tIPA_CONFIG_SENSOR = (1 << 2),\n> \tIPA_CONFIG_DROP_FRAMES = (1 << 3),\n> +\tIPA_CONFIG_STARTUP = (1 << 4)\n> };\n>\n> enum Operations {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index b8c0f955..ff95580e 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -78,8 +78,7 @@ public:\n> \t}\n>\n> \tint init(const IPASettings &settings) override;\n> -\tint start([[maybe_unused]] const IPAOperationData &data,\n> -\t\t [[maybe_unused]] IPAOperationData *result) override { return 0; }\n> +\tint start(const IPAOperationData &data, IPAOperationData *result) override;\n> \tvoid stop() override {}\n>\n> \tvoid configure(const CameraSensorInfo &sensorInfo,\n> @@ -154,6 +153,36 @@ int IPARPi::init(const IPASettings &settings)\n> \treturn 0;\n> }\n>\n> +int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)\n> +{\n> +\tRPiController::Metadata metadata;\n> +\n> +\tASSERT(result);\n> +\tresult->operation = 0;\n> +\tif (data.operation & RPi::IPA_CONFIG_STARTUP) {\n> +\t\t/* We have been given some controls to action before start. */\n> +\t\tqueueRequest(data.controls[0]);\n> +\t}\n> +\n> +\tcontroller_.SwitchMode(mode_, &metadata);\n> +\n> +\t/* SwitchMode may supply updated exposure/gain values to use. */\n> +\tAgcStatus agcStatus;\n> +\tagcStatus.shutter_time = 0.0;\n> +\tagcStatus.analogue_gain = 0.0;\n> +\n> +\t/* SwitchMode may supply updated exposure/gain values to use. */\n> +\tmetadata.Get(\"agc.status\", agcStatus);\n> +\tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> +\t\tControlList ctrls(unicamCtrls_);\n> +\t\tapplyAGC(&agcStatus, ctrls);\n> +\t\tresult->controls.emplace_back(ctrls);\n> +\t\tresult->operation |= RPi::IPA_CONFIG_SENSOR;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> {\n> \tmode_.bitdepth = sensorInfo.bitsPerPixel;\n> @@ -229,7 +258,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tresult->data.push_back(gainDelay);\n> \t\tresult->data.push_back(exposureDelay);\n> \t\tresult->data.push_back(sensorMetadata);\n> -\n> \t\tresult->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> \t}\n>\n> @@ -285,11 +313,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \tresult->data.push_back(dropFrame);\n> \tresult->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n>\n> -\t/* These zero values mean not program anything (unless overwritten). */\n> -\tstruct AgcStatus agcStatus;\n> -\tagcStatus.shutter_time = 0.0;\n> -\tagcStatus.analogue_gain = 0.0;\n> -\n> \tif (!controllerInit_) {\n> \t\t/* Load the tuning file for this sensor. */\n> \t\tcontroller_.Read(tuningFile_.c_str());\n> @@ -297,20 +320,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tcontrollerInit_ = true;\n>\n> \t\t/* Supply initial values for gain and exposure. */\n> +\t\tControlList ctrls(unicamCtrls_);\n> +\t\tAgcStatus agcStatus;\n> \t\tagcStatus.shutter_time = DefaultExposureTime;\n> \t\tagcStatus.analogue_gain = DefaultAnalogueGain;\n> -\t}\n> -\n> -\tRPiController::Metadata metadata;\n> -\tcontroller_.SwitchMode(mode_, &metadata);\n> -\n> -\t/* SwitchMode may supply updated exposure/gain values to use. */\n> -\tmetadata.Get(\"agc.status\", agcStatus);\n> -\tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> -\t\tControlList ctrls(unicamCtrls_);\n> \t\tapplyAGC(&agcStatus, ctrls);\n> -\t\tresult->controls.push_back(ctrls);\n>\n> +\t\tresult->controls.emplace_back(ctrls);\n> \t\tresult->operation |= RPi::IPA_CONFIG_SENSOR;\n> \t}\n>\n> @@ -824,7 +840,7 @@ void IPARPi::processStats(unsigned int bufferId)\n>\n> \t\tIPAOperationData op;\n> \t\top.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;\n> -\t\top.controls.push_back(ctrls);\n> +\t\top.controls.emplace_back(ctrls);\n> \t\tqueueFrameAction.emit(0, op);\n> \t}\n> }\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index bafd0b2d..bc7c56f8 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -753,8 +753,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n>\n> \t/* Start the IPA. */\n> \tIPAOperationData ipaData = {}, result = {};\n> -\tif (controls)\n> +\tif (controls) {\n> +\t\tipaData.operation = RPi::IPA_CONFIG_STARTUP;\n> \t\tipaData.controls.emplace_back(*controls);\n> +\t}\n> \tret = data->ipa_->start(ipaData, &result);\n> \tif (ret) {\n> \t\tLOG(RPI, Error)\n> @@ -763,6 +765,14 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n> \t\treturn ret;\n> \t}\n>\n> +\t/* Apply any gain/exposure settings that the IPA may have passed back. */\n> +\tASSERT(data->staggeredCtrl_);\n> +\tif (result.operation & RPi::IPA_CONFIG_SENSOR) {\n> +\t\tconst ControlList &ctrls = result.controls[0];\n> +\t\tif (!data->staggeredCtrl_.set(ctrls))\n> +\t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> +\t}\n> +\n> \t/*\n> \t * IPA configure may have changed the sensor flips - hence the bayer\n> \t * order. Get the sensor format and set the ISP input now.\n> @@ -783,7 +793,6 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n> \t * starting. First check that the staggered ctrl has been initialised\n> \t * by configure().\n> \t */\n> -\tASSERT(data->staggeredCtrl_);\n> \tdata->staggeredCtrl_.reset();\n> \tdata->staggeredCtrl_.write();\n> \tdata->expectedSequence_ = 0;\n> --\n> 2.25.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 D5B51BE176\n\tfor ;\n\tFri, 4 Dec 2020 16:14:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76F48635DE;\n\tFri, 4 Dec 2020 17:14:53 +0100 (CET)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9584D635D0\n\tfor ;\n\tFri, 4 Dec 2020 17:14:52 +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 relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 18F8E1C0002;\n\tFri, 4 Dec 2020 16:14:51 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 4 Dec 2020 17:14:58 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20201204161458.hpsdi4rozv7pzqdy@uno.localdomain>","References":"<20201204153121.66136-1-naush@raspberrypi.com>\n\t<20201204153121.66136-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201204153121.66136-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi:\n\tPass controls to IPA on start","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14085,"web_url":"https://patchwork.libcamera.org/comment/14085/","msgid":"","date":"2020-12-05T21:30:18","subject":"Re: [libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi:\n\tPass controls to IPA on start","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Fri, Dec 04, 2020 at 03:31:21PM +0000, Naushir Patuck wrote:\n> Forward any controls passed into the pipeline handler to the IPA.\n> The IPA then sets up the Raspberry Pi controller with these settings\n> appropriately, and passes back any V4L2 sensor controls that need\n> to be applied.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Tested-by: David Plowman \n> ---\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 54 ++++++++++++-------\n> .../pipeline/raspberrypi/raspberrypi.cpp | 13 ++++-\n> 3 files changed, 47 insertions(+), 21 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 2b55dbfc..c91a14bd 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -21,6 +21,7 @@ enum ConfigParameters {\n> \tIPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> \tIPA_CONFIG_SENSOR = (1 << 2),\n> \tIPA_CONFIG_DROP_FRAMES = (1 << 3),\n> +\tIPA_CONFIG_STARTUP = (1 << 4)\n\nCould you add a comma at the end of the line ?\n\n> };\n> \n> enum Operations {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index b8c0f955..ff95580e 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -78,8 +78,7 @@ public:\n> \t}\n> \n> \tint init(const IPASettings &settings) override;\n> -\tint start([[maybe_unused]] const IPAOperationData &data,\n> -\t\t [[maybe_unused]] IPAOperationData *result) override { return 0; }\n> +\tint start(const IPAOperationData &data, IPAOperationData *result) override;\n> \tvoid stop() override {}\n> \n> \tvoid configure(const CameraSensorInfo &sensorInfo,\n> @@ -154,6 +153,36 @@ int IPARPi::init(const IPASettings &settings)\n> \treturn 0;\n> }\n> \n> +int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)\n> +{\n> +\tRPiController::Metadata metadata;\n> +\n> +\tASSERT(result);\n> +\tresult->operation = 0;\n> +\tif (data.operation & RPi::IPA_CONFIG_STARTUP) {\n> +\t\t/* We have been given some controls to action before start. */\n> +\t\tqueueRequest(data.controls[0]);\n\nNot something to be addressed now, but do you foresee in the future a\nneed for algorithms to handle initial controls differently than the\ncontrols passed through requests ? Requests are synchronized to frames,\nand I could imagine algorithms potentially relying on that sequencing.\nReusing queueRequest() for initial controls would then interfere with\nthat mechanism.\n\n> +\t}\n> +\n> +\tcontroller_.SwitchMode(mode_, &metadata);\n> +\n> +\t/* SwitchMode may supply updated exposure/gain values to use. */\n> +\tAgcStatus agcStatus;\n> +\tagcStatus.shutter_time = 0.0;\n> +\tagcStatus.analogue_gain = 0.0;\n> +\n> +\t/* SwitchMode may supply updated exposure/gain values to use. */\n> +\tmetadata.Get(\"agc.status\", agcStatus);\n> +\tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> +\t\tControlList ctrls(unicamCtrls_);\n> +\t\tapplyAGC(&agcStatus, ctrls);\n> +\t\tresult->controls.emplace_back(ctrls);\n> +\t\tresult->operation |= RPi::IPA_CONFIG_SENSOR;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> {\n> \tmode_.bitdepth = sensorInfo.bitsPerPixel;\n> @@ -229,7 +258,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tresult->data.push_back(gainDelay);\n> \t\tresult->data.push_back(exposureDelay);\n> \t\tresult->data.push_back(sensorMetadata);\n> -\n\nUnintentional change ?\n\n> \t\tresult->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> \t}\n> \n> @@ -285,11 +313,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \tresult->data.push_back(dropFrame);\n> \tresult->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> \n> -\t/* These zero values mean not program anything (unless overwritten). */\n> -\tstruct AgcStatus agcStatus;\n> -\tagcStatus.shutter_time = 0.0;\n> -\tagcStatus.analogue_gain = 0.0;\n> -\n> \tif (!controllerInit_) {\n> \t\t/* Load the tuning file for this sensor. */\n> \t\tcontroller_.Read(tuningFile_.c_str());\n> @@ -297,20 +320,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tcontrollerInit_ = true;\n> \n> \t\t/* Supply initial values for gain and exposure. */\n> +\t\tControlList ctrls(unicamCtrls_);\n> +\t\tAgcStatus agcStatus;\n> \t\tagcStatus.shutter_time = DefaultExposureTime;\n> \t\tagcStatus.analogue_gain = DefaultAnalogueGain;\n> -\t}\n> -\n> -\tRPiController::Metadata metadata;\n> -\tcontroller_.SwitchMode(mode_, &metadata);\n> -\n> -\t/* SwitchMode may supply updated exposure/gain values to use. */\n> -\tmetadata.Get(\"agc.status\", agcStatus);\n> -\tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> -\t\tControlList ctrls(unicamCtrls_);\n> \t\tapplyAGC(&agcStatus, ctrls);\n> -\t\tresult->controls.push_back(ctrls);\n> \n> +\t\tresult->controls.emplace_back(ctrls);\n> \t\tresult->operation |= RPi::IPA_CONFIG_SENSOR;\n> \t}\n\nShould we skip this at configure() time now that we do it at start()\ntime ? Otherwise we'll set the controls twice.\n\n> \n> @@ -824,7 +840,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> \n> \t\tIPAOperationData op;\n> \t\top.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;\n> -\t\top.controls.push_back(ctrls);\n> +\t\top.controls.emplace_back(ctrls);\n> \t\tqueueFrameAction.emit(0, op);\n> \t}\n> }\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index bafd0b2d..bc7c56f8 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -753,8 +753,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n> \n> \t/* Start the IPA. */\n> \tIPAOperationData ipaData = {}, result = {};\n> -\tif (controls)\n> +\tif (controls) {\n> +\t\tipaData.operation = RPi::IPA_CONFIG_STARTUP;\n> \t\tipaData.controls.emplace_back(*controls);\n> +\t}\n> \tret = data->ipa_->start(ipaData, &result);\n> \tif (ret) {\n> \t\tLOG(RPI, Error)\n> @@ -763,6 +765,14 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n> \t\treturn ret;\n> \t}\n> \n> +\t/* Apply any gain/exposure settings that the IPA may have passed back. */\n> +\tASSERT(data->staggeredCtrl_);\n> +\tif (result.operation & RPi::IPA_CONFIG_SENSOR) {\n> +\t\tconst ControlList &ctrls = result.controls[0];\n> +\t\tif (!data->staggeredCtrl_.set(ctrls))\n> +\t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> +\t}\n> +\n> \t/*\n> \t * IPA configure may have changed the sensor flips - hence the bayer\n> \t * order. Get the sensor format and set the ISP input now.\n> @@ -783,7 +793,6 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n> \t * starting. First check that the staggered ctrl has been initialised\n> \t * by configure().\n> \t */\n> -\tASSERT(data->staggeredCtrl_);\n> \tdata->staggeredCtrl_.reset();\n> \tdata->staggeredCtrl_.write();\n> \tdata->expectedSequence_ = 0;","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 A1825BDB1F\n\tfor ;\n\tSat, 5 Dec 2020 21:30:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1FA38635F0;\n\tSat, 5 Dec 2020 22:30:22 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3D6B60327\n\tfor ;\n\tSat, 5 Dec 2020 22:30:20 +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 717F9D2F;\n\tSat, 5 Dec 2020 22:30:20 +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=\"Cpo7cSGt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607203820;\n\tbh=J2aqh78WuTu2sT2+W8m8toe+/6Mm/EKDfBHL6gJK8PE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Cpo7cSGtmQVNe04PHxIsybeYlSVEXJA9uovkGRljloFXorjTnae7DAJNu2pRtmDgn\n\twVELLsKHUyFgJoix4Ir30I3MnET54wjtLz0Qdk4ZLc7Dqq0KLirIzHByhXixpN8zXm\n\tnjZRBJt5n0M4+oCqB1JQFj1eCLWeeswErjDk1VpQ=","Date":"Sat, 5 Dec 2020 23:30:18 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20201204153121.66136-1-naush@raspberrypi.com>\n\t<20201204153121.66136-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201204153121.66136-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi:\n\tPass controls to IPA on start","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14125,"web_url":"https://patchwork.libcamera.org/comment/14125/","msgid":"","date":"2020-12-08T10:01:07","subject":"Re: [libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi:\n\tPass controls to IPA on start","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your review comments.\n\nOn Sat, 5 Dec 2020 at 21:30, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Fri, Dec 04, 2020 at 03:31:21PM +0000, Naushir Patuck wrote:\n> > Forward any controls passed into the pipeline handler to the IPA.\n> > The IPA then sets up the Raspberry Pi controller with these settings\n> > appropriately, and passes back any V4L2 sensor controls that need\n> > to be applied.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Tested-by: David Plowman \n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 1 +\n> > src/ipa/raspberrypi/raspberrypi.cpp | 54 ++++++++++++-------\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 13 ++++-\n> > 3 files changed, 47 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h\n> b/include/libcamera/ipa/raspberrypi.h\n> > index 2b55dbfc..c91a14bd 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -21,6 +21,7 @@ enum ConfigParameters {\n> > IPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> > IPA_CONFIG_SENSOR = (1 << 2),\n> > IPA_CONFIG_DROP_FRAMES = (1 << 3),\n> > + IPA_CONFIG_STARTUP = (1 << 4)\n>\n> Could you add a comma at the end of the line ?\n>\n> > };\n> >\n> > enum Operations {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index b8c0f955..ff95580e 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -78,8 +78,7 @@ public:\n> > }\n> >\n> > int init(const IPASettings &settings) override;\n> > - int start([[maybe_unused]] const IPAOperationData &data,\n> > - [[maybe_unused]] IPAOperationData *result) override {\n> return 0; }\n> > + int start(const IPAOperationData &data, IPAOperationData *result)\n> override;\n> > void stop() override {}\n> >\n> > void configure(const CameraSensorInfo &sensorInfo,\n> > @@ -154,6 +153,36 @@ int IPARPi::init(const IPASettings &settings)\n> > return 0;\n> > }\n> >\n> > +int IPARPi::start(const IPAOperationData &data, IPAOperationData\n> *result)\n> > +{\n> > + RPiController::Metadata metadata;\n> > +\n> > + ASSERT(result);\n> > + result->operation = 0;\n> > + if (data.operation & RPi::IPA_CONFIG_STARTUP) {\n> > + /* We have been given some controls to action before\n> start. */\n> > + queueRequest(data.controls[0]);\n>\n> Not something to be addressed now, but do you foresee in the future a\n> need for algorithms to handle initial controls differently than the\n> controls passed through requests ? Requests are synchronized to frames,\n> and I could imagine algorithms potentially relying on that sequencing.\n> Reusing queueRequest() for initial controls would then interfere with\n> that mechanism.\n>\n\nThis is an interesting question. There are certainly controls that can be\nactioned outside of Requests and frames. Right now it does not matter too\nmuch (if at all) for the Raspberry Pi implementation. To be honest, right\nnow I cannot think of concrete examples that would require this either. Do\nyou foresee a mechanism to set controls outside of the Request within\nlibcamera?\n\nRegards,\nNaush\n\n\n\n>\n> > + }\n> > +\n> > + controller_.SwitchMode(mode_, &metadata);\n> > +\n> > + /* SwitchMode may supply updated exposure/gain values to use. */\n> > + AgcStatus agcStatus;\n> > + agcStatus.shutter_time = 0.0;\n> > + agcStatus.analogue_gain = 0.0;\n> > +\n> > + /* SwitchMode may supply updated exposure/gain values to use. */\n> > + metadata.Get(\"agc.status\", agcStatus);\n> > + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain !=\n> 0.0) {\n> > + ControlList ctrls(unicamCtrls_);\n> > + applyAGC(&agcStatus, ctrls);\n> > + result->controls.emplace_back(ctrls);\n> > + result->operation |= RPi::IPA_CONFIG_SENSOR;\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> > {\n> > mode_.bitdepth = sensorInfo.bitsPerPixel;\n> > @@ -229,7 +258,6 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > result->data.push_back(gainDelay);\n> > result->data.push_back(exposureDelay);\n> > result->data.push_back(sensorMetadata);\n> > -\n>\n> Unintentional change ?\n>\n> > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> > }\n> >\n> > @@ -285,11 +313,6 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > result->data.push_back(dropFrame);\n> > result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> >\n> > - /* These zero values mean not program anything (unless\n> overwritten). */\n> > - struct AgcStatus agcStatus;\n> > - agcStatus.shutter_time = 0.0;\n> > - agcStatus.analogue_gain = 0.0;\n> > -\n> > if (!controllerInit_) {\n> > /* Load the tuning file for this sensor. */\n> > controller_.Read(tuningFile_.c_str());\n> > @@ -297,20 +320,13 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > controllerInit_ = true;\n> >\n> > /* Supply initial values for gain and exposure. */\n> > + ControlList ctrls(unicamCtrls_);\n> > + AgcStatus agcStatus;\n> > agcStatus.shutter_time = DefaultExposureTime;\n> > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > - }\n> > -\n> > - RPiController::Metadata metadata;\n> > - controller_.SwitchMode(mode_, &metadata);\n> > -\n> > - /* SwitchMode may supply updated exposure/gain values to use. */\n> > - metadata.Get(\"agc.status\", agcStatus);\n> > - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain !=\n> 0.0) {\n> > - ControlList ctrls(unicamCtrls_);\n> > applyAGC(&agcStatus, ctrls);\n> > - result->controls.push_back(ctrls);\n> >\n> > + result->controls.emplace_back(ctrls);\n> > result->operation |= RPi::IPA_CONFIG_SENSOR;\n> > }\n>\n> Should we skip this at configure() time now that we do it at start()\n> time ? Otherwise we'll set the controls twice.\n>\n> >\n> > @@ -824,7 +840,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> >\n> > IPAOperationData op;\n> > op.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;\n> > - op.controls.push_back(ctrls);\n> > + op.controls.emplace_back(ctrls);\n> > queueFrameAction.emit(0, op);\n> > }\n> > }\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index bafd0b2d..bc7c56f8 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -753,8 +753,10 @@ int PipelineHandlerRPi::start(Camera *camera,\n> [[maybe_unused]] ControlList *cont\n> >\n> > /* Start the IPA. */\n> > IPAOperationData ipaData = {}, result = {};\n> > - if (controls)\n> > + if (controls) {\n> > + ipaData.operation = RPi::IPA_CONFIG_STARTUP;\n> > ipaData.controls.emplace_back(*controls);\n> > + }\n> > ret = data->ipa_->start(ipaData, &result);\n> > if (ret) {\n> > LOG(RPI, Error)\n> > @@ -763,6 +765,14 @@ int PipelineHandlerRPi::start(Camera *camera,\n> [[maybe_unused]] ControlList *cont\n> > return ret;\n> > }\n> >\n> > + /* Apply any gain/exposure settings that the IPA may have passed\n> back. */\n> > + ASSERT(data->staggeredCtrl_);\n> > + if (result.operation & RPi::IPA_CONFIG_SENSOR) {\n> > + const ControlList &ctrls = result.controls[0];\n> > + if (!data->staggeredCtrl_.set(ctrls))\n> > + LOG(RPI, Error) << \"V4L2 staggered set failed\";\n> > + }\n> > +\n> > /*\n> > * IPA configure may have changed the sensor flips - hence the\n> bayer\n> > * order. Get the sensor format and set the ISP input now.\n> > @@ -783,7 +793,6 @@ int PipelineHandlerRPi::start(Camera *camera,\n> [[maybe_unused]] ControlList *cont\n> > * starting. First check that the staggered ctrl has been\n> initialised\n> > * by configure().\n> > */\n> > - ASSERT(data->staggeredCtrl_);\n> > data->staggeredCtrl_.reset();\n> > data->staggeredCtrl_.write();\n> > data->expectedSequence_ = 0;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 3757FBDB20\n\tfor ;\n\tTue, 8 Dec 2020 10:01:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB50167E6E;\n\tTue, 8 Dec 2020 11:01:28 +0100 (CET)","from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com\n\t[IPv6:2a00:1450:4864:20::12b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1212A635F2\n\tfor ;\n\tTue, 8 Dec 2020 11:01:26 +0100 (CET)","by mail-lf1-x12b.google.com with SMTP id a8so7469193lfb.3\n\tfor ;\n\tTue, 08 Dec 2020 02:01:26 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"GU3Ia+WU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=KOmx62b7H6ykz/Wk30/pCt5TnNwYDNWpFLvljsf6ke0=;\n\tb=GU3Ia+WU4xmDtC7z+KxLZ/jYNhb0eAmIvpb4hv45Lqbn9oTU2VTSMHJy/F8EzePRMd\n\tJoWk1qQ2TZEnJhSvwnp9RR0k7mdGqXueWJDwigpfn5MfDaSwHDejNgBdRYS5d/tXkQ9h\n\tEBxwuf/JR6L2B7XmC7Z5RknCNtSr5u530X1ksEPbNBZMybewJhBnvMuaWbzeUp1XNI+Q\n\t1ztXdZE5Ix69Pkm+xiuZ2ACYJ1QWio/q7Gig7BAePYL+sSmgXedU38CQx5H8JC4N/CX2\n\t5VsU/rm0af6/5FxN4kk34zhX1NKAXGSxXhjYq2hSvgtfr2pEHNq/g7Ce7Jno2C5P52bZ\n\tDy/w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=KOmx62b7H6ykz/Wk30/pCt5TnNwYDNWpFLvljsf6ke0=;\n\tb=grxA+3Ggr9yeCC2Oap8KEBNSMG+q69ipebqFj1wFzlArppGAjgJqMyt3+wF2GsCKCG\n\tr4vp471bHdOcI/Gx0fJnfiDpB7I7TAqg7H8eFtHtruD+rsJny8ICKNjK1Fnja8g5wu32\n\tTKjXlNnjDiiyAbLrTGlaI3ibtsoc0lPNrO70S7BzDepps0O6+Nel6AXdxA5rBvQws0rV\n\t9RyaI7UAl1velBkSd3wkFZfqLKR2mnrUk2qfwJzgzg982q2EJD2Qa2sG+Sy5h+RClO86\n\tZNMKdqT2RKlQg88GqQ4WrQ7urMoVS+vsD1dhQvk/Vf0/f7MeYwTAJSgNdJ3KbpLiyE0K\n\t/H/g==","X-Gm-Message-State":"AOAM530AlVXpWgGOhmLpFKwsAgMqQjSV/CXuaqOd9HRmFrtMXWETHc6v\n\tmoosRKXr8QF4Nhk8jnPe01VkNRsRLwOV4hkstcXLcw==","X-Google-Smtp-Source":"ABdhPJzAGyzMB/yzn4M4LOP1aj0KeGjraEfUb4VzG1suG/JPD2qS2ytSX53glhM8d/Q4yy6KQ1GRCvkUGORPmdYRmUk=","X-Received":"by 2002:a19:7f90:: with SMTP id a138mr54547lfd.617.1607421686294;\n\tTue, 08 Dec 2020 02:01:26 -0800 (PST)","MIME-Version":"1.0","References":"<20201204153121.66136-1-naush@raspberrypi.com>\n\t<20201204153121.66136-4-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 8 Dec 2020 10:01:07 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi:\n\tPass controls to IPA on start","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============0289596094075541851==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14131,"web_url":"https://patchwork.libcamera.org/comment/14131/","msgid":"","date":"2020-12-08T10:52:05","subject":"Re: [libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi:\n\tPass controls to IPA on start","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Dec 08, 2020 at 10:01:07AM +0000, Naushir Patuck wrote:\n> On Sat, 5 Dec 2020 at 21:30, Laurent Pinchart wrote:\n> > On Fri, Dec 04, 2020 at 03:31:21PM +0000, Naushir Patuck wrote:\n> > > Forward any controls passed into the pipeline handler to the IPA.\n> > > The IPA then sets up the Raspberry Pi controller with these settings\n> > > appropriately, and passes back any V4L2 sensor controls that need\n> > > to be applied.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > Tested-by: David Plowman \n> > > ---\n> > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 54 ++++++++++++-------\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 13 ++++-\n> > > 3 files changed, 47 insertions(+), 21 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > index 2b55dbfc..c91a14bd 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -21,6 +21,7 @@ enum ConfigParameters {\n> > > IPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> > > IPA_CONFIG_SENSOR = (1 << 2),\n> > > IPA_CONFIG_DROP_FRAMES = (1 << 3),\n> > > + IPA_CONFIG_STARTUP = (1 << 4)\n> >\n> > Could you add a comma at the end of the line ?\n> >\n> > > };\n> > >\n> > > enum Operations {\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index b8c0f955..ff95580e 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -78,8 +78,7 @@ public:\n> > > }\n> > >\n> > > int init(const IPASettings &settings) override;\n> > > - int start([[maybe_unused]] const IPAOperationData &data,\n> > > - [[maybe_unused]] IPAOperationData *result) override { return 0; }\n> > > + int start(const IPAOperationData &data, IPAOperationData *result) override;\n> > > void stop() override {}\n> > >\n> > > void configure(const CameraSensorInfo &sensorInfo,\n> > > @@ -154,6 +153,36 @@ int IPARPi::init(const IPASettings &settings)\n> > > return 0;\n> > > }\n> > >\n> > > +int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)\n> > > +{\n> > > + RPiController::Metadata metadata;\n> > > +\n> > > + ASSERT(result);\n> > > + result->operation = 0;\n> > > + if (data.operation & RPi::IPA_CONFIG_STARTUP) {\n> > > + /* We have been given some controls to action before start. */\n> > > + queueRequest(data.controls[0]);\n> >\n> > Not something to be addressed now, but do you foresee in the future a\n> > need for algorithms to handle initial controls differently than the\n> > controls passed through requests ? Requests are synchronized to frames,\n> > and I could imagine algorithms potentially relying on that sequencing.\n> > Reusing queueRequest() for initial controls would then interfere with\n> > that mechanism.\n> \n> This is an interesting question. There are certainly controls that can be\n> actioned outside of Requests and frames. Right now it does not matter too\n> much (if at all) for the Raspberry Pi implementation. To be honest, right\n> now I cannot think of concrete examples that would require this either. Do\n> you foresee a mechanism to set controls outside of the Request within\n> libcamera?\n\nIsn't this patch series adding such a mechanism ? ;-) I was just curious\nif you already saw a need to extend the controller API in this way in\nthe future. I suppose we'll see when/if the need arises.\n\n> > > + }\n> > > +\n> > > + controller_.SwitchMode(mode_, &metadata);\n> > > +\n> > > + /* SwitchMode may supply updated exposure/gain values to use. */\n> > > + AgcStatus agcStatus;\n> > > + agcStatus.shutter_time = 0.0;\n> > > + agcStatus.analogue_gain = 0.0;\n> > > +\n> > > + /* SwitchMode may supply updated exposure/gain values to use. */\n> > > + metadata.Get(\"agc.status\", agcStatus);\n> > > + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> > > + ControlList ctrls(unicamCtrls_);\n> > > + applyAGC(&agcStatus, ctrls);\n> > > + result->controls.emplace_back(ctrls);\n> > > + result->operation |= RPi::IPA_CONFIG_SENSOR;\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> > > {\n> > > mode_.bitdepth = sensorInfo.bitsPerPixel;\n> > > @@ -229,7 +258,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > result->data.push_back(gainDelay);\n> > > result->data.push_back(exposureDelay);\n> > > result->data.push_back(sensorMetadata);\n> > > -\n> >\n> > Unintentional change ?\n> >\n> > > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> > > }\n> > >\n> > > @@ -285,11 +313,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > result->data.push_back(dropFrame);\n> > > result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> > >\n> > > - /* These zero values mean not program anything (unless overwritten). */\n> > > - struct AgcStatus agcStatus;\n> > > - agcStatus.shutter_time = 0.0;\n> > > - agcStatus.analogue_gain = 0.0;\n> > > -\n> > > if (!controllerInit_) {\n> > > /* Load the tuning file for this sensor. */\n> > > controller_.Read(tuningFile_.c_str());\n> > > @@ -297,20 +320,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > controllerInit_ = true;\n> > >\n> > > /* Supply initial values for gain and exposure. */\n> > > + ControlList ctrls(unicamCtrls_);\n> > > + AgcStatus agcStatus;\n> > > agcStatus.shutter_time = DefaultExposureTime;\n> > > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > > - }\n> > > -\n> > > - RPiController::Metadata metadata;\n> > > - controller_.SwitchMode(mode_, &metadata);\n> > > -\n> > > - /* SwitchMode may supply updated exposure/gain values to use. */\n> > > - metadata.Get(\"agc.status\", agcStatus);\n> > > - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> > > - ControlList ctrls(unicamCtrls_);\n> > > applyAGC(&agcStatus, ctrls);\n> > > - result->controls.push_back(ctrls);\n> > >\n> > > + result->controls.emplace_back(ctrls);\n> > > result->operation |= RPi::IPA_CONFIG_SENSOR;\n> > > }\n> >\n> > Should we skip this at configure() time now that we do it at start()\n> > time ? Otherwise we'll set the controls twice.\n> >\n> > >\n> > > @@ -824,7 +840,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> > >\n> > > IPAOperationData op;\n> > > op.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;\n> > > - op.controls.push_back(ctrls);\n> > > + op.controls.emplace_back(ctrls);\n> > > queueFrameAction.emit(0, op);\n> > > }\n> > > }\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index bafd0b2d..bc7c56f8 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -753,8 +753,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n> > >\n> > > /* Start the IPA. */\n> > > IPAOperationData ipaData = {}, result = {};\n> > > - if (controls)\n> > > + if (controls) {\n> > > + ipaData.operation = RPi::IPA_CONFIG_STARTUP;\n> > > ipaData.controls.emplace_back(*controls);\n> > > + }\n> > > ret = data->ipa_->start(ipaData, &result);\n> > > if (ret) {\n> > > LOG(RPI, Error)\n> > > @@ -763,6 +765,14 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n> > > return ret;\n> > > }\n> > >\n> > > + /* Apply any gain/exposure settings that the IPA may have passed back. */\n> > > + ASSERT(data->staggeredCtrl_);\n> > > + if (result.operation & RPi::IPA_CONFIG_SENSOR) {\n> > > + const ControlList &ctrls = result.controls[0];\n> > > + if (!data->staggeredCtrl_.set(ctrls))\n> > > + LOG(RPI, Error) << \"V4L2 staggered set failed\";\n> > > + }\n> > > +\n> > > /*\n> > > * IPA configure may have changed the sensor flips - hence the bayer\n> > > * order. Get the sensor format and set the ISP input now.\n> > > @@ -783,7 +793,6 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n> > > * starting. First check that the staggered ctrl has been initialised\n> > > * by configure().\n> > > */\n> > > - ASSERT(data->staggeredCtrl_);\n> > > data->staggeredCtrl_.reset();\n> > > data->staggeredCtrl_.write();\n> > > data->expectedSequence_ = 0;","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 D6F7EBDB20\n\tfor ;\n\tTue, 8 Dec 2020 10:52:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4664F67E6E;\n\tTue, 8 Dec 2020 11:52:10 +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 8E2F2600FF\n\tfor ;\n\tTue, 8 Dec 2020 11:52:09 +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 F1BB2DD;\n\tTue, 8 Dec 2020 11:52:08 +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=\"S3vritY9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607424729;\n\tbh=20Zo5lleQ6b6CqCuBEZH0TzF5a1CZARi+HwYLPHeeoY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=S3vritY9vpAbQt2kJ2c8PUhMlDF3ys9o6goZfW+SrP71vDFGU4MSbh1Ebk4RhVgzO\n\teiZsAp9QZTCWj6csPZYuOhBDSsdS9DD65X1kYjUN0rWa5eG876ogpObKoI0jAD8K93\n\tPSmUXFqjwQE1HCs0/9w/MHly/wQaYdK5MmbrJses=","Date":"Tue, 8 Dec 2020 12:52:05 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20201204153121.66136-1-naush@raspberrypi.com>\n\t<20201204153121.66136-4-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi:\n\tPass controls to IPA on start","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14132,"web_url":"https://patchwork.libcamera.org/comment/14132/","msgid":"","date":"2020-12-08T11:03:05","subject":"Re: [libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi:\n\tPass controls to IPA on start","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 8 Dec 2020 at 10:52, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Tue, Dec 08, 2020 at 10:01:07AM +0000, Naushir Patuck wrote:\n> > On Sat, 5 Dec 2020 at 21:30, Laurent Pinchart wrote:\n> > > On Fri, Dec 04, 2020 at 03:31:21PM +0000, Naushir Patuck wrote:\n> > > > Forward any controls passed into the pipeline handler to the IPA.\n> > > > The IPA then sets up the Raspberry Pi controller with these settings\n> > > > appropriately, and passes back any V4L2 sensor controls that need\n> > > > to be applied.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: David Plowman \n> > > > Tested-by: David Plowman \n> > > > ---\n> > > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 54\n> ++++++++++++-------\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 13 ++++-\n> > > > 3 files changed, 47 insertions(+), 21 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/raspberrypi.h\n> b/include/libcamera/ipa/raspberrypi.h\n> > > > index 2b55dbfc..c91a14bd 100644\n> > > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > > @@ -21,6 +21,7 @@ enum ConfigParameters {\n> > > > IPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> > > > IPA_CONFIG_SENSOR = (1 << 2),\n> > > > IPA_CONFIG_DROP_FRAMES = (1 << 3),\n> > > > + IPA_CONFIG_STARTUP = (1 << 4)\n> > >\n> > > Could you add a comma at the end of the line ?\n> > >\n> > > > };\n> > > >\n> > > > enum Operations {\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index b8c0f955..ff95580e 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -78,8 +78,7 @@ public:\n> > > > }\n> > > >\n> > > > int init(const IPASettings &settings) override;\n> > > > - int start([[maybe_unused]] const IPAOperationData &data,\n> > > > - [[maybe_unused]] IPAOperationData *result) override\n> { return 0; }\n> > > > + int start(const IPAOperationData &data, IPAOperationData\n> *result) override;\n> > > > void stop() override {}\n> > > >\n> > > > void configure(const CameraSensorInfo &sensorInfo,\n> > > > @@ -154,6 +153,36 @@ int IPARPi::init(const IPASettings &settings)\n> > > > return 0;\n> > > > }\n> > > >\n> > > > +int IPARPi::start(const IPAOperationData &data, IPAOperationData\n> *result)\n> > > > +{\n> > > > + RPiController::Metadata metadata;\n> > > > +\n> > > > + ASSERT(result);\n> > > > + result->operation = 0;\n> > > > + if (data.operation & RPi::IPA_CONFIG_STARTUP) {\n> > > > + /* We have been given some controls to action before\n> start. */\n> > > > + queueRequest(data.controls[0]);\n> > >\n> > > Not something to be addressed now, but do you foresee in the future a\n> > > need for algorithms to handle initial controls differently than the\n> > > controls passed through requests ? Requests are synchronized to frames,\n> > > and I could imagine algorithms potentially relying on that sequencing.\n> > > Reusing queueRequest() for initial controls would then interfere with\n> > > that mechanism.\n> >\n> > This is an interesting question. There are certainly controls that can\n> be\n> > actioned outside of Requests and frames. Right now it does not matter\n> too\n> > much (if at all) for the Raspberry Pi implementation. To be honest,\n> right\n> > now I cannot think of concrete examples that would require this either.\n> Do\n> > you foresee a mechanism to set controls outside of the Request within\n> > libcamera?\n>\n> Isn't this patch series adding such a mechanism ? ;-) I was just curious\n> if you already saw a need to extend the controller API in this way in\n> the future. I suppose we'll see when/if the need arises.\n>\n\nIndeed it does :)\nI was thinking more along the lines of after startup, but again, I don't\nhave a particular use case or example in might right now, so no point\ntrying to do this just yet.\n\nRegards,\nNaush\n\n\n> > > > + }\n> > > > +\n> > > > + controller_.SwitchMode(mode_, &metadata);\n> > > > +\n> > > > + /* SwitchMode may supply updated exposure/gain values to use.\n> */\n> > > > + AgcStatus agcStatus;\n> > > > + agcStatus.shutter_time = 0.0;\n> > > > + agcStatus.analogue_gain = 0.0;\n> > > > +\n> > > > + /* SwitchMode may supply updated exposure/gain values to use.\n> */\n> > > > + metadata.Get(\"agc.status\", agcStatus);\n> > > > + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain\n> != 0.0) {\n> > > > + ControlList ctrls(unicamCtrls_);\n> > > > + applyAGC(&agcStatus, ctrls);\n> > > > + result->controls.emplace_back(ctrls);\n> > > > + result->operation |= RPi::IPA_CONFIG_SENSOR;\n> > > > + }\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> > > > {\n> > > > mode_.bitdepth = sensorInfo.bitsPerPixel;\n> > > > @@ -229,7 +258,6 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > > > result->data.push_back(gainDelay);\n> > > > result->data.push_back(exposureDelay);\n> > > > result->data.push_back(sensorMetadata);\n> > > > -\n> > >\n> > > Unintentional change ?\n> > >\n> > > > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> > > > }\n> > > >\n> > > > @@ -285,11 +313,6 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > > > result->data.push_back(dropFrame);\n> > > > result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> > > >\n> > > > - /* These zero values mean not program anything (unless\n> overwritten). */\n> > > > - struct AgcStatus agcStatus;\n> > > > - agcStatus.shutter_time = 0.0;\n> > > > - agcStatus.analogue_gain = 0.0;\n> > > > -\n> > > > if (!controllerInit_) {\n> > > > /* Load the tuning file for this sensor. */\n> > > > controller_.Read(tuningFile_.c_str());\n> > > > @@ -297,20 +320,13 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > > > controllerInit_ = true;\n> > > >\n> > > > /* Supply initial values for gain and exposure. */\n> > > > + ControlList ctrls(unicamCtrls_);\n> > > > + AgcStatus agcStatus;\n> > > > agcStatus.shutter_time = DefaultExposureTime;\n> > > > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > > > - }\n> > > > -\n> > > > - RPiController::Metadata metadata;\n> > > > - controller_.SwitchMode(mode_, &metadata);\n> > > > -\n> > > > - /* SwitchMode may supply updated exposure/gain values to use.\n> */\n> > > > - metadata.Get(\"agc.status\", agcStatus);\n> > > > - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain\n> != 0.0) {\n> > > > - ControlList ctrls(unicamCtrls_);\n> > > > applyAGC(&agcStatus, ctrls);\n> > > > - result->controls.push_back(ctrls);\n> > > >\n> > > > + result->controls.emplace_back(ctrls);\n> > > > result->operation |= RPi::IPA_CONFIG_SENSOR;\n> > > > }\n> > >\n> > > Should we skip this at configure() time now that we do it at start()\n> > > time ? Otherwise we'll set the controls twice.\n> > >\n> > > >\n> > > > @@ -824,7 +840,7 @@ void IPARPi::processStats(unsigned int bufferId)\n> > > >\n> > > > IPAOperationData op;\n> > > > op.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;\n> > > > - op.controls.push_back(ctrls);\n> > > > + op.controls.emplace_back(ctrls);\n> > > > queueFrameAction.emit(0, op);\n> > > > }\n> > > > }\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index bafd0b2d..bc7c56f8 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -753,8 +753,10 @@ int PipelineHandlerRPi::start(Camera *camera,\n> [[maybe_unused]] ControlList *cont\n> > > >\n> > > > /* Start the IPA. */\n> > > > IPAOperationData ipaData = {}, result = {};\n> > > > - if (controls)\n> > > > + if (controls) {\n> > > > + ipaData.operation = RPi::IPA_CONFIG_STARTUP;\n> > > > ipaData.controls.emplace_back(*controls);\n> > > > + }\n> > > > ret = data->ipa_->start(ipaData, &result);\n> > > > if (ret) {\n> > > > LOG(RPI, Error)\n> > > > @@ -763,6 +765,14 @@ int PipelineHandlerRPi::start(Camera *camera,\n> [[maybe_unused]] ControlList *cont\n> > > > return ret;\n> > > > }\n> > > >\n> > > > + /* Apply any gain/exposure settings that the IPA may have\n> passed back. */\n> > > > + ASSERT(data->staggeredCtrl_);\n> > > > + if (result.operation & RPi::IPA_CONFIG_SENSOR) {\n> > > > + const ControlList &ctrls = result.controls[0];\n> > > > + if (!data->staggeredCtrl_.set(ctrls))\n> > > > + LOG(RPI, Error) << \"V4L2 staggered set failed\";\n> > > > + }\n> > > > +\n> > > > /*\n> > > > * IPA configure may have changed the sensor flips - hence the\n> bayer\n> > > > * order. Get the sensor format and set the ISP input now.\n> > > > @@ -783,7 +793,6 @@ int PipelineHandlerRPi::start(Camera *camera,\n> [[maybe_unused]] ControlList *cont\n> > > > * starting. First check that the staggered ctrl has been\n> initialised\n> > > > * by configure().\n> > > > */\n> > > > - ASSERT(data->staggeredCtrl_);\n> > > > data->staggeredCtrl_.reset();\n> > > > data->staggeredCtrl_.write();\n> > > > data->expectedSequence_ = 0;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 54B02BDB1F\n\tfor ;\n\tTue, 8 Dec 2020 11:03:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A567867E6F;\n\tTue, 8 Dec 2020 12:03:24 +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 EE9EE67E12\n\tfor ;\n\tTue, 8 Dec 2020 12:03:22 +0100 (CET)","by mail-lj1-x243.google.com with SMTP id a1so17280323ljq.3\n\tfor ;\n\tTue, 08 Dec 2020 03:03:22 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"qkNw/g8d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=dk8qz0nEjsVwPQIYw8gV7UTh0t/u2IWzTq/9rM8hL0A=;\n\tb=qkNw/g8dvD7vm+rbkMtvg5wHsREbwqgVRN1moSCCwt0y1qz3gY9iJG6bGMNVzbSer0\n\t4c0MP1dGFoOqWbHKCo+LgUBlxlGgsw8JrZ6XilG21tdV0zW6taBUKsNSu4qAhRLSJgvD\n\tlNcwKjP0/91lm2wsQFj69hWJ93RsQzSbTKh2pOyw/b13AmTYL0tBKK1A3b6p4grcu1DK\n\t+/JU3YJaJ8e6HQk2LWt1B5huVYSIbue5cQLt0503JylqnCr6kVtbnh7O9a/rKrAPWQe3\n\tkrl9xN+4jdtNT1d9Y0d8fm/L04+1eSg5A43XVQgnmKlbEO0pG1chqtFGGH8gXu7+cAPI\n\tKdqA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=dk8qz0nEjsVwPQIYw8gV7UTh0t/u2IWzTq/9rM8hL0A=;\n\tb=QG7oteQYAVixyKpLVNDzXX/dvz5kkki44E8Ejklrk/mwfaZ5iATExkxcrGF3ICtVzr\n\tLzIASJ3wz3IbGLNhvGipHJ7pTOYY1Rkp8l4g/VqvKG+4BGVkVY0eNSaWXvZPT5uxRNPx\n\tPsFcsEub5amJWgOAF62vUYS1xuNBcbKE8tkKMWqSxq8E5szvmKOoVFPUPLbclxzEQ5Jb\n\taXCIRw967oouDLOXbG6i5LJFfCMeZFuwybFXax2cJl6kpJxc6eDzBFnix7V8vJXI5hWJ\n\tSa85jgpw33CsMBATasQtMNcgTmDPPcGCzk33RJdPikZ0t8TzinOUYeW4mY+2RpnGWawg\n\ttrCg==","X-Gm-Message-State":"AOAM533zFMYMAi+5L37KYcwzSNOwOWFkVY7DCI1StwxgxdlkF9OPqCw+\n\tDFlhn3DYmsQ9nmQMbCfywjDJqdUmlsMZFhyy/GUMS1/apt4=","X-Google-Smtp-Source":"ABdhPJzgDpBhq9SXmkJdR8BlmJqGB491kivNNipwIQutiM2Gr3FL7F6wOCZr/8MyHu2kpuSJw364NtuxcF7/u0cToqw=","X-Received":"by 2002:a2e:a58b:: with SMTP id\n\tm11mr5014968ljp.329.1607425402330; \n\tTue, 08 Dec 2020 03:03:22 -0800 (PST)","MIME-Version":"1.0","References":"<20201204153121.66136-1-naush@raspberrypi.com>\n\t<20201204153121.66136-4-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 8 Dec 2020 11:03:05 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi:\n\tPass controls to IPA on start","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============7316799111045512900==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]