[libcamera-devel] [PATCH v4 06/31] libcamera: ipu3: Initialize and configure ImgUs

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Mar 20, 2019 at 05:30:30PM +0100, Jacopo Mondi wrote:
> Create video devices and subdevices associated with an ImgU unit at
> camera registration time.
> 
> Statically assign imgu0 to the first camera and imgu1 to the second one
> and limit support to two camera. This will have to be revised in future.

s/camera/cameras/
s/in future/in the future/

> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 223 +++++++++++++++++++++++++--
>  1 file changed, 209 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 2e351d04a784..26fc56a76eb1 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -27,6 +27,38 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPU3)
>  
> +class ImgUDevice
> +{
> +public:
> +	ImgUDevice()
> +		: imgu(nullptr), input(nullptr), output(nullptr),
> +		  viewfinder(nullptr), stat(nullptr)
> +	{
> +	}
> +
> +	~ImgUDevice()
> +	{
> +		delete imgu;
> +		delete input;
> +		delete output;
> +		delete viewfinder;
> +		delete stat;
> +	}
> +
> +	void init(MediaDevice *media, unsigned int index);
> +
> +	unsigned int index_;
> +	std::string imguName_;

As this is the ImgUDevice class, maybe just "name_" ?

> +	MediaDevice *mediaDevice_;
> +
> +	V4L2Subdevice *imgu;
> +	V4L2Device *input;
> +	V4L2Device *output;
> +	V4L2Device *viewfinder;
> +	V4L2Device *stat;

Missing _ at the end of each variable.

> +	/* \todo Add param video device for 3A tuning */
> +};
> +
>  struct CIO2Device {
>  	CIO2Device()
>  		: output(nullptr), csi2(nullptr), sensor(nullptr)
> @@ -68,6 +100,7 @@ public:
>  	bool match(DeviceEnumerator *enumerator);
>  
>  private:
> +	static constexpr unsigned int IPU3_IMGU_COUNT = 2;
>  	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
>  
>  	class IPU3CameraData : public CameraData
> @@ -81,6 +114,7 @@ private:
>  		void bufferReady(Buffer *buffer);
>  
>  		CIO2Device cio2;
> +		ImgUDevice *imgu;

Here too.

>  
>  		Stream stream_;
>  	};
> @@ -92,10 +126,18 @@ private:
>  	}
>  
>  	int mediaBusToCIO2Format(unsigned int code);
> +	V4L2Device *openDevice(MediaDevice *media, const std::string &name);
> +	V4L2Subdevice *openSubdevice(MediaDevice *media,
> +				     const std::string &name);
>  
>  	int initCIO2(unsigned int index, CIO2Device *cio2);
> +	void deleteCIO2(CIO2Device *cio2);
> +
> +	int initImgU(ImgUDevice *imgu);
> +
>  	void registerCameras();
>  
> +	ImgUDevice imgus_[IPU3_IMGU_COUNT];
>  	std::shared_ptr<MediaDevice> cio2MediaDev_;
>  	std::shared_ptr<MediaDevice> imguMediaDev_;
>  };
> @@ -355,11 +397,45 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
>  		return false;
>  	}
>  
> +	if (imguMediaDev_->open()) {
> +		cio2MediaDev_->close();
> +		return false;
> +	}
> +
> +	if (imguMediaDev_->disableLinks())
> +		goto error_close_mdev;
> +
> +	for (unsigned int i = 0; i < IPU3_IMGU_COUNT; ++i)
> +		imgus_[i].init(imguMediaDev_.get(), i);
> +
>  	registerCameras();
>  
>  	cio2MediaDev_->close();
> +	imguMediaDev_->close();
>  
>  	return true;
> +
> +error_close_mdev:

There's a single error label, just call it error.

