[libcamera-devel] [PATCH 08/11] libcamera: ipu3: Attach to an IPA and allow it to set sensor controls

[Laurent Pinchart]

Hi Jacopo,

On Wed, Nov 18, 2020 at 04:48:28PM +0100, Jacopo Mondi wrote:
> Hi Niklas
> 
> On Thu, Nov 05, 2020 at 01:15:43AM +0100, Niklas Söderlund wrote:
> > Attache to an IPA and allow it to push V4L2 controls that applies on the
> 
> Attach
> to the IPA
> 
> The question on which control identifiers to use is the same as in the
> previous patches. Do we want the IPA and pipeline to speak V4L2_CID_
> or should we use libcamera controls where possible and let the
> CameraSensor do the translation ?

The latter, possible with CameraSensor controls instead of libcamera
controls (at least a separate ControlInfoMap, to report the right
limits). That requires work on the CameraSensor side and I don't think
this series needs to depend on that, as the RPi IPA already uses V4L2
controls.

> > camera sensor. The IPA is not fully integrated in the pipeline as
> > statistics and parameters buffers are not yet allocated, processed by
> > the IPA nor queued to the hardware.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 83 +++++++++++++++++++++++++++-
> >  1 file changed, 82 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 62e99a308a6136a7..d161c2e68c0db0f2 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -14,11 +14,13 @@
> >  #include <libcamera/camera.h>
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/formats.h>
> > +#include <libcamera/ipa/ipu3.h>
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> >
> >  #include "libcamera/internal/camera_sensor.h"
> >  #include "libcamera/internal/device_enumerator.h"
> > +#include "libcamera/internal/ipa_manager.h"
> >  #include "libcamera/internal/log.h"
> >  #include "libcamera/internal/media_device.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> > @@ -49,10 +51,12 @@ class IPU3CameraData : public CameraData
> >  {
> >  public:
> >  	IPU3CameraData(PipelineHandler *pipe)
> > -		: CameraData(pipe)
> > +		: CameraData(pipe), delayedCtrls_(nullptr)
> >  	{
> >  	}
> >
> > +	int loadIPA();
> > +
> >  	void imguOutputBufferReady(FrameBuffer *buffer);
> >  	void cio2BufferReady(FrameBuffer *buffer);
> >
> > @@ -62,6 +66,11 @@ public:
> >  	Stream outStream_;
> >  	Stream vfStream_;
> >  	Stream rawStream_;
> > +
> > +	DelayedControls *delayedCtrls_;
> > +
> > +private:
> > +	void actOnIpa(unsigned int id, const IPAOperationData &op);
> >  };
> >
> >  class IPU3CameraConfiguration : public CameraConfiguration
> > @@ -590,6 +599,12 @@ int PipelineHandlerIPU3::start(Camera *camera)
> >  	IPU3CameraData *data = cameraData(camera);
> >  	CIO2Device *cio2 = &data->cio2_;
> >  	ImgUDevice *imgu = data->imgu_;
> > +
> > +	CameraSensorInfo sensorInfo = {};
> > +	std::map<unsigned int, IPAStream> streamConfig;
> > +	std::map<unsigned int, const ControlInfoMap &> entityControls;
> > +	IPAOperationData ipaConfig;
> > +
> >  	int ret;
> >
> >  	/* Allocate buffers for internal pipeline usage. */
> > @@ -597,6 +612,10 @@ int PipelineHandlerIPU3::start(Camera *camera)
> >  	if (ret)
> >  		return ret;
> >
> > +	ret = data->ipa_->start();
> > +	if (ret)
> > +		goto error;
> > +
> >  	/*
> >  	 * Start the ImgU video devices, buffers will be queued to the
> >  	 * ImgU output and viewfinder when requests will be queued.
> > @@ -612,9 +631,33 @@ int PipelineHandlerIPU3::start(Camera *camera)
> >  		goto error;
> >  	}
> >
> > +	/* Inform IPA of stream configuration and sensor controls. */
> > +	ret = data->cio2_.sensor()->sensorInfo(&sensorInfo);
> > +	if (ret) {
> > +		/* \todo Turn to hard failure once sensors info is mandatory. */
> > +		LOG(IPU3, Warning) << "Camera sensor information not available";
> > +		sensorInfo = {};
> > +		ret = 0;
> > +	}
> > +
> > +	streamConfig[0] = {
> > +		.pixelFormat = data->outStream_.configuration().pixelFormat,
> > +		.size = data->outStream_.configuration().size,
> > +	};
> > +	streamConfig[0] = {
> 
> Overwriting streamConfig[0] ?
> 
> It's anyway so far ignored in the IPA
> 
> > +		.pixelFormat = data->vfStream_.configuration().pixelFormat,
> > +		.size = data->vfStream_.configuration().size,
> > +	};
> > +
> > +	entityControls.emplace(0, data->cio2_.sensor()->controls());
> > +
> > +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > +			      ipaConfig, nullptr);
> > +
> >  	return 0;
> >
> >  error:
> > +	data->ipa_->stop();
> >  	freeBuffers(camera);
> >  	LOG(IPU3, Error) << "Failed to start camera " << camera->id();
> >
> > @@ -631,6 +674,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> >  	if (ret)
> >  		LOG(IPU3, Warning) << "Failed to stop camera " << camera->id();
> >
> > +	data->ipa_->stop();
> > +
> >  	freeBuffers(camera);
> >  }
> >
> > @@ -762,12 +807,20 @@ int PipelineHandlerIPU3::registerCameras()
> >  		if (ret)
> >  			continue;
> >
> > +		ret = data->loadIPA();
> > +		if (ret)
> > +			continue;
> > +
> >  		/* Initialize the camera properties. */
> >  		data->properties_ = cio2->sensor()->properties();
> >
> >  		/* Initialze the camera controls. */
> >  		data->controlInfo_ = IPU3Controls;
> >
> > +		data->delayedCtrls_ = cio2->sensor()->delayedContols();
> > +		data->cio2_.frameStart().connect(data->delayedCtrls_,
> > +						 &DelayedControls::frameStart);
> > +
> >  		/**
> >  		 * \todo Dynamically assign ImgU and output devices to each
> >  		 * stream and camera; as of now, limit support to two cameras
> > @@ -811,6 +864,34 @@ int PipelineHandlerIPU3::registerCameras()
> >  	return numCameras ? 0 : -ENODEV;
> >  }
> >
> > +int IPU3CameraData::loadIPA()
> > +{
> > +	ipa_ = IPAManager::createIPA(pipe_, 1, 1);
> > +	if (!ipa_)
> > +		return -ENOENT;
> > +
> > +	ipa_->queueFrameAction.connect(this, &IPU3CameraData::actOnIpa);
> > +
> > +	ipa_->init(IPASettings{});
> > +
> > +	return 0;
> > +}
> > +
> > +void IPU3CameraData::actOnIpa([[maybe_unused]] unsigned int id,
> > +			      const IPAOperationData &action)
> > +{
> > +	switch (action.operation) {
> > +	case IPU3_IPA_ACTION_SET_SENSOR_CONTROLS: {
> > +		const ControlList &controls = action.controls[0];
> > +		delayedCtrls_->push(controls);
> 
> Do we want to have a CameraSensor::setDelayedControls() ?
> This direct handling of delayed controls bypasses the CameraSensor
> completely pushing to each pipeline handler the controls translation
> burden

I think we want to embed an instance of DelayedControls in CameraSensor,
yes, and create a nice API to interface with it. I think this is also
not a blocker for this series, but it should be done sooner than later.

Overall I think this patch is a good step forward. It would be nice to
address the crash that Jean-Michel experienced though :-)

> > +		break;
> > +	}
> > +	default:
> > +		LOG(IPU3, Error) << "Unknown action " << action.operation;
> > +		break;
> > +	}
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Buffer Ready slots
> >   */

-- 
Regards,

Laurent Pinchart