[{"id":31579,"web_url":"https://patchwork.libcamera.org/comment/31579/","msgid":"<172799191154.1375180.14879682830062875945@ping.linuxembedded.co.uk>","date":"2024-10-03T21:45:11","subject":"Re: [PATCH v2 1/2] libcamera rkisp1: Reduce scope of sensor max\n\tresolution variable","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2024-10-03 14:57:39)\n> Reduce the scope of sensor's max resolution variable in\n> RkISP1Path::validate(), as it is only required to constraint the\n> maximum and minimum resolution bounds on the non-RAW code path.\n> \n> No functional changes intended in this patch.\n> \n> Signed-off-by: Umang Jain \n\n\nReviewed-by: Kieran Bingham \n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 3 ++-\n> 1 file changed, 2 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 cb6117b9..e7f1f12a 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -254,7 +254,6 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> StreamConfiguration *cfg)\n> {\n> const std::vector &mbusCodes = sensor->mbusCodes();\n> - Size resolution = filterSensorResolution(sensor);\n> \n> const StreamConfiguration reqCfg = *cfg;\n> CameraConfiguration::Status status = CameraConfiguration::Valid;\n> @@ -322,6 +321,8 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n\nBut in here - I do wonder if this is another location that should be\nrestricted to the limits of the ISP.\n\nBringing in surrounding context:\n\n if (isRaw) {\n /*\n * Use the sensor output size closest to the requested stream\n * size.\n */\n uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n V4L2SubdeviceFormat sensorFormat =\n sensor->getFormat({ mbusCode }, cfg->size);\n\n\nI wonder if that section in (isRaw) also needs to be clamped to the\nfilteredSensorResolution, or should only pick the closest fit from the\nlist of modes available in the list that got filtered...\n\nSo I think moving this line has only highlighted to me that we've missed\na bit.\n\nNo objection to still merging this anyway - but I think something might\nneed to be adapted in here.\n\n\n\n> minResolution = sensorFormat.size;\n> maxResolution = sensorFormat.size;\n> } else {\n> + Size resolution = filterSensorResolution(sensor);\n> +\n> /*\n> * Adjust the size based on the sensor resolution and absolute\n> * limits of the ISP.\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 6CECAC3257\n\tfor ;\n\tThu, 3 Oct 2024 21:45:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 649866351F;\n\tThu, 3 Oct 2024 23:45:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DAD6760534\n\tfor ;\n\tThu, 3 Oct 2024 23:45:13 +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 545BA66F;\n\tThu, 3 Oct 2024 23:43: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=\"n6qqSnIH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727991820;\n\tbh=rpNnMHsNdqBXV0tnXcQr450SJy7DoesoI96K+qBpolE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=n6qqSnIHvX50fyeqWlHDXu0dpSgFBOsRSgX75y3CRJC5A1PH/tdQAUvLII4EcmtBq\n\tSTb55OcKWEymM8HVl3c95XL/o0wXeiXyrhGoecNdjoWPt/aUKz8VrD5C9n1UawLDs0\n\tdvd2/BiWmOWMa8YYMXCT60GGvx7+vsD6+V7WrlW0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241003135741.60869-2-umang.jain@ideasonboard.com>","References":"<20241003135741.60869-1-umang.jain@ideasonboard.com>\n\t<20241003135741.60869-2-umang.jain@ideasonboard.com>","Subject":"Re: [PATCH v2 1/2] libcamera rkisp1: Reduce scope of sensor max\n\tresolution variable","From":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tLaurent Pinchart ,\n\tPaul Elder ,\n\tUmang Jain ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 03 Oct 2024 22:45:11 +0100","Message-ID":"<172799191154.1375180.14879682830062875945@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":31581,"web_url":"https://patchwork.libcamera.org/comment/31581/","msgid":"<3b5a8a96-33bc-4f11-88a0-198cc1f38dd4@ideasonboard.com>","date":"2024-10-04T04:56:20","subject":"Re: [PATCH v2 1/2] libcamera rkisp1: Reduce scope of sensor max\n\tresolution variable","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran\n\nOn 04/10/24 3:15 am, Kieran Bingham wrote:\n> Quoting Umang Jain (2024-10-03 14:57:39)\n>> Reduce the scope of sensor's max resolution variable in\n>> RkISP1Path::validate(), as it is only required to constraint the\n>> maximum and minimum resolution bounds on the non-RAW code path.\n>>\n>> No functional changes intended in this patch.\n>>\n>> Signed-off-by: Umang Jain \n>\n> Reviewed-by: Kieran Bingham \n>\n>> ---\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 3 ++-\n>> 1 file changed, 2 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 cb6117b9..e7f1f12a 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> @@ -254,7 +254,6 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>> StreamConfiguration *cfg)\n>> {\n>> const std::vector &mbusCodes = sensor->mbusCodes();\n>> - Size resolution = filterSensorResolution(sensor);\n>> \n>> const StreamConfiguration reqCfg = *cfg;\n>> CameraConfiguration::Status status = CameraConfiguration::Valid;\n>> @@ -322,6 +321,8 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> But in here - I do wonder if this is another location that should be\n> restricted to the limits of the ISP.\n>\n> Bringing in surrounding context:\n>\n> if (isRaw) {\n> /*\n> * Use the sensor output size closest to the requested stream\n> * size.\n> */\n> uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n> V4L2SubdeviceFormat sensorFormat =\n> sensor->getFormat({ mbusCode }, cfg->size);\n>\n>\n> I wonder if that section in (isRaw) also needs to be clamped to the\n> filteredSensorResolution, or should only pick the closest fit from the\n> list of modes available in the list that got filtered...\n>\n> So I think moving this line has only highlighted to me that we've missed\n> a bit.\n\nWe have and now there is a fix:\n\n[PATCH] libcamera: rkisp1: Clamp stream configuration to ISP limit on \nraw path\n\nWe missed the clamping because it didn't existed in the first place \nwhen  I wrote\n\nhttps://git.libcamera.org/libcamera/libcamera.git/commit/?id=761545407c7666063f4c98d92a17af2a2c790b08\n\nBut I should have noticed :-/\n\n>\n> No objection to still merging this anyway - but I think something might\n> need to be adapted in here.\n>\n>\n>\n>> minResolution = sensorFormat.size;\n>> maxResolution = sensorFormat.size;\n>> } else {\n>> + Size resolution = filterSensorResolution(sensor);\n>> +\n>> /*\n>> * Adjust the size based on the sensor resolution and absolute\n>> * limits of the ISP.\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 4503EBD80A\n\tfor ;\n\tFri, 4 Oct 2024 04:56:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E25AC63526;\n\tFri, 4 Oct 2024 06:56:26 +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 838B662C8E\n\tfor ;\n\tFri, 4 Oct 2024 06:56:24 +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 1D58F18D;\n\tFri, 4 Oct 2024 06:54:49 +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=\"Pxubv6sX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728017690;\n\tbh=UXcpthr9Tk+8oNPZddoZHyFFqb93RCbYOWeNUrV3vr0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Pxubv6sXbY1ltacc0EPjKsEboufEUNJVV2mWIBA2zEneKgTNPCRyy1cXyXRZS3FMk\n\tbd5wOuPMEenxagwJRaepAfEn6YPLuHW2X2rsesaUqCqpvyWr8Oc1lTO3cRJ0YWpXT/\n\tYaUuI7aY/Sbl9jfGbCca6zotvJ1oKagWUPl5h8f8=","Message-ID":"<3b5a8a96-33bc-4f11-88a0-198cc1f38dd4@ideasonboard.com>","Date":"Fri, 4 Oct 2024 10:26:20 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 1/2] libcamera rkisp1: Reduce scope of sensor max\n\tresolution variable","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Jacopo Mondi ,\n\tLaurent Pinchart ,\n\tPaul Elder ","References":"<20241003135741.60869-1-umang.jain@ideasonboard.com>\n\t<20241003135741.60869-2-umang.jain@ideasonboard.com>\n\t<172799191154.1375180.14879682830062875945@ping.linuxembedded.co.uk>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<172799191154.1375180.14879682830062875945@ping.linuxembedded.co.uk>","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\" "}}]