[libcamera-devel] [PATCH v3 6/8] libcamera: pipeline: rkisp1: Use CameraSensor and delayed controls

[Jacopo Mondi]

Hi Niklas,

On Mon, Nov 23, 2020 at 11:12:32PM +0100, Niklas Söderlund wrote:
> Instead of setting controls using the RkISP1 local Timeline helper use
> the DelayedControls interface provided by the CameraSensor. The result
> are the same, the controls are applied with a delay.

s/are/is

>
> The values of the delays are however different between the two methods.
> The values used in the Timeline solution was chosen after some

s/was/were/

> 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.

How are we going to quantify those dealays when filling in the sensor
database ?

>
> However using the interface provided by CameraSensor we prepare for the
> future where sensor specific delays will provided by the CameraSensor
> and used without any change in the pipeline.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++---------------
>  1 file changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 6e74a49abfda1b55..c3c4b5a65e3d9afe 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -121,8 +121,9 @@ class RkISP1CameraData : public CameraData
>  public:
>  	RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,
>  			 RkISP1SelfPath *selfPath)
> -		: CameraData(pipe), sensor_(nullptr), frame_(0),
> -		  frameInfo_(pipe), mainPath_(mainPath), selfPath_(selfPath)
> +		: CameraData(pipe), sensor_(nullptr), delayedCtrls_(nullptr),
> +		  frame_(0), frameInfo_(pipe), mainPath_(mainPath),
> +		  selfPath_(selfPath)
>  	{
>  	}
>
> @@ -136,6 +137,7 @@ public:
>  	Stream mainPathStream_;
>  	Stream selfPathStream_;
>  	CameraSensor *sensor_;
> +	DelayedControls *delayedCtrls_;
>  	unsigned int frame_;
>  	std::vector<IPABuffer> ipaBuffers_;
>  	RkISP1Frames frameInfo_;
> @@ -346,23 +348,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:
> @@ -430,9 +415,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);

I'm not sure I like this. As I've said I think this sits half-way
between a 1-to-1 replacement of StaggeredCtrl and an half-backed
enhancement.

What I mean is:
1) Delays and which controls are delayed is a decision of the
CameraSensor. The pipeline and the IPA has no say on this with the
current interface.
2) Here we unconditionally push controls received from the IPA, which
implies it has to know which one are delayed (otherwise the push()
here fails).

As suggested in the previous patch I would backtrack and mimic what
StaggeredCtrls did, so let the pipeline initialize delays and which
controls are delayed, as in the end the IPA has to know that
anyway.

Going forward I agree CameraSensor will have to handle this, but we
need to find a better way to do so to decouple IPA and sensor specific
requirements (or decide we can't and make the IPA sensor-aware, but I
think nobody wants this).

>  		break;
>  	}
>  	case RKISP1_IPA_ACTION_PARAM_FILLED: {
> @@ -906,6 +889,8 @@ int PipelineHandlerRkISP1::start(Camera *camera)
>  		};
>  	}
>
> +	isp_->setFrameStartEnabled(true);
> +
>  	activeCamera_ = camera;
>
>  	/* Inform IPA of stream configuration and sensor controls. */
> @@ -933,6 +918,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
>  	RkISP1CameraData *data = cameraData(camera);
>  	int ret;
>
> +	isp_->setFrameStartEnabled(false);
> +
>  	selfPath_.stop();
>  	mainPath_.stop();
>
> @@ -1051,6 +1038,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	/* Initialize the camera properties. */
>  	data->properties_ = data->sensor_->properties();
>
> +	data->delayedCtrls_ = data->sensor_->delayedContols();
> +	isp_->frameStart.connect(data->delayedCtrls_,
> +				 &DelayedControls::frameStart);
> +
>  	ret = data->loadIPA();
>  	if (ret)
>  		return ret;
> --
> 2.29.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel