[libcamera-devel] [PATCH v2 3/3] ipa: raspberrypi: Update sensor's RED/BLUE balance controls when present

[David Plowman]

Hi Jacopo

Thanks very much for the comments.

On Thu, 8 Apr 2021 at 16:59, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi David,
>
> On Thu, Apr 08, 2021 at 02:36:34PM +0100, David Plowman wrote:
> > If the sensor exposes V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE
> > controls, assume it wants to be told the colour gains.
> >
> > We want these to be applied as soon as possible so we add a new
>
> Can you be patient with me and explain why these controls are special
> and qualify for a fast-tracked method ?

Yes of course! So I'm looking at some sensors that do some "clever"
processing on the sensor itself, for which they need to know what the
red and blue gains are. Some examples:

* A "quad Bayer" or "tetracell" sensor may have the ability to output
an HDR Bayer pattern. In order to combine all the different exposures
it needs to have the colour gains.
* The same sensor may have the ability to turn the "quad" pattern of
pixels into a standard full resolution Bayer pattern. This needs the
colour gains too.
* Sensors with non-standard patterns often support conversion to
regular Bayer - which can work better with the colour gains.

In all these cases it's good to get the latest colour gains to the
sensor as fast as we can, to limit the artefacts produced if the white
balance is changing. Note that we never need to try and read them
back, as we do with exposure and analogue gain.

The first version of this patch used the delayed control mechanism. I
think that was OK too, but it felt like a slight abuse of that
mechanism - we don't need the complexity of trying to align it with
other sensor updates. And it was confusing - I gave it the same delay
as the vblank control so that it would get written immediately, yet it
actually happens on the very next frame which would normally be a
delay of 1, so it all seemed a bit strange...

>
> > setSensorControls signal to the IPA which passes these back to the
> > pipeline handler without using the DelayedControls mechanism.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.mojom            |  1 +
> >  src/ipa/raspberrypi/raspberrypi.cpp                | 13 +++++++++++++
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++++
> >  3 files changed, 24 insertions(+)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > index f38c2261..ebaf0a04 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -117,6 +117,7 @@ interface IPARPiEventInterface {
> >       statsMetadataComplete(uint32 bufferId, ControlList controls);
> >       runIsp(uint32 bufferId);
> >       embeddedComplete(uint32 bufferId);
> > +     setSensorControls(ControlList controls);
> >       setIspControls(ControlList controls);
> >       setDelayedControls(ControlList controls);
> >  };
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index dad6395f..f95896d2 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -1036,6 +1036,19 @@ void IPARPi::processStats(unsigned int bufferId)
> >
> >               setDelayedControls.emit(ctrls);
> >       }
> > +
> > +     struct AwbStatus awbStatus;
> > +     if (rpiMetadata_.Get("awb.status", awbStatus) == 0 &&
> > +         sensorCtrls_.find(V4L2_CID_RED_BALANCE) != sensorCtrls_.end() &&
> > +         sensorCtrls_.find(V4L2_CID_BLUE_BALANCE) != sensorCtrls_.end()) {
> > +             ControlList ctrls(sensorCtrls_);
> > +             ctrls.set(V4L2_CID_RED_BALANCE,
> > +                       static_cast<int32_t>(helper_->ColourGainCode(awbStatus.gain_r)));
> > +             ctrls.set(V4L2_CID_BLUE_BALANCE,
> > +                       static_cast<int32_t>(helper_->ColourGainCode(awbStatus.gain_b)));
>
> Won't this trigger the assert(0) in the previous patch ?

Hopefully not - the idea is that you must override the default method
if your sensor wants the RED/BLUE_BALANCE numbers. For sensors that
don't need them, everything is fine as it is.

I hope that all makes sense?

Thanks!
David

>
> Thanks
>   j
>
> > +
> > +             setSensorControls.emit(ctrls);
> > +     }
> >  }
> >
> >  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index f22e286e..8dae4912 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -152,6 +152,7 @@ public:
> >       void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> >       void runIsp(uint32_t bufferId);
> >       void embeddedComplete(uint32_t bufferId);
> > +     void setSensorControls(const ControlList &controls);
> >       void setIspControls(const ControlList &controls);
> >       void setDelayedControls(const ControlList &controls);
> >
> > @@ -1219,6 +1220,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
> >       ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
> >       ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
> >       ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
> > +     ipa_->setSensorControls.connect(this, &RPiCameraData::setSensorControls);
> >       ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
> >       ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
> >
> > @@ -1361,6 +1363,14 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)
> >       handleState();
> >  }
> >
> > +void RPiCameraData::setSensorControls(const ControlList &controls)
> > +{
> > +     ControlList ctrls = controls;
> > +
> > +     unicam_[Unicam::Image].dev()->setControls(&ctrls);
> > +     handleState();
> > +}
> > +
> >  void RPiCameraData::setIspControls(const ControlList &controls)
> >  {
> >       ControlList ctrls = controls;
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel