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

[Umang Jain]

Hi Laurent,

On 24/09/24 4:19 am, Laurent Pinchart wrote:
> 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

yes, that's the crux of the issue here. Hence, this seems more like a 
kernel fix?
> 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.

The resizer can upscale yes, but should it be upscaling beyond ISP (or 
even maximum sensor resolution) limits ? No, right ?

>
>> +	 */
>> +	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; }