[libcamera-devel,v7,06/13] libcamera: ipu3: Apply image format to the pipeline

Message ID 20190402171309.6447-7-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: ipu3: Add ImgU support
Related show

Commit Message

Jacopo Mondi April 2, 2019, 5:13 p.m. UTC
Apply the requested stream configuration to the CIO2 device, and
propagate formats through the the ImgU subdevice and its input and
output video devices.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 304 ++++++++++++++++++++++-----
 1 file changed, 252 insertions(+), 52 deletions(-)

Comments

Laurent Pinchart April 2, 2019, 5:32 p.m. UTC | #1
Hi Jacopo,

Thank you for the patch.

On Tue, Apr 02, 2019 at 07:13:02PM +0200, Jacopo Mondi wrote:
> Apply the requested stream configuration to the CIO2 device, and
> propagate formats through the the ImgU subdevice and its input and
> output video devices.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 304 ++++++++++++++++++++++-----
>  1 file changed, 252 insertions(+), 52 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 53e6b3b461a1..57e4bb89eaa7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -5,6 +5,7 @@
>   * ipu3.cpp - Pipeline handler for Intel IPU3
>   */
>  
> +#include <iomanip>
>  #include <memory>
>  #include <vector>
>  
> @@ -50,22 +51,35 @@ public:
>  	static constexpr unsigned int PAD_VF = 3;
>  	static constexpr unsigned int PAD_STAT = 4;
>  
> +	/* ImgU output descriptor: group data specific to an ImgU output. */
> +	struct ImgUOutput {
> +		V4L2Device *dev;
> +		unsigned int pad;
> +		std::string name;
> +	};
> +
>  	ImgUDevice()
> -		: imgu_(nullptr), input_(nullptr), output_(nullptr),
> -		  viewfinder_(nullptr), stat_(nullptr)
> +		: imgu_(nullptr), input_(nullptr)
>  	{
> +		output_.dev = nullptr;
> +		viewfinder_.dev = nullptr;
> +		stat_.dev = nullptr;
>  	}
>  
>  	~ImgUDevice()
>  	{
>  		delete imgu_;
>  		delete input_;
> -		delete output_;
> -		delete viewfinder_;
> -		delete stat_;
> +		delete output_.dev;
> +		delete viewfinder_.dev;
> +		delete stat_.dev;
>  	}
>  
>  	int init(MediaDevice *media, unsigned int index);
> +	int configureInput(const StreamConfiguration &config,
> +			   V4L2DeviceFormat *inputFormat);
> +	int configureOutput(const ImgUOutput *output,
> +			    const StreamConfiguration &config);

