[libcamera-devel] [PATCH v6 6/8] pipeline: raspberrypi: Use priority write for vblank when writing sensor ctrls

[Jacopo Mondi]

On Fri, Jul 09, 2021 at 03:58:23PM +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>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 41 ++++++++++++++-----
>  1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 082eb1ee1c23..e09328ffa0bc 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)));
>  	}
>
> +	if (!controls.empty())
> +		setSensorControls(controls);
> +
>  	return 0;
>  }
>
> @@ -1379,6 +1378,28 @@ 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.
> +	 *
> +	 * As a consequence of the below logic, VBLANK gets set twice, and we
> +	 * rely on the v4l2 framework to not pass the second control set to the
> +	 * driver as the actual control value has not changed.
> +	 */
> +	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);
> +}
> +

Uff, v4l2...

I see another usage of setControls() at match time, but it should not
involve VBLANK, so it could be kept the way it is

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

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