| Message ID | 20250501141609.717148-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 >
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 > >
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 > > >
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; }
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(+)