[libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation when sensor max is larger than ISP input
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 17 16:57:15 CET 2024
On Wed, Jan 17, 2024 at 11:43:43AM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Paul Elder via libcamera-devel (2024-01-16 09:17:53)
> > If the maximum sensor output size is larger than the maximum ISP input
> > size, the maximum sensor size could be selected and the pipeline would
> > fail with an EPIPE. Fix this by clamping the sensor size to the ISP
> > input size at validation time.
>
> I think we should do more here (in a separate patch) to filter out
> sensor modes that are larger than the ISP max as well.
>
> At the moment it seems too easy for the rkisp1 pipeline handler to
> select an input configuration that it can't actually configure.
>
> But that's aside from this patch itself.
>
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 ++++++++++--
> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------
> > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 ++-
> > 3 files changed, 30 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 586b46d64..fb67ba8f4 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1202,11 +1202,24 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> > if (param_->open() < 0)
> > return false;
> >
> > + /*
> > + * Retrieve ISP maximum input size for config validation in the path
s/Retrieve/Retrieve the/
> > + * classes
s/$/./
> > + */
> > + Size ispMaxInSize = { 0, 0 };
That's the default, so you can just write
Size ispMaxInSize;
I also find ispMaxInputSize more readable.
> > + V4L2Subdevice::Formats ispFormats = isp_->formats(0);
const if you don't need to modify it.
> > + for (const auto &[mbus, sizes] : ispFormats) {
> > + for (const auto &size : sizes) {
> > + if (ispMaxInSize < size.max)
> > + ispMaxInSize = size.max;
> > + }
> > + }
Do the maximum input size depend on the media bus format ? And to we
have multiple sizes in the sizes vector ? We could take some shortcuts,
knowing how the ISP driver is implemented, taking the first entry in
both ispFormats and sizes.
> > +
> > /* Locate and open the ISP main and self paths. */
> > - if (!mainPath_.init(media_))
> > + if (!mainPath_.init(media_, ispMaxInSize))
> > return false;
> >
> > - if (hasSelfPath_ && !selfPath_.init(media_))
> > + if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInSize))
> > return false;
> >
> > mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > index b62b645ca..eaab7e857 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > @@ -62,7 +62,7 @@ RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
> > {
> > }
> >
> > -bool RkISP1Path::init(MediaDevice *media)
> > +bool RkISP1Path::init(MediaDevice *media, const Size &ispMaxInSize)
ispMaxInputSize here too if you rename it above. Same below.
> It feels a bit odd to me passing ispMaxInSize as the only parameter to
> initialize. But I guess it save init from re-determining it for each
> path, and maybe it's the only variable for now - so I think we're ok
> here.
>
> I wonder if we should iterate and 'try' configurations to only expose
> those which can be supported. But we can tackle that on top I think.
Agreed.
> So for now I am ok with this:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
with the above comments taken into account.
>
> > {
> > std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
> > std::string video = std::string("rkisp1_") + name_ + "path";
> > @@ -76,6 +76,7 @@ bool RkISP1Path::init(MediaDevice *media)
> > return false;
> >
> > populateFormats();
> > + ispMaxInSize_ = ispMaxInSize;
> >
> > link_ = media->link("rkisp1_isp", 2, resizer, 0);
> > if (!link_)
> > @@ -172,7 +173,9 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
> > /* Add all the RAW sizes the sensor can produce for this code. */
> > for (const auto &rawSize : sensor->sizes(mbusCode)) {
> > if (rawSize.width > maxResolution_.width ||
> > - rawSize.height > maxResolution_.height)
> > + rawSize.height > maxResolution_.height ||
> > + rawSize.width > ispMaxInSize_.width ||
> > + rawSize.height > ispMaxInSize_.height)
> > continue;
> >
> > streamFormats[format].push_back({ rawSize, rawSize });
> > @@ -275,8 +278,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> > if (!found)
> > cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;
> >
> > - Size minResolution;
> > - Size maxResolution;
> > + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > + .boundedTo(resolution);
> > + Size minResolution = minResolution_.expandedToAspectRatio(resolution);
> >
> > if (isRaw) {
> > /*
> > @@ -287,16 +291,10 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> > V4L2SubdeviceFormat sensorFormat =
> > sensor->getFormat({ mbusCode }, cfg->size);
> >
> > - minResolution = sensorFormat.size;
> > - maxResolution = sensorFormat.size;
> > - } else {
> > - /*
> > - * Adjust the size based on the sensor resolution and absolute
> > - * limits of the ISP.
> > - */
> > - minResolution = minResolution_.expandedToAspectRatio(resolution);
> > - maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > - .boundedTo(resolution);
> > + if (!sensorFormat.size.isNull()) {
> > + minResolution = sensorFormat.size.boundedTo(ispMaxInSize_);
> > + maxResolution = sensorFormat.size.boundedTo(ispMaxInSize_);
> > + }
> > }
> >
> > cfg->size.boundTo(maxResolution);
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > index cd77957ee..c96bd5557 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > @@ -35,7 +35,7 @@ public:
> > RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
> > const Size &minResolution, const Size &maxResolution);
> >
> > - bool init(MediaDevice *media);
> > + bool init(MediaDevice *media, const Size &ispMaxInSize);
> >
> > int setEnabled(bool enable) { return link_->setEnabled(enable); }
> > bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
> > @@ -77,6 +77,8 @@ private:
> > std::unique_ptr<V4L2Subdevice> resizer_;
> > std::unique_ptr<V4L2VideoDevice> video_;
> > MediaLink *link_;
> > +
> > + Size ispMaxInSize_;
> > };
> >
> > class RkISP1MainPath : public RkISP1Path
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list