[libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: configure IPA from configure method instead of start method

[Laurent Pinchart]

Hi Sebastian,

On Sat, Feb 13, 2021 at 10:12:17AM +0100, Sebastian Fricke wrote:
> On 12.02.2021 20:05, Dafna Hirschfeld wrote:
> > Currently the call to the configure method of rkisp1 IPA
> > is called upon the 'start' of the pipeline. This should be done
> > in the 'configure' method instead.
> 
> I assume that the reason for this change is consistency right? I believe
> the raspberry Pi has the most mature pipeline and IPA implementation and
> it makes sense to have some kind of consistent pattern.
> 
> In order to find out why it is necessary to configure the IPA in
> configure instead of start, I first looked into `src/cam/capture.cpp`,
> where both methods are called. In between the configure and the start
> call, the buffers are allocated and the initial set of requests is
> created.
> Additionally, I have compared both the RkISP1 and Raspberry Pi
> implementation of the IPA configure method. Currently, the RkISP1 IPA
> doesn't do a lot so it is difficult to decide, which parts might be added.
> The Raspberry Pi currently does the following actions that are missing in
> the RkISP1:
> - Setup and validate the ISP controls
> - Setup metadata controls
> - Load device specific information from a helper
> - Load the lens shading table
> - Prepare the algorithms by setting the camera mode
> - Load the tuning file for the sensor
> 
> What they both do is initialize the analog gain and exposure controls.
> (With the difference that RkISP1 sets both to their respective minimum
> and Raspberry Pi provides default values).
> 
> This means, that the different order of the IPA configure method can
> only have an impact on the actions happening in between the two states
> configured and start (allocation of buffers and creation of the inital
> requests). So, the current advantage/disadvantage is that the controls
> are reset to their minimum for the initial set of requests as well.

Nice and detailed analysis :-)

>  From my point of view this seems to be a good preparation for the
> following changes to the RkISP1 IPA, even though it currently doesn't
> seem to do to much. Have I missed anything else that makes this order of
> actions essential?

Right now, no, but with Paul's series that implements the IPA IPC
protocol, IPA functions called after start() must be asynchronous. The
IPA configure() function is a synchronous call, so this won't work well.
This patch is in preparation for the IPA IPC (beside improving
consistency, which is usually good).

> Besides that little analysis I have tested the patch by building it and
> performing a test capture. Both works without problems.
> 
> Greetings,
> Sebastian
> 
> I fixed a small typo below that existed before as well.
> 
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld at collabora.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 63 ++++++++++++------------
> >  1 file changed, 31 insertions(+), 32 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index e85979a7..f15efa9f 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -607,11 +607,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >  		<< "ISP output pad configured with " << format.toString()
> >  		<< " crop " << rect.toString();
> > 
> > +	std::map<unsigned int, IPAStream> streamConfig;
> > +
> >  	for (const StreamConfiguration &cfg : *config) {
> > -		if (cfg.stream() == &data->mainPathStream_)
> > +		if (cfg.stream() == &data->mainPathStream_) {
> >  			ret = mainPath_.configure(cfg, format);
> > -		else
> > +			if (data->mainPath_->isEnabled())
> > +				streamConfig[0] = {
> > +					.pixelFormat = cfg.pixelFormat,
> > +					.size = cfg.size,
> > +				};
> > +		} else {
> >  			ret = selfPath_.configure(cfg, format);
> > +			if (data->selfPath_->isEnabled())
> > +				streamConfig[1] = {
> > +					.pixelFormat = cfg.pixelFormat,
> > +					.size = cfg.size,
> > +				};
> > +		}
> > 
> >  		if (ret)
> >  			return ret;
> > @@ -629,6 +642,22 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >  	if (ret)
> >  		return ret;
> > 
> > +	/* Inform IPA of stream configuration and sensor controls. */
> > +	CameraSensorInfo sensorInfo = {};
> > +	ret = data->sensor_->sensorInfo(&sensorInfo);
> > +	if (ret) {
> > +		/* \todo Turn this in an hard failure. */
> 
> s/Turn this in an hard failure./Turn this into a hard failure./
> (or maybe: Make this a hard failure)
> 
> > +		LOG(RkISP1, Warning) << "Camera sensor information not available";
> > +		sensorInfo = {};
> > +		ret = 0;
> > +	}
> > +
> > +	std::map<unsigned int, const ControlInfoMap &> entityControls;
> > +	entityControls.emplace(0, data->sensor_->controls());
> > +
> > +	IPAOperationData ipaConfig;
> > +	data->ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig, nullptr);
> > +
> >  	return 0;
> >  }
> > 
> > @@ -759,8 +788,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
> >  		return ret;
> >  	}
> > 
> > -	std::map<unsigned int, IPAStream> streamConfig;
> > -
> >  	if (data->mainPath_->isEnabled()) {
> >  		ret = mainPath_.start();
> >  		if (ret) {
> > @@ -770,11 +797,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
> >  			freeBuffers(camera);
> >  			return ret;
> >  		}
> > -
> > -		streamConfig[0] = {
> > -			.pixelFormat = data->mainPathStream_.configuration().pixelFormat,
> > -			.size = data->mainPathStream_.configuration().size,
> > -		};
> >  	}
> > 
> >  	if (data->selfPath_->isEnabled()) {
> > @@ -787,34 +809,11 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
> >  			freeBuffers(camera);
> >  			return ret;
> >  		}
> > -
> > -		streamConfig[1] = {
> > -			.pixelFormat = data->selfPathStream_.configuration().pixelFormat,
> > -			.size = data->selfPathStream_.configuration().size,
> > -		};
> >  	}
> > 
> >  	isp_->setFrameStartEnabled(true);
> > 
> >  	activeCamera_ = camera;
> > -
> > -	/* Inform IPA of stream configuration and sensor controls. */
> > -	CameraSensorInfo sensorInfo = {};
> > -	ret = data->sensor_->sensorInfo(&sensorInfo);
> > -	if (ret) {
> > -		/* \todo Turn this in an hard failure. */
> > -		LOG(RkISP1, Warning) << "Camera sensor information not available";
> > -		sensorInfo = {};
> > -		ret = 0;
> > -	}
> > -
> > -	std::map<unsigned int, const ControlInfoMap &> entityControls;
> > -	entityControls.emplace(0, data->sensor_->controls());
> > -
> > -	IPAOperationData ipaConfig;
> > -	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > -			      ipaConfig, nullptr);
> > -
> >  	return ret;
> >  }
> > 

-- 
Regards,

Laurent Pinchart