[libcamera-devel] [PATCH v4 6/8] libcamera: pipeline: rkisp1: Use delayed controls

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tue, Dec 15, 2020 at 01:48:09AM +0100, Niklas Söderlund wrote:
> Instead of setting controls using the RkISP1 local Timeline helper use
> the DelayedControls. The result is the same, the controls are applied
> with a delay.
> 
> The values of the delays are however different between the two methods.
> The values used in the Timeline solution were chosen after some
> experimentation and the values used in DelayedControls are taken from a
> generic sensor. None of the two are a perfect match as the delays can be
> different for different sensors used with the pipeline.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
> * Changes since v3
> - Rewrite to not use CameraSensor.
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 43 +++++++++++++-----------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 4d98c9027f42c759..513a60b04e5f2e21 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -23,6 +23,7 @@
>  #include <libcamera/stream.h>
>  
>  #include "libcamera/internal/camera_sensor.h"
> +#include "libcamera/internal/delayed_controls.h"
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/log.h"
> @@ -136,6 +137,7 @@ public:
>  	Stream mainPathStream_;
>  	Stream selfPathStream_;
>  	CameraSensor *sensor_;
> +	std::unique_ptr<DelayedControls> delayedCtrls_;
>  	unsigned int frame_;
>  	std::vector<IPABuffer> ipaBuffers_;
>  	RkISP1Frames frameInfo_;
> @@ -345,23 +347,6 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)
>  	return nullptr;
>  }
>  
> -class RkISP1ActionSetSensor : public FrameAction
> -{
> -public:
> -	RkISP1ActionSetSensor(unsigned int frame, CameraSensor *sensor, const ControlList &controls)
> -		: FrameAction(frame, SetSensor), sensor_(sensor), controls_(controls) {}
> -
> -protected:
> -	void run() override
> -	{
> -		sensor_->setControls(&controls_);
> -	}
> -
> -private:
> -	CameraSensor *sensor_;
> -	ControlList controls_;
> -};
> -
>  class RkISP1ActionQueueBuffers : public FrameAction
>  {
>  public:
> @@ -429,9 +414,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,
>  	switch (action.operation) {
>  	case RKISP1_IPA_ACTION_V4L2_SET: {
>  		const ControlList &controls = action.controls[0];
> -		timeline_.scheduleAction(std::make_unique<RkISP1ActionSetSensor>(frame,
> -										 sensor_,
> -										 controls));
> +		delayedCtrls_->push(controls);
>  		break;
>  	}
>  	case RKISP1_IPA_ACTION_PARAM_FILLED: {
> @@ -898,6 +881,8 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
>  		};
>  	}
>  
> +	isp_->setFrameStartEnabled(true);
> +
>  	activeCamera_ = camera;
>  
>  	/* Inform IPA of stream configuration and sensor controls. */
> @@ -925,6 +910,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
>  	RkISP1CameraData *data = cameraData(camera);
>  	int ret;
>  
> +	isp_->setFrameStartEnabled(false);
> +
>  	selfPath_.stop();
>  	mainPath_.stop();
>  
> @@ -1043,6 +1030,22 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	/* Initialize the camera properties. */
>  	data->properties_ = data->sensor_->properties();
>  
> +	/*
> +	 * \todo Read dealy values from the sensor itself or from a
> +	 * a sensor database. For now use generic values taken from
> +	 * the Raspberry Pi and listed as generic values.
> +	 */
> +	std::unordered_map<uint32_t, unsigned int> delays = {
> +		{ V4L2_CID_ANALOGUE_GAIN, 1 },
> +		{ V4L2_CID_EXPOSURE, 2 },
> +	};
> +
> +	data->delayedCtrls_ =
> +		std::make_unique<DelayedControls>(data->sensor_->device(),
> +						  delays);
> +	isp_->frameStart.connect(data->delayedCtrls_.get(),
> +				 &DelayedControls::applyControls);
> +
>  	ret = data->loadIPA();
>  	if (ret)
>  		return ret;

-- 
Regards,

Laurent Pinchart