[{"id":31168,"web_url":"https://patchwork.libcamera.org/comment/31168/","msgid":"<172605425249.1037309.9008167754412389745@ping.linuxembedded.co.uk>","date":"2024-09-11T11:30:52","subject":"Re: [PATCH 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP maximum\n\tinput","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2024-09-09 17:37:18)\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\nDoes this mean that the kernel is reporting the wrong maximum sizes on\nthe path video node?\n\nSeems like this should be a kernel side fix too ( but the fact that\nthere are kernels that do not report the right size, means at least we\nneed a libcamera side fix here now anyway).\n\n\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> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +-\n> 3 files changed, 17 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 0a794d63..97ce8457 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> if (isp_->open() < 0)\n> return false;\n> \n> + /*\n> + * Retrieve the ISP maximum input size for config validation in the\n> + * path classes.\n> + *\n> + * The ISP maximum input size is independent of the media bus formats\n> + * hence, pick the one first entry of ispFormats and its size range.\n> + */\n> + V4L2Subdevice::Formats ispFormats = isp_->formats(0);\n> + const SizeRange range = ispFormats.cbegin()->second[0];\n> + Size ispMaxInputSize = range.max;\n> +\n> /* Locate and open the optional CSI-2 receiver. */\n> ispSink_ = isp_->entity()->getPadByIndex(0);\n> if (!ispSink_ || ispSink_->links().empty())\n> @@ -1300,10 +1311,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> return false;\n> \n> /* Locate and open the ISP main and self paths. */\n> - if (!mainPath_.init(media_))\n> + if (!mainPath_.init(media_, ispMaxInputSize))\n> return false;\n> \n> - if (hasSelfPath_ && !selfPath_.init(media_))\n> + if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInputSize))\n> return false;\n> \n> mainPath_.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..9053af18 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> std::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> std::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> @@ -77,6 +77,8 @@ bool RkISP1Path::init(MediaDevice *media)\n> \n> populateFormats();\n> \n> + maxResolution_.boundTo(ispMaxInputSize);\n> +\n\nI think this really needs a comment to explain...\n\n\t/*\n\t * The maximum size reported by the video node during populate\n\t * formats is hard coded on some kernels to a fixed size which\n\t * can exceed the platform specific ISP limitations.\n\t *\n\t * While this should be fixed in the kernel, restrict to the ISP\n\t * limitations correctly.\n\t */\n\nor such ?\n\n> link_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n> if (!link_)\n> return 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> RkISP1Path(const char *name, const Span &formats,\n> const Size &minResolution, const Size &maxResolution);\n> \n> - bool init(MediaDevice *media);\n> + bool init(MediaDevice *media, Size ispMaxInputSize);\n\nI hoped this would be something we could set during the constructor -\nbut it would require changing the allocations and constructs of those\nand it's probably not worth it for now so I could live with this.\n\nWith comments to report /why/ we are clamping the sizes:\n\nReviewed-by: Kieran Bingham \n\n> \n> int setEnabled(bool enable) { return link_->setEnabled(enable); }\n> bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n> -- \n> 2.45.0\n>","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 09861C324C\n\tfor ;\n\tWed, 11 Sep 2024 11:30:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9294D634F9;\n\tWed, 11 Sep 2024 13:30:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81E50618F2\n\tfor ;\n\tWed, 11 Sep 2024 13:30:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B075EBEB;\n\tWed, 11 Sep 2024 13:29:37 +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=\"iLUYj4fg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726054177;\n\tbh=MtDvjnVCKpbr+35ujxwdhfeBCnVNpt3Yf6ruT/9yfAI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=iLUYj4fgrQwzZ7VAo4YqaMXSxtT8Paly9PenQA4+/5lKxi7fHE/ne5FZhEbbOLxRu\n\tuUzC/IDa2CFKYUxbnFZqjEgPFNdTTV1BXRK7+nGG4cEyoA2yEOD4Kxeanx2oHYUuWj\n\tXxl6FipZy6t1Wi1S+r2igGCo61RuSdXpjHpC0jOw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240909163719.267211-2-umang.jain@ideasonboard.com>","References":"<20240909163719.267211-1-umang.jain@ideasonboard.com>\n\t<20240909163719.267211-2-umang.jain@ideasonboard.com>","Subject":"Re: [PATCH 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP maximum\n\tinput","From":"Kieran Bingham ","Cc":"Umang Jain ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 11 Sep 2024 12:30:52 +0100","Message-ID":"<172605425249.1037309.9008167754412389745@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":31178,"web_url":"https://patchwork.libcamera.org/comment/31178/","msgid":"<20240911202614.GI4470@pendragon.ideasonboard.com>","date":"2024-09-11T20:26:14","subject":"Re: [PATCH 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP maximum\n\tinput","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Sep 11, 2024 at 12:30:52PM +0100, Kieran Bingham wrote:\n> Quoting Umang Jain (2024-09-09 17:37:18)\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> Does this mean that the kernel is reporting the wrong maximum sizes on\n> the path video node?\n\nIsn't it the other way around, with RKISP1_RSZ_MP_SRC_MAX being wrong,\nand the kernel reporting the right value when querying the video node ?\nDifferent ISP versions have different limits, so we can't hardcode a\nsingle one. We could hardcode different limits for different versions\nin libcamera, but querying the limit dynamically is probably better.\n\n> Seems like this should be a kernel side fix too ( but the fact that\n> there are kernels that do not report the right size, means at least we\n> need a libcamera side fix here now anyway).\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> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++--\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +-\n> > 3 files changed, 17 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 0a794d63..97ce8457 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> > if (isp_->open() < 0)\n> > return false;\n> > \n> > + /*\n> > + * Retrieve the ISP maximum input size for config validation in the\n> > + * path classes.\n> > + *\n> > + * The ISP maximum input size is independent of the media bus formats\n> > + * hence, pick the one first entry of ispFormats and its size range.\n> > + */\n> > + V4L2Subdevice::Formats ispFormats = isp_->formats(0);\n\nconst ?\n\n> > + const SizeRange range = ispFormats.cbegin()->second[0];\n> > + Size ispMaxInputSize = range.max;\n\nconst ?\n\n> > +\n> > /* Locate and open the optional CSI-2 receiver. */\n> > ispSink_ = isp_->entity()->getPadByIndex(0);\n> > if (!ispSink_ || ispSink_->links().empty())\n> > @@ -1300,10 +1311,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > return false;\n> > \n> > /* Locate and open the ISP main and self paths. */\n> > - if (!mainPath_.init(media_))\n> > + if (!mainPath_.init(media_, ispMaxInputSize))\n> > return false;\n> > \n> > - if (hasSelfPath_ && !selfPath_.init(media_))\n> > + if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInputSize))\n> > return false;\n> > \n> > mainPath_.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..9053af18 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> > std::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> > std::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> > @@ -77,6 +77,8 @@ bool RkISP1Path::init(MediaDevice *media)\n> > \n> > populateFormats();\n> > \n> > + maxResolution_.boundTo(ispMaxInputSize);\n> > +\n> \n> I think this really needs a comment to explain...\n> \n> \t/*\n> \t * The maximum size reported by the video node during populate\n> \t * formats is hard coded on some kernels to a fixed size which\n> \t * can exceed the platform specific ISP limitations.\n> \t *\n> \t * While this should be fixed in the kernel, restrict to the ISP\n> \t * limitations correctly.\n\nI don't think that's right, see above. Did I miss something ?\n\n> \t */\n> \n> or such ?\n> \n> > link_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n> > if (!link_)\n> > return 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> > RkISP1Path(const char *name, const Span &formats,\n> > const Size &minResolution, const Size &maxResolution);\n> > \n> > - bool init(MediaDevice *media);\n> > + bool init(MediaDevice *media, Size ispMaxInputSize);\n> \n> I hoped this would be something we could set during the constructor -\n> but it would require changing the allocations and constructs of those\n> and it's probably not worth it for now so I could live with this.\n> \n> With comments to report /why/ we are clamping the sizes:\n> \n> Reviewed-by: Kieran Bingham \n> \n> > int setEnabled(bool enable) { return link_->setEnabled(enable); }\n> > bool 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 C38A4BF415\n\tfor ;\n\tWed, 11 Sep 2024 20:26:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D716634F9;\n\tWed, 11 Sep 2024 22:26:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 03BB4618F2\n\tfor ;\n\tWed, 11 Sep 2024 22:26:48 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(213-229-8-243.static.upcbusiness.at [213.229.8.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C307266F;\n\tWed, 11 Sep 2024 22:25:30 +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=\"S46YdHS+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726086330;\n\tbh=FsO6ASE4DR6rOeGhLrci4MuBv2IQf/A2vlNmKx7RTOE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=S46YdHS+fEFqH/QGD0+9SooTtKjJXubGz6yds8fO7ayHmcWtXthpOac5DpM+NSbVL\n\tY3NZhjliifabVGCDQZstwqGaeoajtH1WSTH20spX318kw5Ti8nwzfyBFOS7oj/vR3G\n\tAJ/s29KaPa3HmjHNdnXJ/Z/A03HyxJE4W+lbh6qI=","Date":"Wed, 11 Sep 2024 23:26:14 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP maximum\n\tinput","Message-ID":"<20240911202614.GI4470@pendragon.ideasonboard.com>","References":"<20240909163719.267211-1-umang.jain@ideasonboard.com>\n\t<20240909163719.267211-2-umang.jain@ideasonboard.com>\n\t<172605425249.1037309.9008167754412389745@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172605425249.1037309.9008167754412389745@ping.linuxembedded.co.uk>","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":31191,"web_url":"https://patchwork.libcamera.org/comment/31191/","msgid":"<2626b1f4-032c-4569-b89f-71790b6fb33e@ideasonboard.com>","date":"2024-09-12T10:26:36","subject":"Re: [PATCH 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP maximum\n\tinput","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi\n\nOn 12/09/24 1:56 am, Laurent Pinchart wrote:\n> On Wed, Sep 11, 2024 at 12:30:52PM +0100, Kieran Bingham wrote:\n>> Quoting Umang Jain (2024-09-09 17:37:18)\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>> Does this mean that the kernel is reporting the wrong maximum sizes on\n>> the path video node?\n> Isn't it the other way around, with RKISP1_RSZ_MP_SRC_MAX being wrong,\n> and the kernel reporting the right value when querying the video node ?\n\nRkISP1Path::populateFormats() does dynamic query of the sizes for the \nvideo node\n\nIf the right values were reported from the video nodes, \npopulateFormats() would have\nupdated it, as it iterates over the sizes of the video node.\n\n> Different ISP versions have different limits, so we can't hardcode a\n> single one. We could hardcode different limits for different versions\n> in libcamera, but querying the limit dynamically is probably better.\n\nWhat I think is a missing link on the kernel side is the clamping of max \nwidth and height\nin rkisp1-capture.c with respect to ISP limits (specified in  struct \nrkisp1_info)\n\n>\n>> Seems like this should be a kernel side fix too ( but the fact that\n>> there are kernels that do not report the right size, means at least we\n>> need a libcamera side fix here now anyway).\n\nYes, indeed!\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>>> ---\n>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 15 +++++++++++++--\n>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-\n>>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +-\n>>> 3 files changed, 17 insertions(+), 4 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> index 0a794d63..97ce8457 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>>> if (isp_->open() < 0)\n>>> return false;\n>>> \n>>> + /*\n>>> + * Retrieve the ISP maximum input size for config validation in the\n>>> + * path classes.\n>>> + *\n>>> + * The ISP maximum input size is independent of the media bus formats\n>>> + * hence, pick the one first entry of ispFormats and its size range.\n>>> + */\n>>> + V4L2Subdevice::Formats ispFormats = isp_->formats(0);\n> const ?\n>\n>>> + const SizeRange range = ispFormats.cbegin()->second[0];\n>>> + Size ispMaxInputSize = range.max;\n> const ?\n>\n>>> +\n>>> /* Locate and open the optional CSI-2 receiver. */\n>>> ispSink_ = isp_->entity()->getPadByIndex(0);\n>>> if (!ispSink_ || ispSink_->links().empty())\n>>> @@ -1300,10 +1311,10 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>>> return false;\n>>> \n>>> /* Locate and open the ISP main and self paths. */\n>>> - if (!mainPath_.init(media_))\n>>> + if (!mainPath_.init(media_, ispMaxInputSize))\n>>> return false;\n>>> \n>>> - if (hasSelfPath_ && !selfPath_.init(media_))\n>>> + if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInputSize))\n>>> return false;\n>>> \n>>> mainPath_.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..9053af18 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>>> std::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n>>> std::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n>>> @@ -77,6 +77,8 @@ bool RkISP1Path::init(MediaDevice *media)\n>>> \n>>> populateFormats();\n>>> \n>>> + maxResolution_.boundTo(ispMaxInputSize);\n>>> +\n>> I think this really needs a comment to explain...\n>>\n>> \t/*\n>> \t * The maximum size reported by the video node during populate\n>> \t * formats is hard coded on some kernels to a fixed size which\n>> \t * can exceed the platform specific ISP limitations.\n>> \t *\n>> \t * While this should be fixed in the kernel, restrict to the ISP\n>> \t * limitations correctly.\n> I don't think that's right, see above. Did I miss something ?\n\nI think Kieran's comment is correct.\n\nThe ISP limits are model specific in the kernel as dictated by struct \nrkisp1_info.\n\nHowever, the resizer and path (rkisp1-capture.c) are using hard coded values\n\nRKISP1_RSZ_MP_SRC_MAX_WIDTH\nRKISP1_RSZ_MP_SRC_MAX_HEIGHT\n\nfrom drivers/media/platform/rockchip/rkisp1/rkisp1-common.h and that's \nwhat we in libcamera\nduring populateFormats() as maxResolution_.\n>\n>> \t */\n>>\n>> or such ?\n>>\n>>> link_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n>>> if (!link_)\n>>> return 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>>> RkISP1Path(const char *name, const Span &formats,\n>>> const Size &minResolution, const Size &maxResolution);\n>>> \n>>> - bool init(MediaDevice *media);\n>>> + bool init(MediaDevice *media, Size ispMaxInputSize);\n>> I hoped this would be something we could set during the constructor -\n>> but it would require changing the allocations and constructs of those\n>> and it's probably not worth it for now so I could live with this.\n>>\n>> With comments to report /why/ we are clamping the sizes:\n>>\n>> Reviewed-by: Kieran Bingham \n>>\n>>> int setEnabled(bool enable) { return link_->setEnabled(enable); }\n>>> bool 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 46611C324C\n\tfor ;\n\tThu, 12 Sep 2024 10:26:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E75EF634F5;\n\tThu, 12 Sep 2024 12:26:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B44FC634F2\n\tfor ;\n\tThu, 12 Sep 2024 12:26:42 +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 7B6B51054;\n\tThu, 12 Sep 2024 12:25:22 +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=\"Zr7a/+g1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726136723;\n\tbh=Xx5SDuxyX3mfFdb/tZILsFIu08b2KNezxMChlH0kqm8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Zr7a/+g1g2IxwDpUUgzakSE6rWe8YNKGpfpW0qH53SR37cRhI5zLBlZAzGa0QUx9R\n\tsD5Wxksf7wSOkqZ8mXHCcNcgRcqkXnyn5DQ8XkhSXofZho2ZSsPxLt6JTtlW9JnG4n\n\twUUzCNgYtPWyhV2YLnk1S8nurPO1MyB+5hG4SKvw=","Message-ID":"<2626b1f4-032c-4569-b89f-71790b6fb33e@ideasonboard.com>","Date":"Thu, 12 Sep 2024 15:56:36 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 1/2] pipeline: rkisp1: Bound RkISP1 path to ISP maximum\n\tinput","To":"Laurent Pinchart ,\n\tKieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240909163719.267211-1-umang.jain@ideasonboard.com>\n\t<20240909163719.267211-2-umang.jain@ideasonboard.com>\n\t<172605425249.1037309.9008167754412389745@ping.linuxembedded.co.uk>\n\t<20240911202614.GI4470@pendragon.ideasonboard.com>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<20240911202614.GI4470@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]