[libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Make CamHelpers return the frame delay for vblanking
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 8 23:04:27 CET 2021
Hi David,
On Mon, Mar 08, 2021 at 10:01:37PM +0000, David Plowman wrote:
> On Mon, 8 Mar 2021 at 19:19, Laurent Pinchart wrote:
> > On Thu, Mar 04, 2021 at 03:31:19PM +0000, David Plowman wrote:
> > > For some sensors (e.g. imx477) we need to update the vblanking on the
> > > frame before the exposure.
> >
> > That is really peculiar, and quite annoying. I'm OK with this change as
> > a temporary fix, do you think we could get some support from Sony to
> > figure out what's happening ?
>
> Yes, we shall come back and take another look at this. We've been
> round the loop of putting in extra debug, adding careful manipulation
> of the group hold register, but without getting it to work. But once
> we've got the DelayedControls fixed so that all our sensors actually
> work as they're meant to, then I'm sure we'll return to this and
> manage to figure it out.
>
> > > For this reason the GetDelays method must
> > > also return the number of frame delays for the vblanking control.
> > >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > > src/ipa/raspberrypi/cam_helper.cpp | 4 +++-
> > > src/ipa/raspberrypi/cam_helper.hpp | 9 ++++++---
> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 7 +++++--
> > > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 7 +++++--
> > > src/ipa/raspberrypi/raspberrypi.cpp | 6 +++---
> > > 5 files changed, 22 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> > > index 2837fcce..0ae0baa0 100644
> > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > @@ -95,7 +95,8 @@ void CamHelper::SetCameraMode(const CameraMode &mode)
> > > initialized_ = true;
> > > }
> > >
> > > -void CamHelper::GetDelays(int &exposure_delay, int &gain_delay) const
> > > +void CamHelper::GetDelays(int &exposure_delay, int &gain_delay,
> > > + int &vblank_delay) const
> > > {
> > > /*
> > > * These values are correct for many sensors. Other sensors will
> > > @@ -103,6 +104,7 @@ void CamHelper::GetDelays(int &exposure_delay, int &gain_delay) const
> > > */
> > > exposure_delay = 2;
> > > gain_delay = 1;
> > > + vblank_delay = 2;
> > > }
> > >
> > > bool CamHelper::SensorEmbeddedDataPresent() const
> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> > > index 14d70112..8c5659ed 100644
> > > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > > @@ -28,8 +28,10 @@ namespace RPiController {
> > > // exposure time, and to convert between the sensor's gain codes and actual
> > > // gains.
> > > //
> > > -// A method to return the number of frames of delay between updating exposure
> > > -// and analogue gain and the changes taking effect. For many sensors these
> > > +// A method to return the number of frames of delay between updating exposure,
> > > +// analogue gain and vblanking, and for the changes to take effect. For many
> > > +// sensors these take the values 2, 1 and 2 respectively, but sensors that are
> > > +// different will need to over-ride the default method provided.
> > > // take the values 2 and 1 respectively, but sensors that are different will
> > > // need to over-ride the default method provided.
> >
> > Have you forgotten to remove the last two lines ?
>
> Ah yes, must have missed those. Will submit a revised version!
>
> > > //
> > > @@ -72,7 +74,8 @@ public:
> > > double maxFrameDuration) const;
> > > virtual uint32_t GainCode(double gain) const = 0;
> > > virtual double Gain(uint32_t gain_code) const = 0;
> > > - virtual void GetDelays(int &exposure_delay, int &gain_delay) const;
> > > + virtual void GetDelays(int &exposure_delay, int &gain_delay,
> > > + int &vblank_delay) const;
> > > virtual bool SensorEmbeddedDataPresent() const;
> > > virtual unsigned int HideFramesStartup() const;
> > > virtual unsigned int HideFramesModeSwitch() const;
> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > index 419f8e77..73a5ca7d 100644
> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > @@ -37,7 +37,8 @@ public:
> > > CamHelperImx477();
> > > uint32_t GainCode(double gain) const override;
> > > double Gain(uint32_t gain_code) const override;
> > > - void GetDelays(int &exposure_delay, int &gain_delay) const override;
> > > + void GetDelays(int &exposure_delay, int &gain_delay,
> > > + int &vblank_delay) const override;
> > > bool SensorEmbeddedDataPresent() const override;
> > >
> > > private:
> > > @@ -63,10 +64,12 @@ double CamHelperImx477::Gain(uint32_t gain_code) const
> > > return 1024.0 / (1024 - gain_code);
> > > }
> > >
> > > -void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay) const
> > > +void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,
> > > + int &vblank_delay) const
> > > {
> > > exposure_delay = 2;
> > > gain_delay = 2;
> > > + vblank_delay = 3;
> > > }
> > >
> > > bool CamHelperImx477::SensorEmbeddedDataPresent() const
> > > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > > index 75486e90..12be6bf9 100644
> > > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > > @@ -17,7 +17,8 @@ public:
> > > CamHelperOv5647();
> > > uint32_t GainCode(double gain) const override;
> > > double Gain(uint32_t gain_code) const override;
> > > - void GetDelays(int &exposure_delay, int &gain_delay) const override;
> > > + void GetDelays(int &exposure_delay, int &gain_delay,
> > > + int &vblank_delay) const override;
> > > unsigned int HideFramesStartup() const override;
> > > unsigned int HideFramesModeSwitch() const override;
> > > unsigned int MistrustFramesStartup() const override;
> > > @@ -51,7 +52,8 @@ double CamHelperOv5647::Gain(uint32_t gain_code) const
> > > return static_cast<double>(gain_code) / 16.0;
> > > }
> > >
> > > -void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const
> > > +void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay,
> > > + int &vblank_delay) const
> > > {
> > > /*
> > > * We run this sensor in a mode where the gain delay is bumped up to
> > > @@ -59,6 +61,7 @@ void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const
> > > */
> > > exposure_delay = 2;
> > > gain_delay = 2;
> > > + vblank_delay = 2;
> > > }
> > >
> > > unsigned int CamHelperOv5647::HideFramesStartup() const
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 6348d071..741bff4c 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -342,14 +342,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > > * Pass out the sensor config to the pipeline handler in order
> > > * to setup the staggered writer class.
> > > */
> > > - int gainDelay, exposureDelay, sensorMetadata;
> > > - helper_->GetDelays(exposureDelay, gainDelay);
> > > + int gainDelay, exposureDelay, vblankDelay, sensorMetadata;
> > > + helper_->GetDelays(exposureDelay, gainDelay, vblankDelay);
> > > sensorMetadata = helper_->SensorEmbeddedDataPresent();
> > >
> > > result->params |= ipa::RPi::ConfigSensorParams;
> > > result->sensorConfig.gainDelay = gainDelay;
> > > result->sensorConfig.exposureDelay = exposureDelay;
> > > - result->sensorConfig.vblank = exposureDelay;
> > > + result->sensorConfig.vblank = vblankDelay;
> >
> > Unrelated to this patch, should the vblank field be renamed to
> > vblankDelay ?
>
> Indeed, that looks like vblankDelay would be a better name. Looks like
> "raspberrypi.mojom" is the offender there - shall I add an extra
> commit to fix that?
That would be nice. Thank :-)
> Let me make a v2 of this set with that extra patch and I'll re-send.
>
> > At some point it could be useful to make this interface a bit more
> > generic, but that can wait.
> >
> > With the above issue fixed,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > result->sensorConfig.sensorMetadata = sensorMetadata;
> > > }
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list