[libcamera-devel] [PATCH v4 5/8] ipa: raspberrypi: Allow long exposure modes for imx477.
Naushir Patuck
naush at raspberrypi.com
Thu Jul 8 11:28:32 CEST 2021
Hi David,
Thank you for your review feedback.
On Thu, 8 Jul 2021 at 10:21, David Plowman <david.plowman at raspberrypi.com>
wrote:
> Hi Naush
>
> Thank you for the updated patch!
>
> On Wed, 7 Jul 2021 at 10:48, Naushir Patuck <naush at raspberrypi.com> 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.
> >
> > 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>
> > ---
> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 87 +++++++++++++++++++++++
> > 1 file changed, 87 insertions(+)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > index 91d05d9226ff..3059a5c6c4d7 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > @@ -6,14 +6,23 @@
> > */
> >
> > #include <assert.h>
> > +#include <cmath>
> > #include <stddef.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> >
> > +#include <libcamera/base/log.h>
> > +
> > #include "cam_helper.hpp"
> > #include "md_parser.hpp"
> >
> > using namespace RPiController;
> > +using namespace libcamera;
> > +using libcamera::utils::Duration;
> > +
> > +namespace libcamera {
> > +LOG_DECLARE_CATEGORY(IPARPI)
> > +}
> >
> > /*
> > * We care about two gain registers and a pair of exposure registers.
> Their
> > @@ -34,6 +43,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;
> > @@ -44,6 +56,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;
> > @@ -64,6 +80,77 @@ 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)
> > +{
> > + MdParser::RegisterMap registers;
> > + DeviceStatus deviceStatus, parsedDeviceStatus;
> > +
> > + if (metadata.Get("device.status", deviceStatus)) {
> > + LOG(IPARPI, Error) << "DeviceStatus not found from
> DelayedControls";
> > + return;
> > + }
> > +
> > + parseEmbeddedData(buffer, metadata);
> > +
> > + if (metadata.Get("device.status", parsedDeviceStatus)) {
> > + LOG(IPARPI, Error) << "DeviceStatus not found after
> parsing";
> > + return;
> > + }
>
> Just one little question here: if we found the DeviceStatus in the
> metadata before parsing, isn't it guaranteed to be there even if
> parsing fails? So what would happen is the parsedDeviceStatus would be
> the same as the deviceStatus.
>
> I don't know if it's worth actually changing anything, though handling
> that second "not found" error seems unnecessary. Maybe we should get
> the return code and assert if it's wrong? But I agree that what you
> have will work fine, so I don't mind either way.
>
You are right. The second failure test will never hit, and I can get rid of
this test. Will do so for the next revision.
Regards,
Naush
>
> (Maybe this kind of thing would be easier to write if
> parseEmbeddedData returned whether it succeeded or not... but I don't
> really want to propose going there right now!)
>
> > +
> > + /*
> > + * The DeviceStatus struct is first populated with values
> obtained from
> > + * DelayedControls. If this reports frame length is >
> frameLengthMax,
> > + * it means we are using a long exposure mode. Since the long
> exposure
> > + * scale factor is not returned back through embedded data, we
> must rely
> > + * on the existing exposure lines and frame length values
> returned by
> > + * DelayedControls.
> > + *
> > + * Otherwise, all values are updated with what is reported in the
> > + * embedded data.
> > + */
> > + if (deviceStatus.frame_length > frameLengthMax) {
> > + parsedDeviceStatus.shutter_speed =
> deviceStatus.shutter_speed;
> > + parsedDeviceStatus.frame_length =
> deviceStatus.frame_length;
> > + metadata.Set("device.status", parsedDeviceStatus);
> > + LOG(IPARPI, Debug) << "Metadata updated for long
> exposure: "
> > + << parsedDeviceStatus;
> > + }
> > +}
>
> Yes, I think I like this version. It seems easy to understand and we
> don't duplicate all that code from before. It's definitely a better
> model to follow if folks come up against other similar sensors.
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks!
> David
>
> > +
> > +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;
> > + }
> > +
> > + if (shift) {
> > + /* Account for any rounding in the scaled frame length
> value. */
> > + frameLength <<= shift;
> > + exposureLines = ExposureLines(exposure);
> > + exposureLines = std::min(exposureLines, frameLength -
> frameIntegrationDiff);
> > + exposure = Exposure(exposureLines);
> > + }
> > +
> > + return frameLength - mode_.height;
> > +}
> > +
> > void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,
> > int &vblank_delay) const
> > {
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210708/afcebbcb/attachment-0001.htm>
More information about the libcamera-devel
mailing list