[libcamera-devel] [PATCH 01/16] libcamera: ipu3: Use the optimal sensor size

[Jacopo Mondi]

Hello Jean-Michel

On Fri, Sep 03, 2021 at 09:00:37AM +0200, Jean-Michel Hautbois wrote:
> Hi Jacopo,
>
> On 27/08/2021 14:07, Jacopo Mondi wrote:
> > As reported by commit 7208e70211a6 ("libcamera: ipu3: Always use sensor
> > full frame size") the current implementation of the IPU3 pipeline
> > handler always use the sensor resolution as the ImgU input frame size in
> > order to work around an issue with the ImgU configuration procedure.
> >
> > Now that the frame selection policy has been modified in the CIO2Device
> > class implementation to comply with the requirements of the ImgU
> > configuration script we can remove the workaround and select the most
> > opportune sensor size to feed the ImgU with.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++--------------
> >  1 file changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index c287bf86e79a..b321c94e9cb0 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -229,7 +229,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >  		status = Adjusted;
> >  	}
> >
> > -	/* Validate the requested stream configuration */
> > +	/*
> > +	 * Validate the requested stream configuration and select the sensor
> > +	 * format by collecting the maximum RAW stream width and height and
> > +	 * picking the closest larger match.
> > +	 *
> > +	 * If no RAW stream is requested use the one of the largest YUV stream,
> > +	 * plus margin pixels for the IF and BDS rectangle to downscale.
> > +	 *
> > +	 * \todo Clarify the IF and BDS margins requirements.
> > +	 */
> >  	unsigned int rawCount = 0;
> >  	unsigned int yuvCount = 0;
> >  	Size maxYuvSize;
> > @@ -240,7 +249,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >
> >  		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> >  			rawCount++;
> > -			rawSize = cfg.size;
> > +			rawSize.expandTo(cfg.size);
> >  		} else {
> >  			yuvCount++;
> >  			maxYuvSize.expandTo(cfg.size);
> > @@ -269,24 +278,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >  	/*
> >  	 * Generate raw configuration from CIO2.
> >  	 *
> > -	 * \todo The image sensor frame size should be selected to optimize
> > -	 * operations based on the sizes of the requested streams. However such
> > -	 * a selection makes the pipeline configuration procedure fail for small
> > -	 * resolutions (for example: 640x480 with OV5670) and causes the capture
> > -	 * operations to stall for some stream size combinations (see the
> > -	 * commit message of the patch that introduced this comment for more
> > -	 * failure examples).
> > -	 *
> > -	 * Until the sensor frame size calculation criteria are clarified, when
> > -	 * capturing from ImgU always use the largest possible size which
> > -	 * guarantees better results at the expense of the frame rate and CSI-2
> > -	 * bus bandwidth. When only a raw stream is requested use the requested
> > -	 * size instead, as the ImgU is not involved.
> > +	 * The output YUV streams will be limited in size to the
> > +	 * maximum frame size requested for the RAW stream.
> >  	 */
> > -	if (!yuvCount)
> > -		cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);
> > -	else
> > -		cio2Configuration_ = data_->cio2_.generateConfiguration({});
> > +	if (rawSize.isNull())
> > +		rawSize = maxYuvSize.alignedUpTo(40, 540)
> > +				    .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > +						 IMGU_OUTPUT_HEIGHT_MARGIN)
>
> We have mixed magical values and defined ones... What are 40 and 540
> exactly ?
>

You are right, 40 and 540 are magical :)

They are defined in imgu.cpp as

static constexpr unsigned int IF_CROP_MAX_W = 40;
static constexpr unsigned int IF_CROP_MAX_H = 540;

And represent the IF rectangle crop limits.

As the ImgU configuration procedure is iterative, it tries to to find
a suitable configuration by testing one after the other IF rectangles
scaled down by a known factor, up to the maximum scaling limits
expressed above.

	/* Populate the configurations vector by scaling width and height. */
	unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);
	unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);
	unsigned int minIfWidth = in.width - IF_CROP_MAX_W;
	unsigned int minIfHeight = in.height - IF_CROP_MAX_H;
	while (ifWidth >= minIfWidth) {
		while (ifHeight >= minIfHeight) {
			Size iif{ ifWidth, ifHeight };
			calculateBDS(pipe, iif, gdc, sf);
			ifHeight -= IF_ALIGN_H;
		}

		ifWidth -= IF_ALIGN_W;
	}

	/* Repeat search by scaling width first. */
	ifWidth = utils::alignUp(in.width, IF_ALIGN_W);
	ifHeight = utils::alignUp(in.height, IF_ALIGN_H);
	minIfWidth = in.width - IF_CROP_MAX_W;
	minIfHeight = in.height - IF_CROP_MAX_H;
	while (ifHeight >= minIfHeight) {
		/*
		 * \todo This procedure is probably broken:
		 * https://github.com/intel/intel-ipu3-pipecfg/issues/2
		 */
		while (ifWidth >= minIfWidth) {
			Size iif{ ifWidth, ifHeight };
			calculateBDS(pipe, iif, gdc, sf);
			ifWidth -= IF_ALIGN_W;
		}

		ifHeight -= IF_ALIGN_H;
	}

For this reason, we refuse input sizes smaller than the IF scaling
limits

	/*
	 * \todo Filter out all resolutions < IF_CROP_MAX.
	 * See https://bugs.libcamera.org/show_bug.cgi?id=32
	 */
	if (in.width < IF_CROP_MAX_W || in.height < IF_CROP_MAX_H) {
		LOG(IPU3, Error) << "Input resolution " << in.toString()
				 << " not supported";
		return {};
	}

And so here in ipu3.cpp we try to find a raw size from the sensor to
feed the ImgU with larger than [40, 450].

I'll move those definitions to imgu.h and use a more expressive
variable instead of the raw numbers.

Thanks
   j

> > +				    .boundedTo(data_->cio2_.sensor()->resolution());
> > +	cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);
> >  	if (!cio2Configuration_.pixelFormat.isValid())
> >  		return Invalid;
> >
> >