[{"id":31326,"web_url":"https://patchwork.libcamera.org/comment/31326/","msgid":"<20240923224939.GF7165@pendragon.ideasonboard.com>","date":"2024-09-23T22:49:39","subject":"Re: [PATCH v2 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP\n\tmaximum input","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Sep 13, 2024 at 04:07:58PM +0530, Umang Jain wrote:\n> The RkISP1Path class has maximum resolution defined by\n> RKISP1_RSZ_*_SRC_MAX macros. It might get updated in populateFormats()\n> by querying the formats on the rkisp1_*path video nodes. However, it\n> does not take into account the maximum resolution that the ISP can\n> handle.\n> \n> For instance on i.MX8MP, 4096x3072 is the maximum supported ISP\n> resolution however, RkISP1MainPath had the maximum resolution of\n> RKISP1_RSZ_MP_SRC_MAX, prior to this patch.\n> \n> Fix it by bounding the maximum resolution of RkISP1Path class by passing\n> the maximum resolution of the ISP during RkISP1Path::init().\n>\n> Signed-off-by: Umang Jain \n> Reviewed-by: Kieran Bingham \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 13 ++++++++++++-\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +-\n> 3 files changed, 26 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 0a794d63..00b0dad8 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1274,6 +1274,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \tif (isp_->open() < 0)\n> \t\treturn false;\n> \n> +\t/*\n> +\t * Retrieve the ISP maximum input size for config validation in the\n> +\t * path classes.\n> +\t *\n> +\t * The ISP maximum input size is independent of the media bus formats\n> +\t * hence, pick the one first entry of ispFormats and its size range.\n> +\t */\n> +\tconst V4L2Subdevice::Formats ispFormats = isp_->formats(0);\n> +\tconst SizeRange range = ispFormats.cbegin()->second[0];\n> +\tconst Size ispMaxInputSize = range.max;\n> +\n> \t/* Locate and open the optional CSI-2 receiver. */\n> \tispSink_ = isp_->entity()->getPadByIndex(0);\n> \tif (!ispSink_ || ispSink_->links().empty())\n> @@ -1300,10 +1311,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \t\treturn false;\n> \n> \t/* Locate and open the ISP main and self paths. */\n> -\tif (!mainPath_.init(media_))\n> +\tif (!mainPath_.init(media_, ispMaxInputSize))\n> \t\treturn false;\n> \n> -\tif (hasSelfPath_ && !selfPath_.init(media_))\n> +\tif (hasSelfPath_ && !selfPath_.init(media_, ispMaxInputSize))\n> \t\treturn false;\n> \n> \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index c49017d1..08b39e1e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -62,7 +62,7 @@ RkISP1Path::RkISP1Path(const char *name, const Span &formats,\n> {\n> }\n> \n> -bool RkISP1Path::init(MediaDevice *media)\n> +bool RkISP1Path::init(MediaDevice *media, Size ispMaxInputSize)\n> {\n> \tstd::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> \tstd::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> @@ -77,6 +77,17 @@ bool RkISP1Path::init(MediaDevice *media)\n> \n> \tpopulateFormats();\n> \n> +\t/*\n> +\t * The maximum size reported by the video node during populateFormats()\n> +\t * is hard coded to a fixed size which can exceed the platform specific\n\ns/platform specific/platform-specific/\n\n> +\t * ISP limitations.\n> +\t *\n> +\t * The video node should report a maximum size according to the ISP\n> +\t * model. This should be fixed in the kernel. For now, restrict the\n> +\t * maximum size to the ISP limitations correctly.\n\nFollowing the discussions on v1, do I understand correctly that the\nissue is that the ISP driver correctly reports the maximum size it\nsupports (rkisp1_isp_enum_frame_size() uses isp->rkisp1->info->max_width\nand isp->rkisp1->info->max_height), but that the resizer's output video\nnode doesn't take that into account ? If so, shouldn't that limitation\nbe handled in PipelineHandlerRkISP1::generateConfiguration() and/or\nRkISP1CameraConfiguration::validate() instead ? The resizer can upscale,\nso it seems strange to me to handle this in RkISP1Path. The pipeline\nhandler already code flow is already quite convoluted, I'd like to make\nthings clearer.\n\n> +\t */\n> +\tmaxResolution_.boundTo(ispMaxInputSize);\n> +\n> \tlink_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n> \tif (!link_)\n> \t\treturn false;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index 08edefec..13ba4b62 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -35,7 +35,7 @@ public:\n> \tRkISP1Path(const char *name, const Span &formats,\n> \t\t const Size &minResolution, const Size &maxResolution);\n> \n> -\tbool init(MediaDevice *media);\n> +\tbool init(MediaDevice *media, Size ispMaxInputSize);\n> \n> \tint setEnabled(bool enable) { return link_->setEnabled(enable); }\n> \tbool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AA9A6C3257\n\tfor ;\n\tMon, 23 Sep 2024 22:50:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F3A36350B;\n\tTue, 24 Sep 2024 00:50:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C43056037E\n\tfor ;\n\tTue, 24 Sep 2024 00:50:11 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4060F5B2;\n\tTue, 24 Sep 2024 00:48:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AcSyqwR8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727131725;\n\tbh=nBMPNQvcWBgQcJVVz/rnJeVBFlkBqOwsY6rYOCut8/c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AcSyqwR8Q+vY8a08eCjQGti9FZ+uoaozlvuKcKlTVQHwL7yk3BmOie1kiIIcX4hhC\n\t47m+phENdaKoNqjbRHvbekMLuotUsblzkbvcUHWxYYLo8xzVp0u3Nrv/UuxlyAEULU\n\tAwnTrYIj7ED6UEJYoZYmdXgQRL7+cMoeViCyU3r4=","Date":"Tue, 24 Sep 2024 01:49:39 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v2 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP\n\tmaximum input","Message-ID":"<20240923224939.GF7165@pendragon.ideasonboard.com>","References":"<20240913103759.2166-1-umang.jain@ideasonboard.com>\n\t<20240913103759.2166-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240913103759.2166-2-umang.jain@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31328,"web_url":"https://patchwork.libcamera.org/comment/31328/","msgid":"","date":"2024-09-24T06:10:09","subject":"Re: [PATCH v2 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP\n\tmaximum input","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 24/09/24 4:19 am, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Fri, Sep 13, 2024 at 04:07:58PM +0530, Umang Jain wrote:\n>> The RkISP1Path class has maximum resolution defined by\n>> RKISP1_RSZ_*_SRC_MAX macros. It might get updated in populateFormats()\n>> by querying the formats on the rkisp1_*path video nodes. However, it\n>> does not take into account the maximum resolution that the ISP can\n>> handle.\n>>\n>> For instance on i.MX8MP, 4096x3072 is the maximum supported ISP\n>> resolution however, RkISP1MainPath had the maximum resolution of\n>> RKISP1_RSZ_MP_SRC_MAX, prior to this patch.\n>>\n>> Fix it by bounding the maximum resolution of RkISP1Path class by passing\n>> the maximum resolution of the ISP during RkISP1Path::init().\n>>\n>> Signed-off-by: Umang Jain \n>> Reviewed-by: Kieran Bingham \n>> ---\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++--\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 13 ++++++++++++-\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +-\n>> 3 files changed, 26 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index 0a794d63..00b0dad8 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -1274,6 +1274,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>> \tif (isp_->open() < 0)\n>> \t\treturn false;\n>> \n>> +\t/*\n>> +\t * Retrieve the ISP maximum input size for config validation in the\n>> +\t * path classes.\n>> +\t *\n>> +\t * The ISP maximum input size is independent of the media bus formats\n>> +\t * hence, pick the one first entry of ispFormats and its size range.\n>> +\t */\n>> +\tconst V4L2Subdevice::Formats ispFormats = isp_->formats(0);\n>> +\tconst SizeRange range = ispFormats.cbegin()->second[0];\n>> +\tconst Size ispMaxInputSize = range.max;\n>> +\n>> \t/* Locate and open the optional CSI-2 receiver. */\n>> \tispSink_ = isp_->entity()->getPadByIndex(0);\n>> \tif (!ispSink_ || ispSink_->links().empty())\n>> @@ -1300,10 +1311,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>> \t\treturn false;\n>> \n>> \t/* Locate and open the ISP main and self paths. */\n>> -\tif (!mainPath_.init(media_))\n>> +\tif (!mainPath_.init(media_, ispMaxInputSize))\n>> \t\treturn false;\n>> \n>> -\tif (hasSelfPath_ && !selfPath_.init(media_))\n>> +\tif (hasSelfPath_ && !selfPath_.init(media_, ispMaxInputSize))\n>> \t\treturn false;\n>> \n>> \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> index c49017d1..08b39e1e 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> @@ -62,7 +62,7 @@ RkISP1Path::RkISP1Path(const char *name, const Span &formats,\n>> {\n>> }\n>> \n>> -bool RkISP1Path::init(MediaDevice *media)\n>> +bool RkISP1Path::init(MediaDevice *media, Size ispMaxInputSize)\n>> {\n>> \tstd::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n>> \tstd::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n>> @@ -77,6 +77,17 @@ bool RkISP1Path::init(MediaDevice *media)\n>> \n>> \tpopulateFormats();\n>> \n>> +\t/*\n>> +\t * The maximum size reported by the video node during populateFormats()\n>> +\t * is hard coded to a fixed size which can exceed the platform specific\n> s/platform specific/platform-specific/\n>\n>> +\t * ISP limitations.\n>> +\t *\n>> +\t * The video node should report a maximum size according to the ISP\n>> +\t * model. This should be fixed in the kernel. For now, restrict the\n>> +\t * maximum size to the ISP limitations correctly.\n> Following the discussions on v1, do I understand correctly that the\n> issue is that the ISP driver correctly reports the maximum size it\n> supports (rkisp1_isp_enum_frame_size() uses isp->rkisp1->info->max_width\n> and isp->rkisp1->info->max_height), but that the resizer's output video\n> node doesn't take that into account ? If so, shouldn't that limitation\n\nyes, that's the crux of the issue here. Hence, this seems more like a \nkernel fix?\n> be handled in PipelineHandlerRkISP1::generateConfiguration() and/or\n> RkISP1CameraConfiguration::validate() instead ? The resizer can upscale,\n> so it seems strange to me to handle this in RkISP1Path. The pipeline\n> handler already code flow is already quite convoluted, I'd like to make\n> things clearer.\n\nThe resizer can upscale yes, but should it be upscaling beyond ISP (or \neven maximum sensor resolution) limits ? No, right ?\n\n>\n>> +\t */\n>> +\tmaxResolution_.boundTo(ispMaxInputSize);\n>> +\n>> \tlink_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n>> \tif (!link_)\n>> \t\treturn false;\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>> index 08edefec..13ba4b62 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>> @@ -35,7 +35,7 @@ public:\n>> \tRkISP1Path(const char *name, const Span &formats,\n>> \t\t const Size &minResolution, const Size &maxResolution);\n>> \n>> -\tbool init(MediaDevice *media);\n>> +\tbool init(MediaDevice *media, Size ispMaxInputSize);\n>> \n>> \tint setEnabled(bool enable) { return link_->setEnabled(enable); }\n>> \tbool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2028EC0F1B\n\tfor ;\n\tTue, 24 Sep 2024 06:10:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 638286350B;\n\tTue, 24 Sep 2024 08:10:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A5E5A618DA\n\tfor ;\n\tTue, 24 Sep 2024 08:10:13 +0200 (CEST)","from [IPV6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f] (unknown\n\t[IPv6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 89462827;\n\tTue, 24 Sep 2024 08:08:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sbbtNiTz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727158127;\n\tbh=R7WdlUbibGT7ACOCKupDqseDrU8942idErFcr6uLasg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=sbbtNiTzC02lZtaxSO902f0VUkcDaPdn7IsP2YfdEGG0QRfBBo9mfCdk5tMmFLxCr\n\tGYtyR2Po+EgbIwj5Nbo5BN5oo+0+NnuFsEgNVOP7L5zavLxuONn+9xzyvGmqldtPp0\n\tPn+2E3ZGjQgRbNvfo640j2Ij5PHIzeQK4RscplbU=","Message-ID":"","Date":"Tue, 24 Sep 2024 11:40:09 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP\n\tmaximum input","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","References":"<20240913103759.2166-1-umang.jain@ideasonboard.com>\n\t<20240913103759.2166-2-umang.jain@ideasonboard.com>\n\t<20240923224939.GF7165@pendragon.ideasonboard.com>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<20240923224939.GF7165@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31356,"web_url":"https://patchwork.libcamera.org/comment/31356/","msgid":"<20240925115541.GA6976@pendragon.ideasonboard.com>","date":"2024-09-25T11:55:41","subject":"Re: [PATCH v2 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP\n\tmaximum input","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Tue, Sep 24, 2024 at 11:40:09AM +0530, Umang Jain wrote:\n> On 24/09/24 4:19 am, Laurent Pinchart wrote:\n> > On Fri, Sep 13, 2024 at 04:07:58PM +0530, Umang Jain wrote:\n> >> The RkISP1Path class has maximum resolution defined by\n> >> RKISP1_RSZ_*_SRC_MAX macros. It might get updated in populateFormats()\n> >> by querying the formats on the rkisp1_*path video nodes. However, it\n> >> does not take into account the maximum resolution that the ISP can\n> >> handle.\n> >>\n> >> For instance on i.MX8MP, 4096x3072 is the maximum supported ISP\n> >> resolution however, RkISP1MainPath had the maximum resolution of\n> >> RKISP1_RSZ_MP_SRC_MAX, prior to this patch.\n> >>\n> >> Fix it by bounding the maximum resolution of RkISP1Path class by passing\n> >> the maximum resolution of the ISP during RkISP1Path::init().\n> >>\n> >> Signed-off-by: Umang Jain \n> >> Reviewed-by: Kieran Bingham \n> >> ---\n> >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++--\n> >> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 13 ++++++++++++-\n> >> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +-\n> >> 3 files changed, 26 insertions(+), 4 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> index 0a794d63..00b0dad8 100644\n> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> @@ -1274,6 +1274,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> >> \tif (isp_->open() < 0)\n> >> \t\treturn false;\n> >> \n> >> +\t/*\n> >> +\t * Retrieve the ISP maximum input size for config validation in the\n> >> +\t * path classes.\n> >> +\t *\n> >> +\t * The ISP maximum input size is independent of the media bus formats\n> >> +\t * hence, pick the one first entry of ispFormats and its size range.\n> >> +\t */\n> >> +\tconst V4L2Subdevice::Formats ispFormats = isp_->formats(0);\n> >> +\tconst SizeRange range = ispFormats.cbegin()->second[0];\n> >> +\tconst Size ispMaxInputSize = range.max;\n> >> +\n> >> \t/* Locate and open the optional CSI-2 receiver. */\n> >> \tispSink_ = isp_->entity()->getPadByIndex(0);\n> >> \tif (!ispSink_ || ispSink_->links().empty())\n> >> @@ -1300,10 +1311,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> >> \t\treturn false;\n> >> \n> >> \t/* Locate and open the ISP main and self paths. */\n> >> -\tif (!mainPath_.init(media_))\n> >> +\tif (!mainPath_.init(media_, ispMaxInputSize))\n> >> \t\treturn false;\n> >> \n> >> -\tif (hasSelfPath_ && !selfPath_.init(media_))\n> >> +\tif (hasSelfPath_ && !selfPath_.init(media_, ispMaxInputSize))\n> >> \t\treturn false;\n> >> \n> >> \tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> >> index c49017d1..08b39e1e 100644\n> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> >> @@ -62,7 +62,7 @@ RkISP1Path::RkISP1Path(const char *name, const Span &formats,\n> >> {\n> >> }\n> >> \n> >> -bool RkISP1Path::init(MediaDevice *media)\n> >> +bool RkISP1Path::init(MediaDevice *media, Size ispMaxInputSize)\n> >> {\n> >> \tstd::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> >> \tstd::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> >> @@ -77,6 +77,17 @@ bool RkISP1Path::init(MediaDevice *media)\n> >> \n> >> \tpopulateFormats();\n> >> \n> >> +\t/*\n> >> +\t * The maximum size reported by the video node during populateFormats()\n> >> +\t * is hard coded to a fixed size which can exceed the platform specific\n> >\n> > s/platform specific/platform-specific/\n> >\n> >> +\t * ISP limitations.\n> >> +\t *\n> >> +\t * The video node should report a maximum size according to the ISP\n> >> +\t * model. This should be fixed in the kernel. For now, restrict the\n> >> +\t * maximum size to the ISP limitations correctly.\n> >\n> > Following the discussions on v1, do I understand correctly that the\n> > issue is that the ISP driver correctly reports the maximum size it\n> > supports (rkisp1_isp_enum_frame_size() uses isp->rkisp1->info->max_width\n> > and isp->rkisp1->info->max_height), but that the resizer's output video\n> > node doesn't take that into account ? If so, shouldn't that limitation\n> \n> yes, that's the crux of the issue here. Hence, this seems more like a \n> kernel fix?\n>\n> > be handled in PipelineHandlerRkISP1::generateConfiguration() and/or\n> > RkISP1CameraConfiguration::validate() instead ? The resizer can upscale,\n> > so it seems strange to me to handle this in RkISP1Path. The pipeline\n> > handler already code flow is already quite convoluted, I'd like to make\n> > things clearer.\n> \n> The resizer can upscale yes, but should it be upscaling beyond ISP (or \n> even maximum sensor resolution) limits ? No, right ?\n\nOn the kernel side, I don't think we should limit upscaling to the\nsensor resolution or maximum ISP input size. The kernel driver should\nexpose the full capabilities of the hardware. If the resizer can upscale\nto more than 4k x 3k, we shouldn't restrict it.\n\nWhat we can (and should) do in the kernel is to restrict upscaling to\nthe maximum supported by the hardware, in case upscaling too much would\nmake the ISP behave incorrectly. I suspect that that limit would be\ndynamic though, not a static value we could access here, but instead\nsomething to be handled at configuration validation time.\n\nThe problem can be segmented, you don't have to handle everything in one\ngo. We could decide to start without upscaling support in libcamera for\ninstance. As long as the code is kept clean, and limitations are\nproperly documented, I'm fine with that.\n\n> >> +\t */\n> >> +\tmaxResolution_.boundTo(ispMaxInputSize);\n> >> +\n> >> \tlink_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n> >> \tif (!link_)\n> >> \t\treturn false;\n> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> >> index 08edefec..13ba4b62 100644\n> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> >> @@ -35,7 +35,7 @@ public:\n> >> \tRkISP1Path(const char *name, const Span &formats,\n> >> \t\t const Size &minResolution, const Size &maxResolution);\n> >> \n> >> -\tbool init(MediaDevice *media);\n> >> +\tbool init(MediaDevice *media, Size ispMaxInputSize);\n> >> \n> >> \tint setEnabled(bool enable) { return link_->setEnabled(enable); }\n> >> \tbool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BEE9AC0F1B\n\tfor ;\n\tWed, 25 Sep 2024 11:55:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A17336350E;\n\tWed, 25 Sep 2024 13:55:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ECB0C634F4\n\tfor ;\n\tWed, 25 Sep 2024 13:55:44 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3D023A30;\n\tWed, 25 Sep 2024 13:54:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VkkySdBC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727265257;\n\tbh=nrCv8TQxjOQRP/op2YuLMRHp8w6RUhH8dMmjkS+ZqK8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VkkySdBCq/HBzWM1WX7DhgqsaNiGVjclWOVkNQLdXX7IBtHE4oHJoRMRUbLJJiRx0\n\to+TKWzeIMhm96b0yIzlvYFjsMKfC5013FKBpxcqDCdULZxAbb5CATuTBRz7ZuDi924\n\t0IyODfSdHydrtMbyUxzsqfeabkBsMIYw2euPl7jc=","Date":"Wed, 25 Sep 2024 14:55:41 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v2 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP\n\tmaximum input","Message-ID":"<20240925115541.GA6976@pendragon.ideasonboard.com>","References":"<20240913103759.2166-1-umang.jain@ideasonboard.com>\n\t<20240913103759.2166-2-umang.jain@ideasonboard.com>\n\t<20240923224939.GF7165@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]