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

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Fri, Sep 13, 2024 at 04:07:58PM +0530, Umang Jain wrote:
> 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.
> 
> 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>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 15 +++++++++++++--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 13 ++++++++++++-
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  2 +-
>  3 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 0a794d63..00b0dad8 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.
> +	 */
> +	const V4L2Subdevice::Formats ispFormats = isp_->formats(0);
> +	const SizeRange range = ispFormats.cbegin()->second[0];
> +	const 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..08b39e1e 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,17 @@ bool RkISP1Path::init(MediaDevice *media)
>  
>  	populateFormats();
>  
> +	/*
> +	 * The maximum size reported by the video node during populateFormats()
> +	 * is hard coded to a fixed size which can exceed the platform specific

s/platform specific/platform-specific/

> +	 * ISP limitations.
> +	 *
> +	 * The video node should report a maximum size according to the ISP
> +	 * model. This should be fixed in the kernel. For now, restrict the
> +	 * maximum size to the ISP limitations correctly.

Following the discussions on v1, do I understand correctly that the
issue is that the ISP driver correctly reports the maximum size it
supports (rkisp1_isp_enum_frame_size() uses isp->rkisp1->info->max_width
and isp->rkisp1->info->max_height), but that the resizer's output video
node doesn't take that into account ? If so, shouldn't that limitation
be handled in PipelineHandlerRkISP1::generateConfiguration() and/or
RkISP1CameraConfiguration::validate() instead ? The resizer can upscale,
so it seems strange to me to handle this in RkISP1Path. The pipeline
handler already code flow is already quite convoluted, I'd like to make
things clearer.

> +	 */
> +	maxResolution_.boundTo(ispMaxInputSize);
> +
>  	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);
>  
>  	int setEnabled(bool enable) { return link_->setEnabled(enable); }
>  	bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }

-- 
Regards,

Laurent Pinchart