[{"id":28481,"web_url":"https://patchwork.libcamera.org/comment/28481/","msgid":"<170549182302.1011926.15304309484938386532@ping.linuxembedded.co.uk>","date":"2024-01-17T11:43:43","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation\n\twhen sensor max is larger than ISP input","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder via libcamera-devel (2024-01-16 09:17:53)\n> If the maximum sensor output size is larger than the maximum ISP input\n> size, the maximum sensor size could be selected and the pipeline would\n> fail with an EPIPE. Fix this by clamping the sensor size to the ISP\n> input size at validation time.\n\nI think we should do more here (in a separate patch) to filter out\nsensor modes that are larger than the ISP max as well.\n\nAt the moment it seems too easy for the rkisp1 pipeline handler to\nselect an input configuration that it can't actually configure.\n\nBut that's aside from this patch itself.\n\n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 ++++++++++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 ++-\n> 3 files changed, 30 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 586b46d64..fb67ba8f4 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1202,11 +1202,24 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> if (param_->open() < 0)\n> return false;\n> \n> + /*\n> + * Retrieve ISP maximum input size for config validation in the path\n> + * classes\n> + */\n> + Size ispMaxInSize = { 0, 0 };\n> + V4L2Subdevice::Formats ispFormats = isp_->formats(0);\n> + for (const auto &[mbus, sizes] : ispFormats) {\n> + for (const auto &size : sizes) {\n> + if (ispMaxInSize < size.max)\n> + ispMaxInSize = size.max;\n> + }\n> + }\n> +\n> /* Locate and open the ISP main and self paths. */\n> - if (!mainPath_.init(media_))\n> + if (!mainPath_.init(media_, ispMaxInSize))\n> return false;\n> \n> - if (hasSelfPath_ && !selfPath_.init(media_))\n> + if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInSize))\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 b62b645ca..eaab7e857 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, const Size &ispMaxInSize)\n\nIt feels a bit odd to me passing ispMaxInSize as the only parameter to\ninitialize. But I guess it save init from re-determining it for each\npath, and maybe it's the only variable for now - so I think we're ok\nhere.\n\nI wonder if we should iterate and 'try' configurations to only expose\nthose which can be supported. But we can tackle that on top I think.\n\nSo for now I am ok with this:\n\nReviewed-by: Kieran Bingham \n\n> {\n> std::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> std::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> @@ -76,6 +76,7 @@ bool RkISP1Path::init(MediaDevice *media)\n> return false;\n> \n> populateFormats();\n> + ispMaxInSize_ = ispMaxInSize;\n> \n> link_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n> if (!link_)\n> @@ -172,7 +173,9 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> /* Add all the RAW sizes the sensor can produce for this code. */\n> for (const auto &rawSize : sensor->sizes(mbusCode)) {\n> if (rawSize.width > maxResolution_.width ||\n> - rawSize.height > maxResolution_.height)\n> + rawSize.height > maxResolution_.height ||\n> + rawSize.width > ispMaxInSize_.width ||\n> + rawSize.height > ispMaxInSize_.height)\n> continue;\n> \n> streamFormats[format].push_back({ rawSize, rawSize });\n> @@ -275,8 +278,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> if (!found)\n> cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;\n> \n> - Size minResolution;\n> - Size maxResolution;\n> + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> + .boundedTo(resolution);\n> + Size minResolution = minResolution_.expandedToAspectRatio(resolution);\n> \n> if (isRaw) {\n> /*\n> @@ -287,16 +291,10 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> V4L2SubdeviceFormat sensorFormat =\n> sensor->getFormat({ mbusCode }, cfg->size);\n> \n> - minResolution = sensorFormat.size;\n> - maxResolution = sensorFormat.size;\n> - } else {\n> - /*\n> - * Adjust the size based on the sensor resolution and absolute\n> - * limits of the ISP.\n> - */\n> - minResolution = minResolution_.expandedToAspectRatio(resolution);\n> - maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> - .boundedTo(resolution);\n> + if (!sensorFormat.size.isNull()) {\n> + minResolution = sensorFormat.size.boundedTo(ispMaxInSize_);\n> + maxResolution = sensorFormat.size.boundedTo(ispMaxInSize_);\n> + }\n> }\n> \n> cfg->size.boundTo(maxResolution);\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index cd77957ee..c96bd5557 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, const Size &ispMaxInSize);\n> \n> int setEnabled(bool enable) { return link_->setEnabled(enable); }\n> bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n> @@ -77,6 +77,8 @@ private:\n> std::unique_ptr resizer_;\n> std::unique_ptr video_;\n> MediaLink *link_;\n> +\n> + Size ispMaxInSize_;\n> };\n> \n> class RkISP1MainPath : public RkISP1Path\n> -- \n> 2.39.2\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 96503BDB1C\n\tfor ;\n\tWed, 17 Jan 2024 11:43:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E3A64628B7;\n\tWed, 17 Jan 2024 12:43:47 +0100 (CET)","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 6896C628AD\n\tfor ;\n\tWed, 17 Jan 2024 12:43:46 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CE9437E2;\n\tWed, 17 Jan 2024 12:42:36 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705491827;\n\tbh=cyLyVw1XlWkFjUmtO7NuOppY55Ey8oj0XKErPmsptCI=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=Zz/O/SXXnak7fbaL5lsJRSgjZfrh/DRvCl7nRonDCQH13/Bgcw1BdoSWjWbkAgqb4\n\tDpL1nQRWP4kJJoGpIH76dz2XcccRF7lpEcWMLWtSMCFV3KRXUZVHnUSQ+cdGrZxHra\n\tF4yznO8nGYvKaE5n8v9ASfvLNtU3UzTSKJosczVZo4TFHgRqJfG03MkZmxcarKHyNF\n\tgdPjb2oMJZLyvBw0uo+55nYzFJx93uRTPof5O+QRf9z6BG4cHCpUgbOY5b+VsB6xzX\n\tirQGGFVI8MWityeEZ0RN7rccHEq9sImzeEGivDx4cjaK5zS4zAKZggrE1ReQLaxkkv\n\tv3WG13gb9PN9Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1705491756;\n\tbh=cyLyVw1XlWkFjUmtO7NuOppY55Ey8oj0XKErPmsptCI=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=nMADfnAO4qYZ3le/3v/MvKlR8ZUEgmx6mjrNpjTAOnGGt0zblc77Z0l28/cSggwKL\n\tb2fFpNVFNl8MiJfcM7Q+PP1kiZxjyPCPxvkxATdsVt01x4bw3caxwe6W1GLgpxK0az\n\t9FsqLh+8qLlEpnbRj9FEs69iutgLOyOvQixMY0uo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nMADfnAO\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240116091754.100654-3-paul.elder@ideasonboard.com>","References":"<20240116091754.100654-1-paul.elder@ideasonboard.com>\n\t<20240116091754.100654-3-paul.elder@ideasonboard.com>","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 17 Jan 2024 11:43:43 +0000","Message-ID":"<170549182302.1011926.15304309484938386532@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation\n\twhen sensor max is larger than ISP input","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28484,"web_url":"https://patchwork.libcamera.org/comment/28484/","msgid":"<20240117155715.GI4860@pendragon.ideasonboard.com>","date":"2024-01-17T15:57:15","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation\n\twhen sensor max is larger than ISP input","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jan 17, 2024 at 11:43:43AM +0000, Kieran Bingham via libcamera-devel wrote:\n> Quoting Paul Elder via libcamera-devel (2024-01-16 09:17:53)\n> > If the maximum sensor output size is larger than the maximum ISP input\n> > size, the maximum sensor size could be selected and the pipeline would\n> > fail with an EPIPE. Fix this by clamping the sensor size to the ISP\n> > input size at validation time.\n> \n> I think we should do more here (in a separate patch) to filter out\n> sensor modes that are larger than the ISP max as well.\n> \n> At the moment it seems too easy for the rkisp1 pipeline handler to\n> select an input configuration that it can't actually configure.\n> \n> But that's aside from this patch itself.\n> \n> > Signed-off-by: Paul Elder \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 ++++++++++--\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 ++-\n> > 3 files changed, 30 insertions(+), 17 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 586b46d64..fb67ba8f4 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -1202,11 +1202,24 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > if (param_->open() < 0)\n> > return false;\n> > \n> > + /*\n> > + * Retrieve ISP maximum input size for config validation in the path\n\ns/Retrieve/Retrieve the/\n\n> > + * classes\n\ns/$/./\n\n> > + */\n> > + Size ispMaxInSize = { 0, 0 };\n\nThat's the default, so you can just write\n\n\tSize ispMaxInSize;\n\nI also find ispMaxInputSize more readable.\n\n> > + V4L2Subdevice::Formats ispFormats = isp_->formats(0);\n\nconst if you don't need to modify it.\n\n> > + for (const auto &[mbus, sizes] : ispFormats) {\n> > + for (const auto &size : sizes) {\n> > + if (ispMaxInSize < size.max)\n> > + ispMaxInSize = size.max;\n> > + }\n> > + }\n\nDo the maximum input size depend on the media bus format ? And to we\nhave multiple sizes in the sizes vector ? We could take some shortcuts,\nknowing how the ISP driver is implemented, taking the first entry in\nboth ispFormats and sizes.\n\n> > +\n> > /* Locate and open the ISP main and self paths. */\n> > - if (!mainPath_.init(media_))\n> > + if (!mainPath_.init(media_, ispMaxInSize))\n> > return false;\n> > \n> > - if (hasSelfPath_ && !selfPath_.init(media_))\n> > + if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInSize))\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 b62b645ca..eaab7e857 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, const Size &ispMaxInSize)\n\nispMaxInputSize here too if you rename it above. Same below.\n\n> It feels a bit odd to me passing ispMaxInSize as the only parameter to\n> initialize. But I guess it save init from re-determining it for each\n> path, and maybe it's the only variable for now - so I think we're ok\n> here.\n> \n> I wonder if we should iterate and 'try' configurations to only expose\n> those which can be supported. But we can tackle that on top I think.\n\nAgreed.\n\n> So for now I am ok with this:\n> \n> Reviewed-by: Kieran Bingham \n\nReviewed-by: Laurent Pinchart \n\nwith the above comments taken into account.\n\n> \n> > {\n> > std::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> > std::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> > @@ -76,6 +76,7 @@ bool RkISP1Path::init(MediaDevice *media)\n> > return false;\n> > \n> > populateFormats();\n> > + ispMaxInSize_ = ispMaxInSize;\n> > \n> > link_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n> > if (!link_)\n> > @@ -172,7 +173,9 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> > /* Add all the RAW sizes the sensor can produce for this code. */\n> > for (const auto &rawSize : sensor->sizes(mbusCode)) {\n> > if (rawSize.width > maxResolution_.width ||\n> > - rawSize.height > maxResolution_.height)\n> > + rawSize.height > maxResolution_.height ||\n> > + rawSize.width > ispMaxInSize_.width ||\n> > + rawSize.height > ispMaxInSize_.height)\n> > continue;\n> > \n> > streamFormats[format].push_back({ rawSize, rawSize });\n> > @@ -275,8 +278,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> > if (!found)\n> > cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;\n> > \n> > - Size minResolution;\n> > - Size maxResolution;\n> > + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > + .boundedTo(resolution);\n> > + Size minResolution = minResolution_.expandedToAspectRatio(resolution);\n> > \n> > if (isRaw) {\n> > /*\n> > @@ -287,16 +291,10 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> > V4L2SubdeviceFormat sensorFormat =\n> > sensor->getFormat({ mbusCode }, cfg->size);\n> > \n> > - minResolution = sensorFormat.size;\n> > - maxResolution = sensorFormat.size;\n> > - } else {\n> > - /*\n> > - * Adjust the size based on the sensor resolution and absolute\n> > - * limits of the ISP.\n> > - */\n> > - minResolution = minResolution_.expandedToAspectRatio(resolution);\n> > - maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > - .boundedTo(resolution);\n> > + if (!sensorFormat.size.isNull()) {\n> > + minResolution = sensorFormat.size.boundedTo(ispMaxInSize_);\n> > + maxResolution = sensorFormat.size.boundedTo(ispMaxInSize_);\n> > + }\n> > }\n> > \n> > cfg->size.boundTo(maxResolution);\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > index cd77957ee..c96bd5557 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, const Size &ispMaxInSize);\n> > \n> > int setEnabled(bool enable) { return link_->setEnabled(enable); }\n> > bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n> > @@ -77,6 +77,8 @@ private:\n> > std::unique_ptr resizer_;\n> > std::unique_ptr video_;\n> > MediaLink *link_;\n> > +\n> > + Size ispMaxInSize_;\n> > };\n> > \n> > class RkISP1MainPath : public RkISP1Path","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 86938BDB1C\n\tfor ;\n\tWed, 17 Jan 2024 15:57:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC686628AD;\n\tWed, 17 Jan 2024 16:57:12 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7ADCF628AD\n\tfor ;\n\tWed, 17 Jan 2024 16:57:11 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 80E7C14F6;\n\tWed, 17 Jan 2024 16:56:01 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705507032;\n\tbh=0OFNzGZXqqwpzaar698Z9JeK/iqO0czgkDUIZ/0Jyb0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=nhHLd2ZrYaxZoccCS0ZqBkD9SBVlDmOadGekQHsQG8LhvSalaQu+E4O7gOABeQsJ7\n\tO/JRrWvyMDsQq3Wc11MS0GQtvQSc3Fpg+9Rp2RmyvHh4Nip6Lq+NAWYi0fOopJMtwv\n\t3DbaXLpsnQbCeqLyJwaX0v9JhYVZx3XMCFDGQeSDW1l74rkqaL3ICWXvrGuO1XiieK\n\tQIoG1lLScUzDKc+kO/KmjYSyVI4H+nn5QB3Oj2GPLGGlcC156Wp5y7bGVIWrWQG1Qr\n\tqd0jjC8sq0alzDNnuk6bXSdnj1EB6mV91ung0PctZEe9q4GmqKJ8YFY3R88mnPfzC9\n\tDVNUpFF07CipQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1705506961;\n\tbh=0OFNzGZXqqwpzaar698Z9JeK/iqO0czgkDUIZ/0Jyb0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SF9r8tpI3g6OZGnzcCTXl8xns57Tv6t7tMezlEiMH4m/iLnAMOp2qux5ELPif89nd\n\t8z45rQ+VcZmH+GhqlLuC/QRuy+mj3Si1aPAE2sPkNTH3auV1HsG9/gwQcyD8PaRvuV\n\txf3wVeYr1pkX+IpQqAzDtOOYF6cMOuQVTm/kY070="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"SF9r8tpI\"; dkim-atps=neutral","Date":"Wed, 17 Jan 2024 17:57:15 +0200","To":"Kieran Bingham ","Message-ID":"<20240117155715.GI4860@pendragon.ideasonboard.com>","References":"<20240116091754.100654-1-paul.elder@ideasonboard.com>\n\t<20240116091754.100654-3-paul.elder@ideasonboard.com>\n\t<170549182302.1011926.15304309484938386532@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<170549182302.1011926.15304309484938386532@ping.linuxembedded.co.uk>","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation\n\twhen sensor max is larger than ISP input","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28485,"web_url":"https://patchwork.libcamera.org/comment/28485/","msgid":"","date":"2024-01-17T16:21:00","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation\n\twhen sensor max is larger than ISP input","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Paul\n\nOn Tue, Jan 16, 2024 at 06:17:53PM +0900, Paul Elder via libcamera-devel wrote:\n> If the maximum sensor output size is larger than the maximum ISP input\n> size, the maximum sensor size could be selected and the pipeline would\n> fail with an EPIPE. Fix this by clamping the sensor size to the ISP\n> input size at validation time.\n>\n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 ++++++++++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 ++-\n> 3 files changed, 30 insertions(+), 17 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 586b46d64..fb67ba8f4 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1202,11 +1202,24 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> \tif (param_->open() < 0)\n> \t\treturn false;\n>\n> +\t/*\n> +\t * Retrieve ISP maximum input size for config validation in the path\n> +\t * classes\n> +\t */\n> +\tSize ispMaxInSize = { 0, 0 };\n> +\tV4L2Subdevice::Formats ispFormats = isp_->formats(0);\n> +\tfor (const auto &[mbus, sizes] : ispFormats) {\n> +\t\tfor (const auto &size : sizes) {\n> +\t\t\tif (ispMaxInSize < size.max)\n> +\t\t\t\tispMaxInSize = size.max;\n> +\t\t}\n> +\t}\n> +\n\nI presume this could even be kept as a constant in the pipeline\nhandler ? Sure, retrieving it a run time is probably better\n\n> \t/* Locate and open the ISP main and self paths. */\n> -\tif (!mainPath_.init(media_))\n> +\tif (!mainPath_.init(media_, ispMaxInSize))\n> \t\treturn false;\n>\n> -\tif (hasSelfPath_ && !selfPath_.init(media_))\n> +\tif (hasSelfPath_ && !selfPath_.init(media_, ispMaxInSize))\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 b62b645ca..eaab7e857 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, const Size &ispMaxInSize)\n> {\n> \tstd::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> \tstd::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> @@ -76,6 +76,7 @@ bool RkISP1Path::init(MediaDevice *media)\n> \t\treturn false;\n>\n> \tpopulateFormats();\n> +\tispMaxInSize_ = ispMaxInSize;\n>\n> \tlink_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n> \tif (!link_)\n> @@ -172,7 +173,9 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> \t\t/* Add all the RAW sizes the sensor can produce for this code. */\n> \t\tfor (const auto &rawSize : sensor->sizes(mbusCode)) {\n> \t\t\tif (rawSize.width > maxResolution_.width ||\n> -\t\t\t rawSize.height > maxResolution_.height)\n> +\t\t\t rawSize.height > maxResolution_.height ||\n> +\t\t\t rawSize.width > ispMaxInSize_.width ||\n> +\t\t\t rawSize.height > ispMaxInSize_.height)\n\nThis filters out the streamFormats that are reported to the\napplication, and I think this should be maybe only limited by the\nvideo capture device output sizes, not the isp input sizes ?\n\nI do however see a few lines above in generateConfiguration()\n\n\tconst Size &resolution = sensor->resolution();\n\n\t/* Min and max resolutions to populate the available stream formats. */\n\tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n\t\t\t\t\t .boundedTo(resolution);\n\tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n\nShould `resolution` be replaced by the maximum sensor resolution\nsmaller than the isp max input size (maybe computed in\npopulateFormats() ? )\n\n\n> \t\t\t\tcontinue;\n>\n> \t\t\tstreamFormats[format].push_back({ rawSize, rawSize });\n> @@ -275,8 +278,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \tif (!found)\n> \t\tcfg->pixelFormat = isRaw ? rawFormat : formats::NV12;\n>\n> -\tSize minResolution;\n> -\tSize maxResolution;\n> +\tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> +\t\t\t\t\t .boundedTo(resolution);\n> +\tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n\nShould you replace 'resolution' with the above mentioned 'larger\nsensor's resolution which is smaller than the ISP input size' ?\n\n>\n> \tif (isRaw) {\n> \t\t/*\n> @@ -287,16 +291,10 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\tV4L2SubdeviceFormat sensorFormat =\n> \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n>\n> -\t\tminResolution = sensorFormat.size;\n> -\t\tmaxResolution = sensorFormat.size;\n> -\t} else {\n> -\t\t/*\n> -\t\t * Adjust the size based on the sensor resolution and absolute\n> -\t\t * limits of the ISP.\n> -\t\t */\n> -\t\tminResolution = minResolution_.expandedToAspectRatio(resolution);\n> -\t\tmaxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> -\t\t\t\t\t .boundedTo(resolution);\n> +\t\tif (!sensorFormat.size.isNull()) {\n> +\t\t\tminResolution = sensorFormat.size.boundedTo(ispMaxInSize_);\n> +\t\t\tmaxResolution = sensorFormat.size.boundedTo(ispMaxInSize_);\n> +\t\t}\n> \t}\n>\n> \tcfg->size.boundTo(maxResolution);\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index cd77957ee..c96bd5557 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, const Size &ispMaxInSize);\n\nI would creat the path with a pointer to the isp subdev instead of\npassing the value here as this doesn't smell like a function parameter\nbut a system's characterstics . Not a big deal though.\n\n>\n> \tint setEnabled(bool enable) { return link_->setEnabled(enable); }\n> \tbool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n> @@ -77,6 +77,8 @@ private:\n> \tstd::unique_ptr resizer_;\n> \tstd::unique_ptr video_;\n> \tMediaLink *link_;\n> +\n> +\tSize ispMaxInSize_;\n> };\n>\n> class RkISP1MainPath : public RkISP1Path\n> --\n> 2.39.2\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 912A5C323E\n\tfor ;\n\tWed, 17 Jan 2024 16:21:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 02B50628B7;\n\tWed, 17 Jan 2024 17:21:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DAD1C628AD\n\tfor ;\n\tWed, 17 Jan 2024 17:21:03 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2A1347EC;\n\tWed, 17 Jan 2024 17:19:54 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705508465;\n\tbh=wgD+NYcjs5rWEhd9CXVmJuhPPZntWYmxhHFcBG2nC+k=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=tg3JmA0/cMX65FGwHrK1oNijeYxSedWNlS2JnxmXSzS/JwqHAIyTSQWGop6hRnS2l\n\tNbgBAIC/inJ5D3kDEDwyveS5MHkh9nrn6RWq2z4Wnn4XoZSGe/FkI/vuaTCJuK6z6r\n\t9d7a6lCwvBQhrmP0RlC7kE7sBfTiA5RaQSwgdhSxuUYIy+N+7ojtIwWZSse8o7mj7G\n\tx/mP8kb1nIaVrotzxhqvrBzheiRtpbKOxU0IA7RpXzACmepnCq8zeWmBd4h2O5qb0A\n\tMWjM8smHoDdY7pTlQWmfoNUFHvgxZzMoJKbqx00ZzaVM2o9P9VHGFFrUU4OIkpEDp1\n\t2FuYGn3SknZ6Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1705508394;\n\tbh=wgD+NYcjs5rWEhd9CXVmJuhPPZntWYmxhHFcBG2nC+k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ovaXKd6at7iihaYwZ9pGkRuIYlG5X+v/YACnEhnJcjGmp8Xo2qnYJ8WetjiQd89nB\n\tYZw9Tbwq6PYUA8QtPbc6VDWzDAw+Co2plMBNblnv+bnE4RsPbp1JRpANdygm0qjsOb\n\tfA7qAk/ToyDKs6WR8tdS6QFd2xK49mxVWRdFDxg0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ovaXKd6a\"; dkim-atps=neutral","Date":"Wed, 17 Jan 2024 17:21:00 +0100","To":"Paul Elder ","Message-ID":"","References":"<20240116091754.100654-1-paul.elder@ideasonboard.com>\n\t<20240116091754.100654-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240116091754.100654-3-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation\n\twhen sensor max is larger than ISP input","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30451,"web_url":"https://patchwork.libcamera.org/comment/30451/","msgid":"<0052b2f5-1382-4238-a853-1ee152a3b4de@ideasonboard.com>","date":"2024-07-23T07:30:56","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation\n\twhen sensor max is larger than ISP input","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hello all,\n\nressurcting this old patch as it has went abandoned...\n\nOn 17/01/24 9:27 pm, Laurent Pinchart via libcamera-devel wrote:\n> On Wed, Jan 17, 2024 at 11:43:43AM +0000, Kieran Bingham via libcamera-devel wrote:\n>> Quoting Paul Elder via libcamera-devel (2024-01-16 09:17:53)\n>>> If the maximum sensor output size is larger than the maximum ISP input\n>>> size, the maximum sensor size could be selected and the pipeline would\n>>> fail with an EPIPE. Fix this by clamping the sensor size to the ISP\n>>> input size at validation time.\n>> I think we should do more here (in a separate patch) to filter out\n>> sensor modes that are larger than the ISP max as well.\n>>\n>> At the moment it seems too easy for the rkisp1 pipeline handler to\n>> select an input configuration that it can't actually configure.\n>>\n>> But that's aside from this patch itself.\n>>\n>>> Signed-off-by: Paul Elder \n>>> ---\n>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 ++++++++++--\n>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------\n>>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 ++-\n>>> 3 files changed, 30 insertions(+), 17 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> index 586b46d64..fb67ba8f4 100644\n>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> @@ -1202,11 +1202,24 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>>> if (param_->open() < 0)\n>>> return false;\n>>> \n>>> + /*\n>>> + * Retrieve ISP maximum input size for config validation in the path\n> s/Retrieve/Retrieve the/\n>\n>>> + * classes\n> s/$/./\n>\n>>> + */\n>>> + Size ispMaxInSize = { 0, 0 };\n> That's the default, so you can just write\n>\n> \tSize ispMaxInSize;\n>\n> I also find ispMaxInputSize more readable.\n>\n>>> + V4L2Subdevice::Formats ispFormats = isp_->formats(0);\n> const if you don't need to modify it.\n>\n>>> + for (const auto &[mbus, sizes] : ispFormats) {\n>>> + for (const auto &size : sizes) {\n>>> + if (ispMaxInSize < size.max)\n>>> + ispMaxInSize = size.max;\n>>> + }\n>>> + }\n> Do the maximum input size depend on the media bus format ? And to we\n> have multiple sizes in the sizes vector ? We could take some shortcuts,\n> knowing how the ISP driver is implemented, taking the first entry in\n> both ispFormats and sizes.\n>\n>>> +\n>>> /* Locate and open the ISP main and self paths. */\n>>> - if (!mainPath_.init(media_))\n>>> + if (!mainPath_.init(media_, ispMaxInSize))\n>>> return false;\n>>> \n>>> - if (hasSelfPath_ && !selfPath_.init(media_))\n>>> + if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInSize))\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 b62b645ca..eaab7e857 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, const Size &ispMaxInSize)\n> ispMaxInputSize here too if you rename it above. Same below.\n>\n>> It feels a bit odd to me passing ispMaxInSize as the only parameter to\n>> initialize. But I guess it save init from re-determining it for each\n>> path, and maybe it's the only variable for now - so I think we're ok\n>> here.\n>>\n>> I wonder if we should iterate and 'try' configurations to only expose\n>> those which can be supported. But we can tackle that on top I think.\n> Agreed.\n>\n>> So for now I am ok with this:\n>>\n>> Reviewed-by: Kieran Bingham \n> Reviewed-by: Laurent Pinchart \n>\n> with the above comments taken into account.\n>\n>>> {\n>>> std::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n>>> std::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n>>> @@ -76,6 +76,7 @@ bool RkISP1Path::init(MediaDevice *media)\n>>> return false;\n>>> \n>>> populateFormats();\n>>> + ispMaxInSize_ = ispMaxInSize;\n>>> \n>>> link_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n>>> if (!link_)\n>>> @@ -172,7 +173,9 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n>>> /* Add all the RAW sizes the sensor can produce for this code. */\n>>> for (const auto &rawSize : sensor->sizes(mbusCode)) {\n>>> if (rawSize.width > maxResolution_.width ||\n>>> - rawSize.height > maxResolution_.height)\n>>> + rawSize.height > maxResolution_.height ||\n>>> + rawSize.width > ispMaxInSize_.width ||\n>>> + rawSize.height > ispMaxInSize_.height)\n>>> continue;\n>>> \n>>> streamFormats[format].push_back({ rawSize, rawSize });\n>>> @@ -275,8 +278,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>>> if (!found)\n>>> cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;\n>>> \n>>> - Size minResolution;\n>>> - Size maxResolution;\n>>> + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n>>> + .boundedTo(resolution);\n>>> + Size minResolution = minResolution_.expandedToAspectRatio(resolution);\n\nI think one point of discussion went un-noticed here, is whether this is \nthe correct behaviour. If the sensor input size is larger than that of \nISP, the maxResolution_ of the ISP is bounded to aspect ratio of the \nsensor highest resolution.\n\nI hit his recently - running IMX283 on i.MX8MP. I noticed the pipeline \nwas getting a maximum supported resolution of 4096x2944 (ISP input max \nresolution is 4096x3072). And the resolution of IMX283 was 5472x3648\n\nSo aspect ratio of IMX283 5472x3648 (1.5) brings the aspect ratio of ISP \nmax input 4096x3072(1.33)  to 4096x2944 (1.55)\n\nSo the question here really is whether to preserve the original aspect \nratio of the sensor's maximum output (4096x2944) and treat as that \nmaxResolution - or the maxResolution should remain the same as supported \nby the ISP (4096x3072) in all cases.\n\n>>> \n>>> if (isRaw) {\n>>> /*\n>>> @@ -287,16 +291,10 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>>> V4L2SubdeviceFormat sensorFormat =\n>>> sensor->getFormat({ mbusCode }, cfg->size);\n>>> \n>>> - minResolution = sensorFormat.size;\n>>> - maxResolution = sensorFormat.size;\n>>> - } else {\n>>> - /*\n>>> - * Adjust the size based on the sensor resolution and absolute\n>>> - * limits of the ISP.\n>>> - */\n>>> - minResolution = minResolution_.expandedToAspectRatio(resolution);\n>>> - maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n>>> - .boundedTo(resolution);\n>>> + if (!sensorFormat.size.isNull()) {\n>>> + minResolution = sensorFormat.size.boundedTo(ispMaxInSize_);\n>>> + maxResolution = sensorFormat.size.boundedTo(ispMaxInSize_);\n>>> + }\n>>> }\n>>> \n>>> cfg->size.boundTo(maxResolution);\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>>> index cd77957ee..c96bd5557 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, const Size &ispMaxInSize);\n>>> \n>>> int setEnabled(bool enable) { return link_->setEnabled(enable); }\n>>> bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n>>> @@ -77,6 +77,8 @@ private:\n>>> std::unique_ptr resizer_;\n>>> std::unique_ptr video_;\n>>> MediaLink *link_;\n>>> +\n>>> + Size ispMaxInSize_;\n>>> };\n>>> \n>>> class RkISP1MainPath : public RkISP1Path","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 BF5D7C323E\n\tfor ;\n\tTue, 23 Jul 2024 07:31:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D3BBD6336B;\n\tTue, 23 Jul 2024 09:31:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65BF16199E\n\tfor ;\n\tTue, 23 Jul 2024 09:31:00 +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 D232E63C;\n\tTue, 23 Jul 2024 09:30: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=\"QoEXVpB0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721719818;\n\tbh=GiPGpef20y1wt0/DzsMULsLxhrPJ+MsVHpBsXlR3mhs=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=QoEXVpB0USkANDjrLVcRPzk9+85kO07r5/yf+cIlHBvC/EfUnB/672CzJLqUKz8FY\n\t/+HeUV5bvTSk57TFF7AY8NQs7y0hA4sNhzRzc7aBw8EApyNdf6k9ZRHar73truGx0Z\n\tHfGRrR5eGtMe4z/Y+CohTOIg11xLDzBNeC8dXvEs=","Message-ID":"<0052b2f5-1382-4238-a853-1ee152a3b4de@ideasonboard.com>","Date":"Tue, 23 Jul 2024 13:00:56 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation\n\twhen sensor max is larger than ISP input","To":"Laurent Pinchart ,\n\tKieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240116091754.100654-1-paul.elder@ideasonboard.com>\n\t<20240116091754.100654-3-paul.elder@ideasonboard.com>\n\t<170549182302.1011926.15304309484938386532@ping.linuxembedded.co.uk>\n\t<20240117155715.GI4860@pendragon.ideasonboard.com>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<20240117155715.GI4860@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\" "}},{"id":30452,"web_url":"https://patchwork.libcamera.org/comment/30452/","msgid":"<172172470445.392292.10868066912190375815@ping.linuxembedded.co.uk>","date":"2024-07-23T08:51:44","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation\n\twhen sensor max is larger than ISP input","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2024-07-23 08:30:56)\n> Hello all,\n> \n> ressurcting this old patch as it has went abandoned...\n> \n> On 17/01/24 9:27 pm, Laurent Pinchart via libcamera-devel wrote:\n> > On Wed, Jan 17, 2024 at 11:43:43AM +0000, Kieran Bingham via libcamera-devel wrote:\n> >> Quoting Paul Elder via libcamera-devel (2024-01-16 09:17:53)\n> >>> If the maximum sensor output size is larger than the maximum ISP input\n> >>> size, the maximum sensor size could be selected and the pipeline would\n> >>> fail with an EPIPE. Fix this by clamping the sensor size to the ISP\n> >>> input size at validation time.\n> >> I think we should do more here (in a separate patch) to filter out\n> >> sensor modes that are larger than the ISP max as well.\n> >>\n> >> At the moment it seems too easy for the rkisp1 pipeline handler to\n> >> select an input configuration that it can't actually configure.\n> >>\n> >> But that's aside from this patch itself.\n> >>\n> >>> Signed-off-by: Paul Elder \n> >>> ---\n> >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 ++++++++++--\n> >>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------\n> >>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 ++-\n> >>> 3 files changed, 30 insertions(+), 17 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> index 586b46d64..fb67ba8f4 100644\n> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> @@ -1202,11 +1202,24 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> >>> if (param_->open() < 0)\n> >>> return false;\n> >>> \n> >>> + /*\n> >>> + * Retrieve ISP maximum input size for config validation in the path\n> > s/Retrieve/Retrieve the/\n> >\n> >>> + * classes\n> > s/$/./\n> >\n> >>> + */\n> >>> + Size ispMaxInSize = { 0, 0 };\n> > That's the default, so you can just write\n> >\n> > Size ispMaxInSize;\n> >\n> > I also find ispMaxInputSize more readable.\n> >\n> >>> + V4L2Subdevice::Formats ispFormats = isp_->formats(0);\n> > const if you don't need to modify it.\n> >\n> >>> + for (const auto &[mbus, sizes] : ispFormats) {\n> >>> + for (const auto &size : sizes) {\n> >>> + if (ispMaxInSize < size.max)\n> >>> + ispMaxInSize = size.max;\n> >>> + }\n> >>> + }\n> > Do the maximum input size depend on the media bus format ? And to we\n> > have multiple sizes in the sizes vector ? We could take some shortcuts,\n> > knowing how the ISP driver is implemented, taking the first entry in\n> > both ispFormats and sizes.\n> >\n> >>> +\n> >>> /* Locate and open the ISP main and self paths. */\n> >>> - if (!mainPath_.init(media_))\n> >>> + if (!mainPath_.init(media_, ispMaxInSize))\n> >>> return false;\n> >>> \n> >>> - if (hasSelfPath_ && !selfPath_.init(media_))\n> >>> + if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInSize))\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 b62b645ca..eaab7e857 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, const Size &ispMaxInSize)\n> > ispMaxInputSize here too if you rename it above. Same below.\n> >\n> >> It feels a bit odd to me passing ispMaxInSize as the only parameter to\n> >> initialize. But I guess it save init from re-determining it for each\n> >> path, and maybe it's the only variable for now - so I think we're ok\n> >> here.\n> >>\n> >> I wonder if we should iterate and 'try' configurations to only expose\n> >> those which can be supported. But we can tackle that on top I think.\n> > Agreed.\n> >\n> >> So for now I am ok with this:\n> >>\n> >> Reviewed-by: Kieran Bingham \n> > Reviewed-by: Laurent Pinchart \n> >\n> > with the above comments taken into account.\n> >\n> >>> {\n> >>> std::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> >>> std::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> >>> @@ -76,6 +76,7 @@ bool RkISP1Path::init(MediaDevice *media)\n> >>> return false;\n> >>> \n> >>> populateFormats();\n> >>> + ispMaxInSize_ = ispMaxInSize;\n> >>> \n> >>> link_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n> >>> if (!link_)\n> >>> @@ -172,7 +173,9 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> >>> /* Add all the RAW sizes the sensor can produce for this code. */\n> >>> for (const auto &rawSize : sensor->sizes(mbusCode)) {\n> >>> if (rawSize.width > maxResolution_.width ||\n> >>> - rawSize.height > maxResolution_.height)\n> >>> + rawSize.height > maxResolution_.height ||\n> >>> + rawSize.width > ispMaxInSize_.width ||\n> >>> + rawSize.height > ispMaxInSize_.height)\n> >>> continue;\n> >>> \n> >>> streamFormats[format].push_back({ rawSize, rawSize });\n> >>> @@ -275,8 +278,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> >>> if (!found)\n> >>> cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;\n> >>> \n> >>> - Size minResolution;\n> >>> - Size maxResolution;\n> >>> + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> >>> + .boundedTo(resolution);\n> >>> + Size minResolution = minResolution_.expandedToAspectRatio(resolution);\n> \n> I think one point of discussion went un-noticed here, is whether this is \n> the correct behaviour. If the sensor input size is larger than that of \n> ISP, the maxResolution_ of the ISP is bounded to aspect ratio of the \n> sensor highest resolution.\n> \n> I hit his recently - running IMX283 on i.MX8MP. I noticed the pipeline \n> was getting a maximum supported resolution of 4096x2944 (ISP input max \n> resolution is 4096x3072). And the resolution of IMX283 was 5472x3648\n> \n> So aspect ratio of IMX283 5472x3648 (1.5) brings the aspect ratio of ISP \n> max input 4096x3072(1.33)  to 4096x2944 (1.55)\n> \n> So the question here really is whether to preserve the original aspect \n> ratio of the sensor's maximum output (4096x2944) and treat as that \n> maxResolution - or the maxResolution should remain the same as supported \n> by the ISP (4096x3072) in all cases.\n\nIndeed, we now know that a sensor might expose cropped outputs that are\nnot the same aspect ratio as full sensor dimensions. We of course would\nwant to preserve /that/ aspect ratio too though - so I think it means we\nreally need to do better at how we expose what stream configurations are\npossible entirely.\n\nThere could be 16:9, or 4:3 - or any modes with any other aspect ratio\nprovided by the sensor. Preserving only the full sensor aspect ratio\ndoes not make sense.\n\n\n--\nKieran\n\n\n> \n> >>> \n> >>> if (isRaw) {\n> >>> /*\n> >>> @@ -287,16 +291,10 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> >>> V4L2SubdeviceFormat sensorFormat =\n> >>> sensor->getFormat({ mbusCode }, cfg->size);\n> >>> \n> >>> - minResolution = sensorFormat.size;\n> >>> - maxResolution = sensorFormat.size;\n> >>> - } else {\n> >>> - /*\n> >>> - * Adjust the size based on the sensor resolution and absolute\n> >>> - * limits of the ISP.\n> >>> - */\n> >>> - minResolution = minResolution_.expandedToAspectRatio(resolution);\n> >>> - maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> >>> - .boundedTo(resolution);\n> >>> + if (!sensorFormat.size.isNull()) {\n> >>> + minResolution = sensorFormat.size.boundedTo(ispMaxInSize_);\n> >>> + maxResolution = sensorFormat.size.boundedTo(ispMaxInSize_);\n> >>> + }\n> >>> }\n> >>> \n> >>> cfg->size.boundTo(maxResolution);\n> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> >>> index cd77957ee..c96bd5557 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, const Size &ispMaxInSize);\n> >>> \n> >>> int setEnabled(bool enable) { return link_->setEnabled(enable); }\n> >>> bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n> >>> @@ -77,6 +77,8 @@ private:\n> >>> std::unique_ptr resizer_;\n> >>> std::unique_ptr video_;\n> >>> MediaLink *link_;\n> >>> +\n> >>> + Size ispMaxInSize_;\n> >>> };\n> >>> \n> >>> class RkISP1MainPath : public RkISP1Path\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 70BB8BDB1C\n\tfor ;\n\tTue, 23 Jul 2024 08:51:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2C47F6336F;\n\tTue, 23 Jul 2024 10:51:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5ABDE6199E\n\tfor ;\n\tTue, 23 Jul 2024 10:51:47 +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 E7845AFD;\n\tTue, 23 Jul 2024 10:51:04 +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=\"C4YJJvLm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721724665;\n\tbh=B8fsZCwP7qEaDlFjbsiKT9+0Z105FHTkKUZ9QH3yHzo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=C4YJJvLmF7ukJ3/3k3O7h1kgMRM+44n65N3Hx4GKm281h7WDJMCcTi7kvmJPUDYKK\n\t3HTpprSbLzYKI/MvBzhEhSXyHexXoTZB/EsPRzF8NZ3E+btTB7NpX8CpA/Y2LbQgT7\n\tTP38eMqugy9EyB5cU8ofwz2pFJ0vgD6ot4HBIc3M=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<0052b2f5-1382-4238-a853-1ee152a3b4de@ideasonboard.com>","References":"<20240116091754.100654-1-paul.elder@ideasonboard.com>\n\t<20240116091754.100654-3-paul.elder@ideasonboard.com>\n\t<170549182302.1011926.15304309484938386532@ping.linuxembedded.co.uk>\n\t<20240117155715.GI4860@pendragon.ideasonboard.com>\n\t<0052b2f5-1382-4238-a853-1ee152a3b4de@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation\n\twhen sensor max is larger than ISP input","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ,\n\tUmang Jain ","Date":"Tue, 23 Jul 2024 09:51:44 +0100","Message-ID":"<172172470445.392292.10868066912190375815@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":30570,"web_url":"https://patchwork.libcamera.org/comment/30570/","msgid":"","date":"2024-08-05T07:32:03","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation\n\twhen sensor max is larger than ISP 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 17/01/24 9:27 pm, Laurent Pinchart via libcamera-devel wrote:\n> On Wed, Jan 17, 2024 at 11:43:43AM +0000, Kieran Bingham via libcamera-devel wrote:\n>> Quoting Paul Elder via libcamera-devel (2024-01-16 09:17:53)\n>>> If the maximum sensor output size is larger than the maximum ISP input\n>>> size, the maximum sensor size could be selected and the pipeline would\n>>> fail with an EPIPE. Fix this by clamping the sensor size to the ISP\n>>> input size at validation time.\n>> I think we should do more here (in a separate patch) to filter out\n>> sensor modes that are larger than the ISP max as well.\n>>\n>> At the moment it seems too easy for the rkisp1 pipeline handler to\n>> select an input configuration that it can't actually configure.\n>>\n>> But that's aside from this patch itself.\n>>\n>>> Signed-off-by: Paul Elder \n>>> ---\n>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 ++++++++++--\n>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------\n>>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 ++-\n>>> 3 files changed, 30 insertions(+), 17 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> index 586b46d64..fb67ba8f4 100644\n>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> @@ -1202,11 +1202,24 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>>> if (param_->open() < 0)\n>>> return false;\n>>> \n>>> + /*\n>>> + * Retrieve ISP maximum input size for config validation in the path\n> s/Retrieve/Retrieve the/\n>\n>>> + * classes\n> s/$/./\n>\n>>> + */\n>>> + Size ispMaxInSize = { 0, 0 };\n> That's the default, so you can just write\n>\n> \tSize ispMaxInSize;\n>\n> I also find ispMaxInputSize more readable.\n>\n>>> + V4L2Subdevice::Formats ispFormats = isp_->formats(0);\n> const if you don't need to modify it.\n>\n>>> + for (const auto &[mbus, sizes] : ispFormats) {\n>>> + for (const auto &size : sizes) {\n>>> + if (ispMaxInSize < size.max)\n>>> + ispMaxInSize = size.max;\n>>> + }\n>>> + }\n> Do the maximum input size depend on the media bus format ? And to we\n> have multiple sizes in the sizes vector ? We could take some shortcuts,\n> knowing how the ISP driver is implemented, taking the first entry in\n> both ispFormats and sizes.\n\nThe input size (as per the driver implementation) does not depend on the \nmedia bus format and contains only one sizes entry (min..max range) for \neach of the supported media bus formats.\n\nSo indeed, knowing the driver implementation we can probalby get rid of \nthis loop.\n\n>\n>>> +\n>>> /* Locate and open the ISP main and self paths. */\n>>> - if (!mainPath_.init(media_))\n>>> + if (!mainPath_.init(media_, ispMaxInSize))\n>>> return false;\n>>> \n>>> - if (hasSelfPath_ && !selfPath_.init(media_))\n>>> + if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInSize))\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 b62b645ca..eaab7e857 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, const Size &ispMaxInSize)\n> ispMaxInputSize here too if you rename it above. Same below.\n>\n>> It feels a bit odd to me passing ispMaxInSize as the only parameter to\n>> initialize. But I guess it save init from re-determining it for each\n>> path, and maybe it's the only variable for now - so I think we're ok\n>> here.\n>>\n>> I wonder if we should iterate and 'try' configurations to only expose\n>> those which can be supported. But we can tackle that on top I think.\n> Agreed.\n>\n>> So for now I am ok with this:\n>>\n>> Reviewed-by: Kieran Bingham \n> Reviewed-by: Laurent Pinchart \n>\n> with the above comments taken into account.\n>\n>>> {\n>>> std::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n>>> std::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n>>> @@ -76,6 +76,7 @@ bool RkISP1Path::init(MediaDevice *media)\n>>> return false;\n>>> \n>>> populateFormats();\n>>> + ispMaxInSize_ = ispMaxInSize;\n>>> \n>>> link_ = media->link(\"rkisp1_isp\", 2, resizer, 0);\n>>> if (!link_)\n>>> @@ -172,7 +173,9 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n>>> /* Add all the RAW sizes the sensor can produce for this code. */\n>>> for (const auto &rawSize : sensor->sizes(mbusCode)) {\n>>> if (rawSize.width > maxResolution_.width ||\n>>> - rawSize.height > maxResolution_.height)\n>>> + rawSize.height > maxResolution_.height ||\n>>> + rawSize.width > ispMaxInSize_.width ||\n>>> + rawSize.height > ispMaxInSize_.height)\n>>> continue;\n>>> \n>>> streamFormats[format].push_back({ rawSize, rawSize });\n>>> @@ -275,8 +278,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>>> if (!found)\n>>> cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;\n>>> \n>>> - Size minResolution;\n>>> - Size maxResolution;\n>>> + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n>>> + .boundedTo(resolution);\n>>> + Size minResolution = minResolution_.expandedToAspectRatio(resolution);\n>>> \n>>> if (isRaw) {\n>>> /*\n>>> @@ -287,16 +291,10 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>>> V4L2SubdeviceFormat sensorFormat =\n>>> sensor->getFormat({ mbusCode }, cfg->size);\n>>> \n>>> - minResolution = sensorFormat.size;\n>>> - maxResolution = sensorFormat.size;\n>>> - } else {\n>>> - /*\n>>> - * Adjust the size based on the sensor resolution and absolute\n>>> - * limits of the ISP.\n>>> - */\n>>> - minResolution = minResolution_.expandedToAspectRatio(resolution);\n>>> - maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n>>> - .boundedTo(resolution);\n>>> + if (!sensorFormat.size.isNull()) {\n>>> + minResolution = sensorFormat.size.boundedTo(ispMaxInSize_);\n>>> + maxResolution = sensorFormat.size.boundedTo(ispMaxInSize_);\n>>> + }\n>>> }\n>>> \n>>> cfg->size.boundTo(maxResolution);\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>>> index cd77957ee..c96bd5557 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, const Size &ispMaxInSize);\n>>> \n>>> int setEnabled(bool enable) { return link_->setEnabled(enable); }\n>>> bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n>>> @@ -77,6 +77,8 @@ private:\n>>> std::unique_ptr resizer_;\n>>> std::unique_ptr video_;\n>>> MediaLink *link_;\n>>> +\n>>> + Size ispMaxInSize_;\n>>> };\n>>> \n>>> class RkISP1MainPath : public RkISP1Path","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 11D73C323E\n\tfor ;\n\tMon, 5 Aug 2024 07:32:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 06DEC63381;\n\tMon, 5 Aug 2024 09:32:09 +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 388676195A\n\tfor ;\n\tMon, 5 Aug 2024 09:32:07 +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 5B0FC1A2;\n\tMon, 5 Aug 2024 09:31:15 +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=\"s3Ryb0ze\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722843075;\n\tbh=B/brz0f6Kb6oT8+NPtwdVFIkFlqbkOF6AehzpbJOTYY=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=s3Ryb0zeg7zGy+XoqeoGdPEP10OglaoGjsX+0yhtg4l9vHznpmWkFWloeXSTxXsWp\n\tmEt7OGghIDmiJrmTgfkV7UAy2r5tznnltWMToWh2NBPAFDyYB4Q030wpOmxAXJF0sc\n\t5o/cEKMU/MlAjmX489PQJfktAZ12p5L15WPAPHkE=","Message-ID":"","Date":"Mon, 5 Aug 2024 13:02:03 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation\n\twhen sensor max is larger than ISP input","To":"Laurent Pinchart ,\n\tKieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240116091754.100654-1-paul.elder@ideasonboard.com>\n\t<20240116091754.100654-3-paul.elder@ideasonboard.com>\n\t<170549182302.1011926.15304309484938386532@ping.linuxembedded.co.uk>\n\t<20240117155715.GI4860@pendragon.ideasonboard.com>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<20240117155715.GI4860@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\" "}}]