[PATCH] libcamera: rkisp1: Clamp stream configuration to ISP limit on raw path

[Jacopo Mondi]

Hi Umang

On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote:
> Hi Jacopo
>
> On 04/10/24 3:07 pm, Jacopo Mondi wrote:
> > Hi Umang
> >
> > On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote:
> > > Commit 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not
> > > supported by the pipeline") introduced a mechanism to determine maximum
> > > supported sensor resolution and filter out resolutions that cannot be
> > > supported by the ISP.
> > >
> > > However, it missed to update the raw stream configuration path, where
> > > it should have clamped the raw stream configuration size to the maximum
> > > sensor supported resolution.
> > >
> > > This patch fixes the above issue and can be confirmed with IMX283
> > > on i.MX8MP:
> > >
> > > From:
> > > ($) cam -c1 -srole=raw,width=5472,height=3072
> > > INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12
> > > ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12
> > > Failed to configure camera
> > > Failed to start camera session
> > >
> > > To:
> > > ($) cam -c1 -srole=raw,width=5472,height=3072
> > > INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12
> > > cam0: Capture until user interrupts by SIGINT
> > > 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824
> > > 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824
> > > 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824
> > > ...
> > >
> > > Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline")
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > >   src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > index da8d25c3..feb6d89f 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> > >   	if (isRaw) {
> > >   		/*
> > >   		 * Use the sensor output size closest to the requested stream
> > > -		 * size.
> > > +		 * size while ensuring the output size doesn't exceed ISP limits.
> > >   		 */
> > >   		uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
> > > +		cfg->size.boundTo(resolution);
> > > +
> > >   		V4L2SubdeviceFormat sensorFormat =
> > >   			sensor->getFormat({ mbusCode }, cfg->size);
> > CameraSensor::getFormat() returns a sensor resolution large enough to
> > accomodate the requested size, at least that's how I read the
> > following part of the documentation
> >
> >   * \a size indicates the desired size at the output of the sensor. This function
> >   * selects the best media bus code and size supported by the sensor according
> >   * to the following criteria.
> >   *
> >   * - The desired \a size shall fit in the sensor output size to avoid the need
> >   *   to up-scale.
> >
> >   And this part of the code
> >
> >                  if (sz.width < size.width || sz.height < size.height)
> >                          continue;
> >
> > So, at least in my understanding "sensorFormat" could represent a size
> > larger than cfg->size. Is this your understanding as well ?
>
> "sensorFormat" could represent a size larger than cfg->size
>
> and
>
> could also represent a size large than max supported resolution (i.e.
> 'resolution' variable in this case)
>
> For e.g. in IMX283 case it would,  5472x3648
>
> >
> > In the lines below this function we have
> >
> > 		minResolution = sensorFormat.size;
> > 		maxResolution = sensorFormat.size;
>
> From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648
> >
> >          cfg->size.boundTo(maxResolution);
> >          cfg->size.expandTo(minResolution);
> >
> > So if ((maxResolution = minResolution) > cfg->size) will the calls to
> > boundTo() followed by expandTo() enlarge cfg->size ?
>
> so cfg->size is 5472x3648 here, which can't be supported.

Sorry, I'm confused. Aren't you confirming me that the resulting
cfg->size cannot be supported (with this patch I mean) ?

>
> Have you noticed the failure case I mentioned in the commit message?

While the commmit message suggests this patch makes it valid ?

What am I missing ? :)

> >
> > Wouldn't it be better to use 'resolution' in
> >
> > 		V4L2SubdeviceFormat sensorFormat =
> > 			sensor->getFormat({ mbusCode }, cfg->size);
> >
> > instead of cfg->size ?
>
> No, otherwise it will always pick the highest sensor output size.
>
> For e.g. I request -srole=raw,width=1920,height=1080
>
> "cfg->size" will pick 2736x1824 for IMX283
>
> "resolution" will always pick 4096x3072.
> >
> > Thanks
> >    j
> >
> > > --
> > > 2.45.2
> > >
>