| Message ID | 20230129135830.27490-2-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Sun, Jan 29, 2023 at 02:58:26PM +0100, Jacopo Mondi via libcamera-devel wrote: > The current implementation of the ISI pipeline handler handles > translation of PixelFormat to media bus formats from the sensor > through a centralized map. > > As the criteria to select the correct media bus code depend if the > output PixelFormat is a RAW Bayer format or not, start by splitting > the RAW media bus code procedure selection out by adding a method > for such purpose to the ISICameraData class. > > Add the function to the ISICameraData and not to the > ISICameraConfiguration because: > - The sensor is a property of CameraData > - The same function will be re-used by the ISIPipelineHandler > during CameraConfiguration generation. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> In the title, s/Break-out/Break out/ > --- > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 130 +++++++++++++------ > 1 file changed, 90 insertions(+), 40 deletions(-) > > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > index 0c67e35dde73..72bc310d80ec 100644 > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > @@ -59,6 +59,8 @@ public: > return stream - &*streams_.begin(); > } > > + unsigned int getRawMediaBusFormat(PixelFormat *pixelFormat) const; > + > std::unique_ptr<CameraSensor> sensor_; > std::unique_ptr<V4L2Subdevice> csis_; > > @@ -174,6 +176,85 @@ int ISICameraData::init() > return 0; > } > > +/* > + * Get a RAW Bayer media bus format compatible with the requested pixelFormat. > + * > + * If the requested pixelFormat cannot be produced by the sensor adjust it to > + * the one corresponding to the media bus format with the largest bit-depth. > + */ > +unsigned int ISICameraData::getRawMediaBusFormat(PixelFormat *pixelFormat) const I'm wondering why you decided to use a pointer instead of a reference. I suppose since we control all the callers it's fine either way, so just wondering. Otherwise, looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > +{ > + std::vector<unsigned int> mbusCodes = sensor_->mbusCodes(); > + > + const std::map<PixelFormat, unsigned int> rawFormats = { > + { formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 }, > + { formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 }, > + { formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 }, > + { formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 }, > + { formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 }, > + { formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 }, > + { formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 }, > + { formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 }, > + { formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 }, > + { formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 }, > + { formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 }, > + { formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 }, > + }; > + > + /* > + * Make sure the requested PixelFormat is supported in the above > + * map and the sensor can produce the compatible mbus code. > + */ > + auto it = rawFormats.find(*pixelFormat); > + if (it != rawFormats.end() && > + std::count(mbusCodes.begin(), mbusCodes.end(), it->second)) > + return it->second; > + > + if (it == rawFormats.end()) > + LOG(ISI, Warning) << pixelFormat > + << " not supported in ISI formats map."; > + > + /* > + * The desired pixel format cannot be produced. Adjust it to the one > + * corresponding to the raw media bus format with the largest bit-depth > + * the sensor provides. > + */ > + unsigned int sensorCode = 0; > + unsigned int maxDepth = 0; > + *pixelFormat = {}; > + > + for (unsigned int code : mbusCodes) { > + /* Make sure the media bus format is RAW Bayer. */ > + const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code); > + if (!bayerFormat.isValid()) > + continue; > + > + /* Make sure the media format is supported. */ > + it = std::find_if(rawFormats.begin(), rawFormats.end(), > + [code](auto &rawFormat) { > + return rawFormat.second == code; > + }); > + > + if (it == rawFormats.end()) { > + LOG(ISI, Warning) << bayerFormat > + << " not supported in ISI formats map."; > + continue; > + } > + > + /* Pick the one with the largest bit depth. */ > + if (bayerFormat.bitDepth > maxDepth) { > + maxDepth = bayerFormat.bitDepth; > + *pixelFormat = it->first; > + sensorCode = code; > + } > + } > + > + if (!pixelFormat->isValid()) > + LOG(ISI, Error) << "Cannot find a supported RAW format"; > + > + return sensorCode; > +} > + > /* ----------------------------------------------------------------------------- > * Camera Configuration > */ > @@ -311,50 +392,19 @@ ISICameraConfiguration::validateRaw(std::set<Stream *> &availableStreams, > */ > std::vector<unsigned int> mbusCodes = data_->sensor_->mbusCodes(); > StreamConfiguration &rawConfig = config_[0]; > + PixelFormat rawFormat = rawConfig.pixelFormat; > > - bool supported = false; > - auto it = formatsMap_.find(rawConfig.pixelFormat); > - if (it != formatsMap_.end()) { > - unsigned int mbusCode = it->second.sensorCode; > - > - if (std::count(mbusCodes.begin(), mbusCodes.end(), mbusCode)) > - supported = true; > + unsigned int sensorCode = data_->getRawMediaBusFormat(&rawFormat); > + if (!sensorCode) { > + LOG(ISI, Error) << "Cannot adjust RAW pixelformat " > + << rawConfig.pixelFormat; > + return Invalid; > } > > - if (!supported) { > - /* > - * Adjust to the first mbus code supported by both the > - * sensor and the pipeline. > - */ > - const FormatMap::value_type *pipeConfig = nullptr; > - for (unsigned int code : mbusCodes) { > - const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code); > - if (!bayerFormat.isValid()) > - continue; > - > - auto fmt = std::find_if(ISICameraConfiguration::formatsMap_.begin(), > - ISICameraConfiguration::formatsMap_.end(), > - [code](const auto &isiFormat) { > - const auto &pipe = isiFormat.second; > - return pipe.sensorCode == code; > - }); > - > - if (fmt == ISICameraConfiguration::formatsMap_.end()) > - continue; > - > - pipeConfig = &(*fmt); > - break; > - } > - > - if (!pipeConfig) { > - LOG(ISI, Error) << "Cannot adjust RAW format " > - << rawConfig.pixelFormat; > - return Invalid; > - } > - > - rawConfig.pixelFormat = pipeConfig->first; > + if (rawFormat != rawConfig.pixelFormat) { > LOG(ISI, Debug) << "RAW pixelformat adjusted to " > - << pipeConfig->first; > + << rawFormat; > + rawConfig.pixelFormat = rawFormat; > status = Adjusted; > } > > -- > 2.39.0 >
On Thu, Mar 02, 2023 at 08:34:47PM +0900, Paul Elder via libcamera-devel wrote: > On Sun, Jan 29, 2023 at 02:58:26PM +0100, Jacopo Mondi via libcamera-devel wrote: > > The current implementation of the ISI pipeline handler handles > > translation of PixelFormat to media bus formats from the sensor > > through a centralized map. > > > > As the criteria to select the correct media bus code depend if the > > output PixelFormat is a RAW Bayer format or not, start by splitting > > the RAW media bus code procedure selection out by adding a method > > for such purpose to the ISICameraData class. > > > > Add the function to the ISICameraData and not to the > > ISICameraConfiguration because: > > - The sensor is a property of CameraData > > - The same function will be re-used by the ISIPipelineHandler > > during CameraConfiguration generation. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > In the title, s/Break-out/Break out/ > > > --- > > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 130 +++++++++++++------ > > 1 file changed, 90 insertions(+), 40 deletions(-) > > > > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > > index 0c67e35dde73..72bc310d80ec 100644 > > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > > @@ -59,6 +59,8 @@ public: > > return stream - &*streams_.begin(); > > } > > > > + unsigned int getRawMediaBusFormat(PixelFormat *pixelFormat) const; > > + > > std::unique_ptr<CameraSensor> sensor_; > > std::unique_ptr<V4L2Subdevice> csis_; > > > > @@ -174,6 +176,85 @@ int ISICameraData::init() > > return 0; > > } > > > > +/* > > + * Get a RAW Bayer media bus format compatible with the requested pixelFormat. > > + * > > + * If the requested pixelFormat cannot be produced by the sensor adjust it to > > + * the one corresponding to the media bus format with the largest bit-depth. > > + */ > > +unsigned int ISICameraData::getRawMediaBusFormat(PixelFormat *pixelFormat) const > > I'm wondering why you decided to use a pointer instead of a reference. I > suppose since we control all the callers it's fine either way, so just > wondering. > > Otherwise, looks good to me. > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > +{ > > + std::vector<unsigned int> mbusCodes = sensor_->mbusCodes(); > > + > > + const std::map<PixelFormat, unsigned int> rawFormats = { Oh I forgot, how about making this static? Same with the one in the next patch. Paul > > + { formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 }, > > + { formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 }, > > + { formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 }, > > + { formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 }, > > + { formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 }, > > + { formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 }, > > + { formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 }, > > + { formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 }, > > + { formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 }, > > + { formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 }, > > + { formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 }, > > + { formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 }, > > + }; > > + > > + /* > > + * Make sure the requested PixelFormat is supported in the above > > + * map and the sensor can produce the compatible mbus code. > > + */ > > + auto it = rawFormats.find(*pixelFormat); > > + if (it != rawFormats.end() && > > + std::count(mbusCodes.begin(), mbusCodes.end(), it->second)) > > + return it->second; > > + > > + if (it == rawFormats.end()) > > + LOG(ISI, Warning) << pixelFormat > > + << " not supported in ISI formats map."; > > + > > + /* > > + * The desired pixel format cannot be produced. Adjust it to the one > > + * corresponding to the raw media bus format with the largest bit-depth > > + * the sensor provides. > > + */ > > + unsigned int sensorCode = 0; > > + unsigned int maxDepth = 0; > > + *pixelFormat = {}; > > + > > + for (unsigned int code : mbusCodes) { > > + /* Make sure the media bus format is RAW Bayer. */ > > + const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code); > > + if (!bayerFormat.isValid()) > > + continue; > > + > > + /* Make sure the media format is supported. */ > > + it = std::find_if(rawFormats.begin(), rawFormats.end(), > > + [code](auto &rawFormat) { > > + return rawFormat.second == code; > > + }); > > + > > + if (it == rawFormats.end()) { > > + LOG(ISI, Warning) << bayerFormat > > + << " not supported in ISI formats map."; > > + continue; > > + } > > + > > + /* Pick the one with the largest bit depth. */ > > + if (bayerFormat.bitDepth > maxDepth) { > > + maxDepth = bayerFormat.bitDepth; > > + *pixelFormat = it->first; > > + sensorCode = code; > > + } > > + } > > + > > + if (!pixelFormat->isValid()) > > + LOG(ISI, Error) << "Cannot find a supported RAW format"; > > + > > + return sensorCode; > > +} > > + > > /* ----------------------------------------------------------------------------- > > * Camera Configuration > > */ > > @@ -311,50 +392,19 @@ ISICameraConfiguration::validateRaw(std::set<Stream *> &availableStreams, > > */ > > std::vector<unsigned int> mbusCodes = data_->sensor_->mbusCodes(); > > StreamConfiguration &rawConfig = config_[0]; > > + PixelFormat rawFormat = rawConfig.pixelFormat; > > > > - bool supported = false; > > - auto it = formatsMap_.find(rawConfig.pixelFormat); > > - if (it != formatsMap_.end()) { > > - unsigned int mbusCode = it->second.sensorCode; > > - > > - if (std::count(mbusCodes.begin(), mbusCodes.end(), mbusCode)) > > - supported = true; > > + unsigned int sensorCode = data_->getRawMediaBusFormat(&rawFormat); > > + if (!sensorCode) { > > + LOG(ISI, Error) << "Cannot adjust RAW pixelformat " > > + << rawConfig.pixelFormat; > > + return Invalid; > > } > > > > - if (!supported) { > > - /* > > - * Adjust to the first mbus code supported by both the > > - * sensor and the pipeline. > > - */ > > - const FormatMap::value_type *pipeConfig = nullptr; > > - for (unsigned int code : mbusCodes) { > > - const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code); > > - if (!bayerFormat.isValid()) > > - continue; > > - > > - auto fmt = std::find_if(ISICameraConfiguration::formatsMap_.begin(), > > - ISICameraConfiguration::formatsMap_.end(), > > - [code](const auto &isiFormat) { > > - const auto &pipe = isiFormat.second; > > - return pipe.sensorCode == code; > > - }); > > - > > - if (fmt == ISICameraConfiguration::formatsMap_.end()) > > - continue; > > - > > - pipeConfig = &(*fmt); > > - break; > > - } > > - > > - if (!pipeConfig) { > > - LOG(ISI, Error) << "Cannot adjust RAW format " > > - << rawConfig.pixelFormat; > > - return Invalid; > > - } > > - > > - rawConfig.pixelFormat = pipeConfig->first; > > + if (rawFormat != rawConfig.pixelFormat) { > > LOG(ISI, Debug) << "RAW pixelformat adjusted to " > > - << pipeConfig->first; > > + << rawFormat; > > + rawConfig.pixelFormat = rawFormat; > > status = Adjusted; > > } > > > > -- > > 2.39.0 > >
Hi Paul On Thu, Mar 02, 2023 at 08:34:47PM +0900, Paul Elder via libcamera-devel wrote: > On Sun, Jan 29, 2023 at 02:58:26PM +0100, Jacopo Mondi via libcamera-devel wrote: > > The current implementation of the ISI pipeline handler handles > > translation of PixelFormat to media bus formats from the sensor > > through a centralized map. > > > > As the criteria to select the correct media bus code depend if the > > output PixelFormat is a RAW Bayer format or not, start by splitting > > the RAW media bus code procedure selection out by adding a method > > for such purpose to the ISICameraData class. > > > > Add the function to the ISICameraData and not to the > > ISICameraConfiguration because: > > - The sensor is a property of CameraData > > - The same function will be re-used by the ISIPipelineHandler > > during CameraConfiguration generation. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > In the title, s/Break-out/Break out/ > > > --- > > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 130 +++++++++++++------ > > 1 file changed, 90 insertions(+), 40 deletions(-) > > > > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > > index 0c67e35dde73..72bc310d80ec 100644 > > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > > @@ -59,6 +59,8 @@ public: > > return stream - &*streams_.begin(); > > } > > > > + unsigned int getRawMediaBusFormat(PixelFormat *pixelFormat) const; > > + > > std::unique_ptr<CameraSensor> sensor_; > > std::unique_ptr<V4L2Subdevice> csis_; > > > > @@ -174,6 +176,85 @@ int ISICameraData::init() > > return 0; > > } > > > > +/* > > + * Get a RAW Bayer media bus format compatible with the requested pixelFormat. > > + * > > + * If the requested pixelFormat cannot be produced by the sensor adjust it to > > + * the one corresponding to the media bus format with the largest bit-depth. > > + */ > > +unsigned int ISICameraData::getRawMediaBusFormat(PixelFormat *pixelFormat) const > > I'm wondering why you decided to use a pointer instead of a reference. I pixelFormat is an output parameter and by convention we use references for const input parameters and pointers for parameters that can be modified afaik > suppose since we control all the callers it's fine either way, so just > wondering. > > Otherwise, looks good to me. > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> Thanks j > > > +{ > > + std::vector<unsigned int> mbusCodes = sensor_->mbusCodes(); > > + > > + const std::map<PixelFormat, unsigned int> rawFormats = { > > + { formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 }, > > + { formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 }, > > + { formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 }, > > + { formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 }, > > + { formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 }, > > + { formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 }, > > + { formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 }, > > + { formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 }, > > + { formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 }, > > + { formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 }, > > + { formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 }, > > + { formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 }, > > + }; > > + > > + /* > > + * Make sure the requested PixelFormat is supported in the above > > + * map and the sensor can produce the compatible mbus code. > > + */ > > + auto it = rawFormats.find(*pixelFormat); > > + if (it != rawFormats.end() && > > + std::count(mbusCodes.begin(), mbusCodes.end(), it->second)) > > + return it->second; > > + > > + if (it == rawFormats.end()) > > + LOG(ISI, Warning) << pixelFormat > > + << " not supported in ISI formats map."; > > + > > + /* > > + * The desired pixel format cannot be produced. Adjust it to the one > > + * corresponding to the raw media bus format with the largest bit-depth > > + * the sensor provides. > > + */ > > + unsigned int sensorCode = 0; > > + unsigned int maxDepth = 0; > > + *pixelFormat = {}; > > + > > + for (unsigned int code : mbusCodes) { > > + /* Make sure the media bus format is RAW Bayer. */ > > + const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code); > > + if (!bayerFormat.isValid()) > > + continue; > > + > > + /* Make sure the media format is supported. */ > > + it = std::find_if(rawFormats.begin(), rawFormats.end(), > > + [code](auto &rawFormat) { > > + return rawFormat.second == code; > > + }); > > + > > + if (it == rawFormats.end()) { > > + LOG(ISI, Warning) << bayerFormat > > + << " not supported in ISI formats map."; > > + continue; > > + } > > + > > + /* Pick the one with the largest bit depth. */ > > + if (bayerFormat.bitDepth > maxDepth) { > > + maxDepth = bayerFormat.bitDepth; > > + *pixelFormat = it->first; > > + sensorCode = code; > > + } > > + } > > + > > + if (!pixelFormat->isValid()) > > + LOG(ISI, Error) << "Cannot find a supported RAW format"; > > + > > + return sensorCode; > > +} > > + > > /* ----------------------------------------------------------------------------- > > * Camera Configuration > > */ > > @@ -311,50 +392,19 @@ ISICameraConfiguration::validateRaw(std::set<Stream *> &availableStreams, > > */ > > std::vector<unsigned int> mbusCodes = data_->sensor_->mbusCodes(); > > StreamConfiguration &rawConfig = config_[0]; > > + PixelFormat rawFormat = rawConfig.pixelFormat; > > > > - bool supported = false; > > - auto it = formatsMap_.find(rawConfig.pixelFormat); > > - if (it != formatsMap_.end()) { > > - unsigned int mbusCode = it->second.sensorCode; > > - > > - if (std::count(mbusCodes.begin(), mbusCodes.end(), mbusCode)) > > - supported = true; > > + unsigned int sensorCode = data_->getRawMediaBusFormat(&rawFormat); > > + if (!sensorCode) { > > + LOG(ISI, Error) << "Cannot adjust RAW pixelformat " > > + << rawConfig.pixelFormat; > > + return Invalid; > > } > > > > - if (!supported) { > > - /* > > - * Adjust to the first mbus code supported by both the > > - * sensor and the pipeline. > > - */ > > - const FormatMap::value_type *pipeConfig = nullptr; > > - for (unsigned int code : mbusCodes) { > > - const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code); > > - if (!bayerFormat.isValid()) > > - continue; > > - > > - auto fmt = std::find_if(ISICameraConfiguration::formatsMap_.begin(), > > - ISICameraConfiguration::formatsMap_.end(), > > - [code](const auto &isiFormat) { > > - const auto &pipe = isiFormat.second; > > - return pipe.sensorCode == code; > > - }); > > - > > - if (fmt == ISICameraConfiguration::formatsMap_.end()) > > - continue; > > - > > - pipeConfig = &(*fmt); > > - break; > > - } > > - > > - if (!pipeConfig) { > > - LOG(ISI, Error) << "Cannot adjust RAW format " > > - << rawConfig.pixelFormat; > > - return Invalid; > > - } > > - > > - rawConfig.pixelFormat = pipeConfig->first; > > + if (rawFormat != rawConfig.pixelFormat) { > > LOG(ISI, Debug) << "RAW pixelformat adjusted to " > > - << pipeConfig->first; > > + << rawFormat; > > + rawConfig.pixelFormat = rawFormat; > > status = Adjusted; > > } > > > > -- > > 2.39.0 > >
Hi Jacopo On 29/01/2023 13:58, Jacopo Mondi wrote: > The current implementation of the ISI pipeline handler handles > translation of PixelFormat to media bus formats from the sensor > through a centralized map. > > As the criteria to select the correct media bus code depend if the > output PixelFormat is a RAW Bayer format or not, start by splitting > the RAW media bus code procedure selection out by adding a method > for such purpose to the ISICameraData class. > > Add the function to the ISICameraData and not to the > ISICameraConfiguration because: > - The sensor is a property of CameraData > - The same function will be re-used by the ISIPipelineHandler > during CameraConfiguration generation. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- You can add my tags to the series: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> Tested-by: Daniel Scally <dan.scally@ideasonboard.com> I tested with an imx219 so no YUV output from the sensor, but it at least identifies that properly and falls back to generateRawConfiguration(). Just a couple comments on the other patches which I'll reply to directly, but nothing particularly significant. > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 130 +++++++++++++------ > 1 file changed, 90 insertions(+), 40 deletions(-) > > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > index 0c67e35dde73..72bc310d80ec 100644 > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > @@ -59,6 +59,8 @@ public: > return stream - &*streams_.begin(); > } > > + unsigned int getRawMediaBusFormat(PixelFormat *pixelFormat) const; > + > std::unique_ptr<CameraSensor> sensor_; > std::unique_ptr<V4L2Subdevice> csis_; > > @@ -174,6 +176,85 @@ int ISICameraData::init() > return 0; > } > > +/* > + * Get a RAW Bayer media bus format compatible with the requested pixelFormat. > + * > + * If the requested pixelFormat cannot be produced by the sensor adjust it to > + * the one corresponding to the media bus format with the largest bit-depth. > + */ > +unsigned int ISICameraData::getRawMediaBusFormat(PixelFormat *pixelFormat) const > +{ > + std::vector<unsigned int> mbusCodes = sensor_->mbusCodes(); > + > + const std::map<PixelFormat, unsigned int> rawFormats = { > + { formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 }, > + { formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 }, > + { formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 }, > + { formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 }, > + { formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 }, > + { formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 }, > + { formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 }, > + { formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 }, > + { formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 }, > + { formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 }, > + { formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 }, > + { formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 }, > + }; > + > + /* > + * Make sure the requested PixelFormat is supported in the above > + * map and the sensor can produce the compatible mbus code. > + */ > + auto it = rawFormats.find(*pixelFormat); > + if (it != rawFormats.end() && > + std::count(mbusCodes.begin(), mbusCodes.end(), it->second)) > + return it->second; > + > + if (it == rawFormats.end()) > + LOG(ISI, Warning) << pixelFormat > + << " not supported in ISI formats map."; > + > + /* > + * The desired pixel format cannot be produced. Adjust it to the one > + * corresponding to the raw media bus format with the largest bit-depth > + * the sensor provides. > + */ > + unsigned int sensorCode = 0; > + unsigned int maxDepth = 0; > + *pixelFormat = {}; > + > + for (unsigned int code : mbusCodes) { > + /* Make sure the media bus format is RAW Bayer. */ > + const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code); > + if (!bayerFormat.isValid()) > + continue; > + > + /* Make sure the media format is supported. */ > + it = std::find_if(rawFormats.begin(), rawFormats.end(), > + [code](auto &rawFormat) { > + return rawFormat.second == code; > + }); > + > + if (it == rawFormats.end()) { > + LOG(ISI, Warning) << bayerFormat > + << " not supported in ISI formats map."; > + continue; > + } > + > + /* Pick the one with the largest bit depth. */ > + if (bayerFormat.bitDepth > maxDepth) { > + maxDepth = bayerFormat.bitDepth; > + *pixelFormat = it->first; > + sensorCode = code; > + } > + } > + > + if (!pixelFormat->isValid()) > + LOG(ISI, Error) << "Cannot find a supported RAW format"; > + > + return sensorCode; > +} > + > /* ----------------------------------------------------------------------------- > * Camera Configuration > */ > @@ -311,50 +392,19 @@ ISICameraConfiguration::validateRaw(std::set<Stream *> &availableStreams, > */ > std::vector<unsigned int> mbusCodes = data_->sensor_->mbusCodes(); > StreamConfiguration &rawConfig = config_[0]; > + PixelFormat rawFormat = rawConfig.pixelFormat; > > - bool supported = false; > - auto it = formatsMap_.find(rawConfig.pixelFormat); > - if (it != formatsMap_.end()) { > - unsigned int mbusCode = it->second.sensorCode; > - > - if (std::count(mbusCodes.begin(), mbusCodes.end(), mbusCode)) > - supported = true; > + unsigned int sensorCode = data_->getRawMediaBusFormat(&rawFormat); > + if (!sensorCode) { > + LOG(ISI, Error) << "Cannot adjust RAW pixelformat " > + << rawConfig.pixelFormat; > + return Invalid; > } > > - if (!supported) { > - /* > - * Adjust to the first mbus code supported by both the > - * sensor and the pipeline. > - */ > - const FormatMap::value_type *pipeConfig = nullptr; > - for (unsigned int code : mbusCodes) { > - const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code); > - if (!bayerFormat.isValid()) > - continue; > - > - auto fmt = std::find_if(ISICameraConfiguration::formatsMap_.begin(), > - ISICameraConfiguration::formatsMap_.end(), > - [code](const auto &isiFormat) { > - const auto &pipe = isiFormat.second; > - return pipe.sensorCode == code; > - }); > - > - if (fmt == ISICameraConfiguration::formatsMap_.end()) > - continue; > - > - pipeConfig = &(*fmt); > - break; > - } > - > - if (!pipeConfig) { > - LOG(ISI, Error) << "Cannot adjust RAW format " > - << rawConfig.pixelFormat; > - return Invalid; > - } > - > - rawConfig.pixelFormat = pipeConfig->first; > + if (rawFormat != rawConfig.pixelFormat) { > LOG(ISI, Debug) << "RAW pixelformat adjusted to " > - << pipeConfig->first; > + << rawFormat; > + rawConfig.pixelFormat = rawFormat; > status = Adjusted; > } >
Hi Jacopo, Thank you for the patch. On Sun, Jan 29, 2023 at 02:58:26PM +0100, Jacopo Mondi via libcamera-devel wrote: > The current implementation of the ISI pipeline handler handles > translation of PixelFormat to media bus formats from the sensor > through a centralized map. > > As the criteria to select the correct media bus code depend if the s/depend/depends on/ > output PixelFormat is a RAW Bayer format or not, start by splitting > the RAW media bus code procedure selection out by adding a method s/method/function/ > for such purpose to the ISICameraData class. > > Add the function to the ISICameraData and not to the > ISICameraConfiguration because: > - The sensor is a property of CameraData > - The same function will be re-used by the ISIPipelineHandler > during CameraConfiguration generation. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 130 +++++++++++++------ > 1 file changed, 90 insertions(+), 40 deletions(-) > > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > index 0c67e35dde73..72bc310d80ec 100644 > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp > @@ -59,6 +59,8 @@ public: > return stream - &*streams_.begin(); > } > > + unsigned int getRawMediaBusFormat(PixelFormat *pixelFormat) const; > + > std::unique_ptr<CameraSensor> sensor_; > std::unique_ptr<V4L2Subdevice> csis_; > > @@ -174,6 +176,85 @@ int ISICameraData::init() > return 0; > } > > +/* > + * Get a RAW Bayer media bus format compatible with the requested pixelFormat. > + * > + * If the requested pixelFormat cannot be produced by the sensor adjust it to > + * the one corresponding to the media bus format with the largest bit-depth. > + */ > +unsigned int ISICameraData::getRawMediaBusFormat(PixelFormat *pixelFormat) const > +{ > + std::vector<unsigned int> mbusCodes = sensor_->mbusCodes(); > + > + const std::map<PixelFormat, unsigned int> rawFormats = { static const. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + { formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 }, > + { formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 }, > + { formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 }, > + { formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 }, > + { formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 }, > + { formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 }, > + { formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 }, > + { formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 }, > + { formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 }, > + { formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 }, > + { formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 }, > + { formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 }, > + }; > + > + /* > + * Make sure the requested PixelFormat is supported in the above > + * map and the sensor can produce the compatible mbus code. > + */ > + auto it = rawFormats.find(*pixelFormat); > + if (it != rawFormats.end() && > + std::count(mbusCodes.begin(), mbusCodes.end(), it->second)) > + return it->second; > + > + if (it == rawFormats.end()) > + LOG(ISI, Warning) << pixelFormat > + << " not supported in ISI formats map."; > + > + /* > + * The desired pixel format cannot be produced. Adjust it to the one > + * corresponding to the raw media bus format with the largest bit-depth > + * the sensor provides. > + */ > + unsigned int sensorCode = 0; > + unsigned int maxDepth = 0; > + *pixelFormat = {}; > + > + for (unsigned int code : mbusCodes) { > + /* Make sure the media bus format is RAW Bayer. */ > + const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code); > + if (!bayerFormat.isValid()) > + continue; > + > + /* Make sure the media format is supported. */ > + it = std::find_if(rawFormats.begin(), rawFormats.end(), > + [code](auto &rawFormat) { > + return rawFormat.second == code; > + }); > + > + if (it == rawFormats.end()) { > + LOG(ISI, Warning) << bayerFormat > + << " not supported in ISI formats map."; > + continue; > + } > + > + /* Pick the one with the largest bit depth. */ > + if (bayerFormat.bitDepth > maxDepth) { > + maxDepth = bayerFormat.bitDepth; > + *pixelFormat = it->first; > + sensorCode = code; > + } > + } > + > + if (!pixelFormat->isValid()) > + LOG(ISI, Error) << "Cannot find a supported RAW format"; > + > + return sensorCode; > +} > + > /* ----------------------------------------------------------------------------- > * Camera Configuration > */ > @@ -311,50 +392,19 @@ ISICameraConfiguration::validateRaw(std::set<Stream *> &availableStreams, > */ > std::vector<unsigned int> mbusCodes = data_->sensor_->mbusCodes(); > StreamConfiguration &rawConfig = config_[0]; > + PixelFormat rawFormat = rawConfig.pixelFormat; > > - bool supported = false; > - auto it = formatsMap_.find(rawConfig.pixelFormat); > - if (it != formatsMap_.end()) { > - unsigned int mbusCode = it->second.sensorCode; > - > - if (std::count(mbusCodes.begin(), mbusCodes.end(), mbusCode)) > - supported = true; > + unsigned int sensorCode = data_->getRawMediaBusFormat(&rawFormat); > + if (!sensorCode) { > + LOG(ISI, Error) << "Cannot adjust RAW pixelformat " > + << rawConfig.pixelFormat; > + return Invalid; > } > > - if (!supported) { > - /* > - * Adjust to the first mbus code supported by both the > - * sensor and the pipeline. > - */ > - const FormatMap::value_type *pipeConfig = nullptr; > - for (unsigned int code : mbusCodes) { > - const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code); > - if (!bayerFormat.isValid()) > - continue; > - > - auto fmt = std::find_if(ISICameraConfiguration::formatsMap_.begin(), > - ISICameraConfiguration::formatsMap_.end(), > - [code](const auto &isiFormat) { > - const auto &pipe = isiFormat.second; > - return pipe.sensorCode == code; > - }); > - > - if (fmt == ISICameraConfiguration::formatsMap_.end()) > - continue; > - > - pipeConfig = &(*fmt); > - break; > - } > - > - if (!pipeConfig) { > - LOG(ISI, Error) << "Cannot adjust RAW format " > - << rawConfig.pixelFormat; > - return Invalid; > - } > - > - rawConfig.pixelFormat = pipeConfig->first; > + if (rawFormat != rawConfig.pixelFormat) { > LOG(ISI, Debug) << "RAW pixelformat adjusted to " > - << pipeConfig->first; > + << rawFormat; > + rawConfig.pixelFormat = rawFormat; > status = Adjusted; > } >
diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp index 0c67e35dde73..72bc310d80ec 100644 --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp @@ -59,6 +59,8 @@ public: return stream - &*streams_.begin(); } + unsigned int getRawMediaBusFormat(PixelFormat *pixelFormat) const; + std::unique_ptr<CameraSensor> sensor_; std::unique_ptr<V4L2Subdevice> csis_; @@ -174,6 +176,85 @@ int ISICameraData::init() return 0; } +/* + * Get a RAW Bayer media bus format compatible with the requested pixelFormat. + * + * If the requested pixelFormat cannot be produced by the sensor adjust it to + * the one corresponding to the media bus format with the largest bit-depth. + */ +unsigned int ISICameraData::getRawMediaBusFormat(PixelFormat *pixelFormat) const +{ + std::vector<unsigned int> mbusCodes = sensor_->mbusCodes(); + + const std::map<PixelFormat, unsigned int> rawFormats = { + { formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 }, + { formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 }, + { formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 }, + { formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 }, + { formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 }, + { formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 }, + { formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 }, + { formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 }, + { formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 }, + { formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 }, + { formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 }, + { formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 }, + }; + + /* + * Make sure the requested PixelFormat is supported in the above + * map and the sensor can produce the compatible mbus code. + */ + auto it = rawFormats.find(*pixelFormat); + if (it != rawFormats.end() && + std::count(mbusCodes.begin(), mbusCodes.end(), it->second)) + return it->second; + + if (it == rawFormats.end()) + LOG(ISI, Warning) << pixelFormat + << " not supported in ISI formats map."; + + /* + * The desired pixel format cannot be produced. Adjust it to the one + * corresponding to the raw media bus format with the largest bit-depth + * the sensor provides. + */ + unsigned int sensorCode = 0; + unsigned int maxDepth = 0; + *pixelFormat = {}; + + for (unsigned int code : mbusCodes) { + /* Make sure the media bus format is RAW Bayer. */ + const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code); + if (!bayerFormat.isValid()) + continue; + + /* Make sure the media format is supported. */ + it = std::find_if(rawFormats.begin(), rawFormats.end(), + [code](auto &rawFormat) { + return rawFormat.second == code; + }); + + if (it == rawFormats.end()) { + LOG(ISI, Warning) << bayerFormat + << " not supported in ISI formats map."; + continue; + } + + /* Pick the one with the largest bit depth. */ + if (bayerFormat.bitDepth > maxDepth) { + maxDepth = bayerFormat.bitDepth; + *pixelFormat = it->first; + sensorCode = code; + } + } + + if (!pixelFormat->isValid()) + LOG(ISI, Error) << "Cannot find a supported RAW format"; + + return sensorCode; +} + /* ----------------------------------------------------------------------------- * Camera Configuration */ @@ -311,50 +392,19 @@ ISICameraConfiguration::validateRaw(std::set<Stream *> &availableStreams, */ std::vector<unsigned int> mbusCodes = data_->sensor_->mbusCodes(); StreamConfiguration &rawConfig = config_[0]; + PixelFormat rawFormat = rawConfig.pixelFormat; - bool supported = false; - auto it = formatsMap_.find(rawConfig.pixelFormat); - if (it != formatsMap_.end()) { - unsigned int mbusCode = it->second.sensorCode; - - if (std::count(mbusCodes.begin(), mbusCodes.end(), mbusCode)) - supported = true; + unsigned int sensorCode = data_->getRawMediaBusFormat(&rawFormat); + if (!sensorCode) { + LOG(ISI, Error) << "Cannot adjust RAW pixelformat " + << rawConfig.pixelFormat; + return Invalid; } - if (!supported) { - /* - * Adjust to the first mbus code supported by both the - * sensor and the pipeline. - */ - const FormatMap::value_type *pipeConfig = nullptr; - for (unsigned int code : mbusCodes) { - const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code); - if (!bayerFormat.isValid()) - continue; - - auto fmt = std::find_if(ISICameraConfiguration::formatsMap_.begin(), - ISICameraConfiguration::formatsMap_.end(), - [code](const auto &isiFormat) { - const auto &pipe = isiFormat.second; - return pipe.sensorCode == code; - }); - - if (fmt == ISICameraConfiguration::formatsMap_.end()) - continue; - - pipeConfig = &(*fmt); - break; - } - - if (!pipeConfig) { - LOG(ISI, Error) << "Cannot adjust RAW format " - << rawConfig.pixelFormat; - return Invalid; - } - - rawConfig.pixelFormat = pipeConfig->first; + if (rawFormat != rawConfig.pixelFormat) { LOG(ISI, Debug) << "RAW pixelformat adjusted to " - << pipeConfig->first; + << rawFormat; + rawConfig.pixelFormat = rawFormat; status = Adjusted; }
The current implementation of the ISI pipeline handler handles translation of PixelFormat to media bus formats from the sensor through a centralized map. As the criteria to select the correct media bus code depend if the output PixelFormat is a RAW Bayer format or not, start by splitting the RAW media bus code procedure selection out by adding a method for such purpose to the ISICameraData class. Add the function to the ISICameraData and not to the ISICameraConfiguration because: - The sensor is a property of CameraData - The same function will be re-used by the ISIPipelineHandler during CameraConfiguration generation. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 130 +++++++++++++------ 1 file changed, 90 insertions(+), 40 deletions(-)