[PATCH 1/3] libcamera: pipeline: rkisp1: Detect invalid sensor configurations

[Kieran Bingham]

Quoting Jacopo Mondi (2025-05-02 09:21:13)
> Hi Kieran
> 
> On Thu, May 01, 2025 at 03:16:07PM +0100, Kieran Bingham wrote:
> > If we select a Sensor Format that is larger than the ISP capabilities
> > the pipeline will not be able to successfully start.
> >
> > Detect this during validate - and report accordingly, returning
> > an 'Invalid' state to reflect that we were not able to
> > reconfigure to adjust to a working state in this instance.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 675f0a7490a6..d8c6100946bc 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -673,9 +673,21 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >       sensorFormat_ = sensor->getFormat(mbusCodes, maxSize,
> >                                         mainPath->maxResolution());
> >
> > +
> 
> Additional blank line
> 
> > +     /*
> > +      * TODO: There doesn't seem to be a valid occasion to set the size to
> > +      * the native resolution if there was not a supported size found above.
> 
> It might be me, but I can't parse this
> 
> > +      */
> >       if (sensorFormat_.size.isNull())
> >               sensorFormat_.size = sensor->resolution();
> >
> > +     if (sensorFormat_.size > mainPath->maxResolution()) {
> > +             LOG(RkISP1, Error)
> > +                     << "Sensor format size " << sensorFormat_.size
> > +                     << " exceeds maximum possible size " << mainPath->maxResolution();
> > +             return Invalid;
> 
> This should never happen as

Aha, so you see A can't happen and I see B can't happen ;-)

I think somewhere there's a logic change as part of the dewarp rotation
enablement which /isn't/ in this tree but didn't prevent me applying
these patches on mainline - so I could perhaps say this patch needs to
be part of the dewarp support instead of targetting mainline already.

I also think there's something I've probably missed elsewhere so I
should re-parse the logic paths here...

>         sensorFormat_ = sensor->getFormat(mbusCodes, maxSize,
>                                           mainPath->maxResolution());
> 
> Should guarantee that the returned resolution is never larger than
> mainPath->maxResolution() ?


But it can return a null format (As seen by the previous debug
enablement patch) which then leads to :


> >       if (sensorFormat_.size.isNull())
> >               sensorFormat_.size = sensor->resolution();


And in this case I have an IMX283 20MP camera connected which is a
larger resolution than the ISP can parse.

So we're suddenly setting an invalid config, that could never be
configured.



> 
> 
> > +     }
> > +
> >       return status;
> >  }
> >
> > --
> > 2.48.1
> >