[libcamera-devel] [PATCH v2 2/4] pipeline: raspberrypi: Use priority write for vblank when writing sensor ctrls

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Thu, Jul 01, 2021 at 12:34:40PM +0100, Naushir Patuck wrote:
> When directly writing controls to the sensor device, ensure that VBLANK is
> written ahead of and before the EXPOSURE control. This is the same priority
> write mechanism used in DelayedControls.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 37 ++++++++++++++-----
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 082eb1ee1c23..53a30cff1864 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -154,6 +154,7 @@ public:
>  	void embeddedComplete(uint32_t bufferId);
>  	void setIspControls(const ControlList &controls);
>  	void setDelayedControls(const ControlList &controls);
> +	void setSensorControls(ControlList &controls);
>  
>  	/* bufferComplete signal handlers. */
>  	void unicamBufferDequeue(FrameBuffer *buffer);
> @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>  
>  	/* Apply any gain/exposure settings that the IPA may have passed back. */
>  	if (!startConfig.controls.empty())
> -		data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls);
> +		data->setSensorControls(startConfig.controls);
>  
>  	/* Configure the number of dropped frames required on startup. */
>  	data->dropFrameCount_ = startConfig.dropFrameCount;
> @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  		return -EPIPE;
>  	}
>  
> -	if (!controls.empty())
> -		unicam_[Unicam::Image].dev()->setControls(&controls);
> -
>  	/*
>  	 * Configure the H/V flip controls based on the combination of
>  	 * the sensor and user transform.
>  	 */
>  	if (supportsFlips_) {
> -		ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> -		ctrls.set(V4L2_CID_HFLIP,
> -			  static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
> -		ctrls.set(V4L2_CID_VFLIP,
> -			  static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
> -		unicam_[Unicam::Image].dev()->setControls(&ctrls);
> +		controls.set(V4L2_CID_HFLIP,
> +			     static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
> +		controls.set(V4L2_CID_VFLIP,
> +			     static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
>  	}

Strictly speaking, this could have stayed the same, but it doesn't hurt.

>  
> +	if (!controls.empty())
> +		setSensorControls(controls);
> +
>  	return 0;
>  }
>  
> @@ -1379,6 +1378,24 @@ void RPiCameraData::setDelayedControls(const ControlList &controls)
>  	handleState();
>  }
>  
> +void RPiCameraData::setSensorControls(ControlList &controls)
> +{
> +	/*
> +	 * We need to ensure that if both VBLANK and EXPOSURE are present, the
> +	 * former must be written ahead of, and separately from EXPOSURE to avoid
> +	 * V4L2 rejecting the latter. This is identical to what DelayedControls
> +	 * does with the priority write flag.
> +	 */
> +	if (controls.contains(V4L2_CID_EXPOSURE) && controls.contains(V4L2_CID_VBLANK)) {
> +		ControlList vblank_ctrl;
> +
> +		vblank_ctrl.set(V4L2_CID_VBLANK, controls.get(V4L2_CID_VBLANK));
> +		unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);
> +	}
> +
> +	unicam_[Unicam::Image].dev()->setControls(&controls);

VBLANK will be set twice, could it be an issue ? The value will be the
same each time, but that can generate additional I2C writes, which isn't
great.

> +}
> +
>  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  {
>  	RPi::Stream *stream = nullptr;

-- 
Regards,

Laurent Pinchart