[{"id":31616,"web_url":"https://patchwork.libcamera.org/comment/31616/","msgid":"","date":"2024-10-08T10:58:05","subject":"Re: [PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Umang\n\nOn Tue, Oct 08, 2024 at 03:49:18PM GMT, Umang Jain wrote:\n> Integrate the RkISP1 pipeline handler to support sensor configuration\n> provided by applications through CameraConfiguration::sensorConfig.\n>\n> The SensorConfiguration must be validated on both RkISP1Path (mainPath\n> and selfPath), so the parameters of RkISP1Path::validate() have been\n> updated to include sensorConfig.\n>\n> The camera configuration will be marked as invalid when the sensor\n> configuration is supplied, if:\n> - Invalid sensor configuration (SensorConfiguration::isValid())\n> - Bit depth not supported by RkISP1 pipeline\n> - RAW stream configuration size doesn't matches sensor configuration\n> output size\n\nThis is not the case anymore :)\n\n> - Sensor configuration output size is larger than maximum supported\n> sensor configuration on RkISP1 pipeline\n\ns/sensor configuration/sensor input/\n\n> - No matching sensor configuration output size supplied by the sensor\n>\n> Signed-off-by: Umang Jain \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 +++++++++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 +++++++++++++++++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +\n> 3 files changed, 80 insertions(+), 12 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index c02c7cf3..42961c12 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -447,11 +447,12 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> \tStreamConfiguration config;\n>\n> \tconfig = cfg;\n> -\tif (data_->mainPath_->validate(sensor, &config) != Valid)\n> +\tif (data_->mainPath_->validate(sensor, sensorConfig, &config) != Valid)\n> \t\treturn false;\n>\n> \tconfig = cfg;\n> -\tif (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid)\n> +\tif (data_->selfPath_ &&\n> +\t data_->selfPath_->validate(sensor, sensorConfig, &config) != Valid)\n> \t\treturn false;\n>\n> \treturn true;\n> @@ -468,6 +469,27 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>\n> \tstatus = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n>\n> +\t/*\n> +\t * Make sure that if a sensor configuration has been requested it\n> +\t * is valid.\n> +\t */\n> +\tif (sensorConfig) {\n> +\t\tif (!sensorConfig->isValid()) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Invalid sensor configuration request\";\n> +\n> +\t\t\treturn Invalid;\n> +\t\t}\n> +\n\nShould we add a comment to say RkISP1 only support 8, 10 and 12 bit\ndepths ?\n\nThe rest now looks good and the above comments can be applied when\nmerging the patch\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\t\tunsigned int bitDepth = sensorConfig->bitDepth;\n> +\t\tif (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Invalid sensor configuration bit depth\";\n> +\n> +\t\t\treturn Invalid;\n> +\t\t}\n> +\t}\n> +\n> \t/* Cap the number of entries to the available streams. */\n> \tif (config_.size() > pathCount) {\n> \t\tconfig_.resize(pathCount);\n> @@ -514,7 +536,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t/* Try to match stream without adjusting configuration. */\n> \t\tif (mainPathAvailable) {\n> \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (data_->mainPath_->validate(sensor, &tryCfg) == Valid) {\n> +\t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n> \t\t\t\tmainPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n> @@ -524,7 +546,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>\n> \t\tif (selfPathAvailable) {\n> \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (data_->selfPath_->validate(sensor, &tryCfg) == Valid) {\n> +\t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n> \t\t\t\tselfPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n> @@ -535,7 +557,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t/* Try to match stream allowing adjusting configuration. */\n> \t\tif (mainPathAvailable) {\n> \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) {\n> +\t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n> \t\t\t\tmainPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n> @@ -546,7 +568,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>\n> \t\tif (selfPathAvailable) {\n> \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) {\n> +\t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n> \t\t\t\tselfPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n> @@ -723,7 +745,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tV4L2SubdeviceFormat format = config->sensorFormat();\n> \tLOG(RkISP1, Debug) << \"Configuring sensor with \" << format;\n>\n> -\tret = sensor->setFormat(&format, config->combinedTransform());\n> +\tif (config->sensorConfig)\n> +\t\tret = sensor->applyConfiguration(*config->sensorConfig,\n> +\t\t\t\t\t\t config->combinedTransform(),\n> +\t\t\t\t\t\t &format);\n> +\telse\n> +\t\tret = sensor->setFormat(&format, config->combinedTransform());\n> +\n> \tif (ret < 0)\n> \t\treturn ret;\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index da8d25c3..3b5bea96 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -251,8 +251,10 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> \treturn cfg;\n> }\n>\n> -CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> -\t\t\t\t\t\t StreamConfiguration *cfg)\n> +CameraConfiguration::Status\n> +RkISP1Path::validate(const CameraSensor *sensor,\n> +\t\t std::optional &sensorConfig,\n> +\t\t StreamConfiguration *cfg)\n> {\n> \tconst std::vector &mbusCodes = sensor->mbusCodes();\n> \tSize resolution = filterSensorResolution(sensor);\n> @@ -282,9 +284,14 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t\t\tcontinue;\n>\n> \t\t\t/*\n> -\t\t\t * Store the raw format with the highest bits per pixel\n> -\t\t\t * for later usage.\n> +\t\t\t * If the bits per pixel is supplied from the sensor\n> +\t\t\t * configuration, choose a raw format that complies with\n> +\t\t\t * it. Otherwise, store the raw format with the highest\n> +\t\t\t * bits per pixel for later usage.\n> \t\t\t */\n> +\t\t\tif (sensorConfig && info.bitsPerPixel != sensorConfig->bitDepth)\n> +\t\t\t\tcontinue;\n> +\n> \t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> \t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> \t\t\t\trawFormat = format;\n> @@ -297,6 +304,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t}\n> \t}\n>\n> +\tif (sensorConfig && !found)\n> +\t\treturn CameraConfiguration::Invalid;\n> +\n> \tbool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==\n> \t\t PixelFormatInfo::ColourEncodingRAW;\n>\n> @@ -319,11 +329,39 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t * size.\n> \t\t */\n> \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n> +\t\tSize rawSize = sensorConfig ? sensorConfig->outputSize\n> +\t\t\t\t\t : cfg->size;\n> +\n> \t\tV4L2SubdeviceFormat sensorFormat =\n> -\t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> +\t\t\tsensor->getFormat({ mbusCode }, rawSize);\n> +\n> +\t\tif (sensorConfig &&\n> +\t\t sensorConfig->outputSize != sensorFormat.size)\n> +\t\t\treturn CameraConfiguration::Invalid;\n>\n> \t\tminResolution = sensorFormat.size;\n> \t\tmaxResolution = sensorFormat.size;\n> +\t} else if (sensorConfig) {\n> +\t\t/*\n> +\t\t * We have already ensured 'rawFormat' has the matching bit\n> +\t\t * depth with sensorConfig.bitDepth hence, only validate the\n> +\t\t * sensorConfig's output size here.\n> +\t\t */\n> +\t\tSize sensorSize = sensorConfig->outputSize;\n> +\n> +\t\tif (sensorSize > resolution)\n> +\t\t\treturn CameraConfiguration::Invalid;\n> +\n> +\t\tuint32_t mbusCode = formatToMediaBus.at(rawFormat);\n> +\t\tV4L2SubdeviceFormat sensorFormat =\n> +\t\t\tsensor->getFormat({ mbusCode }, sensorSize);\n> +\n> +\t\tif (sensorFormat.size != sensorSize)\n> +\t\t\treturn CameraConfiguration::Invalid;\n> +\n> +\t\tminResolution = minResolution_.expandedToAspectRatio(sensorSize);\n> +\t\tmaxResolution = maxResolution_.boundedTo(sensorSize)\n> +\t\t\t\t\t .boundedToAspectRatio(sensorSize);\n> \t} else {\n> \t\t/*\n> \t\t * Adjust the size based on the sensor resolution and absolute\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index 777fcae7..ce9a5666 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -27,6 +27,7 @@ namespace libcamera {\n> class CameraSensor;\n> class MediaDevice;\n> class V4L2Subdevice;\n> +class SensorConfiguration;\n> struct StreamConfiguration;\n> struct V4L2SubdeviceFormat;\n>\n> @@ -44,6 +45,7 @@ public:\n> \t\t\t\t\t\t const Size &resolution,\n> \t\t\t\t\t\t StreamRole role);\n> \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n> +\t\t\t\t\t std::optional &sensorConfig,\n> \t\t\t\t\t StreamConfiguration *cfg);\n>\n> \tint configure(const StreamConfiguration &config,\n> --\n> 2.45.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E2ADEBD80A\n\tfor ;\n\tTue, 8 Oct 2024 10:58:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A64B7618C9;\n\tTue, 8 Oct 2024 12:58:12 +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 3C126618C9\n\tfor ;\n\tTue, 8 Oct 2024 12:58:10 +0200 (CEST)","from ideasonboard.com (unknown [5.77.124.211])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6F1243D6;\n\tTue, 8 Oct 2024 12:56:33 +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=\"CSFkuL9g\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728384993;\n\tbh=FrEQKv+QEDdy4Qj+0KeUo4RH9rDO+2ljLOiZhC/YrhA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CSFkuL9gwKxGpeGugoNPKx67+ww+P6bfxADAUQW84RtvvVD5/9Sqrn0DAvyoYLNFT\n\tJkquvaWTixxI0WOySMLSIaHFOvaKpDa65f2GUreGndXZRY9+ZyxoZrn7XdBdhvE97y\n\t+Ecnpti1RPwofCZEudq6fb8zVB+mm64e25EXIaoQ=","Date":"Tue, 8 Oct 2024 12:58:05 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ,\n\tKieran Bingham , \n\tLaurent Pinchart ,\n\tPaul Elder ","Subject":"Re: [PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","Message-ID":"","References":"<20241008101918.10414-1-umang.jain@ideasonboard.com>\n\t<20241008101918.10414-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241008101918.10414-2-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":31618,"web_url":"https://patchwork.libcamera.org/comment/31618/","msgid":"<6cc9c2e7-ec68-445b-955c-7c134bb230dd@ideasonboard.com>","date":"2024-10-08T12:17:31","subject":"Re: [PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","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 4:28 pm, Jacopo Mondi wrote:\n> Hi Umang\n>\n> On Tue, Oct 08, 2024 at 03:49:18PM GMT, Umang Jain wrote:\n>> Integrate the RkISP1 pipeline handler to support sensor configuration\n>> provided by applications through CameraConfiguration::sensorConfig.\n>>\n>> The SensorConfiguration must be validated on both RkISP1Path (mainPath\n>> and selfPath), so the parameters of RkISP1Path::validate() have been\n>> updated to include sensorConfig.\n>>\n>> The camera configuration will be marked as invalid when the sensor\n>> configuration is supplied, if:\n>> - Invalid sensor configuration (SensorConfiguration::isValid())\n>> - Bit depth not supported by RkISP1 pipeline\n>> - RAW stream configuration size doesn't matches sensor configuration\n>> output size\n> This is not the case anymore :)\n\nAh yes, indeed\n>\n>> - Sensor configuration output size is larger than maximum supported\n>> sensor configuration on RkISP1 pipeline\n> s/sensor configuration/sensor input/\n>\n>> - No matching sensor configuration output size supplied by the sensor\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 +++++++++++++---\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 +++++++++++++++++--\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +\n>> 3 files changed, 80 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index c02c7cf3..42961c12 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -447,11 +447,12 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n>> \tStreamConfiguration config;\n>>\n>> \tconfig = cfg;\n>> -\tif (data_->mainPath_->validate(sensor, &config) != Valid)\n>> +\tif (data_->mainPath_->validate(sensor, sensorConfig, &config) != Valid)\n>> \t\treturn false;\n>>\n>> \tconfig = cfg;\n>> -\tif (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid)\n>> +\tif (data_->selfPath_ &&\n>> +\t data_->selfPath_->validate(sensor, sensorConfig, &config) != Valid)\n>> \t\treturn false;\n>>\n>> \treturn true;\n>> @@ -468,6 +469,27 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>\n>> \tstatus = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n>>\n>> +\t/*\n>> +\t * Make sure that if a sensor configuration has been requested it\n>> +\t * is valid.\n>> +\t */\n>> +\tif (sensorConfig) {\n>> +\t\tif (!sensorConfig->isValid()) {\n>> +\t\t\tLOG(RkISP1, Error)\n>> +\t\t\t\t<< \"Invalid sensor configuration request\";\n>> +\n>> +\t\t\treturn Invalid;\n>> +\t\t}\n>> +\n> Should we add a comment to say RkISP1 only support 8, 10 and 12 bit\n> depths ?\n\nWe can, but I think it is clear from the code itself ? :D\n\n>\n> The rest now looks good and the above comments can be applied when\n> merging the patch\n>\n> Reviewed-by: Jacopo Mondi \n\nThanks for the reviews!\n>\n> Thanks\n> j\n>\n>> +\t\tunsigned int bitDepth = sensorConfig->bitDepth;\n>> +\t\tif (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {\n>> +\t\t\tLOG(RkISP1, Error)\n>> +\t\t\t\t<< \"Invalid sensor configuration bit depth\";\n>> +\n>> +\t\t\treturn Invalid;\n>> +\t\t}\n>> +\t}\n>> +\n>> \t/* Cap the number of entries to the available streams. */\n>> \tif (config_.size() > pathCount) {\n>> \t\tconfig_.resize(pathCount);\n>> @@ -514,7 +536,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>> \t\t/* Try to match stream without adjusting configuration. */\n>> \t\tif (mainPathAvailable) {\n>> \t\t\tStreamConfiguration tryCfg = cfg;\n>> -\t\t\tif (data_->mainPath_->validate(sensor, &tryCfg) == Valid) {\n>> +\t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n>> \t\t\t\tmainPathAvailable = false;\n>> \t\t\t\tcfg = tryCfg;\n>> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n>> @@ -524,7 +546,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>\n>> \t\tif (selfPathAvailable) {\n>> \t\t\tStreamConfiguration tryCfg = cfg;\n>> -\t\t\tif (data_->selfPath_->validate(sensor, &tryCfg) == Valid) {\n>> +\t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n>> \t\t\t\tselfPathAvailable = false;\n>> \t\t\t\tcfg = tryCfg;\n>> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n>> @@ -535,7 +557,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>> \t\t/* Try to match stream allowing adjusting configuration. */\n>> \t\tif (mainPathAvailable) {\n>> \t\t\tStreamConfiguration tryCfg = cfg;\n>> -\t\t\tif (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) {\n>> +\t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n>> \t\t\t\tmainPathAvailable = false;\n>> \t\t\t\tcfg = tryCfg;\n>> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n>> @@ -546,7 +568,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>\n>> \t\tif (selfPathAvailable) {\n>> \t\t\tStreamConfiguration tryCfg = cfg;\n>> -\t\t\tif (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) {\n>> +\t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n>> \t\t\t\tselfPathAvailable = false;\n>> \t\t\t\tcfg = tryCfg;\n>> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n>> @@ -723,7 +745,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>> \tV4L2SubdeviceFormat format = config->sensorFormat();\n>> \tLOG(RkISP1, Debug) << \"Configuring sensor with \" << format;\n>>\n>> -\tret = sensor->setFormat(&format, config->combinedTransform());\n>> +\tif (config->sensorConfig)\n>> +\t\tret = sensor->applyConfiguration(*config->sensorConfig,\n>> +\t\t\t\t\t\t config->combinedTransform(),\n>> +\t\t\t\t\t\t &format);\n>> +\telse\n>> +\t\tret = sensor->setFormat(&format, config->combinedTransform());\n>> +\n>> \tif (ret < 0)\n>> \t\treturn ret;\n>>\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> index da8d25c3..3b5bea96 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> @@ -251,8 +251,10 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n>> \treturn cfg;\n>> }\n>>\n>> -CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>> -\t\t\t\t\t\t StreamConfiguration *cfg)\n>> +CameraConfiguration::Status\n>> +RkISP1Path::validate(const CameraSensor *sensor,\n>> +\t\t std::optional &sensorConfig,\n>> +\t\t StreamConfiguration *cfg)\n>> {\n>> \tconst std::vector &mbusCodes = sensor->mbusCodes();\n>> \tSize resolution = filterSensorResolution(sensor);\n>> @@ -282,9 +284,14 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>> \t\t\t\tcontinue;\n>>\n>> \t\t\t/*\n>> -\t\t\t * Store the raw format with the highest bits per pixel\n>> -\t\t\t * for later usage.\n>> +\t\t\t * If the bits per pixel is supplied from the sensor\n>> +\t\t\t * configuration, choose a raw format that complies with\n>> +\t\t\t * it. Otherwise, store the raw format with the highest\n>> +\t\t\t * bits per pixel for later usage.\n>> \t\t\t */\n>> +\t\t\tif (sensorConfig && info.bitsPerPixel != sensorConfig->bitDepth)\n>> +\t\t\t\tcontinue;\n>> +\n>> \t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n>> \t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n>> \t\t\t\trawFormat = format;\n>> @@ -297,6 +304,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>> \t\t}\n>> \t}\n>>\n>> +\tif (sensorConfig && !found)\n>> +\t\treturn CameraConfiguration::Invalid;\n>> +\n>> \tbool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==\n>> \t\t PixelFormatInfo::ColourEncodingRAW;\n>>\n>> @@ -319,11 +329,39 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>> \t\t * size.\n>> \t\t */\n>> \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n>> +\t\tSize rawSize = sensorConfig ? sensorConfig->outputSize\n>> +\t\t\t\t\t : cfg->size;\n>> +\n>> \t\tV4L2SubdeviceFormat sensorFormat =\n>> -\t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n>> +\t\t\tsensor->getFormat({ mbusCode }, rawSize);\n>> +\n>> +\t\tif (sensorConfig &&\n>> +\t\t sensorConfig->outputSize != sensorFormat.size)\n>> +\t\t\treturn CameraConfiguration::Invalid;\n>>\n>> \t\tminResolution = sensorFormat.size;\n>> \t\tmaxResolution = sensorFormat.size;\n>> +\t} else if (sensorConfig) {\n>> +\t\t/*\n>> +\t\t * We have already ensured 'rawFormat' has the matching bit\n>> +\t\t * depth with sensorConfig.bitDepth hence, only validate the\n>> +\t\t * sensorConfig's output size here.\n>> +\t\t */\n>> +\t\tSize sensorSize = sensorConfig->outputSize;\n>> +\n>> +\t\tif (sensorSize > resolution)\n>> +\t\t\treturn CameraConfiguration::Invalid;\n>> +\n>> +\t\tuint32_t mbusCode = formatToMediaBus.at(rawFormat);\n>> +\t\tV4L2SubdeviceFormat sensorFormat =\n>> +\t\t\tsensor->getFormat({ mbusCode }, sensorSize);\n>> +\n>> +\t\tif (sensorFormat.size != sensorSize)\n>> +\t\t\treturn CameraConfiguration::Invalid;\n>> +\n>> +\t\tminResolution = minResolution_.expandedToAspectRatio(sensorSize);\n>> +\t\tmaxResolution = maxResolution_.boundedTo(sensorSize)\n>> +\t\t\t\t\t .boundedToAspectRatio(sensorSize);\n>> \t} else {\n>> \t\t/*\n>> \t\t * Adjust the size based on the sensor resolution and absolute\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>> index 777fcae7..ce9a5666 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>> @@ -27,6 +27,7 @@ namespace libcamera {\n>> class CameraSensor;\n>> class MediaDevice;\n>> class V4L2Subdevice;\n>> +class SensorConfiguration;\n>> struct StreamConfiguration;\n>> struct V4L2SubdeviceFormat;\n>>\n>> @@ -44,6 +45,7 @@ public:\n>> \t\t\t\t\t\t const Size &resolution,\n>> \t\t\t\t\t\t StreamRole role);\n>> \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n>> +\t\t\t\t\t std::optional &sensorConfig,\n>> \t\t\t\t\t StreamConfiguration *cfg);\n>>\n>> \tint configure(const StreamConfiguration &config,\n>> --\n>> 2.45.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D3653BD80A\n\tfor ;\n\tTue, 8 Oct 2024 12:17:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1427618CB;\n\tTue, 8 Oct 2024 14:17:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B0DF3618C9\n\tfor ;\n\tTue, 8 Oct 2024 14:17:35 +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 2BFA8514;\n\tTue, 8 Oct 2024 14:15: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=\"M94xvL4e\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728389759;\n\tbh=fpX33UfDaLRxlxcI/lcNiIA48y3ULYTvjZnQIwIVBfg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=M94xvL4esNPoZqS956UYwCSSeN8pQldq4nAzSJOaEeaaFPRnzyUQJnz/W7TavQPxv\n\tkfULZTqM1lydVhz+5vSS+Dn/GFAVnxA1bxp/afvgJRJhV38RdutlvAZXjeicllfBVO\n\t1akTunoVo4J/WguCrFkXDmAD14Xm3N58/5SacQOY=","Message-ID":"<6cc9c2e7-ec68-445b-955c-7c134bb230dd@ideasonboard.com>","Date":"Tue, 8 Oct 2024 17:47:31 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tLaurent Pinchart ,\n\tPaul Elder ","References":"<20241008101918.10414-1-umang.jain@ideasonboard.com>\n\t<20241008101918.10414-2-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":"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\" "}},{"id":31638,"web_url":"https://patchwork.libcamera.org/comment/31638/","msgid":"<44kmuiwlypdr2lefhektndjryfy7vorjagroyxt7i2rco2tmwd@4tprefadxmlv>","date":"2024-10-09T10:54:28","subject":"Re: [PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch. \n\nOn Tue, Oct 08, 2024 at 03:49:18PM +0530, Umang Jain wrote:\n> Integrate the RkISP1 pipeline handler to support sensor configuration\n> provided by applications through CameraConfiguration::sensorConfig.\n> \n> The SensorConfiguration must be validated on both RkISP1Path (mainPath\n> and selfPath), so the parameters of RkISP1Path::validate() have been\n> updated to include sensorConfig.\n> \n> The camera configuration will be marked as invalid when the sensor\n> configuration is supplied, if:\n> - Invalid sensor configuration (SensorConfiguration::isValid())\n> - Bit depth not supported by RkISP1 pipeline\n> - RAW stream configuration size doesn't matches sensor configuration\n> output size\n> - Sensor configuration output size is larger than maximum supported\n> sensor configuration on RkISP1 pipeline\n> - No matching sensor configuration output size supplied by the sensor\n> \n> Signed-off-by: Umang Jain \n\nI tried it out and it works :-). My concern of the stream size beeing\nlimited to the sensorConfig is actually a topic for the dewarper\nintegration and (likely) doesn't apply here.\n\nSo\nReviewed-by: Stefan Klug \nTested-by: Stefan Klug \n\nCheers,\nStefan\n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 +++++++++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 +++++++++++++++++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +\n> 3 files changed, 80 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index c02c7cf3..42961c12 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -447,11 +447,12 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> \tStreamConfiguration config;\n> \n> \tconfig = cfg;\n> -\tif (data_->mainPath_->validate(sensor, &config) != Valid)\n> +\tif (data_->mainPath_->validate(sensor, sensorConfig, &config) != Valid)\n> \t\treturn false;\n> \n> \tconfig = cfg;\n> -\tif (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid)\n> +\tif (data_->selfPath_ &&\n> +\t data_->selfPath_->validate(sensor, sensorConfig, &config) != Valid)\n> \t\treturn false;\n> \n> \treturn true;\n> @@ -468,6 +469,27 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \n> \tstatus = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n> \n> +\t/*\n> +\t * Make sure that if a sensor configuration has been requested it\n> +\t * is valid.\n> +\t */\n> +\tif (sensorConfig) {\n> +\t\tif (!sensorConfig->isValid()) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Invalid sensor configuration request\";\n> +\n> +\t\t\treturn Invalid;\n> +\t\t}\n> +\n> +\t\tunsigned int bitDepth = sensorConfig->bitDepth;\n> +\t\tif (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Invalid sensor configuration bit depth\";\n> +\n> +\t\t\treturn Invalid;\n> +\t\t}\n> +\t}\n> +\n> \t/* Cap the number of entries to the available streams. */\n> \tif (config_.size() > pathCount) {\n> \t\tconfig_.resize(pathCount);\n> @@ -514,7 +536,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t/* Try to match stream without adjusting configuration. */\n> \t\tif (mainPathAvailable) {\n> \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (data_->mainPath_->validate(sensor, &tryCfg) == Valid) {\n> +\t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n> \t\t\t\tmainPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n> @@ -524,7 +546,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \n> \t\tif (selfPathAvailable) {\n> \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (data_->selfPath_->validate(sensor, &tryCfg) == Valid) {\n> +\t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n> \t\t\t\tselfPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n> @@ -535,7 +557,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t/* Try to match stream allowing adjusting configuration. */\n> \t\tif (mainPathAvailable) {\n> \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) {\n> +\t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n> \t\t\t\tmainPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n> @@ -546,7 +568,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \n> \t\tif (selfPathAvailable) {\n> \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) {\n> +\t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n> \t\t\t\tselfPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n> @@ -723,7 +745,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tV4L2SubdeviceFormat format = config->sensorFormat();\n> \tLOG(RkISP1, Debug) << \"Configuring sensor with \" << format;\n> \n> -\tret = sensor->setFormat(&format, config->combinedTransform());\n> +\tif (config->sensorConfig)\n> +\t\tret = sensor->applyConfiguration(*config->sensorConfig,\n> +\t\t\t\t\t\t config->combinedTransform(),\n> +\t\t\t\t\t\t &format);\n> +\telse\n> +\t\tret = sensor->setFormat(&format, config->combinedTransform());\n> +\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index da8d25c3..3b5bea96 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -251,8 +251,10 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> \treturn cfg;\n> }\n> \n> -CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> -\t\t\t\t\t\t StreamConfiguration *cfg)\n> +CameraConfiguration::Status\n> +RkISP1Path::validate(const CameraSensor *sensor,\n> +\t\t std::optional &sensorConfig,\n> +\t\t StreamConfiguration *cfg)\n> {\n> \tconst std::vector &mbusCodes = sensor->mbusCodes();\n> \tSize resolution = filterSensorResolution(sensor);\n> @@ -282,9 +284,14 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t\t\tcontinue;\n> \n> \t\t\t/*\n> -\t\t\t * Store the raw format with the highest bits per pixel\n> -\t\t\t * for later usage.\n> +\t\t\t * If the bits per pixel is supplied from the sensor\n> +\t\t\t * configuration, choose a raw format that complies with\n> +\t\t\t * it. Otherwise, store the raw format with the highest\n> +\t\t\t * bits per pixel for later usage.\n> \t\t\t */\n> +\t\t\tif (sensorConfig && info.bitsPerPixel != sensorConfig->bitDepth)\n> +\t\t\t\tcontinue;\n> +\n> \t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> \t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> \t\t\t\trawFormat = format;\n> @@ -297,6 +304,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t}\n> \t}\n> \n> +\tif (sensorConfig && !found)\n> +\t\treturn CameraConfiguration::Invalid;\n> +\n> \tbool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==\n> \t\t PixelFormatInfo::ColourEncodingRAW;\n> \n> @@ -319,11 +329,39 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t * size.\n> \t\t */\n> \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n> +\t\tSize rawSize = sensorConfig ? sensorConfig->outputSize\n> +\t\t\t\t\t : cfg->size;\n> +\n> \t\tV4L2SubdeviceFormat sensorFormat =\n> -\t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> +\t\t\tsensor->getFormat({ mbusCode }, rawSize);\n> +\n> +\t\tif (sensorConfig &&\n> +\t\t sensorConfig->outputSize != sensorFormat.size)\n> +\t\t\treturn CameraConfiguration::Invalid;\n> \n> \t\tminResolution = sensorFormat.size;\n> \t\tmaxResolution = sensorFormat.size;\n> +\t} else if (sensorConfig) {\n> +\t\t/*\n> +\t\t * We have already ensured 'rawFormat' has the matching bit\n> +\t\t * depth with sensorConfig.bitDepth hence, only validate the\n> +\t\t * sensorConfig's output size here.\n> +\t\t */\n> +\t\tSize sensorSize = sensorConfig->outputSize;\n> +\n> +\t\tif (sensorSize > resolution)\n> +\t\t\treturn CameraConfiguration::Invalid;\n> +\n> +\t\tuint32_t mbusCode = formatToMediaBus.at(rawFormat);\n> +\t\tV4L2SubdeviceFormat sensorFormat =\n> +\t\t\tsensor->getFormat({ mbusCode }, sensorSize);\n> +\n> +\t\tif (sensorFormat.size != sensorSize)\n> +\t\t\treturn CameraConfiguration::Invalid;\n> +\n> +\t\tminResolution = minResolution_.expandedToAspectRatio(sensorSize);\n> +\t\tmaxResolution = maxResolution_.boundedTo(sensorSize)\n> +\t\t\t\t\t .boundedToAspectRatio(sensorSize);\n> \t} else {\n> \t\t/*\n> \t\t * Adjust the size based on the sensor resolution and absolute\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index 777fcae7..ce9a5666 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -27,6 +27,7 @@ namespace libcamera {\n> class CameraSensor;\n> class MediaDevice;\n> class V4L2Subdevice;\n> +class SensorConfiguration;\n> struct StreamConfiguration;\n> struct V4L2SubdeviceFormat;\n> \n> @@ -44,6 +45,7 @@ public:\n> \t\t\t\t\t\t const Size &resolution,\n> \t\t\t\t\t\t StreamRole role);\n> \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n> +\t\t\t\t\t std::optional &sensorConfig,\n> \t\t\t\t\t StreamConfiguration *cfg);\n> \n> \tint configure(const StreamConfiguration &config,\n> -- \n> 2.45.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2759BC32DE\n\tfor ;\n\tWed, 9 Oct 2024 10:54:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4918063538;\n\tWed, 9 Oct 2024 12:54:33 +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 D6EBD618C5\n\tfor ;\n\tWed, 9 Oct 2024 12:54:31 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:749:b3f1:dbb0:6c33])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 632D82EC;\n\tWed, 9 Oct 2024 12:52:54 +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=\"i6Lnt5fq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728471174;\n\tbh=TCR630fjY086CbSf+v0N7Jout+cktS6J6qF4FFdmaEw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=i6Lnt5fqs5fo6WKK8D2gJPbVV7gsYFk+PWyVTXHxVB+cQi1RowhZYkU/U0YkMLtqY\n\tZwpW6VCKPy5VVWJ5pNHhssZVxfqHO9kCzqKUZCMN7VhyDxtUlhljp1cZLhbnf/hePX\n\tf3YZ4p1KXv+bHD2Xo/asO0eiGbj29uQGGEfjxv0c=","Date":"Wed, 9 Oct 2024 12:54:28 +0200","From":"Stefan Klug ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ,\n\tKieran Bingham , \n\tLaurent Pinchart ,\n\tPaul Elder ","Subject":"Re: [PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","Message-ID":"<44kmuiwlypdr2lefhektndjryfy7vorjagroyxt7i2rco2tmwd@4tprefadxmlv>","References":"<20241008101918.10414-1-umang.jain@ideasonboard.com>\n\t<20241008101918.10414-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241008101918.10414-2-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":31663,"web_url":"https://patchwork.libcamera.org/comment/31663/","msgid":"<20241009162420.GJ25706@pendragon.ideasonboard.com>","date":"2024-10-09T16:24:20","subject":"Re: [PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Oct 08, 2024 at 03:49:18PM +0530, Umang Jain wrote:\n> Integrate the RkISP1 pipeline handler to support sensor configuration\n> provided by applications through CameraConfiguration::sensorConfig.\n> \n> The SensorConfiguration must be validated on both RkISP1Path (mainPath\n> and selfPath), so the parameters of RkISP1Path::validate() have been\n> updated to include sensorConfig.\n> \n> The camera configuration will be marked as invalid when the sensor\n> configuration is supplied, if:\n> - Invalid sensor configuration (SensorConfiguration::isValid())\n> - Bit depth not supported by RkISP1 pipeline\n> - RAW stream configuration size doesn't matches sensor configuration\n> output size\n> - Sensor configuration output size is larger than maximum supported\n> sensor configuration on RkISP1 pipeline\n> - No matching sensor configuration output size supplied by the sensor\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 +++++++++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 +++++++++++++++++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +\n> 3 files changed, 80 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index c02c7cf3..42961c12 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -447,11 +447,12 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> \tStreamConfiguration config;\n> \n> \tconfig = cfg;\n> -\tif (data_->mainPath_->validate(sensor, &config) != Valid)\n> +\tif (data_->mainPath_->validate(sensor, sensorConfig, &config) != Valid)\n> \t\treturn false;\n> \n> \tconfig = cfg;\n> -\tif (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid)\n> +\tif (data_->selfPath_ &&\n> +\t data_->selfPath_->validate(sensor, sensorConfig, &config) != Valid)\n> \t\treturn false;\n> \n> \treturn true;\n> @@ -468,6 +469,27 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \n> \tstatus = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n> \n> +\t/*\n> +\t * Make sure that if a sensor configuration has been requested it\n> +\t * is valid.\n> +\t */\n> +\tif (sensorConfig) {\n> +\t\tif (!sensorConfig->isValid()) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Invalid sensor configuration request\";\n> +\n> +\t\t\treturn Invalid;\n> +\t\t}\n> +\n> +\t\tunsigned int bitDepth = sensorConfig->bitDepth;\n> +\t\tif (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Invalid sensor configuration bit depth\";\n> +\n> +\t\t\treturn Invalid;\n> +\t\t}\n> +\t}\n> +\n> \t/* Cap the number of entries to the available streams. */\n> \tif (config_.size() > pathCount) {\n> \t\tconfig_.resize(pathCount);\n> @@ -514,7 +536,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t/* Try to match stream without adjusting configuration. */\n> \t\tif (mainPathAvailable) {\n> \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (data_->mainPath_->validate(sensor, &tryCfg) == Valid) {\n> +\t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n> \t\t\t\tmainPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n> @@ -524,7 +546,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \n> \t\tif (selfPathAvailable) {\n> \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (data_->selfPath_->validate(sensor, &tryCfg) == Valid) {\n> +\t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n> \t\t\t\tselfPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n> @@ -535,7 +557,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t/* Try to match stream allowing adjusting configuration. */\n> \t\tif (mainPathAvailable) {\n> \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) {\n> +\t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n> \t\t\t\tmainPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n> @@ -546,7 +568,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \n> \t\tif (selfPathAvailable) {\n> \t\t\tStreamConfiguration tryCfg = cfg;\n> -\t\t\tif (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) {\n> +\t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n> \t\t\t\tselfPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n> @@ -723,7 +745,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \tV4L2SubdeviceFormat format = config->sensorFormat();\n> \tLOG(RkISP1, Debug) << \"Configuring sensor with \" << format;\n> \n> -\tret = sensor->setFormat(&format, config->combinedTransform());\n> +\tif (config->sensorConfig)\n> +\t\tret = sensor->applyConfiguration(*config->sensorConfig,\n> +\t\t\t\t\t\t config->combinedTransform(),\n> +\t\t\t\t\t\t &format);\n> +\telse\n> +\t\tret = sensor->setFormat(&format, config->combinedTransform());\n> +\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index da8d25c3..3b5bea96 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -251,8 +251,10 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> \treturn cfg;\n> }\n> \n> -CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> -\t\t\t\t\t\t StreamConfiguration *cfg)\n> +CameraConfiguration::Status\n> +RkISP1Path::validate(const CameraSensor *sensor,\n> +\t\t std::optional &sensorConfig,\n\nShouldn't this be const ?\n\n> +\t\t StreamConfiguration *cfg)\n> {\n> \tconst std::vector &mbusCodes = sensor->mbusCodes();\n> \tSize resolution = filterSensorResolution(sensor);\n> @@ -282,9 +284,14 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t\t\tcontinue;\n> \n> \t\t\t/*\n> -\t\t\t * Store the raw format with the highest bits per pixel\n> -\t\t\t * for later usage.\n> +\t\t\t * If the bits per pixel is supplied from the sensor\n> +\t\t\t * configuration, choose a raw format that complies with\n> +\t\t\t * it. Otherwise, store the raw format with the highest\n> +\t\t\t * bits per pixel for later usage.\n> \t\t\t */\n> +\t\t\tif (sensorConfig && info.bitsPerPixel != sensorConfig->bitDepth)\n> +\t\t\t\tcontinue;\n> +\n> \t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> \t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> \t\t\t\trawFormat = format;\n> @@ -297,6 +304,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t}\n> \t}\n> \n> +\tif (sensorConfig && !found)\n> +\t\treturn CameraConfiguration::Invalid;\n\nWhy is this ? For a non-raw stream, why would we reject an invalid\nformat instead of adjusting it when there's a sensor configuration ?\n\n> +\n> \tbool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==\n> \t\t PixelFormatInfo::ColourEncodingRAW;\n> \n> @@ -319,11 +329,39 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t * size.\n> \t\t */\n> \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n> +\t\tSize rawSize = sensorConfig ? sensorConfig->outputSize\n> +\t\t\t\t\t : cfg->size;\n> +\n> \t\tV4L2SubdeviceFormat sensorFormat =\n> -\t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> +\t\t\tsensor->getFormat({ mbusCode }, rawSize);\n> +\n> +\t\tif (sensorConfig &&\n> +\t\t sensorConfig->outputSize != sensorFormat.size)\n> +\t\t\treturn CameraConfiguration::Invalid;\n> \n> \t\tminResolution = sensorFormat.size;\n> \t\tmaxResolution = sensorFormat.size;\n> +\t} else if (sensorConfig) {\n> +\t\t/*\n> +\t\t * We have already ensured 'rawFormat' has the matching bit\n> +\t\t * depth with sensorConfig.bitDepth hence, only validate the\n> +\t\t * sensorConfig's output size here.\n> +\t\t */\n> +\t\tSize sensorSize = sensorConfig->outputSize;\n> +\n> +\t\tif (sensorSize > resolution)\n> +\t\t\treturn CameraConfiguration::Invalid;\n\nIs this check needed ?\n\n> +\n> +\t\tuint32_t mbusCode = formatToMediaBus.at(rawFormat);\n> +\t\tV4L2SubdeviceFormat sensorFormat =\n> +\t\t\tsensor->getFormat({ mbusCode }, sensorSize);\n> +\n> +\t\tif (sensorFormat.size != sensorSize)\n> +\t\t\treturn CameraConfiguration::Invalid;\n> +\n> +\t\tminResolution = minResolution_.expandedToAspectRatio(sensorSize);\n> +\t\tmaxResolution = maxResolution_.boundedTo(sensorSize)\n> +\t\t\t\t\t .boundedToAspectRatio(sensorSize);\n\nBelow we have\n\n\tmaxResolution = maxResolution_.boundedToAspectRatio(resolution)\n\t\t\t\t .boundedTo(resolution);\n\nIs there a reason why you swap the calls here ?\n\n> \t} else {\n> \t\t/*\n> \t\t * Adjust the size based on the sensor resolution and absolute\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index 777fcae7..ce9a5666 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -27,6 +27,7 @@ namespace libcamera {\n> class CameraSensor;\n> class MediaDevice;\n> class V4L2Subdevice;\n> +class SensorConfiguration;\n\nAlphabatical order.\n\n> struct StreamConfiguration;\n> struct V4L2SubdeviceFormat;\n> \n> @@ -44,6 +45,7 @@ public:\n> \t\t\t\t\t\t const Size &resolution,\n> \t\t\t\t\t\t StreamRole role);\n> \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n> +\t\t\t\t\t std::optional &sensorConfig,\n> \t\t\t\t\t StreamConfiguration *cfg);\n> \n> \tint configure(const StreamConfiguration &config,","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 63015C32E0\n\tfor ;\n\tWed, 9 Oct 2024 16:24:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C50663527;\n\tWed, 9 Oct 2024 18:24:27 +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 525A0618C5\n\tfor ;\n\tWed, 9 Oct 2024 18:24:25 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.230.3])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 55E9D2EC;\n\tWed, 9 Oct 2024 18:22:47 +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=\"ld0Dk4k7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728490967;\n\tbh=XDjDVbhID8rD3dNPlqTUUjNPW1kaOuAfSQksh11C3es=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ld0Dk4k7YYRopWAynUQOCeaq+s7xaNJ94i1cZhCIEQQwl79oZ2Q31/AITJBJ9gvgn\n\tHcGIy3zkKjyH2k+AJPTbKudtfDYhPRMzUtkdmbiRP78K8IfJXWs7ihRxytBr1+JLO2\n\t+NaL5omHnYcm8PqvUmkONzoDXMC9qcGh54WOcLY0=","Date":"Wed, 9 Oct 2024 19:24:20 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ,\n\tKieran Bingham ,\n\tPaul Elder ","Subject":"Re: [PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","Message-ID":"<20241009162420.GJ25706@pendragon.ideasonboard.com>","References":"<20241008101918.10414-1-umang.jain@ideasonboard.com>\n\t<20241008101918.10414-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241008101918.10414-2-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":31685,"web_url":"https://patchwork.libcamera.org/comment/31685/","msgid":"","date":"2024-10-10T06:43:46","subject":"Re: [PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Wed, Oct 09, 2024 at 07:24:20PM GMT, Laurent Pinchart wrote:\n> On Tue, Oct 08, 2024 at 03:49:18PM +0530, Umang Jain wrote:\n> > Integrate the RkISP1 pipeline handler to support sensor configuration\n> > provided by applications through CameraConfiguration::sensorConfig.\n> >\n> > The SensorConfiguration must be validated on both RkISP1Path (mainPath\n> > and selfPath), so the parameters of RkISP1Path::validate() have been\n> > updated to include sensorConfig.\n> >\n> > The camera configuration will be marked as invalid when the sensor\n> > configuration is supplied, if:\n> > - Invalid sensor configuration (SensorConfiguration::isValid())\n> > - Bit depth not supported by RkISP1 pipeline\n> > - RAW stream configuration size doesn't matches sensor configuration\n> > output size\n> > - Sensor configuration output size is larger than maximum supported\n> > sensor configuration on RkISP1 pipeline\n> > - No matching sensor configuration output size supplied by the sensor\n> >\n> > Signed-off-by: Umang Jain \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 +++++++++++++---\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 +++++++++++++++++--\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +\n> > 3 files changed, 80 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index c02c7cf3..42961c12 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -447,11 +447,12 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > \tStreamConfiguration config;\n> >\n> > \tconfig = cfg;\n> > -\tif (data_->mainPath_->validate(sensor, &config) != Valid)\n> > +\tif (data_->mainPath_->validate(sensor, sensorConfig, &config) != Valid)\n> > \t\treturn false;\n> >\n> > \tconfig = cfg;\n> > -\tif (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid)\n> > +\tif (data_->selfPath_ &&\n> > +\t data_->selfPath_->validate(sensor, sensorConfig, &config) != Valid)\n> > \t\treturn false;\n> >\n> > \treturn true;\n> > @@ -468,6 +469,27 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >\n> > \tstatus = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n> >\n> > +\t/*\n> > +\t * Make sure that if a sensor configuration has been requested it\n> > +\t * is valid.\n> > +\t */\n> > +\tif (sensorConfig) {\n> > +\t\tif (!sensorConfig->isValid()) {\n> > +\t\t\tLOG(RkISP1, Error)\n> > +\t\t\t\t<< \"Invalid sensor configuration request\";\n> > +\n> > +\t\t\treturn Invalid;\n> > +\t\t}\n> > +\n> > +\t\tunsigned int bitDepth = sensorConfig->bitDepth;\n> > +\t\tif (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {\n> > +\t\t\tLOG(RkISP1, Error)\n> > +\t\t\t\t<< \"Invalid sensor configuration bit depth\";\n> > +\n> > +\t\t\treturn Invalid;\n> > +\t\t}\n> > +\t}\n> > +\n> > \t/* Cap the number of entries to the available streams. */\n> > \tif (config_.size() > pathCount) {\n> > \t\tconfig_.resize(pathCount);\n> > @@ -514,7 +536,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > \t\t/* Try to match stream without adjusting configuration. */\n> > \t\tif (mainPathAvailable) {\n> > \t\t\tStreamConfiguration tryCfg = cfg;\n> > -\t\t\tif (data_->mainPath_->validate(sensor, &tryCfg) == Valid) {\n> > +\t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n> > \t\t\t\tmainPathAvailable = false;\n> > \t\t\t\tcfg = tryCfg;\n> > \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n> > @@ -524,7 +546,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >\n> > \t\tif (selfPathAvailable) {\n> > \t\t\tStreamConfiguration tryCfg = cfg;\n> > -\t\t\tif (data_->selfPath_->validate(sensor, &tryCfg) == Valid) {\n> > +\t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n> > \t\t\t\tselfPathAvailable = false;\n> > \t\t\t\tcfg = tryCfg;\n> > \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n> > @@ -535,7 +557,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > \t\t/* Try to match stream allowing adjusting configuration. */\n> > \t\tif (mainPathAvailable) {\n> > \t\t\tStreamConfiguration tryCfg = cfg;\n> > -\t\t\tif (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) {\n> > +\t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n> > \t\t\t\tmainPathAvailable = false;\n> > \t\t\t\tcfg = tryCfg;\n> > \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n> > @@ -546,7 +568,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >\n> > \t\tif (selfPathAvailable) {\n> > \t\t\tStreamConfiguration tryCfg = cfg;\n> > -\t\t\tif (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) {\n> > +\t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n> > \t\t\t\tselfPathAvailable = false;\n> > \t\t\t\tcfg = tryCfg;\n> > \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n> > @@ -723,7 +745,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \tV4L2SubdeviceFormat format = config->sensorFormat();\n> > \tLOG(RkISP1, Debug) << \"Configuring sensor with \" << format;\n> >\n> > -\tret = sensor->setFormat(&format, config->combinedTransform());\n> > +\tif (config->sensorConfig)\n> > +\t\tret = sensor->applyConfiguration(*config->sensorConfig,\n> > +\t\t\t\t\t\t config->combinedTransform(),\n> > +\t\t\t\t\t\t &format);\n> > +\telse\n> > +\t\tret = sensor->setFormat(&format, config->combinedTransform());\n> > +\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > index da8d25c3..3b5bea96 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > @@ -251,8 +251,10 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> > \treturn cfg;\n> > }\n> >\n> > -CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> > -\t\t\t\t\t\t StreamConfiguration *cfg)\n> > +CameraConfiguration::Status\n> > +RkISP1Path::validate(const CameraSensor *sensor,\n> > +\t\t std::optional &sensorConfig,\n>\n> Shouldn't this be const ?\n>\n> > +\t\t StreamConfiguration *cfg)\n> > {\n> > \tconst std::vector &mbusCodes = sensor->mbusCodes();\n> > \tSize resolution = filterSensorResolution(sensor);\n> > @@ -282,9 +284,14 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> > \t\t\t\tcontinue;\n> >\n> > \t\t\t/*\n> > -\t\t\t * Store the raw format with the highest bits per pixel\n> > -\t\t\t * for later usage.\n> > +\t\t\t * If the bits per pixel is supplied from the sensor\n> > +\t\t\t * configuration, choose a raw format that complies with\n> > +\t\t\t * it. Otherwise, store the raw format with the highest\n> > +\t\t\t * bits per pixel for later usage.\n> > \t\t\t */\n> > +\t\t\tif (sensorConfig && info.bitsPerPixel != sensorConfig->bitDepth)\n> > +\t\t\t\tcontinue;\n> > +\n> > \t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> > \t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> > \t\t\t\trawFormat = format;\n> > @@ -297,6 +304,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> > \t\t}\n> > \t}\n> >\n> > +\tif (sensorConfig && !found)\n> > +\t\treturn CameraConfiguration::Invalid;\n>\n> Why is this ? For a non-raw stream, why would we reject an invalid\n> format instead of adjusting it when there's a sensor configuration ?\n>\n\nRight, I suggested this but this should have been\n\n\n\tif (sensorConfig && !rawFormat)\n\t\treturn CameraConfiguration::Invalid;\n\ninstead\n\n> > +\n> > \tbool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==\n> > \t\t PixelFormatInfo::ColourEncodingRAW;\n> >\n> > @@ -319,11 +329,39 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> > \t\t * size.\n> > \t\t */\n> > \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n> > +\t\tSize rawSize = sensorConfig ? sensorConfig->outputSize\n> > +\t\t\t\t\t : cfg->size;\n> > +\n> > \t\tV4L2SubdeviceFormat sensorFormat =\n> > -\t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> > +\t\t\tsensor->getFormat({ mbusCode }, rawSize);\n> > +\n> > +\t\tif (sensorConfig &&\n> > +\t\t sensorConfig->outputSize != sensorFormat.size)\n> > +\t\t\treturn CameraConfiguration::Invalid;\n> >\n> > \t\tminResolution = sensorFormat.size;\n> > \t\tmaxResolution = sensorFormat.size;\n> > +\t} else if (sensorConfig) {\n> > +\t\t/*\n> > +\t\t * We have already ensured 'rawFormat' has the matching bit\n> > +\t\t * depth with sensorConfig.bitDepth hence, only validate the\n> > +\t\t * sensorConfig's output size here.\n> > +\t\t */\n> > +\t\tSize sensorSize = sensorConfig->outputSize;\n> > +\n> > +\t\tif (sensorSize > resolution)\n> > +\t\t\treturn CameraConfiguration::Invalid;\n>\n> Is this check needed ?\n>\n\nIf the sensorConfig requested output size cannot be processed by the\nISP, I think we should fail yes\n\n> > +\n> > +\t\tuint32_t mbusCode = formatToMediaBus.at(rawFormat);\n> > +\t\tV4L2SubdeviceFormat sensorFormat =\n> > +\t\t\tsensor->getFormat({ mbusCode }, sensorSize);\n> > +\n> > +\t\tif (sensorFormat.size != sensorSize)\n> > +\t\t\treturn CameraConfiguration::Invalid;\n> > +\n> > +\t\tminResolution = minResolution_.expandedToAspectRatio(sensorSize);\n> > +\t\tmaxResolution = maxResolution_.boundedTo(sensorSize)\n> > +\t\t\t\t\t .boundedToAspectRatio(sensorSize);\n>\n> Below we have\n>\n> \tmaxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> \t\t\t\t .boundedTo(resolution);\n>\n> Is there a reason why you swap the calls here ?\n\nI suggested this as well, as my thinking was that it is correct to first bound\ndown to a smaller res and then further reduce down to the desired aspect\nratio.\n\nHowever, if we bound down to a smaller res, we already got the desired\naspect ratio, so the two ops are interchangeable probably ?\n\n>\n> > \t} else {\n> > \t\t/*\n> > \t\t * Adjust the size based on the sensor resolution and absolute\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > index 777fcae7..ce9a5666 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > @@ -27,6 +27,7 @@ namespace libcamera {\n> > class CameraSensor;\n> > class MediaDevice;\n> > class V4L2Subdevice;\n> > +class SensorConfiguration;\n>\n> Alphabatical order.\n>\n> > struct StreamConfiguration;\n> > struct V4L2SubdeviceFormat;\n> >\n> > @@ -44,6 +45,7 @@ public:\n> > \t\t\t\t\t\t const Size &resolution,\n> > \t\t\t\t\t\t StreamRole role);\n> > \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n> > +\t\t\t\t\t std::optional &sensorConfig,\n> > \t\t\t\t\t StreamConfiguration *cfg);\n> >\n> > \tint configure(const StreamConfiguration &config,\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 73EC2C32EA\n\tfor ;\n\tThu, 10 Oct 2024 06:43:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2331063538;\n\tThu, 10 Oct 2024 08:43:52 +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 A3A45618C3\n\tfor ;\n\tThu, 10 Oct 2024 08:43:50 +0200 (CEST)","from ideasonboard.com (unknown [5.77.125.146])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 824F74D4;\n\tThu, 10 Oct 2024 08:42:12 +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=\"RfAIsDAs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728542532;\n\tbh=BWRjV4MWi8ZGriRPF2Mbn08jD98VMz02p3dBnufCr2U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RfAIsDAs5wyD8siiRX1YD0nGCCQNfKrtZAEv7iM3uqb08bnlKPK+RqJpcGNdhdgUu\n\tEt7qLTwi5hZhTtwmv+KzECCR367PVoGv/wjEj6VjWFOkCR1sYV88NvAoA0w9mpriTp\n\tOzomHpqPxD1Ru1VTKxnozisJa4pIBB7xSncsC8f0=","Date":"Thu, 10 Oct 2024 08:43:46 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Umang Jain , \n\tlibcamera-devel@lists.libcamera.org,\n\tJacopo Mondi , \n\tKieran Bingham ,\n\tPaul Elder ","Subject":"Re: [PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","Message-ID":"","References":"<20241008101918.10414-1-umang.jain@ideasonboard.com>\n\t<20241008101918.10414-2-umang.jain@ideasonboard.com>\n\t<20241009162420.GJ25706@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241009162420.GJ25706@pendragon.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":31713,"web_url":"https://patchwork.libcamera.org/comment/31713/","msgid":"<5d30fc3e-f6a6-488d-9acd-8b60a0d6f03e@ideasonboard.com>","date":"2024-10-11T09:27:42","subject":"Re: [PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 09/10/24 9:54 pm, Laurent Pinchart wrote:\n> On Tue, Oct 08, 2024 at 03:49:18PM +0530, Umang Jain wrote:\n>> Integrate the RkISP1 pipeline handler to support sensor configuration\n>> provided by applications through CameraConfiguration::sensorConfig.\n>>\n>> The SensorConfiguration must be validated on both RkISP1Path (mainPath\n>> and selfPath), so the parameters of RkISP1Path::validate() have been\n>> updated to include sensorConfig.\n>>\n>> The camera configuration will be marked as invalid when the sensor\n>> configuration is supplied, if:\n>> - Invalid sensor configuration (SensorConfiguration::isValid())\n>> - Bit depth not supported by RkISP1 pipeline\n>> - RAW stream configuration size doesn't matches sensor configuration\n>> output size\n>> - Sensor configuration output size is larger than maximum supported\n>> sensor configuration on RkISP1 pipeline\n>> - No matching sensor configuration output size supplied by the sensor\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 +++++++++++++---\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 +++++++++++++++++--\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +\n>> 3 files changed, 80 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index c02c7cf3..42961c12 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -447,11 +447,12 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n>> \tStreamConfiguration config;\n>> \n>> \tconfig = cfg;\n>> -\tif (data_->mainPath_->validate(sensor, &config) != Valid)\n>> +\tif (data_->mainPath_->validate(sensor, sensorConfig, &config) != Valid)\n>> \t\treturn false;\n>> \n>> \tconfig = cfg;\n>> -\tif (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid)\n>> +\tif (data_->selfPath_ &&\n>> +\t data_->selfPath_->validate(sensor, sensorConfig, &config) != Valid)\n>> \t\treturn false;\n>> \n>> \treturn true;\n>> @@ -468,6 +469,27 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>> \n>> \tstatus = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n>> \n>> +\t/*\n>> +\t * Make sure that if a sensor configuration has been requested it\n>> +\t * is valid.\n>> +\t */\n>> +\tif (sensorConfig) {\n>> +\t\tif (!sensorConfig->isValid()) {\n>> +\t\t\tLOG(RkISP1, Error)\n>> +\t\t\t\t<< \"Invalid sensor configuration request\";\n>> +\n>> +\t\t\treturn Invalid;\n>> +\t\t}\n>> +\n>> +\t\tunsigned int bitDepth = sensorConfig->bitDepth;\n>> +\t\tif (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {\n>> +\t\t\tLOG(RkISP1, Error)\n>> +\t\t\t\t<< \"Invalid sensor configuration bit depth\";\n>> +\n>> +\t\t\treturn Invalid;\n>> +\t\t}\n>> +\t}\n>> +\n>> \t/* Cap the number of entries to the available streams. */\n>> \tif (config_.size() > pathCount) {\n>> \t\tconfig_.resize(pathCount);\n>> @@ -514,7 +536,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>> \t\t/* Try to match stream without adjusting configuration. */\n>> \t\tif (mainPathAvailable) {\n>> \t\t\tStreamConfiguration tryCfg = cfg;\n>> -\t\t\tif (data_->mainPath_->validate(sensor, &tryCfg) == Valid) {\n>> +\t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n>> \t\t\t\tmainPathAvailable = false;\n>> \t\t\t\tcfg = tryCfg;\n>> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n>> @@ -524,7 +546,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>> \n>> \t\tif (selfPathAvailable) {\n>> \t\t\tStreamConfiguration tryCfg = cfg;\n>> -\t\t\tif (data_->selfPath_->validate(sensor, &tryCfg) == Valid) {\n>> +\t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {\n>> \t\t\t\tselfPathAvailable = false;\n>> \t\t\t\tcfg = tryCfg;\n>> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n>> @@ -535,7 +557,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>> \t\t/* Try to match stream allowing adjusting configuration. */\n>> \t\tif (mainPathAvailable) {\n>> \t\t\tStreamConfiguration tryCfg = cfg;\n>> -\t\t\tif (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) {\n>> +\t\t\tif (data_->mainPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n>> \t\t\t\tmainPathAvailable = false;\n>> \t\t\t\tcfg = tryCfg;\n>> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n>> @@ -546,7 +568,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>> \n>> \t\tif (selfPathAvailable) {\n>> \t\t\tStreamConfiguration tryCfg = cfg;\n>> -\t\t\tif (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) {\n>> +\t\t\tif (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {\n>> \t\t\t\tselfPathAvailable = false;\n>> \t\t\t\tcfg = tryCfg;\n>> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n>> @@ -723,7 +745,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n>> \tV4L2SubdeviceFormat format = config->sensorFormat();\n>> \tLOG(RkISP1, Debug) << \"Configuring sensor with \" << format;\n>> \n>> -\tret = sensor->setFormat(&format, config->combinedTransform());\n>> +\tif (config->sensorConfig)\n>> +\t\tret = sensor->applyConfiguration(*config->sensorConfig,\n>> +\t\t\t\t\t\t config->combinedTransform(),\n>> +\t\t\t\t\t\t &format);\n>> +\telse\n>> +\t\tret = sensor->setFormat(&format, config->combinedTransform());\n>> +\n>> \tif (ret < 0)\n>> \t\treturn ret;\n>> \n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> index da8d25c3..3b5bea96 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>> @@ -251,8 +251,10 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n>> \treturn cfg;\n>> }\n>> \n>> -CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>> -\t\t\t\t\t\t StreamConfiguration *cfg)\n>> +CameraConfiguration::Status\n>> +RkISP1Path::validate(const CameraSensor *sensor,\n>> +\t\t std::optional &sensorConfig,\n> Shouldn't this be const ?\n\nsent a fix\n>\n>> +\t\t StreamConfiguration *cfg)\n>> {\n>> \tconst std::vector &mbusCodes = sensor->mbusCodes();\n>> \tSize resolution = filterSensorResolution(sensor);\n>> @@ -282,9 +284,14 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>> \t\t\t\tcontinue;\n>> \n>> \t\t\t/*\n>> -\t\t\t * Store the raw format with the highest bits per pixel\n>> -\t\t\t * for later usage.\n>> +\t\t\t * If the bits per pixel is supplied from the sensor\n>> +\t\t\t * configuration, choose a raw format that complies with\n>> +\t\t\t * it. Otherwise, store the raw format with the highest\n>> +\t\t\t * bits per pixel for later usage.\n>> \t\t\t */\n>> +\t\t\tif (sensorConfig && info.bitsPerPixel != sensorConfig->bitDepth)\n>> +\t\t\t\tcontinue;\n>> +\n>> \t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n>> \t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n>> \t\t\t\trawFormat = format;\n>> @@ -297,6 +304,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>> \t\t}\n>> \t}\n>> \n>> +\tif (sensorConfig && !found)\n>> +\t\treturn CameraConfiguration::Invalid;\n> Why is this ? For a non-raw stream, why would we reject an invalid\n> format instead of adjusting it when there's a sensor configuration ?\n\nI misunderstood found, oops. Sent a fix to the list.\n>\n>> +\n>> \tbool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==\n>> \t\t PixelFormatInfo::ColourEncodingRAW;\n>> \n>> @@ -319,11 +329,39 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>> \t\t * size.\n>> \t\t */\n>> \t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n>> +\t\tSize rawSize = sensorConfig ? sensorConfig->outputSize\n>> +\t\t\t\t\t : cfg->size;\n>> +\n>> \t\tV4L2SubdeviceFormat sensorFormat =\n>> -\t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n>> +\t\t\tsensor->getFormat({ mbusCode }, rawSize);\n>> +\n>> +\t\tif (sensorConfig &&\n>> +\t\t sensorConfig->outputSize != sensorFormat.size)\n>> +\t\t\treturn CameraConfiguration::Invalid;\n>> \n>> \t\tminResolution = sensorFormat.size;\n>> \t\tmaxResolution = sensorFormat.size;\n>> +\t} else if (sensorConfig) {\n>> +\t\t/*\n>> +\t\t * We have already ensured 'rawFormat' has the matching bit\n>> +\t\t * depth with sensorConfig.bitDepth hence, only validate the\n>> +\t\t * sensorConfig's output size here.\n>> +\t\t */\n>> +\t\tSize sensorSize = sensorConfig->outputSize;\n>> +\n>> +\t\tif (sensorSize > resolution)\n>> +\t\t\treturn CameraConfiguration::Invalid;\n> Is this check needed ?\n\nMy idea is if sensor config size is passed such that it exceeds the \nlimits of the pipeline, it should be invalidated.\n\nresolution here is the highest sensor resolution that's supported on the \npipeline.\n>> +\n>> +\t\tuint32_t mbusCode = formatToMediaBus.at(rawFormat);\n>> +\t\tV4L2SubdeviceFormat sensorFormat =\n>> +\t\t\tsensor->getFormat({ mbusCode }, sensorSize);\n>> +\n>> +\t\tif (sensorFormat.size != sensorSize)\n>> +\t\t\treturn CameraConfiguration::Invalid;\n>> +\n>> +\t\tminResolution = minResolution_.expandedToAspectRatio(sensorSize);\n>> +\t\tmaxResolution = maxResolution_.boundedTo(sensorSize)\n>> +\t\t\t\t\t .boundedToAspectRatio(sensorSize);\n> Below we have\n>\n> \tmaxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> \t\t\t\t .boundedTo(resolution);\n>\n> Is there a reason why you swap the calls here ?\n\nI think it was Jacopo's review comment from v3:\n\n```\n\nIsn't it better to first \"boundedTo\" to make sure maxResolutions_ is\nsmaller than sensorSize and then \"boundedToAspectRatio\" to further\nshrink it to the aspect ratio of the sensorSize ?\n```\n\nwhich made sense to me. Should we swap the below calls instead ?\n>\n>> \t} else {\n>> \t\t/*\n>> \t\t * Adjust the size based on the sensor resolution and absolute\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>> index 777fcae7..ce9a5666 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>> @@ -27,6 +27,7 @@ namespace libcamera {\n>> class CameraSensor;\n>> class MediaDevice;\n>> class V4L2Subdevice;\n>> +class SensorConfiguration;\n> Alphabatical order.\n>\n>> struct StreamConfiguration;\n>> struct V4L2SubdeviceFormat;\n>> \n>> @@ -44,6 +45,7 @@ public:\n>> \t\t\t\t\t\t const Size &resolution,\n>> \t\t\t\t\t\t StreamRole role);\n>> \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n>> +\t\t\t\t\t std::optional &sensorConfig,\n>> \t\t\t\t\t StreamConfiguration *cfg);\n>> \n>> \tint configure(const StreamConfiguration &config,","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 BC26BC32EA\n\tfor ;\n\tFri, 11 Oct 2024 09:27:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B17B965371;\n\tFri, 11 Oct 2024 11:27:49 +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 3AB4963527\n\tfor ;\n\tFri, 11 Oct 2024 11:27:47 +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 A540E220;\n\tFri, 11 Oct 2024 11:26:07 +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=\"uKVVPUns\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728638768;\n\tbh=tjv3oXdWiIl6zqm8tsXva8qFFDsvPwc5wTAi03Oll/U=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=uKVVPUnsndt/S5/qgkYxnCth8WZ7LzEeWSAAPOSLGk3kkXAqHoMke0jmD66iLvr3P\n\t4zaluCtWfhK8DD76M6MhSSP6/GvNvyEvkjeettfGs5CtO8UNJefZ61mwT0OhbEry93\n\tuX/NXovrVQiigCZ1MbF3pD94nyaxeucfHLMwewMc=","Message-ID":"<5d30fc3e-f6a6-488d-9acd-8b60a0d6f03e@ideasonboard.com>","Date":"Fri, 11 Oct 2024 14:57:42 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ,\n\tKieran Bingham ,\n\tPaul Elder ","References":"<20241008101918.10414-1-umang.jain@ideasonboard.com>\n\t<20241008101918.10414-2-umang.jain@ideasonboard.com>\n\t<20241009162420.GJ25706@pendragon.ideasonboard.com>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<20241009162420.GJ25706@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\" "}}]