Even if this function doesn't modify the output structure as such, it
changes the output device configuration, so conceptually I think you
should drop the const keyword for the output argument.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	unsigned int index_;
>  	std::string name_;
> @@ -73,9 +87,9 @@ public:
>  
>  	V4L2Subdevice *imgu_;
>  	V4L2Device *input_;
> -	V4L2Device *output_;
> -	V4L2Device *viewfinder_;
> -	V4L2Device *stat_;
> +	ImgUOutput output_;
> +	ImgUOutput viewfinder_;
> +	ImgUOutput stat_;
>  	/* \todo Add param video device for 3A tuning */
>  };
>  
> @@ -95,6 +109,8 @@ public:
>  	}
>  
>  	int init(const MediaDevice *media, unsigned int index);
> +	int configure(const StreamConfiguration &config,
> +		      V4L2DeviceFormat *format);
>  
>  	V4L2Device *output_;
>  	V4L2Subdevice *csi2_;
> @@ -204,60 +220,62 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>  					  std::map<Stream *, StreamConfiguration> &config)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	StreamConfiguration *cfg = &config[&data->stream_];
> -	V4L2Subdevice *sensor = data->cio2_.sensor_;
> -	V4L2Subdevice *csi2 = data->cio2_.csi2_;
> -	V4L2Device *cio2 = data->cio2_.output_;
> -	V4L2SubdeviceFormat subdevFormat = {};
> -	V4L2DeviceFormat devFormat = {};
> +	const StreamConfiguration &cfg = config[&data->stream_];
> +	CIO2Device *cio2 = &data->cio2_;
> +	ImgUDevice *imgu = data->imgu_;
>  	int ret;
>  
> +	LOG(IPU3, Info)
> +		<< "Requested image format " << cfg.width << "x"
> +		<< cfg.height << "-0x" << std::hex << std::setfill('0')
> +		<< std::setw(8) << cfg.pixelFormat << " on camera '"
> +		<< camera->name() << "'";
> +
>  	/*
> -	 * 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.
> +	 * Verify that the requested size respects the IPU3 alignement
> +	 * requirements (the image width shall be a multiple of 8 pixels and
> +	 * its height a multiple of 4 pixels) and the camera maximum sizes.
> +	 *
> +	 * \todo: consider the BDS scaling factor requirements:
> +	 * "the downscaling factor must be an integer value multiple of 1/32"
>  	 */
> +	if (cfg.width % 8 || cfg.height % 4) {
> +		LOG(IPU3, Error) << "Invalid stream size: bad alignment";
> +		return -EINVAL;
> +	}
>  
> -	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) {
> +	if (cfg.width > cio2->maxSize_.width ||
> +	    cfg.height > cio2->maxSize_.height) {
>  		LOG(IPU3, Error)
> -			<< "Failed to apply image format "
> -			<< subdevFormat.width << "x" << subdevFormat.height
> -			<< " - got: " << cfg->width << "x" << cfg->height;
> +			<< "Invalid stream size: larger than sensor resolution";
>  		return -EINVAL;
>  	}
>  
> -	ret = csi2->setFormat(0, &subdevFormat);
> +	/*
> +	 * Pass the requested stream size to the CIO2 unit and get back the
> +	 * adjusted format to be propagated to the ImgU output devices.
> +	 */
> +	V4L2DeviceFormat cio2Format = {};
> +	ret = cio2->configure(cfg, &cio2Format);
>  	if (ret)
>  		return ret;
>  
> -	ret = cio2->getFormat(&devFormat);
> +	ret = imgu->configureInput(cfg, &cio2Format);
>  	if (ret)
>  		return ret;
>  
> -	devFormat.width = subdevFormat.width;
> -	devFormat.height = subdevFormat.height;
> -	devFormat.fourcc = cfg->pixelFormat;
> +	/* Apply the format to the ImgU output, viewfinder and stat. */
> +	ret = imgu->configureOutput(&imgu->output_, cfg);
> +	if (ret)
> +		return ret;
>  
> -	ret = cio2->setFormat(&devFormat);
> +	ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
>  	if (ret)
>  		return ret;
>  
> -	LOG(IPU3, Info) << cio2->driverName() << ": "
> -			<< devFormat.width << "x" << devFormat.height
> -			<< "- 0x" << std::hex << devFormat.fourcc << " planes: "
> -			<< devFormat.planes;
> +	ret = imgu->configureOutput(&imgu->stat_, cfg);
> +	if (ret)
> +		return ret;
>  
>  	return 0;
>  }
> @@ -519,9 +537,9 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>  	media_ = media;
>  
>  	/*
> -	 * The media entities presence has been verified by the match()
> -	 * function, no need to check for newly created video devices
> -	 * and subdevice validity here.
> +	 * The media entities presence in the media device has been verified
> +	 * by the match() function: no need to check for newly created
> +	 * video devices and subdevice validity here.
>  	 */
>  	imgu_ = V4L2Subdevice::fromEntityName(media, name_);
>  	ret = imgu_->open();
> @@ -533,21 +551,131 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>  	if (ret)
>  		return ret;
>  
> -	output_ = V4L2Device::fromEntityName(media, name_ + " output");
> -	ret = output_->open();
> +	output_.dev = V4L2Device::fromEntityName(media, name_ + " output");
> +	ret = output_.dev->open();
> +	if (ret)
> +		return ret;
> +
> +	output_.pad = PAD_OUTPUT;
> +	output_.name = "output";
> +
> +	viewfinder_.dev = V4L2Device::fromEntityName(media,
> +						     name_ + " viewfinder");
> +	ret = viewfinder_.dev->open();
> +	if (ret)
> +		return ret;
> +
> +	viewfinder_.pad = PAD_VF;
> +	viewfinder_.name = "viewfinder";
> +
> +	stat_.dev = V4L2Device::fromEntityName(media, name_ + " 3a stat");
> +	ret = stat_.dev->open();
> +	if (ret)
> +		return ret;
> +
> +	stat_.pad = PAD_STAT;
> +	stat_.name = "stat";
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Configure the ImgU unit input
> + * \param[in] config The requested stream configuration
> + * \param[in] inputFormat The format to be applied to ImgU input
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int ImgUDevice::configureInput(const StreamConfiguration &config,
> +			       V4L2DeviceFormat *inputFormat)
> +{
> +	/* Configure the ImgU input video device with the requested sizes. */
> +	int ret = input_->setFormat(inputFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug) << "ImgU input format = " << inputFormat->toString();
> +
> +	/*
> +	 * \todo The IPU3 driver implementation shall be changed to use the
> +	 * input sizes as 'ImgU Input' subdevice sizes, and use the desired
> +	 * GDC output sizes to configure the crop/compose rectangles.
> +	 *
> +	 * The current IPU3 driver implementation uses GDC sizes as the
> +	 * 'ImgU Input' subdevice sizes, and the input video device sizes
> +	 * to configure the crop/compose rectangles, contradicting the
> +	 * V4L2 specification.
> +	 */
> +	Rectangle rect = {
> +		.x = 0,
> +		.y = 0,
> +		.w = inputFormat->width,
> +		.h = inputFormat->height,
> +	};
> +	ret = imgu_->setCrop(PAD_INPUT, &rect);
> +	if (ret)
> +		return ret;
> +
> +	ret = imgu_->setCompose(PAD_INPUT, &rect);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug) << "ImgU input feeder and BDS rectangle = "
> +			 << rect.toString();
> +
> +	V4L2SubdeviceFormat imguFormat = {};
> +	imguFormat.width = config.width;
> +	imguFormat.height = config.height;
> +	imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
> +
> +	ret = imgu_->setFormat(PAD_INPUT, &imguFormat);
>  	if (ret)
>  		return ret;
>  
> -	viewfinder_ = V4L2Device::fromEntityName(media, name_ + " viewfinder");
> -	ret = viewfinder_->open();
> +	LOG(IPU3, Debug) << "ImgU GDC format = " << imguFormat.toString();
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Configure the ImgU unit \a id video output
> + * \param[in] output The ImgU output device to configure
> + * \param[in] config The requested configuration
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int ImgUDevice::configureOutput(const ImgUOutput *output,
> +				const StreamConfiguration &config)
> +{
> +	V4L2Device *dev = output->dev;
> +	unsigned int pad = output->pad;
> +
> +	V4L2SubdeviceFormat imguFormat = {};
> +	imguFormat.width = config.width;
> +	imguFormat.height = config.height;
> +	imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
> +
> +	int ret = imgu_->setFormat(pad, &imguFormat);
>  	if (ret)
>  		return ret;
>  
> -	stat_ = V4L2Device::fromEntityName(media, name_ + " 3a stat");
> -	ret = stat_->open();
> +	/* No need to apply format to the stat node. */
> +	if (output == &stat_)
> +		return 0;
> +
> +	V4L2DeviceFormat outputFormat = {};
> +	outputFormat.width = config.width;
> +	outputFormat.height = config.height;
> +	outputFormat.fourcc = V4L2_PIX_FMT_NV12;
> +	outputFormat.planesCount = 2;
> +
> +	ret = dev->setFormat(&outputFormat);
>  	if (ret)
>  		return ret;
>  
> +	LOG(IPU3, Debug) << "ImgU " << output->name << " format = "
> +			 << outputFormat.toString();
> +
>  	return 0;
>  }
>  
> @@ -645,6 +773,78 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  	return 0;
>  }
>  
> +/**
> + * \brief Configure the CIO2 unit
> + * \param[in] config The requested configuration
> + * \param[out] cio2Format The CIO2 unit output image format
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CIO2Device::configure(const StreamConfiguration &config,
> +			  V4L2DeviceFormat *cio2Format)
> +{
> +	unsigned int imageSize = config.width * config.height;
> +	V4L2SubdeviceFormat sensorFormat = {};
> +	unsigned int best = ~0;
> +	int ret;
> +
> +	for (auto it : sensor_->formats(0)) {
> +		/* Only consider formats consumable by the CIO2 unit. */
> +		if (mediaBusToCIO2Format(it.first) < 0)
> +			continue;
> +
> +		for (const SizeRange &size : it.second) {
> +			/*
> +			 * Only select formats bigger than the requested sizes
> +			 * as the IPU3 cannot up-scale.
> +			 *
> +			 * \todo: Unconditionally scale on the sensor as much
> +			 * as possible. This will need to be revisited when
> +			 * implementing the scaling policy.
> +			 */
> +			if (size.maxWidth < config.width ||
> +			    size.maxHeight < config.height)
> +				continue;
> +
> +			unsigned int diff = size.maxWidth * size.maxHeight
> +					  - imageSize;
> +			if (diff >= best)
> +				continue;
> +
> +			best = diff;
> +
> +			sensorFormat.width = size.maxWidth;
> +			sensorFormat.height = size.maxHeight;
> +			sensorFormat.mbus_code = it.first;
> +		}
> +	}
> +
> +	/*
> +	 * Apply the selected format to the sensor, the CSI-2 receiver and
> +	 * the CIO2 output device.
> +	 */
> +	ret = sensor_->setFormat(0, &sensorFormat);
> +	if (ret)
> +		return ret;
> +
> +	ret = csi2_->setFormat(0, &sensorFormat);
> +	if (ret)
> +		return ret;
> +
> +	cio2Format->width = sensorFormat.width;
> +	cio2Format->height = sensorFormat.height;
> +	cio2Format->fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code);
> +	cio2Format->planesCount = 1;
> +
> +	ret = output_->setFormat(cio2Format);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug) << "CIO2 output format " << cio2Format->toString();
> +
> +	return 0;
> +}
> +
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
>  
>  } /* namespace libcamera */
Niklas Söderlund April 2, 2019, 6:22 p.m. UTC | #2
Hi Jacopo,

