[libcamera-devel] [PATCH v2 12/20] libcamera: ipu3: Always use the maximum frame size

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 10, 2020 at 09:38:53AM +0200, Jacopo Mondi wrote:
> On Thu, Jul 09, 2020 at 03:46:54PM +0200, Niklas Söderlund wrote:
> > On 2020-07-09 10:41:20 +0200, Jacopo Mondi wrote:
> > > The requirement of having the ImgU output height 32 pixels smaller
> > > than the input frame produced by the CIO2 makes it complicated to
> > > re-adjust the sensor produced size after the alignement has been
> > > applied.
> > >
> > > To simplify the procedure, always ask for the full frame size from the
> > > CIO2 unit.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++--------
> > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index d07f1a7b5ae8..feabffe641e1 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -156,8 +156,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > >  	unsigned int rawCount = 0;
> > >  	unsigned int outCount = 0;
> > >  	Size yuvSize;
> > > -	Size size;
> > > -
> > >  	for (const StreamConfiguration &cfg : config_) {
> > >  		const PixelFormatInfo &info =
> > >  			PixelFormatInfo::info(cfg.pixelFormat);
> > > @@ -174,11 +172,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > >  			if (cfg.size > yuvSize)
> > >  				yuvSize = cfg.size;
> > >  		}
> > > -
> > > -		if (cfg.size.width > size.width)
> > > -			size.width = cfg.size.width;
> > > -		if (cfg.size.height > size.height)
> > > -			size.height = cfg.size.height;
> > >  	}
> > >  	if (rawCount > 1 || outCount > 2) {
> > >  		LOG(IPU3, Error)
> > > @@ -189,10 +182,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > >  	}
> > >
> > >  	/* Generate raw configuration from CIO2. */
> > > -	cio2Configuration_ = data_->cio2_.generateConfiguration(size);
> > > +	Size sensorSize = data_->cio2_.sensor()->resolution();
> > > +	cio2Configuration_ = data_->cio2_.generateConfiguration(sensorSize);
> >
> > Could this not impact framerate for the CIO2 if we always request the
> > largest sensor size? Would it be possible instead to increase the size
> > passed to CIO2Device::generateConfiguration() by 32?
> 
> Yes it does, and this is probably just wrong, as it ignores the
> requested raw capture size.
> 
> Fun fact which actually lead me to add this change: if I keep the
> previous version where the sensor is asked for the smaller as possible
> resolution that supports all stream (+32 as you here suggest) the IPU3
> pipeline parameter calculations fail on Soraka for some output
> resolutions.
> 
> In example: calculating the configuration for a single stream in
> 1280x720 with a frame resolution of 2112x1188 (which is the smaller
> ov13858 resolution that is large enough) fails: the tool is not able
> to find any suitable configuration that satisfied the pipeline
> constraints:
> 
> $ python pipe_config.py input=2112x1188 main=1280x720
> IndexError: list index out of range
> 
> If I use the full frame size, everything is unicorns&rainbows again
> 
> $ python pipe_config.py input=4224x3136 main=1280x720
> -------- The selected pipe configuration: --------------
> output_res_if:4220x2600
> output_res_bds:1688x1040
> output_res_gdc:1280x720
> FOV Horizontal:0.757576, FOV vertical:0.573980
> 
> Why ? I have no idea, and I think we should stop waiting for Intel to
> actually care about this platform anymore, so I don't expect they
> could help us debugging the tool.

What's the alternative, to you want to debug it yourself ? :-) If there
are issue, please report them, and you can CC Bingbu and Tomasz.

> The re-implementation I made performs the same calculations as the
> script, hence reducing the frame size makes a lot of configurations
> fail.
> 
> If you look at
> https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov5670.xml
> https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov13858.xml
> 
> The sensor is always asked for the full frame,

Is it ? For the ov5670 I see three configurations using 1296x972, and
for the ov13858 I see nine configurations with smaller sensor
resolutions.

> so I suspect that's how
> the system is intenteded to work (even if, I wonder, what happens if I
> connect a sensor that can only produce the above failing 2112x1188
> resolution ?)
> 
> I -fear- for IPU3 we should always go full frame size to make the pipe
> config happy, or there's maybe a requirement to respect some aspect
> ratio I still haven't been able to figure out. In any case, this would
> hinder the ability to capture raw+YUV, as the raw frame resolution in
> that case is selected from the application, and it might prevent the
> pipeline from being able to calculate a configuration.
> 
> Not nice, I know.
> 
> If that's how thing have to work, a software scaler for raw frames
> would nee to be plugged to the pipeline to down-scale raw frames to
> the desired resolution. Or we restrict RAW streams to the full size
> only. What do you think ?

No software scaler, no. If a configuration isn't supported by the
hardware we can obviously reject it. For instance if the application
requests raw in 640x480 and wants YUV in a larger resolution, as the
ImgU can't downscale, we can't accept that, and we can go for a higher
raw resolution or a small YUV resolution. There's nothing new under the
sun here, and I'm OK with temporarily restricting the raw frames to the
full sensor resolution if it helps moving forward, but let's keep in
mind we may have to fix that at some point in the future, so we
shouldn't hardcode too much.

> > >  	if (!cio2Configuration_.pixelFormat.isValid())
> > >  		return Invalid;
> > >
> > > +	LOG(IPU3, Debug) << "CIO2 configuration: " << cio2Configuration_.toString();
> > > +
> > >  	/*
> > >  	 * Adjust the configurations if needed and assign streams while
> > >  	 * iterating them.

-- 
Regards,

Laurent Pinchart