[{"id":31526,"web_url":"https://patchwork.libcamera.org/comment/31526/","msgid":"","date":"2024-10-02T12:15:12","subject":"Re: [PATCH] libcamera: rkisp1: Integrate SensorConfiguration support","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 01, 2024 at 07:47:38PM 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> For RAW paths, if a sensor configuration is provided, it will be\n> adjusted based on the stream configuration.\n>\n> Finally, if the sensor configuration fails to apply, the camera\n> configuration validation will be marked as invalid.\n>\n> Signed-off-by: Umang Jain \n> ---\n> Testing:\n> - Developed over [PATCH 0/2] libcamera: pixelformat: Add isRaw() helper\n> - Tested with cam+[1] on i.MX8MP with IMX283 attached\n>\n> [1]: https://patchwork.libcamera.org/patch/20080/\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++++++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 20 +++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +\n> 3 files changed, 87 insertions(+), 14 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 0c4519de..49fe8fb1 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -474,11 +474,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> @@ -495,6 +496,31 @@ 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) << \"Invalid sensor configuration request\";\n> +\t\treturn Invalid;\n> +\t}\n> +\n> +\tif (sensorConfig) {\n> +\t\tstd::optional bitDepth = sensorConfig->bitDepth;\n> +\t\tif (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {\n> +\t\t\t/*\n> +\t\t\t * Default to the first supported bit depth, if an\n> +\t\t\t * unsupported one is supplied.\n> +\t\t\t */\n> +\t\t\tV4L2SubdeviceFormat format = {};\n> +\t\t\tformat.code = sensor->mbusCodes().at(0);\n> +\n> +\t\t\tsensorConfig->bitDepth =\n> +\t\t\t\tMediaBusFormatInfo::info(format.code).bitsPerPixel;\n> +\t\t\tstatus = Adjusted;\n\nNo, we never adjust sensorConfig. Either it's fully valid or we refuse\nit. The rational is that we cannot adjust it to something meaningful\nand applications that use sensorConfig should be highly specialized\nand know precisely what they're doing.\n\nIt is documented by the SensorConfiguration class itself, even if\nbehind a \\todo that should only apply to the first statement, not to\nthe whole paragraph imho\n\n * \\todo Applications shall fully populate all fields of the\n * CameraConfiguration::sensorConfig class members before validating the\n * CameraConfiguration. If the SensorConfiguration is not fully populated, or if\n * any of its parameters cannot be applied to the sensor in use, the\n * CameraConfiguration validation process will fail and return\n * CameraConfiguration::Status::Invalid.\n\nThis is the relevant part\n\n * If the SensorConfiguration is not fully populated, or if\n * any of its parameters cannot be applied to the sensor in use, the\n * CameraConfiguration validation process will fail and return\n * CameraConfiguration::Status::Invalid.\n\nMaybe it's all behind a todo because we currently allow populating\nonly bitDepth and outputSize, as documented by the isValid() function\n\n * \\todo For now allow applications to populate the bitDepth and the outputSize\n * only as skipping and binnings factors are initialized to 1 and the analog\n * crop is ignored.\n\nBut the idea of never adjusting them is still valid.\n\nI don't think we should adjust the sensor configuration.\n\nAlso the check for validity assumes only 8 10 and 12 are valid, while there\nare sensors that can happily provide 14 or 16 bits.\n\nIf this is a limitation of the pipeline handler, of the driver or the\nhardware itself, applications that specify a sensorConfig should know\nabout this as we assume they're highly specialized.\n\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> @@ -540,7 +566,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> @@ -550,7 +576,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> @@ -561,7 +587,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> @@ -572,7 +598,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> @@ -589,26 +615,63 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>\n> \t/* Select the sensor format. */\n> \tPixelFormat rawFormat;\n> +\tSize rawSize;\n> \tSize maxSize;\n>\n> \tfor (const StreamConfiguration &cfg : config_) {\n> -\t\tif (cfg.pixelFormat.isRaw())\n> +\t\tif (cfg.pixelFormat.isRaw()) {\n> \t\t\trawFormat = cfg.pixelFormat;\n> +\t\t\trawSize = cfg.size;\n> +\t\t}\n>\n> \t\tmaxSize = std::max(maxSize, cfg.size);\n> \t}\n>\n> +\tif (rawFormat.isValid() && sensorConfig) {\n> +\t\tif (sensorConfig->outputSize != rawSize) {\n> +\t\t\tsensorConfig->outputSize = rawSize;\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n> +\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(rawFormat);\n> +\t\tif (sensorConfig->bitDepth != info.bitsPerPixel) {\n> +\t\t\tsensorConfig->bitDepth = info.bitsPerPixel;\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n\nAs said above, never adjust the sensor configuration.\n\n> +\t}\n> +\n> \tstd::vector mbusCodes;\n> +\tSize sz = maxSize;\n>\n> \tif (rawFormat.isValid()) {\n> \t\tmbusCodes = { rawFormats.at(rawFormat) };\n> +\t\tsz = rawSize;\n> +\t} else if (sensorConfig) {\n> +\t\tfor (const auto &value : rawFormats) {\n> +\t\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(value.first);\n> +\t\t\tif (info.bitsPerPixel == sensorConfig->bitDepth)\n> +\t\t\t\tmbusCodes.push_back(value.second);\n> +\t\t}\n> +\t\tsz = sensorConfig->outputSize;\n\nYou need to validate that sensorConfig->outputSize is actually\nsupported by the sensor. I would do that in RkISP1Path::validate() as\nsuggested below ?\n\n> \t} else {\n> \t\tstd::transform(rawFormats.begin(), rawFormats.end(),\n> \t\t\t std::back_inserter(mbusCodes),\n> \t\t\t [](const auto &value) { return value.second; });\n> \t}\n>\n> -\tsensorFormat_ = sensor->getFormat(mbusCodes, maxSize);\n> +\tsensorFormat_ = sensor->getFormat(mbusCodes, sz);\n> +\tif (sensorConfig) {\n> +\t\tint ret = data_->sensor_->applyConfiguration(*sensorConfig,\n> +\t\t\t\t\t\t\t combinedTransform_,\n> +\t\t\t\t\t\t\t &sensorFormat_);\n\nI would avoid applying a sensor configuration at validate() but only\ndo so at configure(), check what RPi does if you need examples.\n\n> +\t\tif (ret) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Sensor Configuration not supported: \"\n> +\t\t\t\t<< strerror(-ret);\n> +\n> +\t\t\treturn Invalid;\n> +\t\t}\n> +\t}\n>\n> \tif (sensorFormat_.size.isNull())\n> \t\tsensorFormat_.size = sensor->resolution();\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index 7c1f0c73..30becb75 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -253,8 +253,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\nDoes copy-constructing an std::optional<> copy the managed instance ?\n\nFrom my understanding of\nhttps://en.cppreference.com/w/cpp/utility/optional/optional\n\nCopy constructor: If other contains a value, initializes the contained\nvalue as if direct-initializing (but not direct-list-initializing) an\nobject of type T with the expression *other\n\nit does.\n\nIn other word, we're not only copying the std::optional<> wrapper\n(which should in this case point to some dynamically allocated memory\nfor the managed object) but the whole object itself ?\n\nI would pass it by reference ?\n\n> +\t\t StreamConfiguration *cfg)\n> {\n> \tconst std::vector &mbusCodes = sensor->mbusCodes();\n> \tSize resolution = filterSensorResolution(sensor);\n> @@ -281,12 +283,18 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t\t mbusCodes.end())\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\n /*\n\t\t\t * If the bits per pixel is supplied from the sensor\n\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\tconst PixelFormatInfo &info = PixelFormatInfo::info(format);\n> -\t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> +\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> +\t\t\t} else if (info.bitsPerPixel > rawBitsPerPixel) {\n> \t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> \t\t\t\trawFormat = format;\n\nA few lines below we have\n\n\tif (isRaw) {\n\t\t/*\n\t\t * Use the sensor output size closest to the requested stream\n\t\t * size.\n\t\t */\n\t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n\t\tV4L2SubdeviceFormat sensorFormat =\n\t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n\n\t\tminResolution = sensorFormat.size;\n\t\tmaxResolution = sensorFormat.size;\n\nIf a SensorConfig is specified, shouldn't the size there provided\nbe used ? Also, make sure that 'sensorFormat' size is the one\nspecified in the sensorConfig. IOW the sensorConfig.size should be\nsupported exactly by the sensor, otherwise we should fail as\nsensorConfig shall not be adjusted.\n\nOnce we have validated that the (optional) sensorConfig.size is\nsupported by the sensor, the StreamConfig::size will be then adjusted\nto it with:\n\n\tcfg->size.boundTo(maxResolution);\n\tcfg->size.expandTo(minResolution);\n\na few lines below\n\nIf a sensorFormat is there, the RAW stream should be adjusted to match\nit, and you can use it to select the sensor format in\nRkISP1CameraConfiguration::validate().\n\nThanks\n j\n\n\n> \t\t\t}\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index 9f75fe1f..8aae00ef 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> @@ -45,6 +46,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 571F8BD80A\n\tfor ;\n\tWed, 2 Oct 2024 12:15:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 05D1163525;\n\tWed, 2 Oct 2024 14:15: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 8DE8663510\n\tfor ;\n\tWed, 2 Oct 2024 14:15:17 +0200 (CEST)","from ideasonboard.com (unknown [37.159.124.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C179B3E6;\n\tWed, 2 Oct 2024 14:13:44 +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=\"oAwGtlF3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727871225;\n\tbh=NSHKqCr3BEi9IqqE0M3Cv1QGvUXtOMlgHeHlnpb+7ow=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oAwGtlF3quNgMLceDTNjeEcfSwU1A0O9+NMEJ8/P1uISwhxwE+FTy+PIcIhS/Fbkq\n\tFhlsBcRt0jAJfZ1WqWocLfzbK3Xfnk21Wm5BdQ70K6sNPvp6bJQpQWIrGU4JfnXdMo\n\t8rvHYc5SBv76AnIAQtUnp1vCq0uXYMLifQXqqOr8=","Date":"Wed, 2 Oct 2024 14:15:12 +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] libcamera: rkisp1: Integrate SensorConfiguration support","Message-ID":"","References":"<20241001141738.17471-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241001141738.17471-1-umang.jain@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]