[libcamera-devel,5/5] libcamera: ipu3: Add IMGUDevice class

593 diff mbox series

Add an IMGUDevice class that represents all devices associated with the
IPU3 imgu unit.

Imgu units will be freely assigned to cameras, depending on the number of
streams that are required. Add two imgu instances to the ipu3 pipeline,
and provide methods to configure the format on the imgu internal
components.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/imgu.cpp    | 338 ++++++++++++++++++++++++
 src/libcamera/pipeline/ipu3/ipu3.cpp    |  71 +++++
 src/libcamera/pipeline/ipu3/ipu3.h      |  34 +++
 src/libcamera/pipeline/ipu3/meson.build |   1 +
 4 files changed, 444 insertions(+)
 create mode 100644 src/libcamera/pipeline/ipu3/imgu.cpp

Hi Jacopo,

Thank you for the patch.

On Wed, Feb 20, 2019 at 02:17:57PM +0100, Jacopo Mondi wrote:
> Add an IMGUDevice class that represents all devices associated with the
> IPU3 imgu unit.

s/imgu/ImgU/

> 
> Imgu units will be freely assigned to cameras, depending on the number of

s/Imgu/ImgU/

> streams that are required. Add two imgu instances to the ipu3 pipeline,

Same here and everywhere else, and s/ipu3/IPU3/

> and provide methods to configure the format on the imgu internal
> components.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/imgu.cpp    | 338 ++++++++++++++++++++++++
>  src/libcamera/pipeline/ipu3/ipu3.cpp    |  71 +++++
>  src/libcamera/pipeline/ipu3/ipu3.h      |  34 +++
>  src/libcamera/pipeline/ipu3/meson.build |   1 +
>  4 files changed, 444 insertions(+)
>  create mode 100644 src/libcamera/pipeline/ipu3/imgu.cpp
> 
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> new file mode 100644
> index 000000000000..7945764eb989
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -0,0 +1,338 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * imgu.cpp - IPU3 IMGU unit
> + */
> +
> +#include <string>
> +#include <vector>
> +
> +#include <linux/media-bus-format.h>
> +
> +#include "geometry.h"
> +#include "ipu3.h"
> +#include "media_device.h"
> +#include "v4l2_device.h"
> +#include "v4l2_subdevice.h"
> +
> +/*
> + * IMGUDevice represents one of the two 'IMGU' units the Intel IPU3 ISP

How about naming the class ImgUDevice ?

> + * provides.
> + *
> + * IMGU units are fed with image data captured from a CIO2 instance and with
> + * configuration parameters for 3A tuning.
> + *
> + * Each IMGU handles 2 video pipes at the time, and each pipe provide 2 image

Did you mean "at the same time" ?

s/provide/provides/

> + * outputs ('output' and 'viewfinder') and one statistics metadata output.
> + *
> + * TODO: support concurrent pipes
> + * TODO: support multiple video capture streams (output + viewfinder)
> + */
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPU3)
> +
> +IMGUDevice::IMGUDevice()
> +	: owner_(nullptr), imgu_(nullptr), input_(nullptr),
> +	  output_(nullptr), viewfinder_(nullptr), stat_(nullptr)
> +{
> +}
> +
> +IMGUDevice::~IMGUDevice()
> +{
> +	delete imgu_;
> +	delete input_;
> +	delete output_;
> +	delete viewfinder_;
> +	delete stat_;
> +}
> +
> +int IMGUDevice::open(unsigned int index, MediaDevice *imguMediaDev)

Same comment as for CIO2Device, here and through this function.

