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

[Jacopo Mondi]

Hi Niklas,

On Thu, Jul 09, 2020 at 03:46:54PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> 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.

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, 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 ?


> >  	if (!cio2Configuration_.pixelFormat.isValid())
> >  		return Invalid;
> >
> > +	LOG(IPU3, Debug) << "CIO2 configuration: " << cio2Configuration_.toString();
> > +
> >  	/*
> >  	 * Adjust the configurations if needed and assign streams while
> >  	 * iterating them.
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund