[PATCH 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP maximum input

[Laurent Pinchart]

On Wed, Sep 11, 2024 at 12:30:52PM +0100, Kieran Bingham wrote:
> Quoting Umang Jain (2024-09-09 17:37:18)
> > The RkISP1Path class has maximum resolution defined by
> > RKISP1_RSZ_*_SRC_MAX macros. It might get updated in populateFormats()
> > by querying the formats on the rkisp1_*path video nodes. However, it
> > does not take into account the maximum resolution that the ISP can
> > handle.
> > 
> > For instance on i.MX8MP, 4096x3072 is the maximum supported ISP
> > resolution however, RkISP1MainPath had the maximum resolution of
> > RKISP1_RSZ_MP_SRC_MAX, prior to this patch.
> 
> Does this mean that the kernel is reporting the wrong maximum sizes on
> the path video node?

Isn't it the other way around, with RKISP1_RSZ_MP_SRC_MAX being wrong,
and the kernel reporting the right value when querying the video node ?
Different ISP versions have different limits, so we can't hardcode a
single one.  We could hardcode different limits for different versions
in libcamera, but querying the limit dynamically is probably better.

> Seems like this should be a kernel side fix too ( but the fact that
> there are kernels that do not report the right size, means at least we
> need a libcamera side fix here now anyway).
> 
> > Fix it by bounding the maximum resolution of RkISP1Path class by passing
> > the maximum resolution of the ISP during RkISP1Path::init().
> > 
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 15 +++++++++++++--
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp |  4 +++-
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  2 +-
> >  3 files changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 0a794d63..97ce8457 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1274,6 +1274,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> >         if (isp_->open() < 0)
> >                 return false;
> >  
> > +       /*
> > +        * Retrieve the ISP maximum input size for config validation in the
> > +        * path classes.
> > +        *
> > +        * The ISP maximum input size is independent of the media bus formats
> > +        * hence, pick the one first entry of ispFormats and its size range.
> > +        */
> > +       V4L2Subdevice::Formats ispFormats = isp_->formats(0);

const ?

> > +       const SizeRange range = ispFormats.cbegin()->second[0];
> > +       Size ispMaxInputSize = range.max;

const ?

> > +
> >         /* Locate and open the optional CSI-2 receiver. */
> >         ispSink_ = isp_->entity()->getPadByIndex(0);
> >         if (!ispSink_ || ispSink_->links().empty())
> > @@ -1300,10 +1311,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> >                 return false;
> >  
> >         /* Locate and open the ISP main and self paths. */
> > -       if (!mainPath_.init(media_))
> > +       if (!mainPath_.init(media_, ispMaxInputSize))
> >                 return false;
> >  
> > -       if (hasSelfPath_ && !selfPath_.init(media_))
> > +       if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInputSize))
> >                 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 c49017d1..9053af18 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, Size ispMaxInputSize)
> >  {
> >         std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
> >         std::string video = std::string("rkisp1_") + name_ + "path";
> > @@ -77,6 +77,8 @@ bool RkISP1Path::init(MediaDevice *media)
> >  
> >         populateFormats();
> >  
> > +       maxResolution_.boundTo(ispMaxInputSize);
> > +
> 
> I think this really needs a comment to explain...
> 
> 	/*
> 	 * The maximum size reported by the video node during populate
> 	 * formats is hard coded on some kernels to a fixed size which
> 	 * can exceed the platform specific ISP limitations.
> 	 *
> 	 * While this should be fixed in the kernel, restrict to the ISP
> 	 * limitations correctly.

I don't think that's right, see above. Did I miss something ?

> 	 */
> 
> or such ?
> 
> >         link_ = media->link("rkisp1_isp", 2, resizer, 0);
> >         if (!link_)
> >                 return false;
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > index 08edefec..13ba4b62 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, Size ispMaxInputSize);
> 
> I hoped this would be something we could set during the constructor -
> but it would require changing the allocations and constructs of those
> and it's probably not worth it for now so I could live with this.
> 
> With comments to report /why/ we are clamping the sizes:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >         int setEnabled(bool enable) { return link_->setEnabled(enable); }
> >         bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }

-- 
Regards,

Laurent Pinchart