Thanks for your work.

On 2019-04-02 19:13:02 +0200, Jacopo Mondi wrote:
> Apply the requested stream configuration to the CIO2 device, and
> propagate formats through the the ImgU subdevice and its input and
> output video devices.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

I like the end result of this patch, nice work.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 304 ++++++++++++++++++++++-----
>  1 file changed, 252 insertions(+), 52 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 53e6b3b461a1..57e4bb89eaa7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -5,6 +5,7 @@
>   * ipu3.cpp - Pipeline handler for Intel IPU3
>   */
>  
> +#include <iomanip>
>  #include <memory>
>  #include <vector>
>  
> @@ -50,22 +51,35 @@ public:
>  	static constexpr unsigned int PAD_VF = 3;
>  	static constexpr unsigned int PAD_STAT = 4;
>  
> +	/* ImgU output descriptor: group data specific to an ImgU output. */
> +	struct ImgUOutput {
> +		V4L2Device *dev;
> +		unsigned int pad;
> +		std::string name;
> +	};
> +
>  	ImgUDevice()
> -		: imgu_(nullptr), input_(nullptr), output_(nullptr),
> -		  viewfinder_(nullptr), stat_(nullptr)
> +		: imgu_(nullptr), input_(nullptr)
>  	{
> +		output_.dev = nullptr;
> +		viewfinder_.dev = nullptr;
> +		stat_.dev = nullptr;
>  	}
>  
>  	~ImgUDevice()
>  	{
>  		delete imgu_;
>  		delete input_;
> -		delete output_;
> -		delete viewfinder_;
> -		delete stat_;
> +		delete output_.dev;
> +		delete viewfinder_.dev;
> +		delete stat_.dev;
>  	}
>  
>  	int init(MediaDevice *media, unsigned int index);
> +	int configureInput(const StreamConfiguration &config,
> +			   V4L2DeviceFormat *inputFormat);
> +	int configureOutput(const ImgUOutput *output,
> +			    const StreamConfiguration &config);
>  
>  	unsigned int index_;
>  	std::string name_;
> @@ -73,9 +87,9 @@ public:
>  
>  	V4L2Subdevice *imgu_;
>  	V4L2Device *input_;
> -	V4L2Device *output_;
> -	V4L2Device *viewfinder_;
> -	V4L2Device *stat_;
> +	ImgUOutput output_;
> +	ImgUOutput viewfinder_;
> +	ImgUOutput stat_;
>  	/* \todo Add param video device for 3A tuning */
>  };
>  
> @@ -95,6 +109,8 @@ public:
>  	}
>  
>  	int init(const MediaDevice *media, unsigned int index);
> +	int configure(const StreamConfiguration &config,
> +		      V4L2DeviceFormat *format);
>  
>  	V4L2Device *output_;
>  	V4L2Subdevice *csi2_;
> @@ -204,60 +220,62 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>  					  std::map<Stream *, StreamConfiguration> &config)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	StreamConfiguration *cfg = &config[&data->stream_];
> -	V4L2Subdevice *sensor = data->cio2_.sensor_;
> -	V4L2Subdevice *csi2 = data->cio2_.csi2_;
> -	V4L2Device *cio2 = data->cio2_.output_;
> -	V4L2SubdeviceFormat subdevFormat = {};
> -	V4L2DeviceFormat devFormat = {};
> +	const StreamConfiguration &cfg = config[&data->stream_];
> +	CIO2Device *cio2 = &data->cio2_;
> +	ImgUDevice *imgu = data->imgu_;
>  	int ret;
>  
> +	LOG(IPU3, Info)
> +		<< "Requested image format " << cfg.width << "x"
> +		<< cfg.height << "-0x" << std::hex << std::setfill('0')
> +		<< std::setw(8) << cfg.pixelFormat << " on camera '"
> +		<< camera->name() << "'";
> +
>  	/*
> -	 * 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.
> +	 * Verify that the requested size respects the IPU3 alignement
> +	 * requirements (the image width shall be a multiple of 8 pixels and
> +	 * its height a multiple of 4 pixels) and the camera maximum sizes.
> +	 *
> +	 * \todo: consider the BDS scaling factor requirements:
> +	 * "the downscaling factor must be an integer value multiple of 1/32"
>  	 */
> +	if (cfg.width % 8 || cfg.height % 4) {
> +		LOG(IPU3, Error) << "Invalid stream size: bad alignment";
> +		return -EINVAL;
> +	}
>  
> -	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) {
> +	if (cfg.width > cio2->maxSize_.width ||
> +	    cfg.height > cio2->maxSize_.height) {
>  		LOG(IPU3, Error)
> -			<< "Failed to apply image format "
> -			<< subdevFormat.width << "x" << subdevFormat.height
> -			<< " - got: " << cfg->width << "x" << cfg->height;
> +			<< "Invalid stream size: larger than sensor resolution";
>  		return -EINVAL;
>  	}
>  
> -	ret = csi2->setFormat(0, &subdevFormat);
> +	/*
> +	 * Pass the requested stream size to the CIO2 unit and get back the
> +	 * adjusted format to be propagated to the ImgU output devices.
> +	 */
> +	V4L2DeviceFormat cio2Format = {};
> +	ret = cio2->configure(cfg, &cio2Format);
>  	if (ret)
>  		return ret;
>  
> -	ret = cio2->getFormat(&devFormat);
> +	ret = imgu->configureInput(cfg, &cio2Format);
>  	if (ret)
>  		return ret;
>  
> -	devFormat.width = subdevFormat.width;
> -	devFormat.height = subdevFormat.height;
> -	devFormat.fourcc = cfg->pixelFormat;
> +	/* Apply the format to the ImgU output, viewfinder and stat. */
> +	ret = imgu->configureOutput(&imgu->output_, cfg);
> +	if (ret)
> +		return ret;
>  
> -	ret = cio2->setFormat(&devFormat);
> +	ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
>  	if (ret)
>  		return ret;
>  
> -	LOG(IPU3, Info) << cio2->driverName() << ": "
> -			<< devFormat.width << "x" << devFormat.height
> -			<< "- 0x" << std::hex << devFormat.fourcc << " planes: "
> -			<< devFormat.planes;
> +	ret = imgu->configureOutput(&imgu->stat_, cfg);
> +	if (ret)
> +		return ret;
>  
>  	return 0;
>  }
> @@ -519,9 +537,9 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>  	media_ = media;
>  
>  	/*
> -	 * The media entities presence has been verified by the match()
> -	 * function, no need to check for newly created video devices
> -	 * and subdevice validity here.
> +	 * The media entities presence in the media device has been verified
> +	 * by the match() function: no need to check for newly created
> +	 * video devices and subdevice validity here.
>  	 */
>  	imgu_ = V4L2Subdevice::fromEntityName(media, name_);
>  	ret = imgu_->open();
> @@ -533,21 +551,131 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>  	if (ret)
>  		return ret;
>  
> -	output_ = V4L2Device::fromEntityName(media, name_ + " output");
> -	ret = output_->open();
> +	output_.dev = V4L2Device::fromEntityName(media, name_ + " output");
> +	ret = output_.dev->open();
> +	if (ret)
> +		return ret;
> +
> +	output_.pad = PAD_OUTPUT;
> +	output_.name = "output";
> +
> +	viewfinder_.dev = V4L2Device::fromEntityName(media,
> +						     name_ + " viewfinder");
> +	ret = viewfinder_.dev->open();
> +	if (ret)
> +		return ret;
> +
> +	viewfinder_.pad = PAD_VF;
> +	viewfinder_.name = "viewfinder";
> +
> +	stat_.dev = V4L2Device::fromEntityName(media, name_ + " 3a stat");
> +	ret = stat_.dev->open();
> +	if (ret)
> +		return ret;
> +
> +	stat_.pad = PAD_STAT;
> +	stat_.name = "stat";
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Configure the ImgU unit input
> + * \param[in] config The requested stream configuration
> + * \param[in] inputFormat The format to be applied to ImgU input
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int ImgUDevice::configureInput(const StreamConfiguration &config,
> +			       V4L2DeviceFormat *inputFormat)
> +{
> +	/* Configure the ImgU input video device with the requested sizes. */
> +	int ret = input_->setFormat(inputFormat);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug) << "ImgU input format = " << inputFormat->toString();
> +
> +	/*
> +	 * \todo The IPU3 driver implementation shall be changed to use the
> +	 * input sizes as 'ImgU Input' subdevice sizes, and use the desired
> +	 * GDC output sizes to configure the crop/compose rectangles.
> +	 *
> +	 * The current IPU3 driver implementation uses GDC sizes as the
> +	 * 'ImgU Input' subdevice sizes, and the input video device sizes
> +	 * to configure the crop/compose rectangles, contradicting the
> +	 * V4L2 specification.
> +	 */
> +	Rectangle rect = {
> +		.x = 0,
> +		.y = 0,
> +		.w = inputFormat->width,
> +		.h = inputFormat->height,
> +	};
> +	ret = imgu_->setCrop(PAD_INPUT, &rect);
> +	if (ret)
> +		return ret;
> +
> +	ret = imgu_->setCompose(PAD_INPUT, &rect);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug) << "ImgU input feeder and BDS rectangle = "
> +			 << rect.toString();
> +
> +	V4L2SubdeviceFormat imguFormat = {};
> +	imguFormat.width = config.width;
> +	imguFormat.height = config.height;
> +	imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
> +
> +	ret = imgu_->setFormat(PAD_INPUT, &imguFormat);
>  	if (ret)
>  		return ret;
>  
> -	viewfinder_ = V4L2Device::fromEntityName(media, name_ + " viewfinder");
> -	ret = viewfinder_->open();
> +	LOG(IPU3, Debug) << "ImgU GDC format = " << imguFormat.toString();
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Configure the ImgU unit \a id video output
> + * \param[in] output The ImgU output device to configure
> + * \param[in] config The requested configuration
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int ImgUDevice::configureOutput(const ImgUOutput *output,
> +				const StreamConfiguration &config)
> +{
> +	V4L2Device *dev = output->dev;
> +	unsigned int pad = output->pad;
> +
> +	V4L2SubdeviceFormat imguFormat = {};
> +	imguFormat.width = config.width;
> +	imguFormat.height = config.height;
> +	imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
> +
> +	int ret = imgu_->setFormat(pad, &imguFormat);
>  	if (ret)
>  		return ret;
>  
> -	stat_ = V4L2Device::fromEntityName(media, name_ + " 3a stat");
> -	ret = stat_->open();
> +	/* No need to apply format to the stat node. */
> +	if (output == &stat_)
> +		return 0;
> +
> +	V4L2DeviceFormat outputFormat = {};
> +	outputFormat.width = config.width;
> +	outputFormat.height = config.height;
> +	outputFormat.fourcc = V4L2_PIX_FMT_NV12;
> +	outputFormat.planesCount = 2;
> +
> +	ret = dev->setFormat(&outputFormat);
>  	if (ret)
>  		return ret;
>  
> +	LOG(IPU3, Debug) << "ImgU " << output->name << " format = "
> +			 << outputFormat.toString();
> +
>  	return 0;
>  }
>  
> @@ -645,6 +773,78 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  	return 0;
>  }
>  
> +/**
> + * \brief Configure the CIO2 unit
> + * \param[in] config The requested configuration
> + * \param[out] cio2Format The CIO2 unit output image format
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CIO2Device::configure(const StreamConfiguration &config,
> +			  V4L2DeviceFormat *cio2Format)
> +{
> +	unsigned int imageSize = config.width * config.height;
> +	V4L2SubdeviceFormat sensorFormat = {};
> +	unsigned int best = ~0;
> +	int ret;
> +
> +	for (auto it : sensor_->formats(0)) {
> +		/* Only consider formats consumable by the CIO2 unit. */
> +		if (mediaBusToCIO2Format(it.first) < 0)
> +			continue;
> +
> +		for (const SizeRange &size : it.second) {
> +			/*
> +			 * Only select formats bigger than the requested sizes
> +			 * as the IPU3 cannot up-scale.
> +			 *
> +			 * \todo: Unconditionally scale on the sensor as much
> +			 * as possible. This will need to be revisited when
> +			 * implementing the scaling policy.
> +			 */
> +			if (size.maxWidth < config.width ||
> +			    size.maxHeight < config.height)
> +				continue;
> +
> +			unsigned int diff = size.maxWidth * size.maxHeight
> +					  - imageSize;
> +			if (diff >= best)
> +				continue;
> +
> +			best = diff;
> +
> +			sensorFormat.width = size.maxWidth;
> +			sensorFormat.height = size.maxHeight;
> +			sensorFormat.mbus_code = it.first;
> +		}
> +	}
> +
> +	/*
> +	 * Apply the selected format to the sensor, the CSI-2 receiver and
> +	 * the CIO2 output device.
> +	 */
> +	ret = sensor_->setFormat(0, &sensorFormat);
> +	if (ret)
> +		return ret;
> +
> +	ret = csi2_->setFormat(0, &sensorFormat);
> +	if (ret)
> +		return ret;
> +
> +	cio2Format->width = sensorFormat.width;
> +	cio2Format->height = sensorFormat.height;
> +	cio2Format->fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code);
> +	cio2Format->planesCount = 1;
> +
> +	ret = output_->setFormat(cio2Format);
> +	if (ret)
> +		return ret;
> +
> +	LOG(IPU3, Debug) << "CIO2 output format " << cio2Format->toString();
> +
> +	return 0;
> +}
> +
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
>  
>  } /* namespace libcamera */
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 53e6b3b461a1..57e4bb89eaa7 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -5,6 +5,7 @@ 
  * ipu3.cpp - Pipeline handler for Intel IPU3
  */
 
