<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 8 Jul 2021 at 10:21, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
Thank you for the updated patch!<br>
<br>
On Wed, 7 Jul 2021 at 10:48, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Update the imx477 CamHelper to use long exposure modes if needed.<br>
> This is done by overloading the CamHelper::GetVBlanking function to return a<br>
> frame length (and vblank value) computed using a scaling factor when the value<br>
> would be larger than what the sensor register could otherwise hold.<br>
><br>
> CamHelperImx477::Prepare is also overloaded to ensure that the "device.status"<br>
> metadata returns the right value if the long exposure scaling factor is used.<br>
> The scaling factor is unfortunately not returned back in metadata.<br>
><br>
> With the current imx477 driver, we can achieve a maximum exposure time of approx<br>
> 127 seconds since the HBLANK control is read-only.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/cam_helper_imx477.cpp | 87 +++++++++++++++++++++++<br>
> 1 file changed, 87 insertions(+)<br>
><br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> index 91d05d9226ff..3059a5c6c4d7 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> @@ -6,14 +6,23 @@<br>
> */<br>
><br>
> #include <assert.h><br>
> +#include <cmath><br>
> #include <stddef.h><br>
> #include <stdio.h><br>
> #include <stdlib.h><br>
><br>
> +#include <libcamera/base/log.h><br>
> +<br>
> #include "cam_helper.hpp"<br>
> #include "md_parser.hpp"<br>
><br>
> using namespace RPiController;<br>
> +using namespace libcamera;<br>
> +using libcamera::utils::Duration;<br>
> +<br>
> +namespace libcamera {<br>
> +LOG_DECLARE_CATEGORY(IPARPI)<br>
> +}<br>
><br>
> /*<br>
> * We care about two gain registers and a pair of exposure registers. Their<br>
> @@ -34,6 +43,9 @@ public:<br>
> CamHelperImx477();<br>
> uint32_t GainCode(double gain) const override;<br>
> double Gain(uint32_t gain_code) const override;<br>
> + void Prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) override;<br>
> + uint32_t GetVBlanking(Duration &exposure, Duration minFrameDuration,<br>
> + Duration maxFrameDuration) const override;<br>
> void GetDelays(int &exposure_delay, int &gain_delay,<br>
> int &vblank_delay) const override;<br>
> bool SensorEmbeddedDataPresent() const override;<br>
> @@ -44,6 +56,10 @@ private:<br>
> * in units of lines.<br>
> */<br>
> static constexpr int frameIntegrationDiff = 22;<br>
> + /* Maximum frame length allowable for long exposure calculations. */<br>
> + static constexpr int frameLengthMax = 0xffdc;<br>
> + /* Largest long exposure scale factor given as a left shift on the frame length. */<br>
> + static constexpr int longExposureShiftMax = 7;<br>
><br>
> void PopulateMetadata(const MdParser::RegisterMap ®isters,<br>
> Metadata &metadata) const override;<br>
> @@ -64,6 +80,77 @@ double CamHelperImx477::Gain(uint32_t gain_code) const<br>
> return 1024.0 / (1024 - gain_code);<br>
> }<br>
><br>
> +void CamHelperImx477::Prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata)<br>
> +{<br>
> + MdParser::RegisterMap registers;<br>
> + DeviceStatus deviceStatus, parsedDeviceStatus;<br>
> +<br>
> + if (metadata.Get("device.status", deviceStatus)) {<br>
> + LOG(IPARPI, Error) << "DeviceStatus not found from DelayedControls";<br>
> + return;<br>
> + }<br>
> +<br>
> + parseEmbeddedData(buffer, metadata);<br>
> +<br>
> + if (metadata.Get("device.status", parsedDeviceStatus)) {<br>
> + LOG(IPARPI, Error) << "DeviceStatus not found after parsing";<br>
> + return;<br>
> + }<br>
<br>
Just one little question here: if we found the DeviceStatus in the<br>
metadata before parsing, isn't it guaranteed to be there even if<br>
parsing fails? So what would happen is the parsedDeviceStatus would be<br>
the same as the deviceStatus.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
I don't know if it's worth actually changing anything, though handling<br>
that second "not found" error seems unnecessary. Maybe we should get<br>
the return code and assert if it's wrong? But I agree that what you<br>
have will work fine, so I don't mind either way.<br></blockquote><div><br></div><div>You are right. The second failure test will never hit, and I can get rid of</div><div>this test. Will do so for the next revision.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
(Maybe this kind of thing would be easier to write if<br>
parseEmbeddedData returned whether it succeeded or not... but I don't<br>
really want to propose going there right now!)<br>
<br>
> +<br>
> + /*<br>
> + * The DeviceStatus struct is first populated with values obtained from<br>
> + * DelayedControls. If this reports frame length is > frameLengthMax,<br>
> + * it means we are using a long exposure mode. Since the long exposure<br>
> + * scale factor is not returned back through embedded data, we must rely<br>
> + * on the existing exposure lines and frame length values returned by<br>
> + * DelayedControls.<br>
> + *<br>
> + * Otherwise, all values are updated with what is reported in the<br>
> + * embedded data.<br>
> + */<br>
> + if (deviceStatus.frame_length > frameLengthMax) {<br>
> + parsedDeviceStatus.shutter_speed = deviceStatus.shutter_speed;<br>
> + parsedDeviceStatus.frame_length = deviceStatus.frame_length;<br>
> + metadata.Set("device.status", parsedDeviceStatus);<br>
> + LOG(IPARPI, Debug) << "Metadata updated for long exposure: "<br>
> + << parsedDeviceStatus;<br>
> + }<br>
> +}<br>
<br>
Yes, I think I like this version. It seems easy to understand and we<br>
don't duplicate all that code from before. It's definitely a better<br>
model to follow if folks come up against other similar sensors.<br>
<br>
Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Thanks!<br>
David<br>
<br>
> +<br>
> +uint32_t CamHelperImx477::GetVBlanking(Duration &exposure,<br>
> + Duration minFrameDuration,<br>
> + Duration maxFrameDuration) const<br>
> +{<br>
> + uint32_t frameLength, exposureLines;<br>
> + unsigned int shift = 0;<br>
> +<br>
> + frameLength = mode_.height + CamHelper::GetVBlanking(exposure, minFrameDuration,<br>
> + maxFrameDuration);<br>
> + /*<br>
> + * Check if the frame length calculated needs to be setup for long<br>
> + * exposure mode. This will require us to use a long exposure scale<br>
> + * factor provided by a shift operation in the sensor.<br>
> + */<br>
> + while (frameLength > frameLengthMax) {<br>
> + if (++shift > longExposureShiftMax) {<br>
> + shift = longExposureShiftMax;<br>
> + frameLength = frameLengthMax;<br>
> + break;<br>
> + }<br>
> + frameLength >>= 1;<br>
> + }<br>
> +<br>
> + if (shift) {<br>
> + /* Account for any rounding in the scaled frame length value. */<br>
> + frameLength <<= shift;<br>
> + exposureLines = ExposureLines(exposure);<br>
> + exposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);<br>
> + exposure = Exposure(exposureLines);<br>
> + }<br>
> +<br>
> + return frameLength - mode_.height;<br>
> +}<br>
> +<br>
> void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,<br>
> int &vblank_delay) const<br>
> {<br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>