[{"id":5316,"web_url":"https://patchwork.libcamera.org/comment/5316/","msgid":"<20200622040401.GJ25355@pendragon.ideasonboard.com>","date":"2020-06-22T04:04:01","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi:\n\trecalculate camera exposure/gain when camera mode changes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nNitpicking a little bit, the commit message subject prefix should be\n\"libcamera: ipa: raspberrypi:\". Same for 1/2.\n\nOn Thu, Jun 18, 2020 at 12:12:36PM +0100, David Plowman wrote:\n> This commit causes the AGC to recalculate its camera exposure/gain\n> values when the camera mode changes. For example it's possible\n> that the exposure profile could be changed by the application so\n> the division between exposure time and analogue gain may be\n> different.\n> \n> The other underlying reason (and which this commit accomplishes too)\n> is that the sensor's line timing may change in a new mode, and because\n> V4L2 drivers store a number of exposure _lines_, the resulting _time_\n> will \"change under our feet\". So we have to go through the process of\n> recalculating the correct number of lines and writing this back to the\n> sensor with every mode switch, regardless of anything else.\n\nI wonder if in the future we shouldn't try to abstract this in the\nCameraSensor class, to avoid having to deal with this sensor-specific\nbehaviour in IPAs. Not something for this patch series.\n\n> Signed-off-by: David Plowman \n\nReviewed-by: Laurent Pinchart \n\nBoth patches applied to my tree with the subjects updated. I'll push to\nmaster after running the compilation tests.\n\n> ---\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 +++++++++++\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 25 +++++++++++-----------\n> 3 files changed, 26 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index a474287..c02b5ec 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)\n> \tconstraint_mode_name_ = constraint_mode_name;\n> }\n> \n> +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> +{\n> +\t// On a mode switch, it's possible the exposure profile could change,\n> +\t// so we run through the dividing up of exposure/gain again and\n> +\t// write the results into the metadata we've been given.\n> +\tif (status_.total_exposure_value) {\n> +\t\thousekeepConfig();\n> +\t\tdivvyupExposure();\n> +\t\twriteAndFinish(metadata, false);\n> +\t}\n> +}\n> +\n> void Agc::Prepare(Metadata *image_metadata)\n> {\n> \tAgcStatus status;\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index dbcefba..9a7e89c 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> @@ -75,6 +75,7 @@ public:\n> \tvoid SetMeteringMode(std::string const &metering_mode_name) override;\n> \tvoid SetExposureMode(std::string const &exposure_mode_name) override;\n> \tvoid SetConstraintMode(std::string const &contraint_mode_name) override;\n> +\tvoid SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> \tvoid Prepare(Metadata *image_metadata) override;\n> \tvoid Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index d6fd3df..42c84b1 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -247,29 +247,30 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tmistrust_count_ = helper_->MistrustFramesStartup();\n> \t}\n> \n> +\tstruct AgcStatus agcStatus;\n> +\t/* These zero values mean not program anything (unless overwritten). */\n> +\tagcStatus.shutter_time = 0.0;\n> +\tagcStatus.analogue_gain = 0.0;\n> +\n> \tif (!controllerInit_) {\n> \t\t/* Load the tuning file for this sensor. */\n> \t\tcontroller_.Read(tuningFile_.c_str());\n> \t\tcontroller_.Initialise();\n> \t\tcontrollerInit_ = true;\n> \n> -\t\t/* Calculate initial values for gain and exposure. */\n> -\t\tint32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN);\n> -\t\tint32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME);\n> -\n> -\t\tControlList ctrls(unicam_ctrls_);\n> -\t\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> -\t\tctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n> -\n> -\t\tIPAOperationData op;\n> -\t\top.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;\n> -\t\top.controls.push_back(ctrls);\n> -\t\tqueueFrameAction.emit(0, op);\n> +\t\t/* Supply initial values for gain and exposure. */\n> +\t\tagcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;\n> +\t\tagcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n> \t}\n> \n> \tRPi::Metadata metadata;\n> \tcontroller_.SwitchMode(mode_, &metadata);\n> \n> +\t/* SwitchMode may supply updated exposure/gain values to use. */\n> +\tmetadata.Get(\"agc.status\", agcStatus);\n> +\tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)\n> +\t\tapplyAGC(&agcStatus);\n> +\n> \tlastMode_ = mode_;\n> }\n>","headers":{"Return-Path":"","Received":["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 D2420609A3\n\tfor ;\n\tMon, 22 Jun 2020 06:04:28 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1A2C230D;\n\tMon, 22 Jun 2020 06:04:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"CEEM4B54\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592798666;\n\tbh=M/lR6SCLHagoqR0jR4fKzzpqIMKBYvQedXvelhtGQ6Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CEEM4B54UI9b69OumbKfT2Rkl2h0asZegyodOcED3ShaeKpq84YLILDCn0Mt0cJ5e\n\tEJ8PcK5KpUM1z+HqJuvw6DrMz7dgmF9nYoQ4btynPcOKrl7PuoM1f92BG8jhKM9xLz\n\tAFjDwwqIg1emVQ5QdZKmCJOPkHVJY1NhgVQ4bfjc=","Date":"Mon, 22 Jun 2020 07:04:01 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200622040401.GJ25355@pendragon.ideasonboard.com>","References":"<20200618111236.26897-1-david.plowman@raspberrypi.com>\n\t<20200618111236.26897-3-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200618111236.26897-3-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi:\n\trecalculate camera exposure/gain when camera mode changes","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","X-List-Received-Date":"Mon, 22 Jun 2020 04:04:29 -0000"}},{"id":5320,"web_url":"https://patchwork.libcamera.org/comment/5320/","msgid":"","date":"2020-06-22T08:09:56","subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi:\n\trecalculate camera exposure/gain when camera mode changes","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nOn Mon, 22 Jun 2020 at 05:04, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> Nitpicking a little bit, the commit message subject prefix should be\n> \"libcamera: ipa: raspberrypi:\". Same for 1/2.\n\nNoted for next time! :)\n\n>\n> On Thu, Jun 18, 2020 at 12:12:36PM +0100, David Plowman wrote:\n> > This commit causes the AGC to recalculate its camera exposure/gain\n> > values when the camera mode changes. For example it's possible\n> > that the exposure profile could be changed by the application so\n> > the division between exposure time and analogue gain may be\n> > different.\n> >\n> > The other underlying reason (and which this commit accomplishes too)\n> > is that the sensor's line timing may change in a new mode, and because\n> > V4L2 drivers store a number of exposure _lines_, the resulting _time_\n> > will \"change under our feet\". So we have to go through the process of\n> > recalculating the correct number of lines and writing this back to the\n> > sensor with every mode switch, regardless of anything else.\n>\n> I wonder if in the future we shouldn't try to abstract this in the\n> CameraSensor class, to avoid having to deal with this sensor-specific\n> behaviour in IPAs. Not something for this patch series.\n\nAgree. I think the current behaviour is probably unhelpful for many\npipeline handlers.\n\nBest regards\nDavid\n\n>\n> > Signed-off-by: David Plowman \n>\n> Reviewed-by: Laurent Pinchart \n>\n> Both patches applied to my tree with the subjects updated. I'll push to\n> master after running the compilation tests.\n>\n> > ---\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 +++++++++++\n> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 1 +\n> > src/ipa/raspberrypi/raspberrypi.cpp | 25 +++++++++++-----------\n> > 3 files changed, 26 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index a474287..c02b5ec 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)\n> > constraint_mode_name_ = constraint_mode_name;\n> > }\n> >\n> > +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> > +{\n> > + // On a mode switch, it's possible the exposure profile could change,\n> > + // so we run through the dividing up of exposure/gain again and\n> > + // write the results into the metadata we've been given.\n> > + if (status_.total_exposure_value) {\n> > + housekeepConfig();\n> > + divvyupExposure();\n> > + writeAndFinish(metadata, false);\n> > + }\n> > +}\n> > +\n> > void Agc::Prepare(Metadata *image_metadata)\n> > {\n> > AgcStatus status;\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > index dbcefba..9a7e89c 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > @@ -75,6 +75,7 @@ public:\n> > void SetMeteringMode(std::string const &metering_mode_name) override;\n> > void SetExposureMode(std::string const &exposure_mode_name) override;\n> > void SetConstraintMode(std::string const &contraint_mode_name) override;\n> > + void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> > void Prepare(Metadata *image_metadata) override;\n> > void Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index d6fd3df..42c84b1 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -247,29 +247,30 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > mistrust_count_ = helper_->MistrustFramesStartup();\n> > }\n> >\n> > + struct AgcStatus agcStatus;\n> > + /* These zero values mean not program anything (unless overwritten). */\n> > + agcStatus.shutter_time = 0.0;\n> > + agcStatus.analogue_gain = 0.0;\n> > +\n> > if (!controllerInit_) {\n> > /* Load the tuning file for this sensor. */\n> > controller_.Read(tuningFile_.c_str());\n> > controller_.Initialise();\n> > controllerInit_ = true;\n> >\n> > - /* Calculate initial values for gain and exposure. */\n> > - int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN);\n> > - int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME);\n> > -\n> > - ControlList ctrls(unicam_ctrls_);\n> > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> > - ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n> > -\n> > - IPAOperationData op;\n> > - op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;\n> > - op.controls.push_back(ctrls);\n> > - queueFrameAction.emit(0, op);\n> > + /* Supply initial values for gain and exposure. */\n> > + agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;\n> > + agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n> > }\n> >\n> > RPi::Metadata metadata;\n> > controller_.SwitchMode(mode_, &metadata);\n> >\n> > + /* SwitchMode may supply updated exposure/gain values to use. */\n> > + metadata.Get(\"agc.status\", agcStatus);\n> > + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)\n> > + applyAGC(&agcStatus);\n> > +\n> > lastMode_ = mode_;\n> > }\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-oi1-x242.google.com (mail-oi1-x242.google.com\n\t[IPv6:2607:f8b0:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 26084603BB\n\tfor ;\n\tMon, 22 Jun 2020 10:10:08 +0200 (CEST)","by mail-oi1-x242.google.com with SMTP id a21so14845565oic.8\n\tfor ;\n\tMon, 22 Jun 2020 01:10:08 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"deYHWPy/\"; 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=5g5q3/xAnRfqybBJgYA4UvkK2F/qXenOWSlAT4l4b9Y=;\n\tb=deYHWPy/bXMxU2HX4IKmIfE9k9NVJwQOXzFC8WsGWICQLqUt4VPnyK8LuFhCG2wINZ\n\tbdnUrJLyGmQKyphZ1vWy9mcmo8kbHm2x/wfPIzaQ7GszqpJdDhqUpIHauP4OPN2WZxAd\n\t2qsRhnIi4bztDJIs8SqqQpblsB5bSjv6MnTB7LBpf1sNAjjhRL9/dS7eqZWFac0rYt4Z\n\t5eMK4A8Bc5fPCt3axGlXS5kdZAHoalOULclxLK0Pl1KJiG5U+Ua+6yPjzikXkG7LliVs\n\tWp6V0VUNxqlzbNrkrWDBkYg3XMKuvCssSY5mQhDP/sv8QF2Uzv21W7ASydnUKVZIMDGn\n\tujdg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=5g5q3/xAnRfqybBJgYA4UvkK2F/qXenOWSlAT4l4b9Y=;\n\tb=YKvNzL77FSwklju1vkqy7RD5gM/GKQ4G8ugyH08RDlgoT3Jys5DIHuoJiMWJhVvTDr\n\tIIG1Bo0vqo7xb9ZWa++zxEUCDgT98kR/ABkzJ+7vQS0w3RUsdyTxlTo0zG3wMgiJ0pmV\n\tSgv39i1G71djsLb2NODMvfFroLUdZZRzKQvANdbBd02opso/v/fnUrSZ0hwRrGgWsV1q\n\t29O40LJN+sIkWdYEfUazz66mL3F3w7Nc8gXGiWgJPLg1OqT7oC3uEz4cRR+qfjRvMRYw\n\tlCyOoNKGD4IPYasESnqpt5xz2bUmE9F05kguDdIYqP940dVTLOPKPMogvX9DVvQx0vbt\n\tET+g==","X-Gm-Message-State":"AOAM532B2Ovxks6eb3tIWDiNk5QpwdX7IR3XL7XWonYBqW3kr7+PmPBk\n\ttT5hjfVLr+gEPCw2k7jg7pSynznv7PdlsFnkB5op3PkV","X-Google-Smtp-Source":"ABdhPJyzufhHmvJ/+K+WbItUra8aYWHWabyTxXRGQU1rilELDLQq/8Dqfn7N+4VU+yIbFuPa8I3g/BL/84cPVmYO6uw=","X-Received":"by 2002:a05:6808:149:: with SMTP id\n\th9mr12149601oie.107.1592813406796; \n\tMon, 22 Jun 2020 01:10:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20200618111236.26897-1-david.plowman@raspberrypi.com>\n\t<20200618111236.26897-3-david.plowman@raspberrypi.com>\n\t<20200622040401.GJ25355@pendragon.ideasonboard.com>","In-Reply-To":"<20200622040401.GJ25355@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Mon, 22 Jun 2020 09:09:56 +0100","Message-ID":"","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi:\n\trecalculate camera exposure/gain when camera mode changes","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","X-List-Received-Date":"Mon, 22 Jun 2020 08:10:08 -0000"}}]