+#include <iomanip>
 #include <memory>
 #include <vector>
 
@@ -50,22 +51,35 @@  public:
 	static constexpr unsigned int PAD_VF = 3;
 	static constexpr unsigned int PAD_STAT = 4;
 
+	/* ImgU output descriptor: group data specific to an ImgU output. */
+	struct ImgUOutput {
+		V4L2Device *dev;
+		unsigned int pad;
+		std::string name;
+	};
+
 	ImgUDevice()
-		: imgu_(nullptr), input_(nullptr), output_(nullptr),
-		  viewfinder_(nullptr), stat_(nullptr)
+		: imgu_(nullptr), input_(nullptr)
 	{
+		output_.dev = nullptr;
+		viewfinder_.dev = nullptr;
+		stat_.dev = nullptr;
 	}
 
 	~ImgUDevice()
 	{
 		delete imgu_;
 		delete input_;
-		delete output_;
-		delete viewfinder_;
-		delete stat_;
+		delete output_.dev;
+		delete viewfinder_.dev;
+		delete stat_.dev;
 	}
 
 	int init(MediaDevice *media, unsigned int index);
+	int configureInput(const StreamConfiguration &config,
+			   V4L2DeviceFormat *inputFormat);
+	int configureOutput(const ImgUOutput *output,
+			    const StreamConfiguration &config);
 
 	unsigned int index_;
 	std::string name_;
