[libcamera-devel] [PATCH 04/10] libcamera: ipu3: Propagate image format

[Jacopo Mondi]

Hi Laurent,

On Sun, Mar 03, 2019 at 12:44:23AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Feb 28, 2019 at 09:04:04PM +0100, Jacopo Mondi wrote:
> > Apply the requested image format to the sensor device, and apply the
> > adjusted one to the CIO2 device, the ImgU subdevice and its input and
> > output video devices.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 257 +++++++++++++++++++++++----
> >  1 file changed, 224 insertions(+), 33 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 9fa59c1bc97e..1e89e57f628b 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -8,11 +8,14 @@
> >  #include <memory>
> >  #include <vector>
> >
> > +#include <linux/media-bus-format.h>
> > +
> >  #include <libcamera/camera.h>
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> >
> >  #include "device_enumerator.h"
> > +#include "geometry.h"
> >  #include "log.h"
> >  #include "media_device.h"
> >  #include "pipeline_handler.h"
> > @@ -106,6 +109,29 @@ private:
> >  			PipelineHandler::cameraData(camera));
> >  	}
> >
> > +	void printDevFormat(const V4L2Device *dev, const V4L2DeviceFormat &fmt)
> > +	{
> > +		LOG(IPU3, Info)
>
> I think this is way too verbose in general.
>
> > +			<< dev->deviceNode() << ": " << fmt.width << "x"
> > +			<< fmt.height << "- 0x" << std::hex << fmt.fourcc
> > +			<< " planes: " << fmt.planesCount;
> > +	}
> > +
> > +	void printSubdevFormat(const V4L2Subdevice *dev, unsigned int pad,
> > +			       const V4L2SubdeviceFormat &fmt)
> > +	{
> > +		LOG(IPU3, Info)
> > +			<< "'" << dev->deviceName() << "':" << pad << " = "
> > +			<< fmt.width << "x" << fmt.height << " - 0x"
> > +			<< std::hex << fmt.mbus_code;
> > +	}
>
> How about adding a std::string toString() const method to the
> V4L2DeviceFormat and V4L2SubdeviceFormat classes ? print methods in the
> PipelineHandlerIPU3 class aren't very nice :-/ (especially with the
> hardcoded log level)
>

More code in the framework, less in pipeline handlers: it's a good idea!

> > +
> > +	int setImguFormat(V4L2Subdevice *imgu,
> > +			  const StreamConfiguration &config,
> > +			  Rectangle *rect);
> > +	int setSensorFormat(V4L2Subdevice *sensor,
> > +			    const StreamConfiguration &config,
> > +			    V4L2SubdeviceFormat *format);
> >  	int linkImgu(ImguDevice *imgu);
> >
> >  	V4L2Device *openDevice(MediaDevice *media, std::string &name);
> > @@ -186,14 +212,29 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> >  					  std::map<Stream *, StreamConfiguration> &config)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> > -	StreamConfiguration *cfg = &config[&data->stream_];
> > -	V4L2Subdevice *sensor = data->cio2.sensor;
> > +	const StreamConfiguration &cfg = config[&data->stream_];
> >  	V4L2Subdevice *csi2 = data->cio2.csi2;
> >  	V4L2Device *cio2 = data->cio2.output;
> > -	V4L2SubdeviceFormat subdevFormat = {};
> > -	V4L2DeviceFormat devFormat = {};
> >  	int ret;
> >
> > +	/*
> > +	 * Verify that the requested size respects the IPU3 alignement
> > +	 * requirements: image size shall be 8-aligned in width and 4-aligned
> > +	 * in height.
>
> Maybe "the image width shall be a multiple of 8 pixels and its height a
> multiple of 4 pixels" to clarify we're talking about pixels, not bytes ?
>

Ack

> > +	 *
> > +	 * 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) << "Stream format not support: bad alignement";
> > +		return -EINVAL;
> > +	}
> > +
> > +	LOG(IPU3, Info)
> > +		<< "Camera :'" << camera->name() << " - Stream format: "
> > +		<< cfg.width << "x" << cfg.height << " - "
> > +		<< std::hex << cfg.pixelFormat;
>
> Logging is important, but this is all way too verbose. Please consider
> for every log message if it's needed, and if it is, what the appropriate
> log level should be. The messages should also be more informative. This
> message, for instance, should explicitly state that it prints the
> requested format, and also mention which stream it applies to. You
> should also move it before the alignment check.

As we have clarified offline I'll make this Debug and will make this
and the others more user-consumable.
>
> > +
> >  	/*
> >  	 * TODO: dynamically assign ImgU devices; as of now, with a single
> >  	 * stream supported, always use 'imgu0'.
> > @@ -203,52 +244,73 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> >  	if (ret)
> >  		return ret;
> >
> > +	V4L2Device *viewfinder = data->imgu->viewfinder;
> > +	V4L2Subdevice *sensor = data->cio2.sensor;
> > +	V4L2Device *output = data->imgu->output;
> > +	V4L2Subdevice *imgu = data->imgu->imgu;
> > +	V4L2Device *input = data->imgu->input;
> > +
> >  	/*
> > -	 * 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.
> > +	 * Pass the requested output image size to the sensor and get back the
> > +	 * adjusted one to be propagated to the CIO2 device and to the ImgU
> > +	 * input.
> >  	 */
>
> If the sensor driver returns the closest match, you may get a size
> smaller than requested, which would require up-scaling. I'm not sure if
> the IPU3 supports that, but in any case it may not be a very good idea.
> I think you should instead iterate over the sizes supported by the
> sensor and pick the best one yourself. We may also want to use cropping
> at some point, but that can be left for later.
>
> And now that I've written this, I realized that your setSensorFormat()
> method has such an iteration. Could you update the above comment
> accordingly, and possibly rename setSensorFormat() to a more descriptive
> name ?

