[libcamera-devel] [PATCH v2 1/4] ipa: raspberrypi: Allow long exposure modes for imx477.
Naushir Patuck
naush at raspberrypi.com
Fri Jul 2 14:58:51 CEST 2021
Hi again,
On Fri, 2 Jul 2021 at 08:45, Naushir Patuck <naush at raspberrypi.com> wrote:
> Hi Laurent,
>
> Thank you for your review feedback.
>
> On Fri, 2 Jul 2021 at 01:14, Laurent Pinchart <
> laurent.pinchart at ideasonboard.com> wrote:
>
>> Hi Naush,
>>
>> Thank you for the patch.
>>
>> On Thu, Jul 01, 2021 at 12:34:39PM +0100, Naushir Patuck wrote:
>> > Update the imx477 CamHelper to use long exposure modes if needed.
>> > This is done by overloading the CamHelper::GetVBlanking function to
>> return a
>> > frame length (and vblank value) computed using a scaling factor when
>> the value
>> > would be larger than what the sensor register could otherwise hold.
>> >
>> > CamHelperImx477::Prepare is also overloaded to ensure that the
>> "device.status"
>> > metadata returns the right value if the long exposure scaling factor is
>> used.
>> > The scaling factor is unfortunately not returned back in metadata.
>>
>> That's not nice :-(
>>
>> > With the current imx477 driver, we can achieve a maximum exposure time
>> of approx
>> > 127 seconds since the HBLANK control is read-only.
>> >
>> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
>> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>> > ---
>> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 69 +++++++++++++++++++++++
>> > 1 file changed, 69 insertions(+)
>> >
>> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp
>> b/src/ipa/raspberrypi/cam_helper_imx477.cpp
>> > index 4098fde6f322..139cc7dbd39a 100644
>> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
>> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
>> > @@ -6,6 +6,7 @@
>> > */
>> >
>> > #include <assert.h>
>> > +#include <cmath>
>> > #include <stddef.h>
>> > #include <stdio.h>
>> > #include <stdlib.h>
>> > @@ -14,6 +15,7 @@
>> > #include "md_parser.hpp"
>> >
>> > using namespace RPiController;
>> > +using libcamera::utils::Duration;
>> >
>> > /*
>> > * We care about two gain registers and a pair of exposure registers.
>> Their
>> > @@ -31,6 +33,9 @@ public:
>> > CamHelperImx477();
>> > uint32_t GainCode(double gain) const override;
>> > double Gain(uint32_t gain_code) const override;
>> > + void Prepare(libcamera::Span<const uint8_t> buffer, Metadata
>> &metadata) override;
>> > + uint32_t GetVBlanking(Duration &exposure, Duration
>> minFrameDuration,
>> > + Duration maxFrameDuration) const override;
>> > void GetDelays(int &exposure_delay, int &gain_delay,
>> > int &vblank_delay) const override;
>> > bool SensorEmbeddedDataPresent() const override;
>> > @@ -41,6 +46,10 @@ private:
>> > * in units of lines.
>> > */
>> > static constexpr int frameIntegrationDiff = 22;
>> > + /* Maximum frame length allowable for long exposure calculations.
>> */
>> > + static constexpr int frameLengthMax = 0xffdc;
>> > + /* Largest long exposure scale factor given as a left shift on
>> the frame length. */
>> > + static constexpr int longExposureShiftMax = 7;
>> >
>> > void PopulateMetadata(const MdParser::RegisterMap ®isters,
>> > Metadata &metadata) const override;
>> > @@ -61,6 +70,66 @@ double CamHelperImx477::Gain(uint32_t gain_code)
>> const
>> > return 1024.0 / (1024 - gain_code);
>> > }
>> >
>> > +void CamHelperImx477::Prepare(libcamera::Span<const uint8_t> buffer,
>> Metadata &metadata)
>> > +{
>> > + DeviceStatus deviceStatus, parsedStatus;
>> > +
>> > + /* Get the device status provided by DelayedControls */
>> > + if (metadata.Get("device.status", deviceStatus) != 0)
>> > + return;
>> > +
>> > + /* Get the device status provided by the embedded data buffer. */
>> > + CamHelper::parseEmbeddedData(buffer, metadata);
>> > + metadata.Get("device.status", parsedStatus);
>> > +
>> > + /*
>> > + * If the ratio of DelayedControls to embedded data shutter speed
>> is > 1
>> > + * and is a factor of 2^N, then we can assume this is a long
>> exposure mode
>> > + * frame. Since embedded data does not provide any hints of long
>> exposure
>> > + * modes, make sure we use the DelayedControls values in the
>> metadata.
>> > + * Otherwise, just go with the embedded data values.
>> > + */
>> > + unsigned long ratio = std::lround(deviceStatus.shutter_speed /
>> parsedStatus.shutter_speed);
>> > + bool replace = (ratio > 1) && ((ratio & (~ratio + 1)) == ratio);
>>
>> Couldn't this be simplified by just checking if the shutter speed
>> provided by the delayed controls is larger than the long exposure mode
>> threshold ?
>>
>
This comment got me thinking that the logic above is not strictly correct.
There may
be a case when a user asks for a low exposure with a very large frame
length, e.g.
33ms shutter speed with a framerate of 1/10 fps (don't ask me why). In
these cases,
the condition above would return the shutter speed that has been scaled,
and no way
of un-scaling it.
Instead, what I need to do is do the test above on frame length rather than
exposure,
as long exposure implies long frame length mode, and not necessarily the
other way
round. So I have to add a frame length field to DeviceStatus and replace
shutter_speed
with frame_length in the above calculations.
Naush
>
> Yes, perhaps I was being overly defensive with the 2^N test.
>
>
>> > +
>> > + if (replace)
>> > + metadata.Set("device.status", deviceStatus);
>>
>> Is there any risk in replacing the whole status instead of only the
>> shutter speed ?
>>
>
> Strictly speaking, yes, I should really only touch the shutter speed field.
>
> I will probably refactor this function a bit to reduce the number of
> metadata
> set/get operations for the next revision.
>
> Thanks,
> Naush
>
>
>>
>> > +}
>> > +
>> > +uint32_t CamHelperImx477::GetVBlanking(Duration &exposure,
>> > + Duration minFrameDuration,
>> > + Duration maxFrameDuration) const
>> > +{
>> > + uint32_t frameLength, exposureLines;
>> > + unsigned int shift = 0;
>> > +
>> > + frameLength = mode_.height + CamHelper::GetVBlanking(exposure,
>> minFrameDuration,
>> > +
>> maxFrameDuration);
>> > + /*
>> > + * Check if the frame length calculated needs to be setup for long
>> > + * exposure mode. This will require us to use a long exposure
>> scale
>> > + * factor provided by a shift operation in the sensor.
>> > + */
>> > + while (frameLength > frameLengthMax) {
>> > + if (++shift > longExposureShiftMax) {
>> > + shift = longExposureShiftMax;
>> > + frameLength = frameLengthMax;
>> > + break;
>> > + }
>> > + frameLength >>= 1;
>> > + }
>>
>> Do I understand correctly that the driver will perform the same
>> computation ? Is the exposure time handled similarly, or is it only the
>> vblank ?
>>
>
> Yes, the sensor driver has pretty much this exact loop to compute the
> long exposure factor to program in. The exposure time is handled in
> the same way as line length.
>
> Regards,
> Naush
>
>
>>
>> > +
>> > + if (shift) {
>> > + /* Account for any rounding in the scaled frame length
>> value. */
>> > + frameLength <<= shift;
>> > + exposureLines = CamHelper::ExposureLines(exposure);
>> > + exposureLines = std::min(exposureLines, frameLength -
>> frameIntegrationDiff);
>> > + exposure = CamHelper::Exposure(exposureLines);
>> > + }
>> > +
>> > + return frameLength - mode_.height;
>> > +}
>> > +
>> > void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,
>> > int &vblank_delay) const
>> > {
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210702/6da5b60b/attachment-0001.htm>
More information about the libcamera-devel
mailing list