@@ -73,9 +87,9 @@  public:
 
 	V4L2Subdevice *imgu_;
 	V4L2Device *input_;
-	V4L2Device *output_;
-	V4L2Device *viewfinder_;
-	V4L2Device *stat_;
+	ImgUOutput output_;
+	ImgUOutput viewfinder_;
+	ImgUOutput stat_;
 	/* \todo Add param video device for 3A tuning */
 };
 
@@ -95,6 +109,8 @@  public:
 	}
 
 	int init(const MediaDevice *media, unsigned int index);
+	int configure(const StreamConfiguration &config,
+		      V4L2DeviceFormat *format);
 
 	V4L2Device *output_;
 	V4L2Subdevice *csi2_;
@@ -204,60 +220,62 @@  int PipelineHandlerIPU3::configureStreams(Camera *camera,
 					  std::map<Stream *, StreamConfiguration> &config)
 {
 	IPU3CameraData *data = cameraData(camera);
-	StreamConfiguration *cfg = &config[&data->stream_];
-	V4L2Subdevice *sensor = data->cio2_.sensor_;
-	V4L2Subdevice *csi2 = data->cio2_.csi2_;
-	V4L2Device *cio2 = data->cio2_.output_;
-	V4L2SubdeviceFormat subdevFormat = {};
-	V4L2DeviceFormat devFormat = {};
+	const StreamConfiguration &cfg = config[&data->stream_];
+	CIO2Device *cio2 = &data->cio2_;
+	ImgUDevice *imgu = data->imgu_;
 	int ret;
 
+	LOG(IPU3, Info)
+		<< "Requested image format " << cfg.width << "x"
+		<< cfg.height << "-0x" << std::hex << std::setfill('0')
+		<< std::setw(8) << cfg.pixelFormat << " on camera '"
+		<< camera->name() << "'";
+
 	/*
-	 * 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.
+	 * Verify that the requested size respects the IPU3 alignement
+	 * requirements (the image width shall be a multiple of 8 pixels and
+	 * its height a multiple of 4 pixels) and the camera maximum sizes.
+	 *
+	 * \todo: consider the BDS scaling factor requirements:
+	 * "the downscaling factor must be an integer value multiple of 1/32"
 	 */
+	if (cfg.width % 8 || cfg.height % 4) {
+		LOG(IPU3, Error) << "Invalid stream size: bad alignment";
+		return -EINVAL;
+	}
 
-	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) {
+	if (cfg.width > cio2->maxSize_.width ||
+	    cfg.height > cio2->maxSize_.height) {
 		LOG(IPU3, Error)
-			<< "Failed to apply image format "
-			<< subdevFormat.width << "x" << subdevFormat.height
-			<< " - got: " << cfg->width << "x" << cfg->height;
+			<< "Invalid stream size: larger than sensor resolution";
 		return -EINVAL;
 	}
 
-	ret = csi2->setFormat(0, &subdevFormat);
+	/*
+	 * Pass the requested stream size to the CIO2 unit and get back the
+	 * adjusted format to be propagated to the ImgU output devices.
+	 */
+	V4L2DeviceFormat cio2Format = {};
+	ret = cio2->configure(cfg, &cio2Format);
 	if (ret)
 		return ret;
 
-	ret = cio2->getFormat(&devFormat);
+	ret = imgu->configureInput(cfg, &cio2Format);
 	if (ret)
 		return ret;
 
-	devFormat.width = subdevFormat.width;
-	devFormat.height = subdevFormat.height;
-	devFormat.fourcc = cfg->pixelFormat;
+	/* Apply the format to the ImgU output, viewfinder and stat. */
+	ret = imgu->configureOutput(&imgu->output_, cfg);
+	if (ret)
+		return ret;
 
-	ret = cio2->setFormat(&devFormat);
+	ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
 	if (ret)
 		return ret;
 
-	LOG(IPU3, Info) << cio2->driverName() << ": "
-			<< devFormat.width << "x" << devFormat.height
-			<< "- 0x" << std::hex << devFormat.fourcc << " planes: "
-			<< devFormat.planes;
+	ret = imgu->configureOutput(&imgu->stat_, cfg);
+	if (ret)
+		return ret;
 
 	return 0;
 }
