[{"id":21939,"web_url":"https://patchwork.libcamera.org/comment/21939/","msgid":"<164129482623.3986460.10407047347806659820@Monstersaurus>","date":"2022-01-04T11:13:46","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2021-09-22 14:29:15)\n> These changes retrieve the correct value for sensitivity of the mode\n> selected for the sensor. This value is known to the CamHelper which\n> passes it across to the pipeline handler so that it can be set\n> correctly in the CameraConfiguration.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---\n> 3 files changed, 18 insertions(+), 6 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index e453d46c..a92a76f8 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -38,6 +38,10 @@ struct IPAConfig {\n> libcamera.FileDescriptor lsTableHandle;\n> };\n> \n> +struct IPAConfigResult {\n> + float modeSensitivity;\n> +};\n> +\n> struct StartConfig {\n> libcamera.ControlList controls;\n> int32 dropFrameCount;\n> @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> * \\param[in] entityControls Controls provided by the pipeline entities\n> * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> * \\param[out] controls Controls to apply by the pipeline entity\n> + * \\param[out] result Other results that the pipeline handler may require\n> *\n> * This function shall be called when the camera is configured to inform\n> * the IPA of the camera's streams and the sensor settings.\n> @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> map streamConfig,\n> map entityControls,\n> IPAConfig ipaConfig)\n> - => (int32 ret, libcamera.ControlList controls);\n> + => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);\n> \n> /**\n> * \\fn mapBuffers()\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 047123ab..796e6d15 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -97,7 +97,7 @@ public:\n> const std::map &streamConfig,\n> const std::map &entityControls,\n> const ipa::RPi::IPAConfig &data,\n> - ControlList *controls) override;\n> + ControlList *controls, ipa::RPi::IPAConfigResult *result) override;\n> void mapBuffers(const std::vector &buffers) override;\n> void unmapBuffers(const std::vector &ids) override;\n> void signalStatReady(const uint32_t bufferId) override;\n> @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> [[maybe_unused]] const std::map &streamConfig,\n> const std::map &entityControls,\n> const ipa::RPi::IPAConfig &ipaConfig,\n> - ControlList *controls)\n> + ControlList *controls, ipa::RPi::IPAConfigResult *result)\n> {\n> if (entityControls.size() != 2) {\n> LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> /* Pass the camera mode to the CamHelper to setup algorithms. */\n> helper_->SetCameraMode(mode_);\n> \n> + /* The pipeline handler passes out the mode's sensitivity. */\n> + result->modeSensitivity = mode_.sensitivity;\n> +\n> if (firstStart_) {\n> /* Supply initial values for frame durations. */\n> applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 0bdfa727..caf0030e 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -147,7 +147,7 @@ public:\n> void frameStarted(uint32_t sequence);\n> \n> int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> - int configureIPA(const CameraConfiguration *config);\n> + int configureIPA(CameraConfiguration *config);\n> \n> void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> void runIsp(uint32_t bufferId);\n> @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> return ipa_->init(settings, sensorConfig);\n> }\n> \n> -int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> {\n> /* We know config must be an RPiCameraConfiguration. */\n> const RPiCameraConfiguration *rpiConfig =\n> @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \n> /* Ready the IPA - it must know about the sensor resolution. */\n> ControlList controls;\n> + ipa::RPi::IPAConfigResult result;\n> ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> - &controls);\n> + &controls, &result);\n\nThis should probably be being passed back as a control/property here.\nBut I think right now the &controls is a set of V4L2 Controls, and not\nlibcamera controls.\n\nWe've been discussing about changing that so that the IPAs pass\nlibcamera::controls instead. Currently Isolated IPAs will break when we\ntry to pass lens controls because that's 'yet another infomap' that will\nhave to be serialised.\n\nIf we instead use only libcamera controls in the interface between the\npipeline handler and the IPA this would be resolved, and it would mean\nthat list could also pass the sensitivity property.\n\nIs there any reason you're aware of as to why we could or couldn't use\nlibcamera controls here? I know Laurent has been concerned in the past\nabout perhaps if there was any precision issues or rounding that the IPA\nmight need to know about that would otherwise occur in the pipeline\nhandler. Do you see this as an issue? or would we be able to\nsuccessfully convert the interfaces to use libcamera control lists only\n(and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler\nonly).\n\nAll that said, changing the controls interface there is still possible\non top, even with this patch, so I don't object to passing it directly\nback for now.\n\n\nReviewed-by: Kieran Bingham \n\n--\nKieran\n\n\n> if (ret < 0) {\n> LOG(RPI, Error) << \"IPA configuration failed!\";\n> return -EPIPE;\n> }\n> \n> + /* Store the mode sensitivity for the application. */\n> + config->modeSensitivity = result.modeSensitivity;\n> +\n> /*\n> * Configure the H/V flip controls based on the combination of\n> * the sensor and user transform.\n> -- \n> 2.20.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 6AD32BF415\n\tfor ;\n\tTue, 4 Jan 2022 11:13:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A795360921;\n\tTue, 4 Jan 2022 12:13:50 +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 7A90D604F4\n\tfor ;\n\tTue, 4 Jan 2022 12:13:49 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 12EDB501;\n\tTue, 4 Jan 2022 12:13:49 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BA9VLVur\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641294829;\n\tbh=rvrRBSwFZ82RTdbxxCMuDtjk4Qh05UzD+nakaQsBp3A=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=BA9VLVurKvFVoxHDIc4txtRf3yeEp31kZF1EExXE6E50DW46VPnV1toUlHnjlQwrX\n\tUASMgM/dUPqdoELNOh6yms6AskSUxReo5IKHLbOT6ulk/QZt5/Sw34mce0MdeuRoZa\n\tVdKKz/IzR+ygiEcZtoW6xsOXuCqiUO0/J245uUCE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20210922132915.14881-3-david.plowman@raspberrypi.com>","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 04 Jan 2022 11:13:46 +0000","Message-ID":"<164129482623.3986460.10407047347806659820@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21945,"web_url":"https://patchwork.libcamera.org/comment/21945/","msgid":"","date":"2022-01-04T13:33:05","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi again\n\nOn Tue, 4 Jan 2022 at 11:13, Kieran Bingham\n wrote:\n>\n> Quoting David Plowman (2021-09-22 14:29:15)\n> > These changes retrieve the correct value for sensitivity of the mode\n> > selected for the sensor. This value is known to the CamHelper which\n> > passes it across to the pipeline handler so that it can be set\n> > correctly in the CameraConfiguration.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---\n> > 3 files changed, 18 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > index e453d46c..a92a76f8 100644\n> > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -38,6 +38,10 @@ struct IPAConfig {\n> > libcamera.FileDescriptor lsTableHandle;\n> > };\n> >\n> > +struct IPAConfigResult {\n> > + float modeSensitivity;\n> > +};\n> > +\n> > struct StartConfig {\n> > libcamera.ControlList controls;\n> > int32 dropFrameCount;\n> > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> > * \\param[in] entityControls Controls provided by the pipeline entities\n> > * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> > * \\param[out] controls Controls to apply by the pipeline entity\n> > + * \\param[out] result Other results that the pipeline handler may require\n> > *\n> > * This function shall be called when the camera is configured to inform\n> > * the IPA of the camera's streams and the sensor settings.\n> > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> > map streamConfig,\n> > map entityControls,\n> > IPAConfig ipaConfig)\n> > - => (int32 ret, libcamera.ControlList controls);\n> > + => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);\n> >\n> > /**\n> > * \\fn mapBuffers()\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 047123ab..796e6d15 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -97,7 +97,7 @@ public:\n> > const std::map &streamConfig,\n> > const std::map &entityControls,\n> > const ipa::RPi::IPAConfig &data,\n> > - ControlList *controls) override;\n> > + ControlList *controls, ipa::RPi::IPAConfigResult *result) override;\n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> > void signalStatReady(const uint32_t bufferId) override;\n> > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> > [[maybe_unused]] const std::map &streamConfig,\n> > const std::map &entityControls,\n> > const ipa::RPi::IPAConfig &ipaConfig,\n> > - ControlList *controls)\n> > + ControlList *controls, ipa::RPi::IPAConfigResult *result)\n> > {\n> > if (entityControls.size() != 2) {\n> > LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> > /* Pass the camera mode to the CamHelper to setup algorithms. */\n> > helper_->SetCameraMode(mode_);\n> >\n> > + /* The pipeline handler passes out the mode's sensitivity. */\n> > + result->modeSensitivity = mode_.sensitivity;\n> > +\n> > if (firstStart_) {\n> > /* Supply initial values for frame durations. */\n> > applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 0bdfa727..caf0030e 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -147,7 +147,7 @@ public:\n> > void frameStarted(uint32_t sequence);\n> >\n> > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > - int configureIPA(const CameraConfiguration *config);\n> > + int configureIPA(CameraConfiguration *config);\n> >\n> > void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> > void runIsp(uint32_t bufferId);\n> > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> > return ipa_->init(settings, sensorConfig);\n> > }\n> >\n> > -int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> > {\n> > /* We know config must be an RPiCameraConfiguration. */\n> > const RPiCameraConfiguration *rpiConfig =\n> > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> >\n> > /* Ready the IPA - it must know about the sensor resolution. */\n> > ControlList controls;\n> > + ipa::RPi::IPAConfigResult result;\n> > ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> > - &controls);\n> > + &controls, &result);\n>\n> This should probably be being passed back as a control/property here.\n> But I think right now the &controls is a set of V4L2 Controls, and not\n> libcamera controls.\n>\n> We've been discussing about changing that so that the IPAs pass\n> libcamera::controls instead. Currently Isolated IPAs will break when we\n> try to pass lens controls because that's 'yet another infomap' that will\n> have to be serialised.\n>\n> If we instead use only libcamera controls in the interface between the\n> pipeline handler and the IPA this would be resolved, and it would mean\n> that list could also pass the sensitivity property.\n>\n> Is there any reason you're aware of as to why we could or couldn't use\n> libcamera controls here? I know Laurent has been concerned in the past\n> about perhaps if there was any precision issues or rounding that the IPA\n> might need to know about that would otherwise occur in the pipeline\n> handler. Do you see this as an issue? or would we be able to\n> successfully convert the interfaces to use libcamera control lists only\n> (and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler\n> only).\n\nI can't think of any precision issues that particularly bother me\n(which of course doesn't mean there aren't any). As I said in my other\nreply, I do need this value to be available from the call to\nconfigure(), before you get to call start(), so it's not clear to me\nat the minute where I would read this value back - but I'm guessing\nsomeone has some ideas?\n\nDavid\n\n>\n> All that said, changing the controls interface there is still possible\n> on top, even with this patch, so I don't object to passing it directly\n> back for now.\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n> --\n> Kieran\n>\n>\n> > if (ret < 0) {\n> > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > return -EPIPE;\n> > }\n> >\n> > + /* Store the mode sensitivity for the application. */\n> > + config->modeSensitivity = result.modeSensitivity;\n> > +\n> > /*\n> > * Configure the H/V flip controls based on the combination of\n> > * the sensor and user transform.\n> > --\n> > 2.20.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 6315EBE080\n\tfor ;\n\tTue, 4 Jan 2022 13:33:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6734460921;\n\tTue, 4 Jan 2022 14:33:17 +0100 (CET)","from mail-wr1-x433.google.com (mail-wr1-x433.google.com\n\t[IPv6:2a00:1450:4864:20::433])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4C173604F4\n\tfor ;\n\tTue, 4 Jan 2022 14:33:16 +0100 (CET)","by mail-wr1-x433.google.com with SMTP id q16so76233936wrg.7\n\tfor ;\n\tTue, 04 Jan 2022 05:33:16 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Qo5hjyTb\"; 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=6q1r6j528pEGsY6HVWS17r/cDtjNrb7K386Eb34E5Kc=;\n\tb=Qo5hjyTbeW5qbnHBO+7ET5QGAmAUVxaHbEyURfCKJ2ZBScwD5892mq0++U2oZmHMl5\n\tv9Zz495gYZgO5MxylaqsQprTBSISY0COWsUk4bSiQYYG79WYEvr5fy6csYyPQjSzs4Gn\n\tUxow/BuqK2KXsTS3Te2G9A/MTymt27oEq7QAqQmTcP6Ae4IjEKq7Z/LeBrxiphqiPzw9\n\t7XKtGr3/vPbi6rC866z+UeWJF+Vhu7LLlHfOgX323d3JaTgp9oVVwd7v6jCSx0gOJjp0\n\trac0wJwdvfQJWJ8BBSIyby9tfz3FoSUZ9Kg13Hqa726fzC9IZghkhgZp8EONtoFq79+x\n\t9WqQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=6q1r6j528pEGsY6HVWS17r/cDtjNrb7K386Eb34E5Kc=;\n\tb=LpzGhXpBvLZD29DJoTylX0u/O6Ay6NIuglNOp7lELLmd0X0ifNZOPVBr4Zjd37cLLX\n\tVdbdP8jKn6hOOmWTqEPQngLEpgjlOfWNHeah1zhkYsh9RQXrU+fKO24tMHz38FQyMOmJ\n\tK9LJsuPVmHahkwgs3MpZuPQO0UqK6Enl/n2c9v9cL5KD0b1OW7GUGgg1MkVrLyXVgCMP\n\tj4E3oIClcDN+h9iDziLOUhB/mVDiFhEx4qeWDY/HEAikOBppaN6jbPY+MvQUZdGGTUiB\n\tJaNHxStQFev5Ujnzw99CnHxFCbBKuA9iX/NH5ASuuNDjAFfqlvz5kUGjkuEPyPS7q5GX\n\tHifQ==","X-Gm-Message-State":"AOAM532U/4++0pUIwA0Fsf7VFXT79ff7D5nnLxAO8GXmUyysyGwYtGoy\n\tdB3IFzy0Gvn0tY9LtlePmy37wUfxSlJd2pp01Nd+oOsiddI=","X-Google-Smtp-Source":"ABdhPJwjOSns88gwl4GJMt4T8kEtavPlYvyYsx1i9pEcfpyQuAWRO8P3k3Pql0r53Uzmt8ieN2Jcg/tdMIq/AV1Ru6g=","X-Received":"by 2002:adf:a352:: with SMTP id\n\td18mr42241971wrb.317.1641303195899; \n\tTue, 04 Jan 2022 05:33:15 -0800 (PST)","MIME-Version":"1.0","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164129482623.3986460.10407047347806659820@Monstersaurus>","In-Reply-To":"<164129482623.3986460.10407047347806659820@Monstersaurus>","From":"David Plowman ","Date":"Tue, 4 Jan 2022 13:33:05 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22156,"web_url":"https://patchwork.libcamera.org/comment/22156/","msgid":"<164440962799.3354066.5892501977382531458@Monstersaurus>","date":"2022-02-09T12:27:07","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2022-01-04 13:33:05)\n> Hi again\n> \n> On Tue, 4 Jan 2022 at 11:13, Kieran Bingham\n> wrote:\n> >\n> > Quoting David Plowman (2021-09-22 14:29:15)\n> > > These changes retrieve the correct value for sensitivity of the mode\n> > > selected for the sensor. This value is known to the CamHelper which\n> > > passes it across to the pipeline handler so that it can be set\n> > > correctly in the CameraConfiguration.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---\n> > > 3 files changed, 18 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > > index e453d46c..a92a76f8 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > @@ -38,6 +38,10 @@ struct IPAConfig {\n> > > libcamera.FileDescriptor lsTableHandle;\n> > > };\n> > >\n> > > +struct IPAConfigResult {\n> > > + float modeSensitivity;\n> > > +};\n> > > +\n> > > struct StartConfig {\n> > > libcamera.ControlList controls;\n> > > int32 dropFrameCount;\n> > > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> > > * \\param[in] entityControls Controls provided by the pipeline entities\n> > > * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> > > * \\param[out] controls Controls to apply by the pipeline entity\n> > > + * \\param[out] result Other results that the pipeline handler may require\n> > > *\n> > > * This function shall be called when the camera is configured to inform\n> > > * the IPA of the camera's streams and the sensor settings.\n> > > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> > > map streamConfig,\n> > > map entityControls,\n> > > IPAConfig ipaConfig)\n> > > - => (int32 ret, libcamera.ControlList controls);\n> > > + => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);\n> > >\n> > > /**\n> > > * \\fn mapBuffers()\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 047123ab..796e6d15 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -97,7 +97,7 @@ public:\n> > > const std::map &streamConfig,\n> > > const std::map &entityControls,\n> > > const ipa::RPi::IPAConfig &data,\n> > > - ControlList *controls) override;\n> > > + ControlList *controls, ipa::RPi::IPAConfigResult *result) override;\n> > > void mapBuffers(const std::vector &buffers) override;\n> > > void unmapBuffers(const std::vector &ids) override;\n> > > void signalStatReady(const uint32_t bufferId) override;\n> > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> > > [[maybe_unused]] const std::map &streamConfig,\n> > > const std::map &entityControls,\n> > > const ipa::RPi::IPAConfig &ipaConfig,\n> > > - ControlList *controls)\n> > > + ControlList *controls, ipa::RPi::IPAConfigResult *result)\n> > > {\n> > > if (entityControls.size() != 2) {\n> > > LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> > > /* Pass the camera mode to the CamHelper to setup algorithms. */\n> > > helper_->SetCameraMode(mode_);\n> > >\n> > > + /* The pipeline handler passes out the mode's sensitivity. */\n> > > + result->modeSensitivity = mode_.sensitivity;\n> > > +\n> > > if (firstStart_) {\n> > > /* Supply initial values for frame durations. */\n> > > applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 0bdfa727..caf0030e 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -147,7 +147,7 @@ public:\n> > > void frameStarted(uint32_t sequence);\n> > >\n> > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > > - int configureIPA(const CameraConfiguration *config);\n> > > + int configureIPA(CameraConfiguration *config);\n> > >\n> > > void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> > > void runIsp(uint32_t bufferId);\n> > > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> > > return ipa_->init(settings, sensorConfig);\n> > > }\n> > >\n> > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> > > {\n> > > /* We know config must be an RPiCameraConfiguration. */\n> > > const RPiCameraConfiguration *rpiConfig =\n> > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > >\n> > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > ControlList controls;\n> > > + ipa::RPi::IPAConfigResult result;\n> > > ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> > > - &controls);\n> > > + &controls, &result);\n> >\n> > This should probably be being passed back as a control/property here.\n> > But I think right now the &controls is a set of V4L2 Controls, and not\n> > libcamera controls.\n> >\n> > We've been discussing about changing that so that the IPAs pass\n> > libcamera::controls instead. Currently Isolated IPAs will break when we\n> > try to pass lens controls because that's 'yet another infomap' that will\n> > have to be serialised.\n> >\n> > If we instead use only libcamera controls in the interface between the\n> > pipeline handler and the IPA this would be resolved, and it would mean\n> > that list could also pass the sensitivity property.\n> >\n> > Is there any reason you're aware of as to why we could or couldn't use\n> > libcamera controls here? I know Laurent has been concerned in the past\n> > about perhaps if there was any precision issues or rounding that the IPA\n> > might need to know about that would otherwise occur in the pipeline\n> > handler. Do you see this as an issue? or would we be able to\n> > successfully convert the interfaces to use libcamera control lists only\n> > (and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler\n> > only).\n> \n> I can't think of any precision issues that particularly bother me\n> (which of course doesn't mean there aren't any). As I said in my other\n> reply, I do need this value to be available from the call to\n> configure(), before you get to call start(), so it's not clear to me\n> at the minute where I would read this value back - but I'm guessing\n> someone has some ideas?\n\nThat discussion above was more about the type of the Control rather than\nwhere it gets returned.\n\nI'm confused here if the start() and configure() you are referencing is\nthe public API, or the IPA interface API?\n\n--\nKieran\n\n\n> \n> David\n> \n> >\n> > All that said, changing the controls interface there is still possible\n> > on top, even with this patch, so I don't object to passing it directly\n> > back for now.\n> >\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > --\n> > Kieran\n> >\n> >\n> > > if (ret < 0) {\n> > > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > return -EPIPE;\n> > > }\n> > >\n> > > + /* Store the mode sensitivity for the application. */\n> > > + config->modeSensitivity = result.modeSensitivity;\n> > > +\n> > > /*\n> > > * Configure the H/V flip controls based on the combination of\n> > > * the sensor and user transform.\n> > > --\n> > > 2.20.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 8DBD1BDCBF\n\tfor ;\n\tWed, 9 Feb 2022 12:27:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6870610EC;\n\tWed, 9 Feb 2022 13:27:12 +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 E90186106D\n\tfor ;\n\tWed, 9 Feb 2022 13:27:10 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 82A7493;\n\tWed, 9 Feb 2022 13:27:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BHflmvo+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1644409630;\n\tbh=6WiDHBJKhM2vXw5H/8uUfxIVGp7+RPbhSdXGVMmNgqA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=BHflmvo+wV3JseB3sPZ4MOcwlCyplM+4Xj8dzkxwslVXBZbCGbqH1z5Uwdjrdd1ef\n\tzzbp5QapIZ4jpSpMvxf6gyT8xb6+ceEWSwbVyoH+ddfztJ9sAWiWLwk3m9qPRtLuyO\n\tgSTnM8Qs56CpNqgqIArCkTBXtQG+JSWffk9qyNLc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164129482623.3986460.10407047347806659820@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"David Plowman ","Date":"Wed, 09 Feb 2022 12:27:07 +0000","Message-ID":"<164440962799.3354066.5892501977382531458@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22158,"web_url":"https://patchwork.libcamera.org/comment/22158/","msgid":"","date":"2022-02-09T13:00:57","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran\n\nOn Wed, 9 Feb 2022 at 12:27, Kieran Bingham\n wrote:\n>\n> Quoting David Plowman (2022-01-04 13:33:05)\n> > Hi again\n> >\n> > On Tue, 4 Jan 2022 at 11:13, Kieran Bingham\n> > wrote:\n> > >\n> > > Quoting David Plowman (2021-09-22 14:29:15)\n> > > > These changes retrieve the correct value for sensitivity of the mode\n> > > > selected for the sensor. This value is known to the CamHelper which\n> > > > passes it across to the pipeline handler so that it can be set\n> > > > correctly in the CameraConfiguration.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---\n> > > > 3 files changed, 18 insertions(+), 6 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > > > index e453d46c..a92a76f8 100644\n> > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > > @@ -38,6 +38,10 @@ struct IPAConfig {\n> > > > libcamera.FileDescriptor lsTableHandle;\n> > > > };\n> > > >\n> > > > +struct IPAConfigResult {\n> > > > + float modeSensitivity;\n> > > > +};\n> > > > +\n> > > > struct StartConfig {\n> > > > libcamera.ControlList controls;\n> > > > int32 dropFrameCount;\n> > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> > > > * \\param[in] entityControls Controls provided by the pipeline entities\n> > > > * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> > > > * \\param[out] controls Controls to apply by the pipeline entity\n> > > > + * \\param[out] result Other results that the pipeline handler may require\n> > > > *\n> > > > * This function shall be called when the camera is configured to inform\n> > > > * the IPA of the camera's streams and the sensor settings.\n> > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> > > > map streamConfig,\n> > > > map entityControls,\n> > > > IPAConfig ipaConfig)\n> > > > - => (int32 ret, libcamera.ControlList controls);\n> > > > + => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);\n> > > >\n> > > > /**\n> > > > * \\fn mapBuffers()\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 047123ab..796e6d15 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -97,7 +97,7 @@ public:\n> > > > const std::map &streamConfig,\n> > > > const std::map &entityControls,\n> > > > const ipa::RPi::IPAConfig &data,\n> > > > - ControlList *controls) override;\n> > > > + ControlList *controls, ipa::RPi::IPAConfigResult *result) override;\n> > > > void mapBuffers(const std::vector &buffers) override;\n> > > > void unmapBuffers(const std::vector &ids) override;\n> > > > void signalStatReady(const uint32_t bufferId) override;\n> > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> > > > [[maybe_unused]] const std::map &streamConfig,\n> > > > const std::map &entityControls,\n> > > > const ipa::RPi::IPAConfig &ipaConfig,\n> > > > - ControlList *controls)\n> > > > + ControlList *controls, ipa::RPi::IPAConfigResult *result)\n> > > > {\n> > > > if (entityControls.size() != 2) {\n> > > > LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> > > > /* Pass the camera mode to the CamHelper to setup algorithms. */\n> > > > helper_->SetCameraMode(mode_);\n> > > >\n> > > > + /* The pipeline handler passes out the mode's sensitivity. */\n> > > > + result->modeSensitivity = mode_.sensitivity;\n> > > > +\n> > > > if (firstStart_) {\n> > > > /* Supply initial values for frame durations. */\n> > > > applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 0bdfa727..caf0030e 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -147,7 +147,7 @@ public:\n> > > > void frameStarted(uint32_t sequence);\n> > > >\n> > > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > > > - int configureIPA(const CameraConfiguration *config);\n> > > > + int configureIPA(CameraConfiguration *config);\n> > > >\n> > > > void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> > > > void runIsp(uint32_t bufferId);\n> > > > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> > > > return ipa_->init(settings, sensorConfig);\n> > > > }\n> > > >\n> > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> > > > {\n> > > > /* We know config must be an RPiCameraConfiguration. */\n> > > > const RPiCameraConfiguration *rpiConfig =\n> > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > >\n> > > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > > ControlList controls;\n> > > > + ipa::RPi::IPAConfigResult result;\n> > > > ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> > > > - &controls);\n> > > > + &controls, &result);\n> > >\n> > > This should probably be being passed back as a control/property here.\n> > > But I think right now the &controls is a set of V4L2 Controls, and not\n> > > libcamera controls.\n> > >\n> > > We've been discussing about changing that so that the IPAs pass\n> > > libcamera::controls instead. Currently Isolated IPAs will break when we\n> > > try to pass lens controls because that's 'yet another infomap' that will\n> > > have to be serialised.\n> > >\n> > > If we instead use only libcamera controls in the interface between the\n> > > pipeline handler and the IPA this would be resolved, and it would mean\n> > > that list could also pass the sensitivity property.\n> > >\n> > > Is there any reason you're aware of as to why we could or couldn't use\n> > > libcamera controls here? I know Laurent has been concerned in the past\n> > > about perhaps if there was any precision issues or rounding that the IPA\n> > > might need to know about that would otherwise occur in the pipeline\n> > > handler. Do you see this as an issue? or would we be able to\n> > > successfully convert the interfaces to use libcamera control lists only\n> > > (and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler\n> > > only).\n> >\n> > I can't think of any precision issues that particularly bother me\n> > (which of course doesn't mean there aren't any). As I said in my other\n> > reply, I do need this value to be available from the call to\n> > configure(), before you get to call start(), so it's not clear to me\n> > at the minute where I would read this value back - but I'm guessing\n> > someone has some ideas?\n>\n> That discussion above was more about the type of the Control rather than\n> where it gets returned.\n>\n> I'm confused here if the start() and configure() you are referencing is\n> the public API, or the IPA interface API?\n\nI'm referring to the public Camera API. It's applications that will\nwant to know this number.\n\nDavid\n\n>\n> --\n> Kieran\n>\n>\n> >\n> > David\n> >\n> > >\n> > > All that said, changing the controls interface there is still possible\n> > > on top, even with this patch, so I don't object to passing it directly\n> > > back for now.\n> > >\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > --\n> > > Kieran\n> > >\n> > >\n> > > > if (ret < 0) {\n> > > > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > > return -EPIPE;\n> > > > }\n> > > >\n> > > > + /* Store the mode sensitivity for the application. */\n> > > > + config->modeSensitivity = result.modeSensitivity;\n> > > > +\n> > > > /*\n> > > > * Configure the H/V flip controls based on the combination of\n> > > > * the sensor and user transform.\n> > > > --\n> > > > 2.20.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 091DCBDCBF\n\tfor ;\n\tWed, 9 Feb 2022 13:01:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC77A61092;\n\tWed, 9 Feb 2022 14:01:10 +0100 (CET)","from mail-wm1-x32c.google.com (mail-wm1-x32c.google.com\n\t[IPv6:2a00:1450:4864:20::32c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 159136106D\n\tfor ;\n\tWed, 9 Feb 2022 14:01:09 +0100 (CET)","by mail-wm1-x32c.google.com with SMTP id\n\tq198-20020a1ca7cf000000b0037bb52545c6so3145477wme.1\n\tfor ;\n\tWed, 09 Feb 2022 05:01:09 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"qI2k2szx\"; 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=NXSAwuRBQDW1sr8Ix700aNfrWe8iuzIqRxH9Th19+4M=;\n\tb=qI2k2szxbhWa/8aJhxEaoy10i43bF1BroasTAP4+qNxWi39hH4c1QwNxVXHp4oRChx\n\t5s9QAwgu8C0WD40ihCk1ED94aUDaGjXPdThpLeJCZIZnznmSDStEgxEwh0sm7HC3oCpy\n\tvcx5goJvQRs6SqN6A4nGIyZ5pJ7d4WJP5XHmHJefikQwh13+BHz5x694bNr63IyJNnBx\n\tw7N/nmLu52J2p7STrnfPgrOUIBtsgxyn9//UAtAiIMemkeYS8cUCqgG8Pw2BVWJ1JoKY\n\tt8PZd62/9ha2nlrBylBMJCE0QbPi0+UzDQktiGY/mcu1A8KMEVjBBkpW1SLC843j1rZE\n\ttA+g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=NXSAwuRBQDW1sr8Ix700aNfrWe8iuzIqRxH9Th19+4M=;\n\tb=6Yyx5XEV6nBy5o8LoOkb7SFhjJXN6qgDBDuHcLGV9QgOVEgYik50/wS+F+QZ8pVdaH\n\tE7vE+XhDxN8RNDyduSAL+u+ZtSjhWqDAiwBp95eGS2AEzB19mX+CBDE0Nbsrd+gIwqes\n\tbPDiW7oscjlogJB6MiVVVgyB9sKVbnTqFkOge3cGsQ5zsjVhhgPbXWi2mZSivy+YMsAt\n\tKkXS4pDZIAtg1wdzObizI3ZAheFwD4jZ5qt2QgczgybFijTR+hiT5Q2dcgrdxdQoNQqo\n\tBKLvoDGJUPDjTm220Ek9Oo4ekN2zxuu5Kw3PFSRpJubsEqtAngccrg/wIUT1LjKNJYz5\n\ti83w==","X-Gm-Message-State":"AOAM533K0sZKM/N0c7ZgSsfjamGqQPtzWj5Jc7sqyBrUmHmhlxvdvc+s\n\tEEFT0Iz+u94VvILyeOQm4m5JZ2cIG5gl8V2ALHu1QEXXw2e2Aw==","X-Google-Smtp-Source":"ABdhPJz4oU0oTEc9qxtMFZe34R+HlZWIL2jDAdcL+B8VORHAsx8hf/uXJLtIoavhH+ohamgxqpFiXjT2GhL91OxodZE=","X-Received":"by 2002:a05:600c:1ca0:: with SMTP id\n\tk32mr1977761wms.171.1644411668655; \n\tWed, 09 Feb 2022 05:01:08 -0800 (PST)","MIME-Version":"1.0","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164129482623.3986460.10407047347806659820@Monstersaurus>\n\t\n\t<164440962799.3354066.5892501977382531458@Monstersaurus>","In-Reply-To":"<164440962799.3354066.5892501977382531458@Monstersaurus>","From":"David Plowman ","Date":"Wed, 9 Feb 2022 13:00:57 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22159,"web_url":"https://patchwork.libcamera.org/comment/22159/","msgid":"<164441266230.3354066.6384409217676509999@Monstersaurus>","date":"2022-02-09T13:17:42","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2022-02-09 13:00:57)\n> Hi Kieran\n> \n> On Wed, 9 Feb 2022 at 12:27, Kieran Bingham\n> wrote:\n> >\n> > Quoting David Plowman (2022-01-04 13:33:05)\n> > > Hi again\n> > >\n> > > On Tue, 4 Jan 2022 at 11:13, Kieran Bingham\n> > > wrote:\n> > > >\n> > > > Quoting David Plowman (2021-09-22 14:29:15)\n> > > > > These changes retrieve the correct value for sensitivity of the mode\n> > > > > selected for the sensor. This value is known to the CamHelper which\n> > > > > passes it across to the pipeline handler so that it can be set\n> > > > > correctly in the CameraConfiguration.\n> > > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---\n> > > > > 3 files changed, 18 insertions(+), 6 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > > > > index e453d46c..a92a76f8 100644\n> > > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > > > @@ -38,6 +38,10 @@ struct IPAConfig {\n> > > > > libcamera.FileDescriptor lsTableHandle;\n> > > > > };\n> > > > >\n> > > > > +struct IPAConfigResult {\n> > > > > + float modeSensitivity;\n> > > > > +};\n> > > > > +\n> > > > > struct StartConfig {\n> > > > > libcamera.ControlList controls;\n> > > > > int32 dropFrameCount;\n> > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> > > > > * \\param[in] entityControls Controls provided by the pipeline entities\n> > > > > * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> > > > > * \\param[out] controls Controls to apply by the pipeline entity\n> > > > > + * \\param[out] result Other results that the pipeline handler may require\n> > > > > *\n> > > > > * This function shall be called when the camera is configured to inform\n> > > > > * the IPA of the camera's streams and the sensor settings.\n> > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> > > > > map streamConfig,\n> > > > > map entityControls,\n> > > > > IPAConfig ipaConfig)\n> > > > > - => (int32 ret, libcamera.ControlList controls);\n> > > > > + => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);\n> > > > >\n> > > > > /**\n> > > > > * \\fn mapBuffers()\n> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > index 047123ab..796e6d15 100644\n> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > @@ -97,7 +97,7 @@ public:\n> > > > > const std::map &streamConfig,\n> > > > > const std::map &entityControls,\n> > > > > const ipa::RPi::IPAConfig &data,\n> > > > > - ControlList *controls) override;\n> > > > > + ControlList *controls, ipa::RPi::IPAConfigResult *result) override;\n> > > > > void mapBuffers(const std::vector &buffers) override;\n> > > > > void unmapBuffers(const std::vector &ids) override;\n> > > > > void signalStatReady(const uint32_t bufferId) override;\n> > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> > > > > [[maybe_unused]] const std::map &streamConfig,\n> > > > > const std::map &entityControls,\n> > > > > const ipa::RPi::IPAConfig &ipaConfig,\n> > > > > - ControlList *controls)\n> > > > > + ControlList *controls, ipa::RPi::IPAConfigResult *result)\n> > > > > {\n> > > > > if (entityControls.size() != 2) {\n> > > > > LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> > > > > /* Pass the camera mode to the CamHelper to setup algorithms. */\n> > > > > helper_->SetCameraMode(mode_);\n> > > > >\n> > > > > + /* The pipeline handler passes out the mode's sensitivity. */\n> > > > > + result->modeSensitivity = mode_.sensitivity;\n> > > > > +\n> > > > > if (firstStart_) {\n> > > > > /* Supply initial values for frame durations. */\n> > > > > applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index 0bdfa727..caf0030e 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -147,7 +147,7 @@ public:\n> > > > > void frameStarted(uint32_t sequence);\n> > > > >\n> > > > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > > > > - int configureIPA(const CameraConfiguration *config);\n> > > > > + int configureIPA(CameraConfiguration *config);\n> > > > >\n> > > > > void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> > > > > void runIsp(uint32_t bufferId);\n> > > > > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> > > > > return ipa_->init(settings, sensorConfig);\n> > > > > }\n> > > > >\n> > > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> > > > > {\n> > > > > /* We know config must be an RPiCameraConfiguration. */\n> > > > > const RPiCameraConfiguration *rpiConfig =\n> > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > > >\n> > > > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > > > ControlList controls;\n> > > > > + ipa::RPi::IPAConfigResult result;\n> > > > > ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> > > > > - &controls);\n> > > > > + &controls, &result);\n> > > >\n> > > > This should probably be being passed back as a control/property here.\n> > > > But I think right now the &controls is a set of V4L2 Controls, and not\n> > > > libcamera controls.\n> > > >\n> > > > We've been discussing about changing that so that the IPAs pass\n> > > > libcamera::controls instead. Currently Isolated IPAs will break when we\n> > > > try to pass lens controls because that's 'yet another infomap' that will\n> > > > have to be serialised.\n> > > >\n> > > > If we instead use only libcamera controls in the interface between the\n> > > > pipeline handler and the IPA this would be resolved, and it would mean\n> > > > that list could also pass the sensitivity property.\n> > > >\n> > > > Is there any reason you're aware of as to why we could or couldn't use\n> > > > libcamera controls here? I know Laurent has been concerned in the past\n> > > > about perhaps if there was any precision issues or rounding that the IPA\n> > > > might need to know about that would otherwise occur in the pipeline\n> > > > handler. Do you see this as an issue? or would we be able to\n> > > > successfully convert the interfaces to use libcamera control lists only\n> > > > (and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler\n> > > > only).\n> > >\n> > > I can't think of any precision issues that particularly bother me\n> > > (which of course doesn't mean there aren't any). As I said in my other\n> > > reply, I do need this value to be available from the call to\n> > > configure(), before you get to call start(), so it's not clear to me\n> > > at the minute where I would read this value back - but I'm guessing\n> > > someone has some ideas?\n> >\n> > That discussion above was more about the type of the Control rather than\n> > where it gets returned.\n> >\n> > I'm confused here if the start() and configure() you are referencing is\n> > the public API, or the IPA interface API?\n> \n> I'm referring to the public Camera API. It's applications that will\n> want to know this number.\n\nOk, so it can certainly be updated by the configure operation, but I\nwonder if it's something that the model would expect to be determined\nduring validate()?\n\nConfigure() is supposed to be a 'non-changing' operation ... but this is\nmore like a return value. Would an application expect to know the mode\nsensitivity after they've validated a configuration? (to check it is\nwhat they expect?) before even calling configure?\n\n--\nKieran\n\n> \n> David\n> \n> >\n> > --\n> > Kieran\n> >\n> >\n> > >\n> > > David\n> > >\n> > > >\n> > > > All that said, changing the controls interface there is still possible\n> > > > on top, even with this patch, so I don't object to passing it directly\n> > > > back for now.\n> > > >\n> > > >\n> > > > Reviewed-by: Kieran Bingham \n> > > >\n> > > > --\n> > > > Kieran\n> > > >\n> > > >\n> > > > > if (ret < 0) {\n> > > > > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > > > return -EPIPE;\n> > > > > }\n> > > > >\n> > > > > + /* Store the mode sensitivity for the application. */\n> > > > > + config->modeSensitivity = result.modeSensitivity;\n> > > > > +\n> > > > > /*\n> > > > > * Configure the H/V flip controls based on the combination of\n> > > > > * the sensor and user transform.\n> > > > > --\n> > > > > 2.20.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 20926BF415\n\tfor ;\n\tWed, 9 Feb 2022 13:17:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 819216106D;\n\tWed, 9 Feb 2022 14:17:45 +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 A515A6106D\n\tfor ;\n\tWed, 9 Feb 2022 14:17:44 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 51C3493;\n\tWed, 9 Feb 2022 14:17:44 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OiTxDlbJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1644412664;\n\tbh=QTIZCtPEp02M4BtKEoQg8fEBog62bvRndrAbNHoF2dQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=OiTxDlbJht6FrcxR09hCo81pTCZM00wF9Z90JBd73q7iJG+gPHW763rsajpalezjN\n\td+EG2RqzRVNlL2ye+cqw3WlaayN5/5V3Czmznmt1hkMqHKqLkE7crtCI5T6qmk7rew\n\tIkcxwrqmPbzNox/skB9iKvqrU+vtOUv3sI4J1TJU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164129482623.3986460.10407047347806659820@Monstersaurus>\n\t\n\t<164440962799.3354066.5892501977382531458@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"David Plowman ","Date":"Wed, 09 Feb 2022 13:17:42 +0000","Message-ID":"<164441266230.3354066.6384409217676509999@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22692,"web_url":"https://patchwork.libcamera.org/comment/22692/","msgid":"","date":"2022-04-13T09:13:28","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your patch.\n\nOn Wed, 22 Sept 2021 at 14:29, David Plowman \nwrote:\n\n> These changes retrieve the correct value for sensitivity of the mode\n> selected for the sensor. This value is known to the CamHelper which\n> passes it across to the pipeline handler so that it can be set\n> correctly in the CameraConfiguration.\n>\n> Signed-off-by: David Plowman \n>\n\nI agree with Kieran that we might want to consider passing libcamera\ncontrols from the IPA\ninstead of v4l2 controls. The mode sensitivity can then be put into a\ncontrol list. But that is\nnot for this series.\n\nReviewed-by: Naushir Patuck \n\n\n> ---\n> include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---\n> 3 files changed, 18 insertions(+), 6 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> index e453d46c..a92a76f8 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -38,6 +38,10 @@ struct IPAConfig {\n> libcamera.FileDescriptor lsTableHandle;\n> };\n>\n> +struct IPAConfigResult {\n> + float modeSensitivity;\n> +};\n> +\n> struct StartConfig {\n> libcamera.ControlList controls;\n> int32 dropFrameCount;\n> @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> * \\param[in] entityControls Controls provided by the pipeline\n> entities\n> * \\param[in] ipaConfig Pipeline-handler-specific configuration\n> data\n> * \\param[out] controls Controls to apply by the pipeline entity\n> + * \\param[out] result Other results that the pipeline handler may\n> require\n> *\n> * This function shall be called when the camera is configured to\n> inform\n> * the IPA of the camera's streams and the sensor settings.\n> @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> map streamConfig,\n> map entityControls,\n> IPAConfig ipaConfig)\n> - => (int32 ret, libcamera.ControlList controls);\n> + => (int32 ret, libcamera.ControlList controls,\n> IPAConfigResult result);\n>\n> /**\n> * \\fn mapBuffers()\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 047123ab..796e6d15 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -97,7 +97,7 @@ public:\n> const std::map\n> &streamConfig,\n> const std::map\n> &entityControls,\n> const ipa::RPi::IPAConfig &data,\n> - ControlList *controls) override;\n> + ControlList *controls, ipa::RPi::IPAConfigResult\n> *result) override;\n> void mapBuffers(const std::vector &buffers) override;\n> void unmapBuffers(const std::vector &ids) override;\n> void signalStatReady(const uint32_t bufferId) override;\n> @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> const std::map\n> &entityControls,\n> const ipa::RPi::IPAConfig &ipaConfig,\n> - ControlList *controls)\n> + ControlList *controls, ipa::RPi::IPAConfigResult\n> *result)\n> {\n> if (entityControls.size() != 2) {\n> LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> /* Pass the camera mode to the CamHelper to setup algorithms. */\n> helper_->SetCameraMode(mode_);\n>\n> + /* The pipeline handler passes out the mode's sensitivity. */\n> + result->modeSensitivity = mode_.sensitivity;\n> +\n> if (firstStart_) {\n> /* Supply initial values for frame durations. */\n> applyFrameDurations(defaultMinFrameDuration,\n> defaultMaxFrameDuration);\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 0bdfa727..caf0030e 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -147,7 +147,7 @@ public:\n> void frameStarted(uint32_t sequence);\n>\n> int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> - int configureIPA(const CameraConfiguration *config);\n> + int configureIPA(CameraConfiguration *config);\n>\n> void statsMetadataComplete(uint32_t bufferId, const ControlList\n> &controls);\n> void runIsp(uint32_t bufferId);\n> @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig\n> *sensorConfig)\n> return ipa_->init(settings, sensorConfig);\n> }\n>\n> -int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> {\n> /* We know config must be an RPiCameraConfiguration. */\n> const RPiCameraConfiguration *rpiConfig =\n> @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n>\n> /* Ready the IPA - it must know about the sensor resolution. */\n> ControlList controls;\n> + ipa::RPi::IPAConfigResult result;\n> ret = ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> ipaConfig,\n> - &controls);\n> + &controls, &result);\n> if (ret < 0) {\n> LOG(RPI, Error) << \"IPA configuration failed!\";\n> return -EPIPE;\n> }\n>\n> + /* Store the mode sensitivity for the application. */\n> + config->modeSensitivity = result.modeSensitivity;\n> +\n> /*\n> * Configure the H/V flip controls based on the combination of\n> * the sensor and user transform.\n> --\n> 2.20.1\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 C46D8C3256\n\tfor ;\n\tWed, 13 Apr 2022 09:13:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1B2B365645;\n\tWed, 13 Apr 2022 11:13:46 +0200 (CEST)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ADD866563E\n\tfor ;\n\tWed, 13 Apr 2022 11:13:44 +0200 (CEST)","by mail-lj1-x230.google.com with SMTP id 17so1422363lji.1\n\tfor ;\n\tWed, 13 Apr 2022 02:13:44 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649841226;\n\tbh=vx45+ds/R/V3xkSW79b54cloBQGdoxMQJZGFsJmG5k8=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=PIw0cyFd7JFOwohPNPv7o1xdqM+Qff65Ck5MD8FyKQjSBt4kXKw8MB8BdqZCgMv/o\n\tQDDascmY3Rvh4nKInyYjFt2c3ZPTjk1ksUIwwuIsjuZgd70ku0SApZqoVwR7KuDPjg\n\tSVdb/1A/yi7I/Zb2bEWEqb9r5eSrKMduaH8An4E8C2kt3m+FX6B+K56sv17Utuy0ui\n\tXuOQq2R/viLaCL7JG53jexm4My125XlU1gh8YsILMBW9MMVjCgOKaCmzvdvwgpOrOu\n\t+A+nf9USsTbqEFLa4dD418dCR5TJuUA/Uy6gQC2yxiV6lEMQgUoDgiURqckcG8NIXD\n\tJ6fTeRNFjt6DA==","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=VyMi+cLcAnN84tHbAsZe2y9YiyNPwBBzhAxGVe/H8Sw=;\n\tb=AHTDKkNmWZQ7p/0tgaUG8heQZ+PumrEAVGYy3B2YQBP8p4oEeXyhLBAK0FTT24LWdT\n\t0nNGuAx0oI7vJebLM2q9sJ3yZ35+DAsM8NDNcQaFKs0QuibhwBk1FFUwiY6Dez0qRuCZ\n\tENZKomIHKu2h3+ig+5U47aHI1dYnL6N1XGGp94yGlzxecY4j11R8PLSmfrnuawKfRQl5\n\tjB+fQy3M2FAjwM7kE3s63omW7pUraq/LF+fHTEqPznDLRhJ8KnOWeDnqZON2JmSsmgIX\n\tO32K2rKMirpETqJKcW0IaTvOuLdH4ON+vjBmtOyuEZPm8iiWzsGtpD5USDwDNzV5X4R6\n\ttDMA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"AHTDKkNm\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=VyMi+cLcAnN84tHbAsZe2y9YiyNPwBBzhAxGVe/H8Sw=;\n\tb=neumfkpg3OIe2PABG9rRXGumDEf7D51NSuit2ScMUIYSlse5wWU0XHIINQeWAK1F5/\n\tR/9tKOI9o4uSWtr3sUUsWGkqfWUKlmgwXavHrP+IZeJPyH/e93oXfVbsMDs4u14SD5wA\n\tWhT6DbTYTYC34bT9Dr6zu4Ga/3nx9oJe61YvuymDIOfRJ4hPmzhaIgBvCm4mUInvkGYW\n\tx6eUiL7h0C+Ag1SS9G6jnxxKcNKasSTk/Z0T+ohdlDspl/P14K1XNBe0ByI9IaL9cpFJ\n\tPkIDy8UshTqApIpa1uKER9syv8MebtqKiGRwcRyi6/92JAaSbZRYyhDGE8OoM4kcSB99\n\tfVIA==","X-Gm-Message-State":"AOAM5308iR+OiOe9MN1aFUVUCd0RR6XHqW2dc+Gzh1m46BBxc83JOyi9\n\t/Pqo947KyjQ5l/q+pudz4IJmxq3WMYsq1egg3s7iWlTPaP0=","X-Google-Smtp-Source":"ABdhPJxqlLDFUHHDziaJqbht/x3IkiZY4LnPEiQSJAN4tUGHgG5m4uCPRLQf/i8DtbITxZ9mCHYx1Dwu3k+wX7FKnG4=","X-Received":"by 2002:a05:651c:205:b0:24c:89f6:75aa with SMTP id\n\ty5-20020a05651c020500b0024c89f675aamr268804ljn.46.1649841223856;\n\tWed, 13 Apr 2022 02:13:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>","In-Reply-To":"<20210922132915.14881-3-david.plowman@raspberrypi.com>","Date":"Wed, 13 Apr 2022 10:13:28 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"0000000000001592a605dc8598e8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22694,"web_url":"https://patchwork.libcamera.org/comment/22694/","msgid":"<164984269099.22830.1157254031970565968@Monstersaurus>","date":"2022-04-13T09:38:10","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David, Naush,\n\nQuoting David Plowman (2021-09-22 14:29:15)\n> These changes retrieve the correct value for sensitivity of the mode\n> selected for the sensor. This value is known to the CamHelper which\n> passes it across to the pipeline handler so that it can be set\n> correctly in the CameraConfiguration.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---\n> 3 files changed, 18 insertions(+), 6 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index e453d46c..a92a76f8 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -38,6 +38,10 @@ struct IPAConfig {\n> libcamera.FileDescriptor lsTableHandle;\n> };\n> \n> +struct IPAConfigResult {\n> + float modeSensitivity;\n> +};\n> +\n> struct StartConfig {\n> libcamera.ControlList controls;\n> int32 dropFrameCount;\n> @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> * \\param[in] entityControls Controls provided by the pipeline entities\n> * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> * \\param[out] controls Controls to apply by the pipeline entity\n> + * \\param[out] result Other results that the pipeline handler may require\n> *\n> * This function shall be called when the camera is configured to inform\n> * the IPA of the camera's streams and the sensor settings.\n> @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> map streamConfig,\n> map entityControls,\n> IPAConfig ipaConfig)\n> - => (int32 ret, libcamera.ControlList controls);\n> + => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);\n> \n> /**\n> * \\fn mapBuffers()\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 047123ab..796e6d15 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -97,7 +97,7 @@ public:\n> const std::map &streamConfig,\n> const std::map &entityControls,\n> const ipa::RPi::IPAConfig &data,\n> - ControlList *controls) override;\n> + ControlList *controls, ipa::RPi::IPAConfigResult *result) override;\n> void mapBuffers(const std::vector &buffers) override;\n> void unmapBuffers(const std::vector &ids) override;\n> void signalStatReady(const uint32_t bufferId) override;\n> @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> [[maybe_unused]] const std::map &streamConfig,\n> const std::map &entityControls,\n> const ipa::RPi::IPAConfig &ipaConfig,\n> - ControlList *controls)\n> + ControlList *controls, ipa::RPi::IPAConfigResult *result)\n> {\n> if (entityControls.size() != 2) {\n> LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> /* Pass the camera mode to the CamHelper to setup algorithms. */\n> helper_->SetCameraMode(mode_);\n> \n> + /* The pipeline handler passes out the mode's sensitivity. */\n> + result->modeSensitivity = mode_.sensitivity;\n> +\n> if (firstStart_) {\n> /* Supply initial values for frame durations. */\n> applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 0bdfa727..caf0030e 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -147,7 +147,7 @@ public:\n> void frameStarted(uint32_t sequence);\n> \n> int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> - int configureIPA(const CameraConfiguration *config);\n> + int configureIPA(CameraConfiguration *config);\n> \n> void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> void runIsp(uint32_t bufferId);\n> @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> return ipa_->init(settings, sensorConfig);\n> }\n> \n> -int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> {\n> /* We know config must be an RPiCameraConfiguration. */\n> const RPiCameraConfiguration *rpiConfig =\n> @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \n> /* Ready the IPA - it must know about the sensor resolution. */\n> ControlList controls;\n> + ipa::RPi::IPAConfigResult result;\n> ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> - &controls);\n> + &controls, &result);\n\nI've just rebased this series to master to facilitate merging, and with\nfresh eyes I can't help but wonder if this value shouldn't be returned\nin the validate() phase. (Not sure if this has been asked before / yet).\n\nIs there anything that prevents us adding a validate() to the IPA\ninterface to allow validating the configuration and at that point,\nsetting the mode sensitivity? Or can this value /only/ be determined\nwhen configuring?\n\nThen the validate phase should be able to return the mode sensitivity of\nthe configuration that is being validated.\n\nDoing that we could keep the configure calls using a const parameter for\ntheir config structures.\n\n> if (ret < 0) {\n> LOG(RPI, Error) << \"IPA configuration failed!\";\n> return -EPIPE;\n> }\n> \n> + /* Store the mode sensitivity for the application. */\n> + config->modeSensitivity = result.modeSensitivity;\n> +\n> /*\n> * Configure the H/V flip controls based on the combination of\n> * the sensor and user transform.\n> -- \n> 2.20.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 10F92C0F1B\n\tfor ;\n\tWed, 13 Apr 2022 09:38:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75F956563E;\n\tWed, 13 Apr 2022 11:38:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C26C7604B3\n\tfor ;\n\tWed, 13 Apr 2022 11:38:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 63CDE25B;\n\tWed, 13 Apr 2022 11:38:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649842695;\n\tbh=C+wtzTqeAGVi6YmQJsVRJ1k98D9lfvO/6fPtVuPRv6g=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=PX5H42nbSIEjthIGydA+KvCyv4ZyeGrko1o98edTGwAW+HCybdRYDt7pCDgZpL6BC\n\t2YW1zfPwjbIoj0c/zcVRLJ1gldw+UzAzax4EGBI4V2cNU0PoJnJhjnDQChpiT/wNh5\n\t/T3YcxDL7GFCQi26TS8llGpD4ZoFeKtayL/FKBi4OKpTqQRxfPnPo0wy/1oeb4Y0Ks\n\tn9OSkuIkrgFDpOATHsuqd0p23GeRnOi+aAQ7W6OPcApgSsGmmbbHRY6CPHF4zJpV+J\n\tCeiLs3OfbW6ntQpzttiBPfpZM+kP+zJATqgnngi4DUfaLSciupuqk5UMh4enjfWRiy\n\tIXaytQ3MvmR/A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649842693;\n\tbh=C+wtzTqeAGVi6YmQJsVRJ1k98D9lfvO/6fPtVuPRv6g=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=jHoSXw8uKmGl5sFHYQmAPmlS68fj7h1qcqt/030LKumHkUG+Pzuimhu8GxgCb2w6Y\n\trjitsf4QKN/wv8ej6VybyGKkEEAZ36g1HZ8ouLu8icSe76pUL0F+aoN1bZB6Q4GqF8\n\tY47oFORsc7Cc17Rm3iwKoquNEZcyBjh/97klKFls="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jHoSXw8u\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20210922132915.14881-3-david.plowman@raspberrypi.com>","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org,\n\tNaushir Patuck , ","Date":"Wed, 13 Apr 2022 10:38:10 +0100","Message-ID":"<164984269099.22830.1157254031970565968@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22695,"web_url":"https://patchwork.libcamera.org/comment/22695/","msgid":"<164984278427.22830.832783742823710611@Monstersaurus>","date":"2022-04-13T09:39:44","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2022-02-09 13:17:42)\n> Quoting David Plowman (2022-02-09 13:00:57)\n> > Hi Kieran\n> > \n> > On Wed, 9 Feb 2022 at 12:27, Kieran Bingham\n> > wrote:\n> > >\n> > > Quoting David Plowman (2022-01-04 13:33:05)\n> > > > Hi again\n> > > >\n> > > > On Tue, 4 Jan 2022 at 11:13, Kieran Bingham\n> > > > wrote:\n> > > > >\n> > > > > Quoting David Plowman (2021-09-22 14:29:15)\n> > > > > > These changes retrieve the correct value for sensitivity of the mode\n> > > > > > selected for the sensor. This value is known to the CamHelper which\n> > > > > > passes it across to the pipeline handler so that it can be set\n> > > > > > correctly in the CameraConfiguration.\n> > > > > >\n> > > > > > Signed-off-by: David Plowman \n> > > > > > ---\n> > > > > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---\n> > > > > > 3 files changed, 18 insertions(+), 6 deletions(-)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > > > > > index e453d46c..a92a76f8 100644\n> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {\n> > > > > > libcamera.FileDescriptor lsTableHandle;\n> > > > > > };\n> > > > > >\n> > > > > > +struct IPAConfigResult {\n> > > > > > + float modeSensitivity;\n> > > > > > +};\n> > > > > > +\n> > > > > > struct StartConfig {\n> > > > > > libcamera.ControlList controls;\n> > > > > > int32 dropFrameCount;\n> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> > > > > > * \\param[in] entityControls Controls provided by the pipeline entities\n> > > > > > * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> > > > > > * \\param[out] controls Controls to apply by the pipeline entity\n> > > > > > + * \\param[out] result Other results that the pipeline handler may require\n> > > > > > *\n> > > > > > * This function shall be called when the camera is configured to inform\n> > > > > > * the IPA of the camera's streams and the sensor settings.\n> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> > > > > > map streamConfig,\n> > > > > > map entityControls,\n> > > > > > IPAConfig ipaConfig)\n> > > > > > - => (int32 ret, libcamera.ControlList controls);\n> > > > > > + => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);\n> > > > > >\n> > > > > > /**\n> > > > > > * \\fn mapBuffers()\n> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > > index 047123ab..796e6d15 100644\n> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > > @@ -97,7 +97,7 @@ public:\n> > > > > > const std::map &streamConfig,\n> > > > > > const std::map &entityControls,\n> > > > > > const ipa::RPi::IPAConfig &data,\n> > > > > > - ControlList *controls) override;\n> > > > > > + ControlList *controls, ipa::RPi::IPAConfigResult *result) override;\n> > > > > > void mapBuffers(const std::vector &buffers) override;\n> > > > > > void unmapBuffers(const std::vector &ids) override;\n> > > > > > void signalStatReady(const uint32_t bufferId) override;\n> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> > > > > > [[maybe_unused]] const std::map &streamConfig,\n> > > > > > const std::map &entityControls,\n> > > > > > const ipa::RPi::IPAConfig &ipaConfig,\n> > > > > > - ControlList *controls)\n> > > > > > + ControlList *controls, ipa::RPi::IPAConfigResult *result)\n> > > > > > {\n> > > > > > if (entityControls.size() != 2) {\n> > > > > > LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> > > > > > /* Pass the camera mode to the CamHelper to setup algorithms. */\n> > > > > > helper_->SetCameraMode(mode_);\n> > > > > >\n> > > > > > + /* The pipeline handler passes out the mode's sensitivity. */\n> > > > > > + result->modeSensitivity = mode_.sensitivity;\n> > > > > > +\n> > > > > > if (firstStart_) {\n> > > > > > /* Supply initial values for frame durations. */\n> > > > > > applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);\n> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > index 0bdfa727..caf0030e 100644\n> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > @@ -147,7 +147,7 @@ public:\n> > > > > > void frameStarted(uint32_t sequence);\n> > > > > >\n> > > > > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > > > > > - int configureIPA(const CameraConfiguration *config);\n> > > > > > + int configureIPA(CameraConfiguration *config);\n> > > > > >\n> > > > > > void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);\n> > > > > > void runIsp(uint32_t bufferId);\n> > > > > > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> > > > > > return ipa_->init(settings, sensorConfig);\n> > > > > > }\n> > > > > >\n> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> > > > > > {\n> > > > > > /* We know config must be an RPiCameraConfiguration. */\n> > > > > > const RPiCameraConfiguration *rpiConfig =\n> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > > > >\n> > > > > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > > > > ControlList controls;\n> > > > > > + ipa::RPi::IPAConfigResult result;\n> > > > > > ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> > > > > > - &controls);\n> > > > > > + &controls, &result);\n> > > > >\n> > > > > This should probably be being passed back as a control/property here.\n> > > > > But I think right now the &controls is a set of V4L2 Controls, and not\n> > > > > libcamera controls.\n> > > > >\n> > > > > We've been discussing about changing that so that the IPAs pass\n> > > > > libcamera::controls instead. Currently Isolated IPAs will break when we\n> > > > > try to pass lens controls because that's 'yet another infomap' that will\n> > > > > have to be serialised.\n> > > > >\n> > > > > If we instead use only libcamera controls in the interface between the\n> > > > > pipeline handler and the IPA this would be resolved, and it would mean\n> > > > > that list could also pass the sensitivity property.\n> > > > >\n> > > > > Is there any reason you're aware of as to why we could or couldn't use\n> > > > > libcamera controls here? I know Laurent has been concerned in the past\n> > > > > about perhaps if there was any precision issues or rounding that the IPA\n> > > > > might need to know about that would otherwise occur in the pipeline\n> > > > > handler. Do you see this as an issue? or would we be able to\n> > > > > successfully convert the interfaces to use libcamera control lists only\n> > > > > (and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler\n> > > > > only).\n> > > >\n> > > > I can't think of any precision issues that particularly bother me\n> > > > (which of course doesn't mean there aren't any). As I said in my other\n> > > > reply, I do need this value to be available from the call to\n> > > > configure(), before you get to call start(), so it's not clear to me\n> > > > at the minute where I would read this value back - but I'm guessing\n> > > > someone has some ideas?\n> > >\n> > > That discussion above was more about the type of the Control rather than\n> > > where it gets returned.\n> > >\n> > > I'm confused here if the start() and configure() you are referencing is\n> > > the public API, or the IPA interface API?\n> > \n> > I'm referring to the public Camera API. It's applications that will\n> > want to know this number.\n> \n> Ok, so it can certainly be updated by the configure operation, but I\n> wonder if it's something that the model would expect to be determined\n> during validate()?\n> \n> Configure() is supposed to be a 'non-changing' operation ... but this is\n> more like a return value. Would an application expect to know the mode\n> sensitivity after they've validated a configuration? (to check it is\n> what they expect?) before even calling configure?\n\nAha - this is the question I'd like answered before merging ;-)\n\n--\nKieran\n\n\n> \n> --\n> Kieran\n> \n> > \n> > David\n> > \n> > >\n> > > --\n> > > Kieran\n> > >\n> > >\n> > > >\n> > > > David\n> > > >\n> > > > >\n> > > > > All that said, changing the controls interface there is still possible\n> > > > > on top, even with this patch, so I don't object to passing it directly\n> > > > > back for now.\n> > > > >\n> > > > >\n> > > > > Reviewed-by: Kieran Bingham \n> > > > >\n> > > > > --\n> > > > > Kieran\n> > > > >\n> > > > >\n> > > > > > if (ret < 0) {\n> > > > > > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > > > > return -EPIPE;\n> > > > > > }\n> > > > > >\n> > > > > > + /* Store the mode sensitivity for the application. */\n> > > > > > + config->modeSensitivity = result.modeSensitivity;\n> > > > > > +\n> > > > > > /*\n> > > > > > * Configure the H/V flip controls based on the combination of\n> > > > > > * the sensor and user transform.\n> > > > > > --\n> > > > > > 2.20.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 514EFC0F1B\n\tfor ;\n\tWed, 13 Apr 2022 09:39:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0481665644;\n\tWed, 13 Apr 2022 11:39:48 +0200 (CEST)","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 1C83B604B3\n\tfor ;\n\tWed, 13 Apr 2022 11:39:47 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CB49225B;\n\tWed, 13 Apr 2022 11:39:46 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649842788;\n\tbh=3KazAFStSBPqvfSdJeRLy94GPn7uvKlGcym89veESOA=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=fWVp4USg3dfBIORKGPwOTfNX/4oBvDKdm8Q1uaL9I01yPobw/9fS4ZV58ybqePwF4\n\th4JOupQKUOhxEqyxogCB30LlsnxU8yQmzYLDKjhy33BhMJEAJRzyCak4By6MmEq0Gc\n\ta23ug7Jo6Wz4cFe063yZK1h3Gn9P4iGpMUZRawwGCQ58PR11Uw35PeXZ2tDTMLVqBl\n\tiMFXE5XvIbCQo/ZMkjhHDLQhfuTCB8ozstkr94kP4SB93qpWzj1FKQ9pYZFm2fkM6C\n\tnldTrzxxvdcT3F/1ZW5tKBCaLae0jfDl/8xryTpBKKsykXMT3kvaNcKI+nh3l+zonm\n\t2k/2Y1/pkzKSw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649842786;\n\tbh=3KazAFStSBPqvfSdJeRLy94GPn7uvKlGcym89veESOA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=YfAv1T/8hcIihb9TjH6fnSFUhhgX9s6yu6dJEBQGW3qlBGuua0aVu5o6ecRhZkrtm\n\tNc8MbuoxQltkDiZaNeATIY6ocz77UHowyNHXT1j8m4GO6cYoG/eIU9UbLJpFYI/+p4\n\tH0DNmVdJmfc9H8yqA4VpNVdtW2oh4WDjCx1XnRik="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YfAv1T/8\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<164441266230.3354066.6384409217676509999@Monstersaurus>","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164129482623.3986460.10407047347806659820@Monstersaurus>\n\t\n\t<164440962799.3354066.5892501977382531458@Monstersaurus>\n\t\n\t<164441266230.3354066.6384409217676509999@Monstersaurus>","To":"David Plowman ","Date":"Wed, 13 Apr 2022 10:39:44 +0100","Message-ID":"<164984278427.22830.832783742823710611@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22698,"web_url":"https://patchwork.libcamera.org/comment/22698/","msgid":"","date":"2022-04-13T10:14:01","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Wed, 13 Apr 2022 at 10:38, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi David, Naush,\n>\n> Quoting David Plowman (2021-09-22 14:29:15)\n> > These changes retrieve the correct value for sensitivity of the mode\n> > selected for the sensor. This value is known to the CamHelper which\n> > passes it across to the pipeline handler so that it can be set\n> > correctly in the CameraConfiguration.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---\n> > 3 files changed, 18 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> > index e453d46c..a92a76f8 100644\n> > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -38,6 +38,10 @@ struct IPAConfig {\n> > libcamera.FileDescriptor lsTableHandle;\n> > };\n> >\n> > +struct IPAConfigResult {\n> > + float modeSensitivity;\n> > +};\n> > +\n> > struct StartConfig {\n> > libcamera.ControlList controls;\n> > int32 dropFrameCount;\n> > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> > * \\param[in] entityControls Controls provided by the pipeline\n> entities\n> > * \\param[in] ipaConfig Pipeline-handler-specific configuration\n> data\n> > * \\param[out] controls Controls to apply by the pipeline entity\n> > + * \\param[out] result Other results that the pipeline handler\n> may require\n> > *\n> > * This function shall be called when the camera is configured\n> to inform\n> > * the IPA of the camera's streams and the sensor settings.\n> > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> > map streamConfig,\n> > map entityControls,\n> > IPAConfig ipaConfig)\n> > - => (int32 ret, libcamera.ControlList controls);\n> > + => (int32 ret, libcamera.ControlList controls,\n> IPAConfigResult result);\n> >\n> > /**\n> > * \\fn mapBuffers()\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 047123ab..796e6d15 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -97,7 +97,7 @@ public:\n> > const std::map\n> &streamConfig,\n> > const std::map\n> &entityControls,\n> > const ipa::RPi::IPAConfig &data,\n> > - ControlList *controls) override;\n> > + ControlList *controls, ipa::RPi::IPAConfigResult\n> *result) override;\n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> > void signalStatReady(const uint32_t bufferId) override;\n> > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> > [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > const std::map\n> &entityControls,\n> > const ipa::RPi::IPAConfig &ipaConfig,\n> > - ControlList *controls)\n> > + ControlList *controls, ipa::RPi::IPAConfigResult\n> *result)\n> > {\n> > if (entityControls.size() != 2) {\n> > LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> > /* Pass the camera mode to the CamHelper to setup algorithms. */\n> > helper_->SetCameraMode(mode_);\n> >\n> > + /* The pipeline handler passes out the mode's sensitivity. */\n> > + result->modeSensitivity = mode_.sensitivity;\n> > +\n> > if (firstStart_) {\n> > /* Supply initial values for frame durations. */\n> > applyFrameDurations(defaultMinFrameDuration,\n> defaultMaxFrameDuration);\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 0bdfa727..caf0030e 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -147,7 +147,7 @@ public:\n> > void frameStarted(uint32_t sequence);\n> >\n> > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > - int configureIPA(const CameraConfiguration *config);\n> > + int configureIPA(CameraConfiguration *config);\n> >\n> > void statsMetadataComplete(uint32_t bufferId, const ControlList\n> &controls);\n> > void runIsp(uint32_t bufferId);\n> > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig\n> *sensorConfig)\n> > return ipa_->init(settings, sensorConfig);\n> > }\n> >\n> > -int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> > {\n> > /* We know config must be an RPiCameraConfiguration. */\n> > const RPiCameraConfiguration *rpiConfig =\n> > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> >\n> > /* Ready the IPA - it must know about the sensor resolution. */\n> > ControlList controls;\n> > + ipa::RPi::IPAConfigResult result;\n> > ret = ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> ipaConfig,\n> > - &controls);\n> > + &controls, &result);\n>\n> I've just rebased this series to master to facilitate merging, and with\n> fresh eyes I can't help but wonder if this value shouldn't be returned\n> in the validate() phase. (Not sure if this has been asked before / yet).\n>\n> Is there anything that prevents us adding a validate() to the IPA\n> interface to allow validating the configuration and at that point,\n> setting the mode sensitivity? Or can this value /only/ be determined\n> when configuring?\n>\n\nI can't think of any reason that this couldn't be done in validate(), but\nDavid\nmight have some reasons. However, is an application required to call\nCameraConfiguration::validate() directly? I can see that it does get called\nin Camera::configure(), just before the call to\nPipelineHandler::configure().\nIf there is no requirement to call CameraConfiguration::validate(), would it\nmatter where the result gets set?\n\nRegards,\nNaush\n\n\n>\n> Then the validate phase should be able to return the mode sensitivity of\n> the configuration that is being validated.\n>\n> Doing that we could keep the configure calls using a const parameter for\n> their config structures.\n>\n> > if (ret < 0) {\n> > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > return -EPIPE;\n> > }\n> >\n> > + /* Store the mode sensitivity for the application. */\n> > + config->modeSensitivity = result.modeSensitivity;\n> > +\n> > /*\n> > * Configure the H/V flip controls based on the combination of\n> > * the sensor and user transform.\n> > --\n> > 2.20.1\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 074DBC3256\n\tfor ;\n\tWed, 13 Apr 2022 10:14:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5192365646;\n\tWed, 13 Apr 2022 12:14:19 +0200 (CEST)","from mail-lf1-x134.google.com (mail-lf1-x134.google.com\n\t[IPv6:2a00:1450:4864:20::134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1EB24604B4\n\tfor ;\n\tWed, 13 Apr 2022 12:14:18 +0200 (CEST)","by mail-lf1-x134.google.com with SMTP id bu29so2696915lfb.0\n\tfor ;\n\tWed, 13 Apr 2022 03:14:18 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649844859;\n\tbh=3VyljuEIodm3EW8ur8B7AHIIkU6gzPVmX/ut/xP1H2Q=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=tW+FKKWNoYbN9SGFUZVAyj8VL5SCKjdmH6s7nEymcInBJEentSLnFNVarVI/a+eYE\n\tIk7F0fRsAKqhKGzweFvPuCpyhBuzY3aIOyje9xiL8+4cCGgkJNtvFC/GCSUpKAKYeN\n\trjijLW1YGc5pZHkUdBgqhL2BXq3z82ocppM/00VSGxZqfNHrD5tbPzLoUsPXPCQmwW\n\tMBV2+wchFbqcLSaHGs0excug0+s2ErrHwgD7Y1m4GzPH6q5gyoh9VNZCcMM3F1Mnjp\n\tXUOz7cfSEwu7sYPptzXpk448Res0sAAADZTuLb9Pp7b4AP9Byf0rxHmKrYRdIUsoUZ\n\tRFZBz1ph2xaWw==","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=4KW3t3h9LShs2aGWYPmBjPg58N/Nwj6bPGBreG/vJCk=;\n\tb=U9uts6QEfYKuJOCt/dSV94H+THYRxMBwT2kq+N6qkoiRJOnQ1Xtkah1IKZ8+kpuV1z\n\t6txTKU6kRo/OLs6ejl4vizXxy+QUm8OMGuww1i3/G8/Ir8lg7KDPgfxapoROB5Ti00e8\n\t9uU1p8PFrlSX3mEypSCrmWOGqX1Sle18LwsrHv9D6MOy7WPiSUvRE2f7acer9FEea/zD\n\tPIABnHdHfT2NYOjdk4DOqCdu/KlK8AhaIDnmwf9v4Yrg7MbTjmCKjXHVKyUVxssrLs6N\n\tIf8eyNvteW43NM+8aggEcXKlmRs0BHbmF24t2ux9zXTBsVc9sL16+qQfHY21g5UD52zs\n\t/foQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"U9uts6QE\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=4KW3t3h9LShs2aGWYPmBjPg58N/Nwj6bPGBreG/vJCk=;\n\tb=OmmS96TTwLVWs9sLfQk39K8kaUfc2R12ssR6tkxGxtQGyqTCisWK5lcWQK+RNVkLSQ\n\tTxiHwkh93Y43/XnHK0JJhKfbJ5XqNt4LOLZdKm8vsmgfHn/6AFfzjiN7q/Y7n1Gy9qmZ\n\tSuAxsn4FVpdbri83OG974dqFw1QtLu+7e5jyCLnnWfK6R42BKNVvMoTMkEMu3Q/oyJhN\n\tyTz5shi/ZlXBuIw81lNAs9DLeEmjteQ46OKyg7jFLM9oL6mg0zGjW/gA5Yd9cz21WiSZ\n\trpnxak1D1MfW7qV0zs5tQEO4qiNZmOP/S8g/qGOju7/YHFogncprsmAlpKPQHXl3sL9K\n\tqnNw==","X-Gm-Message-State":"AOAM530BHkRuDylgyu/BFYrlCM4AQnXpXd9uEBrkFZ58e/SB3WcQmORS\n\t9kICWD+6YFk3Ytv37RGF0Rm51rFSWU9MyN3rnU9brA==","X-Google-Smtp-Source":"ABdhPJxwiRrIwcEAKNrVQY8A76m45iSUzEn3bgKX7VRJL5MWfpDcpavuitIYrbMLlSsWRmS+pLK7Pkoijkj/Nwu0twg=","X-Received":"by 2002:a05:6512:1090:b0:46b:825b:88c6 with SMTP id\n\tj16-20020a056512109000b0046b825b88c6mr18304978lfg.363.1649844857415;\n\tWed, 13 Apr 2022 03:14:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164984269099.22830.1157254031970565968@Monstersaurus>","In-Reply-To":"<164984269099.22830.1157254031970565968@Monstersaurus>","Date":"Wed, 13 Apr 2022 11:14:01 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000a940ff05dc8670a1\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22699,"web_url":"https://patchwork.libcamera.org/comment/22699/","msgid":"<164984520193.22830.3479884127443747901@Monstersaurus>","date":"2022-04-13T10:20:01","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2022-04-13 11:14:01)\n> Hi Kieran,\n> \n> On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <\n> kieran.bingham@ideasonboard.com> wrote:\n> \n> > Hi David, Naush,\n> >\n> > Quoting David Plowman (2021-09-22 14:29:15)\n> > > These changes retrieve the correct value for sensitivity of the mode\n> > > selected for the sensor. This value is known to the CamHelper which\n> > > passes it across to the pipeline handler so that it can be set\n> > > correctly in the CameraConfiguration.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---\n> > > 3 files changed, 18 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> > b/include/libcamera/ipa/raspberrypi.mojom\n> > > index e453d46c..a92a76f8 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > @@ -38,6 +38,10 @@ struct IPAConfig {\n> > > libcamera.FileDescriptor lsTableHandle;\n> > > };\n> > >\n> > > +struct IPAConfigResult {\n> > > + float modeSensitivity;\n> > > +};\n> > > +\n> > > struct StartConfig {\n> > > libcamera.ControlList controls;\n> > > int32 dropFrameCount;\n> > > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> > > * \\param[in] entityControls Controls provided by the pipeline\n> > entities\n> > > * \\param[in] ipaConfig Pipeline-handler-specific configuration\n> > data\n> > > * \\param[out] controls Controls to apply by the pipeline entity\n> > > + * \\param[out] result Other results that the pipeline handler\n> > may require\n> > > *\n> > > * This function shall be called when the camera is configured\n> > to inform\n> > > * the IPA of the camera's streams and the sensor settings.\n> > > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> > > map streamConfig,\n> > > map entityControls,\n> > > IPAConfig ipaConfig)\n> > > - => (int32 ret, libcamera.ControlList controls);\n> > > + => (int32 ret, libcamera.ControlList controls,\n> > IPAConfigResult result);\n> > >\n> > > /**\n> > > * \\fn mapBuffers()\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 047123ab..796e6d15 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -97,7 +97,7 @@ public:\n> > > const std::map\n> > &streamConfig,\n> > > const std::map\n> > &entityControls,\n> > > const ipa::RPi::IPAConfig &data,\n> > > - ControlList *controls) override;\n> > > + ControlList *controls, ipa::RPi::IPAConfigResult\n> > *result) override;\n> > > void mapBuffers(const std::vector &buffers) override;\n> > > void unmapBuffers(const std::vector &ids) override;\n> > > void signalStatReady(const uint32_t bufferId) override;\n> > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo\n> > &sensorInfo,\n> > > [[maybe_unused]] const std::map > IPAStream> &streamConfig,\n> > > const std::map\n> > &entityControls,\n> > > const ipa::RPi::IPAConfig &ipaConfig,\n> > > - ControlList *controls)\n> > > + ControlList *controls, ipa::RPi::IPAConfigResult\n> > *result)\n> > > {\n> > > if (entityControls.size() != 2) {\n> > > LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo\n> > &sensorInfo,\n> > > /* Pass the camera mode to the CamHelper to setup algorithms. */\n> > > helper_->SetCameraMode(mode_);\n> > >\n> > > + /* The pipeline handler passes out the mode's sensitivity. */\n> > > + result->modeSensitivity = mode_.sensitivity;\n> > > +\n> > > if (firstStart_) {\n> > > /* Supply initial values for frame durations. */\n> > > applyFrameDurations(defaultMinFrameDuration,\n> > defaultMaxFrameDuration);\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 0bdfa727..caf0030e 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -147,7 +147,7 @@ public:\n> > > void frameStarted(uint32_t sequence);\n> > >\n> > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > > - int configureIPA(const CameraConfiguration *config);\n> > > + int configureIPA(CameraConfiguration *config);\n> > >\n> > > void statsMetadataComplete(uint32_t bufferId, const ControlList\n> > &controls);\n> > > void runIsp(uint32_t bufferId);\n> > > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig\n> > *sensorConfig)\n> > > return ipa_->init(settings, sensorConfig);\n> > > }\n> > >\n> > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> > > {\n> > > /* We know config must be an RPiCameraConfiguration. */\n> > > const RPiCameraConfiguration *rpiConfig =\n> > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const\n> > CameraConfiguration *config)\n> > >\n> > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > ControlList controls;\n> > > + ipa::RPi::IPAConfigResult result;\n> > > ret = ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> > ipaConfig,\n> > > - &controls);\n> > > + &controls, &result);\n> >\n> > I've just rebased this series to master to facilitate merging, and with\n> > fresh eyes I can't help but wonder if this value shouldn't be returned\n> > in the validate() phase. (Not sure if this has been asked before / yet).\n> >\n> > Is there anything that prevents us adding a validate() to the IPA\n> > interface to allow validating the configuration and at that point,\n> > setting the mode sensitivity? Or can this value /only/ be determined\n> > when configuring?\n> >\n> \n> I can't think of any reason that this couldn't be done in validate(), but\n> David\n> might have some reasons. However, is an application required to call\n> CameraConfiguration::validate() directly? I can see that it does get called\n> in Camera::configure(), just before the call to\n> PipelineHandler::configure().\n> If there is no requirement to call CameraConfiguration::validate(), would it\n> matter where the result gets set?\n\nThere isn't a 'requirement' to do so as such, but it's recommended. If\nthe call to configure can't be satisfied 'precisely' then the configure\ncall should fail. It should not make any changes or adjustments to the\nconfiguration.\n\nValidate will tell you if the pipeline handler made any changes, and\nmore than that - the validate before configure /must/ report that no\nchanges were made.\n\nWhen an application calls validate() - the pipeline handler can make\nchanges to the Configuration (such as filling in the ModeSensitivity\nhere) and if a change was made - it can return 'ConfigurationAdjusted' -\nwhich then means the application should check to see what was different\nfrom what it requested - to what the pipeline handler will deliver.\n\nI would like there to be a way to highlight 'what' changes were made if\nthe validate returns 'Adjusted' - but I'm not sure how we could easily\nimplement that yet.\n\n--\nKieran\n\n\n> Regards,\n> Naush\n> \n> \n> >\n> > Then the validate phase should be able to return the mode sensitivity of\n> > the configuration that is being validated.\n> >\n> > Doing that we could keep the configure calls using a const parameter for\n> > their config structures.\n> >\n> > > if (ret < 0) {\n> > > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > return -EPIPE;\n> > > }\n> > >\n> > > + /* Store the mode sensitivity for the application. */\n> > > + config->modeSensitivity = result.modeSensitivity;\n> > > +\n> > > /*\n> > > * Configure the H/V flip controls based on the combination of\n> > > * the sensor and user transform.\n> > > --\n> > > 2.20.1\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 A1D4BC0F1B\n\tfor ;\n\tWed, 13 Apr 2022 10:20:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A551C65644;\n\tWed, 13 Apr 2022 12:20:05 +0200 (CEST)","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 D5080604B4\n\tfor ;\n\tWed, 13 Apr 2022 12:20:04 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6C83125B;\n\tWed, 13 Apr 2022 12:20:04 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649845205;\n\tbh=2RLuTZIApwQmJ15bfMX3+FtKcJ+l9J9I6NE+Vzde3SQ=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=pEPUCbjigRD2AMNLBwoB3XwWehW2A2s3BKIMhbh4PuOd4ePiwo34MkKbPpIwlTfn6\n\txgkQKr8nZ+/+/UFrXUA/PAgKUqwWn2OubCOU8BpTQ/JV83I/jn6maRkjFiYGDGwKA6\n\tPmpt8oov/T5T9oTGxTX/ES4Wt4KFSbrUZxX1kFk0a3irFKC9Sq6+GLQQfRWJtxkfCt\n\t/huNAlDu2VIVouypTg1CGebF1CnGOC6Ay1EKBvS1SDwwvXYtU9qgglp4Bex7Qz6PgC\n\tbMiCGmLUyDrHK7X2D4pNwz95ThX61Y2cKMFlgIfImuNmsXMzkBTOIyda3DzbW7+vYw\n\tHnfcUziHotAGA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649845204;\n\tbh=2RLuTZIApwQmJ15bfMX3+FtKcJ+l9J9I6NE+Vzde3SQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=rd+Jf85Q62tSgeyqYtFzodSADnN8Er9P8cs7iflid3C0CjZ9ij8NIhyeoFcnH/pLP\n\tyo8BT8tKvfNR7s/VLBT5JoKrWxTBypmV7aZDCtKzAVKVlgioXnY6W1aj+GtLXbOAx3\n\tFlLOoMutCJxOKPbubOKX4z9zzUZFm380bK1wbdT8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rd+Jf85Q\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164984269099.22830.1157254031970565968@Monstersaurus>\n\t","To":"Naushir Patuck ","Date":"Wed, 13 Apr 2022 11:20:01 +0100","Message-ID":"<164984520193.22830.3479884127443747901@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22700,"web_url":"https://patchwork.libcamera.org/comment/22700/","msgid":"<164984754726.22830.12000365412223581826@Monstersaurus>","date":"2022-04-13T10:59:07","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2022-04-13 11:44:14)\n> Hi Kieran,\n> \n> On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <\n> kieran.bingham@ideasonboard.com> wrote:\n> \n> > Quoting Naushir Patuck (2022-04-13 11:14:01)\n> > > Hi Kieran,\n> > >\n> > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <\n> > > kieran.bingham@ideasonboard.com> wrote:\n> > >\n> > > > Hi David, Naush,\n> > > >\n> > > > Quoting David Plowman (2021-09-22 14:29:15)\n> > > > > These changes retrieve the correct value for sensitivity of the mode\n> > > > > selected for the sensor. This value is known to the CamHelper which\n> > > > > passes it across to the pipeline handler so that it can be set\n> > > > > correctly in the CameraConfiguration.\n> > > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---\n> > > > > 3 files changed, 18 insertions(+), 6 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> > > > b/include/libcamera/ipa/raspberrypi.mojom\n> > > > > index e453d46c..a92a76f8 100644\n> > > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > > > @@ -38,6 +38,10 @@ struct IPAConfig {\n> > > > > libcamera.FileDescriptor lsTableHandle;\n> > > > > };\n> > > > >\n> > > > > +struct IPAConfigResult {\n> > > > > + float modeSensitivity;\n> > > > > +};\n> > > > > +\n> > > > > struct StartConfig {\n> > > > > libcamera.ControlList controls;\n> > > > > int32 dropFrameCount;\n> > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> > > > > * \\param[in] entityControls Controls provided by the\n> > pipeline\n> > > > entities\n> > > > > * \\param[in] ipaConfig Pipeline-handler-specific\n> > configuration\n> > > > data\n> > > > > * \\param[out] controls Controls to apply by the pipeline\n> > entity\n> > > > > + * \\param[out] result Other results that the pipeline handler\n> > > > may require\n> > > > > *\n> > > > > * This function shall be called when the camera is\n> > configured\n> > > > to inform\n> > > > > * the IPA of the camera's streams and the sensor settings.\n> > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> > > > > map streamConfig,\n> > > > > map\n> > entityControls,\n> > > > > IPAConfig ipaConfig)\n> > > > > - => (int32 ret, libcamera.ControlList controls);\n> > > > > + => (int32 ret, libcamera.ControlList controls,\n> > > > IPAConfigResult result);\n> > > > >\n> > > > > /**\n> > > > > * \\fn mapBuffers()\n> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > index 047123ab..796e6d15 100644\n> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > @@ -97,7 +97,7 @@ public:\n> > > > > const std::map\n> > > > &streamConfig,\n> > > > > const std::map\n> > > > &entityControls,\n> > > > > const ipa::RPi::IPAConfig &data,\n> > > > > - ControlList *controls) override;\n> > > > > + ControlList *controls,\n> > ipa::RPi::IPAConfigResult\n> > > > *result) override;\n> > > > > void mapBuffers(const std::vector &buffers)\n> > override;\n> > > > > void unmapBuffers(const std::vector &ids)\n> > override;\n> > > > > void signalStatReady(const uint32_t bufferId) override;\n> > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo\n> > > > &sensorInfo,\n> > > > > [[maybe_unused]] const std::map > > > IPAStream> &streamConfig,\n> > > > > const std::map\n> > > > &entityControls,\n> > > > > const ipa::RPi::IPAConfig &ipaConfig,\n> > > > > - ControlList *controls)\n> > > > > + ControlList *controls,\n> > ipa::RPi::IPAConfigResult\n> > > > *result)\n> > > > > {\n> > > > > if (entityControls.size() != 2) {\n> > > > > LOG(IPARPI, Error) << \"No ISP or sensor controls\n> > found.\";\n> > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo\n> > > > &sensorInfo,\n> > > > > /* Pass the camera mode to the CamHelper to setup\n> > algorithms. */\n> > > > > helper_->SetCameraMode(mode_);\n> > > > >\n> > > > > + /* The pipeline handler passes out the mode's sensitivity. */\n> > > > > + result->modeSensitivity = mode_.sensitivity;\n> > > > > +\n> > > > > if (firstStart_) {\n> > > > > /* Supply initial values for frame durations. */\n> > > > > applyFrameDurations(defaultMinFrameDuration,\n> > > > defaultMaxFrameDuration);\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index 0bdfa727..caf0030e 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -147,7 +147,7 @@ public:\n> > > > > void frameStarted(uint32_t sequence);\n> > > > >\n> > > > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > > > > - int configureIPA(const CameraConfiguration *config);\n> > > > > + int configureIPA(CameraConfiguration *config);\n> > > > >\n> > > > > void statsMetadataComplete(uint32_t bufferId, const\n> > ControlList\n> > > > &controls);\n> > > > > void runIsp(uint32_t bufferId);\n> > > > > @@ -1250,7 +1250,7 @@ int\n> > RPiCameraData::loadIPA(ipa::RPi::SensorConfig\n> > > > *sensorConfig)\n> > > > > return ipa_->init(settings, sensorConfig);\n> > > > > }\n> > > > >\n> > > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> > > > > {\n> > > > > /* We know config must be an RPiCameraConfiguration. */\n> > > > > const RPiCameraConfiguration *rpiConfig =\n> > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const\n> > > > CameraConfiguration *config)\n> > > > >\n> > > > > /* Ready the IPA - it must know about the sensor resolution.\n> > */\n> > > > > ControlList controls;\n> > > > > + ipa::RPi::IPAConfigResult result;\n> > > > > ret = ipa_->configure(sensorInfo_, streamConfig,\n> > entityControls,\n> > > > ipaConfig,\n> > > > > - &controls);\n> > > > > + &controls, &result);\n> > > >\n> > > > I've just rebased this series to master to facilitate merging, and with\n> > > > fresh eyes I can't help but wonder if this value shouldn't be returned\n> > > > in the validate() phase. (Not sure if this has been asked before /\n> > yet).\n> > > >\n> > > > Is there anything that prevents us adding a validate() to the IPA\n> > > > interface to allow validating the configuration and at that point,\n> > > > setting the mode sensitivity? Or can this value /only/ be determined\n> > > > when configuring?\n> > > >\n> > >\n> > > I can't think of any reason that this couldn't be done in validate(), but\n> > > David\n> > > might have some reasons. However, is an application required to call\n> > > CameraConfiguration::validate() directly? I can see that it does get\n> > called\n> > > in Camera::configure(), just before the call to\n> > > PipelineHandler::configure().\n> > > If there is no requirement to call CameraConfiguration::validate(),\n> > would it\n> > > matter where the result gets set?\n> >\n> > There isn't a 'requirement' to do so as such, but it's recommended. If\n> > the call to configure can't be satisfied 'precisely' then the configure\n> > call should fail. It should not make any changes or adjustments to the\n> > configuration.\n> >\n> > Validate will tell you if the pipeline handler made any changes, and\n> > more than that - the validate before configure /must/ report that no\n> > changes were made.\n> >\n> \n> I think this is the part that may be troublesome. Assume an application\n> did not call CameraConfiguration::validate() directly, but it does get\n> called\n> from PipelineHandler::configure(). The validate() will then adjust the\n> sensitivity value and return a Status::Adjusted. This will then fail the\n> PipelineHandler::configure() call. Perhaps CameraConfiguration::validate()\n> should be a mandatory call from the application? Or if we adjust the\n> sensitivity in CameraConfiguration::validate(), we don't return\n> Status::Adjusted?\n> \n\nI'm sorry - I was wrong to say there isn't a requirement to do so.\n\nhttps://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details\n\ndocuments that applications 'shall' be validated by a call to\nvalidate().\n\n\"\"\"\nThe CameraConfiguration holds an ordered list of stream configurations.\nIt supports iterators and operates as a vector of StreamConfiguration\ninstances. The stream configurations are inserted by addConfiguration(),\nand the at() function or operator[] return a reference to the\nStreamConfiguration based on its insertion index. Accessing a stream\nconfiguration with an invalid index results in undefined behaviour.\n\nCameraConfiguration instances are retrieved from the camera with\nCamera::generateConfiguration(). Applications may then inspect the\nconfiguration, modify it, and possibly add new stream configuration\nentries with addConfiguration(). Once the camera configuration satisfies\nthe application, it shall be validated by a call to validate(). The\nvalidation implements \"try\" semantics: it adjusts invalid configurations\nto the closest achievable parameters instead of rejecting them\ncompletely. Applications then decide whether to accept the modified\nconfiguration, or try again with a different set of parameters. Once the\nconfiguration is valid, it is passed to Camera::configure().\n\n\"\"\"\n\n--\nKieran\n\n\n> Naush\n> \n> \n> >\n> > When an application calls validate() - the pipeline handler can make\n> > changes to the Configuration (such as filling in the ModeSensitivity\n> > here) and if a change was made - it can return 'ConfigurationAdjusted' -\n> > which then means the application should check to see what was different\n> > from what it requested - to what the pipeline handler will deliver.\n> >\n> > I would like there to be a way to highlight 'what' changes were made if\n> > the validate returns 'Adjusted' - but I'm not sure how we could easily\n> > implement that yet.\n> >\n> > --\n> > Kieran\n> >\n> >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > > >\n> > > > Then the validate phase should be able to return the mode sensitivity\n> > of\n> > > > the configuration that is being validated.\n> > > >\n> > > > Doing that we could keep the configure calls using a const parameter\n> > for\n> > > > their config structures.\n> > > >\n> > > > > if (ret < 0) {\n> > > > > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > > > return -EPIPE;\n> > > > > }\n> > > > >\n> > > > > + /* Store the mode sensitivity for the application. */\n> > > > > + config->modeSensitivity = result.modeSensitivity;\n> > > > > +\n> > > > > /*\n> > > > > * Configure the H/V flip controls based on the combination\n> > of\n> > > > > * the sensor and user transform.\n> > > > > --\n> > > > > 2.20.1\n> > > > >\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 EEF80C3256\n\tfor ;\n\tWed, 13 Apr 2022 10:59:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C16C6563E;\n\tWed, 13 Apr 2022 12:59:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0979604B4\n\tfor ;\n\tWed, 13 Apr 2022 12:59:09 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5AF2C25B;\n\tWed, 13 Apr 2022 12:59:09 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649847551;\n\tbh=RzZDiqDDYHzyQAxv/Zs8ILKYkzL4lz6lAWz+Qj9LAHU=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=CG50PzCEcmzWMgCr2FGXfOyP6HY9aoqc8/X0bOy7nGj5u/SGv9HZvLU1qoXLA8eDV\n\tbmfSWnRCAC7AbelVB40A6rbJqEJl+IxprYnhtHnNMd6Jo0kEFd6ovMe/wT59tkjRx5\n\tPh79F46yDU6dwhJRq64vDrVhVHN1m9NJJEk1Dd5ywJkoVAjbB6/yNotqTbzYvjIjvs\n\tn+SrY1+vYFafnKpfSUDDPRNjN49JP2ld/IGo3LxyEZS9iIANoCCPL2nFJMvknbE3qx\n\tk4v00kv/Fp5jkEFQ2O6YVMvdJXUzwFr96NRKmBiko0S+G0OSVIEpTi82bvGN+qsQWP\n\tgPTAXgqOFMigg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649847549;\n\tbh=RzZDiqDDYHzyQAxv/Zs8ILKYkzL4lz6lAWz+Qj9LAHU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=N3Fe7hBXnRP5BMjAWkonvGUnUaAj0mpN0GN+vx/X9LC5XPSdw4m0a3TpeEkGcM4j9\n\ttQ+6K3kOyAw97iWAYWmr3uLKtTglZKgyrc5CBiqzaAmm3V4MViDlWqKcRUYWs/5W9h\n\tSzu6EfMPpY1VyDv+vio3imsyb/YDO53lwxHLUKKU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"N3Fe7hBX\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164984269099.22830.1157254031970565968@Monstersaurus>\n\t\n\t<164984520193.22830.3479884127443747901@Monstersaurus>\n\t","To":"Naushir Patuck ","Date":"Wed, 13 Apr 2022 11:59:07 +0100","Message-ID":"<164984754726.22830.12000365412223581826@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22703,"web_url":"https://patchwork.libcamera.org/comment/22703/","msgid":"","date":"2022-04-13T12:36:41","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Wed, 13 Apr 2022 at 11:59, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Naushir Patuck (2022-04-13 11:44:14)\n> > Hi Kieran,\n> >\n> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <\n> > kieran.bingham@ideasonboard.com> wrote:\n> >\n> > > Quoting Naushir Patuck (2022-04-13 11:14:01)\n> > > > Hi Kieran,\n> > > >\n> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <\n> > > > kieran.bingham@ideasonboard.com> wrote:\n> > > >\n> > > > > Hi David, Naush,\n> > > > >\n> > > > > Quoting David Plowman (2021-09-22 14:29:15)\n> > > > > > These changes retrieve the correct value for sensitivity of the\n> mode\n> > > > > > selected for the sensor. This value is known to the CamHelper\n> which\n> > > > > > passes it across to the pipeline handler so that it can be set\n> > > > > > correctly in the CameraConfiguration.\n> > > > > >\n> > > > > > Signed-off-by: David Plowman \n> > > > > > ---\n> > > > > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10\n> +++++++---\n> > > > > > 3 files changed, 18 insertions(+), 6 deletions(-)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> > > > > b/include/libcamera/ipa/raspberrypi.mojom\n> > > > > > index e453d46c..a92a76f8 100644\n> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {\n> > > > > > libcamera.FileDescriptor lsTableHandle;\n> > > > > > };\n> > > > > >\n> > > > > > +struct IPAConfigResult {\n> > > > > > + float modeSensitivity;\n> > > > > > +};\n> > > > > > +\n> > > > > > struct StartConfig {\n> > > > > > libcamera.ControlList controls;\n> > > > > > int32 dropFrameCount;\n> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> > > > > > * \\param[in] entityControls Controls provided by the\n> > > pipeline\n> > > > > entities\n> > > > > > * \\param[in] ipaConfig Pipeline-handler-specific\n> > > configuration\n> > > > > data\n> > > > > > * \\param[out] controls Controls to apply by the pipeline\n> > > entity\n> > > > > > + * \\param[out] result Other results that the pipeline\n> handler\n> > > > > may require\n> > > > > > *\n> > > > > > * This function shall be called when the camera is\n> > > configured\n> > > > > to inform\n> > > > > > * the IPA of the camera's streams and the sensor\n> settings.\n> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> > > > > > map streamConfig,\n> > > > > > map\n> > > entityControls,\n> > > > > > IPAConfig ipaConfig)\n> > > > > > - => (int32 ret, libcamera.ControlList controls);\n> > > > > > + => (int32 ret, libcamera.ControlList controls,\n> > > > > IPAConfigResult result);\n> > > > > >\n> > > > > > /**\n> > > > > > * \\fn mapBuffers()\n> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > > index 047123ab..796e6d15 100644\n> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > > @@ -97,7 +97,7 @@ public:\n> > > > > > const std::map\n> > > > > &streamConfig,\n> > > > > > const std::map ControlInfoMap>\n> > > > > &entityControls,\n> > > > > > const ipa::RPi::IPAConfig &data,\n> > > > > > - ControlList *controls) override;\n> > > > > > + ControlList *controls,\n> > > ipa::RPi::IPAConfigResult\n> > > > > *result) override;\n> > > > > > void mapBuffers(const std::vector &buffers)\n> > > override;\n> > > > > > void unmapBuffers(const std::vector &ids)\n> > > override;\n> > > > > > void signalStatReady(const uint32_t bufferId) override;\n> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const\n> IPACameraSensorInfo\n> > > > > &sensorInfo,\n> > > > > > [[maybe_unused]] const std::map int,\n> > > > > IPAStream> &streamConfig,\n> > > > > > const std::map ControlInfoMap>\n> > > > > &entityControls,\n> > > > > > const ipa::RPi::IPAConfig &ipaConfig,\n> > > > > > - ControlList *controls)\n> > > > > > + ControlList *controls,\n> > > ipa::RPi::IPAConfigResult\n> > > > > *result)\n> > > > > > {\n> > > > > > if (entityControls.size() != 2) {\n> > > > > > LOG(IPARPI, Error) << \"No ISP or sensor controls\n> > > found.\";\n> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const\n> IPACameraSensorInfo\n> > > > > &sensorInfo,\n> > > > > > /* Pass the camera mode to the CamHelper to setup\n> > > algorithms. */\n> > > > > > helper_->SetCameraMode(mode_);\n> > > > > >\n> > > > > > + /* The pipeline handler passes out the mode's\n> sensitivity. */\n> > > > > > + result->modeSensitivity = mode_.sensitivity;\n> > > > > > +\n> > > > > > if (firstStart_) {\n> > > > > > /* Supply initial values for frame durations. */\n> > > > > > applyFrameDurations(defaultMinFrameDuration,\n> > > > > defaultMaxFrameDuration);\n> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > index 0bdfa727..caf0030e 100644\n> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > @@ -147,7 +147,7 @@ public:\n> > > > > > void frameStarted(uint32_t sequence);\n> > > > > >\n> > > > > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > > > > > - int configureIPA(const CameraConfiguration *config);\n> > > > > > + int configureIPA(CameraConfiguration *config);\n> > > > > >\n> > > > > > void statsMetadataComplete(uint32_t bufferId, const\n> > > ControlList\n> > > > > &controls);\n> > > > > > void runIsp(uint32_t bufferId);\n> > > > > > @@ -1250,7 +1250,7 @@ int\n> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig\n> > > > > *sensorConfig)\n> > > > > > return ipa_->init(settings, sensorConfig);\n> > > > > > }\n> > > > > >\n> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration\n> *config)\n> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> > > > > > {\n> > > > > > /* We know config must be an RPiCameraConfiguration. */\n> > > > > > const RPiCameraConfiguration *rpiConfig =\n> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const\n> > > > > CameraConfiguration *config)\n> > > > > >\n> > > > > > /* Ready the IPA - it must know about the sensor\n> resolution.\n> > > */\n> > > > > > ControlList controls;\n> > > > > > + ipa::RPi::IPAConfigResult result;\n> > > > > > ret = ipa_->configure(sensorInfo_, streamConfig,\n> > > entityControls,\n> > > > > ipaConfig,\n> > > > > > - &controls);\n> > > > > > + &controls, &result);\n> > > > >\n> > > > > I've just rebased this series to master to facilitate merging, and\n> with\n> > > > > fresh eyes I can't help but wonder if this value shouldn't be\n> returned\n> > > > > in the validate() phase. (Not sure if this has been asked before /\n> > > yet).\n> > > > >\n> > > > > Is there anything that prevents us adding a validate() to the IPA\n> > > > > interface to allow validating the configuration and at that point,\n> > > > > setting the mode sensitivity? Or can this value /only/ be\n> determined\n> > > > > when configuring?\n> > > > >\n> > > >\n> > > > I can't think of any reason that this couldn't be done in\n> validate(), but\n> > > > David\n> > > > might have some reasons. However, is an application required to call\n> > > > CameraConfiguration::validate() directly? I can see that it does get\n> > > called\n> > > > in Camera::configure(), just before the call to\n> > > > PipelineHandler::configure().\n> > > > If there is no requirement to call CameraConfiguration::validate(),\n> > > would it\n> > > > matter where the result gets set?\n> > >\n> > > There isn't a 'requirement' to do so as such, but it's recommended. If\n> > > the call to configure can't be satisfied 'precisely' then the configure\n> > > call should fail. It should not make any changes or adjustments to the\n> > > configuration.\n> > >\n> > > Validate will tell you if the pipeline handler made any changes, and\n> > > more than that - the validate before configure /must/ report that no\n> > > changes were made.\n> > >\n> >\n> > I think this is the part that may be troublesome. Assume an application\n> > did not call CameraConfiguration::validate() directly, but it does get\n> > called\n> > from PipelineHandler::configure(). The validate() will then adjust the\n> > sensitivity value and return a Status::Adjusted. This will then fail the\n> > PipelineHandler::configure() call. Perhaps\n> CameraConfiguration::validate()\n> > should be a mandatory call from the application? Or if we adjust the\n> > sensitivity in CameraConfiguration::validate(), we don't return\n> > Status::Adjusted?\n> >\n>\n> I'm sorry - I was wrong to say there isn't a requirement to do so.\n>\n>\n> https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details\n>\n> documents that applications 'shall' be validated by a call to\n> validate().\n>\n\nAh, that makes things less ambiguous then. I'll let David comment further\nwhen he is back, but I see no reason not to switch this to validate() now.\n\nRegards,\nNaush\n\n\n>\n> \"\"\"\n> The CameraConfiguration holds an ordered list of stream configurations.\n> It supports iterators and operates as a vector of StreamConfiguration\n> instances. The stream configurations are inserted by addConfiguration(),\n> and the at() function or operator[] return a reference to the\n> StreamConfiguration based on its insertion index. Accessing a stream\n> configuration with an invalid index results in undefined behaviour.\n>\n> CameraConfiguration instances are retrieved from the camera with\n> Camera::generateConfiguration(). Applications may then inspect the\n> configuration, modify it, and possibly add new stream configuration\n> entries with addConfiguration(). Once the camera configuration satisfies\n> the application, it shall be validated by a call to validate(). The\n> validation implements \"try\" semantics: it adjusts invalid configurations\n> to the closest achievable parameters instead of rejecting them\n> completely. Applications then decide whether to accept the modified\n> configuration, or try again with a different set of parameters. Once the\n> configuration is valid, it is passed to Camera::configure().\n>\n> \"\"\"\n>\n> --\n> Kieran\n>\n>\n> > Naush\n> >\n> >\n> > >\n> > > When an application calls validate() - the pipeline handler can make\n> > > changes to the Configuration (such as filling in the ModeSensitivity\n> > > here) and if a change was made - it can return 'ConfigurationAdjusted'\n> -\n> > > which then means the application should check to see what was different\n> > > from what it requested - to what the pipeline handler will deliver.\n> > >\n> > > I would like there to be a way to highlight 'what' changes were made if\n> > > the validate returns 'Adjusted' - but I'm not sure how we could easily\n> > > implement that yet.\n> > >\n> > > --\n> > > Kieran\n> > >\n> > >\n> > > > Regards,\n> > > > Naush\n> > > >\n> > > >\n> > > > >\n> > > > > Then the validate phase should be able to return the mode\n> sensitivity\n> > > of\n> > > > > the configuration that is being validated.\n> > > > >\n> > > > > Doing that we could keep the configure calls using a const\n> parameter\n> > > for\n> > > > > their config structures.\n> > > > >\n> > > > > > if (ret < 0) {\n> > > > > > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > > > > return -EPIPE;\n> > > > > > }\n> > > > > >\n> > > > > > + /* Store the mode sensitivity for the application. */\n> > > > > > + config->modeSensitivity = result.modeSensitivity;\n> > > > > > +\n> > > > > > /*\n> > > > > > * Configure the H/V flip controls based on the\n> combination\n> > > of\n> > > > > > * the sensor and user transform.\n> > > > > > --\n> > > > > > 2.20.1\n> > > > > >\n> > > > >\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 3A7FFC0F1B\n\tfor ;\n\tWed, 13 Apr 2022 12:37:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 278AD65644;\n\tWed, 13 Apr 2022 14:37:02 +0200 (CEST)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E0342604B4\n\tfor ;\n\tWed, 13 Apr 2022 14:36:59 +0200 (CEST)","by mail-lj1-x229.google.com with SMTP id bn33so2017639ljb.6\n\tfor ;\n\tWed, 13 Apr 2022 05:36:59 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649853422;\n\tbh=2pwOVO2wd2887oMFisFkbmzbgaJgL6vZtdLjNW25/KQ=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=EOXuWNM/VY8wPc/I9nsZTZuLcIl/EA8XCo1mGPI1uuz85ZzID37ne+mHHlAr5/TLF\n\tsGktdFyN5t9CklvnjrU5eW/ZyvAsdgHa963JrDFqI8lAJFxg9g98HvLoYURZa11L+l\n\ts3P63H8ldCyxMsxLWkqCYSSD5v5p6pnSldie7bIS+nt2v2RsFqfSEFII6Bgv5a4dY2\n\tq8yfjtnPkrcmqiR+MT9oiQHi8wt8YSv/mkCxXlSSIeQmkXz8+ho2bmXF2oI+3uPLd9\n\to/fwJyrjFwUPUbsvWib4Ck7uA+J362fbSgV7lR3Gn5h1zgdr+aRX27udiDgPFemfhC\n\tvYU5E5abEjqMQ==","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=/XkcDmO0/52q4JF+vmFThZZiS9SLRW7CCzrpKeYr+9g=;\n\tb=oOF9bj4r7+1JdvHSZI8o5MCZ0w6h6fuTE+K8QtKcIADDAH8eFhA9cTUgjRduH5xc4w\n\tuHGmAfoMxMAZ92FUjiPE/ADvXDWuJ7GIwvrRzUj/PD6g6tXzmng2OPhBomKsq43CEy8z\n\t44xdcHV2D/5HLE7hvFbIJ3RPBerKDUnMgW7CLZR0CnZLYVBW8hdch90D+QLAYbXEgH96\n\tJG+K5cXs2MARmwNxh8XCk2BGD5kmTzJgPKjH85OGtow0iKr3AG1I5St3YpKUoyyaM00N\n\tEFEpiCcfTaR+A1V8BzjrowKK0BSiK8hRhg+z6lCKvTii/ImdW6GN9dbY8cMW6XAjdJHL\n\tTkaw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"oOF9bj4r\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=/XkcDmO0/52q4JF+vmFThZZiS9SLRW7CCzrpKeYr+9g=;\n\tb=UnzvIFpN5nvKNN2QA7Pn7BC6vfA1FkzkZisfpwcmTWwHVDKWvV971PzVAzU8RqZgnY\n\tQ2hyDQ1ovFCN7wctx9l7aUtMKTyBmsr0ePdaezs/diUUwdh9Gqij37ucoZ8Hb37onruu\n\txtTlfrMXYE/BErncZknq94b0M4dNUCbGvyXUYDkxP7uxKOz8WSdf9bzCAlulfRK63G1q\n\tcZdI9qpgl9Ey0Z7R4cZLtwqH6DYknNlgZzoMwSSQVcLGXZ0gV9fC+pJbL1+r1fiuMOEo\n\tbRSCSsPvMCyWwiYNKGxdCoiedkAmW6650Zx7GIDuD/iN2Jupmkzo94YHhCoN8y8VNQk8\n\tw7ig==","X-Gm-Message-State":"AOAM531tRT8nKAvJeTCT5NzTOXO9s9Bh4jAK2vOeJEkEl3bYIIvHvNct\n\thPFJfqzFCot/vEtlFv4minSBuIqOZiDeiugc8N3aVQ==","X-Google-Smtp-Source":"ABdhPJzXAFj9NZLJjBEqu7fwcuV0MKC2eFxC702SnLOaD1ghodJgB/UNcc5fDnlLKCYY1g/J9+XAhXMyqFMhWKcfaAQ=","X-Received":"by 2002:a2e:9017:0:b0:24b:6502:d63c with SMTP id\n\th23-20020a2e9017000000b0024b6502d63cmr8434906ljg.426.1649853417656;\n\tWed, 13 Apr 2022 05:36:57 -0700 (PDT)","MIME-Version":"1.0","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164984269099.22830.1157254031970565968@Monstersaurus>\n\t\n\t<164984520193.22830.3479884127443747901@Monstersaurus>\n\t\n\t<164984754726.22830.12000365412223581826@Monstersaurus>","In-Reply-To":"<164984754726.22830.12000365412223581826@Monstersaurus>","Date":"Wed, 13 Apr 2022 13:36:41 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000e42a3805dc886e78\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22769,"web_url":"https://patchwork.libcamera.org/comment/22769/","msgid":"","date":"2022-04-21T13:13:45","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Wed, 13 Apr 2022 at 13:36, Naushir Patuck wrote:\n\n>\n>\n> On Wed, 13 Apr 2022 at 11:59, Kieran Bingham <\n> kieran.bingham@ideasonboard.com> wrote:\n>\n>> Quoting Naushir Patuck (2022-04-13 11:44:14)\n>> > Hi Kieran,\n>> >\n>> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <\n>> > kieran.bingham@ideasonboard.com> wrote:\n>> >\n>> > > Quoting Naushir Patuck (2022-04-13 11:14:01)\n>> > > > Hi Kieran,\n>> > > >\n>> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <\n>> > > > kieran.bingham@ideasonboard.com> wrote:\n>> > > >\n>> > > > > Hi David, Naush,\n>> > > > >\n>> > > > > Quoting David Plowman (2021-09-22 14:29:15)\n>> > > > > > These changes retrieve the correct value for sensitivity of the\n>> mode\n>> > > > > > selected for the sensor. This value is known to the CamHelper\n>> which\n>> > > > > > passes it across to the pipeline handler so that it can be set\n>> > > > > > correctly in the CameraConfiguration.\n>> > > > > >\n>> > > > > > Signed-off-by: David Plowman \n>> > > > > > ---\n>> > > > > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n>> > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n>> > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10\n>> +++++++---\n>> > > > > > 3 files changed, 18 insertions(+), 6 deletions(-)\n>> > > > > >\n>> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n>> > > > > b/include/libcamera/ipa/raspberrypi.mojom\n>> > > > > > index e453d46c..a92a76f8 100644\n>> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n>> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n>> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {\n>> > > > > > libcamera.FileDescriptor lsTableHandle;\n>> > > > > > };\n>> > > > > >\n>> > > > > > +struct IPAConfigResult {\n>> > > > > > + float modeSensitivity;\n>> > > > > > +};\n>> > > > > > +\n>> > > > > > struct StartConfig {\n>> > > > > > libcamera.ControlList controls;\n>> > > > > > int32 dropFrameCount;\n>> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n>> > > > > > * \\param[in] entityControls Controls provided by the\n>> > > pipeline\n>> > > > > entities\n>> > > > > > * \\param[in] ipaConfig Pipeline-handler-specific\n>> > > configuration\n>> > > > > data\n>> > > > > > * \\param[out] controls Controls to apply by the\n>> pipeline\n>> > > entity\n>> > > > > > + * \\param[out] result Other results that the pipeline\n>> handler\n>> > > > > may require\n>> > > > > > *\n>> > > > > > * This function shall be called when the camera is\n>> > > configured\n>> > > > > to inform\n>> > > > > > * the IPA of the camera's streams and the sensor\n>> settings.\n>> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n>> > > > > > map streamConfig,\n>> > > > > > map\n>> > > entityControls,\n>> > > > > > IPAConfig ipaConfig)\n>> > > > > > - => (int32 ret, libcamera.ControlList controls);\n>> > > > > > + => (int32 ret, libcamera.ControlList controls,\n>> > > > > IPAConfigResult result);\n>> > > > > >\n>> > > > > > /**\n>> > > > > > * \\fn mapBuffers()\n>> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n>> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > > > > > index 047123ab..796e6d15 100644\n>> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > > > > > @@ -97,7 +97,7 @@ public:\n>> > > > > > const std::map\n>> > > > > &streamConfig,\n>> > > > > > const std::map> ControlInfoMap>\n>> > > > > &entityControls,\n>> > > > > > const ipa::RPi::IPAConfig &data,\n>> > > > > > - ControlList *controls) override;\n>> > > > > > + ControlList *controls,\n>> > > ipa::RPi::IPAConfigResult\n>> > > > > *result) override;\n>> > > > > > void mapBuffers(const std::vector &buffers)\n>> > > override;\n>> > > > > > void unmapBuffers(const std::vector &ids)\n>> > > override;\n>> > > > > > void signalStatReady(const uint32_t bufferId) override;\n>> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const\n>> IPACameraSensorInfo\n>> > > > > &sensorInfo,\n>> > > > > > [[maybe_unused]] const std::map> int,\n>> > > > > IPAStream> &streamConfig,\n>> > > > > > const std::map> ControlInfoMap>\n>> > > > > &entityControls,\n>> > > > > > const ipa::RPi::IPAConfig &ipaConfig,\n>> > > > > > - ControlList *controls)\n>> > > > > > + ControlList *controls,\n>> > > ipa::RPi::IPAConfigResult\n>> > > > > *result)\n>> > > > > > {\n>> > > > > > if (entityControls.size() != 2) {\n>> > > > > > LOG(IPARPI, Error) << \"No ISP or sensor controls\n>> > > found.\";\n>> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const\n>> IPACameraSensorInfo\n>> > > > > &sensorInfo,\n>> > > > > > /* Pass the camera mode to the CamHelper to setup\n>> > > algorithms. */\n>> > > > > > helper_->SetCameraMode(mode_);\n>> > > > > >\n>> > > > > > + /* The pipeline handler passes out the mode's\n>> sensitivity. */\n>> > > > > > + result->modeSensitivity = mode_.sensitivity;\n>> > > > > > +\n>> > > > > > if (firstStart_) {\n>> > > > > > /* Supply initial values for frame durations. */\n>> > > > > > applyFrameDurations(defaultMinFrameDuration,\n>> > > > > defaultMaxFrameDuration);\n>> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > > > > > index 0bdfa727..caf0030e 100644\n>> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > > > > > @@ -147,7 +147,7 @@ public:\n>> > > > > > void frameStarted(uint32_t sequence);\n>> > > > > >\n>> > > > > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n>> > > > > > - int configureIPA(const CameraConfiguration *config);\n>> > > > > > + int configureIPA(CameraConfiguration *config);\n>> > > > > >\n>> > > > > > void statsMetadataComplete(uint32_t bufferId, const\n>> > > ControlList\n>> > > > > &controls);\n>> > > > > > void runIsp(uint32_t bufferId);\n>> > > > > > @@ -1250,7 +1250,7 @@ int\n>> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig\n>> > > > > *sensorConfig)\n>> > > > > > return ipa_->init(settings, sensorConfig);\n>> > > > > > }\n>> > > > > >\n>> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration\n>> *config)\n>> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n>> > > > > > {\n>> > > > > > /* We know config must be an RPiCameraConfiguration. */\n>> > > > > > const RPiCameraConfiguration *rpiConfig =\n>> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const\n>> > > > > CameraConfiguration *config)\n>> > > > > >\n>> > > > > > /* Ready the IPA - it must know about the sensor\n>> resolution.\n>> > > */\n>> > > > > > ControlList controls;\n>> > > > > > + ipa::RPi::IPAConfigResult result;\n>> > > > > > ret = ipa_->configure(sensorInfo_, streamConfig,\n>> > > entityControls,\n>> > > > > ipaConfig,\n>> > > > > > - &controls);\n>> > > > > > + &controls, &result);\n>> > > > >\n>> > > > > I've just rebased this series to master to facilitate merging,\n>> and with\n>> > > > > fresh eyes I can't help but wonder if this value shouldn't be\n>> returned\n>> > > > > in the validate() phase. (Not sure if this has been asked before /\n>> > > yet).\n>> > > > >\n>> > > > > Is there anything that prevents us adding a validate() to the IPA\n>> > > > > interface to allow validating the configuration and at that point,\n>> > > > > setting the mode sensitivity? Or can this value /only/ be\n>> determined\n>> > > > > when configuring?\n>> > > > >\n>> > > >\n>> > > > I can't think of any reason that this couldn't be done in\n>> validate(), but\n>> > > > David\n>> > > > might have some reasons. However, is an application required to\n>> call\n>> > > > CameraConfiguration::validate() directly? I can see that it does\n>> get\n>> > > called\n>> > > > in Camera::configure(), just before the call to\n>> > > > PipelineHandler::configure().\n>> > > > If there is no requirement to call CameraConfiguration::validate(),\n>> > > would it\n>> > > > matter where the result gets set?\n>> > >\n>> > > There isn't a 'requirement' to do so as such, but it's recommended. If\n>> > > the call to configure can't be satisfied 'precisely' then the\n>> configure\n>> > > call should fail. It should not make any changes or adjustments to the\n>> > > configuration.\n>> > >\n>> > > Validate will tell you if the pipeline handler made any changes, and\n>> > > more than that - the validate before configure /must/ report that no\n>> > > changes were made.\n>> > >\n>> >\n>> > I think this is the part that may be troublesome. Assume an application\n>> > did not call CameraConfiguration::validate() directly, but it does get\n>> > called\n>> > from PipelineHandler::configure(). The validate() will then adjust the\n>> > sensitivity value and return a Status::Adjusted. This will then fail\n>> the\n>> > PipelineHandler::configure() call. Perhaps\n>> CameraConfiguration::validate()\n>> > should be a mandatory call from the application? Or if we adjust the\n>> > sensitivity in CameraConfiguration::validate(), we don't return\n>> > Status::Adjusted?\n>> >\n>>\n>> I'm sorry - I was wrong to say there isn't a requirement to do so.\n>>\n>>\n>> https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details\n>>\n>> documents that applications 'shall' be validated by a call to\n>> validate().\n>>\n>\n> Ah, that makes things less ambiguous then. I'll let David comment further\n> when he is back, but I see no reason not to switch this to validate() now.\n>\n\nAfter having prototyped an ipa::validate() to pass the sensitivity back to\nthe\nPH, I've changed my mind on the above statement :-)\n\nThe reason for this is that things just feel a bit more awkward in the\nvalidate\ncall.For instance, we have not yet set up a sensor mode in the IPA to use\nfor\ndeducing the mode sensitivity. If we move the mode setup into\nipa::validate(),\nthere are further things associated with the mode that need to happen there\n(e.g.\nhelper->set_mode())that do not feel right to me.\n\nInstead of doing things this way, I wonder if we should look to pass the\nsensitivity out via the Camera::properties_ ControlList. My initial\nreservation\nsuggesting this was the assumption that properties were static through the\nlifetime of the camera.However, this is not the case, as we have\nproperties::ScalerCropMaximum that changes on every call to configure().\nWhat if\nwe added a properties::SensorSensitivity that also gets updated in the same\nway?\nThis will remove the need for CameraConfiguration::modeSensitivityfield\nentirely.\n\nThanks,\nNaush\n\n\n>\n> Regards,\n> Naush\n>\n>\n>>\n>> \"\"\"\n>> The CameraConfiguration holds an ordered list of stream configurations.\n>> It supports iterators and operates as a vector of StreamConfiguration\n>> instances. The stream configurations are inserted by addConfiguration(),\n>> and the at() function or operator[] return a reference to the\n>> StreamConfiguration based on its insertion index. Accessing a stream\n>> configuration with an invalid index results in undefined behaviour.\n>>\n>> CameraConfiguration instances are retrieved from the camera with\n>> Camera::generateConfiguration(). Applications may then inspect the\n>> configuration, modify it, and possibly add new stream configuration\n>> entries with addConfiguration(). Once the camera configuration satisfies\n>> the application, it shall be validated by a call to validate(). The\n>> validation implements \"try\" semantics: it adjusts invalid configurations\n>> to the closest achievable parameters instead of rejecting them\n>> completely. Applications then decide whether to accept the modified\n>> configuration, or try again with a different set of parameters. Once the\n>> configuration is valid, it is passed to Camera::configure().\n>>\n>> \"\"\"\n>>\n>> --\n>> Kieran\n>>\n>>\n>> > Naush\n>> >\n>> >\n>> > >\n>> > > When an application calls validate() - the pipeline handler can make\n>> > > changes to the Configuration (such as filling in the ModeSensitivity\n>> > > here) and if a change was made - it can return\n>> 'ConfigurationAdjusted' -\n>> > > which then means the application should check to see what was\n>> different\n>> > > from what it requested - to what the pipeline handler will deliver.\n>> > >\n>> > > I would like there to be a way to highlight 'what' changes were made\n>> if\n>> > > the validate returns 'Adjusted' - but I'm not sure how we could easily\n>> > > implement that yet.\n>> > >\n>> > > --\n>> > > Kieran\n>> > >\n>> > >\n>> > > > Regards,\n>> > > > Naush\n>> > > >\n>> > > >\n>> > > > >\n>> > > > > Then the validate phase should be able to return the mode\n>> sensitivity\n>> > > of\n>> > > > > the configuration that is being validated.\n>> > > > >\n>> > > > > Doing that we could keep the configure calls using a const\n>> parameter\n>> > > for\n>> > > > > their config structures.\n>> > > > >\n>> > > > > > if (ret < 0) {\n>> > > > > > LOG(RPI, Error) << \"IPA configuration failed!\";\n>> > > > > > return -EPIPE;\n>> > > > > > }\n>> > > > > >\n>> > > > > > + /* Store the mode sensitivity for the application. */\n>> > > > > > + config->modeSensitivity = result.modeSensitivity;\n>> > > > > > +\n>> > > > > > /*\n>> > > > > > * Configure the H/V flip controls based on the\n>> combination\n>> > > of\n>> > > > > > * the sensor and user transform.\n>> > > > > > --\n>> > > > > > 2.20.1\n>> > > > > >\n>> > > > >\n>> > >\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 5F218C0F1B\n\tfor ;\n\tThu, 21 Apr 2022 13:14:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A986B65646;\n\tThu, 21 Apr 2022 15:14:03 +0200 (CEST)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 52E4865641\n\tfor ;\n\tThu, 21 Apr 2022 15:14:02 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id bq30so8645071lfb.3\n\tfor ;\n\tThu, 21 Apr 2022 06:14:02 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650546843;\n\tbh=fuM9QRm2kmYV7Q6wdGZuc0FX31yZ3iYlgXvBQvemyD4=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=4goNdke0Iw+OftDLC9TLUlPS97YayO1pZzDThBlPfsvvNOFNeRdu8ZDNXL0FTeKIY\n\teH+/Qok5LlK9ZGnK9fzhKhkPwhZUCk6JOb+y97idLwxO/St14Jiye8TbBJnYB8h1NR\n\t/0TNLPm943WBN41HWazKext6E04OkPm6Lpq8Y3pV9PYbMUdjGdkLeuz6B9XnBFnCNS\n\tOQnNnVUCLqC3nVmLanXCZkSja3l5q5i2IAsdSY8uKWky77xPwlYYyqP5tfHRTWs7et\n\tg3NpnrxwkPKUNlO7S2RlZY+hRNWSFkhPWKexneChosI5Vmq4/s+XxJ4OTetSL1aGyd\n\t/Q+5ngVa3jB3A==","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=At38JUyoS+nB5SpoVG7XlzSAwg3vHgHFYBzDlsodGDc=;\n\tb=oEchC6I2J/y9eU+YNlzXe3EdWOcpyJ62jq8SwNJHTnBWy0xztdZ6989UVArtUQ96nk\n\tglAgsaoaPxaeQTFxV4+K4U1K8nBH+KDuDWiXeZoOIssvfo5XTvjXVBEYNvf4B/haeRON\n\tV+8t+ppSk2Iv41oi985VktEmlH1325/97PlMRFAiONA2v+7i2SDMQWwNUQpp5vWD+Ts1\n\t9dBxyqdGjVkD+VYsiTqHi4J3yUqdJg5qhomguWh9C0X8OOzWuaxsovCu7tGjHzJNk6CK\n\tSek3ymd/n0TWPzzT2CRNMU2Qkjjhmp+sAyg6F2XPvSc/8VBjBsuYTH9cg6+s/Ib0TUoA\n\tQYnA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"oEchC6I2\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=At38JUyoS+nB5SpoVG7XlzSAwg3vHgHFYBzDlsodGDc=;\n\tb=tjRSWpewFkAQirUHDPhjxIHv15odBGlxzl7+GEGGRifMobW/kuW8DI4ilXefBNQJ0Z\n\tk+/rH3FvbEIq68ANjnPH015fwDRBN3Ks2QqEdlbBVJUJq89QTLoxb92qWDPSaZuiPopC\n\tQvjmm90J6V9Ikik7QBaYWuQlz0RDPuVQKUV+bT2ayXmk536HXS8IXEXnROWZ0Qxz4rnN\n\txm+Jxxr0ow/OeyH9cWr8V8T+573N6UjYormarTtkCp5gFFjJXSXkhQRByeoa6v92iIzk\n\ts+zAbMHK24Kqp0BPDB6gd1odssONqWU2nqnVOgqtimMS2Oa2NbkcgKSXxJU9GHYYSQmO\n\tMIgg==","X-Gm-Message-State":"AOAM530zbZh5emI1GfU5W1owGk7c+PW8abX2BgQZAlI+d8jYA+V8nWLX\n\t7YEWU6D0vG421BsC+04GSaj1XefdCkjS/Y3qut95O9pGi/LAkw==","X-Google-Smtp-Source":"ABdhPJwtpIbZ50x+Go3e10G0Sf6idCpWYgw1tl1cJ0Q1INv3bJc3rZdY89cvWRsDFadpfWpzvV4erjjHX5ICWqYld6k=","X-Received":"by 2002:a19:7909:0:b0:46b:beb7:12f1 with SMTP id\n\tu9-20020a197909000000b0046bbeb712f1mr18570167lfc.63.1650546841448;\n\tThu, 21 Apr 2022 06:14:01 -0700 (PDT)","MIME-Version":"1.0","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164984269099.22830.1157254031970565968@Monstersaurus>\n\t\n\t<164984520193.22830.3479884127443747901@Monstersaurus>\n\t\n\t<164984754726.22830.12000365412223581826@Monstersaurus>\n\t","In-Reply-To":"","Date":"Thu, 21 Apr 2022 14:13:45 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"0000000000002b864205dd29e2fa\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22770,"web_url":"https://patchwork.libcamera.org/comment/22770/","msgid":"","date":"2022-04-21T13:32:20","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Yes, I recall that there was discussion that this number would be\nbetter as a property, and wasn't the conclusion that we should move it\n(even if not right now)? So doing it like this is certainly fine with\nme.\n\nDavid\n\nOn Thu, 21 Apr 2022 at 14:14, Naushir Patuck wrote:\n>\n> Hi Kieran,\n>\n> On Wed, 13 Apr 2022 at 13:36, Naushir Patuck wrote:\n>>\n>>\n>>\n>> On Wed, 13 Apr 2022 at 11:59, Kieran Bingham wrote:\n>>>\n>>> Quoting Naushir Patuck (2022-04-13 11:44:14)\n>>> > Hi Kieran,\n>>> >\n>>> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <\n>>> > kieran.bingham@ideasonboard.com> wrote:\n>>> >\n>>> > > Quoting Naushir Patuck (2022-04-13 11:14:01)\n>>> > > > Hi Kieran,\n>>> > > >\n>>> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <\n>>> > > > kieran.bingham@ideasonboard.com> wrote:\n>>> > > >\n>>> > > > > Hi David, Naush,\n>>> > > > >\n>>> > > > > Quoting David Plowman (2021-09-22 14:29:15)\n>>> > > > > > These changes retrieve the correct value for sensitivity of the mode\n>>> > > > > > selected for the sensor. This value is known to the CamHelper which\n>>> > > > > > passes it across to the pipeline handler so that it can be set\n>>> > > > > > correctly in the CameraConfiguration.\n>>> > > > > >\n>>> > > > > > Signed-off-by: David Plowman \n>>> > > > > > ---\n>>> > > > > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n>>> > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n>>> > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---\n>>> > > > > > 3 files changed, 18 insertions(+), 6 deletions(-)\n>>> > > > > >\n>>> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n>>> > > > > b/include/libcamera/ipa/raspberrypi.mojom\n>>> > > > > > index e453d46c..a92a76f8 100644\n>>> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n>>> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n>>> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {\n>>> > > > > > libcamera.FileDescriptor lsTableHandle;\n>>> > > > > > };\n>>> > > > > >\n>>> > > > > > +struct IPAConfigResult {\n>>> > > > > > + float modeSensitivity;\n>>> > > > > > +};\n>>> > > > > > +\n>>> > > > > > struct StartConfig {\n>>> > > > > > libcamera.ControlList controls;\n>>> > > > > > int32 dropFrameCount;\n>>> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n>>> > > > > > * \\param[in] entityControls Controls provided by the\n>>> > > pipeline\n>>> > > > > entities\n>>> > > > > > * \\param[in] ipaConfig Pipeline-handler-specific\n>>> > > configuration\n>>> > > > > data\n>>> > > > > > * \\param[out] controls Controls to apply by the pipeline\n>>> > > entity\n>>> > > > > > + * \\param[out] result Other results that the pipeline handler\n>>> > > > > may require\n>>> > > > > > *\n>>> > > > > > * This function shall be called when the camera is\n>>> > > configured\n>>> > > > > to inform\n>>> > > > > > * the IPA of the camera's streams and the sensor settings.\n>>> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n>>> > > > > > map streamConfig,\n>>> > > > > > map\n>>> > > entityControls,\n>>> > > > > > IPAConfig ipaConfig)\n>>> > > > > > - => (int32 ret, libcamera.ControlList controls);\n>>> > > > > > + => (int32 ret, libcamera.ControlList controls,\n>>> > > > > IPAConfigResult result);\n>>> > > > > >\n>>> > > > > > /**\n>>> > > > > > * \\fn mapBuffers()\n>>> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n>>> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp\n>>> > > > > > index 047123ab..796e6d15 100644\n>>> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>>> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>>> > > > > > @@ -97,7 +97,7 @@ public:\n>>> > > > > > const std::map\n>>> > > > > &streamConfig,\n>>> > > > > > const std::map\n>>> > > > > &entityControls,\n>>> > > > > > const ipa::RPi::IPAConfig &data,\n>>> > > > > > - ControlList *controls) override;\n>>> > > > > > + ControlList *controls,\n>>> > > ipa::RPi::IPAConfigResult\n>>> > > > > *result) override;\n>>> > > > > > void mapBuffers(const std::vector &buffers)\n>>> > > override;\n>>> > > > > > void unmapBuffers(const std::vector &ids)\n>>> > > override;\n>>> > > > > > void signalStatReady(const uint32_t bufferId) override;\n>>> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo\n>>> > > > > &sensorInfo,\n>>> > > > > > [[maybe_unused]] const std::map>> > > > > IPAStream> &streamConfig,\n>>> > > > > > const std::map\n>>> > > > > &entityControls,\n>>> > > > > > const ipa::RPi::IPAConfig &ipaConfig,\n>>> > > > > > - ControlList *controls)\n>>> > > > > > + ControlList *controls,\n>>> > > ipa::RPi::IPAConfigResult\n>>> > > > > *result)\n>>> > > > > > {\n>>> > > > > > if (entityControls.size() != 2) {\n>>> > > > > > LOG(IPARPI, Error) << \"No ISP or sensor controls\n>>> > > found.\";\n>>> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo\n>>> > > > > &sensorInfo,\n>>> > > > > > /* Pass the camera mode to the CamHelper to setup\n>>> > > algorithms. */\n>>> > > > > > helper_->SetCameraMode(mode_);\n>>> > > > > >\n>>> > > > > > + /* The pipeline handler passes out the mode's sensitivity. */\n>>> > > > > > + result->modeSensitivity = mode_.sensitivity;\n>>> > > > > > +\n>>> > > > > > if (firstStart_) {\n>>> > > > > > /* Supply initial values for frame durations. */\n>>> > > > > > applyFrameDurations(defaultMinFrameDuration,\n>>> > > > > defaultMaxFrameDuration);\n>>> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> > > > > > index 0bdfa727..caf0030e 100644\n>>> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> > > > > > @@ -147,7 +147,7 @@ public:\n>>> > > > > > void frameStarted(uint32_t sequence);\n>>> > > > > >\n>>> > > > > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n>>> > > > > > - int configureIPA(const CameraConfiguration *config);\n>>> > > > > > + int configureIPA(CameraConfiguration *config);\n>>> > > > > >\n>>> > > > > > void statsMetadataComplete(uint32_t bufferId, const\n>>> > > ControlList\n>>> > > > > &controls);\n>>> > > > > > void runIsp(uint32_t bufferId);\n>>> > > > > > @@ -1250,7 +1250,7 @@ int\n>>> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig\n>>> > > > > *sensorConfig)\n>>> > > > > > return ipa_->init(settings, sensorConfig);\n>>> > > > > > }\n>>> > > > > >\n>>> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)\n>>> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n>>> > > > > > {\n>>> > > > > > /* We know config must be an RPiCameraConfiguration. */\n>>> > > > > > const RPiCameraConfiguration *rpiConfig =\n>>> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const\n>>> > > > > CameraConfiguration *config)\n>>> > > > > >\n>>> > > > > > /* Ready the IPA - it must know about the sensor resolution.\n>>> > > */\n>>> > > > > > ControlList controls;\n>>> > > > > > + ipa::RPi::IPAConfigResult result;\n>>> > > > > > ret = ipa_->configure(sensorInfo_, streamConfig,\n>>> > > entityControls,\n>>> > > > > ipaConfig,\n>>> > > > > > - &controls);\n>>> > > > > > + &controls, &result);\n>>> > > > >\n>>> > > > > I've just rebased this series to master to facilitate merging, and with\n>>> > > > > fresh eyes I can't help but wonder if this value shouldn't be returned\n>>> > > > > in the validate() phase. (Not sure if this has been asked before /\n>>> > > yet).\n>>> > > > >\n>>> > > > > Is there anything that prevents us adding a validate() to the IPA\n>>> > > > > interface to allow validating the configuration and at that point,\n>>> > > > > setting the mode sensitivity? Or can this value /only/ be determined\n>>> > > > > when configuring?\n>>> > > > >\n>>> > > >\n>>> > > > I can't think of any reason that this couldn't be done in validate(), but\n>>> > > > David\n>>> > > > might have some reasons. However, is an application required to call\n>>> > > > CameraConfiguration::validate() directly? I can see that it does get\n>>> > > called\n>>> > > > in Camera::configure(), just before the call to\n>>> > > > PipelineHandler::configure().\n>>> > > > If there is no requirement to call CameraConfiguration::validate(),\n>>> > > would it\n>>> > > > matter where the result gets set?\n>>> > >\n>>> > > There isn't a 'requirement' to do so as such, but it's recommended. If\n>>> > > the call to configure can't be satisfied 'precisely' then the configure\n>>> > > call should fail. It should not make any changes or adjustments to the\n>>> > > configuration.\n>>> > >\n>>> > > Validate will tell you if the pipeline handler made any changes, and\n>>> > > more than that - the validate before configure /must/ report that no\n>>> > > changes were made.\n>>> > >\n>>> >\n>>> > I think this is the part that may be troublesome. Assume an application\n>>> > did not call CameraConfiguration::validate() directly, but it does get\n>>> > called\n>>> > from PipelineHandler::configure(). The validate() will then adjust the\n>>> > sensitivity value and return a Status::Adjusted. This will then fail the\n>>> > PipelineHandler::configure() call. Perhaps CameraConfiguration::validate()\n>>> > should be a mandatory call from the application? Or if we adjust the\n>>> > sensitivity in CameraConfiguration::validate(), we don't return\n>>> > Status::Adjusted?\n>>> >\n>>>\n>>> I'm sorry - I was wrong to say there isn't a requirement to do so.\n>>>\n>>> https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details\n>>>\n>>> documents that applications 'shall' be validated by a call to\n>>> validate().\n>>\n>>\n>> Ah, that makes things less ambiguous then. I'll let David comment further\n>> when he is back, but I see no reason not to switch this to validate() now.\n>\n>\n> After having prototyped an ipa::validate() to pass the sensitivity back to the\n> PH, I've changed my mind on the above statement :-)\n>\n> The reason for this is that things just feel a bit more awkward in the validate\n> call.For instance, we have not yet set up a sensor mode in the IPA to use for\n> deducing the mode sensitivity. If we move the mode setup into ipa::validate(),\n> there are further things associated with the mode that need to happen there (e.g.\n> helper->set_mode())that do not feel right to me.\n>\n> Instead of doing things this way, I wonder if we should look to pass the\n> sensitivity out via the Camera::properties_ ControlList. My initial reservation\n> suggesting this was the assumption that properties were static through the\n> lifetime of the camera.However, this is not the case, as we have\n> properties::ScalerCropMaximum that changes on every call to configure(). What if\n> we added a properties::SensorSensitivity that also gets updated in the same way?\n> This will remove the need for CameraConfiguration::modeSensitivityfield entirely.\n>\n> Thanks,\n> Naush\n>\n>>\n>>\n>> Regards,\n>> Naush\n>>\n>>>\n>>>\n>>> \"\"\"\n>>> The CameraConfiguration holds an ordered list of stream configurations.\n>>> It supports iterators and operates as a vector of StreamConfiguration\n>>> instances. The stream configurations are inserted by addConfiguration(),\n>>> and the at() function or operator[] return a reference to the\n>>> StreamConfiguration based on its insertion index. Accessing a stream\n>>> configuration with an invalid index results in undefined behaviour.\n>>>\n>>> CameraConfiguration instances are retrieved from the camera with\n>>> Camera::generateConfiguration(). Applications may then inspect the\n>>> configuration, modify it, and possibly add new stream configuration\n>>> entries with addConfiguration(). Once the camera configuration satisfies\n>>> the application, it shall be validated by a call to validate(). The\n>>> validation implements \"try\" semantics: it adjusts invalid configurations\n>>> to the closest achievable parameters instead of rejecting them\n>>> completely. Applications then decide whether to accept the modified\n>>> configuration, or try again with a different set of parameters. Once the\n>>> configuration is valid, it is passed to Camera::configure().\n>>>\n>>> \"\"\"\n>>>\n>>> --\n>>> Kieran\n>>>\n>>>\n>>> > Naush\n>>> >\n>>> >\n>>> > >\n>>> > > When an application calls validate() - the pipeline handler can make\n>>> > > changes to the Configuration (such as filling in the ModeSensitivity\n>>> > > here) and if a change was made - it can return 'ConfigurationAdjusted' -\n>>> > > which then means the application should check to see what was different\n>>> > > from what it requested - to what the pipeline handler will deliver.\n>>> > >\n>>> > > I would like there to be a way to highlight 'what' changes were made if\n>>> > > the validate returns 'Adjusted' - but I'm not sure how we could easily\n>>> > > implement that yet.\n>>> > >\n>>> > > --\n>>> > > Kieran\n>>> > >\n>>> > >\n>>> > > > Regards,\n>>> > > > Naush\n>>> > > >\n>>> > > >\n>>> > > > >\n>>> > > > > Then the validate phase should be able to return the mode sensitivity\n>>> > > of\n>>> > > > > the configuration that is being validated.\n>>> > > > >\n>>> > > > > Doing that we could keep the configure calls using a const parameter\n>>> > > for\n>>> > > > > their config structures.\n>>> > > > >\n>>> > > > > > if (ret < 0) {\n>>> > > > > > LOG(RPI, Error) << \"IPA configuration failed!\";\n>>> > > > > > return -EPIPE;\n>>> > > > > > }\n>>> > > > > >\n>>> > > > > > + /* Store the mode sensitivity for the application. */\n>>> > > > > > + config->modeSensitivity = result.modeSensitivity;\n>>> > > > > > +\n>>> > > > > > /*\n>>> > > > > > * Configure the H/V flip controls based on the combination\n>>> > > of\n>>> > > > > > * the sensor and user transform.\n>>> > > > > > --\n>>> > > > > > 2.20.1\n>>> > > > > >\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 7B29EC3256\n\tfor ;\n\tThu, 21 Apr 2022 13:32:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D028765642;\n\tThu, 21 Apr 2022 15:32:33 +0200 (CEST)","from mail-wr1-x435.google.com (mail-wr1-x435.google.com\n\t[IPv6:2a00:1450:4864:20::435])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B23565641\n\tfor ;\n\tThu, 21 Apr 2022 15:32:31 +0200 (CEST)","by mail-wr1-x435.google.com with SMTP id e2so445486wrh.7\n\tfor ;\n\tThu, 21 Apr 2022 06:32:31 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650547953;\n\tbh=hOQnTnwL6x4W66Pyc1elFRg2ZO6pT5p25qlKblNfTUg=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=dkh1G+PfMiMno/2BMepo3OY+LciEErQ7aCqYuyjJZlx0XRRHia4a9IfAijlwXhv6H\n\tBa88N8d+HmI5C7qPEk9kT2lqsgmGWyQ0e0hdAhI2aaVyqPL6a4bf5mRTXeFEK8ag2r\n\tt/VmqcQ76tKVqOllvgxzOyxGgcIlciC4Mw7IapcXCpVcOavj5/3mn1OKeURpgH7UAf\n\txN2CgYCXEjd+FK0uGTS91IxasvMOPneRwoBwt77RWu9l8mcA03d4ERiJwDH0dvGs3N\n\tdCkOexlNZ8qPBkF3vlGCeN6EO1OL/+xlkA4TPOOKIu6Vj0O7j+MRguxIkHbJEeo5S0\n\tCQznwUXw1OfEQ==","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=rr0efjN4HwBojMRqMTC6MjXLHIV8AGLmQPfh/Dg4FgM=;\n\tb=Wj2Kruv8XvTdBJA0wEEKpLq6WjYcobbbSyx8TrOoq2cb66GCXHWpXw0Laur8Dr968k\n\t1ZtV3kfrnM3tquYM+nsHllOdSNP+/9wpEiUurPHhA//ouKBX2aTw9Uvr87Bgl3jKnGyd\n\tdpwPVgjJbpvayULucaGhJWhxk18HYxkEIRRxf95FJZNPNEubG2pf6GQwfgDj1u2/YcEf\n\t3oZmbOWaAtJtxCRsedJswMFA7WWb3IlsOYInQchYqG/GMewIQP4uQFbXKYIEFUFyxR02\n\t342w/m7zMyI/N9cF8YjPQ9L3bwhVQqrdLUeBnxVJGqdVXEjMKH7DBH8iCKYTJdeD8X+S\n\tb7fA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Wj2Kruv8\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=rr0efjN4HwBojMRqMTC6MjXLHIV8AGLmQPfh/Dg4FgM=;\n\tb=g/Ge84pJILf9QmOB0bYGQ8dBH2Z1FGSB5C5oWgA45vxxRaYiznm2WLcaWrahLDRNV7\n\t/SnwlbtXUuGgvjS/zEvLDA0cSQ95u1+xw//ruKweVcgAH1E9fZZGPQWMwgs8JqElWibs\n\tVHnHKpUpqdAKEuSq6VsFRjjV14BoEJiWcI5Mn8Nvk2p/ZtpKOsmY8ROQyOBXHSuCQ2dr\n\tnwbbCLHgfRZ1vW1LMyyNMgTQCybh9XN0jKdLE1rPwmaIjLEjpirYJzNlZeXXCBMuC+UN\n\tRnCyakK9i2bcdhaFaOb74UxU5cmryaTx/8dUPtzfbDY9GxBBdmWVo/LfoDLRq8CWbxkO\n\tvgJQ==","X-Gm-Message-State":"AOAM533dahSsK9ec41KCWwPy6S/S+4JQxqjj5qYM2acwFLIBlERSFaaf\n\tGYt7dydfqVeNhzE3GZE672qJPHdPyO0dEbX7uzyi46eXnVm7Pg==","X-Google-Smtp-Source":"ABdhPJyPcknloOZYEIpVTET37rx1+QfJTG2n7q5AARE1C+lCNzooyjQTCntyVDrRyTUKEgNJtSlGmYjdwN0XE2PRhNA=","X-Received":"by 2002:adf:f4c9:0:b0:207:9e3d:cc1d with SMTP id\n\th9-20020adff4c9000000b002079e3dcc1dmr19041545wrp.597.1650547950906;\n\tThu, 21 Apr 2022 06:32:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164984269099.22830.1157254031970565968@Monstersaurus>\n\t\n\t<164984520193.22830.3479884127443747901@Monstersaurus>\n\t\n\t<164984754726.22830.12000365412223581826@Monstersaurus>\n\t\n\t","In-Reply-To":"","Date":"Thu, 21 Apr 2022 14:32:20 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22771,"web_url":"https://patchwork.libcamera.org/comment/22771/","msgid":"<165054949035.3016671.12341680569043361126@Monstersaurus>","date":"2022-04-21T13:58:10","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush, David,\n\nQuoting Naushir Patuck (2022-04-21 14:13:45)\n> Hi Kieran,\n> \n> On Wed, 13 Apr 2022 at 13:36, Naushir Patuck wrote:\n> \n> >\n> >\n> > On Wed, 13 Apr 2022 at 11:59, Kieran Bingham <\n> > kieran.bingham@ideasonboard.com> wrote:\n> >\n> >> Quoting Naushir Patuck (2022-04-13 11:44:14)\n> >> > Hi Kieran,\n> >> >\n> >> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <\n> >> > kieran.bingham@ideasonboard.com> wrote:\n> >> >\n> >> > > Quoting Naushir Patuck (2022-04-13 11:14:01)\n> >> > > > Hi Kieran,\n> >> > > >\n> >> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <\n> >> > > > kieran.bingham@ideasonboard.com> wrote:\n> >> > > >\n> >> > > > > Hi David, Naush,\n> >> > > > >\n> >> > > > > Quoting David Plowman (2021-09-22 14:29:15)\n> >> > > > > > These changes retrieve the correct value for sensitivity of the\n> >> mode\n> >> > > > > > selected for the sensor. This value is known to the CamHelper\n> >> which\n> >> > > > > > passes it across to the pipeline handler so that it can be set\n> >> > > > > > correctly in the CameraConfiguration.\n> >> > > > > >\n> >> > > > > > Signed-off-by: David Plowman \n> >> > > > > > ---\n> >> > > > > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> >> > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> >> > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10\n> >> +++++++---\n> >> > > > > > 3 files changed, 18 insertions(+), 6 deletions(-)\n> >> > > > > >\n> >> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> >> > > > > b/include/libcamera/ipa/raspberrypi.mojom\n> >> > > > > > index e453d46c..a92a76f8 100644\n> >> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> >> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> >> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {\n> >> > > > > > libcamera.FileDescriptor lsTableHandle;\n> >> > > > > > };\n> >> > > > > >\n> >> > > > > > +struct IPAConfigResult {\n> >> > > > > > + float modeSensitivity;\n> >> > > > > > +};\n> >> > > > > > +\n> >> > > > > > struct StartConfig {\n> >> > > > > > libcamera.ControlList controls;\n> >> > > > > > int32 dropFrameCount;\n> >> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> >> > > > > > * \\param[in] entityControls Controls provided by the\n> >> > > pipeline\n> >> > > > > entities\n> >> > > > > > * \\param[in] ipaConfig Pipeline-handler-specific\n> >> > > configuration\n> >> > > > > data\n> >> > > > > > * \\param[out] controls Controls to apply by the\n> >> pipeline\n> >> > > entity\n> >> > > > > > + * \\param[out] result Other results that the pipeline\n> >> handler\n> >> > > > > may require\n> >> > > > > > *\n> >> > > > > > * This function shall be called when the camera is\n> >> > > configured\n> >> > > > > to inform\n> >> > > > > > * the IPA of the camera's streams and the sensor\n> >> settings.\n> >> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> >> > > > > > map streamConfig,\n> >> > > > > > map\n> >> > > entityControls,\n> >> > > > > > IPAConfig ipaConfig)\n> >> > > > > > - => (int32 ret, libcamera.ControlList controls);\n> >> > > > > > + => (int32 ret, libcamera.ControlList controls,\n> >> > > > > IPAConfigResult result);\n> >> > > > > >\n> >> > > > > > /**\n> >> > > > > > * \\fn mapBuffers()\n> >> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> >> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> > > > > > index 047123ab..796e6d15 100644\n> >> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> > > > > > @@ -97,7 +97,7 @@ public:\n> >> > > > > > const std::map\n> >> > > > > &streamConfig,\n> >> > > > > > const std::map >> ControlInfoMap>\n> >> > > > > &entityControls,\n> >> > > > > > const ipa::RPi::IPAConfig &data,\n> >> > > > > > - ControlList *controls) override;\n> >> > > > > > + ControlList *controls,\n> >> > > ipa::RPi::IPAConfigResult\n> >> > > > > *result) override;\n> >> > > > > > void mapBuffers(const std::vector &buffers)\n> >> > > override;\n> >> > > > > > void unmapBuffers(const std::vector &ids)\n> >> > > override;\n> >> > > > > > void signalStatReady(const uint32_t bufferId) override;\n> >> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const\n> >> IPACameraSensorInfo\n> >> > > > > &sensorInfo,\n> >> > > > > > [[maybe_unused]] const std::map >> int,\n> >> > > > > IPAStream> &streamConfig,\n> >> > > > > > const std::map >> ControlInfoMap>\n> >> > > > > &entityControls,\n> >> > > > > > const ipa::RPi::IPAConfig &ipaConfig,\n> >> > > > > > - ControlList *controls)\n> >> > > > > > + ControlList *controls,\n> >> > > ipa::RPi::IPAConfigResult\n> >> > > > > *result)\n> >> > > > > > {\n> >> > > > > > if (entityControls.size() != 2) {\n> >> > > > > > LOG(IPARPI, Error) << \"No ISP or sensor controls\n> >> > > found.\";\n> >> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const\n> >> IPACameraSensorInfo\n> >> > > > > &sensorInfo,\n> >> > > > > > /* Pass the camera mode to the CamHelper to setup\n> >> > > algorithms. */\n> >> > > > > > helper_->SetCameraMode(mode_);\n> >> > > > > >\n> >> > > > > > + /* The pipeline handler passes out the mode's\n> >> sensitivity. */\n> >> > > > > > + result->modeSensitivity = mode_.sensitivity;\n> >> > > > > > +\n> >> > > > > > if (firstStart_) {\n> >> > > > > > /* Supply initial values for frame durations. */\n> >> > > > > > applyFrameDurations(defaultMinFrameDuration,\n> >> > > > > defaultMaxFrameDuration);\n> >> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > > > > > index 0bdfa727..caf0030e 100644\n> >> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > > > > > @@ -147,7 +147,7 @@ public:\n> >> > > > > > void frameStarted(uint32_t sequence);\n> >> > > > > >\n> >> > > > > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> >> > > > > > - int configureIPA(const CameraConfiguration *config);\n> >> > > > > > + int configureIPA(CameraConfiguration *config);\n> >> > > > > >\n> >> > > > > > void statsMetadataComplete(uint32_t bufferId, const\n> >> > > ControlList\n> >> > > > > &controls);\n> >> > > > > > void runIsp(uint32_t bufferId);\n> >> > > > > > @@ -1250,7 +1250,7 @@ int\n> >> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig\n> >> > > > > *sensorConfig)\n> >> > > > > > return ipa_->init(settings, sensorConfig);\n> >> > > > > > }\n> >> > > > > >\n> >> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration\n> >> *config)\n> >> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> >> > > > > > {\n> >> > > > > > /* We know config must be an RPiCameraConfiguration. */\n> >> > > > > > const RPiCameraConfiguration *rpiConfig =\n> >> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const\n> >> > > > > CameraConfiguration *config)\n> >> > > > > >\n> >> > > > > > /* Ready the IPA - it must know about the sensor\n> >> resolution.\n> >> > > */\n> >> > > > > > ControlList controls;\n> >> > > > > > + ipa::RPi::IPAConfigResult result;\n> >> > > > > > ret = ipa_->configure(sensorInfo_, streamConfig,\n> >> > > entityControls,\n> >> > > > > ipaConfig,\n> >> > > > > > - &controls);\n> >> > > > > > + &controls, &result);\n> >> > > > >\n> >> > > > > I've just rebased this series to master to facilitate merging,\n> >> and with\n> >> > > > > fresh eyes I can't help but wonder if this value shouldn't be\n> >> returned\n> >> > > > > in the validate() phase. (Not sure if this has been asked before /\n> >> > > yet).\n> >> > > > >\n> >> > > > > Is there anything that prevents us adding a validate() to the IPA\n> >> > > > > interface to allow validating the configuration and at that point,\n> >> > > > > setting the mode sensitivity? Or can this value /only/ be\n> >> determined\n> >> > > > > when configuring?\n> >> > > > >\n> >> > > >\n> >> > > > I can't think of any reason that this couldn't be done in\n> >> validate(), but\n> >> > > > David\n> >> > > > might have some reasons. However, is an application required to\n> >> call\n> >> > > > CameraConfiguration::validate() directly? I can see that it does\n> >> get\n> >> > > called\n> >> > > > in Camera::configure(), just before the call to\n> >> > > > PipelineHandler::configure().\n> >> > > > If there is no requirement to call CameraConfiguration::validate(),\n> >> > > would it\n> >> > > > matter where the result gets set?\n> >> > >\n> >> > > There isn't a 'requirement' to do so as such, but it's recommended. If\n> >> > > the call to configure can't be satisfied 'precisely' then the\n> >> configure\n> >> > > call should fail. It should not make any changes or adjustments to the\n> >> > > configuration.\n> >> > >\n> >> > > Validate will tell you if the pipeline handler made any changes, and\n> >> > > more than that - the validate before configure /must/ report that no\n> >> > > changes were made.\n> >> > >\n> >> >\n> >> > I think this is the part that may be troublesome. Assume an application\n> >> > did not call CameraConfiguration::validate() directly, but it does get\n> >> > called\n> >> > from PipelineHandler::configure(). The validate() will then adjust the\n> >> > sensitivity value and return a Status::Adjusted. This will then fail\n> >> the\n> >> > PipelineHandler::configure() call. Perhaps\n> >> CameraConfiguration::validate()\n> >> > should be a mandatory call from the application? Or if we adjust the\n> >> > sensitivity in CameraConfiguration::validate(), we don't return\n> >> > Status::Adjusted?\n> >> >\n> >>\n> >> I'm sorry - I was wrong to say there isn't a requirement to do so.\n> >>\n> >>\n> >> https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details\n> >>\n> >> documents that applications 'shall' be validated by a call to\n> >> validate().\n> >>\n> >\n> > Ah, that makes things less ambiguous then. I'll let David comment further\n> > when he is back, but I see no reason not to switch this to validate() now.\n> >\n> \n> After having prototyped an ipa::validate() to pass the sensitivity back to\n> the\n> PH, I've changed my mind on the above statement :-)\n> \n> The reason for this is that things just feel a bit more awkward in the\n> validate\n> call.For instance, we have not yet set up a sensor mode in the IPA to use\n> for\n> deducing the mode sensitivity. If we move the mode setup into\n> ipa::validate(),\n> there are further things associated with the mode that need to happen there\n> (e.g.\n> helper->set_mode())that do not feel right to me.\n> \n> Instead of doing things this way, I wonder if we should look to pass the\n> sensitivity out via the Camera::properties_ ControlList. My initial\n> reservation\n> suggesting this was the assumption that properties were static through the\n> lifetime of the camera.However, this is not the case, as we have\n> properties::ScalerCropMaximum that changes on every call to configure().\n> What if\n> we added a properties::SensorSensitivity that also gets updated in the same\n> way?\n> This will remove the need for CameraConfiguration::modeSensitivityfield\n> entirely.\n\nThat sounds ok to me too. I wonder if 'ModeSensitivity' is still the\nright name in that instance ...\n\nI think that would mean that it's only readable after the camera is\nconfigured though - is that ok?\n\n--\nKieran\n\n> \n> Thanks,\n> Naush\n> \n> \n> >\n> > Regards,\n> > Naush\n> >\n> >\n> >>\n> >> \"\"\"\n> >> The CameraConfiguration holds an ordered list of stream configurations.\n> >> It supports iterators and operates as a vector of StreamConfiguration\n> >> instances. The stream configurations are inserted by addConfiguration(),\n> >> and the at() function or operator[] return a reference to the\n> >> StreamConfiguration based on its insertion index. Accessing a stream\n> >> configuration with an invalid index results in undefined behaviour.\n> >>\n> >> CameraConfiguration instances are retrieved from the camera with\n> >> Camera::generateConfiguration(). Applications may then inspect the\n> >> configuration, modify it, and possibly add new stream configuration\n> >> entries with addConfiguration(). Once the camera configuration satisfies\n> >> the application, it shall be validated by a call to validate(). The\n> >> validation implements \"try\" semantics: it adjusts invalid configurations\n> >> to the closest achievable parameters instead of rejecting them\n> >> completely. Applications then decide whether to accept the modified\n> >> configuration, or try again with a different set of parameters. Once the\n> >> configuration is valid, it is passed to Camera::configure().\n> >>\n> >> \"\"\"\n> >>\n> >> --\n> >> Kieran\n> >>\n> >>\n> >> > Naush\n> >> >\n> >> >\n> >> > >\n> >> > > When an application calls validate() - the pipeline handler can make\n> >> > > changes to the Configuration (such as filling in the ModeSensitivity\n> >> > > here) and if a change was made - it can return\n> >> 'ConfigurationAdjusted' -\n> >> > > which then means the application should check to see what was\n> >> different\n> >> > > from what it requested - to what the pipeline handler will deliver.\n> >> > >\n> >> > > I would like there to be a way to highlight 'what' changes were made\n> >> if\n> >> > > the validate returns 'Adjusted' - but I'm not sure how we could easily\n> >> > > implement that yet.\n> >> > >\n> >> > > --\n> >> > > Kieran\n> >> > >\n> >> > >\n> >> > > > Regards,\n> >> > > > Naush\n> >> > > >\n> >> > > >\n> >> > > > >\n> >> > > > > Then the validate phase should be able to return the mode\n> >> sensitivity\n> >> > > of\n> >> > > > > the configuration that is being validated.\n> >> > > > >\n> >> > > > > Doing that we could keep the configure calls using a const\n> >> parameter\n> >> > > for\n> >> > > > > their config structures.\n> >> > > > >\n> >> > > > > > if (ret < 0) {\n> >> > > > > > LOG(RPI, Error) << \"IPA configuration failed!\";\n> >> > > > > > return -EPIPE;\n> >> > > > > > }\n> >> > > > > >\n> >> > > > > > + /* Store the mode sensitivity for the application. */\n> >> > > > > > + config->modeSensitivity = result.modeSensitivity;\n> >> > > > > > +\n> >> > > > > > /*\n> >> > > > > > * Configure the H/V flip controls based on the\n> >> combination\n> >> > > of\n> >> > > > > > * the sensor and user transform.\n> >> > > > > > --\n> >> > > > > > 2.20.1\n> >> > > > > >\n> >> > > > >\n> >> > >\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 2932DC0F1B\n\tfor ;\n\tThu, 21 Apr 2022 13:58:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2BE2A65646;\n\tThu, 21 Apr 2022 15:58:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D3E265641\n\tfor ;\n\tThu, 21 Apr 2022 15:58:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B15C492;\n\tThu, 21 Apr 2022 15:58:12 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650549494;\n\tbh=1V9SjuGXUxTq9PjFcOl/Rudif2Qeskp9ORD1n21+6Lw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=UspbUSM/VI08xiQpAUe07vtBPZZA02x8/3xn2x8QS9u7fKHBp3rHQdYb+nuPvJGBc\n\tVuoOyiSswyKH0Dwap11zvNufDU6osssFVrhFX2ydC+CgB10iTdfLFhqXZ1R97vw+ic\n\tVwUdXj2NXNc0Hpu6eTMEAZXq2orFeHir/5Hg03gUSb8YlCUv8s0onb9RPbcwgk5JCR\n\tA+BxpuQhZeEzs0wfayu28j66fd6GRYC5oT9jCgpOGWo64AUQveGlWfQP/HcCZcYvdz\n\ttQRe5vSlLdtp0rWl6zoImPhyJ3U3iHkE8HB35COzy9JfaiKb3vkhmdP+7TQ4AHWa/x\n\tP1cZOdiA1MVHw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1650549492;\n\tbh=1V9SjuGXUxTq9PjFcOl/Rudif2Qeskp9ORD1n21+6Lw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=mFfmcBAHqLFdkVFiuNE5ZSJ9FDwtHw5/+JUXQIzVIPP+4kNG52tVdQQMauvgePDF7\n\tGpDnJeONVYAyo+saEzGxaM8+fakx6Nr3kvyXIryS4fYYJV/m0BQ6I+QSmDSs+M+vn+\n\tnV8EF+V6/0Vy+yhmUw/by2rO7JUW/iCfl2Q9JPGk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mFfmcBAH\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164984269099.22830.1157254031970565968@Monstersaurus>\n\t\n\t<164984520193.22830.3479884127443747901@Monstersaurus>\n\t\n\t<164984754726.22830.12000365412223581826@Monstersaurus>\n\t\n\t","To":"Naushir Patuck ","Date":"Thu, 21 Apr 2022 14:58:10 +0100","Message-ID":"<165054949035.3016671.12341680569043361126@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22772,"web_url":"https://patchwork.libcamera.org/comment/22772/","msgid":"","date":"2022-04-21T14:10:25","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Thu, 21 Apr 2022 at 14:58, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi Naush, David,\n>\n> Quoting Naushir Patuck (2022-04-21 14:13:45)\n> > Hi Kieran,\n> >\n> > On Wed, 13 Apr 2022 at 13:36, Naushir Patuck \n> wrote:\n> >\n> > >\n> > >\n> > > On Wed, 13 Apr 2022 at 11:59, Kieran Bingham <\n> > > kieran.bingham@ideasonboard.com> wrote:\n> > >\n> > >> Quoting Naushir Patuck (2022-04-13 11:44:14)\n> > >> > Hi Kieran,\n> > >> >\n> > >> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <\n> > >> > kieran.bingham@ideasonboard.com> wrote:\n> > >> >\n> > >> > > Quoting Naushir Patuck (2022-04-13 11:14:01)\n> > >> > > > Hi Kieran,\n> > >> > > >\n> > >> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <\n> > >> > > > kieran.bingham@ideasonboard.com> wrote:\n> > >> > > >\n> > >> > > > > Hi David, Naush,\n> > >> > > > >\n> > >> > > > > Quoting David Plowman (2021-09-22 14:29:15)\n> > >> > > > > > These changes retrieve the correct value for sensitivity of\n> the\n> > >> mode\n> > >> > > > > > selected for the sensor. This value is known to the\n> CamHelper\n> > >> which\n> > >> > > > > > passes it across to the pipeline handler so that it can be\n> set\n> > >> > > > > > correctly in the CameraConfiguration.\n> > >> > > > > >\n> > >> > > > > > Signed-off-by: David Plowman >\n> > >> > > > > > ---\n> > >> > > > > > include/libcamera/ipa/raspberrypi.mojom | 7\n> ++++++-\n> > >> > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7\n> +++++--\n> > >> > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10\n> > >> +++++++---\n> > >> > > > > > 3 files changed, 18 insertions(+), 6 deletions(-)\n> > >> > > > > >\n> > >> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> > >> > > > > b/include/libcamera/ipa/raspberrypi.mojom\n> > >> > > > > > index e453d46c..a92a76f8 100644\n> > >> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > >> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > >> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {\n> > >> > > > > > libcamera.FileDescriptor lsTableHandle;\n> > >> > > > > > };\n> > >> > > > > >\n> > >> > > > > > +struct IPAConfigResult {\n> > >> > > > > > + float modeSensitivity;\n> > >> > > > > > +};\n> > >> > > > > > +\n> > >> > > > > > struct StartConfig {\n> > >> > > > > > libcamera.ControlList controls;\n> > >> > > > > > int32 dropFrameCount;\n> > >> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> > >> > > > > > * \\param[in] entityControls Controls provided by\n> the\n> > >> > > pipeline\n> > >> > > > > entities\n> > >> > > > > > * \\param[in] ipaConfig Pipeline-handler-specific\n> > >> > > configuration\n> > >> > > > > data\n> > >> > > > > > * \\param[out] controls Controls to apply by the\n> > >> pipeline\n> > >> > > entity\n> > >> > > > > > + * \\param[out] result Other results that the\n> pipeline\n> > >> handler\n> > >> > > > > may require\n> > >> > > > > > *\n> > >> > > > > > * This function shall be called when the camera is\n> > >> > > configured\n> > >> > > > > to inform\n> > >> > > > > > * the IPA of the camera's streams and the sensor\n> > >> settings.\n> > >> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> > >> > > > > > map\n> streamConfig,\n> > >> > > > > > map\n> > >> > > entityControls,\n> > >> > > > > > IPAConfig ipaConfig)\n> > >> > > > > > - => (int32 ret, libcamera.ControlList\n> controls);\n> > >> > > > > > + => (int32 ret, libcamera.ControlList\n> controls,\n> > >> > > > > IPAConfigResult result);\n> > >> > > > > >\n> > >> > > > > > /**\n> > >> > > > > > * \\fn mapBuffers()\n> > >> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> > > > > > index 047123ab..796e6d15 100644\n> > >> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> > > > > > @@ -97,7 +97,7 @@ public:\n> > >> > > > > > const std::map IPAStream>\n> > >> > > > > &streamConfig,\n> > >> > > > > > const std::map > >> ControlInfoMap>\n> > >> > > > > &entityControls,\n> > >> > > > > > const ipa::RPi::IPAConfig &data,\n> > >> > > > > > - ControlList *controls) override;\n> > >> > > > > > + ControlList *controls,\n> > >> > > ipa::RPi::IPAConfigResult\n> > >> > > > > *result) override;\n> > >> > > > > > void mapBuffers(const std::vector\n> &buffers)\n> > >> > > override;\n> > >> > > > > > void unmapBuffers(const std::vector\n> &ids)\n> > >> > > override;\n> > >> > > > > > void signalStatReady(const uint32_t bufferId)\n> override;\n> > >> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const\n> > >> IPACameraSensorInfo\n> > >> > > > > &sensorInfo,\n> > >> > > > > > [[maybe_unused]] const\n> std::map > >> int,\n> > >> > > > > IPAStream> &streamConfig,\n> > >> > > > > > const std::map > >> ControlInfoMap>\n> > >> > > > > &entityControls,\n> > >> > > > > > const ipa::RPi::IPAConfig &ipaConfig,\n> > >> > > > > > - ControlList *controls)\n> > >> > > > > > + ControlList *controls,\n> > >> > > ipa::RPi::IPAConfigResult\n> > >> > > > > *result)\n> > >> > > > > > {\n> > >> > > > > > if (entityControls.size() != 2) {\n> > >> > > > > > LOG(IPARPI, Error) << \"No ISP or sensor\n> controls\n> > >> > > found.\";\n> > >> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const\n> > >> IPACameraSensorInfo\n> > >> > > > > &sensorInfo,\n> > >> > > > > > /* Pass the camera mode to the CamHelper to setup\n> > >> > > algorithms. */\n> > >> > > > > > helper_->SetCameraMode(mode_);\n> > >> > > > > >\n> > >> > > > > > + /* The pipeline handler passes out the mode's\n> > >> sensitivity. */\n> > >> > > > > > + result->modeSensitivity = mode_.sensitivity;\n> > >> > > > > > +\n> > >> > > > > > if (firstStart_) {\n> > >> > > > > > /* Supply initial values for frame\n> durations. */\n> > >> > > > > > applyFrameDurations(defaultMinFrameDuration,\n> > >> > > > > defaultMaxFrameDuration);\n> > >> > > > > > diff --git\n> a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > > > > > index 0bdfa727..caf0030e 100644\n> > >> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > > > > > @@ -147,7 +147,7 @@ public:\n> > >> > > > > > void frameStarted(uint32_t sequence);\n> > >> > > > > >\n> > >> > > > > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > >> > > > > > - int configureIPA(const CameraConfiguration *config);\n> > >> > > > > > + int configureIPA(CameraConfiguration *config);\n> > >> > > > > >\n> > >> > > > > > void statsMetadataComplete(uint32_t bufferId, const\n> > >> > > ControlList\n> > >> > > > > &controls);\n> > >> > > > > > void runIsp(uint32_t bufferId);\n> > >> > > > > > @@ -1250,7 +1250,7 @@ int\n> > >> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig\n> > >> > > > > *sensorConfig)\n> > >> > > > > > return ipa_->init(settings, sensorConfig);\n> > >> > > > > > }\n> > >> > > > > >\n> > >> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration\n> > >> *config)\n> > >> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration\n> *config)\n> > >> > > > > > {\n> > >> > > > > > /* We know config must be an\n> RPiCameraConfiguration. */\n> > >> > > > > > const RPiCameraConfiguration *rpiConfig =\n> > >> > > > > > @@ -1299,13 +1299,17 @@ int\n> RPiCameraData::configureIPA(const\n> > >> > > > > CameraConfiguration *config)\n> > >> > > > > >\n> > >> > > > > > /* Ready the IPA - it must know about the sensor\n> > >> resolution.\n> > >> > > */\n> > >> > > > > > ControlList controls;\n> > >> > > > > > + ipa::RPi::IPAConfigResult result;\n> > >> > > > > > ret = ipa_->configure(sensorInfo_, streamConfig,\n> > >> > > entityControls,\n> > >> > > > > ipaConfig,\n> > >> > > > > > - &controls);\n> > >> > > > > > + &controls, &result);\n> > >> > > > >\n> > >> > > > > I've just rebased this series to master to facilitate merging,\n> > >> and with\n> > >> > > > > fresh eyes I can't help but wonder if this value shouldn't be\n> > >> returned\n> > >> > > > > in the validate() phase. (Not sure if this has been asked\n> before /\n> > >> > > yet).\n> > >> > > > >\n> > >> > > > > Is there anything that prevents us adding a validate() to the\n> IPA\n> > >> > > > > interface to allow validating the configuration and at that\n> point,\n> > >> > > > > setting the mode sensitivity? Or can this value /only/ be\n> > >> determined\n> > >> > > > > when configuring?\n> > >> > > > >\n> > >> > > >\n> > >> > > > I can't think of any reason that this couldn't be done in\n> > >> validate(), but\n> > >> > > > David\n> > >> > > > might have some reasons. However, is an application required to\n> > >> call\n> > >> > > > CameraConfiguration::validate() directly? I can see that it\n> does\n> > >> get\n> > >> > > called\n> > >> > > > in Camera::configure(), just before the call to\n> > >> > > > PipelineHandler::configure().\n> > >> > > > If there is no requirement to call\n> CameraConfiguration::validate(),\n> > >> > > would it\n> > >> > > > matter where the result gets set?\n> > >> > >\n> > >> > > There isn't a 'requirement' to do so as such, but it's\n> recommended. If\n> > >> > > the call to configure can't be satisfied 'precisely' then the\n> > >> configure\n> > >> > > call should fail. It should not make any changes or adjustments\n> to the\n> > >> > > configuration.\n> > >> > >\n> > >> > > Validate will tell you if the pipeline handler made any changes,\n> and\n> > >> > > more than that - the validate before configure /must/ report that\n> no\n> > >> > > changes were made.\n> > >> > >\n> > >> >\n> > >> > I think this is the part that may be troublesome. Assume an\n> application\n> > >> > did not call CameraConfiguration::validate() directly, but it does\n> get\n> > >> > called\n> > >> > from PipelineHandler::configure(). The validate() will then adjust\n> the\n> > >> > sensitivity value and return a Status::Adjusted. This will then\n> fail\n> > >> the\n> > >> > PipelineHandler::configure() call. Perhaps\n> > >> CameraConfiguration::validate()\n> > >> > should be a mandatory call from the application? Or if we adjust the\n> > >> > sensitivity in CameraConfiguration::validate(), we don't return\n> > >> > Status::Adjusted?\n> > >> >\n> > >>\n> > >> I'm sorry - I was wrong to say there isn't a requirement to do so.\n> > >>\n> > >>\n> > >>\n> https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details\n> > >>\n> > >> documents that applications 'shall' be validated by a call to\n> > >> validate().\n> > >>\n> > >\n> > > Ah, that makes things less ambiguous then. I'll let David comment\n> further\n> > > when he is back, but I see no reason not to switch this to validate()\n> now.\n> > >\n> >\n> > After having prototyped an ipa::validate() to pass the sensitivity back\n> to\n> > the\n> > PH, I've changed my mind on the above statement :-)\n> >\n> > The reason for this is that things just feel a bit more awkward in the\n> > validate\n> > call.For instance, we have not yet set up a sensor mode in the IPA to use\n> > for\n> > deducing the mode sensitivity. If we move the mode setup into\n> > ipa::validate(),\n> > there are further things associated with the mode that need to happen\n> there\n> > (e.g.\n> > helper->set_mode())that do not feel right to me.\n> >\n> > Instead of doing things this way, I wonder if we should look to pass the\n> > sensitivity out via the Camera::properties_ ControlList. My initial\n> > reservation\n> > suggesting this was the assumption that properties were static through\n> the\n> > lifetime of the camera.However, this is not the case, as we have\n> > properties::ScalerCropMaximum that changes on every call to configure().\n> > What if\n> > we added a properties::SensorSensitivity that also gets updated in the\n> same\n> > way?\n> > This will remove the need for CameraConfiguration::modeSensitivityfield\n> > entirely.\n>\n> That sounds ok to me too. I wonder if 'ModeSensitivity' is still the\n> right name in that instance ...\n>\n\nWe could use SensorSensitivity?\n\n\n>\n> I think that would mean that it's only readable after the camera is\n> configured though - is that ok?\n>\n\nI think that is unavoidable in this case, but equivalent to how we would\ndeal with properties::ScalerCropMaximum.\n\nNaush\n\n\n\n>\n> --\n> Kieran\n>\n> >\n> > Thanks,\n> > Naush\n> >\n> >\n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > >>\n> > >> \"\"\"\n> > >> The CameraConfiguration holds an ordered list of stream\n> configurations.\n> > >> It supports iterators and operates as a vector of StreamConfiguration\n> > >> instances. The stream configurations are inserted by\n> addConfiguration(),\n> > >> and the at() function or operator[] return a reference to the\n> > >> StreamConfiguration based on its insertion index. Accessing a stream\n> > >> configuration with an invalid index results in undefined behaviour.\n> > >>\n> > >> CameraConfiguration instances are retrieved from the camera with\n> > >> Camera::generateConfiguration(). Applications may then inspect the\n> > >> configuration, modify it, and possibly add new stream configuration\n> > >> entries with addConfiguration(). Once the camera configuration\n> satisfies\n> > >> the application, it shall be validated by a call to validate(). The\n> > >> validation implements \"try\" semantics: it adjusts invalid\n> configurations\n> > >> to the closest achievable parameters instead of rejecting them\n> > >> completely. Applications then decide whether to accept the modified\n> > >> configuration, or try again with a different set of parameters. Once\n> the\n> > >> configuration is valid, it is passed to Camera::configure().\n> > >>\n> > >> \"\"\"\n> > >>\n> > >> --\n> > >> Kieran\n> > >>\n> > >>\n> > >> > Naush\n> > >> >\n> > >> >\n> > >> > >\n> > >> > > When an application calls validate() - the pipeline handler can\n> make\n> > >> > > changes to the Configuration (such as filling in the\n> ModeSensitivity\n> > >> > > here) and if a change was made - it can return\n> > >> 'ConfigurationAdjusted' -\n> > >> > > which then means the application should check to see what was\n> > >> different\n> > >> > > from what it requested - to what the pipeline handler will\n> deliver.\n> > >> > >\n> > >> > > I would like there to be a way to highlight 'what' changes were\n> made\n> > >> if\n> > >> > > the validate returns 'Adjusted' - but I'm not sure how we could\n> easily\n> > >> > > implement that yet.\n> > >> > >\n> > >> > > --\n> > >> > > Kieran\n> > >> > >\n> > >> > >\n> > >> > > > Regards,\n> > >> > > > Naush\n> > >> > > >\n> > >> > > >\n> > >> > > > >\n> > >> > > > > Then the validate phase should be able to return the mode\n> > >> sensitivity\n> > >> > > of\n> > >> > > > > the configuration that is being validated.\n> > >> > > > >\n> > >> > > > > Doing that we could keep the configure calls using a const\n> > >> parameter\n> > >> > > for\n> > >> > > > > their config structures.\n> > >> > > > >\n> > >> > > > > > if (ret < 0) {\n> > >> > > > > > LOG(RPI, Error) << \"IPA configuration\n> failed!\";\n> > >> > > > > > return -EPIPE;\n> > >> > > > > > }\n> > >> > > > > >\n> > >> > > > > > + /* Store the mode sensitivity for the application.\n> */\n> > >> > > > > > + config->modeSensitivity = result.modeSensitivity;\n> > >> > > > > > +\n> > >> > > > > > /*\n> > >> > > > > > * Configure the H/V flip controls based on the\n> > >> combination\n> > >> > > of\n> > >> > > > > > * the sensor and user transform.\n> > >> > > > > > --\n> > >> > > > > > 2.20.1\n> > >> > > > > >\n> > >> > > > >\n> > >> > >\n> > >>\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 66B4EC3256\n\tfor ;\n\tThu, 21 Apr 2022 14:10:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8A78465644;\n\tThu, 21 Apr 2022 16:10:44 +0200 (CEST)","from mail-lf1-x134.google.com (mail-lf1-x134.google.com\n\t[IPv6:2a00:1450:4864:20::134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 859DB65641\n\tfor ;\n\tThu, 21 Apr 2022 16:10:42 +0200 (CEST)","by mail-lf1-x134.google.com with SMTP id x17so8914396lfa.10\n\tfor ;\n\tThu, 21 Apr 2022 07:10:42 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650550244;\n\tbh=wviB+kZDhG2jkyHZhMDACu8xioUUg5T4dzkizCwJ35M=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=v8bYsThAmigvNGYiOWH6WpdrBl0T9SZj2GFwg2nPeztMexBLNPAYp9QUjVAnWCTma\n\ttcwt9A0FyjWqbhC/5p48nHZuQWqw+ZelxfY6kbHSMdMKJXwqu2jtZZaK48nwVCWPys\n\tXwRS+O5LmyEZ3icjPzcGzloPLOYjKskugPsmVQFREBMZMEUY3xSihu1xadLt29BSvz\n\tiv+GtB4dwluqeNPq/a8xPK2yt9jQx8omfX1UV1gfwakzu1CMSTLpPK3Dj+CsC9Omk3\n\tWWhRydnbS1WrxeX3lf6jP13Ka5FBgfySlPlcPoSHRhR1abXHDvFkb1olDjuLOdkQrH\n\tjgyuFM4i6CbuQ==","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=Mho0EyNgPlYP4+ZmiqsyNmVN9z2HG5TJ2xsflQJ9sGA=;\n\tb=BGbFxnamKdEjfmxb5qN5k/5pp3J17F6qWjHdSJyp6npvl1ncfh0+n68rVSkmgWtUTr\n\tMT9+w7xHeIHTLKYx8PgrPr17SfRtUVEhbzMmlZpQh5mCiPeZ8PPnqcCwP9M9E6EScTzX\n\tXwsbBnw7Pk8iTPytcheS6/ozR+SRYoGDUUpjWWTUhioDwfXm86aGstfgzlcmemO4fYdd\n\tq16Zg51iw7bSyjwF1iXTRc5wlLXR+LltITB+ae2SN3BgX+L3c3lIKNFiYPwXaqrd+Kcn\n\t53gdZ48H3fm16QXrwk4NI+fiL6exYNAW8CyYC8E24FjsXmlOIJO4aEY9OrPgEXpzPgM5\n\thdjg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"BGbFxnam\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Mho0EyNgPlYP4+ZmiqsyNmVN9z2HG5TJ2xsflQJ9sGA=;\n\tb=JD2Gn+BcaxPx+YKo4+FOFUhsRFVL9xFfxOlqWz0O5X/rf7LV6htg4f9oJSFOEaS/l+\n\t11AphWySWVda9dgA07dR044qJsLGvN5AvNz92CyLpeyIUlrkgw07sFgEk9SeevFjNa6x\n\t3s2JjtluUehT/uu+SbzfjUqHGvcDfjO4IYZDXnWp5aHHo4UZ317WkyAXTRgIyAXutcp6\n\t39ms7+CAudnNFak9FcqlfNhRkSZzHk/9uqXxJtGr2qp1jN6erz+5QBGwy8cxYbs7f4p4\n\tf+ZtefcX4GN/7N1S/O9QkBoR5jRQ8v6kSJJ0YqSFI+kZ4bYxvuVQLVWH/7Vskif1Yoti\n\tL17Q==","X-Gm-Message-State":"AOAM532QyYW3Vjfv9aQP4PikodPHNZk75FZPEeA831Hi9Jz10VtWZ1yB\n\tun3NwOSVW2bWT3MSVRd6I/gAp4686SEUCwN3Qrvk1A==","X-Google-Smtp-Source":"ABdhPJzkmoUIHfJ3PuOytlV3iA46N8zNK6hoF2EjW2+ewqPgBfclynBgLY10P3k90nRVr9Qi01eB9llkQPx+EeOlQLY=","X-Received":"by 2002:a05:6512:368c:b0:46d:9d6:a832 with SMTP id\n\td12-20020a056512368c00b0046d09d6a832mr17657460lfs.276.1650550241713;\n\tThu, 21 Apr 2022 07:10:41 -0700 (PDT)","MIME-Version":"1.0","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164984269099.22830.1157254031970565968@Monstersaurus>\n\t\n\t<164984520193.22830.3479884127443747901@Monstersaurus>\n\t\n\t<164984754726.22830.12000365412223581826@Monstersaurus>\n\t\n\t\n\t<165054949035.3016671.12341680569043361126@Monstersaurus>","In-Reply-To":"<165054949035.3016671.12341680569043361126@Monstersaurus>","Date":"Thu, 21 Apr 2022 15:10:25 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000d7775c05dd2aacee\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22773,"web_url":"https://patchwork.libcamera.org/comment/22773/","msgid":"","date":"2022-04-21T14:16:16","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran\n\nOn Thu, 21 Apr 2022 at 14:58, Kieran Bingham\n wrote:\n>\n> Hi Naush, David,\n>\n> Quoting Naushir Patuck (2022-04-21 14:13:45)\n> > Hi Kieran,\n> >\n> > On Wed, 13 Apr 2022 at 13:36, Naushir Patuck wrote:\n> >\n> > >\n> > >\n> > > On Wed, 13 Apr 2022 at 11:59, Kieran Bingham <\n> > > kieran.bingham@ideasonboard.com> wrote:\n> > >\n> > >> Quoting Naushir Patuck (2022-04-13 11:44:14)\n> > >> > Hi Kieran,\n> > >> >\n> > >> > On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <\n> > >> > kieran.bingham@ideasonboard.com> wrote:\n> > >> >\n> > >> > > Quoting Naushir Patuck (2022-04-13 11:14:01)\n> > >> > > > Hi Kieran,\n> > >> > > >\n> > >> > > > On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <\n> > >> > > > kieran.bingham@ideasonboard.com> wrote:\n> > >> > > >\n> > >> > > > > Hi David, Naush,\n> > >> > > > >\n> > >> > > > > Quoting David Plowman (2021-09-22 14:29:15)\n> > >> > > > > > These changes retrieve the correct value for sensitivity of the\n> > >> mode\n> > >> > > > > > selected for the sensor. This value is known to the CamHelper\n> > >> which\n> > >> > > > > > passes it across to the pipeline handler so that it can be set\n> > >> > > > > > correctly in the CameraConfiguration.\n> > >> > > > > >\n> > >> > > > > > Signed-off-by: David Plowman \n> > >> > > > > > ---\n> > >> > > > > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-\n> > >> > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--\n> > >> > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10\n> > >> +++++++---\n> > >> > > > > > 3 files changed, 18 insertions(+), 6 deletions(-)\n> > >> > > > > >\n> > >> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> > >> > > > > b/include/libcamera/ipa/raspberrypi.mojom\n> > >> > > > > > index e453d46c..a92a76f8 100644\n> > >> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > >> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > >> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {\n> > >> > > > > > libcamera.FileDescriptor lsTableHandle;\n> > >> > > > > > };\n> > >> > > > > >\n> > >> > > > > > +struct IPAConfigResult {\n> > >> > > > > > + float modeSensitivity;\n> > >> > > > > > +};\n> > >> > > > > > +\n> > >> > > > > > struct StartConfig {\n> > >> > > > > > libcamera.ControlList controls;\n> > >> > > > > > int32 dropFrameCount;\n> > >> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {\n> > >> > > > > > * \\param[in] entityControls Controls provided by the\n> > >> > > pipeline\n> > >> > > > > entities\n> > >> > > > > > * \\param[in] ipaConfig Pipeline-handler-specific\n> > >> > > configuration\n> > >> > > > > data\n> > >> > > > > > * \\param[out] controls Controls to apply by the\n> > >> pipeline\n> > >> > > entity\n> > >> > > > > > + * \\param[out] result Other results that the pipeline\n> > >> handler\n> > >> > > > > may require\n> > >> > > > > > *\n> > >> > > > > > * This function shall be called when the camera is\n> > >> > > configured\n> > >> > > > > to inform\n> > >> > > > > > * the IPA of the camera's streams and the sensor\n> > >> settings.\n> > >> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {\n> > >> > > > > > map streamConfig,\n> > >> > > > > > map\n> > >> > > entityControls,\n> > >> > > > > > IPAConfig ipaConfig)\n> > >> > > > > > - => (int32 ret, libcamera.ControlList controls);\n> > >> > > > > > + => (int32 ret, libcamera.ControlList controls,\n> > >> > > > > IPAConfigResult result);\n> > >> > > > > >\n> > >> > > > > > /**\n> > >> > > > > > * \\fn mapBuffers()\n> > >> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> > > > > > index 047123ab..796e6d15 100644\n> > >> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> > > > > > @@ -97,7 +97,7 @@ public:\n> > >> > > > > > const std::map\n> > >> > > > > &streamConfig,\n> > >> > > > > > const std::map > >> ControlInfoMap>\n> > >> > > > > &entityControls,\n> > >> > > > > > const ipa::RPi::IPAConfig &data,\n> > >> > > > > > - ControlList *controls) override;\n> > >> > > > > > + ControlList *controls,\n> > >> > > ipa::RPi::IPAConfigResult\n> > >> > > > > *result) override;\n> > >> > > > > > void mapBuffers(const std::vector &buffers)\n> > >> > > override;\n> > >> > > > > > void unmapBuffers(const std::vector &ids)\n> > >> > > override;\n> > >> > > > > > void signalStatReady(const uint32_t bufferId) override;\n> > >> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const\n> > >> IPACameraSensorInfo\n> > >> > > > > &sensorInfo,\n> > >> > > > > > [[maybe_unused]] const std::map > >> int,\n> > >> > > > > IPAStream> &streamConfig,\n> > >> > > > > > const std::map > >> ControlInfoMap>\n> > >> > > > > &entityControls,\n> > >> > > > > > const ipa::RPi::IPAConfig &ipaConfig,\n> > >> > > > > > - ControlList *controls)\n> > >> > > > > > + ControlList *controls,\n> > >> > > ipa::RPi::IPAConfigResult\n> > >> > > > > *result)\n> > >> > > > > > {\n> > >> > > > > > if (entityControls.size() != 2) {\n> > >> > > > > > LOG(IPARPI, Error) << \"No ISP or sensor controls\n> > >> > > found.\";\n> > >> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const\n> > >> IPACameraSensorInfo\n> > >> > > > > &sensorInfo,\n> > >> > > > > > /* Pass the camera mode to the CamHelper to setup\n> > >> > > algorithms. */\n> > >> > > > > > helper_->SetCameraMode(mode_);\n> > >> > > > > >\n> > >> > > > > > + /* The pipeline handler passes out the mode's\n> > >> sensitivity. */\n> > >> > > > > > + result->modeSensitivity = mode_.sensitivity;\n> > >> > > > > > +\n> > >> > > > > > if (firstStart_) {\n> > >> > > > > > /* Supply initial values for frame durations. */\n> > >> > > > > > applyFrameDurations(defaultMinFrameDuration,\n> > >> > > > > defaultMaxFrameDuration);\n> > >> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > > > > > index 0bdfa727..caf0030e 100644\n> > >> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > > > > > @@ -147,7 +147,7 @@ public:\n> > >> > > > > > void frameStarted(uint32_t sequence);\n> > >> > > > > >\n> > >> > > > > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > >> > > > > > - int configureIPA(const CameraConfiguration *config);\n> > >> > > > > > + int configureIPA(CameraConfiguration *config);\n> > >> > > > > >\n> > >> > > > > > void statsMetadataComplete(uint32_t bufferId, const\n> > >> > > ControlList\n> > >> > > > > &controls);\n> > >> > > > > > void runIsp(uint32_t bufferId);\n> > >> > > > > > @@ -1250,7 +1250,7 @@ int\n> > >> > > RPiCameraData::loadIPA(ipa::RPi::SensorConfig\n> > >> > > > > *sensorConfig)\n> > >> > > > > > return ipa_->init(settings, sensorConfig);\n> > >> > > > > > }\n> > >> > > > > >\n> > >> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration\n> > >> *config)\n> > >> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)\n> > >> > > > > > {\n> > >> > > > > > /* We know config must be an RPiCameraConfiguration. */\n> > >> > > > > > const RPiCameraConfiguration *rpiConfig =\n> > >> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const\n> > >> > > > > CameraConfiguration *config)\n> > >> > > > > >\n> > >> > > > > > /* Ready the IPA - it must know about the sensor\n> > >> resolution.\n> > >> > > */\n> > >> > > > > > ControlList controls;\n> > >> > > > > > + ipa::RPi::IPAConfigResult result;\n> > >> > > > > > ret = ipa_->configure(sensorInfo_, streamConfig,\n> > >> > > entityControls,\n> > >> > > > > ipaConfig,\n> > >> > > > > > - &controls);\n> > >> > > > > > + &controls, &result);\n> > >> > > > >\n> > >> > > > > I've just rebased this series to master to facilitate merging,\n> > >> and with\n> > >> > > > > fresh eyes I can't help but wonder if this value shouldn't be\n> > >> returned\n> > >> > > > > in the validate() phase. (Not sure if this has been asked before /\n> > >> > > yet).\n> > >> > > > >\n> > >> > > > > Is there anything that prevents us adding a validate() to the IPA\n> > >> > > > > interface to allow validating the configuration and at that point,\n> > >> > > > > setting the mode sensitivity? Or can this value /only/ be\n> > >> determined\n> > >> > > > > when configuring?\n> > >> > > > >\n> > >> > > >\n> > >> > > > I can't think of any reason that this couldn't be done in\n> > >> validate(), but\n> > >> > > > David\n> > >> > > > might have some reasons. However, is an application required to\n> > >> call\n> > >> > > > CameraConfiguration::validate() directly? I can see that it does\n> > >> get\n> > >> > > called\n> > >> > > > in Camera::configure(), just before the call to\n> > >> > > > PipelineHandler::configure().\n> > >> > > > If there is no requirement to call CameraConfiguration::validate(),\n> > >> > > would it\n> > >> > > > matter where the result gets set?\n> > >> > >\n> > >> > > There isn't a 'requirement' to do so as such, but it's recommended. If\n> > >> > > the call to configure can't be satisfied 'precisely' then the\n> > >> configure\n> > >> > > call should fail. It should not make any changes or adjustments to the\n> > >> > > configuration.\n> > >> > >\n> > >> > > Validate will tell you if the pipeline handler made any changes, and\n> > >> > > more than that - the validate before configure /must/ report that no\n> > >> > > changes were made.\n> > >> > >\n> > >> >\n> > >> > I think this is the part that may be troublesome. Assume an application\n> > >> > did not call CameraConfiguration::validate() directly, but it does get\n> > >> > called\n> > >> > from PipelineHandler::configure(). The validate() will then adjust the\n> > >> > sensitivity value and return a Status::Adjusted. This will then fail\n> > >> the\n> > >> > PipelineHandler::configure() call. Perhaps\n> > >> CameraConfiguration::validate()\n> > >> > should be a mandatory call from the application? Or if we adjust the\n> > >> > sensitivity in CameraConfiguration::validate(), we don't return\n> > >> > Status::Adjusted?\n> > >> >\n> > >>\n> > >> I'm sorry - I was wrong to say there isn't a requirement to do so.\n> > >>\n> > >>\n> > >> https://www.libcamera.org/api-html/classlibcamera_1_1CameraConfiguration.html#details\n> > >>\n> > >> documents that applications 'shall' be validated by a call to\n> > >> validate().\n> > >>\n> > >\n> > > Ah, that makes things less ambiguous then. I'll let David comment further\n> > > when he is back, but I see no reason not to switch this to validate() now.\n> > >\n> >\n> > After having prototyped an ipa::validate() to pass the sensitivity back to\n> > the\n> > PH, I've changed my mind on the above statement :-)\n> >\n> > The reason for this is that things just feel a bit more awkward in the\n> > validate\n> > call.For instance, we have not yet set up a sensor mode in the IPA to use\n> > for\n> > deducing the mode sensitivity. If we move the mode setup into\n> > ipa::validate(),\n> > there are further things associated with the mode that need to happen there\n> > (e.g.\n> > helper->set_mode())that do not feel right to me.\n> >\n> > Instead of doing things this way, I wonder if we should look to pass the\n> > sensitivity out via the Camera::properties_ ControlList. My initial\n> > reservation\n> > suggesting this was the assumption that properties were static through the\n> > lifetime of the camera.However, this is not the case, as we have\n> > properties::ScalerCropMaximum that changes on every call to configure().\n> > What if\n> > we added a properties::SensorSensitivity that also gets updated in the same\n> > way?\n> > This will remove the need for CameraConfiguration::modeSensitivityfield\n> > entirely.\n>\n> That sounds ok to me too. I wonder if 'ModeSensitivity' is still the\n> right name in that instance ...\n>\n> I think that would mean that it's only readable after the camera is\n> configured though - is that ok?\n\nYes. The use-case is if you want to calculate exposure/gain values for\na new camera mode from values that you obtained from a different mode.\nSo you need to have these values before you call camera.start(). So\nthis is fine.\n\nFor anyone interested, it's exactly this\nhttps://github.com/raspberrypi/picamera2/blob/main/examples/opencv_mertens_merge.py\nthat will go wrong. \"exposure_normal\" needs to be adjusted by the\nratio of the mode sensitivities.\n\nDavid\n\n>\n> --\n> Kieran\n>\n> >\n> > Thanks,\n> > Naush\n> >\n> >\n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > >>\n> > >> \"\"\"\n> > >> The CameraConfiguration holds an ordered list of stream configurations.\n> > >> It supports iterators and operates as a vector of StreamConfiguration\n> > >> instances. The stream configurations are inserted by addConfiguration(),\n> > >> and the at() function or operator[] return a reference to the\n> > >> StreamConfiguration based on its insertion index. Accessing a stream\n> > >> configuration with an invalid index results in undefined behaviour.\n> > >>\n> > >> CameraConfiguration instances are retrieved from the camera with\n> > >> Camera::generateConfiguration(). Applications may then inspect the\n> > >> configuration, modify it, and possibly add new stream configuration\n> > >> entries with addConfiguration(). Once the camera configuration satisfies\n> > >> the application, it shall be validated by a call to validate(). The\n> > >> validation implements \"try\" semantics: it adjusts invalid configurations\n> > >> to the closest achievable parameters instead of rejecting them\n> > >> completely. Applications then decide whether to accept the modified\n> > >> configuration, or try again with a different set of parameters. Once the\n> > >> configuration is valid, it is passed to Camera::configure().\n> > >>\n> > >> \"\"\"\n> > >>\n> > >> --\n> > >> Kieran\n> > >>\n> > >>\n> > >> > Naush\n> > >> >\n> > >> >\n> > >> > >\n> > >> > > When an application calls validate() - the pipeline handler can make\n> > >> > > changes to the Configuration (such as filling in the ModeSensitivity\n> > >> > > here) and if a change was made - it can return\n> > >> 'ConfigurationAdjusted' -\n> > >> > > which then means the application should check to see what was\n> > >> different\n> > >> > > from what it requested - to what the pipeline handler will deliver.\n> > >> > >\n> > >> > > I would like there to be a way to highlight 'what' changes were made\n> > >> if\n> > >> > > the validate returns 'Adjusted' - but I'm not sure how we could easily\n> > >> > > implement that yet.\n> > >> > >\n> > >> > > --\n> > >> > > Kieran\n> > >> > >\n> > >> > >\n> > >> > > > Regards,\n> > >> > > > Naush\n> > >> > > >\n> > >> > > >\n> > >> > > > >\n> > >> > > > > Then the validate phase should be able to return the mode\n> > >> sensitivity\n> > >> > > of\n> > >> > > > > the configuration that is being validated.\n> > >> > > > >\n> > >> > > > > Doing that we could keep the configure calls using a const\n> > >> parameter\n> > >> > > for\n> > >> > > > > their config structures.\n> > >> > > > >\n> > >> > > > > > if (ret < 0) {\n> > >> > > > > > LOG(RPI, Error) << \"IPA configuration failed!\";\n> > >> > > > > > return -EPIPE;\n> > >> > > > > > }\n> > >> > > > > >\n> > >> > > > > > + /* Store the mode sensitivity for the application. */\n> > >> > > > > > + config->modeSensitivity = result.modeSensitivity;\n> > >> > > > > > +\n> > >> > > > > > /*\n> > >> > > > > > * Configure the H/V flip controls based on the\n> > >> combination\n> > >> > > of\n> > >> > > > > > * the sensor and user transform.\n> > >> > > > > > --\n> > >> > > > > > 2.20.1\n> > >> > > > > >\n> > >> > > > >\n> > >> > >\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 6708BC0F1B\n\tfor ;\n\tThu, 21 Apr 2022 14:16:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D218D65642;\n\tThu, 21 Apr 2022 16:16:28 +0200 (CEST)","from mail-wm1-x331.google.com (mail-wm1-x331.google.com\n\t[IPv6:2a00:1450:4864:20::331])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6201C65641\n\tfor ;\n\tThu, 21 Apr 2022 16:16:27 +0200 (CEST)","by mail-wm1-x331.google.com with SMTP id\n\to20-20020a05600c511400b0038ebbbb2ad8so3518990wms.0\n\tfor ;\n\tThu, 21 Apr 2022 07:16:27 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1650550588;\n\tbh=VYxpCILUT0aaC0XGhXsyyxZKl4E541QuKN3JDBROw0s=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=CIw1NGCJ/kBn979YXvBShXYosWKoTaagiBkovGcspCsqsj8Dy2d32h8EM+nemPQJE\n\tvV2sQ+5wy4QWow34HTUbGwMUA1lXlig7ZYLu1C8NZk0YoDT7iJm4/EeeHn5S3Nthsc\n\tjU9o9rNf+PqmbScMpewNJILX2315th1UX3EXKCI2CAij7LlMXF9jUoUq3JiP2+lbPl\n\tVXMRsLPHGG2YTa4LJ0JX5pmei6KPBYJcds/JY+IoKL+yvE3sWevac7Yi9lmc7ZOUGK\n\tDk/QULZAVPFle1EVROTpxnZKTt5nhhYbEtm2+Ws4Ge6EQDHVFgSA2DZKvYHGIy8VlC\n\tG3U6WpZJXN5OQ==","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=GS1iXTjHrGsTzCLkj0gb5OzZddZ3W4OfOyxka1A5v6I=;\n\tb=TWICgVJh1houh50CoA9qLo96pUIyn04xIGTDsOrh+6riHok0DOhWMoXf9L0oVp5LYm\n\tyvQThwXf1eumAxT/5E2oO6FjztKJQwSpB07nF7krxH6foFMUgc+L9mf/QLK7MbsIpH2I\n\txybtkP8PTB4gqKsUC9zNH65ZlvkImubyNrcifhj6Je0KVsn+Ho9lI844LhG2ZUcLrjjj\n\tQ7o42NdX9pVEAZwYnlscEIUMEgP8f9cVK/owrAZeKRKT8GIzacp6yhQzIR7TbW+sxPSy\n\tbBPWtarsJm8tGSP4KaVPcKWmxU0vQ1B2op5qQFBoqeO+Ih5YVx55X1UbOQkqahHCCKZq\n\t49JQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"TWICgVJh\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=GS1iXTjHrGsTzCLkj0gb5OzZddZ3W4OfOyxka1A5v6I=;\n\tb=fcS7FFKoUNYQOLHifXmsbuzC4YpqtdjAtq9rrJ+8DFpPIPd4F7gylv+/TwrkhL0Sa8\n\tSjSHxsa3QGGauOgwUEYpUxaH0NNwFdWRp7EfVbfTxHTS6TYsIyH9BG3JzFspDfWqnz6v\n\tpGmGieegaASShQhwa2gx8Egwpl4DIjZNjUXcSSilbboclk7A4fJqnp45gNhwc4cu1mqb\n\tIRpRZx7IoP0V+A+5j+kyWd91Fo9W4dGdWPtzjAu607Oh8tO/C0qmYusdAmrd9txvl5OY\n\t/WA0wQE997o1tlqya7AeIe/BAbQINsUjP77+Zz06z/hQe3vFiGD1HXVEAWDbsDUUyIVx\n\t28/g==","X-Gm-Message-State":"AOAM532zIjtEn/sm4Wr5kRmODM3CFHeJElqKbzV5gn2+xQcJqRD9QjMK\n\teiYwxLboFGtDIqWU96GlvYSZbPIDhmOV9B9vmRdYw1B6FSr7mqzV","X-Google-Smtp-Source":"ABdhPJy0Xh01oWZuUU1TVq5WZz841xTg8pw6SN04of+q1dlxY+PtQtBz73UFAiPq6OO62um9RNzsbV1AMVm+fhSAAUk=","X-Received":"by 2002:a1c:f207:0:b0:38e:9aac:df41 with SMTP id\n\ts7-20020a1cf207000000b0038e9aacdf41mr8867778wmc.14.1650550586926;\n\tThu, 21 Apr 2022 07:16:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20210922132915.14881-1-david.plowman@raspberrypi.com>\n\t<20210922132915.14881-3-david.plowman@raspberrypi.com>\n\t<164984269099.22830.1157254031970565968@Monstersaurus>\n\t\n\t<164984520193.22830.3479884127443747901@Monstersaurus>\n\t\n\t<164984754726.22830.12000365412223581826@Monstersaurus>\n\t\n\t\n\t<165054949035.3016671.12341680569043361126@Monstersaurus>","In-Reply-To":"<165054949035.3016671.12341680569043361126@Monstersaurus>","Date":"Thu, 21 Apr 2022 15:16:16 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch\n\tcorrect value for sensor's modeSensitivity","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]