[libcamera-devel] [PATCH 3/3] pipeline: rkisp1: Plumb SensorConfiguration
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Wed Jan 17 17:36:58 CET 2024
Hi Paul
On Tue, Jan 16, 2024 at 06:17:54PM +0900, Paul Elder via libcamera-devel wrote:
> Add support to the rkisp1 pipeline handler for validating and
> configuring SensorConfiguration.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 97 ++++++++++++++++---
> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 37 +++++--
> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +-
> 3 files changed, 115 insertions(+), 22 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index fb67ba8f4..74da9c1b1 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -128,7 +128,8 @@ public:
> const Transform &combinedTransform() { return combinedTransform_; }
>
> private:
> - bool fitsAllPaths(const StreamConfiguration &cfg);
> + bool fitsAllPaths(const StreamConfiguration &cfg,
> + std::optional<unsigned int> sensorBPP);
>
> /*
> * The RkISP1CameraData instance is guaranteed to be valid as long as the
> @@ -448,17 +449,18 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> data_ = data;
> }
>
> -bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
> +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg,
> + std::optional<unsigned int> sensorBPP)
Nit: SensorConfiguration uses 'bitDepth'. Let's try to reuse it.
> {
> const CameraSensor *sensor = data_->sensor_.get();
> StreamConfiguration config;
>
> config = cfg;
> - if (data_->mainPath_->validate(sensor, &config) != Valid)
> + if (data_->mainPath_->validate(sensor, &config, sensorBPP) != Valid)
> return false;
>
> config = cfg;
> - if (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid)
> + if (data_->selfPath_ && data_->selfPath_->validate(sensor, &config, sensorBPP) != Valid)
> return false;
>
> return true;
> @@ -475,6 +477,24 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>
> status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);
>
> + /*
> + * For the sensor configuration, default to the first supported bit
> + * depth for the sensor configuration if an unsupported one is
> + * supplied.
> + */
> + std::optional<unsigned int> bitDepth;
> + if (sensorConfig) {
> + bitDepth = sensorConfig->bitDepth;
> + if (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {
> + V4L2SubdeviceFormat format = {};
I don't this this is correct.
The SensorConfiguration HAS to be correct as we cannot meaningfully
adjust it. If the supplied config is not correct, you should simply
fail and return Invalid here.
> + format.mbus_code = data_->sensor_->mbusCodes().at(0);
> +
> + sensorConfig->bitDepth = format.bitsPerPixel();
> + bitDepth = sensorConfig->bitDepth;
> + status = Adjusted;
> + }
> + }
> +
> /* Cap the number of entries to the available streams. */
> if (config_.size() > pathCount) {
> config_.resize(pathCount);
> @@ -510,7 +530,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> */
> std::vector<unsigned int> order(config_.size());
> std::iota(order.begin(), order.end(), 0);
> - if (config_.size() == 2 && fitsAllPaths(config_[0]))
> + if (config_.size() == 2 && fitsAllPaths(config_[0], bitDepth))
> std::reverse(order.begin(), order.end());
>
> bool mainPathAvailable = true;
> @@ -521,7 +541,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> /* Try to match stream without adjusting configuration. */
> if (mainPathAvailable) {
> StreamConfiguration tryCfg = cfg;
> - if (data_->mainPath_->validate(sensor, &tryCfg) == Valid) {
> + if (data_->mainPath_->validate(sensor, &tryCfg, bitDepth) == Valid) {
> mainPathAvailable = false;
> cfg = tryCfg;
> cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> @@ -531,7 +551,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>
> if (selfPathAvailable) {
> StreamConfiguration tryCfg = cfg;
> - if (data_->selfPath_->validate(sensor, &tryCfg) == Valid) {
> + if (data_->selfPath_->validate(sensor, &tryCfg, bitDepth) == Valid) {
> selfPathAvailable = false;
> cfg = tryCfg;
> cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> @@ -542,7 +562,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> /* Try to match stream allowing adjusting configuration. */
> if (mainPathAvailable) {
> StreamConfiguration tryCfg = cfg;
> - if (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) {
> + if (data_->mainPath_->validate(sensor, &tryCfg, bitDepth) == Adjusted) {
> mainPathAvailable = false;
> cfg = tryCfg;
> cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> @@ -553,7 +573,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>
> if (selfPathAvailable) {
> StreamConfiguration tryCfg = cfg;
> - if (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) {
> + if (data_->selfPath_->validate(sensor, &tryCfg, bitDepth) == Adjusted) {
> selfPathAvailable = false;
> cfg = tryCfg;
> cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> @@ -570,28 +590,75 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>
> /* Select the sensor format. */
> PixelFormat rawFormat;
> + Size rawSize;
> Size maxSize;
>
> for (const StreamConfiguration &cfg : config_) {
> const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> rawFormat = cfg.pixelFormat;
> + rawSize = cfg.size;
> + }
>
> + /* path->validate binds this to a sensor-supported resolution */
> maxSize = std::max(maxSize, cfg.size);
> }
>
> + if (rawFormat.isValid() && sensorConfig) {
> + if (sensorConfig->outputSize != rawSize) {
> + sensorConfig->outputSize = rawSize;
sensorConfig shall not be adjusted.
> + status = Adjusted;
> + }
> +
> + const PixelFormatInfo &info = PixelFormatInfo::info(rawFormat);
> + if (sensorConfig->bitDepth != info.bitsPerPixel) {
> + sensorConfig->bitDepth = info.bitsPerPixel;
> + status = Adjusted;
> + }
> + } else if (!rawFormat.isValid() && sensorConfig) {
> + /*
> + * TODO Handle this properly taking into account the upscaling
> + * capabilities and dual ISP mode (for the i.MX8MP).
> + *
> + * x1.5 should be a reasonable hardcoded ballpark for now.
> + */
> + double factor = 1.5;
> + Size before = sensorConfig->outputSize;
> + Size upscaleLimit = {
> + static_cast<unsigned int>(maxSize.width / factor),
> + static_cast<unsigned int>(maxSize.height / factor)
> + };
> +
> + if (sensorConfig->outputSize < upscaleLimit)
> + sensorConfig->outputSize = maxSize;
> +
> + if (before != sensorConfig->outputSize)
> + status = Adjusted;
> +
> + /* No need to validate bpp for non-raw */
> + }
> +
> std::vector<unsigned int> mbusCodes;
> + Size size = maxSize;
>
> if (rawFormat.isValid()) {
> mbusCodes = { rawFormats.at(rawFormat) };
> + size = rawSize;
> + } else if (sensorConfig) {
> + for (const auto &value : rawFormats) {
> + const PixelFormatInfo &info = PixelFormatInfo::info(value.first);
> + if (info.bitsPerPixel == sensorConfig->bitDepth)
> + mbusCodes.push_back(value.second);
> + }
> } else {
> std::transform(rawFormats.begin(), rawFormats.end(),
> std::back_inserter(mbusCodes),
> [](const auto &value) { return value.second; });
> }
>
> - sensorFormat_ = sensor->getFormat(mbusCodes, maxSize);
> + sensorFormat_ = sensor->getFormat(mbusCodes, size);
>
> + /* This should never happen if sensorConfig is valid */
> if (sensorFormat_.size.isNull())
> sensorFormat_.size = sensor->resolution();
>
> @@ -730,7 +797,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> V4L2SubdeviceFormat format = config->sensorFormat();
> LOG(RkISP1, Debug) << "Configuring sensor with " << format;
>
> - ret = sensor->setFormat(&format, config->combinedTransform());
> + if (config->sensorConfig) {
> + ret = sensor->applyConfiguration(*config->sensorConfig,
> + config->combinedTransform(), &format);
> + } else {
> + ret = sensor->setFormat(&format, config->combinedTransform());
> + }
> +
> if (ret < 0)
> return ret;
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index eaab7e857..a598b289b 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -220,7 +220,8 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
> }
>
> CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> - StreamConfiguration *cfg)
> + StreamConfiguration *cfg,
> + std::optional<unsigned int> sensorBPP)
> {
> const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> const Size &resolution = sensor->resolution();
> @@ -231,10 +232,15 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> /*
> * Validate the pixel format. If the requested format isn't supported,
> * default to either NV12 (all versions of the ISP are guaranteed to
> - * support NV12 on both the main and self paths) if the requested format
> - * is not a raw format, or to the supported raw format with the highest
> - * bits per pixel otherwise.
> + * support NV12 on both the main and self paths) if the requested
> + * format is not a raw format, or otherwise to a supported raw format
> + * with the same number of bits per pixel, or to maximum bits per pixel
> + * if the same is not supported.
> */
> + const PixelFormatInfo &formatInfo = PixelFormatInfo::info(cfg->pixelFormat);
> + unsigned int reqRawBitsPerPixel = formatInfo.bitsPerPixel;
What guarantees you that cfg is Raw here ?
> + PixelFormat reqRawFormat;
> +
> unsigned int rawBitsPerPixel = 0;
> PixelFormat rawFormat;
> bool found = false;
> @@ -250,12 +256,22 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> continue;
>
> /*
> - * Store the raw format with the highest bits per pixel
> - * for later usage.
> + * If the bits per pixel is supplied from the sensor
> + * configuration, choose a raw format that complies
> + * with it. Otherwise store the raw format with the
> + * highest bits per pixel for later usage.
> */
> - if (info.bitsPerPixel > rawBitsPerPixel) {
> - rawBitsPerPixel = info.bitsPerPixel;
> - rawFormat = format;
> + if (sensorBPP) {
> + if (info.bitsPerPixel == sensorBPP)
> + rawFormat = format;
> + } else {
> + if (info.bitsPerPixel == reqRawBitsPerPixel)
> + reqRawFormat = format;
> +
> + if (info.bitsPerPixel > rawBitsPerPixel) {
> + rawBitsPerPixel = info.bitsPerPixel;
> + rawFormat = format;
> + }
I don't think this is the right level where to do this.
If a StreamConfiguration is RAW and you have a SensorConfiguration you
should adjust the StreamConfiguration to the size and to a PixelFormat
with the desired bitDepth, then you should validate it with the Path.
> }
> }
>
> @@ -265,6 +281,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> }
> }
>
> + if (reqRawFormat.isValid())
> + rawFormat = reqRawFormat;
> +
> bool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==
> PixelFormatInfo::ColourEncodingRAW;
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index c96bd5557..784bcea77 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -44,7 +44,8 @@ public:
> const Size &resolution,
> StreamRole role);
> CameraConfiguration::Status validate(const CameraSensor *sensor,
> - StreamConfiguration *cfg);
> + StreamConfiguration *cfg,
> + std::optional<unsigned int> sensorBPP);
>
> int configure(const StreamConfiguration &config,
> const V4L2SubdeviceFormat &inputFormat);
> --
> 2.39.2
>
More information about the libcamera-devel
mailing list