<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 15 Jun 2021 at 04:19, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">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 Mon, Jun 14, 2021 at 11:00:37AM +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 use.<br>
<br>
s/use/used/<br>
<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 | 67 +++++++++++++++++++++++<br>
> 1 file changed, 67 insertions(+)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> index 7a1100c25afc..549c2620765a 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>
> @@ -30,6 +32,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>
> @@ -40,6 +45,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>
> MdParserSmia imx477_parser;<br>
> <br>
> @@ -72,6 +81,64 @@ 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>
> + if (replace)<br>
> + metadata.Set("device.status", deviceStatus);<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>
> + /* 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>
I'll try to review this series in more details shortly, but I'd like to<br>
already take this as an opportunity to discuss an idea related to the<br>
cam helpers.<br>
<br>
As they stand today, the cam helpers translate between natural units<br>
(time for exposure, and a linear multipler for gain) and V4L2 control<br>
values. This pushes knowledge of V4L2 to the IPA, which isn't great.<br>
I've been toying with the idea of gathering all the V4L2 knowledge in<br>
the libcamera core, which would move these conversions to the pipeline<br>
handler (with the helper of camera helpers on the libcamera side of<br>
course).<br>
<br>
One issue that bothered me is that the IPA wouldn't know about rounding<br>
(or at least not right away, it could get information back from the<br>
pipeline handler, but with a delay, and in any case that wouldn't be<br>
very clean). The code above takes rounding into account. I'd thus like<br>
your feedback on how important this is, and, in case (as I suspect) the<br>
IPA needs to know about rounding, whether you think there would still be<br>
a way to not add V4L2 knowledge to the IPA.<br></blockquote><div><br></div><div>This is purely from the perspective of the Raspberry Pi IPA, so other vendors</div><div>may have different opinions or requirements.</div><div><br></div><div>For the RPi IPA, the key is that we get given the *actual* shutter speed and gain</div><div>values used by the sensor for that frame, and these may or may not be rounded</div><div>from what the AGC originally requested. When fetching shutter speed and gain</div><div>values from embedded data, this is implicit. When no embedded data is available,</div><div>the CamHelper does the rounding before passing the values into DelayedControls,</div><div>and the value given back to the IPA when the frame comes through (with appropriate</div><div>frame delay) will also be rounded. So, if this CamHelper functionality were to be</div><div>moved to the pipeline handler domain mostly as-is, we would see no ill effect. If the</div><div>IPA were to somehow get shutter speed and gain values that were not rounded</div><div>appropriately, you would likely see some tiny oscillations in the AGC, and David</div><div>would not be happy with that :-)</div><div></div><div><br></div><div>So in summary, I don't think the rounding will give us any problems, other IPAs</div><div>might have to take a similar approach to ours to claim the same. I do think removing</div><div> these sorts of conversions from the IPA domain is the right thing to do. Having the</div><div> IPA deal with only natural units will be a welcome change!</div><div><br></div><div>Regards,</div><div>Naush</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>
> + 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>