[{"id":31613,"web_url":"https://patchwork.libcamera.org/comment/31613/","msgid":"<3utl63hqttiao64p35hivmys4iudf3rfjw5lcph6hxhtm6onbk@wothuobmiolk>","date":"2024-10-08T08:54:14","subject":"Re: [PATCH v4 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 Fri, Oct 04, 2024 at 05:51:03PM 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> - 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 | 47 +++++++++++++++++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +\n> 3 files changed, 79 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 feb6d89f..c51554bc 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,13 +329,40 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\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\tSize rawSize = sensorConfig ? sensorConfig->outputSize\n> +\t\t\t\t\t : cfg->size;\n> \t\tcfg->size.boundTo(resolution);\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 reqCfg.size != sensorConfig->outputSize)\n\nShould this be reqCfg or sensorFormat ?\n\nIf checking sensorConfig->outputSize agains \"reqCfg\" you're validating\nthat the requested RAW stream configuration has the desired sensor\noutput size.\n\nIf checking sensorFormat->size instead you're validating that the\nrequested sensorConfig->outputSize is supported by the sensor\nnatively.\n\nI think this should be the latter, if reqCfg->size doesn't match the\nsensorConfig->outputSize it will later be adjusted to it, and I think\nthat's fine (as long as we return Adjusted), but we should make sure\nsensorFormat->size is exactly what has been requested with\nsensorConfig ?\n\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\nAh ok, I was about to suggest to move this just after initializing\n'resolution', but in case of isRaw we don't go through the ISP, so the\nISP max input constraint doesn't apply\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\nThat's what I was suggesting to do in the raw case as well\n\nOne small push and we should be there!\nThanks\n j\n\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.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 1405ABD80A\n\tfor ;\n\tTue, 8 Oct 2024 08:54:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADDEB6353B;\n\tTue, 8 Oct 2024 10:54:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A72F63537\n\tfor ;\n\tTue, 8 Oct 2024 10:54:17 +0200 (CEST)","from ideasonboard.com (unknown [5.179.165.198])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C99363D6;\n\tTue, 8 Oct 2024 10:52: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=\"bWVMpTAG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728377561;\n\tbh=dvHgbLAgibi9nenj4BiCk84xhDXXFu41JfTySqUMtgk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bWVMpTAG56Um3WbJmIHLREia1J1cDow5tJ2oi85tas+m+VVvOGCk3f3eVoYoUVCf0\n\tBPch8p8VEPJPd9MpKM7By8CPepN9Ts3x496V1XXklJddOR39dTOf3Hy2/F7Q4joovI\n\tlJMnrxsT0j+FgSrVdQcXzNKUmcnXGFhNrh/77i/U=","Date":"Tue, 8 Oct 2024 10:54:14 +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 , \n\tStefan Klug ","Subject":"Re: [PATCH v4 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","Message-ID":"<3utl63hqttiao64p35hivmys4iudf3rfjw5lcph6hxhtm6onbk@wothuobmiolk>","References":"<20241004122103.89807-1-umang.jain@ideasonboard.com>\n\t<20241004122103.89807-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241004122103.89807-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":31614,"web_url":"https://patchwork.libcamera.org/comment/31614/","msgid":"<01a617dd-2b83-49e6-a3f5-e1af44942001@ideasonboard.com>","date":"2024-10-08T09:47:14","subject":"Re: [PATCH v4 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 2:24 pm, Jacopo Mondi wrote:\n> Hi Umang\n>\n> On Fri, Oct 04, 2024 at 05:51:03PM 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>> - 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 | 47 +++++++++++++++++--\n>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +\n>> 3 files changed, 79 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 feb6d89f..c51554bc 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,13 +329,40 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\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\tSize rawSize = sensorConfig ? sensorConfig->outputSize\n>> +\t\t\t\t\t : cfg->size;\n>> \t\tcfg->size.boundTo(resolution);\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 reqCfg.size != sensorConfig->outputSize)\n> Should this be reqCfg or sensorFormat ?\n>\n> If checking sensorConfig->outputSize agains \"reqCfg\" you're validating\n> that the requested RAW stream configuration has the desired sensor\n> output size.\n>\n> If checking sensorFormat->size instead you're validating that the\n> requested sensorConfig->outputSize is supported by the sensor\n> natively.\n>\n> I think this should be the latter, if reqCfg->size doesn't match the\n> sensorConfig->outputSize it will later be adjusted to it, and I think\n> that's fine (as long as we return Adjusted), but we should make sure\n> sensorFormat->size is exactly what has been requested with\n> sensorConfig ?\n\nyou're absolutely correct here. It should be sensorFormat, not refCfg, \nas reqCfg can get adjusted.\n\nWill address in v5.\n>\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> Ah ok, I was about to suggest to move this just after initializing\n> 'resolution', but in case of isRaw we don't go through the ISP, so the\n> ISP max input constraint doesn't apply\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> That's what I was suggesting to do in the raw case as well\n>\n> One small push and we should be there!\n> Thanks\n> j\n>\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.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 06F3DBE080\n\tfor ;\n\tTue, 8 Oct 2024 09:47:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 273F16353A;\n\tTue, 8 Oct 2024 11:47:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 58E8562C8E\n\tfor ;\n\tTue, 8 Oct 2024 11:47:20 +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 7E3C63D6;\n\tTue, 8 Oct 2024 11:45:42 +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=\"GZQYKXBF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728380743;\n\tbh=+UhT88l5VfgWeJ+GE6AuCHmaYbhyzoP+GG0jH6xHTds=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=GZQYKXBF84iDyjr2QpIlmViEmtV20QVIwBmlA4zD0ggr1IWww5oPKToceHfbST1FA\n\tLbUUa45QYy2Z+5J/6Di76SlN/nt12JXXrng2ssufPF99mr5qVn2UQEIZZA8XfnE2nV\n\tI6o9CpxrdE6Ehg7k4B+zCcPMw2jCbwJpMhazc61M=","Message-ID":"<01a617dd-2b83-49e6-a3f5-e1af44942001@ideasonboard.com>","Date":"Tue, 8 Oct 2024 15:17:14 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 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 ,\n\tStefan Klug ","References":"<20241004122103.89807-1-umang.jain@ideasonboard.com>\n\t<20241004122103.89807-2-umang.jain@ideasonboard.com>\n\t<3utl63hqttiao64p35hivmys4iudf3rfjw5lcph6hxhtm6onbk@wothuobmiolk>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<3utl63hqttiao64p35hivmys4iudf3rfjw5lcph6hxhtm6onbk@wothuobmiolk>","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\" "}}]