> +	cio2MediaDev_->close();
> +	imguMediaDev_->close();
> +
> +	return false;
> +}
> +
> +/* ----------------------------------------------------------------------------
> + * Helpers
> + */
> +
> +/**
> + * \brief Initialize fields of the ImgU instance
> + * \param mediaDevice The ImgU instance media device
> + * \param index The ImgU instance index
> + */
> +void ImgUDevice::init(MediaDevice *mediaDevice, unsigned int index)
> +{
> +	index_ = index;
> +	imguName_ = "ipu3-imgu " + std::to_string(index_);
> +	mediaDevice_ = mediaDevice;
>  }
>  
>  int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)
> @@ -378,6 +454,114 @@ int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)
>  	}
>  }
>  
> +/**
> + * \brief Create and open the video device with \a name in media device \a media
> + *
> + * \todo Make a generic helper out of this method.

Please do :-) You can create a static function in the V4L2Device class
that takes a media device and entity name and return a newly created
V4L2Device. I would however keep the open() call outside of the helper
as we'll need to refactor open/close later.

> + *
> + * \return Pointer to the video device on success, nullptr otherwise
> + */
> +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media,
> +					    const std::string &name)
> +{
> +	MediaEntity *entity = media->getEntityByName(name);
> +	if (!entity) {
> +		LOG(IPU3, Error)
> +			<< "Failed to get entity '" << name << "'";
> +		return nullptr;
> +	}
> +
> +	V4L2Device *dev = new V4L2Device(entity);
> +	if (dev->open()) {
> +		delete dev;
> +		return nullptr;
> +	}
> +
> +	return dev;
> +}
> +
> +/**
> + * \brief Create and open the subdevice with \a name in media device \a media
> + *
> + * \todo Make a generic helper out of this method.

I'd say the same here, but you use this helper in a single location, so
I would just inline it.

> + *
> + * \return Pointer to the subdevice on success, nullptr otherwise
> + */
> +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media,
> +						  const std::string &name)
> +{
> +	MediaEntity *entity = media->getEntityByName(name);
> +	if (!entity) {
> +		LOG(IPU3, Error)
> +			<< "Failed to get entity '" << name << "'";
> +		return nullptr;
> +	}
> +
> +	V4L2Subdevice *dev = new V4L2Subdevice(entity);
> +	if (dev->open()) {
> +		delete dev;
> +		return nullptr;
> +	}
> +
> +	return dev;
> +}
> +
> +/* ----------------------------------------------------------------------------
> + * IPU3 pipeline configuration
> + */
> +
> +/**
> + * \brief Initialize and configure components of the ImgU instance
> + *
> + * Create and open the devices and subdevices in the ImgU instance.
> + * This methods configures the ImgU instance for capture operations, and
> + * should be called at stream configuration time.
> + *
> + * \todo Expand the ImgU configuration with controls setting
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -ENODEV Failed to open one of the video devices or subdevices
> + */
> +int PipelineHandlerIPU3::initImgU(ImgUDevice *imgu)
> +{
> +	imgu->imgu = openSubdevice(imgu->mediaDevice_, imgu->imguName_);
> +	if (!imgu->imgu)
> +		return -ENODEV;
> +
> +	imgu->input = openDevice(imgu->mediaDevice_,
> +				 imgu->imguName_ + " input");
> +	if (!imgu->input)
> +		goto error_delete_imgu;
> +
> +	imgu->output = openDevice(imgu->mediaDevice_,
> +				  imgu->imguName_ + " output");
> +	if (!imgu->output)
> +		goto error_delete_input;
> +
> +	imgu->viewfinder = openDevice(imgu->mediaDevice_,
> +				      imgu->imguName_ + " viewfinder");
> +	if (!imgu->viewfinder)
> +		goto error_delete_output;
> +
> +	imgu->stat = openDevice(imgu->mediaDevice_,
> +				imgu->imguName_ + " 3a stat");
> +	if (!imgu->stat)
> +		goto error_delete_vf;
> +
> +	return 0;
> +
> +error_delete_vf:
> +	delete imgu->viewfinder;
> +error_delete_output:
> +	delete imgu->output;
> +error_delete_input:
> +	delete imgu->input;
> +error_delete_imgu:
> +	delete imgu->imgu;

Let's simplify this by removing the need for this code. The caller can
just propagate the error up, the match function will fail, and the imgu
instances will be destroyed.

> +
> +	return -ENODEV;
> +}
> +
>  /**
>   * \brief Initialize components of the CIO2 device \a index used by a camera
>   * \param index The CIO2 device index
> @@ -458,23 +642,12 @@ int PipelineHandlerIPU3::initCIO2(unsigned int index, CIO2Device *cio2)
>  	if (ret)
>  		goto error_delete_csi2;
>  
> -	entity = cio2MediaDev_->getEntityByName(cio2Name);
> -	if (!entity) {
> -		LOG(IPU3, Error)
> -			<< "Failed to get entity '" << cio2Name << "'";
> -		ret = -EINVAL;
> +	cio2->output = openDevice(cio2MediaDev_.get(), cio2Name);
> +	if (!cio2->output)
>  		goto error_delete_csi2;
> -	}
> -
> -	cio2->output = new V4L2Device(entity);
> -	ret = cio2->output->open();
> -	if (ret)
> -		goto error_delete_output;
>  
>  	return 0;
>  
> -error_delete_output:
> -	delete cio2->output;
>  error_delete_csi2:
>  	delete cio2->csi2;
>  error_delete_sensor:
> @@ -483,6 +656,16 @@ error_delete_sensor:
>  	return ret;
>  }
>  
> +/**
> + * \brief Delete all devices associated with a CIO2 unit
> + */
> +void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2)
> +{
> +	delete cio2->output;
> +	delete cio2->csi2;
> +	delete cio2->sensor;
> +}

I don't think this is needed for the reason explained above.

> +
>  /*
>   * Cameras are created associating an image sensor (represented by a
>   * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
> @@ -495,7 +678,7 @@ void PipelineHandlerIPU3::registerCameras()
>  	 * image sensor is connected to it.
>  	 */
>  	unsigned int numCameras = 0;
> -	for (unsigned int id = 0; id < 4; ++id) {
> +	for (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {
>  		std::unique_ptr<IPU3CameraData> data =
>  			utils::make_unique<IPU3CameraData>(this);
>  		std::set<Stream *> streams{ &data->stream_ };
> @@ -506,6 +689,18 @@ void PipelineHandlerIPU3::registerCameras()
>  		if (ret)
>  			continue;
>  
> +		/**
> +		 * \todo Dynamically assign ImgU devices; as of now, limit
> +		 * support to two cameras only, and assign imgu0 to the first
> +		 * one and imgu1 to the second.
> +		 */
> +		data->imgu = &imgus_[numCameras];
> +		ret = initImgU(data->imgu);

The imgus are separate from the cameras, please initialize them before
this loop, and return an error if initialization fails.

> +		if (ret) {
> +			deleteCIO2(cio2);
> +			continue;
> +		}
> +
>  		std::string cameraName = cio2->sensor->deviceName() + " "
>  				       + std::to_string(id);
>  		std::shared_ptr<Camera> camera = Camera::create(this,

-- 
Regards,

Laurent Pinchart