[libcamera-devel,v2,3/3] libcamera: pipeline: raspberrypi: Update sensor's V4L2_CID_NOTIFY_GAINS control

15250 diff mbox series

If the sensor exposes the V4L2_CID_NOTIFY_GAINS control, assume it
means the sensor wants to be told the latest colour gains.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

Hi David,

Thank you for the patch.

On Wed, Jan 05, 2022 at 08:55:53AM +0000, David Plowman wrote:
> If the sensor exposes the V4L2_CID_NOTIFY_GAINS control, assume it
> means the sensor wants to be told the latest colour gains.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index b5c687da..0d4b5a57 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1495,6 +1495,32 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
>  	Request *request = requestQueue_.front();
>  	request->metadata().merge(controls);
>  
> +	/*
> +	 * If the sensor has the V4L2_CID_NOTIFY_GAIN control then it wants
> +	 * to be notified of the latest colour gains.
> +	 */
> +	auto it = sensor_->controls().find(V4L2_CID_NOTIFY_GAINS);

As this will not vary at runtime, this check could be done at init time,
with the unity value cached and a flag set to indicate notify gains
support. Up to you.

> +	if (it != sensor_->controls().end()) {
> +		if (controls.contains(libcamera::controls::ColourGains)) {
> +			libcamera::Span<const float> colourGains = controls.get(libcamera::controls::ColourGains);
> +			/*
> +			 * This control is a linear gain value where the default is
> +			 * defined to correspond to a gain of 1. The order of the gains
> +			 * is always B, Gb, Gr, R.
> +			 */
> +			int unity = it->second.def().get<int32_t>();
> +			ControlList ctrls(sensor_->controls());
> +			int32_t gains[4] = { static_cast<int32_t>(colourGains[1] * unity),
> +					     unity, unity,
> +					     static_cast<int32_t>(colourGains[0] * unity) };

std::array could be used too, up to you.

> +			ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(gains),
> +							    sizeof(gains) });

I'm a bit surprised that you have to cast this to a uint8_t array, as
the control uses 32-bit integer array. Would

			ControlValue c(Span<const uint32_t>{ gains });

work ? It may even be possible to write

			ctrls.set(V4L2_CID_NOTIFY_GAINS, Span<const uint32_t>{ gains });

as the ControlValue constructor is implicit.

> +			ctrls.set(V4L2_CID_NOTIFY_GAINS, c);
> +
> +			sensor_->setControls(&ctrls);
> +		}
> +	}
> +
>  	state_ = State::IpaComplete;
>  	handleState();
>  }

Hi Laurent

Thanks for the suggestions.

On Wed, 5 Jan 2022 at 10:19, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Wed, Jan 05, 2022 at 08:55:53AM +0000, David Plowman wrote:
> > If the sensor exposes the V4L2_CID_NOTIFY_GAINS control, assume it
> > means the sensor wants to be told the latest colour gains.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 26 +++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index b5c687da..0d4b5a57 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1495,6 +1495,32 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
> >       Request *request = requestQueue_.front();
> >       request->metadata().merge(controls);
> >
> > +     /*
> > +      * If the sensor has the V4L2_CID_NOTIFY_GAIN control then it wants
> > +      * to be notified of the latest colour gains.
> > +      */
> > +     auto it = sensor_->controls().find(V4L2_CID_NOTIFY_GAINS);
>
> As this will not vary at runtime, this check could be done at init time,
> with the unity value cached and a flag set to indicate notify gains
> support. Up to you.

Yes, I could do that. Actually, I can sense another std::optional
coming on so I think I'll try that!

>
> > +     if (it != sensor_->controls().end()) {
> > +             if (controls.contains(libcamera::controls::ColourGains)) {
> > +                     libcamera::Span<const float> colourGains = controls.get(libcamera::controls::ColourGains);
> > +                     /*
> > +                      * This control is a linear gain value where the default is
> > +                      * defined to correspond to a gain of 1. The order of the gains
> > +                      * is always B, Gb, Gr, R.
> > +                      */
> > +                     int unity = it->second.def().get<int32_t>();
> > +                     ControlList ctrls(sensor_->controls());
> > +                     int32_t gains[4] = { static_cast<int32_t>(colourGains[1] * unity),
> > +                                          unity, unity,
> > +                                          static_cast<int32_t>(colourGains[0] * unity) };
>
> std::array could be used too, up to you.

Perhaps it's the modern C++ way these days, so yes, I'll do that.

>
> > +                     ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(gains),
> > +                                                         sizeof(gains) });
>
> I'm a bit surprised that you have to cast this to a uint8_t array, as
> the control uses 32-bit integer array. Would
>
>                         ControlValue c(Span<const uint32_t>{ gains });
>
> work ? It may even be possible to write
>
>                         ctrls.set(V4L2_CID_NOTIFY_GAINS, Span<const uint32_t>{ gains });

Thanks for the tidy-ups there, that seems to work. I had probably
copied it from somewhere and been relieved when all the nasty template
stuff just compiled! :)

Best regards
David

>
> as the ControlValue constructor is implicit.
>
> > +                     ctrls.set(V4L2_CID_NOTIFY_GAINS, c);
> > +
> > +                     sensor_->setControls(&ctrls);
> > +             }
> > +     }
> > +
> >       state_ = State::IpaComplete;
> >       handleState();
> >  }
>
> --
> Regards,
>
> Laurent Pinchart