Re-reading the comment, I got why you got mis-lead... I'll change
this. Suggestions welcome for the method name.

>
> > +	V4L2SubdeviceFormat sensorFormat = {};
> > +	ret = setSensorFormat(sensor, cfg, &sensorFormat);
> > +	if (ret) {
> > +		LOG(IPU3, Error) << "Stream format not supported: ";
>
> Looks like you're missing part of the message. I would also move it to
> the setSensorFormat() function in order to print the exact reason of the
> error.
>
> > +		return ret;
> > +	}
> > +	printSubdevFormat(sensor, 0, sensorFormat);
> >
> > -	ret = sensor->getFormat(0, &subdevFormat);
> > +	ret = csi2->setFormat(0, &sensorFormat);
> >  	if (ret)
> >  		return ret;
> > -
> > -	subdevFormat.width = cfg->width;
> > -	subdevFormat.height = cfg->height;
> > -	ret = sensor->setFormat(0, &subdevFormat);
> > +	printSubdevFormat(csi2, 0, sensorFormat);
> > +
> > +	/* Apply the CIO2 image format to the CIO2 output and ImgU input. */
> > +	V4L2DeviceFormat cio2Format = {};
> > +	cio2Format.width = sensorFormat.width;
> > +	cio2Format.height = sensorFormat.height;
> > +	cio2Format.fourcc = V4L2_PIX_FMT_IPU3_SGRBG10;
>
> Let's not hardcode a particular bayer pattern, please pick the
> appropriate one based on what the sensor provides.

Ah! Isn't the output of the CIO2 device IPU3 specific and fixed?
That's not what the sensor provides, but the CIO2 produced one
(for which there are a few versions, which are different permutations
of the RGB bayer components, but I don't think that' too relevant)

>
> > +	cio2Format.planesCount = 1;
> > +	ret = cio2->setFormat(&cio2Format);
> >  	if (ret)
> >  		return ret;
> > +	printDevFormat(cio2, cio2Format);
> >
> > -	/* 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;
> > -		return -EINVAL;
> > -	}
> > -
> > -	ret = csi2->setFormat(0, &subdevFormat);
> > +	ret = input->setFormat(&cio2Format);
> >  	if (ret)
> >  		return ret;
> > -
> > -	ret = cio2->getFormat(&devFormat);
> > +	printDevFormat(input, cio2Format);
> > +
> > +	/* Apply pad formats and crop/compose rectangle to the ImgU. */
> > +	Rectangle rect = {
> > +		.x = 0,
> > +		.y = 0,
> > +		.w = cio2Format.width,
> > +		.h = cio2Format.height,
> > +	};
> > +	ret = setImguFormat(imgu, cfg, &rect);
> >  	if (ret)
> >  		return ret;
> >
> > -	devFormat.width = subdevFormat.width;
> > -	devFormat.height = subdevFormat.height;
> > -	devFormat.fourcc = cfg->pixelFormat;
> > +	/* Apply the format to the ImgU output and viewfinder devices. */
>
> As commented on the previous patch, with a single stream supported, do
> you really need both outputs ?
>

See my comments on the previous patch.

