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

[Jacopo Mondi]

Hi Kieran

On Fri, May 02, 2025 at 11:09:49AM +0100, Kieran Bingham wrote:
> 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.

Probably better, to get a more complete picture

>
> 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.

Right. Should we simply return Invalid instead of trying to adjust to
something that can't be guaranteed to work ?

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