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

Message ID 20190220131757.14004-5-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: IPU3: create CIO2 and IMGU devices
Related show

Commit Message

Jacopo Mondi Feb. 20, 2019, 1:17 p.m. UTC
Add a CIO2Device class that represents the CIO2 unit associated with a
Camera. Implement image format negotiation in the CIO2Device and create
a CIO2Device instance for each camera created by the IPU3 pipeline
handler.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/cio2.cpp    | 266 ++++++++++++++++++++++++
 src/libcamera/pipeline/ipu3/ipu3.cpp    | 158 +++-----------
 src/libcamera/pipeline/ipu3/ipu3.h      |  51 +++--
 src/libcamera/pipeline/ipu3/meson.build |   1 +
 4 files changed, 335 insertions(+), 141 deletions(-)
 create mode 100644 src/libcamera/pipeline/ipu3/cio2.cpp

Comments

Laurent Pinchart Feb. 22, 2019, 12:09 a.m. UTC | #1
Hi Jacopo,

Thank you for the patch.

On Wed, Feb 20, 2019 at 02:17:56PM +0100, Jacopo Mondi wrote:
> Add a CIO2Device class that represents the CIO2 unit associated with a
> Camera. Implement image format negotiation in the CIO2Device and create
> a CIO2Device instance for each camera created by the IPU3 pipeline
> handler.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp    | 266 ++++++++++++++++++++++++
>  src/libcamera/pipeline/ipu3/ipu3.cpp    | 158 +++-----------
>  src/libcamera/pipeline/ipu3/ipu3.h      |  51 +++--
>  src/libcamera/pipeline/ipu3/meson.build |   1 +
>  4 files changed, 335 insertions(+), 141 deletions(-)
>  create mode 100644 src/libcamera/pipeline/ipu3/cio2.cpp
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> new file mode 100644
> index 000000000000..b0eeff987f64
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -0,0 +1,266 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * cio2.cpp - IPU3 CIO2 unit
> + */
> +
> +#include <string>
> +#include <vector>
> +
> +#include "ipu3.h"
> +#include "media_device.h"
> +#include "v4l2_device.h"
> +#include "v4l2_subdevice.h"
> +
> +/*
> + * CIO2Device represents one of the four 'CIO2' units the Intel IPU3 ISP

s/ISP //

> + * provides.
> + *
> + * Each CIO2 unit has associated one image sensor, which provides RAW image

s/has associated/is associated with/ (or "has one image sensor
associated to it")

> + * data to be processed by one of the IPU3's IMGU units.

s/IMGU/ImgU/

> + */
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPU3)
> +
> +CIO2Device::CIO2Device()
> +	: cio2_(nullptr), csi2_(nullptr), sensor_(nullptr)
> +{
> +}
> +
> +CIO2Device::~CIO2Device()
> +{
> +	delete cio2_;
> +	delete csi2_;
> +	delete sensor_;
> +}
> +
> +/*
> + * Create and open the video device and video subdevices associated with
> + * this CIO2 unit.

Could you document he parameters too ? I'd format this using the Doxygen
format.

> + */
> +int CIO2Device::open(unsigned int index, MediaDevice *cio2MediaDev)

I would name the second parameter mediaDevice as I don't think it could
be confused with another media device. I would also swap the two
parameters, to have the more generic first and the more specific last.

