[1/3] libcamera: pipeline: rkisp1: Detect invalid sensor configurations
diff mbox series

Message ID 20250501141609.717148-2-kieran.bingham@ideasonboard.com
State New
Headers show
Series
  • libcamera: rkisp1: Camera Sensor Mode Validation
Related show

Commit Message

Kieran Bingham May 1, 2025, 2:16 p.m. UTC
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@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Jacopo Mondi May 2, 2025, 8:21 a.m. UTC | #1
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@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

  	sensorFormat_ = sensor->getFormat(mbusCodes, maxSize,
  					  mainPath->maxResolution());

Should guarantee that the returned resolution is never larger than
mainPath->maxResolution() ?


> +	}
> +
>  	return status;
>  }
>
> --
> 2.48.1
>
Kieran Bingham May 2, 2025, 10:09 a.m. UTC | #2
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@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
> >
Jacopo Mondi May 2, 2025, 10:17 a.m. UTC | #3
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@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
> > >

Patch
diff mbox series

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());
 
+
+	/*
+	 * 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.
+	 */
 	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;
+	}
+
 	return status;
 }