[PATCH v5 10/13] libcamera: mali-c55: Correct input/output format representation
Dan Scally
dan.scally at ideasonboard.com
Thu Nov 14 11:15:04 CET 2024
Hi Jacopo
On 11/11/2024 09:30, Jacopo Mondi wrote:
> Hi Dan
> I would mention RAW somewhere in the commit message
I thought that
>
> On Thu, Nov 07, 2024 at 10:58:43AM +0000, Daniel Scally wrote:
>> At present we configure raw streams by looping through the pixel
>> formats we support and finding one with an associated media bus
>> format code that the sensor can produce. In the new representation
>> of raw data from the kernel driver this will not work - the sensor
>> could produce 8, 10, 12, 14 or 16 bit data and the ISP will force
>> it to RAW16, which is the only actually supported output.
>>
>> To fix the issue move to simply finding a pixel format with a bayer
>> order that matches that of the media bus format produced by the
>> sensor. If the sensor can produce multiple formats with the same
>> bayer order use the one with the largest bitdepth.
>>
>> Finally, remove the claim to support RAW formats of less than 16 bits.
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
>> ---
>> Changes in v5:
>>
>> - Dropped the Fatal log.
>>
>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 128 ++++++++++++-------
>> 1 file changed, 80 insertions(+), 48 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>> index 156560c1..97827abd 100644
>> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>> @@ -57,22 +57,6 @@ const std::map<libcamera::PixelFormat, unsigned int> maliC55FmtToCode = {
>> { formats::NV21, MEDIA_BUS_FMT_YUV10_1X30 },
>>
>> /* RAW formats, FR pipe only. */
>> - { formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },
>> - { formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },
>> - { formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },
>> - { formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },
>> - { formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },
>> - { formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },
>> - { formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },
>> - { formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },
>> - { formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },
>> - { formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },
>> - { formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },
>> - { formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },
>> - { formats::SGBRG14, MEDIA_BUS_FMT_SGBRG14_1X14 },
>> - { formats::SRGGB14, MEDIA_BUS_FMT_SRGGB14_1X14 },
>> - { formats::SBGGR14, MEDIA_BUS_FMT_SBGGR14_1X14 },
>> - { formats::SGRBG14, MEDIA_BUS_FMT_SGRBG14_1X14 },
>> { formats::SGBRG16, MEDIA_BUS_FMT_SGBRG16_1X16 },
>> { formats::SRGGB16, MEDIA_BUS_FMT_SRGGB16_1X16 },
>> { formats::SBGGR16, MEDIA_BUS_FMT_SBGGR16_1X16 },
>> @@ -98,7 +82,8 @@ public:
>> const std::vector<Size> sizes(unsigned int mbusCode) const;
>> const Size resolution() const;
>>
>> - PixelFormat bestRawFormat() const;
>> + int pixfmtToMbusCode(const PixelFormat &pixFmt) const;
>> + const PixelFormat &bestRawFormat() const;
>>
>> PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const;
>> Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const;
>> @@ -208,33 +193,75 @@ const Size MaliC55CameraData::resolution() const
>> return tpgResolution_;
>> }
>>
>> -PixelFormat MaliC55CameraData::bestRawFormat() const
>> +/*
>> + * The Mali C55 ISP can only produce 16-bit RAW output in bypass modes, but the
>> + * sensors connected to it might produce 8/10/12/16 bits. We simply search the
>> + * sensor's supported formats for the one with a matching bayer order and the
>> + * greatest bitdepth.
>> + */
>> +int MaliC55CameraData::pixfmtToMbusCode(const PixelFormat &pixFmt) const
>> {
>> + auto it = maliC55FmtToCode.find(pixFmt);
>> + if (it == maliC55FmtToCode.end())
>> + return -EINVAL;
>> +
>> + BayerFormat bayerFormat = BayerFormat::fromMbusCode(it->second);
>> + if (!bayerFormat.isValid())
>> + return -EINVAL;
> I don't think this can't happen, but I'm fine keeping it here to
> protect against extensions of the maliC55FmtToCode map
>
>> +
>> + V4L2Subdevice::Formats formats = sd_->formats(0);
>> + unsigned int sensorMbusCode = 0;
>> unsigned int bitDepth = 0;
>> - PixelFormat rawFormat;
>>
>> - /*
>> - * Iterate over all the supported PixelFormat and find the one
>> - * supported by the camera with the largest bitdepth.
>> - */
>> - for (const auto &maliFormat : maliC55FmtToCode) {
>> - PixelFormat pixFmt = maliFormat.first;
>> - if (!isFormatRaw(pixFmt))
>> + for (const auto &[code, sizes] : formats) {
>> + BayerFormat sdBayerFormat = BayerFormat::fromMbusCode(code);
>> + if (!sdBayerFormat.isValid())
>> continue;
>>
>> - unsigned int rawCode = maliFormat.second;
>> - const auto rawSizes = sizes(rawCode);
>> - if (rawSizes.empty())
>> + if (sdBayerFormat.order != bayerFormat.order)
>> continue;
>>
>> - BayerFormat bayer = BayerFormat::fromMbusCode(rawCode);
>> - if (bayer.bitDepth > bitDepth) {
>> - bitDepth = bayer.bitDepth;
>> - rawFormat = pixFmt;
>> + if (sdBayerFormat.bitDepth > bitDepth) {
>> + bitDepth = sdBayerFormat.bitDepth;
>> + sensorMbusCode = code;
>> + }
>> + }
>> +
>> + if (!sensorMbusCode)
>> + return -EINVAL;
>> +
>> + return sensorMbusCode;
>> +}
>> +
>> +/*
>> + * Find a RAW PixelFormat supported by both the ISP and the sensor.
>> + *
>> + * The situation is mildly complicated by the fact that we expect the sensor to
>> + * output something like RAW8/10/12/16, but the ISP can only accept as input
>> + * RAW20 and can only produce as output RAW16. The one constant in that is the
>> + * bayer order of the data, so we'll simply check that the sensor produces a
>> + * format with a bayer order that matches that of one of the formats we support,
>> + * and select that.
>> + */
>> +const PixelFormat &MaliC55CameraData::bestRawFormat() const
>> +{
>> + static const PixelFormat invalidPixFmt = {};
>> +
>> + for (const auto &fmt : sd_->formats(0)) {
>> + BayerFormat sensorBayer = BayerFormat::fromMbusCode(fmt.first);
> As this comes from the sensor, here I would
>
> if (!bayerFormat.isValid())
> continue;
>
>> +
>> + for (const auto &[pixFmt, rawCode] : maliC55FmtToCode) {
>> + if (!isFormatRaw(pixFmt))
>> + continue;
>> +
>> + BayerFormat bayer = BayerFormat::fromMbusCode(rawCode);
>> + if (bayer.order == sensorBayer.order)
>> + return pixFmt;
>> }
>> }
>>
>> - return rawFormat;
>> + LOG(MaliC55, Error) << "Sensor doesn't provide a compatible format";
>> + return invalidPixFmt;
>> }
>>
>> /*
>> @@ -243,13 +270,11 @@ PixelFormat MaliC55CameraData::bestRawFormat() const
>> */
>> PixelFormat MaliC55CameraData::adjustRawFormat(const PixelFormat &rawFmt) const
>> {
>> - /* Make sure the provided raw format is supported by the pipeline. */
>> - auto it = maliC55FmtToCode.find(rawFmt);
>> - if (it == maliC55FmtToCode.end())
>> + /* Make sure the RAW mbus code is supported by the image source. */
>> + int rawCode = pixfmtToMbusCode(rawFmt);
>> + if (rawCode < 0)
>> return bestRawFormat();
>>
>> - /* Now make sure the RAW mbus code is supported by the image source. */
>> - unsigned int rawCode = it->second;
>> const auto rawSizes = sizes(rawCode);
>> if (rawSizes.empty())
>> return bestRawFormat();
>> @@ -259,16 +284,14 @@ PixelFormat MaliC55CameraData::adjustRawFormat(const PixelFormat &rawFmt) const
>>
>> Size MaliC55CameraData::adjustRawSizes(const PixelFormat &rawFmt, const Size &size) const
>> {
>> - /* Just make sure the format is supported. */
>> - auto it = maliC55FmtToCode.find(rawFmt);
>> - if (it == maliC55FmtToCode.end())
>> - return {};
>> -
>> /* Expand the RAW size to the minimum ISP input size. */
>> Size rawSize = size.expandedTo(kMaliC55MinInputSize);
>>
>> /* Check if the size is natively supported. */
>> - unsigned int rawCode = it->second;
>> + int rawCode = pixfmtToMbusCode(rawFmt);
>> + if (rawCode < 0)
>> + return {};
>> +
>> const auto rawSizes = sizes(rawCode);
>> auto sizeIt = std::find(rawSizes.begin(), rawSizes.end(), rawSize);
>> if (sizeIt != rawSizes.end())
>> @@ -349,6 +372,10 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()
>> */
>> PixelFormat rawFormat =
>> data_->adjustRawFormat(rawConfig->pixelFormat);
>> +
>> + if (!rawFormat.isValid())
>> + return Invalid;
>> +
>> if (rawFormat != rawConfig->pixelFormat) {
>> LOG(MaliC55, Debug)
>> << "RAW format adjusted to " << rawFormat;
>> @@ -418,8 +445,7 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()
>>
>> /* If there's a RAW config, sensor configuration follows it. */
>> if (rawConfig) {
>> - const auto it = maliC55FmtToCode.find(rawConfig->pixelFormat);
>> - sensorFormat_.code = it->second;
>> + sensorFormat_.code = data_->pixfmtToMbusCode(rawConfig->pixelFormat);
>> sensorFormat_.size = rawConfig->size.expandedTo(minSensorSize);
>>
>> return status;
>> @@ -427,6 +453,9 @@ CameraConfiguration::Status MaliC55CameraConfiguration::validate()
>>
>> /* If there's no RAW config, compute the sensor configuration here. */
>> PixelFormat rawFormat = data_->bestRawFormat();
>> + if (!rawFormat.isValid())
>> + return Invalid;
>> +
> Is this correct now ?
>
> bestRawFormat() will always return a 16bpp PixelFormat, as it extracts
> from maliC55FmtToCode.
It's unnecessary - but previously I had a Fatal log in bestRawFormat() and Kieran asked for it to be
downgraded to an Error - I added the check at that point as I feel uneasy at allowing the assumption
that it's correct.
>
>> const auto it = maliC55FmtToCode.find(rawFormat);
> And here we get back a 16bpp mbus code
>
>> sensorFormat_.code = it->second;
> A 16bpp mbus code is not guaranteed to be supported by the sensor, as
> the 16bpp PixelFormat can be generated from any 8,12,14 bpp mbus code
> (you only make sure the component ordering matches, in
> bestRawFormat()).
>
> One option would be to pass an &mbusCode argument to bestRawFormat()
> to optionally return what sensor mbus code generates the 16bpp pixel
> format.
Ah, good spot. I think it needs a call to pixfmtToMbusCode() here which finds the highest bitdepth
equivalent supported by the sensor.
Thanks!
>
> The rest looks good!
>
>> @@ -614,7 +643,10 @@ PipelineHandlerMaliC55::generateConfiguration(Camera *camera,
>>
>> if (isRaw) {
>> /* Make sure the mbus code is supported. */
>> - unsigned int rawCode = maliFormat.second;
>> + int rawCode = data->pixfmtToMbusCode(pixFmt);
>> + if (rawCode < 0)
>> + continue;
>> +
>> const auto sizes = data->sizes(rawCode);
>> if (sizes.empty())
>> continue;
>> --
>> 2.30.2
>>
More information about the libcamera-devel
mailing list