[{"id":13762,"web_url":"https://patchwork.libcamera.org/comment/13762/","msgid":"","date":"2020-11-17T16:36:07","subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: ipa: raspberrypi: Pass\n\tcontrols to IPA on start","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for the patch! Only a couple of very minor things...\n\nOn Thu, 12 Nov 2020 at 08:59, Naushir Patuck wrote:\n>\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> ---\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++-------\n> .../pipeline/raspberrypi/raspberrypi.cpp | 14 ++++-\n> 3 files changed, 46 insertions(+), 20 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>\n> enum Operations {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 7a07b477..d4471f02 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -77,8 +77,7 @@ public:\n> }\n>\n> int init(const IPASettings &settings) override;\n> - int start([[maybe_unused]] const IPAOperationData &ipaConfig,\n> - [[maybe_unused]] IPAOperationData *result) override { return 0; }\n> + int start(const IPAOperationData &ipaConfig, IPAOperationData *result) override;\n> void stop() override {}\n>\n> void configure(const CameraSensorInfo &sensorInfo,\n> @@ -154,6 +153,35 @@ int IPARPi::init(const IPASettings &settings)\n> return 0;\n> }\n>\n> +int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)\n> +{\n> + RPiController::Metadata metadata;\n> +\n> + result->operation = 0;\n\nAh, I see we zero this here, answering some of my questions about the\nprevious patch!\n\n> + if (ipaConfig.operation & RPi::IPA_CONFIG_STARTUP) {\n> + /* We have been given some controls to action before start. */\n> + queueRequest(ipaConfig.controls[0]);\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.push_back(ctrls);\n\nI wonder if there's just a little bit more copying of controls going\non here than is strictly necessary (maybe emplace_back?), but I agree\nthat it's entirely inconsequential.\n\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 +257,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> result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> }\n>\n> @@ -285,11 +312,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 +319,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.push_back(ctrls);\n\nSame comment here. Though I wonder if maybe I wrote this originally,\nwhich would make it all my fault (most things are!).\n\n> result->operation |= RPi::IPA_CONFIG_SENSOR;\n> }\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index a8e4997a..6efe2403 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -748,7 +748,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n>\n> /* Start the IPA. */\n> IPAOperationData ipaConfig, result;\n> - ipaConfig.controls.emplace_back(*controls);\n> + if (controls) {\n> + ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;\n> + ipaConfig.controls.emplace_back(*controls);\n> + }\n\nI guess this is the spot where I'd just want to convince myself\nnothing bad can happen if ipaConfig.operation is undefined but\ncontrols was NULL. I'm probably worrying over nothing...\n\nThose little things aside:\n\nReviewed-by: David Plowman \nTested-by: David Plowman \n\nThanks and best regards\nDavid\n\n> ret = data->ipa_->start(ipaConfig, &result);\n> if (ret) {\n> LOG(RPI, Error)\n> @@ -757,6 +760,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> @@ -777,7 +788,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;\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 61F2CBE082\n\tfor ;\n\tTue, 17 Nov 2020 16:36:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 242536033B;\n\tTue, 17 Nov 2020 17:36:21 +0100 (CET)","from mail-oi1-x241.google.com (mail-oi1-x241.google.com\n\t[IPv6:2607:f8b0:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A0A46033B\n\tfor ;\n\tTue, 17 Nov 2020 17:36:19 +0100 (CET)","by mail-oi1-x241.google.com with SMTP id t143so23168708oif.10\n\tfor ;\n\tTue, 17 Nov 2020 08:36:19 -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=\"p9E12Qmb\"; 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=kN0izEaGzQOKzmh1vmRs3f5OFZ8eeZfrTSzGit11IY8=;\n\tb=p9E12QmbU4i81pKoZqZI3b7F3sm+Y8Q3mQo2/XQDbIK2mzI0Yqj9WkMyjti6kCScg+\n\tXDfROxz3Rq5FRzme7FNHP1dRSY9ZCiuO05y+/I00LyNnjQhtMfikU+00JZNSWxbhr0Es\n\tsNeQUY/R1wyuMT3hR+KG7hraQG8DTOwXjPvGjuOs4mcuucPligWecIg+D+Vw/pXUpzaY\n\tkTQjrqj8huvqjVo2SMaoqpdPnoNagvPETBi/6DiHvy4+cKpuskGSnn1A8zOBW9VlhSXv\n\trZTHXKnmS9boGW82idXk1hd/nSBgaq+XwmyQ+oDjqUNRatpuQ0o5rPnkrWAigpFG6jkD\n\tgifg==","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=kN0izEaGzQOKzmh1vmRs3f5OFZ8eeZfrTSzGit11IY8=;\n\tb=Te6c03v7nssABC6CXfAqCmIgwzvCov1VJJ4yE4LrdMJWoMO4pNt+NGP7p+pZbuGhAk\n\tPgeg/qnF66u38535EGQYT4hKIYY7F8jMnca/yk0qlZOevNhe+Hg6VEN7c8mJh6jutkoS\n\tcP5iiMJ98/RdvTJPOAyG+FU84JmGIwfx7S5bBsgQbw/Xa1N3SWsZ/7VLexwiRl1ArGOs\n\t+uahZMDybotvHGmJBHxI4VhUODO4Sd66AGxesbrAlnO2uaAQF6/Xw2BxBXwMPztvXx+I\n\tFwcHq1ztAJWyzNXRTeUeNdvjV7Opa5ycGF0LxeTiNSxb9EbT8+pSTtr2kEKq9cpFCCfM\n\t07qQ==","X-Gm-Message-State":"AOAM531HAwPO+63w6vJGc+oDKhvfOfl6hB4ZifMiNyshKAYcMjggdPgW\n\tC+YnQR91mFDSAMMLlnVBjPvIyk208mTdTQBGrIPZvYYwRlzQvg==","X-Google-Smtp-Source":"ABdhPJzmINqGo3hRFIHMPKBZ+AtSkeMOnhIPdMd161+booqp8jxKpASxoJtIdHKqRD2tuLF/DVEuyousj01B0ZvMaH8=","X-Received":"by 2002:aca:c589:: with SMTP id\n\tv131mr2700225oif.55.1605630978128; \n\tTue, 17 Nov 2020 08:36:18 -0800 (PST)","MIME-Version":"1.0","References":"<20201112085915.3053-1-naush@raspberrypi.com>\n\t<20201112085915.3053-4-naush@raspberrypi.com>","In-Reply-To":"<20201112085915.3053-4-naush@raspberrypi.com>","From":"David Plowman ","Date":"Tue, 17 Nov 2020 16:36:07 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: ipa: raspberrypi: Pass\n\tcontrols 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\" "}}]