[{"id":32095,"web_url":"https://patchwork.libcamera.org/comment/32095/","msgid":"","date":"2024-11-11T09:30:59","subject":"Re: [PATCH v5 10/13] libcamera: mali-c55: Correct input/output\n\tformat representation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Dan\n I would mention RAW somewhere in the commit message\n\nOn Thu, Nov 07, 2024 at 10:58:43AM +0000, Daniel Scally wrote:\n> At present we configure raw streams by looping through the pixel\n> formats we support and finding one with an associated media bus\n> format code that the sensor can produce. In the new representation\n> of raw data from the kernel driver this will not work - the sensor\n> could produce 8, 10, 12, 14 or 16 bit data and the ISP will force\n> it to RAW16, which is the only actually supported output.\n>\n> To fix the issue move to simply finding a pixel format with a bayer\n> order that matches that of the media bus format produced by the\n> sensor. If the sensor can produce multiple formats with the same\n> bayer order use the one with the largest bitdepth.\n>\n> Finally, remove the claim to support RAW formats of less than 16 bits.\n>\n> Reviewed-by: Kieran Bingham \n> Signed-off-by: Daniel Scally \n> ---\n> Changes in v5:\n>\n> \t- Dropped the Fatal log.\n>\n> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 128 ++++++++++++-------\n> 1 file changed, 80 insertions(+), 48 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> index 156560c1..97827abd 100644\n> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> @@ -57,22 +57,6 @@ const std::map maliC55FmtToCode = {\n> \t{ formats::NV21, MEDIA_BUS_FMT_YUV10_1X30 },\n>\n> \t/* RAW formats, FR pipe only. */\n> -\t{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },\n> -\t{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },\n> -\t{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },\n> -\t{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },\n> -\t{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },\n> -\t{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },\n> -\t{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },\n> -\t{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },\n> -\t{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },\n> -\t{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },\n> -\t{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },\n> -\t{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },\n> -\t{ formats::SGBRG14, MEDIA_BUS_FMT_SGBRG14_1X14 },\n> -\t{ formats::SRGGB14, MEDIA_BUS_FMT_SRGGB14_1X14 },\n> -\t{ formats::SBGGR14, MEDIA_BUS_FMT_SBGGR14_1X14 },\n> -\t{ formats::SGRBG14, MEDIA_BUS_FMT_SGRBG14_1X14 },\n> \t{ formats::SGBRG16, MEDIA_BUS_FMT_SGBRG16_1X16 },\n> \t{ formats::SRGGB16, MEDIA_BUS_FMT_SRGGB16_1X16 },\n> \t{ formats::SBGGR16, MEDIA_BUS_FMT_SBGGR16_1X16 },\n> @@ -98,7 +82,8 @@ public:\n> \tconst std::vector sizes(unsigned int mbusCode) const;\n> \tconst Size resolution() const;\n>\n> -\tPixelFormat bestRawFormat() const;\n> +\tint pixfmtToMbusCode(const PixelFormat &pixFmt) const;\n> +\tconst PixelFormat &bestRawFormat() const;\n>\n> \tPixelFormat adjustRawFormat(const PixelFormat &pixFmt) const;\n> \tSize adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const;\n> @@ -208,33 +193,75 @@ const Size MaliC55CameraData::resolution() const\n> \treturn tpgResolution_;\n> }\n>\n> -PixelFormat MaliC55CameraData::bestRawFormat() const\n> +/*\n> + * The Mali C55 ISP can only produce 16-bit RAW output in bypass modes, but the\n> + * sensors connected to it might produce 8/10/12/16 bits. We simply search the\n> + * sensor's supported formats for the one with a matching bayer order and the\n> + * greatest bitdepth.\n> + */\n> +int MaliC55CameraData::pixfmtToMbusCode(const PixelFormat &pixFmt) const\n> {\n> +\tauto it = maliC55FmtToCode.find(pixFmt);\n> +\tif (it == maliC55FmtToCode.end())\n> +\t\treturn -EINVAL;\n> +\n> +\tBayerFormat bayerFormat = BayerFormat::fromMbusCode(it->second);\n> +\tif (!bayerFormat.isValid())\n> +\t\treturn -EINVAL;\n\nI don't think this can't happen, but I'm fine keeping it here to\nprotect against extensions of the maliC55FmtToCode map\n\n> +\n> +\tV4L2Subdevice::Formats formats = sd_->formats(0);\n> +\tunsigned int sensorMbusCode = 0;\n> \tunsigned int bitDepth = 0;\n> -\tPixelFormat rawFormat;\n>\n> -\t/*\n> -\t * Iterate over all the supported PixelFormat and find the one\n> -\t * supported by the camera with the largest bitdepth.\n> -\t */\n> -\tfor (const auto &maliFormat : maliC55FmtToCode) {\n> -\t\tPixelFormat pixFmt = maliFormat.first;\n> -\t\tif (!isFormatRaw(pixFmt))\n> +\tfor (const auto &[code, sizes] : formats) {\n> +\t\tBayerFormat sdBayerFormat = BayerFormat::fromMbusCode(code);\n> +\t\tif (!sdBayerFormat.isValid())\n> \t\t\tcontinue;\n>\n> -\t\tunsigned int rawCode = maliFormat.second;\n> -\t\tconst auto rawSizes = sizes(rawCode);\n> -\t\tif (rawSizes.empty())\n> +\t\tif (sdBayerFormat.order != bayerFormat.order)\n> \t\t\tcontinue;\n>\n> -\t\tBayerFormat bayer = BayerFormat::fromMbusCode(rawCode);\n> -\t\tif (bayer.bitDepth > bitDepth) {\n> -\t\t\tbitDepth = bayer.bitDepth;\n> -\t\t\trawFormat = pixFmt;\n> +\t\tif (sdBayerFormat.bitDepth > bitDepth) {\n> +\t\t\tbitDepth = sdBayerFormat.bitDepth;\n> +\t\t\tsensorMbusCode = code;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!sensorMbusCode)\n> +\t\treturn -EINVAL;\n> +\n> +\treturn sensorMbusCode;\n> +}\n> +\n> +/*\n> + * Find a RAW PixelFormat supported by both the ISP and the sensor.\n> + *\n> + * The situation is mildly complicated by the fact that we expect the sensor to\n> + * output something like RAW8/10/12/16, but the ISP can only accept as input\n> + * RAW20 and can only produce as output RAW16. The one constant in that is the\n> + * bayer order of the data, so we'll simply check that the sensor produces a\n> + * format with a bayer order that matches that of one of the formats we support,\n> + * and select that.\n> + */\n> +const PixelFormat &MaliC55CameraData::bestRawFormat() const\n> +{\n> +\tstatic const PixelFormat invalidPixFmt = {};\n> +\n> +\tfor (const auto &fmt : sd_->formats(0)) {\n> +\t\tBayerFormat sensorBayer = BayerFormat::fromMbusCode(fmt.first);\n\nAs this comes from the sensor, here I would\n\n\tif (!bayerFormat.isValid())\n\t\tcontinue;\n\n> +\n> +\t\tfor (const auto &[pixFmt, rawCode] : maliC55FmtToCode) {\n> +\t\t\tif (!isFormatRaw(pixFmt))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tBayerFormat bayer = BayerFormat::fromMbusCode(rawCode);\n> +\t\t\tif (bayer.order == sensorBayer.order)\n> +\t\t\t\treturn pixFmt;\n> \t\t}\n> \t}\n>\n> -\treturn rawFormat;\n> +\tLOG(MaliC55, Error) << \"Sensor doesn't provide a compatible format\";\n> +\treturn invalidPixFmt;\n> }\n>\n> /*\n> @@ -243,13 +270,11 @@ PixelFormat MaliC55CameraData::bestRawFormat() const\n> */\n> PixelFormat MaliC55CameraData::adjustRawFormat(const PixelFormat &rawFmt) const\n> {\n> -\t/* Make sure the provided raw format is supported by the pipeline. */\n> -\tauto it = maliC55FmtToCode.find(rawFmt);\n> -\tif (it == maliC55FmtToCode.end())\n> +\t/* Make sure the RAW mbus code is supported by the image source. */\n> +\tint rawCode = pixfmtToMbusCode(rawFmt);\n> +\tif (rawCode < 0)\n> \t\treturn bestRawFormat();\n>\n> -\t/* Now make sure the RAW mbus code is supported by the image source. */\n> -\tunsigned int rawCode = it->second;\n> \tconst auto rawSizes = sizes(rawCode);\n> \tif (rawSizes.empty())\n> \t\treturn bestRawFormat();\n> @@ -259,16 +284,14 @@ PixelFormat MaliC55CameraData::adjustRawFormat(const PixelFormat &rawFmt) const\n>\n> Size MaliC55CameraData::adjustRawSizes(const PixelFormat &rawFmt, const Size &size) const\n> {\n> -\t/* Just make sure the format is supported. */\n> -\tauto it = maliC55FmtToCode.find(rawFmt);\n> -\tif (it == maliC55FmtToCode.end())\n> -\t\treturn {};\n> -\n> \t/* Expand the RAW size to the minimum ISP input size. */\n> \tSize rawSize = size.expandedTo(kMaliC55MinInputSize);\n>\n> \t/* Check if the size is natively supported. */\n> -\tunsigned int rawCode = it->second;\n> +\tint rawCode = pixfmtToMbusCode(rawFmt);\n> +\tif (rawCode < 0)\n> +\t\treturn {};\n> +\n> \tconst auto rawSizes = sizes(rawCode);\n> \tauto sizeIt = std::find(rawSizes.begin(), rawSizes.end(), rawSize);\n> \tif (sizeIt != rawSizes.end())\n> @@ -349,6 +372,10 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n> \t\t */\n> \t\tPixelFormat rawFormat =\n> \t\t\tdata_->adjustRawFormat(rawConfig->pixelFormat);\n> +\n> +\t\tif (!rawFormat.isValid())\n> +\t\t\treturn Invalid;\n> +\n> \t\tif (rawFormat != rawConfig->pixelFormat) {\n> \t\t\tLOG(MaliC55, Debug)\n> \t\t\t\t<< \"RAW format adjusted to \" << rawFormat;\n> @@ -418,8 +445,7 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n>\n> \t/* If there's a RAW config, sensor configuration follows it. */\n> \tif (rawConfig) {\n> -\t\tconst auto it = maliC55FmtToCode.find(rawConfig->pixelFormat);\n> -\t\tsensorFormat_.code = it->second;\n> +\t\tsensorFormat_.code = data_->pixfmtToMbusCode(rawConfig->pixelFormat);\n> \t\tsensorFormat_.size = rawConfig->size.expandedTo(minSensorSize);\n>\n> \t\treturn status;\n> @@ -427,6 +453,9 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n>\n> \t/* If there's no RAW config, compute the sensor configuration here. */\n> \tPixelFormat rawFormat = data_->bestRawFormat();\n> +\tif (!rawFormat.isValid())\n> +\t\treturn Invalid;\n> +\n\nIs this correct now ?\n\nbestRawFormat() will always return a 16bpp PixelFormat, as it extracts\nfrom maliC55FmtToCode.\n\n> \tconst auto it = maliC55FmtToCode.find(rawFormat);\n\nAnd here we get back a 16bpp mbus code\n\n> \tsensorFormat_.code = it->second;\n\nA 16bpp mbus code is not guaranteed to be supported by the sensor, as\nthe 16bpp PixelFormat can be generated from any 8,12,14 bpp mbus code\n(you only make sure the component ordering matches, in\nbestRawFormat()).\n\nOne option would be to pass an &mbusCode argument to bestRawFormat()\nto optionally return what sensor mbus code generates the 16bpp pixel\nformat.\n\nThe rest looks good!\n\n>\n> @@ -614,7 +643,10 @@ PipelineHandlerMaliC55::generateConfiguration(Camera *camera,\n>\n> \t\t\tif (isRaw) {\n> \t\t\t\t/* Make sure the mbus code is supported. */\n> -\t\t\t\tunsigned int rawCode = maliFormat.second;\n> +\t\t\t\tint rawCode = data->pixfmtToMbusCode(pixFmt);\n> +\t\t\t\tif (rawCode < 0)\n> +\t\t\t\t\tcontinue;\n> +\n> \t\t\t\tconst auto sizes = data->sizes(rawCode);\n> \t\t\t\tif (sizes.empty())\n> \t\t\t\t\tcontinue;\n> --\n> 2.30.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 DA765BF415\n\tfor ;\n\tMon, 11 Nov 2024 09:31:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 757B0657CC;\n\tMon, 11 Nov 2024 10:31:04 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1452C657C5\n\tfor ;\n\tMon, 11 Nov 2024 10:31:03 +0100 (CET)","from ideasonboard.com (mob-5-90-143-109.net.vodafone.it\n\t[5.90.143.109])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 644AB6AF;\n\tMon, 11 Nov 2024 10:30:51 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tXrSyjfn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731317451;\n\tbh=IK4VGePVltm8PcLTYSyfaJZwlFN5FBxELmkU5dKH6Lc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tXrSyjfnYXfWtuI3d/ImHjiLLeR363unJIN9SFkqWvCkOgD9+VC9vmQeqpDuC3YhC\n\tL1AmyzVym8uY7w2d5jFl7lqdpTZVZUA2YOS3P62vtR+yuzzE8Opel9gtY2EHRIl2VE\n\tmljZNQ6wlhzEy+mifAT+voSooft9kT7hnmcnN6GI=","Date":"Mon, 11 Nov 2024 10:30:59 +0100","From":"Jacopo Mondi ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org, \n\tKieran Bingham ","Subject":"Re: [PATCH v5 10/13] libcamera: mali-c55: Correct input/output\n\tformat representation","Message-ID":"","References":"<20241107105846.52287-1-dan.scally@ideasonboard.com>\n\t<20241107105846.52287-11-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241107105846.52287-11-dan.scally@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":32165,"web_url":"https://patchwork.libcamera.org/comment/32165/","msgid":"","date":"2024-11-14T10:15:04","subject":"Re: [PATCH v5 10/13] libcamera: mali-c55: Correct input/output\n\tformat representation","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 11/11/2024 09:30, Jacopo Mondi wrote:\n> Hi Dan\n> I would mention RAW somewhere in the commit message\nI thought that\n>\n> On Thu, Nov 07, 2024 at 10:58:43AM +0000, Daniel Scally wrote:\n>> At present we configure raw streams by looping through the pixel\n>> formats we support and finding one with an associated media bus\n>> format code that the sensor can produce. In the new representation\n>> of raw data from the kernel driver this will not work - the sensor\n>> could produce 8, 10, 12, 14 or 16 bit data and the ISP will force\n>> it to RAW16, which is the only actually supported output.\n>>\n>> To fix the issue move to simply finding a pixel format with a bayer\n>> order that matches that of the media bus format produced by the\n>> sensor. If the sensor can produce multiple formats with the same\n>> bayer order use the one with the largest bitdepth.\n>>\n>> Finally, remove the claim to support RAW formats of less than 16 bits.\n>>\n>> Reviewed-by: Kieran Bingham \n>> Signed-off-by: Daniel Scally \n>> ---\n>> Changes in v5:\n>>\n>> \t- Dropped the Fatal log.\n>>\n>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 128 ++++++++++++-------\n>> 1 file changed, 80 insertions(+), 48 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n>> index 156560c1..97827abd 100644\n>> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n>> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n>> @@ -57,22 +57,6 @@ const std::map maliC55FmtToCode = {\n>> \t{ formats::NV21, MEDIA_BUS_FMT_YUV10_1X30 },\n>>\n>> \t/* RAW formats, FR pipe only. */\n>> -\t{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },\n>> -\t{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },\n>> -\t{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },\n>> -\t{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },\n>> -\t{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },\n>> -\t{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },\n>> -\t{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },\n>> -\t{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },\n>> -\t{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },\n>> -\t{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },\n>> -\t{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },\n>> -\t{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },\n>> -\t{ formats::SGBRG14, MEDIA_BUS_FMT_SGBRG14_1X14 },\n>> -\t{ formats::SRGGB14, MEDIA_BUS_FMT_SRGGB14_1X14 },\n>> -\t{ formats::SBGGR14, MEDIA_BUS_FMT_SBGGR14_1X14 },\n>> -\t{ formats::SGRBG14, MEDIA_BUS_FMT_SGRBG14_1X14 },\n>> \t{ formats::SGBRG16, MEDIA_BUS_FMT_SGBRG16_1X16 },\n>> \t{ formats::SRGGB16, MEDIA_BUS_FMT_SRGGB16_1X16 },\n>> \t{ formats::SBGGR16, MEDIA_BUS_FMT_SBGGR16_1X16 },\n>> @@ -98,7 +82,8 @@ public:\n>> \tconst std::vector sizes(unsigned int mbusCode) const;\n>> \tconst Size resolution() const;\n>>\n>> -\tPixelFormat bestRawFormat() const;\n>> +\tint pixfmtToMbusCode(const PixelFormat &pixFmt) const;\n>> +\tconst PixelFormat &bestRawFormat() const;\n>>\n>> \tPixelFormat adjustRawFormat(const PixelFormat &pixFmt) const;\n>> \tSize adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const;\n>> @@ -208,33 +193,75 @@ const Size MaliC55CameraData::resolution() const\n>> \treturn tpgResolution_;\n>> }\n>>\n>> -PixelFormat MaliC55CameraData::bestRawFormat() const\n>> +/*\n>> + * The Mali C55 ISP can only produce 16-bit RAW output in bypass modes, but the\n>> + * sensors connected to it might produce 8/10/12/16 bits. We simply search the\n>> + * sensor's supported formats for the one with a matching bayer order and the\n>> + * greatest bitdepth.\n>> + */\n>> +int MaliC55CameraData::pixfmtToMbusCode(const PixelFormat &pixFmt) const\n>> {\n>> +\tauto it = maliC55FmtToCode.find(pixFmt);\n>> +\tif (it == maliC55FmtToCode.end())\n>> +\t\treturn -EINVAL;\n>> +\n>> +\tBayerFormat bayerFormat = BayerFormat::fromMbusCode(it->second);\n>> +\tif (!bayerFormat.isValid())\n>> +\t\treturn -EINVAL;\n> I don't think this can't happen, but I'm fine keeping it here to\n> protect against extensions of the maliC55FmtToCode map\n>\n>> +\n>> +\tV4L2Subdevice::Formats formats = sd_->formats(0);\n>> +\tunsigned int sensorMbusCode = 0;\n>> \tunsigned int bitDepth = 0;\n>> -\tPixelFormat rawFormat;\n>>\n>> -\t/*\n>> -\t * Iterate over all the supported PixelFormat and find the one\n>> -\t * supported by the camera with the largest bitdepth.\n>> -\t */\n>> -\tfor (const auto &maliFormat : maliC55FmtToCode) {\n>> -\t\tPixelFormat pixFmt = maliFormat.first;\n>> -\t\tif (!isFormatRaw(pixFmt))\n>> +\tfor (const auto &[code, sizes] : formats) {\n>> +\t\tBayerFormat sdBayerFormat = BayerFormat::fromMbusCode(code);\n>> +\t\tif (!sdBayerFormat.isValid())\n>> \t\t\tcontinue;\n>>\n>> -\t\tunsigned int rawCode = maliFormat.second;\n>> -\t\tconst auto rawSizes = sizes(rawCode);\n>> -\t\tif (rawSizes.empty())\n>> +\t\tif (sdBayerFormat.order != bayerFormat.order)\n>> \t\t\tcontinue;\n>>\n>> -\t\tBayerFormat bayer = BayerFormat::fromMbusCode(rawCode);\n>> -\t\tif (bayer.bitDepth > bitDepth) {\n>> -\t\t\tbitDepth = bayer.bitDepth;\n>> -\t\t\trawFormat = pixFmt;\n>> +\t\tif (sdBayerFormat.bitDepth > bitDepth) {\n>> +\t\t\tbitDepth = sdBayerFormat.bitDepth;\n>> +\t\t\tsensorMbusCode = code;\n>> +\t\t}\n>> +\t}\n>> +\n>> +\tif (!sensorMbusCode)\n>> +\t\treturn -EINVAL;\n>> +\n>> +\treturn sensorMbusCode;\n>> +}\n>> +\n>> +/*\n>> + * Find a RAW PixelFormat supported by both the ISP and the sensor.\n>> + *\n>> + * The situation is mildly complicated by the fact that we expect the sensor to\n>> + * output something like RAW8/10/12/16, but the ISP can only accept as input\n>> + * RAW20 and can only produce as output RAW16. The one constant in that is the\n>> + * bayer order of the data, so we'll simply check that the sensor produces a\n>> + * format with a bayer order that matches that of one of the formats we support,\n>> + * and select that.\n>> + */\n>> +const PixelFormat &MaliC55CameraData::bestRawFormat() const\n>> +{\n>> +\tstatic const PixelFormat invalidPixFmt = {};\n>> +\n>> +\tfor (const auto &fmt : sd_->formats(0)) {\n>> +\t\tBayerFormat sensorBayer = BayerFormat::fromMbusCode(fmt.first);\n> As this comes from the sensor, here I would\n>\n> \tif (!bayerFormat.isValid())\n> \t\tcontinue;\n>\n>> +\n>> +\t\tfor (const auto &[pixFmt, rawCode] : maliC55FmtToCode) {\n>> +\t\t\tif (!isFormatRaw(pixFmt))\n>> +\t\t\t\tcontinue;\n>> +\n>> +\t\t\tBayerFormat bayer = BayerFormat::fromMbusCode(rawCode);\n>> +\t\t\tif (bayer.order == sensorBayer.order)\n>> +\t\t\t\treturn pixFmt;\n>> \t\t}\n>> \t}\n>>\n>> -\treturn rawFormat;\n>> +\tLOG(MaliC55, Error) << \"Sensor doesn't provide a compatible format\";\n>> +\treturn invalidPixFmt;\n>> }\n>>\n>> /*\n>> @@ -243,13 +270,11 @@ PixelFormat MaliC55CameraData::bestRawFormat() const\n>> */\n>> PixelFormat MaliC55CameraData::adjustRawFormat(const PixelFormat &rawFmt) const\n>> {\n>> -\t/* Make sure the provided raw format is supported by the pipeline. */\n>> -\tauto it = maliC55FmtToCode.find(rawFmt);\n>> -\tif (it == maliC55FmtToCode.end())\n>> +\t/* Make sure the RAW mbus code is supported by the image source. */\n>> +\tint rawCode = pixfmtToMbusCode(rawFmt);\n>> +\tif (rawCode < 0)\n>> \t\treturn bestRawFormat();\n>>\n>> -\t/* Now make sure the RAW mbus code is supported by the image source. */\n>> -\tunsigned int rawCode = it->second;\n>> \tconst auto rawSizes = sizes(rawCode);\n>> \tif (rawSizes.empty())\n>> \t\treturn bestRawFormat();\n>> @@ -259,16 +284,14 @@ PixelFormat MaliC55CameraData::adjustRawFormat(const PixelFormat &rawFmt) const\n>>\n>> Size MaliC55CameraData::adjustRawSizes(const PixelFormat &rawFmt, const Size &size) const\n>> {\n>> -\t/* Just make sure the format is supported. */\n>> -\tauto it = maliC55FmtToCode.find(rawFmt);\n>> -\tif (it == maliC55FmtToCode.end())\n>> -\t\treturn {};\n>> -\n>> \t/* Expand the RAW size to the minimum ISP input size. */\n>> \tSize rawSize = size.expandedTo(kMaliC55MinInputSize);\n>>\n>> \t/* Check if the size is natively supported. */\n>> -\tunsigned int rawCode = it->second;\n>> +\tint rawCode = pixfmtToMbusCode(rawFmt);\n>> +\tif (rawCode < 0)\n>> +\t\treturn {};\n>> +\n>> \tconst auto rawSizes = sizes(rawCode);\n>> \tauto sizeIt = std::find(rawSizes.begin(), rawSizes.end(), rawSize);\n>> \tif (sizeIt != rawSizes.end())\n>> @@ -349,6 +372,10 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n>> \t\t */\n>> \t\tPixelFormat rawFormat =\n>> \t\t\tdata_->adjustRawFormat(rawConfig->pixelFormat);\n>> +\n>> +\t\tif (!rawFormat.isValid())\n>> +\t\t\treturn Invalid;\n>> +\n>> \t\tif (rawFormat != rawConfig->pixelFormat) {\n>> \t\t\tLOG(MaliC55, Debug)\n>> \t\t\t\t<< \"RAW format adjusted to \" << rawFormat;\n>> @@ -418,8 +445,7 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n>>\n>> \t/* If there's a RAW config, sensor configuration follows it. */\n>> \tif (rawConfig) {\n>> -\t\tconst auto it = maliC55FmtToCode.find(rawConfig->pixelFormat);\n>> -\t\tsensorFormat_.code = it->second;\n>> +\t\tsensorFormat_.code = data_->pixfmtToMbusCode(rawConfig->pixelFormat);\n>> \t\tsensorFormat_.size = rawConfig->size.expandedTo(minSensorSize);\n>>\n>> \t\treturn status;\n>> @@ -427,6 +453,9 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n>>\n>> \t/* If there's no RAW config, compute the sensor configuration here. */\n>> \tPixelFormat rawFormat = data_->bestRawFormat();\n>> +\tif (!rawFormat.isValid())\n>> +\t\treturn Invalid;\n>> +\n> Is this correct now ?\n>\n> bestRawFormat() will always return a 16bpp PixelFormat, as it extracts\n> from maliC55FmtToCode.\nIt's unnecessary - but previously I had a Fatal log in bestRawFormat() and Kieran asked for it to be \ndowngraded to an Error - I added the check at that point as I feel uneasy at allowing the assumption \nthat it's correct.\n>\n>> \tconst auto it = maliC55FmtToCode.find(rawFormat);\n> And here we get back a 16bpp mbus code\n>\n>> \tsensorFormat_.code = it->second;\n> A 16bpp mbus code is not guaranteed to be supported by the sensor, as\n> the 16bpp PixelFormat can be generated from any 8,12,14 bpp mbus code\n> (you only make sure the component ordering matches, in\n> bestRawFormat()).\n>\n> One option would be to pass an &mbusCode argument to bestRawFormat()\n> to optionally return what sensor mbus code generates the 16bpp pixel\n> format.\n\n\nAh, good spot. I think it needs a call to pixfmtToMbusCode() here which finds the highest bitdepth \nequivalent supported by the sensor.\n\n\nThanks!\n\n>\n> The rest looks good!\n>\n>> @@ -614,7 +643,10 @@ PipelineHandlerMaliC55::generateConfiguration(Camera *camera,\n>>\n>> \t\t\tif (isRaw) {\n>> \t\t\t\t/* Make sure the mbus code is supported. */\n>> -\t\t\t\tunsigned int rawCode = maliFormat.second;\n>> +\t\t\t\tint rawCode = data->pixfmtToMbusCode(pixFmt);\n>> +\t\t\t\tif (rawCode < 0)\n>> +\t\t\t\t\tcontinue;\n>> +\n>> \t\t\t\tconst auto sizes = data->sizes(rawCode);\n>> \t\t\t\tif (sizes.empty())\n>> \t\t\t\t\tcontinue;\n>> --\n>> 2.30.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 89267C0F1B\n\tfor ;\n\tThu, 14 Nov 2024 10:15:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C3C865837;\n\tThu, 14 Nov 2024 11:15:10 +0100 (CET)","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 4617E657DA\n\tfor ;\n\tThu, 14 Nov 2024 11:15:08 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1026D502;\n\tThu, 14 Nov 2024 11:14:53 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FMMgoNit\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731579294;\n\tbh=hy35ZBMtDxlA52HopnYNl+PlfJ9rH2j7L7oUcb03pHE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=FMMgoNitTFqulByQrBaO23XoW9s8Iv3+M//NVfFrDKt5p9n+IBigcs5KDIPjhMZQC\n\trwSYVGa1JWmsKpjnvC4/7T+FPXCRxSnuDyz4BpNWPXc5A4QIR1NASXo6emt6Mn8Yhz\n\t3UmRTWUwdcsZj7ny0FbG7oTUJr1x5X2LEDBwweUs=","Message-ID":"","Date":"Thu, 14 Nov 2024 10:15:04 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 10/13] libcamera: mali-c55: Correct input/output\n\tformat representation","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","References":"<20241107105846.52287-1-dan.scally@ideasonboard.com>\n\t<20241107105846.52287-11-dan.scally@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","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":32167,"web_url":"https://patchwork.libcamera.org/comment/32167/","msgid":"","date":"2024-11-14T10:23:17","subject":"Re: [PATCH v5 10/13] libcamera: mali-c55: Correct input/output\n\tformat representation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Dan\n\nOn Thu, Nov 14, 2024 at 10:15:04AM +0000, Dan Scally wrote:\n> Hi Jacopo\n>\n> On 11/11/2024 09:30, Jacopo Mondi wrote:\n> > Hi Dan\n> > I would mention RAW somewhere in the commit message\n> I thought that\n> >\n> > On Thu, Nov 07, 2024 at 10:58:43AM +0000, Daniel Scally wrote:\n> > > At present we configure raw streams by looping through the pixel\n> > > formats we support and finding one with an associated media bus\n> > > format code that the sensor can produce. In the new representation\n> > > of raw data from the kernel driver this will not work - the sensor\n> > > could produce 8, 10, 12, 14 or 16 bit data and the ISP will force\n> > > it to RAW16, which is the only actually supported output.\n> > >\n> > > To fix the issue move to simply finding a pixel format with a bayer\n> > > order that matches that of the media bus format produced by the\n> > > sensor. If the sensor can produce multiple formats with the same\n> > > bayer order use the one with the largest bitdepth.\n> > >\n> > > Finally, remove the claim to support RAW formats of less than 16 bits.\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > > Signed-off-by: Daniel Scally \n> > > ---\n> > > Changes in v5:\n> > >\n> > > \t- Dropped the Fatal log.\n> > >\n> > > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 128 ++++++++++++-------\n> > > 1 file changed, 80 insertions(+), 48 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > > index 156560c1..97827abd 100644\n> > > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp\n> > > @@ -57,22 +57,6 @@ const std::map maliC55FmtToCode = {\n> > > \t{ formats::NV21, MEDIA_BUS_FMT_YUV10_1X30 },\n> > >\n> > > \t/* RAW formats, FR pipe only. */\n> > > -\t{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },\n> > > -\t{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },\n> > > -\t{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },\n> > > -\t{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },\n> > > -\t{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },\n> > > -\t{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },\n> > > -\t{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },\n> > > -\t{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },\n> > > -\t{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },\n> > > -\t{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },\n> > > -\t{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },\n> > > -\t{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },\n> > > -\t{ formats::SGBRG14, MEDIA_BUS_FMT_SGBRG14_1X14 },\n> > > -\t{ formats::SRGGB14, MEDIA_BUS_FMT_SRGGB14_1X14 },\n> > > -\t{ formats::SBGGR14, MEDIA_BUS_FMT_SBGGR14_1X14 },\n> > > -\t{ formats::SGRBG14, MEDIA_BUS_FMT_SGRBG14_1X14 },\n> > > \t{ formats::SGBRG16, MEDIA_BUS_FMT_SGBRG16_1X16 },\n> > > \t{ formats::SRGGB16, MEDIA_BUS_FMT_SRGGB16_1X16 },\n> > > \t{ formats::SBGGR16, MEDIA_BUS_FMT_SBGGR16_1X16 },\n> > > @@ -98,7 +82,8 @@ public:\n> > > \tconst std::vector sizes(unsigned int mbusCode) const;\n> > > \tconst Size resolution() const;\n> > >\n> > > -\tPixelFormat bestRawFormat() const;\n> > > +\tint pixfmtToMbusCode(const PixelFormat &pixFmt) const;\n> > > +\tconst PixelFormat &bestRawFormat() const;\n> > >\n> > > \tPixelFormat adjustRawFormat(const PixelFormat &pixFmt) const;\n> > > \tSize adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const;\n> > > @@ -208,33 +193,75 @@ const Size MaliC55CameraData::resolution() const\n> > > \treturn tpgResolution_;\n> > > }\n> > >\n> > > -PixelFormat MaliC55CameraData::bestRawFormat() const\n> > > +/*\n> > > + * The Mali C55 ISP can only produce 16-bit RAW output in bypass modes, but the\n> > > + * sensors connected to it might produce 8/10/12/16 bits. We simply search the\n> > > + * sensor's supported formats for the one with a matching bayer order and the\n> > > + * greatest bitdepth.\n> > > + */\n> > > +int MaliC55CameraData::pixfmtToMbusCode(const PixelFormat &pixFmt) const\n> > > {\n> > > +\tauto it = maliC55FmtToCode.find(pixFmt);\n> > > +\tif (it == maliC55FmtToCode.end())\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tBayerFormat bayerFormat = BayerFormat::fromMbusCode(it->second);\n> > > +\tif (!bayerFormat.isValid())\n> > > +\t\treturn -EINVAL;\n> > I don't think this can't happen, but I'm fine keeping it here to\n> > protect against extensions of the maliC55FmtToCode map\n> >\n> > > +\n> > > +\tV4L2Subdevice::Formats formats = sd_->formats(0);\n> > > +\tunsigned int sensorMbusCode = 0;\n> > > \tunsigned int bitDepth = 0;\n> > > -\tPixelFormat rawFormat;\n> > >\n> > > -\t/*\n> > > -\t * Iterate over all the supported PixelFormat and find the one\n> > > -\t * supported by the camera with the largest bitdepth.\n> > > -\t */\n> > > -\tfor (const auto &maliFormat : maliC55FmtToCode) {\n> > > -\t\tPixelFormat pixFmt = maliFormat.first;\n> > > -\t\tif (!isFormatRaw(pixFmt))\n> > > +\tfor (const auto &[code, sizes] : formats) {\n> > > +\t\tBayerFormat sdBayerFormat = BayerFormat::fromMbusCode(code);\n> > > +\t\tif (!sdBayerFormat.isValid())\n> > > \t\t\tcontinue;\n> > >\n> > > -\t\tunsigned int rawCode = maliFormat.second;\n> > > -\t\tconst auto rawSizes = sizes(rawCode);\n> > > -\t\tif (rawSizes.empty())\n> > > +\t\tif (sdBayerFormat.order != bayerFormat.order)\n> > > \t\t\tcontinue;\n> > >\n> > > -\t\tBayerFormat bayer = BayerFormat::fromMbusCode(rawCode);\n> > > -\t\tif (bayer.bitDepth > bitDepth) {\n> > > -\t\t\tbitDepth = bayer.bitDepth;\n> > > -\t\t\trawFormat = pixFmt;\n> > > +\t\tif (sdBayerFormat.bitDepth > bitDepth) {\n> > > +\t\t\tbitDepth = sdBayerFormat.bitDepth;\n> > > +\t\t\tsensorMbusCode = code;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tif (!sensorMbusCode)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\treturn sensorMbusCode;\n> > > +}\n> > > +\n> > > +/*\n> > > + * Find a RAW PixelFormat supported by both the ISP and the sensor.\n> > > + *\n> > > + * The situation is mildly complicated by the fact that we expect the sensor to\n> > > + * output something like RAW8/10/12/16, but the ISP can only accept as input\n> > > + * RAW20 and can only produce as output RAW16. The one constant in that is the\n> > > + * bayer order of the data, so we'll simply check that the sensor produces a\n> > > + * format with a bayer order that matches that of one of the formats we support,\n> > > + * and select that.\n> > > + */\n> > > +const PixelFormat &MaliC55CameraData::bestRawFormat() const\n> > > +{\n> > > +\tstatic const PixelFormat invalidPixFmt = {};\n> > > +\n> > > +\tfor (const auto &fmt : sd_->formats(0)) {\n> > > +\t\tBayerFormat sensorBayer = BayerFormat::fromMbusCode(fmt.first);\n> > As this comes from the sensor, here I would\n> >\n> > \tif (!bayerFormat.isValid())\n> > \t\tcontinue;\n> >\n> > > +\n> > > +\t\tfor (const auto &[pixFmt, rawCode] : maliC55FmtToCode) {\n> > > +\t\t\tif (!isFormatRaw(pixFmt))\n> > > +\t\t\t\tcontinue;\n> > > +\n> > > +\t\t\tBayerFormat bayer = BayerFormat::fromMbusCode(rawCode);\n> > > +\t\t\tif (bayer.order == sensorBayer.order)\n> > > +\t\t\t\treturn pixFmt;\n> > > \t\t}\n> > > \t}\n> > >\n> > > -\treturn rawFormat;\n> > > +\tLOG(MaliC55, Error) << \"Sensor doesn't provide a compatible format\";\n> > > +\treturn invalidPixFmt;\n> > > }\n> > >\n> > > /*\n> > > @@ -243,13 +270,11 @@ PixelFormat MaliC55CameraData::bestRawFormat() const\n> > > */\n> > > PixelFormat MaliC55CameraData::adjustRawFormat(const PixelFormat &rawFmt) const\n> > > {\n> > > -\t/* Make sure the provided raw format is supported by the pipeline. */\n> > > -\tauto it = maliC55FmtToCode.find(rawFmt);\n> > > -\tif (it == maliC55FmtToCode.end())\n> > > +\t/* Make sure the RAW mbus code is supported by the image source. */\n> > > +\tint rawCode = pixfmtToMbusCode(rawFmt);\n> > > +\tif (rawCode < 0)\n> > > \t\treturn bestRawFormat();\n> > >\n> > > -\t/* Now make sure the RAW mbus code is supported by the image source. */\n> > > -\tunsigned int rawCode = it->second;\n> > > \tconst auto rawSizes = sizes(rawCode);\n> > > \tif (rawSizes.empty())\n> > > \t\treturn bestRawFormat();\n> > > @@ -259,16 +284,14 @@ PixelFormat MaliC55CameraData::adjustRawFormat(const PixelFormat &rawFmt) const\n> > >\n> > > Size MaliC55CameraData::adjustRawSizes(const PixelFormat &rawFmt, const Size &size) const\n> > > {\n> > > -\t/* Just make sure the format is supported. */\n> > > -\tauto it = maliC55FmtToCode.find(rawFmt);\n> > > -\tif (it == maliC55FmtToCode.end())\n> > > -\t\treturn {};\n> > > -\n> > > \t/* Expand the RAW size to the minimum ISP input size. */\n> > > \tSize rawSize = size.expandedTo(kMaliC55MinInputSize);\n> > >\n> > > \t/* Check if the size is natively supported. */\n> > > -\tunsigned int rawCode = it->second;\n> > > +\tint rawCode = pixfmtToMbusCode(rawFmt);\n> > > +\tif (rawCode < 0)\n> > > +\t\treturn {};\n> > > +\n> > > \tconst auto rawSizes = sizes(rawCode);\n> > > \tauto sizeIt = std::find(rawSizes.begin(), rawSizes.end(), rawSize);\n> > > \tif (sizeIt != rawSizes.end())\n> > > @@ -349,6 +372,10 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n> > > \t\t */\n> > > \t\tPixelFormat rawFormat =\n> > > \t\t\tdata_->adjustRawFormat(rawConfig->pixelFormat);\n> > > +\n> > > +\t\tif (!rawFormat.isValid())\n> > > +\t\t\treturn Invalid;\n> > > +\n> > > \t\tif (rawFormat != rawConfig->pixelFormat) {\n> > > \t\t\tLOG(MaliC55, Debug)\n> > > \t\t\t\t<< \"RAW format adjusted to \" << rawFormat;\n> > > @@ -418,8 +445,7 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n> > >\n> > > \t/* If there's a RAW config, sensor configuration follows it. */\n> > > \tif (rawConfig) {\n> > > -\t\tconst auto it = maliC55FmtToCode.find(rawConfig->pixelFormat);\n> > > -\t\tsensorFormat_.code = it->second;\n> > > +\t\tsensorFormat_.code = data_->pixfmtToMbusCode(rawConfig->pixelFormat);\n> > > \t\tsensorFormat_.size = rawConfig->size.expandedTo(minSensorSize);\n> > >\n> > > \t\treturn status;\n> > > @@ -427,6 +453,9 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()\n> > >\n> > > \t/* If there's no RAW config, compute the sensor configuration here. */\n> > > \tPixelFormat rawFormat = data_->bestRawFormat();\n> > > +\tif (!rawFormat.isValid())\n> > > +\t\treturn Invalid;\n> > > +\n> > Is this correct now ?\n> >\n> > bestRawFormat() will always return a 16bpp PixelFormat, as it extracts\n> > from maliC55FmtToCode.\n> It's unnecessary - but previously I had a Fatal log in bestRawFormat() and\n> Kieran asked for it to be downgraded to an Error - I added the check at that\n> point as I feel uneasy at allowing the assumption that it's correct.\n\nPlease note I was pointing out that the check isn't correct, but it\nwas only about the 16bpp format being selected by this function and\npossibily not being available on the sensor.\n\nSorry for the confusion, I broke a single comment down to three sparse\nparagraphs :)\n\n> >\n> > > \tconst auto it = maliC55FmtToCode.find(rawFormat);\n> > And here we get back a 16bpp mbus code\n> >\n> > > \tsensorFormat_.code = it->second;\n> > A 16bpp mbus code is not guaranteed to be supported by the sensor, as\n> > the 16bpp PixelFormat can be generated from any 8,12,14 bpp mbus code\n> > (you only make sure the component ordering matches, in\n> > bestRawFormat()).\n> >\n> > One option would be to pass an &mbusCode argument to bestRawFormat()\n> > to optionally return what sensor mbus code generates the 16bpp pixel\n> > format.\n>\n>\n> Ah, good spot. I think it needs a call to pixfmtToMbusCode() here which\n> finds the highest bitdepth equivalent supported by the sensor.\n>\n\nThat's another option, at the expense of a double lookup. The map is\nsmall and we're not in an hot path here so no big deal probably.\n\nThanks\n j\n\n>\n> Thanks!\n>\n> >\n> > The rest looks good!\n> >\n> > > @@ -614,7 +643,10 @@ PipelineHandlerMaliC55::generateConfiguration(Camera *camera,\n> > >\n> > > \t\t\tif (isRaw) {\n> > > \t\t\t\t/* Make sure the mbus code is supported. */\n> > > -\t\t\t\tunsigned int rawCode = maliFormat.second;\n> > > +\t\t\t\tint rawCode = data->pixfmtToMbusCode(pixFmt);\n> > > +\t\t\t\tif (rawCode < 0)\n> > > +\t\t\t\t\tcontinue;\n> > > +\n> > > \t\t\t\tconst auto sizes = data->sizes(rawCode);\n> > > \t\t\t\tif (sizes.empty())\n> > > \t\t\t\t\tcontinue;\n> > > --\n> > > 2.30.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 D521FC324C\n\tfor ;\n\tThu, 14 Nov 2024 10:23:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 083656583E;\n\tThu, 14 Nov 2024 11:23:23 +0100 (CET)","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 995C165834\n\tfor ;\n\tThu, 14 Nov 2024 11:23:21 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:6462:5de2:459e:1ee6:26ea:2d31])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 62666502;\n\tThu, 14 Nov 2024 11:23:07 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DZrdGf59\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731579787;\n\tbh=vX5XMtJKFUT+l5vn5cVBBiJTfjQNoNk1LMT9792nZ/8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DZrdGf591VGeR3Md0S8bmNNM0TkcOItDjFqDvCowpKpDUvcH1pLati5rvbV/PcrOn\n\t7N653kiLHDUtLjkyOXtq6ERoi0ovkRkzyeHgcDLj6zwDdA8OB7HtIZz+Ji5RLlExZH\n\t+fKztI+3qSeOMrLKoxSoAYA4zbeg1GPjmRWitQzk=","Date":"Thu, 14 Nov 2024 11:23:17 +0100","From":"Jacopo Mondi ","To":"Dan Scally ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v5 10/13] libcamera: mali-c55: Correct input/output\n\tformat representation","Message-ID":"","References":"<20241107105846.52287-1-dan.scally@ideasonboard.com>\n\t<20241107105846.52287-11-dan.scally@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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\" "}}]