@@ -519,9 +537,9 @@  int ImgUDevice::init(MediaDevice *media, unsigned int index)
 	media_ = media;
 
 	/*
-	 * The media entities presence has been verified by the match()
-	 * function, no need to check for newly created video devices
-	 * and subdevice validity here.
+	 * The media entities presence in the media device has been verified
+	 * by the match() function: no need to check for newly created
+	 * video devices and subdevice validity here.
 	 */
 	imgu_ = V4L2Subdevice::fromEntityName(media, name_);
 	ret = imgu_->open();
@@ -533,21 +551,131 @@  int ImgUDevice::init(MediaDevice *media, unsigned int index)
 	if (ret)
 		return ret;
 
-	output_ = V4L2Device::fromEntityName(media, name_ + " output");
-	ret = output_->open();
+	output_.dev = V4L2Device::fromEntityName(media, name_ + " output");
+	ret = output_.dev->open();
+	if (ret)
+		return ret;
+
+	output_.pad = PAD_OUTPUT;
+	output_.name = "output";
+
+	viewfinder_.dev = V4L2Device::fromEntityName(media,
+						     name_ + " viewfinder");
+	ret = viewfinder_.dev->open();
+	if (ret)
+		return ret;
+
+	viewfinder_.pad = PAD_VF;
+	viewfinder_.name = "viewfinder";
+
+	stat_.dev = V4L2Device::fromEntityName(media, name_ + " 3a stat");
+	ret = stat_.dev->open();
+	if (ret)
+		return ret;
+
+	stat_.pad = PAD_STAT;
+	stat_.name = "stat";
+
+	return 0;
+}
+
+/**
+ * \brief Configure the ImgU unit input
+ * \param[in] config The requested stream configuration
+ * \param[in] inputFormat The format to be applied to ImgU input
+ *
+ * \return 0 on success or a negative error code otherwise
+ */
+int ImgUDevice::configureInput(const StreamConfiguration &config,
+			       V4L2DeviceFormat *inputFormat)
+{
+	/* Configure the ImgU input video device with the requested sizes. */
+	int ret = input_->setFormat(inputFormat);
+	if (ret)
+		return ret;
+
+	LOG(IPU3, Debug) << "ImgU input format = " << inputFormat->toString();
+
+	/*
+	 * \todo The IPU3 driver implementation shall be changed to use the
+	 * input sizes as 'ImgU Input' subdevice sizes, and use the desired
+	 * GDC output sizes to configure the crop/compose rectangles.
+	 *
+	 * The current IPU3 driver implementation uses GDC sizes as the
+	 * 'ImgU Input' subdevice sizes, and the input video device sizes
+	 * to configure the crop/compose rectangles, contradicting the
+	 * V4L2 specification.
+	 */
+	Rectangle rect = {
+		.x = 0,
+		.y = 0,
+		.w = inputFormat->width,
+		.h = inputFormat->height,
+	};
+	ret = imgu_->setCrop(PAD_INPUT, &rect);
+	if (ret)
+		return ret;
+
+	ret = imgu_->setCompose(PAD_INPUT, &rect);
+	if (ret)
+		return ret;
+
+	LOG(IPU3, Debug) << "ImgU input feeder and BDS rectangle = "
+			 << rect.toString();
+
+	V4L2SubdeviceFormat imguFormat = {};
+	imguFormat.width = config.width;
+	imguFormat.height = config.height;
+	imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
+
+	ret = imgu_->setFormat(PAD_INPUT, &imguFormat);
 	if (ret)
 		return ret;
 
-	viewfinder_ = V4L2Device::fromEntityName(media, name_ + " viewfinder");
-	ret = viewfinder_->open();
+	LOG(IPU3, Debug) << "ImgU GDC format = " << imguFormat.toString();
+
+	return 0;
+}
+
+/**
+ * \brief Configure the ImgU unit \a id video output
+ * \param[in] output The ImgU output device to configure
+ * \param[in] config The requested configuration
+ *
+ * \return 0 on success or a negative error code otherwise
+ */
+int ImgUDevice::configureOutput(const ImgUOutput *output,
+				const StreamConfiguration &config)
+{
+	V4L2Device *dev = output->dev;
+	unsigned int pad = output->pad;
+
+	V4L2SubdeviceFormat imguFormat = {};
+	imguFormat.width = config.width;
+	imguFormat.height = config.height;
+	imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
+
+	int ret = imgu_->setFormat(pad, &imguFormat);
 	if (ret)
 		return ret;
 
-	stat_ = V4L2Device::fromEntityName(media, name_ + " 3a stat");
-	ret = stat_->open();
+	/* No need to apply format to the stat node. */
+	if (output == &stat_)
+		return 0;
+
+	V4L2DeviceFormat outputFormat = {};
+	outputFormat.width = config.width;
+	outputFormat.height = config.height;
+	outputFormat.fourcc = V4L2_PIX_FMT_NV12;
+	outputFormat.planesCount = 2;
+
+	ret = dev->setFormat(&outputFormat);
 	if (ret)
 		return ret;
 
+	LOG(IPU3, Debug) << "ImgU " << output->name << " format = "
+			 << outputFormat.toString();
+
 	return 0;
 }
 
