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

[Laurent Pinchart]

Hi Jacopo,


On Mon, Oct 11, 2021 at 10:18:17AM +0200, Jacopo Mondi wrote:
> On Wed, Sep 22, 2021 at 04:51:26AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > (Resending with Umang's correct e-mail address now)
> >
> > On Tue, Sep 07, 2021 at 09:40:51PM +0200, 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 uses 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.
> >
> > On a side note, I missed reviewing the changes in cio2.cpp, doesn't that
> > belong to ipu3.cpp instead ?
> 
> $ git show 5fc426fbfe58a82e30021d7a9ca12a4daeaec0f3 --stat
>  src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++

I meant doesn't CIO2Device::getSensorFormat() belong to ipu3.cpp ?

> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 41 ++++++++++++++++------------
> > >  1 file changed, 23 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index c287bf86e79a..291338288685 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);
> >
> > We support a single raw stream only, is this change needed ?
> 
> It makes the two branches look the same
> 
> > >  		} else {
> > >  			yuvCount++;
> > >  			maxYuvSize.expandTo(cfg.size);
> > > @@ -269,24 +278,20 @@ 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).
> > > +	 * The output YUV streams will be limited in size to the
> > > +	 * maximum frame size requested for the RAW stream, if present.
> >
> > This could be reflowed.
> >
> > >  	 *
> > > -	 * 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.
> > > +	 * If no raw stream is requested generate a size as large as the maximum
> > > +	 * requested YUV size aligned to the ImgU constraints and bound by the
> > > +	 * sensor's maximum resolution. See
> > > +	 * https://bugs.libcamera.org/show_bug.cgi?id=32
> > >  	 */
> > > -	if (!yuvCount)
> > > -		cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);
> > > -	else
> > > -		cio2Configuration_ = data_->cio2_.generateConfiguration({});
> > > +	if (rawSize.isNull())
> > > +		rawSize = maxYuvSize.alignedUpTo(40, 540)
> >
> > Is alignedUpTo() the right function ? It will round up the height to a
> > multiple of 540, that doesn't sound right.
> >
> 
> No it should probably be expandedTo()
> 
> > > +				    .alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
> > > +						 IMGU_OUTPUT_HEIGHT_MARGIN)
> >
> > Same here.
> 
> While I guess this one is correct as we need to align up to the
> margins

Are margins alignments, or a number of pixels to be added because they
will be cropped by the processing blocks ?

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

-- 
Regards,

Laurent Pinchart