[libcamera-devel] [PATCH 3/3] pipeline: rkisp1: Plumb SensorConfiguration
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 17 17:31:55 CET 2024
Hi Paul,
Thank you for the patch.
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:
#include <optional>
> 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)
> {
> 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.
Holds in two lines.
> + */
> + std::optional<unsigned int> bitDepth;
> + if (sensorConfig) {
> + bitDepth = sensorConfig->bitDepth;
> + if (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {
What if sensorConfig->bitDepth is 8, 10 or 12, but takes a value that
the sensor doesn't support ?
> + V4L2SubdeviceFormat format = {};
> + 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 */
s/validate/validate()/
s/resolution/resolution./
> maxSize = std::max(maxSize, cfg.size);
> }
>
> + if (rawFormat.isValid() && sensorConfig) {
> + if (sensorConfig->outputSize != rawSize) {
> + sensorConfig->outputSize = rawSize;
If I understand correctly, the raw stream configuration takes precedence
over the sensorConfig. Is that a standard behaviour we document ?
> + 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
s/TODO/\\todo/
> + * capabilities and dual ISP mode (for the i.MX8MP).
> + *
> + * x1.5 should be a reasonable hardcoded ballpark for now.
> + */
> + double factor = 1.5;
const
> + Size before = sensorConfig->outputSize;
const
s/before/originalSize/ ?
> + Size upscaleLimit = {
> + static_cast<unsigned int>(maxSize.width / factor),
> + static_cast<unsigned int>(maxSize.height / factor)
> + };
const
I think you can write it
const Size upscaleLimit = maxSize / factor;
> +
> + if (sensorConfig->outputSize < upscaleLimit)
Don't you need to compare the width and height separately ? Comparison
of sizes is tricky :-S
> + sensorConfig->outputSize = maxSize;
> +
> + if (before != sensorConfig->outputSize)
> + status = Adjusted;
> +
> + /* No need to validate bpp for non-raw */
Why not ?
> + }
> +
> std::vector<unsigned int> mbusCodes;
> + Size size = maxSize;
s/size/sensorSize/
>
> 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);
> + }
I may be wrong, but shouldn't you set size = sensorConfig->outputSize
here ?
> } 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 */
s/valid/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());
> + }
No need for curly braces.
> +
> 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
I think this line doesn't need to change, it wasn't above the 80
characters limit.
> + * 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;
> + 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;
Do we have a guarantee that at least one format will match sensorBPP ?
If so, a comment to explain why would be useful.
> + } else {
> + if (info.bitsPerPixel == reqRawBitsPerPixel)
> + reqRawFormat = format;
> +
> + if (info.bitsPerPixel > rawBitsPerPixel) {
> + rawBitsPerPixel = info.bitsPerPixel;
> + rawFormat = format;
> + }
> }
I wonder if all this could be simplified by handling the bpp selection
outside of the path code, and use the selected valud in
RkISP1Path::validate(), without considering the bpp from the pixel
format here.
> }
>
> @@ -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);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list