<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 2 Jul 2021 at 01:14, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.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 patch.<br>
<br>
On Thu, Jul 01, 2021 at 12:34:39PM +0100, Naushir Patuck wrote:<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>
That's not nice :-(<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>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/cam_helper_imx477.cpp | 69 +++++++++++++++++++++++<br>
> 1 file changed, 69 insertions(+)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> index 4098fde6f322..139cc7dbd39a 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> @@ -6,6 +6,7 @@<br>
> */<br>
> <br>
> #include <assert.h><br>
> +#include <cmath><br>
> #include <stddef.h><br>
> #include <stdio.h><br>
> #include <stdlib.h><br>
> @@ -14,6 +15,7 @@<br>
> #include "md_parser.hpp"<br>
> <br>
> using namespace RPiController;<br>
> +using libcamera::utils::Duration;<br>
> <br>
> /*<br>
> * We care about two gain registers and a pair of exposure registers. Their<br>
> @@ -31,6 +33,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>
> @@ -41,6 +46,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>
> @@ -61,6 +70,66 @@ 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>
> + DeviceStatus deviceStatus, parsedStatus;<br>
> +<br>
> + /* Get the device status provided by DelayedControls */<br>
> + if (metadata.Get("device.status", deviceStatus) != 0)<br>
> + return;<br>
> +<br>
> + /* Get the device status provided by the embedded data buffer. */<br>
> + CamHelper::parseEmbeddedData(buffer, metadata);<br>
> + metadata.Get("device.status", parsedStatus);<br>
> +<br>
> + /*<br>
> + * If the ratio of DelayedControls to embedded data shutter speed is > 1<br>
> + * and is a factor of 2^N, then we can assume this is a long exposure mode<br>
> + * frame. Since embedded data does not provide any hints of long exposure<br>
> + * modes, make sure we use the DelayedControls values in the metadata.<br>
> + * Otherwise, just go with the embedded data values.<br>
> + */<br>
> + unsigned long ratio = std::lround(deviceStatus.shutter_speed / parsedStatus.shutter_speed);<br>
> + bool replace = (ratio > 1) && ((ratio & (~ratio + 1)) == ratio);<br>
<br>
Couldn't this be simplified by just checking if the shutter speed<br>
provided by the delayed controls is larger than the long exposure mode<br>
threshold ?<br></blockquote><div><br></div><div>Yes, perhaps I was being overly defensive with the 2^N test.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> + if (replace)<br>
> + metadata.Set("device.status", deviceStatus);<br>
<br>
Is there any risk in replacing the whole status instead of only the<br>
shutter speed ?<br></blockquote><div><br></div><div>Strictly speaking, yes, I should really only touch the shutter speed field.</div><div><br>I will probably refactor this function a bit to reduce the number of metadata</div><div>set/get operations for the next revision.</div><div><br></div><div>Thanks,</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>
> +}<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>
Do I understand correctly that the driver will perform the same<br>
computation ? Is the exposure time handled similarly, or is it only the<br>
vblank ?<br></blockquote><div><br></div><div>Yes, the sensor driver has pretty much this exact loop to compute the</div><div>long exposure factor to program in. The exposure time is handled in</div><div>the same way as line length.</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>
> +<br>
> + if (shift) {<br>
> + /* Account for any rounding in the scaled frame length value. */<br>
> + frameLength <<= shift;<br>
> + exposureLines = CamHelper::ExposureLines(exposure);<br>
> + exposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);<br>
> + exposure = CamHelper::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>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>