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

[Kieran Bingham]

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?

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 SizeRange range = ispFormats.cbegin()->second[0];
> +       Size ispMaxInputSize = range.max;
> +
>         /* 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.
	 */

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; }
> -- 
> 2.45.0
>