> > +	V4L2DeviceFormat outputFormat = {};
> > +	outputFormat.width = cfg.width;
> > +	outputFormat.height = cfg.height;
> > +	outputFormat.fourcc = V4L2_PIX_FMT_NV12;
> > +	outputFormat.planesCount = 2;
> >
> > -	ret = cio2->setFormat(&devFormat);
> > +	ret = output->setFormat(&outputFormat);
> >  	if (ret)
> >  		return ret;
> > +	printDevFormat(output, outputFormat);
> >
> > -	LOG(IPU3, Info) << cio2->driverName() << ": "
> > -			<< devFormat.width << "x" << devFormat.height
> > -			<< "- 0x" << std::hex << devFormat.fourcc << " planes: "
> > -			<< devFormat.planes;
> > +	ret = viewfinder->setFormat(&outputFormat);
> > +	if (ret)
> > +		return ret;
> > +	printDevFormat(viewfinder, outputFormat);
> >
> >  	return 0;
> >  }
> > @@ -409,6 +471,135 @@ error_release_mdev:
> >  	return false;
> >  }
> >
> > +int PipelineHandlerIPU3::setImguFormat(V4L2Subdevice *imgu,
> > +				       const StreamConfiguration &config,
> > +				       Rectangle *rect)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * Configure the 'imgu' subdevice with the requested sizes.
> > +	 *
> > +	 * 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.
> > +	 */
> > +	ret = imgu->setCrop(IMGU_PAD_INPUT, rect);
> > +	if (ret)
> > +		return ret;
> > +
> > +	LOG(IPU3, Info)
> > +		<< "'" << imgu->deviceName() << "':" << IMGU_PAD_INPUT
> > +		<< " = crop: (0,0)/" << rect->w << "x" << rect->h;
> > +
> > +	ret = imgu->setCompose(IMGU_PAD_INPUT, rect);
> > +	if (ret)
> > +		return ret;
> > +
> > +	LOG(IPU3, Info)
> > +		<< "'" << imgu->deviceName() << "':" << IMGU_PAD_INPUT
> > +		<< " = compose: (0,0)/" << rect->w << "x" << rect->h;
> > +
> > +
> > +	V4L2SubdeviceFormat imguFormat = {};
> > +	imguFormat.width = config.width;
> > +	imguFormat.height = config.height;
> > +	imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
> > +
> > +	ret = imgu->setFormat(IMGU_PAD_INPUT, &imguFormat);
> > +	if (ret)
> > +		return ret;
> > +	printSubdevFormat(imgu, IMGU_PAD_INPUT, imguFormat);
> > +
> > +	ret = imgu->setFormat(IMGU_PAD_OUTPUT, &imguFormat);
> > +	if (ret)
> > +		return ret;
> > +	printSubdevFormat(imgu, IMGU_PAD_OUTPUT, imguFormat);
> > +
> > +	ret = imgu->setFormat(IMGU_PAD_VF, &imguFormat);
> > +	if (ret)
> > +		return ret;
> > +	printSubdevFormat(imgu, IMGU_PAD_VF, imguFormat);
> > +
> > +	ret = imgu->setFormat(IMGU_PAD_STAT, &imguFormat);
> > +	if (ret)
> > +		return ret;
> > +	printSubdevFormat(imgu, IMGU_PAD_STAT, imguFormat);
> > +
> > +	return 0;
> > +}
> > +
> > +int PipelineHandlerIPU3::setSensorFormat(V4L2Subdevice *sensor,
> > +					 const StreamConfiguration &config,
> > +					 V4L2SubdeviceFormat *format)
> > +{
> > +	std::map<unsigned int, std::vector<SizeRange>> formats;
> > +	unsigned int best = ~0;
>
> Please use -1 instead of ~0.
>

Ack. Personal tastes or some reason I'm missing?

> > +	bool found = false;
> > +	int ret;
> > +
> > +	formats = sensor->formats(0);
> > +	if (formats.empty()) {
> > +		/*
> > +		 * If the format list is empty, try with the currently
> > +		 * applied one.
> > +		 */
>
> This shouldn't happen.
>

Agreed. Let's always assume sensor drivers are well written (creepy
laughs in background..)

> > +		ret = sensor->getFormat(0, format);
> > +		if (ret)
> > +			return ret;
> > +
> > +		if (format->width < config.width ||
> > +		    format->height < config.height)
> > +			return -EINVAL;
> > +
> > +		return 0;
> > +	}
> > +
> > +	/* Search for the best approximation the sensor can provide. */
> > +	auto it = formats.begin();
> > +	while (it != formats.end()) {
>
> for loop.
>

Ack

> > +		for (SizeRange &size : it->second) {
> > +			if (size.maxWidth < config.width ||
> > +			    size.maxHeight < config.height)
> > +				continue;
> > +
> > +			unsigned int diff =
> > +				(abs(size.maxWidth - config.width) +
> > +				 abs(size.maxHeight - config.height));
>
> No need for abs() as you check that maxWidth >= config.width above.
>
> Should you use the difference of area instead of the difference of
> lengths ?
>

I'll try to move most of this to geometry

> > +			if (diff >= best)
> > +				continue;
> > +
> > +			best = diff;
> > +			found = true;
>
> No need for the found variable, you can just check best != -1.
>

Correct, this probably answers my above question.

Thanks
  j

> > +
> > +			format->width = size.maxWidth;
> > +			format->height = size.maxHeight;
> > +			format->mbus_code = it->first;
> > +		}
> > +
> > +		++it;
> > +	}
> > +	if (!found)
> > +		return -EINVAL;
> > +
> > +	ret = sensor->setFormat(0, format);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Make sure everything is all right and the format did not get
> > +	 * adjusted.
> > +	 */
> > +	if (format->width < config.width ||
> > +	    format->height < config.height)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> >  /* Link entities in the ImgU unit to prepare for capture operations. */
> >  int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice)
> >  {
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190309/df23b791/attachment.sig>