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

[Kieran Bingham]

Quoting Stefan Klug (2024-10-09 12:00:41)
> Hi Umang,
> 
> Thank you for the patch. 
> 
> On Wed, Oct 09, 2024 at 03:09:19PM +0530, 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>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 
> Looks good to me.
> 
> Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com> 
> 
> > ---
> > Changes in v2:
> > - Extend the comment on what 'resolution' var is and denote that
> >   sensor->getFormat() will never return something greter than
> >  'resolution'
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > index 3b5bea96..1999094e 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > @@ -326,9 +326,15 @@ 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.
> > +              *
> > +              * As 'resolution' is the largest sensor resolution
> > +              * supported by the ISP, CameraSensor::getFormat() will never
> > +              * return a V4L2SubdeviceFormat with a larger size.
> >                */
> >               uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
> > +             cfg->size.boundTo(resolution);
> > +

I wonder if 'resolution' should be better named, but I think this is
fine now.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> >               Size rawSize = sensorConfig ? sensorConfig->outputSize
> >                                           : cfg->size;
> >  
> > -- 
> > 2.45.2
> >