@@ -645,6 +773,78 @@  int CIO2Device::init(const MediaDevice *media, unsigned int index)
 	return 0;
 }
 
+/**
+ * \brief Configure the CIO2 unit
+ * \param[in] config The requested configuration
+ * \param[out] cio2Format The CIO2 unit output image format
+ *
+ * \return 0 on success or a negative error code otherwise
+ */
+int CIO2Device::configure(const StreamConfiguration &config,
+			  V4L2DeviceFormat *cio2Format)
+{
+	unsigned int imageSize = config.width * config.height;
+	V4L2SubdeviceFormat sensorFormat = {};
+	unsigned int best = ~0;
+	int ret;
+
+	for (auto it : sensor_->formats(0)) {
+		/* Only consider formats consumable by the CIO2 unit. */
+		if (mediaBusToCIO2Format(it.first) < 0)
+			continue;
+
+		for (const SizeRange &size : it.second) {
+			/*
+			 * Only select formats bigger than the requested sizes
+			 * as the IPU3 cannot up-scale.
+			 *
+			 * \todo: Unconditionally scale on the sensor as much
+			 * as possible. This will need to be revisited when
+			 * implementing the scaling policy.
+			 */
+			if (size.maxWidth < config.width ||
+			    size.maxHeight < config.height)
+				continue;
+
+			unsigned int diff = size.maxWidth * size.maxHeight
+					  - imageSize;
+			if (diff >= best)
+				continue;
+
+			best = diff;
+
+			sensorFormat.width = size.maxWidth;
+			sensorFormat.height = size.maxHeight;
+			sensorFormat.mbus_code = it.first;
+		}
+	}
+
+	/*
+	 * Apply the selected format to the sensor, the CSI-2 receiver and
+	 * the CIO2 output device.
+	 */
+	ret = sensor_->setFormat(0, &sensorFormat);
+	if (ret)
+		return ret;
+
+	ret = csi2_->setFormat(0, &sensorFormat);
+	if (ret)
+		return ret;
+
+	cio2Format->width = sensorFormat.width;
+	cio2Format->height = sensorFormat.height;
+	cio2Format->fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code);
+	cio2Format->planesCount = 1;
+
+	ret = output_->setFormat(cio2Format);
+	if (ret)
+		return ret;
+
+	LOG(IPU3, Debug) << "CIO2 output format " << cio2Format->toString();
+
+	return 0;
+}
+
 REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
 
 } /* namespace libcamera */