> +{
> +	int ret;
> +
> +	std::string cio2Name = "ipu3-cio2 " + std::to_string(index);
> +	MediaEntity *cio2Entity = cio2MediaDev->getEntityByName(cio2Name);
> +	if (!cio2Entity) {
> +		LOG(IPU3, Error)
> +			<< "Failed to get entity '" << cio2Name << "'";
> +		return -EINVAL;

Maybe -ENODEV here and below, to indicate the CIO2 device isn't found ?

> +	}
> +
> +	std::string csi2Name = "ipu3-csi2 " + std::to_string(index);
> +	MediaEntity *csi2Entity = cio2MediaDev->getEntityByName(csi2Name);
> +	if (!csi2Entity) {
> +		LOG(IPU3, Error)
> +			<< "Failed to get entity '" << csi2Name << "'";
> +		return -EINVAL;
> +	}
> +
> +	const std::vector<MediaPad *> &pads = csi2Entity->pads();
> +	if (pads.empty())
> +		return -EINVAL;
> +
> +	/* IPU3 CSI-2 receivers have a single sink pad at index 0. */
> +	MediaPad *sink = pads[0];
> +	const std::vector<MediaLink *> &links = sink->links();
> +	if (links.empty())
> +		return -EINVAL;
> +
> +	/*
> +	 * Verify that the CSI-2 receiver is connected to a sensor and enable
> +	 * the media link between the two.
> +	 */
> +	MediaLink *link = links[0];
> +	MediaEntity *sensorEntity = link->source()->entity();
> +	if (sensorEntity->function() != MEDIA_ENT_F_CAM_SENSOR)
> +		return -EINVAL;
> +
> +	if (link->setEnabled(true))
> +		return -EINVAL;
> +
> +	/*
> +	 * Create and open video devices and subdevices associated with
> +	 * the camera.
> +	 */
> +	cio2_ = new V4L2Device(cio2Entity);
> +	ret = cio2_->open();
> +	if (ret)
> +		return ret;
> +
> +	sensor_ = new V4L2Subdevice(sensorEntity);
> +	ret = sensor_->open();
> +	if (ret)
> +		return ret;
> +
> +	csi2_ = new V4L2Subdevice(csi2Entity);
> +	ret = csi2_->open();
> +	if (ret)
> +		return ret;

I would call close() for the last two errors (or possibly even the last
three for symmetry, it won't hurt).

> +
> +	return 0;
> +}
> +
> +int CIO2Device::sensorSetFormat(unsigned int width, unsigned int height,
> +				V4L2SubdeviceFormat *format)

How about just setFormat() ?

> +{
> +	unsigned int best = ~0;
> +	bool found = false;
> +	int ret;
> +
> +	ret = sensor_->getFormat(0, format);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Enumerate the sensor formats until a matching one is found.
> +	 * Re-use the default media_bus pixel format as it is configured
> +	 * in the sensor.
> +	 */

I think this isn't the right layer to perform these operations. The code
should be moved to the IPU3 pipeline handler that will enumerate all
resolutions supported by the sensor and decide what to offer to
applications, taking into account cropping and scaling capabilities of
the ImgU. This class shouldn't try to be smart.

> +	V4L2SubdeviceFormatEnum bestEnum = {};
> +	V4L2SubdeviceFormatEnum formatEnum = {};
> +	formatEnum.index = 0;
> +	formatEnum.mbus_code = format->mbus_code;
> +
> +	while (true) {
> +		unsigned int diff;
> +
> +		int ret = sensor_->enumFormat(0, &formatEnum);
> +		if (ret)
> +			break;
> +
> +		diff = (abs(formatEnum.minWidth - width) +
> +			abs(formatEnum.minHeight - height));
> +
> +		if (formatEnum.minWidth >= width &&
> +		    formatEnum.minHeight >= height &&
> +		    diff < best) {
> +			best = diff;
> +			bestEnum = formatEnum;
> +			found = true;
> +		}
> +
> +		LOG(IPU3, Debug)
> +			<< "Enumerate image sizes : " << formatEnum.index
> +			<< " = " << formatEnum.minWidth << "x"
> +			<< formatEnum.minHeight
> +			<< "Diff " << diff << " Best " << best;
> +
> +		formatEnum.index++;
> +	}
> +	if (!found) {
> +		LOG(IPU3, Error)
> +			<< "Failed to find image resolution for: "
> +			<< width << "x" << height;
> +
> +		return -EINVAL;
> +	}
> +
> +	format->width = bestEnum.minWidth;
> +	format->height = bestEnum.minHeight;
> +	ret = sensor_->setFormat(0, format);
> +	if (ret)
> +		return ret;
> +
> +	/* Make sure everything is all right. */
> +	if (format->width < width || format->height < height) {
> +		LOG(IPU3, Error)
> +			<< "Failed to find image resolution for: "
> +			<< width << "x" << height << " - Got : "
> +			<< format->width << "x" << format->height
> +			<< " instead";
> +
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Configure the CIO2 unit to provide big enough images to satisfy
> + * the requested width and height.

Same thing here, the IPU3 pipeline handler should make the smart
decisions, and request an exact resolution that the CIO2 can provide.

> + *
> + * The images captured from the CIO2 unit will be fed to an IMGU unit that
> + * can scale and crop on them to obtain the desired output sizes.
> + */
> +int CIO2Device::configure(const IPU3DeviceFormat &format)
> +{
> +	V4L2SubdeviceFormat subdevFormat = {};
> +	V4L2DeviceFormat devFormat = {};
> +	int ret;
> +
> +	ret = sensorSetFormat(format.width, format.height, &subdevFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< "'" << sensor_->deviceName() << "':0 = "
> +		<< subdevFormat.width << "x" << subdevFormat.height << " - "
> +		<< std::hex << subdevFormat.mbus_code;
> +
> +	/* Propagate the format along the pipeline. */
> +	ret = csi2_->setFormat(0, &subdevFormat);
> +	if (ret)
> +		return ret;
> +
> +	ret = cio2_->getFormat(&devFormat);
> +	if (ret)
> +		return ret;
> +
> +	devFormat.width = subdevFormat.width;
> +	devFormat.height = subdevFormat.height;
> +	devFormat.fourcc = V4L2_PIX_FMT_IPU3_SGRBG10;
> +
> +	ret = cio2_->setFormat(&devFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug)
> +		<< cio2_->driverName() << ": "
> +		<< devFormat.width << "x" << devFormat.height
> +		<< "- 0x" << std::hex << devFormat.fourcc << " planes: "
> +		<< devFormat.planesCount;
> +
> +	/* Store the applied format in the cio2Format_ member for re-use. */
> +	cio2Format_.width = devFormat.width;
> +	cio2Format_.height = devFormat.height;
> +	cio2Format_.fourcc = devFormat.fourcc;
> +
> +	return 0;
> +}
> +
> +const IPU3DeviceFormat &CIO2Device::format() const
> +{
> +	return cio2Format_;
> +}
> +
> +int CIO2Device::exportBuffers(BufferPool *pool)
> +{
> +	return cio2_->exportBuffers(pool);
> +}
> +
> +int CIO2Device::releaseBuffers()
> +{
> +	return cio2_->releaseBuffers();
> +}
> +
> +int CIO2Device::queueBuffer(Buffer *buffer)
> +{
> +	return cio2_->queueBuffer(buffer);
> +}
> +
> +int CIO2Device::streamOn()
> +{
> +	return cio2_->streamOn();
> +}
> +
> +int CIO2Device::streamOff()
> +{
> +	return cio2_->streamOff();
> +}

This almost makes me feel you should inherit from V4L2Device.

> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 07029dd763c9..5fcdd6335db6 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -60,74 +60,37 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,
>  int PipelineHandlerIPU3::configureStreams(Camera *camera,
>  					  std::map<Stream *, StreamConfiguration> &config)
>  {
> -	IPU3CameraData *data = cameraData(camera);
> -	StreamConfiguration *cfg = &config[&data->stream_];
> -	V4L2Subdevice *sensor = data->sensor_;
> -	V4L2Subdevice *csi2 = data->csi2_;
> -	V4L2Device *cio2 = data->cio2_;
> -	V4L2SubdeviceFormat subdevFormat = {};
> -	V4L2DeviceFormat devFormat = {};
> +	CIO2Device *cio2 = &cameraData(camera)->cio2Unit_;
> +	Stream *stream = &cameraData(camera)->stream_;
> +	StreamConfiguration *cfg = &config[stream];
> +	IPU3DeviceFormat outputFormat = {};
>  	int ret;
>  
> -	/*
> -	 * FIXME: as of now, the format gets applied to the sensor and is
> -	 * propagated along the pipeline. It should instead be applied on the
> -	 * capture device and the sensor format calculated accordingly.
> -	 */
> -
> -	ret = sensor->getFormat(0, &subdevFormat);
> -	if (ret)
> -		return ret;
> -
> -	subdevFormat.width = cfg->width;
> -	subdevFormat.height = cfg->height;
> -	ret = sensor->setFormat(0, &subdevFormat);
> -	if (ret)
> -		return ret;
> -
> -	/* Return error if the requested format cannot be applied to sensor. */
> -	if (subdevFormat.width != cfg->width ||
> -	    subdevFormat.height != cfg->height) {
> -		LOG(IPU3, Error)
> -			<< "Failed to apply image format "
> -			<< subdevFormat.width << "x" << subdevFormat.height
> -			<< " - got: " << cfg->width << "x" << cfg->height;
> +	/* Validate the requested image format and resolution. */
> +	if (cfg->pixelFormat != V4L2_PIX_FMT_NV12) {
> +		LOG(IPU3, Error) << "Image format not supported";
>  		return -EINVAL;
>  	}
> +	outputFormat.width = cfg->width;
> +	outputFormat.height = cfg->height;
> +	outputFormat.fourcc = cfg->pixelFormat;
>  
> -	ret = csi2->setFormat(0, &subdevFormat);
> +	ret = cio2->configure(outputFormat);
>  	if (ret)
>  		return ret;
>  
> -	ret = cio2->getFormat(&devFormat);
> -	if (ret)
> -		return ret;
> -
> -	devFormat.width = subdevFormat.width;
> -	devFormat.height = subdevFormat.height;
> -	devFormat.fourcc = cfg->pixelFormat;
> -
> -	ret = cio2->setFormat(&devFormat);
> -	if (ret)
> -		return ret;
> -
> -	LOG(IPU3, Info) << cio2->driverName() << ": "
> -			<< devFormat.width << "x" << devFormat.height
> -			<< "- 0x" << std::hex << devFormat.fourcc << " planes: "
> -			<< devFormat.planes;
> -
>  	return 0;
>  }
>  
>  int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
>  {
> -	IPU3CameraData *data = cameraData(camera);
>  	const StreamConfiguration &cfg = stream->configuration();
> +	CIO2Device *cio2 = &cameraData(camera)->cio2Unit_;

You could use a reference instead of a pointer.

>  
>  	if (!cfg.bufferCount)
>  		return -EINVAL;
>  
> -	int ret = data->cio2_->exportBuffers(&stream->bufferPool());
> +	int ret = cio2->exportBuffers(&stream->bufferPool());
>  	if (ret) {
>  		LOG(IPU3, Error) << "Failed to request memory";
>  		return ret;
> @@ -138,9 +101,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
>  
>  int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
>  {
> -	IPU3CameraData *data = cameraData(camera);
> +	CIO2Device *cio2 = &cameraData(camera)->cio2Unit_;
>  
> -	int ret = data->cio2_->releaseBuffers();
> +	int ret = cio2->releaseBuffers();
>  	if (ret) {
>  		LOG(IPU3, Error) << "Failed to release memory";
>  		return ret;
> @@ -151,10 +114,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
>  
>  int PipelineHandlerIPU3::start(const Camera *camera)
>  {
> -	IPU3CameraData *data = cameraData(camera);
> +	CIO2Device *cio2 = &cameraData(camera)->cio2Unit_;
>  	int ret;
>  
> -	ret = data->cio2_->streamOn();
> +	ret = cio2->streamOn();
>  	if (ret) {
>  		LOG(IPU3, Info) << "Failed to start camera " << camera->name();
>  		return ret;
> @@ -165,16 +128,16 @@ int PipelineHandlerIPU3::start(const Camera *camera)
>  
>  void PipelineHandlerIPU3::stop(const Camera *camera)
>  {
> -	IPU3CameraData *data = cameraData(camera);
> +	CIO2Device *cio2 = &cameraData(camera)->cio2Unit_;
>  
> -	if (data->cio2_->streamOff())
> +	if (cio2->streamOff())
>  		LOG(IPU3, Info) << "Failed to stop camera " << camera->name();
>  }
>  
>  int PipelineHandlerIPU3::queueRequest(const Camera *camera, Request *request)
>  {
> -	IPU3CameraData *data = cameraData(camera);
> -	Stream *stream = &data->stream_;
> +	CIO2Device *cio2 = &cameraData(camera)->cio2Unit_;
> +	Stream *stream = &cameraData(camera)->stream_;
>  
>  	Buffer *buffer = request->findBuffer(stream);
>  	if (!buffer) {
> @@ -183,7 +146,7 @@ int PipelineHandlerIPU3::queueRequest(const Camera *camera, Request *request)
>  		return -ENOENT;
>  	}
>  
> -	data->cio2_->queueBuffer(buffer);
> +	cio2->queueBuffer(buffer);
>  
>  	return 0;
>  }
> @@ -271,79 +234,22 @@ void PipelineHandlerIPU3::registerCameras()
>  	 */
>  	unsigned int numCameras = 0;
>  	for (unsigned int id = 0; id < 4; ++id) {
> -		std::string csi2Name = "ipu3-csi2 " + std::to_string(id);
> -		MediaEntity *csi2 = cio2_->getEntityByName(csi2Name);
> -		int ret;
> +		std::unique_ptr<IPU3CameraData> data =
> +			utils::make_unique<IPU3CameraData>();
>  
>  		/*
> -		 * This shall not happen, as the device enumerator matched
> -		 * all entities described in the cio2_dm DeviceMatch.
> -		 *
> -		 * As this check is basically free, better stay safe than sorry.
> +		 * If opening the CIO2 unit fails, the Camera instance won't
> +		 * be registered and the 'data' unique pointers goes out of
> +		 * scope and delete the objects they manage.
>  		 */
> -		if (!csi2)
> -			continue;
> -
> -		const std::vector<MediaPad *> &pads = csi2->pads();
> -		if (pads.empty())
> -			continue;
> -
> -		/* IPU3 CSI-2 receivers have a single sink pad at index 0. */
> -		MediaPad *sink = pads[0];
> -		const std::vector<MediaLink *> &links = sink->links();
> -		if (links.empty())
> -			continue;
> -
> -		/*
> -		 * Verify that the receiver is connected to a sensor, enable
> -		 * the media link between the two, and create a Camera with
> -		 * a unique name.
> -		 */
> -		MediaLink *link = links[0];
> -		MediaEntity *sensor = link->source()->entity();
> -		if (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)
> -			continue;
> -
> -		if (link->setEnabled(true))
> +		if (data->cio2Unit_.open(id, cio2_.get()))
>  			continue;
>  
> -		std::unique_ptr<IPU3CameraData> data = utils::make_unique<IPU3CameraData>();
> -
> -		std::string cameraName = sensor->name() + " " + std::to_string(id);
> +		std::string cameraName = data->cio2Unit_.sensor_->deviceName()
> +				       + " " + std::to_string(id);
>  		std::vector<Stream *> streams{ &data->stream_ };
> -		std::shared_ptr<Camera> camera = Camera::create(this, cameraName, streams);
> -
> -		/*
> -		 * Create and open video devices and subdevices associated with
> -		 * the camera.
> -		 *
> -		 * If any of these operations fails, the Camera instance won't
> -		 * be registered. The 'camera' shared pointer and the 'data'
> -		 * unique pointers go out of scope and delete the objects they
> -		 * manage.
> -		 */
> -		std::string cio2Name = "ipu3-cio2 " + std::to_string(id);
> -		MediaEntity *cio2 = cio2_->getEntityByName(cio2Name);
> -		if (!cio2) {
> -			LOG(IPU3, Error)
> -				<< "Failed to get entity '" << cio2Name << "'";
> -			continue;
> -		}
> -
> -		data->cio2_ = new V4L2Device(cio2);
> -		ret = data->cio2_->open();
> -		if (ret)
> -			continue;
> -
> -		data->sensor_ = new V4L2Subdevice(sensor);
> -		ret = data->sensor_->open();
> -		if (ret)
> -			continue;
> -
> -		data->csi2_ = new V4L2Subdevice(csi2);
> -		ret = data->csi2_->open();
> -		if (ret)
> -			continue;
> +		std::shared_ptr<Camera> camera = Camera::create(this, cameraName,
> +								streams);
>  
>  		setCameraData(camera.get(), std::move(data));
>  		registerCamera(std::move(camera));
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.h b/src/libcamera/pipeline/ipu3/ipu3.h
> index 48c2a3e16980..2a8b6f13b1c7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.h
> +++ b/src/libcamera/pipeline/ipu3/ipu3.h
> @@ -23,6 +23,40 @@
>  
>  namespace libcamera {
>  
> +struct IPU3DeviceFormat {
> +	unsigned int width;
> +	unsigned int height;
> +	unsigned int fourcc;
> +};

This doesn't seem very IPU3-specific to me, could we use a more generic
class ?

> +
> +class CIO2Device
> +{
> +public:
> +	CIO2Device();
> +	CIO2Device(const CIO2Device &) = delete;

Should you also delete the copy assignment operator ?

> +	~CIO2Device();
> +
> +	int open(unsigned int index, MediaDevice *cio2MediaDev);

How about a close() (that you would call from the destructor) ?

> +	int sensorSetFormat(unsigned int width, unsigned int height,
> +			    V4L2SubdeviceFormat *format);
> +	int configure(const IPU3DeviceFormat &format);
> +	const IPU3DeviceFormat &format() const;
> +
> +	int exportBuffers(BufferPool *pool);
> +	int releaseBuffers();
> +
> +	int queueBuffer(Buffer *buffer);
> +
> +	int streamOn();
> +	int streamOff();
> +
> +	V4L2Device *cio2_;
> +	V4L2Subdevice *csi2_;
> +	V4L2Subdevice *sensor_;
> +
> +	IPU3DeviceFormat cio2Format_;

Should the last four members be private ? Otherwise the cio2_ operations
wrappers are a bit useless. As this is an internal class I'm tempted to
keep the members public and remove the wrappers.

Given the above comments regarding handling the smart decisions in the
IPU3 pipeline handler, this class will become a bit of an empty shell, I
wonder if it's really worth splitting it. You may want to instead expand
the IPU3CameraData() class a bit, and add a cio2 initialization function
for instance, containing more or less the code from CIO2Device::open(),
as that should certainly be split out of
PipelineHandlerIPU3::registerCameras().

> +};
> +
>  class PipelineHandlerIPU3 : public PipelineHandler
>  {
>  public:
> @@ -49,19 +83,8 @@ private:
>  	class IPU3CameraData : public CameraData
>  	{
>  	public:
> -		IPU3CameraData()
> -			: cio2_(nullptr), csi2_(nullptr), sensor_(nullptr) {}
> -
> -		~IPU3CameraData()
> -		{
> -			delete cio2_;
> -			delete csi2_;
> -			delete sensor_;
> -		}
> -
> -		V4L2Device *cio2_;
> -		V4L2Subdevice *csi2_;
> -		V4L2Subdevice *sensor_;
> +		/* Each camera has a CIO2 unit associated. */

Same comment as above.

> +		CIO2Device cio2Unit_;
>  
>  		Stream stream_;
>  	};
> @@ -76,8 +99,6 @@ private:
>  
>  	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 0ab766a257a0..fcb2d319d517 100644
> --- a/src/libcamera/pipeline/ipu3/meson.build
> +++ b/src/libcamera/pipeline/ipu3/meson.build
> @@ -1,3 +1,4 @@
>  libcamera_sources += files([
> +    'cio2.cpp',
>      'ipu3.cpp',
>  ])

Patch

diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
new file mode 100644
index 000000000000..b0eeff987f64
--- /dev/null
+++ b/src/libcamera/pipeline/ipu3/cio2.cpp
@@ -0,0 +1,266 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2019, Google Inc.
+ *
+ * cio2.cpp - IPU3 CIO2 unit
+ */
+
+#include <string>
+#include <vector>
+
+#include "ipu3.h"
+#include "media_device.h"
+#include "v4l2_device.h"
+#include "v4l2_subdevice.h"
+
+/*
+ * CIO2Device represents one of the four 'CIO2' units the Intel IPU3 ISP
+ * provides.
+ *
+ * Each CIO2 unit has associated one image sensor, which provides RAW image
+ * data to be processed by one of the IPU3's IMGU units.
+ */
+
+namespace libcamera {
+
+LOG_DECLARE_CATEGORY(IPU3)
+
+CIO2Device::CIO2Device()
+	: cio2_(nullptr), csi2_(nullptr), sensor_(nullptr)
+{
+}
+
+CIO2Device::~CIO2Device()
+{
+	delete cio2_;
+	delete csi2_;
+	delete sensor_;
+}
+
+/*
+ * Create and open the video device and video subdevices associated with
+ * this CIO2 unit.
+ */
+int CIO2Device::open(unsigned int index, MediaDevice *cio2MediaDev)
+{
+	int ret;
+
+	std::string cio2Name = "ipu3-cio2 " + std::to_string(index);
+	MediaEntity *cio2Entity = cio2MediaDev->getEntityByName(cio2Name);
+	if (!cio2Entity) {
+		LOG(IPU3, Error)
+			<< "Failed to get entity '" << cio2Name << "'";
+		return -EINVAL;
+	}
+
+	std::string csi2Name = "ipu3-csi2 " + std::to_string(index);
+	MediaEntity *csi2Entity = cio2MediaDev->getEntityByName(csi2Name);
+	if (!csi2Entity) {
+		LOG(IPU3, Error)
+			<< "Failed to get entity '" << csi2Name << "'";
+		return -EINVAL;
+	}
+
+	const std::vector<MediaPad *> &pads = csi2Entity->pads();
+	if (pads.empty())
+		return -EINVAL;
+
+	/* IPU3 CSI-2 receivers have a single sink pad at index 0. */
+	MediaPad *sink = pads[0];
+	const std::vector<MediaLink *> &links = sink->links();
+	if (links.empty())
+		return -EINVAL;
+
+	/*
+	 * Verify that the CSI-2 receiver is connected to a sensor and enable
+	 * the media link between the two.
+	 */
+	MediaLink *link = links[0];
+	MediaEntity *sensorEntity = link->source()->entity();
+	if (sensorEntity->function() != MEDIA_ENT_F_CAM_SENSOR)
+		return -EINVAL;
+
+	if (link->setEnabled(true))
+		return -EINVAL;
+
+	/*
+	 * Create and open video devices and subdevices associated with
+	 * the camera.
+	 */
+	cio2_ = new V4L2Device(cio2Entity);
+	ret = cio2_->open();
+	if (ret)
+		return ret;
+
+	sensor_ = new V4L2Subdevice(sensorEntity);
+	ret = sensor_->open();
+	if (ret)
+		return ret;
+
+	csi2_ = new V4L2Subdevice(csi2Entity);
+	ret = csi2_->open();
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+int CIO2Device::sensorSetFormat(unsigned int width, unsigned int height,
+				V4L2SubdeviceFormat *format)
+{
+	unsigned int best = ~0;
+	bool found = false;
+	int ret;
+
+	ret = sensor_->getFormat(0, format);
+	if (ret)
+		return ret;
+
+	/*
+	 * Enumerate the sensor formats until a matching one is found.
+	 * Re-use the default media_bus pixel format as it is configured
+	 * in the sensor.
+	 */
+	V4L2SubdeviceFormatEnum bestEnum = {};
+	V4L2SubdeviceFormatEnum formatEnum = {};
+	formatEnum.index = 0;
+	formatEnum.mbus_code = format->mbus_code;
+
+	while (true) {
+		unsigned int diff;
+
+		int ret = sensor_->enumFormat(0, &formatEnum);
+		if (ret)
+			break;
+
+		diff = (abs(formatEnum.minWidth - width) +
+			abs(formatEnum.minHeight - height));
+
+		if (formatEnum.minWidth >= width &&
+		    formatEnum.minHeight >= height &&
+		    diff < best) {
+			best = diff;
+			bestEnum = formatEnum;
+			found = true;
+		}
+
+		LOG(IPU3, Debug)
+			<< "Enumerate image sizes : " << formatEnum.index
+			<< " = " << formatEnum.minWidth << "x"
+			<< formatEnum.minHeight
+			<< "Diff " << diff << " Best " << best;
+
+		formatEnum.index++;
+	}
+	if (!found) {
+		LOG(IPU3, Error)
+			<< "Failed to find image resolution for: "
+			<< width << "x" << height;
+
+		return -EINVAL;
+	}
+
+	format->width = bestEnum.minWidth;
+	format->height = bestEnum.minHeight;
+	ret = sensor_->setFormat(0, format);
+	if (ret)
+		return ret;
+
+	/* Make sure everything is all right. */
+	if (format->width < width || format->height < height) {
+		LOG(IPU3, Error)
+			<< "Failed to find image resolution for: "
+			<< width << "x" << height << " - Got : "
+			<< format->width << "x" << format->height
+			<< " instead";
+
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/*
+ * Configure the CIO2 unit to provide big enough images to satisfy
+ * the requested width and height.
+ *
+ * The images captured from the CIO2 unit will be fed to an IMGU unit that
+ * can scale and crop on them to obtain the desired output sizes.
+ */
+int CIO2Device::configure(const IPU3DeviceFormat &format)
+{
+	V4L2SubdeviceFormat subdevFormat = {};
+	V4L2DeviceFormat devFormat = {};
+	int ret;
+
+	ret = sensorSetFormat(format.width, format.height, &subdevFormat);
+	if (ret)
+		return ret;
+
+	LOG(IPU3, Debug)
+		<< "'" << sensor_->deviceName() << "':0 = "
+		<< subdevFormat.width << "x" << subdevFormat.height << " - "
+		<< std::hex << subdevFormat.mbus_code;
+
+	/* Propagate the format along the pipeline. */
+	ret = csi2_->setFormat(0, &subdevFormat);
+	if (ret)
+		return ret;
+
+	ret = cio2_->getFormat(&devFormat);
+	if (ret)
+		return ret;
+
+	devFormat.width = subdevFormat.width;
+	devFormat.height = subdevFormat.height;
+	devFormat.fourcc = V4L2_PIX_FMT_IPU3_SGRBG10;
+
+	ret = cio2_->setFormat(&devFormat);
+	if (ret)
+		return ret;
+
+	LOG(IPU3, Debug)
+		<< cio2_->driverName() << ": "
+		<< devFormat.width << "x" << devFormat.height
+		<< "- 0x" << std::hex << devFormat.fourcc << " planes: "
+		<< devFormat.planesCount;
+
+	/* Store the applied format in the cio2Format_ member for re-use. */
+	cio2Format_.width = devFormat.width;
+	cio2Format_.height = devFormat.height;
+	cio2Format_.fourcc = devFormat.fourcc;
+
+	return 0;
+}
+
+const IPU3DeviceFormat &CIO2Device::format() const
+{
+	return cio2Format_;
+}
+
+int CIO2Device::exportBuffers(BufferPool *pool)
+{
+	return cio2_->exportBuffers(pool);
+}
+
+int CIO2Device::releaseBuffers()
+{
+	return cio2_->releaseBuffers();
+}
+
+int CIO2Device::queueBuffer(Buffer *buffer)
+{
+	return cio2_->queueBuffer(buffer);
+}
+
+int CIO2Device::streamOn()
+{
+	return cio2_->streamOn();
+}
+
+int CIO2Device::streamOff()
+{
+	return cio2_->streamOff();
+}
+
+} /* namespace libcamera */
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 07029dd763c9..5fcdd6335db6 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -60,74 +60,37 @@  PipelineHandlerIPU3::streamConfiguration(Camera *camera,
 int PipelineHandlerIPU3::configureStreams(Camera *camera,
 					  std::map<Stream *, StreamConfiguration> &config)
 {
-	IPU3CameraData *data = cameraData(camera);
-	StreamConfiguration *cfg = &config[&data->stream_];
-	V4L2Subdevice *sensor = data->sensor_;
-	V4L2Subdevice *csi2 = data->csi2_;
-	V4L2Device *cio2 = data->cio2_;
-	V4L2SubdeviceFormat subdevFormat = {};
-	V4L2DeviceFormat devFormat = {};
+	CIO2Device *cio2 = &cameraData(camera)->cio2Unit_;
+	Stream *stream = &cameraData(camera)->stream_;
+	StreamConfiguration *cfg = &config[stream];
+	IPU3DeviceFormat outputFormat = {};
 	int ret;
 
-	/*
-	 * FIXME: as of now, the format gets applied to the sensor and is
-	 * propagated along the pipeline. It should instead be applied on the
-	 * capture device and the sensor format calculated accordingly.
-	 */
-
-	ret = sensor->getFormat(0, &subdevFormat);
-	if (ret)
-		return ret;
-
-	subdevFormat.width = cfg->width;
-	subdevFormat.height = cfg->height;
-	ret = sensor->setFormat(0, &subdevFormat);
-	if (ret)
-		return ret;
-
-	/* Return error if the requested format cannot be applied to sensor. */
-	if (subdevFormat.width != cfg->width ||
-	    subdevFormat.height != cfg->height) {
-		LOG(IPU3, Error)
-			<< "Failed to apply image format "
-			<< subdevFormat.width << "x" << subdevFormat.height
-			<< " - got: " << cfg->width << "x" << cfg->height;
+	/* Validate the requested image format and resolution. */
+	if (cfg->pixelFormat != V4L2_PIX_FMT_NV12) {
+		LOG(IPU3, Error) << "Image format not supported";
 		return -EINVAL;
 	}
+	outputFormat.width = cfg->width;
+	outputFormat.height = cfg->height;
+	outputFormat.fourcc = cfg->pixelFormat;
 
-	ret = csi2->setFormat(0, &subdevFormat);
+	ret = cio2->configure(outputFormat);
 	if (ret)
 		return ret;
 
-	ret = cio2->getFormat(&devFormat);
-	if (ret)
-		return ret;
-
-	devFormat.width = subdevFormat.width;
-	devFormat.height = subdevFormat.height;
-	devFormat.fourcc = cfg->pixelFormat;
-
-	ret = cio2->setFormat(&devFormat);
-	if (ret)
-		return ret;
-
-	LOG(IPU3, Info) << cio2->driverName() << ": "
-			<< devFormat.width << "x" << devFormat.height
-			<< "- 0x" << std::hex << devFormat.fourcc << " planes: "
-			<< devFormat.planes;
-
 	return 0;
 }
 
 int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
 {
-	IPU3CameraData *data = cameraData(camera);
 	const StreamConfiguration &cfg = stream->configuration();
+	CIO2Device *cio2 = &cameraData(camera)->cio2Unit_;
 
 	if (!cfg.bufferCount)
 		return -EINVAL;
 
-	int ret = data->cio2_->exportBuffers(&stream->bufferPool());
+	int ret = cio2->exportBuffers(&stream->bufferPool());
 	if (ret) {
 		LOG(IPU3, Error) << "Failed to request memory";
 		return ret;
@@ -138,9 +101,9 @@  int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
 
 int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
 {
-	IPU3CameraData *data = cameraData(camera);
+	CIO2Device *cio2 = &cameraData(camera)->cio2Unit_;
 
-	int ret = data->cio2_->releaseBuffers();
+	int ret = cio2->releaseBuffers();
 	if (ret) {
 		LOG(IPU3, Error) << "Failed to release memory";
 		return ret;
@@ -151,10 +114,10 @@  int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
 
 int PipelineHandlerIPU3::start(const Camera *camera)
 {
-	IPU3CameraData *data = cameraData(camera);
+	CIO2Device *cio2 = &cameraData(camera)->cio2Unit_;
 	int ret;
 
-	ret = data->cio2_->streamOn();
+	ret = cio2->streamOn();
 	if (ret) {
 		LOG(IPU3, Info) << "Failed to start camera " << camera->name();
 		return ret;
@@ -165,16 +128,16 @@  int PipelineHandlerIPU3::start(const Camera *camera)
 
 void PipelineHandlerIPU3::stop(const Camera *camera)
 {
-	IPU3CameraData *data = cameraData(camera);
+	CIO2Device *cio2 = &cameraData(camera)->cio2Unit_;
 
-	if (data->cio2_->streamOff())
+	if (cio2->streamOff())
 		LOG(IPU3, Info) << "Failed to stop camera " << camera->name();
 }
 
 int PipelineHandlerIPU3::queueRequest(const Camera *camera, Request *request)
 {
-	IPU3CameraData *data = cameraData(camera);
-	Stream *stream = &data->stream_;
+	CIO2Device *cio2 = &cameraData(camera)->cio2Unit_;
+	Stream *stream = &cameraData(camera)->stream_;
 
 	Buffer *buffer = request->findBuffer(stream);
 	if (!buffer) {
@@ -183,7 +146,7 @@  int PipelineHandlerIPU3::queueRequest(const Camera *camera, Request *request)
 		return -ENOENT;
 	}
 
-	data->cio2_->queueBuffer(buffer);
+	cio2->queueBuffer(buffer);
 
 	return 0;
 }
@@ -271,79 +234,22 @@  void PipelineHandlerIPU3::registerCameras()
 	 */
 	unsigned int numCameras = 0;
 	for (unsigned int id = 0; id < 4; ++id) {
-		std::string csi2Name = "ipu3-csi2 " + std::to_string(id);
-		MediaEntity *csi2 = cio2_->getEntityByName(csi2Name);
-		int ret;
+		std::unique_ptr<IPU3CameraData> data =
+			utils::make_unique<IPU3CameraData>();
 
 		/*
-		 * This shall not happen, as the device enumerator matched
-		 * all entities described in the cio2_dm DeviceMatch.
-		 *
-		 * As this check is basically free, better stay safe than sorry.
+		 * If opening the CIO2 unit fails, the Camera instance won't
+		 * be registered and the 'data' unique pointers goes out of
+		 * scope and delete the objects they manage.
 		 */
-		if (!csi2)
-			continue;
-
-		const std::vector<MediaPad *> &pads = csi2->pads();
-		if (pads.empty())
-			continue;
-
-		/* IPU3 CSI-2 receivers have a single sink pad at index 0. */
-		MediaPad *sink = pads[0];
-		const std::vector<MediaLink *> &links = sink->links();
-		if (links.empty())
-			continue;
-
-		/*
-		 * Verify that the receiver is connected to a sensor, enable
-		 * the media link between the two, and create a Camera with
-		 * a unique name.
-		 */
-		MediaLink *link = links[0];
-		MediaEntity *sensor = link->source()->entity();
-		if (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)
-			continue;
-
-		if (link->setEnabled(true))
+		if (data->cio2Unit_.open(id, cio2_.get()))
 			continue;
 
-		std::unique_ptr<IPU3CameraData> data = utils::make_unique<IPU3CameraData>();
-
-		std::string cameraName = sensor->name() + " " + std::to_string(id);
+		std::string cameraName = data->cio2Unit_.sensor_->deviceName()
+				       + " " + std::to_string(id);
 		std::vector<Stream *> streams{ &data->stream_ };
-		std::shared_ptr<Camera> camera = Camera::create(this, cameraName, streams);
-
-		/*
-		 * Create and open video devices and subdevices associated with
-		 * the camera.
-		 *
-		 * If any of these operations fails, the Camera instance won't
-		 * be registered. The 'camera' shared pointer and the 'data'
-		 * unique pointers go out of scope and delete the objects they
-		 * manage.
-		 */
-		std::string cio2Name = "ipu3-cio2 " + std::to_string(id);
-		MediaEntity *cio2 = cio2_->getEntityByName(cio2Name);
-		if (!cio2) {
-			LOG(IPU3, Error)
-				<< "Failed to get entity '" << cio2Name << "'";
-			continue;
-		}
-
-		data->cio2_ = new V4L2Device(cio2);
-		ret = data->cio2_->open();
-		if (ret)
-			continue;
-
-		data->sensor_ = new V4L2Subdevice(sensor);
-		ret = data->sensor_->open();
-		if (ret)
-			continue;
-
-		data->csi2_ = new V4L2Subdevice(csi2);
-		ret = data->csi2_->open();
-		if (ret)
-			continue;
+		std::shared_ptr<Camera> camera = Camera::create(this, cameraName,
+								streams);
 
 		setCameraData(camera.get(), std::move(data));
 		registerCamera(std::move(camera));
diff --git a/src/libcamera/pipeline/ipu3/ipu3.h b/src/libcamera/pipeline/ipu3/ipu3.h
index 48c2a3e16980..2a8b6f13b1c7 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.h
+++ b/src/libcamera/pipeline/ipu3/ipu3.h
@@ -23,6 +23,40 @@ 
 
 namespace libcamera {
 
+struct IPU3DeviceFormat {
+	unsigned int width;
+	unsigned int height;
+	unsigned int fourcc;
+};
+
+class CIO2Device
+{
+public:
+	CIO2Device();
+	CIO2Device(const CIO2Device &) = delete;
+	~CIO2Device();
+
+	int open(unsigned int index, MediaDevice *cio2MediaDev);
+	int sensorSetFormat(unsigned int width, unsigned int height,
+			    V4L2SubdeviceFormat *format);
+	int configure(const IPU3DeviceFormat &format);
+	const IPU3DeviceFormat &format() const;
+
+	int exportBuffers(BufferPool *pool);
+	int releaseBuffers();
+
+	int queueBuffer(Buffer *buffer);
+
+	int streamOn();
+	int streamOff();
+
+	V4L2Device *cio2_;
+	V4L2Subdevice *csi2_;
+	V4L2Subdevice *sensor_;
+
+	IPU3DeviceFormat cio2Format_;
+};
+
 class PipelineHandlerIPU3 : public PipelineHandler
 {
 public:
@@ -49,19 +83,8 @@  private:
 	class IPU3CameraData : public CameraData
 	{
 	public:
-		IPU3CameraData()
-			: cio2_(nullptr), csi2_(nullptr), sensor_(nullptr) {}
-
-		~IPU3CameraData()
-		{
-			delete cio2_;
-			delete csi2_;
-			delete sensor_;
-		}
-
-		V4L2Device *cio2_;
-		V4L2Subdevice *csi2_;
-		V4L2Subdevice *sensor_;
+		/* Each camera has a CIO2 unit associated. */
+		CIO2Device cio2Unit_;
 
 		Stream stream_;
 	};
@@ -76,8 +99,6 @@  private:
 
 	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 0ab766a257a0..fcb2d319d517 100644
--- a/src/libcamera/pipeline/ipu3/meson.build
+++ b/src/libcamera/pipeline/ipu3/meson.build
@@ -1,3 +1,4 @@ 
 libcamera_sources += files([
+    'cio2.cpp',
     'ipu3.cpp',
 ])