[{"id":34103,"web_url":"https://patchwork.libcamera.org/comment/34103/","msgid":"<355hbz3twcl2vjal2vti7rr2fbrklgru6jpo2gakvkypgue7c6@nox47k35qf2z>","date":"2025-05-02T08:21:13","subject":"Re: [PATCH 1/3] libcamera: pipeline: rkisp1: Detect invalid sensor\n\tconfigurations","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Thu, May 01, 2025 at 03:16:07PM +0100, Kieran Bingham wrote:\n> If we select a Sensor Format that is larger than the ISP capabilities\n> the pipeline will not be able to successfully start.\n>\n> Detect this during validate - and report accordingly, returning\n> an 'Invalid' state to reflect that we were not able to\n> reconfigure to adjust to a working state in this instance.\n>\n> Signed-off-by: Kieran Bingham \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 675f0a7490a6..d8c6100946bc 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -673,9 +673,21 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \tsensorFormat_ = sensor->getFormat(mbusCodes, maxSize,\n> \t\t\t\t\t mainPath->maxResolution());\n>\n> +\n\nAdditional blank line\n\n> +\t/*\n> +\t * TODO: There doesn't seem to be a valid occasion to set the size to\n> +\t * the native resolution if there was not a supported size found above.\n\nIt might be me, but I can't parse this\n\n> +\t */\n> \tif (sensorFormat_.size.isNull())\n> \t\tsensorFormat_.size = sensor->resolution();\n>\n> +\tif (sensorFormat_.size > mainPath->maxResolution()) {\n> +\t\tLOG(RkISP1, Error)\n> +\t\t\t<< \"Sensor format size \" << sensorFormat_.size\n> +\t\t\t<< \" exceeds maximum possible size \" << mainPath->maxResolution();\n> +\t\treturn Invalid;\n\nThis should never happen as\n\n \tsensorFormat_ = sensor->getFormat(mbusCodes, maxSize,\n \t\t\t\t\t mainPath->maxResolution());\n\nShould guarantee that the returned resolution is never larger than\nmainPath->maxResolution() ?\n\n\n> +\t}\n> +\n> \treturn status;\n> }\n>\n> --\n> 2.48.1\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 41EBDBE08B\n\tfor ;\n\tFri, 2 May 2025 08:21:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99BEE68AD9;\n\tFri, 2 May 2025 10:21:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7141968AD8\n\tfor ;\n\tFri, 2 May 2025 10:21:17 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9DD71353;\n\tFri, 2 May 2025 10:21:09 +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=\"UGI5APg+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746174069;\n\tbh=6gsg+EeNAp7B0cHLEXgzisye0TYi5T7t+a9BNlLZGYQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UGI5APg+3eTNYFUhNoCcChqM8AphuidmDnwNeHcw4hk9Dd6pFqF/DUmStuK5/09U3\n\t0T57Pyn2hI4Q2KGDuxakgU6zuyevrvlhwBxWlll93+AE7zbxwD+dMhjHh95X7DOr+4\n\tpx69ES1xrrUt0RtvMEffY4LPBg1OLD1omT8/ZSF0=","Date":"Fri, 2 May 2025 10:21:13 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"libcamera devel ","Subject":"Re: [PATCH 1/3] libcamera: pipeline: rkisp1: Detect invalid sensor\n\tconfigurations","Message-ID":"<355hbz3twcl2vjal2vti7rr2fbrklgru6jpo2gakvkypgue7c6@nox47k35qf2z>","References":"<20250501141609.717148-1-kieran.bingham@ideasonboard.com>\n\t<20250501141609.717148-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250501141609.717148-2-kieran.bingham@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":34108,"web_url":"https://patchwork.libcamera.org/comment/34108/","msgid":"<174618058941.1586992.11622922141180449232@ping.linuxembedded.co.uk>","date":"2025-05-02T10:09:49","subject":"Re: [PATCH 1/3] libcamera: pipeline: rkisp1: Detect invalid sensor\n\tconfigurations","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2025-05-02 09:21:13)\n> Hi Kieran\n> \n> On Thu, May 01, 2025 at 03:16:07PM +0100, Kieran Bingham wrote:\n> > If we select a Sensor Format that is larger than the ISP capabilities\n> > the pipeline will not be able to successfully start.\n> >\n> > Detect this during validate - and report accordingly, returning\n> > an 'Invalid' state to reflect that we were not able to\n> > reconfigure to adjust to a working state in this instance.\n> >\n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 ++++++++++++\n> > 1 file changed, 12 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 675f0a7490a6..d8c6100946bc 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -673,9 +673,21 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > sensorFormat_ = sensor->getFormat(mbusCodes, maxSize,\n> > mainPath->maxResolution());\n> >\n> > +\n> \n> Additional blank line\n> \n> > + /*\n> > + * TODO: There doesn't seem to be a valid occasion to set the size to\n> > + * the native resolution if there was not a supported size found above.\n> \n> It might be me, but I can't parse this\n> \n> > + */\n> > if (sensorFormat_.size.isNull())\n> > sensorFormat_.size = sensor->resolution();\n> >\n> > + if (sensorFormat_.size > mainPath->maxResolution()) {\n> > + LOG(RkISP1, Error)\n> > + << \"Sensor format size \" << sensorFormat_.size\n> > + << \" exceeds maximum possible size \" << mainPath->maxResolution();\n> > + return Invalid;\n> \n> This should never happen as\n\nAha, so you see A can't happen and I see B can't happen ;-)\n\nI think somewhere there's a logic change as part of the dewarp rotation\nenablement which /isn't/ in this tree but didn't prevent me applying\nthese patches on mainline - so I could perhaps say this patch needs to\nbe part of the dewarp support instead of targetting mainline already.\n\nI also think there's something I've probably missed elsewhere so I\nshould re-parse the logic paths here...\n\n> sensorFormat_ = sensor->getFormat(mbusCodes, maxSize,\n> mainPath->maxResolution());\n> \n> Should guarantee that the returned resolution is never larger than\n> mainPath->maxResolution() ?\n\n\nBut it can return a null format (As seen by the previous debug\nenablement patch) which then leads to :\n\n\n> > if (sensorFormat_.size.isNull())\n> > sensorFormat_.size = sensor->resolution();\n\n\nAnd in this case I have an IMX283 20MP camera connected which is a\nlarger resolution than the ISP can parse.\n\nSo we're suddenly setting an invalid config, that could never be\nconfigured.\n\n\n\n> \n> \n> > + }\n> > +\n> > return status;\n> > }\n> >\n> > --\n> > 2.48.1\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 148C8BE08B\n\tfor ;\n\tFri, 2 May 2025 10:09:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5687368ADE;\n\tFri, 2 May 2025 12:09:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 260D068AD3\n\tfor ;\n\tFri, 2 May 2025 12:09:53 +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 3FB2E353;\n\tFri, 2 May 2025 12:09: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=\"bOOQJPJl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746180585;\n\tbh=rGBQw/ruSYrKg+8G3oR5xvslXZm11nye2qk8XU7pqSg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=bOOQJPJlkmdfAlL/tWPskzQNS0HyxMtQ5bMLq7ykwFCFXNVhrP5tZxOzeC32cyR41\n\tzS5tLK401J//zxRckbkiCXMNfJZvXprJIIW7oFWNLH8W8vjGKASowMm+dRMKSa1hsV\n\tAaABUa6HtIsJQJyb7cug5lSgB5u8nfXwVFNq7jIA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<355hbz3twcl2vjal2vti7rr2fbrklgru6jpo2gakvkypgue7c6@nox47k35qf2z>","References":"<20250501141609.717148-1-kieran.bingham@ideasonboard.com>\n\t<20250501141609.717148-2-kieran.bingham@ideasonboard.com>\n\t<355hbz3twcl2vjal2vti7rr2fbrklgru6jpo2gakvkypgue7c6@nox47k35qf2z>","Subject":"Re: [PATCH 1/3] libcamera: pipeline: rkisp1: Detect invalid sensor\n\tconfigurations","From":"Kieran Bingham ","Cc":"libcamera devel ","To":"Jacopo Mondi ","Date":"Fri, 02 May 2025 11:09:49 +0100","Message-ID":"<174618058941.1586992.11622922141180449232@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":34112,"web_url":"https://patchwork.libcamera.org/comment/34112/","msgid":"","date":"2025-05-02T10:17:09","subject":"Re: [PATCH 1/3] libcamera: pipeline: rkisp1: Detect invalid sensor\n\tconfigurations","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Fri, May 02, 2025 at 11:09:49AM +0100, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2025-05-02 09:21:13)\n> > Hi Kieran\n> >\n> > On Thu, May 01, 2025 at 03:16:07PM +0100, Kieran Bingham wrote:\n> > > If we select a Sensor Format that is larger than the ISP capabilities\n> > > the pipeline will not be able to successfully start.\n> > >\n> > > Detect this during validate - and report accordingly, returning\n> > > an 'Invalid' state to reflect that we were not able to\n> > > reconfigure to adjust to a working state in this instance.\n> > >\n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 ++++++++++++\n> > > 1 file changed, 12 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 675f0a7490a6..d8c6100946bc 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -673,9 +673,21 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > sensorFormat_ = sensor->getFormat(mbusCodes, maxSize,\n> > > mainPath->maxResolution());\n> > >\n> > > +\n> >\n> > Additional blank line\n> >\n> > > + /*\n> > > + * TODO: There doesn't seem to be a valid occasion to set the size to\n> > > + * the native resolution if there was not a supported size found above.\n> >\n> > It might be me, but I can't parse this\n> >\n> > > + */\n> > > if (sensorFormat_.size.isNull())\n> > > sensorFormat_.size = sensor->resolution();\n> > >\n> > > + if (sensorFormat_.size > mainPath->maxResolution()) {\n> > > + LOG(RkISP1, Error)\n> > > + << \"Sensor format size \" << sensorFormat_.size\n> > > + << \" exceeds maximum possible size \" << mainPath->maxResolution();\n> > > + return Invalid;\n> >\n> > This should never happen as\n>\n> Aha, so you see A can't happen and I see B can't happen ;-)\n>\n> I think somewhere there's a logic change as part of the dewarp rotation\n> enablement which /isn't/ in this tree but didn't prevent me applying\n> these patches on mainline - so I could perhaps say this patch needs to\n> be part of the dewarp support instead of targetting mainline already.\n\nProbably better, to get a more complete picture\n\n>\n> I also think there's something I've probably missed elsewhere so I\n> should re-parse the logic paths here...\n>\n> > sensorFormat_ = sensor->getFormat(mbusCodes, maxSize,\n> > mainPath->maxResolution());\n> >\n> > Should guarantee that the returned resolution is never larger than\n> > mainPath->maxResolution() ?\n>\n>\n> But it can return a null format (As seen by the previous debug\n> enablement patch) which then leads to :\n>\n>\n> > > if (sensorFormat_.size.isNull())\n> > > sensorFormat_.size = sensor->resolution();\n>\n>\n> And in this case I have an IMX283 20MP camera connected which is a\n> larger resolution than the ISP can parse.\n>\n> So we're suddenly setting an invalid config, that could never be\n> configured.\n\nRight. Should we simply return Invalid instead of trying to adjust to\nsomething that can't be guaranteed to work ?\n\n>\n>\n>\n> >\n> >\n> > > + }\n> > > +\n> > > return status;\n> > > }\n> > >\n> > > --\n> > > 2.48.1\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 DDB23C327D\n\tfor ;\n\tFri, 2 May 2025 10:17:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 99A0168B25;\n\tFri, 2 May 2025 12:17:13 +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 9591568AD8\n\tfor ;\n\tFri, 2 May 2025 12:17:12 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DC537A98;\n\tFri, 2 May 2025 12:17: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=\"IoZ4Mz+k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746181025;\n\tbh=kLsEALyGkzeBP4o13O8SU5yoaTqriWZI88loRt4avqE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IoZ4Mz+k2HnUkr2AIWykXdquea/ujsX7OWSvPJzXXf0VDOnZQ0X7PpFl9GwCCAa0l\n\talqkA3vwsXx6BBkORadpGnZZEfoYxVH7D3Y99Knp9FpHjcT4EhVjmRjz2cBzh35x01\n\t3d+e8uCvMgJZ6TH9HCQqHsro1E/+ZxP6np/NrmBk=","Date":"Fri, 2 May 2025 12:17:09 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"Jacopo Mondi , \n\tlibcamera devel ","Subject":"Re: [PATCH 1/3] libcamera: pipeline: rkisp1: Detect invalid sensor\n\tconfigurations","Message-ID":"","References":"<20250501141609.717148-1-kieran.bingham@ideasonboard.com>\n\t<20250501141609.717148-2-kieran.bingham@ideasonboard.com>\n\t<355hbz3twcl2vjal2vti7rr2fbrklgru6jpo2gakvkypgue7c6@nox47k35qf2z>\n\t<174618058941.1586992.11622922141180449232@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<174618058941.1586992.11622922141180449232@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\" "}}]