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

[Laurent Pinchart]

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 at 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();
>  }

-- 
Regards,

Laurent Pinchart