[{"id":31583,"web_url":"https://patchwork.libcamera.org/comment/31583/","msgid":"","date":"2024-10-04T09:37:55","subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Umang\n\nOn Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote:\n> Commit 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not\n> supported by the pipeline\") introduced a mechanism to determine maximum\n> supported sensor resolution and filter out resolutions that cannot be\n> supported by the ISP.\n>\n> However, it missed to update the raw stream configuration path, where\n> it should have clamped the raw stream configuration size to the maximum\n> sensor supported resolution.\n>\n> This patch fixes the above issue and can be confirmed with IMX283\n> on i.MX8MP:\n>\n> From:\n> ($) cam -c1 -srole=raw,width=5472,height=3072\n> INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12\n> ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12\n> Failed to configure camera\n> Failed to start camera session\n>\n> To:\n> ($) cam -c1 -srole=raw,width=5472,height=3072\n> INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12\n> cam0: Capture until user interrupts by SIGINT\n> 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824\n> 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824\n> 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824\n> ...\n>\n> Fixes: 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline\")\n> Signed-off-by: Umang Jain \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index da8d25c3..feb6d89f 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \tif (isRaw) {\n> \t\t/*\n> \t\t * Use the sensor output size closest to the requested stream\n> -\t\t * size.\n> +\t\t * size while ensuring the output size doesn't exceed ISP limits.\n> \t\t */\n> \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n> +\t\tcfg->size.boundTo(resolution);\n> +\n> \t\tV4L2SubdeviceFormat sensorFormat =\n> \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n\nCameraSensor::getFormat() returns a sensor resolution large enough to\naccomodate the requested size, at least that's how I read the\nfollowing part of the documentation\n\n * \\a size indicates the desired size at the output of the sensor. This function\n * selects the best media bus code and size supported by the sensor according\n * to the following criteria.\n *\n * - The desired \\a size shall fit in the sensor output size to avoid the need\n * to up-scale.\n\n And this part of the code\n\n if (sz.width < size.width || sz.height < size.height)\n continue;\n\nSo, at least in my understanding \"sensorFormat\" could represent a size\nlarger than cfg->size. Is this your understanding as well ?\n\nIn the lines below this function we have\n\n\t\tminResolution = sensorFormat.size;\n\t\tmaxResolution = sensorFormat.size;\n\n cfg->size.boundTo(maxResolution);\n cfg->size.expandTo(minResolution);\n\nSo if ((maxResolution = minResolution) > cfg->size) will the calls to\nboundTo() followed by expandTo() enlarge cfg->size ?\n\nWouldn't it be better to use 'resolution' in\n\n\t\tV4L2SubdeviceFormat sensorFormat =\n\t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n\ninstead of cfg->size ?\n\nThanks\n j\n\n>\n> --\n> 2.45.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 8CFCDC3257\n\tfor ;\n\tFri, 4 Oct 2024 09:38:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E5AE63527;\n\tFri, 4 Oct 2024 11:38: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 7F75863512\n\tfor ;\n\tFri, 4 Oct 2024 11:38:07 +0200 (CEST)","from ideasonboard.com (unknown [5.77.86.253])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5EFF059C;\n\tFri, 4 Oct 2024 11:36:31 +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=\"YFvE4f/A\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728034593;\n\tbh=NIgPegcHMSajo7SjeTlassWSd7lCDtIpdFc75VT4ly4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YFvE4f/ARiHnSZpfLUkiyVPoT1bNmkj7qQmA+R/6CiGhIwv4p0dZDNFQmlBepOb2H\n\tqH2sPtF5zDV9MznVshNQ22Qa7sgrzmjGmmyy+5TH7VerJy7VwsX2IMnK4+2J7qEAfz\n\twDgNgE+S3iHcDY31vDHcyGUSWo8N85L+a+tZHpF4=","Date":"Fri, 4 Oct 2024 11:37:55 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org, \n\tKieran Bingham ,\n\tJacopo Mondi ","Subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","Message-ID":"","References":"<20241004045338.18443-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241004045338.18443-1-umang.jain@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31584,"web_url":"https://patchwork.libcamera.org/comment/31584/","msgid":"","date":"2024-10-04T09:47:34","subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 04/10/24 3:07 pm, Jacopo Mondi wrote:\n> Hi Umang\n>\n> On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote:\n>> Commit 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not\n>> supported by the pipeline\") introduced a mechanism to determine maximum\n>> supported sensor resolution and filter out resolutions that cannot be\n>> supported by the ISP.\n>>\n>> However, it missed to update the raw stream configuration path, where\n>> it should have clamped the raw stream configuration size to the maximum\n>> sensor supported resolution.\n>>\n>> This patch fixes the above issue and can be confirmed with IMX283\n>> on i.MX8MP:\n>>\n>> From:\n>> ($) cam -c1 -srole=raw,width=5472,height=3072\n>> INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12\n>> ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12\n>> Failed to configure camera\n>> Failed to start camera session\n>>\n>> To:\n>> ($) cam -c1 -srole=raw,width=5472,height=3072\n>> INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12\n>> cam0: Capture until user interrupts by SIGINT\n>> 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824\n>> 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824\n>> 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824\n>> ...\n>>\n>> Fixes: 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline\")\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-\n>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> index da8d25c3..feb6d89f 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>> \tif (isRaw) {\n>> \t\t/*\n>> \t\t * Use the sensor output size closest to the requested stream\n>> -\t\t * size.\n>> +\t\t * size while ensuring the output size doesn't exceed ISP limits.\n>> \t\t */\n>> \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n>> +\t\tcfg->size.boundTo(resolution);\n>> +\n>> \t\tV4L2SubdeviceFormat sensorFormat =\n>> \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> CameraSensor::getFormat() returns a sensor resolution large enough to\n> accomodate the requested size, at least that's how I read the\n> following part of the documentation\n>\n> * \\a size indicates the desired size at the output of the sensor. This function\n> * selects the best media bus code and size supported by the sensor according\n> * to the following criteria.\n> *\n> * - The desired \\a size shall fit in the sensor output size to avoid the need\n> * to up-scale.\n>\n> And this part of the code\n>\n> if (sz.width < size.width || sz.height < size.height)\n> continue;\n>\n> So, at least in my understanding \"sensorFormat\" could represent a size\n> larger than cfg->size. Is this your understanding as well ?\n\n\"sensorFormat\" could represent a size larger than cfg->size\n\nand\n\ncould also represent a size large than max supported resolution (i.e. \n'resolution' variable in this case)\n\nFor e.g. in IMX283 case it would,  5472x3648\n\n>\n> In the lines below this function we have\n>\n> \t\tminResolution = sensorFormat.size;\n> \t\tmaxResolution = sensorFormat.size;\n\n From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648\n>\n> cfg->size.boundTo(maxResolution);\n> cfg->size.expandTo(minResolution);\n>\n> So if ((maxResolution = minResolution) > cfg->size) will the calls to\n> boundTo() followed by expandTo() enlarge cfg->size ?\n\nso cfg->size is 5472x3648 here, which can't be supported.\n\nHave you noticed the failure case I mentioned in the commit message?\n>\n> Wouldn't it be better to use 'resolution' in\n>\n> \t\tV4L2SubdeviceFormat sensorFormat =\n> \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n>\n> instead of cfg->size ?\n\nNo, otherwise it will always pick the highest sensor output size.\n\nFor e.g. I request -srole=raw,width=1920,height=1080\n\n\"cfg->size\" will pick 2736x1824 for IMX283\n\n\"resolution\" will always pick 4096x3072.\n>\n> Thanks\n> j\n>\n>> --\n>> 2.45.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 4F57FBD80A\n\tfor ;\n\tFri, 4 Oct 2024 09:47:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1F40763512;\n\tFri, 4 Oct 2024 11:47:40 +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 6380563512\n\tfor ;\n\tFri, 4 Oct 2024 11:47:38 +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 EC20459C;\n\tFri, 4 Oct 2024 11:46:03 +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=\"i7aGL+2b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728035164;\n\tbh=RPxUYOeq7Asj6HrzEYrttlAGaeAR1MGl1W2c/kgW7w8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=i7aGL+2bckTUc3iipjQMxLifikUk7LPG0Xq9LXXMtxYsD0yfY9HYN6W6IZeKx17Hg\n\t6+Uvb2AGpLVSzbqaJiG5/3Qz6N9NICEo2d/7FIosw+o7bdghvEshC3KqusVQTVsH5s\n\tyLhwtC4ND3/qgbQyvCHKJK2N9LkaImE0WoeE23so=","Message-ID":"","Date":"Fri, 4 Oct 2024 15:17:34 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","References":"<20241004045338.18443-1-umang.jain@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"","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":31593,"web_url":"https://patchwork.libcamera.org/comment/31593/","msgid":"","date":"2024-10-04T10:28:50","subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Umang\n\nOn Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote:\n> Hi Jacopo\n>\n> On 04/10/24 3:07 pm, Jacopo Mondi wrote:\n> > Hi Umang\n> >\n> > On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote:\n> > > Commit 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not\n> > > supported by the pipeline\") introduced a mechanism to determine maximum\n> > > supported sensor resolution and filter out resolutions that cannot be\n> > > supported by the ISP.\n> > >\n> > > However, it missed to update the raw stream configuration path, where\n> > > it should have clamped the raw stream configuration size to the maximum\n> > > sensor supported resolution.\n> > >\n> > > This patch fixes the above issue and can be confirmed with IMX283\n> > > on i.MX8MP:\n> > >\n> > > From:\n> > > ($) cam -c1 -srole=raw,width=5472,height=3072\n> > > INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12\n> > > ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12\n> > > Failed to configure camera\n> > > Failed to start camera session\n> > >\n> > > To:\n> > > ($) cam -c1 -srole=raw,width=5472,height=3072\n> > > INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12\n> > > cam0: Capture until user interrupts by SIGINT\n> > > 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824\n> > > 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824\n> > > 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824\n> > > ...\n> > >\n> > > Fixes: 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline\")\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-\n> > > 1 file changed, 3 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > index da8d25c3..feb6d89f 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> > > \tif (isRaw) {\n> > > \t\t/*\n> > > \t\t * Use the sensor output size closest to the requested stream\n> > > -\t\t * size.\n> > > +\t\t * size while ensuring the output size doesn't exceed ISP limits.\n> > > \t\t */\n> > > \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n> > > +\t\tcfg->size.boundTo(resolution);\n> > > +\n> > > \t\tV4L2SubdeviceFormat sensorFormat =\n> > > \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> > CameraSensor::getFormat() returns a sensor resolution large enough to\n> > accomodate the requested size, at least that's how I read the\n> > following part of the documentation\n> >\n> > * \\a size indicates the desired size at the output of the sensor. This function\n> > * selects the best media bus code and size supported by the sensor according\n> > * to the following criteria.\n> > *\n> > * - The desired \\a size shall fit in the sensor output size to avoid the need\n> > * to up-scale.\n> >\n> > And this part of the code\n> >\n> > if (sz.width < size.width || sz.height < size.height)\n> > continue;\n> >\n> > So, at least in my understanding \"sensorFormat\" could represent a size\n> > larger than cfg->size. Is this your understanding as well ?\n>\n> \"sensorFormat\" could represent a size larger than cfg->size\n>\n> and\n>\n> could also represent a size large than max supported resolution (i.e.\n> 'resolution' variable in this case)\n>\n> For e.g. in IMX283 case it would,  5472x3648\n>\n> >\n> > In the lines below this function we have\n> >\n> > \t\tminResolution = sensorFormat.size;\n> > \t\tmaxResolution = sensorFormat.size;\n>\n> From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648\n> >\n> > cfg->size.boundTo(maxResolution);\n> > cfg->size.expandTo(minResolution);\n> >\n> > So if ((maxResolution = minResolution) > cfg->size) will the calls to\n> > boundTo() followed by expandTo() enlarge cfg->size ?\n>\n> so cfg->size is 5472x3648 here, which can't be supported.\n\nSorry, I'm confused. Aren't you confirming me that the resulting\ncfg->size cannot be supported (with this patch I mean) ?\n\n>\n> Have you noticed the failure case I mentioned in the commit message?\n\nWhile the commmit message suggests this patch makes it valid ?\n\nWhat am I missing ? :)\n\n> >\n> > Wouldn't it be better to use 'resolution' in\n> >\n> > \t\tV4L2SubdeviceFormat sensorFormat =\n> > \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> >\n> > instead of cfg->size ?\n>\n> No, otherwise it will always pick the highest sensor output size.\n>\n> For e.g. I request -srole=raw,width=1920,height=1080\n>\n> \"cfg->size\" will pick 2736x1824 for IMX283\n>\n> \"resolution\" will always pick 4096x3072.\n> >\n> > Thanks\n> > j\n> >\n> > > --\n> > > 2.45.2\n> > >\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 CDE81BD80A\n\tfor ;\n\tFri, 4 Oct 2024 10:29:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CF01463523;\n\tFri, 4 Oct 2024 12:29:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C98362C8F\n\tfor ;\n\tFri, 4 Oct 2024 12:29:03 +0200 (CEST)","from ideasonboard.com (unknown [5.77.86.253])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0AF6555A;\n\tFri, 4 Oct 2024 12:27:26 +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=\"S2VVCO71\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728037649;\n\tbh=Dxj0N3Z0q604XcIz3Qw+TMKey70E/Yyqei4/vadPq2M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=S2VVCO71zyXfWJI2asxeW1cxCR0FYoShy8l/K1oQ4Xh96tjY++uKNyoHza90P1I82\n\t1hJjXzS6qPlJm91RjvJ3zzX0HLqiAwc3EphcayuC7QAJR5XjSrXvpQqMOoy6KcuODK\n\tukSYBR/aGCwYhmRRoFc7foJdkx5+yAHMd7YFdvx4=","Date":"Fri, 4 Oct 2024 12:28:50 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","Message-ID":"","References":"<20241004045338.18443-1-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31594,"web_url":"https://patchwork.libcamera.org/comment/31594/","msgid":"<4e1e134e-97a8-48c0-bd6c-8056c6436247@ideasonboard.com>","date":"2024-10-04T11:15:01","subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 04/10/24 3:58 pm, Jacopo Mondi wrote:\n> Hi Umang\n>\n> On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote:\n>> Hi Jacopo\n>>\n>> On 04/10/24 3:07 pm, Jacopo Mondi wrote:\n>>> Hi Umang\n>>>\n>>> On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote:\n>>>> Commit 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not\n>>>> supported by the pipeline\") introduced a mechanism to determine maximum\n>>>> supported sensor resolution and filter out resolutions that cannot be\n>>>> supported by the ISP.\n>>>>\n>>>> However, it missed to update the raw stream configuration path, where\n>>>> it should have clamped the raw stream configuration size to the maximum\n>>>> sensor supported resolution.\n>>>>\n>>>> This patch fixes the above issue and can be confirmed with IMX283\n>>>> on i.MX8MP:\n>>>>\n>>>> From:\n>>>> ($) cam -c1 -srole=raw,width=5472,height=3072\n>>>> INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12\n>>>> ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12\n>>>> Failed to configure camera\n>>>> Failed to start camera session\n>>>>\n>>>> To:\n>>>> ($) cam -c1 -srole=raw,width=5472,height=3072\n>>>> INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12\n>>>> cam0: Capture until user interrupts by SIGINT\n>>>> 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824\n>>>> 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824\n>>>> 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824\n>>>> ...\n>>>>\n>>>> Fixes: 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline\")\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-\n>>>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>>> index da8d25c3..feb6d89f 100644\n>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>>> @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>>>> \tif (isRaw) {\n>>>> \t\t/*\n>>>> \t\t * Use the sensor output size closest to the requested stream\n>>>> -\t\t * size.\n>>>> +\t\t * size while ensuring the output size doesn't exceed ISP limits.\n>>>> \t\t */\n>>>> \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n>>>> +\t\tcfg->size.boundTo(resolution);\n>>>> +\n>>>> \t\tV4L2SubdeviceFormat sensorFormat =\n>>>> \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n>>> CameraSensor::getFormat() returns a sensor resolution large enough to\n>>> accomodate the requested size, at least that's how I read the\n>>> following part of the documentation\n>>>\n>>> * \\a size indicates the desired size at the output of the sensor. This function\n>>> * selects the best media bus code and size supported by the sensor according\n>>> * to the following criteria.\n>>> *\n>>> * - The desired \\a size shall fit in the sensor output size to avoid the need\n>>> * to up-scale.\n>>>\n>>> And this part of the code\n>>>\n>>> if (sz.width < size.width || sz.height < size.height)\n>>> continue;\n>>>\n>>> So, at least in my understanding \"sensorFormat\" could represent a size\n>>> larger than cfg->size. Is this your understanding as well ?\n>> \"sensorFormat\" could represent a size larger than cfg->size\n>>\n>> and\n>>\n>> could also represent a size large than max supported resolution (i.e.\n>> 'resolution' variable in this case)\n>>\n>> For e.g. in IMX283 case it would,  5472x3648\n>>\n>>> In the lines below this function we have\n>>>\n>>> \t\tminResolution = sensorFormat.size;\n>>> \t\tmaxResolution = sensorFormat.size;\n>> From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648\n>>> cfg->size.boundTo(maxResolution);\n>>> cfg->size.expandTo(minResolution);\n>>>\n>>> So if ((maxResolution = minResolution) > cfg->size) will the calls to\n>>> boundTo() followed by expandTo() enlarge cfg->size ?\n>> so cfg->size is 5472x3648 here, which can't be supported.\n> Sorry, I'm confused. Aren't you confirming me that the resulting\n> cfg->size cannot be supported (with this patch I mean) ?\n\nSo user passes a relatively high resolution, supported by the sensor \n(but not with ISP)\n\n\tcam -c1 -srole=raw,width=5472,height=3648\n\nso cfg-size here is 5472x3648.\n\nSo without this patch, you would right now get:\n\n\tERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12\n\tFailed to configure camera\n\tFailed to start camera session\n\nWith this patch patched, you would get:\n\n\tINFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12\n\tcam0: Capture until user interrupts by SIGINT\n\t536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824\n....\n\nSo What has happened:\n\nUser has requested a raw stream with 5472x3648. It was found that this \ncan't be supported hence,\nthe camera configuration is adjusted to a (lower) but maximum supported \nresolution and delivers raw\nframes with 4096x3072.\n\nOne can argue that we should never adjust camera configuration to a \nlower size than cfg->size (i.e. what the user asked).\n\nBut then,\n\n* \\var CameraConfiguration::Adjusted\n* The configuration has been adjusted to a valid configuration\n\nand\n\n  * \\retval CameraConfigutation::Adjusted The configuration has been \nadjusted\n  * and is now valid. Parameters may have changed for any stream, and \nstream\n  * configurations may have been removed. The caller shall check the\n  * configuration carefully.\n\n\n>\n>> Have you noticed the failure case I mentioned in the commit message?\n> While the commmit message suggests this patch makes it valid ?\n>\n> What am I missing ? :)\n>\n>>> Wouldn't it be better to use 'resolution' in\n>>>\n>>> \t\tV4L2SubdeviceFormat sensorFormat =\n>>> \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n>>>\n>>> instead of cfg->size ?\n>> No, otherwise it will always pick the highest sensor output size.\n>>\n>> For e.g. I request -srole=raw,width=1920,height=1080\n>>\n>> \"cfg->size\" will pick 2736x1824 for IMX283\n>>\n>> \"resolution\" will always pick 4096x3072.\n>>> Thanks\n>>> j\n>>>\n>>>> --\n>>>> 2.45.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 499DFC3257\n\tfor ;\n\tFri, 4 Oct 2024 11:15:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0219E63523;\n\tFri, 4 Oct 2024 13:15: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 9AF3462C8F\n\tfor ;\n\tFri, 4 Oct 2024 13:15:08 +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 517B055;\n\tFri, 4 Oct 2024 13:13:32 +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=\"Dk/zFQlX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728040413;\n\tbh=WvTgjskgn72/mzPPcjOD5+v8XgDGZnu99tkroLeEkU0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Dk/zFQlXSmvbIss2rgcfWru/+WA7dSm5KpV+s1ukE/QpChVKHE13YN5d5EfyAVKKO\n\tGr7S4chUbwOH/Go3barZMqpTN1+1aI5RWrm/Nq888y8itoJ/TsRNS1P7tw7Lq8iTMu\n\tY227HeGMmQz5yn5b5j9ENOKzPbK3j1/rQgDFZQpk=","Message-ID":"<4e1e134e-97a8-48c0-bd6c-8056c6436247@ideasonboard.com>","Date":"Fri, 4 Oct 2024 16:45:01 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","References":"<20241004045338.18443-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"","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":31610,"web_url":"https://patchwork.libcamera.org/comment/31610/","msgid":"","date":"2024-10-08T07:48:30","subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Umang,\n seems like I'm failing to make my point across, sorry about this\n\nOn Fri, Oct 04, 2024 at 04:45:01PM GMT, Umang Jain wrote:\n> Hi Jacopo,\n>\n> On 04/10/24 3:58 pm, Jacopo Mondi wrote:\n> > Hi Umang\n> >\n> > On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote:\n> > > Hi Jacopo\n> > >\n> > > On 04/10/24 3:07 pm, Jacopo Mondi wrote:\n> > > > Hi Umang\n> > > >\n> > > > On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote:\n> > > > > Commit 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not\n> > > > > supported by the pipeline\") introduced a mechanism to determine maximum\n> > > > > supported sensor resolution and filter out resolutions that cannot be\n> > > > > supported by the ISP.\n> > > > >\n> > > > > However, it missed to update the raw stream configuration path, where\n> > > > > it should have clamped the raw stream configuration size to the maximum\n> > > > > sensor supported resolution.\n> > > > >\n> > > > > This patch fixes the above issue and can be confirmed with IMX283\n> > > > > on i.MX8MP:\n> > > > >\n> > > > > From:\n> > > > > ($) cam -c1 -srole=raw,width=5472,height=3072\n> > > > > INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12\n> > > > > ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12\n> > > > > Failed to configure camera\n> > > > > Failed to start camera session\n> > > > >\n> > > > > To:\n> > > > > ($) cam -c1 -srole=raw,width=5472,height=3072\n> > > > > INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12\n> > > > > cam0: Capture until user interrupts by SIGINT\n> > > > > 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824\n> > > > > 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824\n> > > > > 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824\n> > > > > ...\n> > > > >\n> > > > > Fixes: 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline\")\n> > > > > Signed-off-by: Umang Jain \n> > > > > ---\n> > > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-\n> > > > > 1 file changed, 3 insertions(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > > index da8d25c3..feb6d89f 100644\n> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > > @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> > > > > \tif (isRaw) {\n> > > > > \t\t/*\n> > > > > \t\t * Use the sensor output size closest to the requested stream\n> > > > > -\t\t * size.\n> > > > > +\t\t * size while ensuring the output size doesn't exceed ISP limits.\n> > > > > \t\t */\n> > > > > \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n> > > > > +\t\tcfg->size.boundTo(resolution);\n> > > > > +\n> > > > > \t\tV4L2SubdeviceFormat sensorFormat =\n> > > > > \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> > > > CameraSensor::getFormat() returns a sensor resolution large enough to\n> > > > accomodate the requested size, at least that's how I read the\n> > > > following part of the documentation\n> > > >\n> > > > * \\a size indicates the desired size at the output of the sensor. This function\n> > > > * selects the best media bus code and size supported by the sensor according\n> > > > * to the following criteria.\n> > > > *\n> > > > * - The desired \\a size shall fit in the sensor output size to avoid the need\n> > > > * to up-scale.\n> > > >\n> > > > And this part of the code\n> > > >\n> > > > if (sz.width < size.width || sz.height < size.height)\n> > > > continue;\n> > > >\n> > > > So, at least in my understanding \"sensorFormat\" could represent a size\n> > > > larger than cfg->size. Is this your understanding as well ?\n> > > \"sensorFormat\" could represent a size larger than cfg->size\n> > >\n> > > and\n> > >\n> > > could also represent a size large than max supported resolution (i.e.\n> > > 'resolution' variable in this case)\n> > >\n> > > For e.g. in IMX283 case it would,  5472x3648\n> > >\n> > > > In the lines below this function we have\n> > > >\n> > > > \t\tminResolution = sensorFormat.size;\n> > > > \t\tmaxResolution = sensorFormat.size;\n> > > From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648\n> > > > cfg->size.boundTo(maxResolution);\n> > > > cfg->size.expandTo(minResolution);\n> > > >\n> > > > So if ((maxResolution = minResolution) > cfg->size) will the calls to\n> > > > boundTo() followed by expandTo() enlarge cfg->size ?\n> > > so cfg->size is 5472x3648 here, which can't be supported.\n> > Sorry, I'm confused. Aren't you confirming me that the resulting\n> > cfg->size cannot be supported (with this patch I mean) ?\n>\n> So user passes a relatively high resolution, supported by the sensor (but\n> not with ISP)\n>\n> \tcam -c1 -srole=raw,width=5472,height=3648\n>\n> so cfg-size here is 5472x3648.\n>\n> So without this patch, you would right now get:\n>\n> \tERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12\n> \tFailed to configure camera\n> \tFailed to start camera session\n>\n> With this patch patched, you would get:\n>\n> \tINFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12\n> \tcam0: Capture until user interrupts by SIGINT\n> \t536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824\n> ....\n>\n> So What has happened:\n>\n> User has requested a raw stream with 5472x3648. It was found that this can't\n> be supported hence,\n> the camera configuration is adjusted to a (lower) but maximum supported\n> resolution and delivers raw\n> frames with 4096x3072.\n\nI understand this fixes your use case but my concern, as explained in\nhe previous email is that\n\n cfg->size.boundTo(resolution);\n\n\t\tV4L2SubdeviceFormat sensorFormat =\n\t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n\n\t\tminResolution = sensorFormat.size;\n\t\tmaxResolution = sensorFormat.size;\n\n\n\tcfg->size.boundTo(maxResolution);\n\tcfg->size.expandTo(minResolution);\n\nas CameraSensor::getFormat() can return a resolution -higher- than the\nrequested one (cfg->size) there is a risk that cfg->size is later\nexpanded to a larger value.\n\nNow, if I get this right \"resolution\" is returned by the newly introduced\nRkISP1Path::filterSensorResolution() and it's guaranteed to be the\n\"higher sensor's resolution supported by the ISP\".\n\nif that's the case, then CameraSensor::getFormat() will never return\na size larger than \"cfg->size.boundedTo(resolution)\" as \"resolution\"\nis known to be a size supported by the sensor itself.\n\nIf my understanding is correct, then please add my tag\nReviewed-by: Jacopo Mondi \n\n>\n> One can argue that we should never adjust camera configuration to a lower\n> size than cfg->size (i.e. what the user asked).\n>\n\nI don't think that's a problem\n\n> But then,\n>\n> * \\var CameraConfiguration::Adjusted\n> * The configuration has been adjusted to a valid configuration\n>\n> and\n>\n>  * \\retval CameraConfigutation::Adjusted The configuration has been adjusted\n>  * and is now valid. Parameters may have changed for any stream, and stream\n>  * configurations may have been removed. The caller shall check the\n>  * configuration carefully.\n>\n>\n> >\n> > > Have you noticed the failure case I mentioned in the commit message?\n> > While the commmit message suggests this patch makes it valid ?\n> >\n> > What am I missing ? :)\n> >\n> > > > Wouldn't it be better to use 'resolution' in\n> > > >\n> > > > \t\tV4L2SubdeviceFormat sensorFormat =\n> > > > \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> > > >\n> > > > instead of cfg->size ?\n> > > No, otherwise it will always pick the highest sensor output size.\n> > >\n> > > For e.g. I request -srole=raw,width=1920,height=1080\n> > >\n> > > \"cfg->size\" will pick 2736x1824 for IMX283\n> > >\n> > > \"resolution\" will always pick 4096x3072.\n> > > > Thanks\n> > > > j\n> > > >\n> > > > > --\n> > > > > 2.45.2\n> > > > >\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 05EE8BE080\n\tfor ;\n\tTue, 8 Oct 2024 07:48:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 02B216353A;\n\tTue, 8 Oct 2024 09:48:37 +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 9CD5F62C8E\n\tfor ;\n\tTue, 8 Oct 2024 09:48:35 +0200 (CEST)","from ideasonboard.com (unknown [5.179.165.198])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4266C514;\n\tTue, 8 Oct 2024 09:46:58 +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=\"KpGknV3P\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728373618;\n\tbh=6o5V1DC/a4+xZRqoAn7pA7gZUF0iTvy+9yiWlWg2btc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KpGknV3Pk9rUXZzd+fKWCeCj+OqUpScnzWB1S/Rgme1GBJX+UMqjfyjjlkMJ5lFLF\n\t/Ap7f671ol1h2yXTB2XEgtawYfUBA9VXhkGYTzbx0zbDAGD1tBASsBlo+0tF02A3X2\n\tHY2K++eQ4L812A+Dt3TVmK3159ZFy+0UpI60Wh48=","Date":"Tue, 8 Oct 2024 09:48:30 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","Message-ID":"","References":"<20241004045338.18443-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t<4e1e134e-97a8-48c0-bd6c-8056c6436247@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<4e1e134e-97a8-48c0-bd6c-8056c6436247@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":31628,"web_url":"https://patchwork.libcamera.org/comment/31628/","msgid":"<78f9bfed-f856-4d5d-820e-5f53b56a4c71@ideasonboard.com>","date":"2024-10-08T18:40:37","subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 08/10/24 1:18 pm, Jacopo Mondi wrote:\n> Umang,\n> seems like I'm failing to make my point across, sorry about this\n>\n> On Fri, Oct 04, 2024 at 04:45:01PM GMT, Umang Jain wrote:\n>> Hi Jacopo,\n>>\n>> On 04/10/24 3:58 pm, Jacopo Mondi wrote:\n>>> Hi Umang\n>>>\n>>> On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote:\n>>>> Hi Jacopo\n>>>>\n>>>> On 04/10/24 3:07 pm, Jacopo Mondi wrote:\n>>>>> Hi Umang\n>>>>>\n>>>>> On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote:\n>>>>>> Commit 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not\n>>>>>> supported by the pipeline\") introduced a mechanism to determine maximum\n>>>>>> supported sensor resolution and filter out resolutions that cannot be\n>>>>>> supported by the ISP.\n>>>>>>\n>>>>>> However, it missed to update the raw stream configuration path, where\n>>>>>> it should have clamped the raw stream configuration size to the maximum\n>>>>>> sensor supported resolution.\n>>>>>>\n>>>>>> This patch fixes the above issue and can be confirmed with IMX283\n>>>>>> on i.MX8MP:\n>>>>>>\n>>>>>> From:\n>>>>>> ($) cam -c1 -srole=raw,width=5472,height=3072\n>>>>>> INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12\n>>>>>> ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12\n>>>>>> Failed to configure camera\n>>>>>> Failed to start camera session\n>>>>>>\n>>>>>> To:\n>>>>>> ($) cam -c1 -srole=raw,width=5472,height=3072\n>>>>>> INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12\n>>>>>> cam0: Capture until user interrupts by SIGINT\n>>>>>> 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824\n>>>>>> 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824\n>>>>>> 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824\n>>>>>> ...\n>>>>>>\n>>>>>> Fixes: 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline\")\n>>>>>> Signed-off-by: Umang Jain \n>>>>>> ---\n>>>>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-\n>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>>>>>>\n>>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>>>>> index da8d25c3..feb6d89f 100644\n>>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>>>>> @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>>>>>> \tif (isRaw) {\n>>>>>> \t\t/*\n>>>>>> \t\t * Use the sensor output size closest to the requested stream\n>>>>>> -\t\t * size.\n>>>>>> +\t\t * size while ensuring the output size doesn't exceed ISP limits.\n>>>>>> \t\t */\n>>>>>> \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n>>>>>> +\t\tcfg->size.boundTo(resolution);\n>>>>>> +\n>>>>>> \t\tV4L2SubdeviceFormat sensorFormat =\n>>>>>> \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n>>>>> CameraSensor::getFormat() returns a sensor resolution large enough to\n>>>>> accomodate the requested size, at least that's how I read the\n>>>>> following part of the documentation\n>>>>>\n>>>>> * \\a size indicates the desired size at the output of the sensor. This function\n>>>>> * selects the best media bus code and size supported by the sensor according\n>>>>> * to the following criteria.\n>>>>> *\n>>>>> * - The desired \\a size shall fit in the sensor output size to avoid the need\n>>>>> * to up-scale.\n>>>>>\n>>>>> And this part of the code\n>>>>>\n>>>>> if (sz.width < size.width || sz.height < size.height)\n>>>>> continue;\n>>>>>\n>>>>> So, at least in my understanding \"sensorFormat\" could represent a size\n>>>>> larger than cfg->size. Is this your understanding as well ?\n>>>> \"sensorFormat\" could represent a size larger than cfg->size\n>>>>\n>>>> and\n>>>>\n>>>> could also represent a size large than max supported resolution (i.e.\n>>>> 'resolution' variable in this case)\n>>>>\n>>>> For e.g. in IMX283 case it would,  5472x3648\n>>>>\n>>>>> In the lines below this function we have\n>>>>>\n>>>>> \t\tminResolution = sensorFormat.size;\n>>>>> \t\tmaxResolution = sensorFormat.size;\n>>>> From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648\n>>>>> cfg->size.boundTo(maxResolution);\n>>>>> cfg->size.expandTo(minResolution);\n>>>>>\n>>>>> So if ((maxResolution = minResolution) > cfg->size) will the calls to\n>>>>> boundTo() followed by expandTo() enlarge cfg->size ?\n>>>> so cfg->size is 5472x3648 here, which can't be supported.\n>>> Sorry, I'm confused. Aren't you confirming me that the resulting\n>>> cfg->size cannot be supported (with this patch I mean) ?\n>> So user passes a relatively high resolution, supported by the sensor (but\n>> not with ISP)\n>>\n>> \tcam -c1 -srole=raw,width=5472,height=3648\n>>\n>> so cfg-size here is 5472x3648.\n>>\n>> So without this patch, you would right now get:\n>>\n>> \tERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12\n>> \tFailed to configure camera\n>> \tFailed to start camera session\n>>\n>> With this patch patched, you would get:\n>>\n>> \tINFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12\n>> \tcam0: Capture until user interrupts by SIGINT\n>> \t536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824\n>> ....\n>>\n>> So What has happened:\n>>\n>> User has requested a raw stream with 5472x3648. It was found that this can't\n>> be supported hence,\n>> the camera configuration is adjusted to a (lower) but maximum supported\n>> resolution and delivers raw\n>> frames with 4096x3072.\n> I understand this fixes your use case but my concern, as explained in\n> he previous email is that\n>\n> cfg->size.boundTo(resolution);\n>\n> \t\tV4L2SubdeviceFormat sensorFormat =\n> \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n>\n> \t\tminResolution = sensorFormat.size;\n> \t\tmaxResolution = sensorFormat.size;\n>\n>\n> \tcfg->size.boundTo(maxResolution);\n> \tcfg->size.expandTo(minResolution);\n>\n> as CameraSensor::getFormat() can return a resolution -higher- than the\n> requested one (cfg->size) there is a risk that cfg->size is later\n> expanded to a larger value.\n>\n> Now, if I get this right \"resolution\" is returned by the newly introduced\n> RkISP1Path::filterSensorResolution() and it's guaranteed to be the\n> \"higher sensor's resolution supported by the ISP\".\n\nyes, resolution is is the 'highest sensor's resolution supported by the \nISP'  hence, we never allow cfg->size to go beyond that.\n>\n> if that's the case, then CameraSensor::getFormat() will never return\n> a size larger than \"cfg->size.boundedTo(resolution)\" as \"resolution\"\n> is known to be a size supported by the sensor itself.\n\nIndeed, that's what's the intent of the patch.\n\n>\n> If my understanding is correct, then please add my tag\n> Reviewed-by: Jacopo Mondi \n\nYou're understanding is correct.\n\nDo you think this understand needs to be documented in form of comments ?\n\n>\n>> One can argue that we should never adjust camera configuration to a lower\n>> size than cfg->size (i.e. what the user asked).\n>>\n> I don't think that's a problem\n>\n>> But then,\n>>\n>> * \\var CameraConfiguration::Adjusted\n>> * The configuration has been adjusted to a valid configuration\n>>\n>> and\n>>\n>>  * \\retval CameraConfigutation::Adjusted The configuration has been adjusted\n>>  * and is now valid. Parameters may have changed for any stream, and stream\n>>  * configurations may have been removed. The caller shall check the\n>>  * configuration carefully.\n>>\n>>\n>>>> Have you noticed the failure case I mentioned in the commit message?\n>>> While the commmit message suggests this patch makes it valid ?\n>>>\n>>> What am I missing ? :)\n>>>\n>>>>> Wouldn't it be better to use 'resolution' in\n>>>>>\n>>>>> \t\tV4L2SubdeviceFormat sensorFormat =\n>>>>> \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n>>>>>\n>>>>> instead of cfg->size ?\n>>>> No, otherwise it will always pick the highest sensor output size.\n>>>>\n>>>> For e.g. I request -srole=raw,width=1920,height=1080\n>>>>\n>>>> \"cfg->size\" will pick 2736x1824 for IMX283\n>>>>\n>>>> \"resolution\" will always pick 4096x3072.\n>>>>> Thanks\n>>>>> j\n>>>>>\n>>>>>> --\n>>>>>> 2.45.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 60B12C32DE\n\tfor ;\n\tTue, 8 Oct 2024 18:40:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B9CE6536B;\n\tTue, 8 Oct 2024 20:40:42 +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 8C37B63525\n\tfor ;\n\tTue, 8 Oct 2024 20:40:41 +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 1D83EA47;\n\tTue, 8 Oct 2024 20:39:03 +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=\"Z6F46wyp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728412744;\n\tbh=MS9Ja4VR7IHEvp3LpXC6MFqY26wGjKvmNZ6S6OB7OsU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Z6F46wypUGNi/s3ldueLCcTqWseyg2WPgAlmb9fT28Ocs1U2h2/As1EbWWzGN3Yjo\n\tuv0ICbjHzDx0JHncNobdQC0Ll0Kei2s2M6YQNqa4DFmYGTCOMkk49yDfsdrAI0/Txs\n\tETNSrozIspv9jUvTtjERma7eGBMVX9j3WgSREOLo=","Message-ID":"<78f9bfed-f856-4d5d-820e-5f53b56a4c71@ideasonboard.com>","Date":"Wed, 9 Oct 2024 00:10:37 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","References":"<20241004045338.18443-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t<4e1e134e-97a8-48c0-bd6c-8056c6436247@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"","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":31630,"web_url":"https://patchwork.libcamera.org/comment/31630/","msgid":"","date":"2024-10-09T05:52:23","subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Umang\n\nOn Wed, Oct 09, 2024 at 12:10:37AM GMT, Umang Jain wrote:\n> Hi Jacopo,\n>\n> On 08/10/24 1:18 pm, Jacopo Mondi wrote:\n> > Umang,\n> > seems like I'm failing to make my point across, sorry about this\n> >\n> > On Fri, Oct 04, 2024 at 04:45:01PM GMT, Umang Jain wrote:\n> > > Hi Jacopo,\n> > >\n> > > On 04/10/24 3:58 pm, Jacopo Mondi wrote:\n> > > > Hi Umang\n> > > >\n> > > > On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote:\n> > > > > Hi Jacopo\n> > > > >\n> > > > > On 04/10/24 3:07 pm, Jacopo Mondi wrote:\n> > > > > > Hi Umang\n> > > > > >\n> > > > > > On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote:\n> > > > > > > Commit 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not\n> > > > > > > supported by the pipeline\") introduced a mechanism to determine maximum\n> > > > > > > supported sensor resolution and filter out resolutions that cannot be\n> > > > > > > supported by the ISP.\n> > > > > > >\n> > > > > > > However, it missed to update the raw stream configuration path, where\n> > > > > > > it should have clamped the raw stream configuration size to the maximum\n> > > > > > > sensor supported resolution.\n> > > > > > >\n> > > > > > > This patch fixes the above issue and can be confirmed with IMX283\n> > > > > > > on i.MX8MP:\n> > > > > > >\n> > > > > > > From:\n> > > > > > > ($) cam -c1 -srole=raw,width=5472,height=3072\n> > > > > > > INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12\n> > > > > > > ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12\n> > > > > > > Failed to configure camera\n> > > > > > > Failed to start camera session\n> > > > > > >\n> > > > > > > To:\n> > > > > > > ($) cam -c1 -srole=raw,width=5472,height=3072\n> > > > > > > INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12\n> > > > > > > cam0: Capture until user interrupts by SIGINT\n> > > > > > > 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824\n> > > > > > > 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824\n> > > > > > > 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824\n> > > > > > > ...\n> > > > > > >\n> > > > > > > Fixes: 761545407c76 (\"pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline\")\n> > > > > > > Signed-off-by: Umang Jain \n> > > > > > > ---\n> > > > > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-\n> > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > > > > index da8d25c3..feb6d89f 100644\n> > > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > > > > @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> > > > > > > \tif (isRaw) {\n> > > > > > > \t\t/*\n> > > > > > > \t\t * Use the sensor output size closest to the requested stream\n> > > > > > > -\t\t * size.\n> > > > > > > +\t\t * size while ensuring the output size doesn't exceed ISP limits.\n> > > > > > > \t\t */\n> > > > > > > \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n> > > > > > > +\t\tcfg->size.boundTo(resolution);\n> > > > > > > +\n> > > > > > > \t\tV4L2SubdeviceFormat sensorFormat =\n> > > > > > > \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> > > > > > CameraSensor::getFormat() returns a sensor resolution large enough to\n> > > > > > accomodate the requested size, at least that's how I read the\n> > > > > > following part of the documentation\n> > > > > >\n> > > > > > * \\a size indicates the desired size at the output of the sensor. This function\n> > > > > > * selects the best media bus code and size supported by the sensor according\n> > > > > > * to the following criteria.\n> > > > > > *\n> > > > > > * - The desired \\a size shall fit in the sensor output size to avoid the need\n> > > > > > * to up-scale.\n> > > > > >\n> > > > > > And this part of the code\n> > > > > >\n> > > > > > if (sz.width < size.width || sz.height < size.height)\n> > > > > > continue;\n> > > > > >\n> > > > > > So, at least in my understanding \"sensorFormat\" could represent a size\n> > > > > > larger than cfg->size. Is this your understanding as well ?\n> > > > > \"sensorFormat\" could represent a size larger than cfg->size\n> > > > >\n> > > > > and\n> > > > >\n> > > > > could also represent a size large than max supported resolution (i.e.\n> > > > > 'resolution' variable in this case)\n> > > > >\n> > > > > For e.g. in IMX283 case it would,  5472x3648\n> > > > >\n> > > > > > In the lines below this function we have\n> > > > > >\n> > > > > > \t\tminResolution = sensorFormat.size;\n> > > > > > \t\tmaxResolution = sensorFormat.size;\n> > > > > From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648\n> > > > > > cfg->size.boundTo(maxResolution);\n> > > > > > cfg->size.expandTo(minResolution);\n> > > > > >\n> > > > > > So if ((maxResolution = minResolution) > cfg->size) will the calls to\n> > > > > > boundTo() followed by expandTo() enlarge cfg->size ?\n> > > > > so cfg->size is 5472x3648 here, which can't be supported.\n> > > > Sorry, I'm confused. Aren't you confirming me that the resulting\n> > > > cfg->size cannot be supported (with this patch I mean) ?\n> > > So user passes a relatively high resolution, supported by the sensor (but\n> > > not with ISP)\n> > >\n> > > \tcam -c1 -srole=raw,width=5472,height=3648\n> > >\n> > > so cfg-size here is 5472x3648.\n> > >\n> > > So without this patch, you would right now get:\n> > >\n> > > \tERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12\n> > > \tFailed to configure camera\n> > > \tFailed to start camera session\n> > >\n> > > With this patch patched, you would get:\n> > >\n> > > \tINFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12\n> > > \tcam0: Capture until user interrupts by SIGINT\n> > > \t536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824\n> > > ....\n> > >\n> > > So What has happened:\n> > >\n> > > User has requested a raw stream with 5472x3648. It was found that this can't\n> > > be supported hence,\n> > > the camera configuration is adjusted to a (lower) but maximum supported\n> > > resolution and delivers raw\n> > > frames with 4096x3072.\n> > I understand this fixes your use case but my concern, as explained in\n> > he previous email is that\n> >\n> > cfg->size.boundTo(resolution);\n> >\n> > \t\tV4L2SubdeviceFormat sensorFormat =\n> > \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> >\n> > \t\tminResolution = sensorFormat.size;\n> > \t\tmaxResolution = sensorFormat.size;\n> >\n> >\n> > \tcfg->size.boundTo(maxResolution);\n> > \tcfg->size.expandTo(minResolution);\n> >\n> > as CameraSensor::getFormat() can return a resolution -higher- than the\n> > requested one (cfg->size) there is a risk that cfg->size is later\n> > expanded to a larger value.\n> >\n> > Now, if I get this right \"resolution\" is returned by the newly introduced\n> > RkISP1Path::filterSensorResolution() and it's guaranteed to be the\n> > \"higher sensor's resolution supported by the ISP\".\n>\n> yes, resolution is is the 'highest sensor's resolution supported by the\n> ISP'  hence, we never allow cfg->size to go beyond that.\n> >\n> > if that's the case, then CameraSensor::getFormat() will never return\n> > a size larger than \"cfg->size.boundedTo(resolution)\" as \"resolution\"\n> > is known to be a size supported by the sensor itself.\n>\n> Indeed, that's what's the intent of the patch.\n>\n> >\n> > If my understanding is correct, then please add my tag\n> > Reviewed-by: Jacopo Mondi \n>\n> You're understanding is correct.\n>\n> Do you think this understand needs to be documented in form of comments ?\n>\n\nMaybe yes.. Just a small note along the lines of\n\n\n- * size.\n+ * size while ensuring the output size doesn't exceed ISP limits.\n+ *\n+ * As 'resolution' is the largest sensor resolution\n+ * supported by the ISP, CameraSensor::getFormat() will never\n+ * return a V4L2SubdeviceFormat with a larger size.\n */\n\nOr whatever you like the most.\n\nThanks\n j\n\n> >\n> > > One can argue that we should never adjust camera configuration to a lower\n> > > size than cfg->size (i.e. what the user asked).\n> > >\n> > I don't think that's a problem\n> >\n> > > But then,\n> > >\n> > > * \\var CameraConfiguration::Adjusted\n> > > * The configuration has been adjusted to a valid configuration\n> > >\n> > > and\n> > >\n> > >  * \\retval CameraConfigutation::Adjusted The configuration has been adjusted\n> > >  * and is now valid. Parameters may have changed for any stream, and stream\n> > >  * configurations may have been removed. The caller shall check the\n> > >  * configuration carefully.\n> > >\n> > >\n> > > > > Have you noticed the failure case I mentioned in the commit message?\n> > > > While the commmit message suggests this patch makes it valid ?\n> > > >\n> > > > What am I missing ? :)\n> > > >\n> > > > > > Wouldn't it be better to use 'resolution' in\n> > > > > >\n> > > > > > \t\tV4L2SubdeviceFormat sensorFormat =\n> > > > > > \t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> > > > > >\n> > > > > > instead of cfg->size ?\n> > > > > No, otherwise it will always pick the highest sensor output size.\n> > > > >\n> > > > > For e.g. I request -srole=raw,width=1920,height=1080\n> > > > >\n> > > > > \"cfg->size\" will pick 2736x1824 for IMX283\n> > > > >\n> > > > > \"resolution\" will always pick 4096x3072.\n> > > > > > Thanks\n> > > > > > j\n> > > > > >\n> > > > > > > --\n> > > > > > > 2.45.2\n> > > > > > >\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 EF0B1BE080\n\tfor ;\n\tWed, 9 Oct 2024 05:53:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C24BB6536B;\n\tWed, 9 Oct 2024 07:53:31 +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 B6BF5618C4\n\tfor ;\n\tWed, 9 Oct 2024 07:53:29 +0200 (CEST)","from ideasonboard.com (unknown [95.131.47.214])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A345E82A;\n\tWed, 9 Oct 2024 07:51:40 +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=\"WrhOycPD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728453112;\n\tbh=r9DvElgAmPHSbbU9bnIjDYPvF0P8SsNZRxopKFf+pnM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WrhOycPDYmHeelAjnb2vAlAYGkQjx04Oef7zcwa4Gq3YpqPb3njw6SzYvg3IbW6e/\n\trlQDnzPhpxxPSV86TOqluh5AHGkudZviB5cfI4iqcpz+rwzZyChWVsPBGwJOcdwmZl\n\tGs08cwvSNZfB3VBtD2b/pBmuotFewUbAJ+mnhrFA=","Date":"Wed, 9 Oct 2024 07:52:23 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH] libcamera: rkisp1: Clamp stream configuration to ISP\n\tlimit on raw path","Message-ID":"","References":"<20241004045338.18443-1-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t<4e1e134e-97a8-48c0-bd6c-8056c6436247@ideasonboard.com>\n\t\n\t<78f9bfed-f856-4d5d-820e-5f53b56a4c71@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<78f9bfed-f856-4d5d-820e-5f53b56a4c71@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\" "}}]