> +{
> +	int ret;
> +
> +	std::string imguName = "ipu3-imgu " + std::to_string(index);
> +	MediaEntity *entity = imguMediaDev->getEntityByName(imguName);
> +	if (!entity) {
> +		LOG(IPU3, Error) << "Failed to get entity '" << imguName << "'";
> +		return -ENODEV;
> +	}
> +
> +	imgu_ = new V4L2Subdevice(entity);
> +	ret = imgu_->open();
> +	if (ret)
> +		return ret;
> +
> +	std::string inputName = imguName + " input";
> +	entity = imguMediaDev->getEntityByName(inputName);
> +	if (!entity) {
> +		LOG(IPU3, Error) << "Failed to get entity '" << inputName << "'";
> +		return -ENODEV;
> +	}
> +
> +	input_ = new V4L2Device(entity);
> +	ret = input_->open();
> +	if (ret)
> +		return ret;
> +
> +	std::string outputName = imguName + " output";
> +	entity = imguMediaDev->getEntityByName(outputName);
> +	if (!entity) {
> +		LOG(IPU3, Error) << "Failed to get entity '" << outputName << "'";
> +		return -ENODEV;
> +	}
> +
> +	output_ = new V4L2Device(entity);
> +	ret = output_->open();
> +	if (ret)
> +		return ret;
> +
> +	std::string viewfinderName = imguName + " viewfinder";
> +	entity = imguMediaDev->getEntityByName(viewfinderName);
> +	if (!entity) {
> +		LOG(IPU3, Error) << "Failed to get entity '"
> +				 << viewfinderName << "'";
> +		return -ENODEV;
> +	}
> +
> +	viewfinder_ = new V4L2Device(entity);
> +	ret = viewfinder_->open();
> +	if (ret)
> +		return ret;
> +
> +	std::string statName = imguName + " 3a stat";
> +	entity = imguMediaDev->getEntityByName(statName);
> +	if (!entity) {
> +		LOG(IPU3, Error) << "Failed to get entity '"
> +				 << statName << "'";
> +		return -ENODEV;
> +	}
> +
> +	stat_ = new V4L2Device(entity);
> +	ret = stat_->open();
> +	if (ret)
> +		return ret;
> +
> +	/* Link entities to configure the IMGU unit for capture. */
> +	MediaLink *link = imguMediaDev->link(inputName, 0, imguName, 0);

Shouldn't you use the constants you defined for pad numbers ?

> +	if (!link) {
> +		LOG(IPU3, Error)
> +			<< "Failed to get link '" << inputName << "':0 -> '"
> +			<< imguName << "':0";
> +		return -EINVAL;
> +	}
> +	link->setEnabled(true);
> +
> +	link = imguMediaDev->link(imguName, 2, outputName, 0);
> +	if (!link) {
> +		LOG(IPU3, Error)
> +			<< "Failed to get link '" << imguName << "':2 -> '"
> +			<< outputName << "':0";
> +		return -EINVAL;
> +	}
> +	link->setEnabled(true);
> +
> +	link = imguMediaDev->link(imguName, 3, viewfinderName, 0);
> +	if (!link) {
> +		LOG(IPU3, Error)
> +			<< "Failed to get link '" << imguName << "':3 -> '"
> +			<< viewfinderName << "':0";
> +		return -EINVAL;
> +	}
> +	link->setEnabled(true);
> +
> +	link = imguMediaDev->link(imguName, 4, statName, 0);
> +	if (!link) {
> +		LOG(IPU3, Error)
> +			<< "Failed to get link '" << imguName << "':4 -> '"
> +			<< statName << "':0";
> +		return -EINVAL;
> +	}
> +	link->setEnabled(true);
> +
> +	return 0;
> +}
> +
> +int IMGUDevice::configure(const IPU3DeviceFormat &inputFormat,
> +			  const IPU3DeviceFormat &outputFormat)
> +{
> +	int ret;
> +
> +	Rectangle selectionRectangle = {};

Maybe just rectangle or rect ? Let's not use long names needlessly.

> +	selectionRectangle.x = 0;
> +	selectionRectangle.y = 0;
> +	selectionRectangle.w = inputFormat.width;
> +	selectionRectangle.h = inputFormat.height;

Can't you initialize the rectangle when declaring it ?

> +
> +	ret = imgu_->setCrop(IMGU_PAD_INPUT, selectionRectangle);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< "'" << imgu_->deviceName() << "':" << IMGU_PAD_INPUT << " = "
> +		<< "crop: (0,0)/" << selectionRectangle.w << "x"
> +		<< selectionRectangle.h;
> +
> +	ret = imgu_->setCompose(IMGU_PAD_INPUT, selectionRectangle);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< "'" << imgu_->deviceName() << "':" << IMGU_PAD_INPUT << " = "
> +		<< "compose: (0,0)/" << selectionRectangle.w << "x"
> +		<< selectionRectangle.h;
> +
> +	V4L2SubdeviceFormat subdevFormat = {};
> +	ret = imgu_->getFormat(IMGU_PAD_INPUT, &subdevFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< "'" << imgu_->deviceName() << "':" << IMGU_PAD_INPUT << " = "
> +		<< subdevFormat.width << "x" << subdevFormat.height << " - "
> +		<< std::hex << subdevFormat.mbus_code;
> +
> +	/*
> +	 * Apply image format to the 'imgu' unit subdevices.
> +	 *
> +	 * FIXME: the IPU3 driver implementation shall be changed to use the
> +	 * actual input sizes as 'imgu input' subdevice sizes, and use the
> +	 * desired output sizes to configure the crop/compose rectangles.  The
> +	 * current implementation uses output sizes as 'imgu input' sizes, and
> +	 * uses the input dimension to configure the crop/compose rectangles,
> +	 * which contradicts the V4L2 specifications.
> +	 */
> +	subdevFormat = {};
> +	subdevFormat.width = outputFormat.width;
> +	subdevFormat.height = outputFormat.height;
> +	subdevFormat.mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +
> +	ret = imgu_->setFormat(IMGU_PAD_INPUT, &subdevFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< "'" << imgu_->deviceName() << "':" << IMGU_PAD_INPUT << " = "
> +		<< subdevFormat.width << "x" << subdevFormat.height << " - "
> +		<< std::hex << subdevFormat.mbus_code;
> +
> +	/*
> +	 * Configure the 'imgu output' and 'imgu 3a stat' pads with the
> +	 * desired output image sizes.
> +	 */
> +	subdevFormat = {};
> +	subdevFormat.width = outputFormat.width;;
> +	subdevFormat.height = outputFormat.height;
> +	subdevFormat.mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +
> +	ret = imgu_->setFormat(IMGU_PAD_OUTPUT, &subdevFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< "'" << imgu_->deviceName() << "':" << IMGU_PAD_OUTPUT << " = "
> +		<< subdevFormat.width << "x" << subdevFormat.height << " - "
> +		<< std::hex << subdevFormat.mbus_code;
> +
> +	subdevFormat = {};
> +	subdevFormat.width = outputFormat.width;;
> +	subdevFormat.height = outputFormat.height;
> +	subdevFormat.mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +
> +	ret = imgu_->setFormat(IMGU_PAD_STAT, &subdevFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< "'" << imgu_->deviceName() << "':" << IMGU_PAD_STAT << " = "
> +		<< subdevFormat.width << "x" << subdevFormat.height << " - "
> +		<< std::hex << subdevFormat.mbus_code;
> +
> +	/*
> +	 * FIXME: Set the 'imgu viewfinder' to hardcoded size
> +	 * It shall be configured as secondary stream when support for
> +	 * multiple streams is added.
> +	 */
> +	subdevFormat = {};
> +	subdevFormat.width = outputFormat.width / 2;
> +	subdevFormat.height = outputFormat.height / 2;
> +	subdevFormat.mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +
> +	ret = imgu_->setFormat(IMGU_PAD_VF, &subdevFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< "'" << imgu_->deviceName() << "':" << IMGU_PAD_VF << " = "
> +		<< subdevFormat.width << "x" << subdevFormat.height << " - "
> +		<< std::hex << subdevFormat.mbus_code;
> +
> +	/*
> +	 * Apply the CIO2 provided 'inputFormat' to the 'imgu input' video
> +	 * device, and the requested 'outputFormat' to the to the 'imgu output'
> +	 * 'imgu viewfinder' and 'imgu 3a stat' nodes.
> +	 *
> +	 * FIXME: Keep the viewfinder size reduced.
> +	 */
> +	V4L2DeviceFormat devFormat = {};
> +	devFormat.width = inputFormat.width;
> +	devFormat.height = inputFormat.height;
> +	devFormat.fourcc = inputFormat.fourcc;
> +
> +	ret = input_->setFormat(&devFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< "'" << input_->driverName() << "': "
> +		<< devFormat.width << "x" << devFormat.height
> +		<< "- 0x" << std::hex << devFormat.fourcc << " planes: "
> +		<< devFormat.planesCount;
> +
> +	devFormat = {};
> +	devFormat.width = outputFormat.width;
> +	devFormat.height = outputFormat.height;
> +	devFormat.fourcc = outputFormat.fourcc;
> +	V4L2DeviceFormat tmpFormat = devFormat;
> +
> +	ret = output_->setFormat(&tmpFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< "'" << output_->driverName() << "': "
> +		<< tmpFormat.width << "x" << tmpFormat.height
> +		<< "- 0x" << std::hex << tmpFormat.fourcc << " planes: "
> +		<< tmpFormat.planesCount;
> +
> +	tmpFormat = devFormat;
> +	ret = stat_->setFormat(&tmpFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< "'" << stat_->driverName() << "': "
> +		<< tmpFormat.width << "x" << tmpFormat.height
> +		<< "- 0x" << std::hex << tmpFormat.fourcc << " planes: "
> +		<< tmpFormat.planesCount;
> +
> +	tmpFormat = devFormat;
> +	tmpFormat.width = devFormat.width / 2;
> +	tmpFormat.height = devFormat.height / 2;
> +
> +	ret = viewfinder_->setFormat(&tmpFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< "'" << viewfinder_->driverName() << "': "
> +		<< tmpFormat.width << "x" << tmpFormat.height
> +		<< "- 0x" << std::hex << tmpFormat.fourcc << " planes: "
> +		<< tmpFormat.planesCount;
> +
> +	return 0;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 5fcdd6335db6..8b4c88048f6c 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -64,6 +64,7 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>  	Stream *stream = &cameraData(camera)->stream_;
>  	StreamConfiguration *cfg = &config[stream];
>  	IPU3DeviceFormat outputFormat = {};
> +	IMGUDevice *imgu;
>  	int ret;
>  
>  	/* Validate the requested image format and resolution. */
> @@ -75,10 +76,46 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>  	outputFormat.height = cfg->height;
>  	outputFormat.fourcc = cfg->pixelFormat;
>  
> +	/*
> +	 * FIXME: as of now, a single stream is supported, so we can claim
> +	 * the first available imgu unit, if any. When multiple streams per
> +	 * camera will be supported, a single camera could use both the imgu

s/both the imgu units/both ImgUs/ (U stands for unit)

> +	 * units and prevent other cameras to be used.

s/to be used/from being used/

> +	 */
> +	if (imgu0_.available())
> +		imgu = &imgu0_;
> +	else if (imgu1_.available())
> +		imgu = &imgu1_;
> +	else
> +		return -EBUSY;

You can just use imgu0_ unconditionally for now.

> +	/*
> +	 * FIXME: the imgu unit shall be released when streams gets released.
> +	 * How to deal with cross-camera dependencies ?
> +	 */
> +	imgu->acquire(camera);
> +
> +	/*
> +	 * Configure the CIO2 unit to provide images in a resolution that
> +	 * satisfies the desired output format.
> +	 */
>  	ret = cio2->configure(outputFormat);
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Get the CIO2 output format and apply it to the IMGU input and
> +	 * apply the output format to the IMGU output.
> +	 *
> +	 * Hardcode the fourcc code output from CIO2 to IMGU.
> +	 */
> +	IPU3DeviceFormat inputFormat = cio2->format();
> +	inputFormat.fourcc = V4L2_PIX_FMT_IPU3_SGRBG10;

Should cio2->format() provide the right fourcc already ?

> +
> +	ret = imgu->configure(inputFormat, outputFormat);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  
> @@ -205,12 +242,22 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
>  	if (cio2_->disableLinks())
>  		goto error_close_cio2;
>  
> +	if (imgu_->open())
> +		goto error_close_cio2;
> +
>  	registerCameras();
>  
> +	if (registerImgus())
> +		goto error_close_imgu;
> +
>  	cio2_->close();
> +	imgu_->close();
>  
>  	return true;
>  
> +error_close_imgu:
> +	imgu_->close();
> +
>  error_close_cio2:
>  	cio2_->close();

You can close an unopened device, so let's go for a single error label,
it makes code simpler.

>  
> @@ -221,6 +268,30 @@ error_release_mdev:
>  	return false;
>  }
>  
> +/*
> + * IPU3 has two 'imgu' units which can be freely assigned to cameras;

s/has two 'imgu' units which/two ImgUs that/
s/;/./

> + *
> + * Create instances for both the units, creating video devices and subdevices

s/both the units/both units/

> + * associated with an instance.

You open the instances, you don't create them.

> + */
> +
> +int PipelineHandlerIPU3::registerImgus()

The ImgUs are not registered anywhere, this function should be renamed.

> +{
> +	int ret = imgu0_.open(0, imgu_.get());
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to open 'imgu0' unit";
> +		return ret;
> +	}
> +
> +	ret = imgu1_.open(1, imgu_.get());
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to open 'imgu1' unit";
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Cameras are created associating an image sensor (represented by a
>   * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.h b/src/libcamera/pipeline/ipu3/ipu3.h
> index 2a8b6f13b1c7..83d6e7cf27db 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.h
> +++ b/src/libcamera/pipeline/ipu3/ipu3.h
> @@ -57,6 +57,36 @@ public:
>  	IPU3DeviceFormat cio2Format_;
>  };
>  
> +class IMGUDevice
> +{
> +public:
> +	static constexpr unsigned int IMGU_PAD_INPUT = 0;
> +	static constexpr unsigned int IMGU_PAD_OUTPUT = 2;
> +	static constexpr unsigned int IMGU_PAD_VF = 3;
> +	static constexpr unsigned int IMGU_PAD_STAT = 4;

I wonder if static is needed here.

> +
> +	IMGUDevice();
> +	IMGUDevice(const IMGUDevice &) = delete;

Should you delete the copy assignement operator too ?

> +	~IMGUDevice();
> +
> +	Camera *owner_;
> +	bool available() const { return owner_ == nullptr; }
> +	void acquire(Camera *camera) { owner_ = camera; }
> +	void release() { owner_ = nullptr; }

We don't know yet how we will deal with ownership, so for now I'd drop
this.

> +
> +	int open(unsigned int index, MediaDevice *imguMediaDev);

Missing close().

> +
> +	int configure(const IPU3DeviceFormat &cio2Format,
> +		      const IPU3DeviceFormat &imguFormat);
> +
> +	V4L2Subdevice *imgu_;
> +	V4L2Device *input_;
> +	V4L2Device *output_;
> +	V4L2Device *viewfinder_;
> +	V4L2Device *stat_;
> +	/* TODO: add param video device for 3A tuning */
> +};
> +
>  class PipelineHandlerIPU3 : public PipelineHandler
>  {
>  public:
> @@ -95,10 +125,14 @@ private:
>  			PipelineHandler::cameraData(camera));
>  	}
>  
> +	int registerImgus();
>  	void registerCameras();
>  
>  	std::shared_ptr<MediaDevice> cio2_;
>  	std::shared_ptr<MediaDevice> imgu_;
> +
> +	IMGUDevice imgu0_;
> +	IMGUDevice imgu1_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build
> index fcb2d319d517..67f1a5e656d9 100644
> --- a/src/libcamera/pipeline/ipu3/meson.build
> +++ b/src/libcamera/pipeline/ipu3/meson.build
> @@ -1,4 +1,5 @@
>  libcamera_sources += files([
>      'cio2.cpp',
> +    'imgu.cpp',
>      'ipu3.cpp',
>  ])