| Message ID | 20210408133634.16815-4-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 ? > 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@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 ? 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo Thanks very much for the comments. On Thu, 8 Apr 2021 at 16:59, Jacopo Mondi <jacopo@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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi David, On 08/04/2021 18:12, David Plowman wrote: > Hi Jacopo > > Thanks very much for the comments. > > On Thu, 8 Apr 2021 at 16:59, Jacopo Mondi <jacopo@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@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. Yes, I can see that the ColourGainCode is only called if the sensor has a control for boht RED and BLUE balance. And if that control is available on the sensor, then the CamHelper *must* override the ColourGainCode(). I think that makes me more convinced that the assertion in the previous patch should be a more verbose message explaining what must be done if it is hit, as it could be hit by a user just changing to a new (not yet supported) sensor... but otherwise it seems fine. I'm a bit weary of having two routes to set controls on the sensor through setSensorControls() and through setDelayedControls(), but given the namings of both show that one is 'delayed', I think the reasoning is clear ... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > 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@lists.libcamera.org >>> https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
HI Kieran Thanks for the review! On Wed, 14 Apr 2021 at 09:39, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi David, > > On 08/04/2021 18:12, David Plowman wrote: > > Hi Jacopo > > > > Thanks very much for the comments. > > > > On Thu, 8 Apr 2021 at 16:59, Jacopo Mondi <jacopo@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@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. > > Yes, I can see that the ColourGainCode is only called if the sensor has > a control for boht RED and BLUE balance. > > And if that control is available on the sensor, then the CamHelper > *must* override the ColourGainCode(). > > I think that makes me more convinced that the assertion in the previous > patch should be a more verbose message explaining what must be done if > it is hit, as it could be hit by a user just changing to a new (not yet > supported) sensor... but otherwise it seems fine. Yes, I think that seems reasonable to me too. I'll update the patch shortly. Best regards David > > > I'm a bit weary of having two routes to set controls on the sensor > through setSensorControls() and through setDelayedControls(), but given > the namings of both show that one is 'delayed', I think the reasoning is > clear ... > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > 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@lists.libcamera.org > >>> https://lists.libcamera.org/listinfo/libcamera-devel > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran
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))); + + 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;
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 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@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(+)