[{"id":28486,"web_url":"https://patchwork.libcamera.org/comment/28486/","msgid":"<20240117163155.GJ4860@pendragon.ideasonboard.com>","date":"2024-01-17T16:31:55","subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: rkisp1: Plumb\n\tSensorConfiguration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Jan 16, 2024 at 06:17:54PM +0900, Paul Elder via libcamera-devel wrote:\n> Add support to the rkisp1 pipeline handler for validating and\n> configuring SensorConfiguration.\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 97 ++++++++++++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 37 +++++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +-\n> 3 files changed, 115 insertions(+), 22 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index fb67ba8f4..74da9c1b1 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -128,7 +128,8 @@ public:\n\n#include \n\n> \tconst Transform &combinedTransform() { return combinedTransform_; }\n> \n> private:\n> -\tbool fitsAllPaths(const StreamConfiguration &cfg);\n> +\tbool fitsAllPaths(const StreamConfiguration &cfg,\n> +\t\t\t std::optional sensorBPP);\n> \n> \t/*\n> \t * The RkISP1CameraData instance is guaranteed to be valid as long as the\n> @@ -448,17 +449,18 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> \tdata_ = data;\n> }\n> \n> -bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg,\n> +\t\t\t\t\t std::optional sensorBPP)\n> {\n> \tconst CameraSensor *sensor = data_->sensor_.get();\n> \tStreamConfiguration config;\n> \n> \tconfig = cfg;\n> -\tif (data_->mainPath_->validate(sensor, &config) != Valid)\n> +\tif (data_->mainPath_->validate(sensor, &config, sensorBPP) != Valid)\n> \t\treturn false;\n> \n> \tconfig = cfg;\n> -\tif (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid)\n> +\tif (data_->selfPath_ && data_->selfPath_->validate(sensor, &config, sensorBPP) != Valid)\n> \t\treturn false;\n> \n> \treturn true;\n> @@ -475,6 +477,24 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \n> \tstatus = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n> \n> +\t/*\n> +\t * For the sensor configuration, default to the first supported bit\n> +\t * depth for the sensor configuration if an unsupported one is\n> +\t * supplied.\n\nHolds in two lines.\n\n> +\t */\n> +\tstd::optional bitDepth;\n> +\tif (sensorConfig) {\n> +\t\tbitDepth = sensorConfig->bitDepth;\n> +\t\tif (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {\n\nWhat if sensorConfig->bitDepth is 8, 10 or 12, but takes a value that\nthe sensor doesn't support ?\n\n> +\t\t\tV4L2SubdeviceFormat format = {};\n> +\t\t\tformat.mbus_code = data_->sensor_->mbusCodes().at(0);\n> +\n> +\t\t\tsensorConfig->bitDepth = format.bitsPerPixel();\n> +\t\t\tbitDepth = sensorConfig->bitDepth;\n> +\t\t\tstatus = Adjusted;\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> @@ -510,7 +530,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t */\n> \tstd::vector order(config_.size());\n> \tstd::iota(order.begin(), order.end(), 0);\n> -\tif (config_.size() == 2 && fitsAllPaths(config_[0]))\n> +\tif (config_.size() == 2 && fitsAllPaths(config_[0], bitDepth))\n> \t\tstd::reverse(order.begin(), order.end());\n> \n> \tbool mainPathAvailable = true;\n> @@ -521,7 +541,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, &tryCfg, bitDepth) == Valid) {\n> \t\t\t\tmainPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n> @@ -531,7 +551,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, &tryCfg, bitDepth) == Valid) {\n> \t\t\t\tselfPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n> @@ -542,7 +562,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, &tryCfg, bitDepth) == Adjusted) {\n> \t\t\t\tmainPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n> @@ -553,7 +573,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, &tryCfg, bitDepth) == Adjusted) {\n> \t\t\t\tselfPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n> @@ -570,28 +590,75 @@ 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\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> \t\t\trawFormat = cfg.pixelFormat;\n> +\t\t\trawSize = cfg.size;\n> +\t\t}\n> \n> +\t\t/* path->validate binds this to a sensor-supported resolution */\n\ns/validate/validate()/\ns/resolution/resolution./\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\nIf I understand correctly, the raw stream configuration takes precedence\nover the sensorConfig. Is that a standard behaviour we document ?\n\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> +\t} else if (!rawFormat.isValid() && sensorConfig) {\n> +\t\t/*\n> +\t\t * TODO Handle this properly taking into account the upscaling\n\ns/TODO/\\\\todo/\n\n> +\t\t * capabilities and dual ISP mode (for the i.MX8MP).\n> +\t\t *\n> +\t\t * x1.5 should be a reasonable hardcoded ballpark for now.\n> +\t\t */\n> +\t\tdouble factor = 1.5;\n\nconst\n\n> +\t\tSize before = sensorConfig->outputSize;\n\nconst\n\ns/before/originalSize/ ?\n\n> +\t\tSize upscaleLimit = {\n> +\t\t\tstatic_cast(maxSize.width / factor),\n> +\t\t\tstatic_cast(maxSize.height / factor)\n> +\t\t};\n\nconst\n\nI think you can write it\n\n\t\tconst Size upscaleLimit = maxSize / factor;\n\n> +\n> +\t\tif (sensorConfig->outputSize < upscaleLimit)\n\nDon't you need to compare the width and height separately ? Comparison\nof sizes is tricky :-S\n\n> +\t\t\tsensorConfig->outputSize = maxSize;\n> +\n> +\t\tif (before != sensorConfig->outputSize)\n> +\t\t\tstatus = Adjusted;\n> +\n> +\t\t/* No need to validate bpp for non-raw */\n\nWhy not ?\n\n> +\t}\n> +\n> \tstd::vector mbusCodes;\n> +\tSize size = maxSize;\n\ns/size/sensorSize/\n\n> \n> \tif (rawFormat.isValid()) {\n> \t\tmbusCodes = { rawFormats.at(rawFormat) };\n> +\t\tsize = 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\nI may be wrong, but shouldn't you set size = sensorConfig->outputSize\nhere ?\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, size);\n> \n> +\t/* This should never happen if sensorConfig is valid */\n\ns/valid/valid./\n\n> \tif (sensorFormat_.size.isNull())\n> \t\tsensorFormat_.size = sensor->resolution();\n> \n> @@ -730,7 +797,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(), &format);\n> +\t} else {\n> +\t\tret = sensor->setFormat(&format, config->combinedTransform());\n> +\t}\n\nNo need for curly braces.\n\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 eaab7e857..a598b289b 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -220,7 +220,8 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> }\n> \n> CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> -\t\t\t\t\t\t StreamConfiguration *cfg)\n> +\t\t\t\t\t\t StreamConfiguration *cfg,\n> +\t\t\t\t\t\t std::optional sensorBPP)\n> {\n> \tconst std::vector &mbusCodes = sensor->mbusCodes();\n> \tconst Size &resolution = sensor->resolution();\n> @@ -231,10 +232,15 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t/*\n> \t * Validate the pixel format. If the requested format isn't supported,\n> \t * default to either NV12 (all versions of the ISP are guaranteed to\n> -\t * support NV12 on both the main and self paths) if the requested format\n> -\t * is not a raw format, or to the supported raw format with the highest\n> -\t * bits per pixel otherwise.\n> +\t * support NV12 on both the main and self paths) if the requested\n\nI think this line doesn't need to change, it wasn't above the 80\ncharacters limit.\n\n> +\t * format is not a raw format, or otherwise to a supported raw format\n> +\t * with the same number of bits per pixel, or to maximum bits per pixel\n> +\t * if the same is not supported.\n> \t */\n> +\tconst PixelFormatInfo &formatInfo = PixelFormatInfo::info(cfg->pixelFormat);\n> +\tunsigned int reqRawBitsPerPixel = formatInfo.bitsPerPixel;\n> +\tPixelFormat reqRawFormat;\n> +\n> \tunsigned int rawBitsPerPixel = 0;\n> \tPixelFormat rawFormat;\n> \tbool found = false;\n> @@ -250,12 +256,22 @@ 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\n> +\t\t\t * with it. Otherwise store the raw format with the\n> +\t\t\t * highest bits per pixel for later usage.\n> \t\t\t */\n> -\t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> -\t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> -\t\t\t\trawFormat = format;\n> +\t\t\tif (sensorBPP) {\n> +\t\t\t\tif (info.bitsPerPixel == sensorBPP)\n> +\t\t\t\t\trawFormat = format;\n\nDo we have a guarantee that at least one format will match sensorBPP ?\nIf so, a comment to explain why would be useful.\n\n> +\t\t\t} else {\n> +\t\t\t\tif (info.bitsPerPixel == reqRawBitsPerPixel)\n> +\t\t\t\t\treqRawFormat = format;\n> +\n> +\t\t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> +\t\t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> +\t\t\t\t\trawFormat = format;\n> +\t\t\t\t}\n> \t\t\t}\n\nI wonder if all this could be simplified by handling the bpp selection\noutside of the path code, and use the selected valud in\nRkISP1Path::validate(), without considering the bpp from the pixel\nformat here.\n\n> \t\t}\n> \n> @@ -265,6 +281,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t}\n> \t}\n> \n> +\tif (reqRawFormat.isValid())\n> +\t\trawFormat = reqRawFormat;\n> +\n> \tbool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==\n> \t\t PixelFormatInfo::ColourEncodingRAW;\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index c96bd5557..784bcea77 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -44,7 +44,8 @@ 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 StreamConfiguration *cfg);\n> +\t\t\t\t\t StreamConfiguration *cfg,\n> +\t\t\t\t\t std::optional sensorBPP);\n> \n> \tint configure(const StreamConfiguration &config,\n> \t\t const V4L2SubdeviceFormat &inputFormat);","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 01327BDB1C\n\tfor ;\n\tWed, 17 Jan 2024 16:31:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3425D628AE;\n\tWed, 17 Jan 2024 17:31:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ABEEE628AD\n\tfor ;\n\tWed, 17 Jan 2024 17:31:51 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D73DE7EC;\n\tWed, 17 Jan 2024 17:30:41 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705509113;\n\tbh=gKCmTwbcdEUhvIrGOUdiQIL5qKrgN133cdrSclpItJs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ddNmr0bzUDxltDxfF9zKslTqFPK4kcvnkTu/8yfE0KySpe3idZhESxPFOLiYrZlt4\n\tjBcp+AJl+rGRluU0ijUbgZP/inSD4DOXRiNzS1mRXkM8CgLMIISLeBc/LCoxyBt1i6\n\th+ojglMsqdOgA3jffKN8oe/1CCApZDNoF2F6vLgQyfY3KDQ48mZpAnWLP9F5ZfPojy\n\tvMIjS5COuoEU4kVSK0q6wDLU2UfQDYVV5WIN4emzMXxTLec+U0qmqRfDFsvsW9Tvze\n\tDYfHWZoEi8dYozqlU4ovx8+crYj9ffZLQkbRdXA9bKRlugbotE+ASvQxjbK55xRKRa\n\tR8PXn9wOL64LA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1705509042;\n\tbh=gKCmTwbcdEUhvIrGOUdiQIL5qKrgN133cdrSclpItJs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TZeeqFFfF8eJGonftRMDQ+HBrlIdffYJsFmTvUVlfMWaXUU0lpz84FrTCF+fYGUfz\n\t6SeixD3cAhZt/pGduHb18e70Fk+OvR5ZL5J4YbXPSxBHE4icT47UiaTgaXwySozqJF\n\tyGktywP42oCWToHmYP2xXtn5tD8stZZYBt74ouFI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TZeeqFFf\"; dkim-atps=neutral","Date":"Wed, 17 Jan 2024 18:31:55 +0200","To":"Paul Elder ","Message-ID":"<20240117163155.GJ4860@pendragon.ideasonboard.com>","References":"<20240116091754.100654-1-paul.elder@ideasonboard.com>\n\t<20240116091754.100654-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240116091754.100654-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: rkisp1: Plumb\n\tSensorConfiguration","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28487,"web_url":"https://patchwork.libcamera.org/comment/28487/","msgid":"","date":"2024-01-17T16:36:58","subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: rkisp1: Plumb\n\tSensorConfiguration","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Paul\n\nOn Tue, Jan 16, 2024 at 06:17:54PM +0900, Paul Elder via libcamera-devel wrote:\n> Add support to the rkisp1 pipeline handler for validating and\n> configuring SensorConfiguration.\n>\n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 97 ++++++++++++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 37 +++++--\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +-\n> 3 files changed, 115 insertions(+), 22 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index fb67ba8f4..74da9c1b1 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -128,7 +128,8 @@ public:\n> \tconst Transform &combinedTransform() { return combinedTransform_; }\n>\n> private:\n> -\tbool fitsAllPaths(const StreamConfiguration &cfg);\n> +\tbool fitsAllPaths(const StreamConfiguration &cfg,\n> +\t\t\t std::optional sensorBPP);\n>\n> \t/*\n> \t * The RkISP1CameraData instance is guaranteed to be valid as long as the\n> @@ -448,17 +449,18 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> \tdata_ = data;\n> }\n>\n> -bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg,\n> +\t\t\t\t\t std::optional sensorBPP)\n\nNit: SensorConfiguration uses 'bitDepth'. Let's try to reuse it.\n\n> {\n> \tconst CameraSensor *sensor = data_->sensor_.get();\n> \tStreamConfiguration config;\n>\n> \tconfig = cfg;\n> -\tif (data_->mainPath_->validate(sensor, &config) != Valid)\n> +\tif (data_->mainPath_->validate(sensor, &config, sensorBPP) != Valid)\n> \t\treturn false;\n>\n> \tconfig = cfg;\n> -\tif (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid)\n> +\tif (data_->selfPath_ && data_->selfPath_->validate(sensor, &config, sensorBPP) != Valid)\n> \t\treturn false;\n>\n> \treturn true;\n> @@ -475,6 +477,24 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>\n> \tstatus = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n>\n> +\t/*\n> +\t * For the sensor configuration, default to the first supported bit\n> +\t * depth for the sensor configuration if an unsupported one is\n> +\t * supplied.\n> +\t */\n> +\tstd::optional bitDepth;\n> +\tif (sensorConfig) {\n> +\t\tbitDepth = sensorConfig->bitDepth;\n> +\t\tif (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {\n> +\t\t\tV4L2SubdeviceFormat format = {};\n\nI don't this this is correct.\n\nThe SensorConfiguration HAS to be correct as we cannot meaningfully\nadjust it. If the supplied config is not correct, you should simply\nfail and return Invalid here.\n\n> +\t\t\tformat.mbus_code = data_->sensor_->mbusCodes().at(0);\n> +\n> +\t\t\tsensorConfig->bitDepth = format.bitsPerPixel();\n> +\t\t\tbitDepth = sensorConfig->bitDepth;\n> +\t\t\tstatus = Adjusted;\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> @@ -510,7 +530,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t */\n> \tstd::vector order(config_.size());\n> \tstd::iota(order.begin(), order.end(), 0);\n> -\tif (config_.size() == 2 && fitsAllPaths(config_[0]))\n> +\tif (config_.size() == 2 && fitsAllPaths(config_[0], bitDepth))\n> \t\tstd::reverse(order.begin(), order.end());\n>\n> \tbool mainPathAvailable = true;\n> @@ -521,7 +541,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, &tryCfg, bitDepth) == Valid) {\n> \t\t\t\tmainPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n> @@ -531,7 +551,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, &tryCfg, bitDepth) == Valid) {\n> \t\t\t\tselfPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n> @@ -542,7 +562,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, &tryCfg, bitDepth) == Adjusted) {\n> \t\t\t\tmainPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->mainPathStream_));\n> @@ -553,7 +573,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, &tryCfg, bitDepth) == Adjusted) {\n> \t\t\t\tselfPathAvailable = false;\n> \t\t\t\tcfg = tryCfg;\n> \t\t\t\tcfg.setStream(const_cast(&data_->selfPathStream_));\n> @@ -570,28 +590,75 @@ 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\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> \t\t\trawFormat = cfg.pixelFormat;\n> +\t\t\trawSize = cfg.size;\n> +\t\t}\n>\n> +\t\t/* path->validate binds this to a sensor-supported resolution */\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\nsensorConfig shall not be adjusted.\n\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> +\t} else if (!rawFormat.isValid() && sensorConfig) {\n> +\t\t/*\n> +\t\t * TODO Handle this properly taking into account the upscaling\n> +\t\t * capabilities and dual ISP mode (for the i.MX8MP).\n> +\t\t *\n> +\t\t * x1.5 should be a reasonable hardcoded ballpark for now.\n> +\t\t */\n> +\t\tdouble factor = 1.5;\n> +\t\tSize before = sensorConfig->outputSize;\n> +\t\tSize upscaleLimit = {\n> +\t\t\tstatic_cast(maxSize.width / factor),\n> +\t\t\tstatic_cast(maxSize.height / factor)\n> +\t\t};\n> +\n> +\t\tif (sensorConfig->outputSize < upscaleLimit)\n> +\t\t\tsensorConfig->outputSize = maxSize;\n> +\n> +\t\tif (before != sensorConfig->outputSize)\n> +\t\t\tstatus = Adjusted;\n> +\n> +\t\t/* No need to validate bpp for non-raw */\n> +\t}\n> +\n> \tstd::vector mbusCodes;\n> +\tSize size = maxSize;\n>\n> \tif (rawFormat.isValid()) {\n> \t\tmbusCodes = { rawFormats.at(rawFormat) };\n> +\t\tsize = 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} 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, size);\n>\n> +\t/* This should never happen if sensorConfig is valid */\n> \tif (sensorFormat_.size.isNull())\n> \t\tsensorFormat_.size = sensor->resolution();\n>\n> @@ -730,7 +797,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(), &format);\n> +\t} else {\n> +\t\tret = sensor->setFormat(&format, config->combinedTransform());\n> +\t}\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 eaab7e857..a598b289b 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -220,7 +220,8 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> }\n>\n> CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> -\t\t\t\t\t\t StreamConfiguration *cfg)\n> +\t\t\t\t\t\t StreamConfiguration *cfg,\n> +\t\t\t\t\t\t std::optional sensorBPP)\n> {\n> \tconst std::vector &mbusCodes = sensor->mbusCodes();\n> \tconst Size &resolution = sensor->resolution();\n> @@ -231,10 +232,15 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t/*\n> \t * Validate the pixel format. If the requested format isn't supported,\n> \t * default to either NV12 (all versions of the ISP are guaranteed to\n> -\t * support NV12 on both the main and self paths) if the requested format\n> -\t * is not a raw format, or to the supported raw format with the highest\n> -\t * bits per pixel otherwise.\n> +\t * support NV12 on both the main and self paths) if the requested\n> +\t * format is not a raw format, or otherwise to a supported raw format\n> +\t * with the same number of bits per pixel, or to maximum bits per pixel\n> +\t * if the same is not supported.\n> \t */\n> +\tconst PixelFormatInfo &formatInfo = PixelFormatInfo::info(cfg->pixelFormat);\n> +\tunsigned int reqRawBitsPerPixel = formatInfo.bitsPerPixel;\n\nWhat guarantees you that cfg is Raw here ?\n\n> +\tPixelFormat reqRawFormat;\n> +\n> \tunsigned int rawBitsPerPixel = 0;\n> \tPixelFormat rawFormat;\n> \tbool found = false;\n> @@ -250,12 +256,22 @@ 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\n> +\t\t\t * with it. Otherwise store the raw format with the\n> +\t\t\t * highest bits per pixel for later usage.\n> \t\t\t */\n> -\t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> -\t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> -\t\t\t\trawFormat = format;\n> +\t\t\tif (sensorBPP) {\n> +\t\t\t\tif (info.bitsPerPixel == sensorBPP)\n> +\t\t\t\t\trawFormat = format;\n> +\t\t\t} else {\n> +\t\t\t\tif (info.bitsPerPixel == reqRawBitsPerPixel)\n> +\t\t\t\t\treqRawFormat = format;\n> +\n> +\t\t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> +\t\t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> +\t\t\t\t\trawFormat = format;\n> +\t\t\t\t}\n\nI don't think this is the right level where to do this.\n\nIf a StreamConfiguration is RAW and you have a SensorConfiguration you\nshould adjust the StreamConfiguration to the size and to a PixelFormat\nwith the desired bitDepth, then you should validate it with the Path.\n\n> \t\t\t}\n> \t\t}\n>\n> @@ -265,6 +281,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t}\n> \t}\n>\n> +\tif (reqRawFormat.isValid())\n> +\t\trawFormat = reqRawFormat;\n> +\n> \tbool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==\n> \t\t PixelFormatInfo::ColourEncodingRAW;\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index c96bd5557..784bcea77 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -44,7 +44,8 @@ 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 StreamConfiguration *cfg);\n> +\t\t\t\t\t StreamConfiguration *cfg,\n> +\t\t\t\t\t std::optional sensorBPP);\n>\n> \tint configure(const StreamConfiguration &config,\n> \t\t const V4L2SubdeviceFormat &inputFormat);\n> --\n> 2.39.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 10C72C323E\n\tfor ;\n\tWed, 17 Jan 2024 16:37:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32BD7628B7;\n\tWed, 17 Jan 2024 17:37:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD734628AD\n\tfor ;\n\tWed, 17 Jan 2024 17:37:01 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 186E57EC;\n\tWed, 17 Jan 2024 17:35:52 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705509423;\n\tbh=f/hWdzuzsICHF3qdWOPQP98WNJP8MEkQEA32GYvE2tc=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=MgUpX9pw1MkJWkgXMOPJXh/1kx0iIgVG0CKIPsoWK7teEAaNQqalsLr4T0ypUOT+7\n\tnXM6WC8/1gMAfQX/n8fKxYy3uUmpiMC4AMobFj+WkfvZyv9aKdx0lz9lmV/OkHM4tv\n\tutcpXmmxki+EL27v/RE28cvhQS6Rf1BE/US/dEGy9DLZs9dFFOzQX35/tT4VG/RnVM\n\tQ/QYzGhmdVJ+yLC9YF8OVO6pEgtGIVcULicClkUIRMgw47+NH3DynBjcfu3K/U1KST\n\tgCwnv0rMbOqToWbxlvoWdYcxTytRL5Q+1UnLR4NuB/KbN2lVyo2byj7PEB8cSnTlyv\n\t7UgC4eobQ2t+g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1705509352;\n\tbh=f/hWdzuzsICHF3qdWOPQP98WNJP8MEkQEA32GYvE2tc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ByQu+DCOIL7JSZuRjbur/+3toXt2han3lPbhaq7fRO6/YpEvLdm9Q5nbm7Qt/XCs7\n\tMoCQN9RSn+XzkPe5Q28WLKnhwESGtCVX81iYmXD6QOrRnCbDL/obJBHtwZoPPPV0TF\n\tNZQI1BgyDGbLfJTEQigpk+o1WR3lXwazAjvbIl6c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ByQu+DCO\"; dkim-atps=neutral","Date":"Wed, 17 Jan 2024 17:36:58 +0100","To":"Paul Elder ","Message-ID":"","References":"<20240116091754.100654-1-paul.elder@ideasonboard.com>\n\t<20240116091754.100654-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240116091754.100654-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/3] pipeline: rkisp1: Plumb\n\tSensorConfiguration","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]