[libcamera-devel,3/3] pipeline: rkisp1: Plumb SensorConfiguration
diff mbox series

Message ID 20240116091754.100654-4-paul.elder@ideasonboard.com
State New
Headers show
Series
  • i.MX8MP support, plus misc fixes
Related show

Commit Message

Paul Elder Jan. 16, 2024, 9:17 a.m. UTC 
Add support to the rkisp1 pipeline handler for validating and
configuring SensorConfiguration.

Signed-off-by: Paul Elder <paul.elder@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(-)

Comments

Laurent Pinchart Jan. 17, 2024, 4:31 p.m. UTC | #1 
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@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);
Jacopo Mondi Jan. 17, 2024, 4:36 p.m. UTC | #2 
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@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
>

Patch
diff mbox series 

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)
 {
 	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 = {};
+			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;
+			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;
+	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;
+				}
 			}
 		}
 
@@ -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);