[{"id":31592,"web_url":"https://patchwork.libcamera.org/comment/31592/","msgid":"","date":"2024-10-04T10:09:35","subject":"Re: [PATCH v3 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 02:11:27PM 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 | 43 ++++++++++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 47 ++++++++++++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +\n> 3 files changed, 79 insertions(+), 13 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index c02c7cf3..8c3d78dc 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 && !sensorConfig->isValid()) {\n> +\t\tLOG(RkISP1, Error)\n> +\t\t\t<< \"Invalid sensor configuration request\";\n> +\n> +\t\treturn Invalid;\n> +\t}\n> +\n> +\tif (sensorConfig) {\n\nCan we unify the two if branches ?\n\n> +\t\tstd::optional bitDepth = sensorConfig->bitDepth;\n\nWhy an optional ?\n\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\nWhat about:\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\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\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,14 @@ 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> +\t} else {\n> +\t\tret = sensor->setFormat(&format, config->combinedTransform());\n> +\t}\n> +\n\nNo need for {} I think\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..20bff0a4 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,10 +284,16 @@ 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 (info.bitsPerPixel > rawBitsPerPixel) {\n> +\t\t\tif (sensorConfig &&\n> +\t\t\t info.bitsPerPixel == sensorConfig->bitDepth) {\n> +\t\t\t\trawFormat = format;\n> +\t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n\nShould this be\n\n\t\t\tif (sensorConfig && info.bitsPerPixel != sensorConfig->bitDepth)\n\t\t\t\tcontinue;\n\nOtherwise if (sensorConfig) and\n(info.bitsPerPixel != sensorConfig->bitDepth)\n\n> +\t\t\t} else if (info.bitsPerPixel > rawBitsPerPixel) {\n\nWe can still enter this branch and wrongly pick a format with a\nbitDepth != than the one specified in sensorConfig ?\n\nThen, if we exit the for() loop with \"found == false && sensorConfig\"\nyou should return invalid ?\n\n> \t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> \t\t\t\trawFormat = format;\n> \t\t\t}\n> @@ -319,13 +327,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> +\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\tuint32_t mbusCode = formatToMediaBus.at(rawFormat);\n\nnit: you can declare this a few lines below before using it\n\n> +\t\tSize sensorSize = sensorConfig->outputSize;\n> +\n> +\t\tif (sensorSize > resolution)\n> +\t\t\treturn CameraConfiguration::Invalid;\n> +\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_.boundedToAspectRatio(sensorSize)\n> +\t\t\t\t\t .boundedTo(sensorSize);\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\nExcept the bitDepth handling at the beginning of this function, most\ncomments are nits, I presume the next version should be good to go\n\nThanks\n j\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> 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 65A40C3257\n\tfor ;\n\tFri, 4 Oct 2024 10:09:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A47563524;\n\tFri, 4 Oct 2024 12:09:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9DE8162C92\n\tfor ;\n\tFri, 4 Oct 2024 12:09:44 +0200 (CEST)","from ideasonboard.com (unknown [5.77.86.253])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3FD1F55A;\n\tFri, 4 Oct 2024 12:08:08 +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=\"rRTh69gH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728036490;\n\tbh=mrY5vKrLU+jDDqUN8O+RHqDtWp8MNh8qe4GCTAWCV9g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rRTh69gH67dA1wPtq/pgOSebKf2ddY9O49kOhsGoEjv9BzCrECl4ORvbiWkOZy1DA\n\tYJQKvH3oFB035PzBl/yowAtpynR5GUEjmAkizVJbI/uGjZmoMA13rdgEf3TO+U4FqI\n\tJpKXwSHg4wmXXozihSQkdZpmezE/9DVNi7QCW5qs=","Date":"Fri, 4 Oct 2024 12:09:35 +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 v3 1/1] libcamera: rkisp1: Integrate SensorConfiguration\n\tsupport","Message-ID":"","References":"<20241004084128.14672-1-umang.jain@ideasonboard.com>\n\t<20241004084128.14672-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241004084128.14672-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\" "}}]