| Message ID | 20241107105846.52287-11-dan.scally@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Dan I would mention RAW somewhere in the commit message 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@ideasonboard.com> > Signed-off-by: Daniel Scally <dan.scally@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. > 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. 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 >
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@ideasonboard.com> >> Signed-off-by: Daniel Scally <dan.scally@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 >>
Hi Dan On Thu, Nov 14, 2024 at 10:15:04AM +0000, Dan Scally wrote: > 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@ideasonboard.com> > > > Signed-off-by: Daniel Scally <dan.scally@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. Please note I was pointing out that the check isn't correct, but it was only about the 16bpp format being selected by this function and possibily not being available on the sensor. Sorry for the confusion, I broke a single comment down to three sparse paragraphs :) > > > > > 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. > That's another option, at the expense of a double lookup. The map is small and we're not in an hot path here so no big deal probably. Thanks j > > 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 > > >
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; + + 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); + + 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; + const auto it = maliC55FmtToCode.find(rawFormat); sensorFormat_.code = it->second; @@ -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;