libcamera: rkisp1: Integrate SensorConfiguration support

21463 diff mbox series

Integrate the RkISP1 pipeline handler to support sensor configuration
provided by applications through CameraConfiguration::sensorConfig.

The SensorConfiguration must be validated on both RkISP1Path (mainPath
and selfPath), so the parameters of RkISP1Path::validate() have been
updated to include sensorConfig.

For RAW paths, if a sensor configuration is provided, it will be
adjusted based on the stream configuration.

Finally, if the sensor configuration fails to apply, the camera
configuration validation will be marked as invalid.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
Testing:
- Developed over [PATCH 0/2] libcamera: pixelformat: Add isRaw() helper
- Tested with cam+[1] on i.MX8MP with IMX283 attached

[1]: https://patchwork.libcamera.org/patch/20080/
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 79 +++++++++++++++++--
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 20 +++--
 src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  2 +
 3 files changed, 87 insertions(+), 14 deletions(-)

Hi Umang

On Tue, Oct 01, 2024 at 07:47:38PM GMT, Umang Jain wrote:
> Integrate the RkISP1 pipeline handler to support sensor configuration
> provided by applications through CameraConfiguration::sensorConfig.
>
> The SensorConfiguration must be validated on both RkISP1Path (mainPath
> and selfPath), so the parameters of RkISP1Path::validate() have been
> updated to include sensorConfig.
>
> For RAW paths, if a sensor configuration is provided, it will be
> adjusted based on the stream configuration.
>
> Finally, if the sensor configuration fails to apply, the camera
> configuration validation will be marked as invalid.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
> Testing:
> - Developed over [PATCH 0/2] libcamera: pixelformat: Add isRaw() helper
> - Tested with cam+[1] on i.MX8MP with IMX283 attached
>
> [1]: https://patchwork.libcamera.org/patch/20080/
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 79 +++++++++++++++++--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 20 +++--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  2 +
>  3 files changed, 87 insertions(+), 14 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 0c4519de..49fe8fb1 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -474,11 +474,12 @@ bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)
>  	StreamConfiguration config;
>
>  	config = cfg;
> -	if (data_->mainPath_->validate(sensor, &config) != Valid)
> +	if (data_->mainPath_->validate(sensor, sensorConfig, &config) != Valid)
>  		return false;
>
>  	config = cfg;
> -	if (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid)
> +	if (data_->selfPath_ &&
> +	    data_->selfPath_->validate(sensor, sensorConfig, &config) != Valid)
>  		return false;
>
>  	return true;
> @@ -495,6 +496,31 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>
>  	status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);
>
> +	/*
> +	 * Make sure that if a sensor configuration has been requested it
> +	 * is valid.
> +	 */
> +	if (sensorConfig && !sensorConfig->isValid()) {
> +		LOG(RkISP1, Error) << "Invalid sensor configuration request";
> +		return Invalid;
> +	}
> +
> +	if (sensorConfig) {
> +		std::optional<unsigned int> bitDepth = sensorConfig->bitDepth;
> +		if (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {
> +			/*
> +			 * Default to the first supported bit depth, if an
> +			 * unsupported one is supplied.
> +			 */
> +			V4L2SubdeviceFormat format = {};
> +			format.code = sensor->mbusCodes().at(0);
> +
> +			sensorConfig->bitDepth =
> +				MediaBusFormatInfo::info(format.code).bitsPerPixel;
> +			status = Adjusted;

No, we never adjust sensorConfig. Either it's fully valid or we refuse
it. The rational is that we cannot adjust it to something meaningful
and applications that use sensorConfig should be highly specialized
and know precisely what they're doing.

It is documented by the SensorConfiguration class itself, even if
behind a \todo that should only apply to the first statement, not to
the whole paragraph imho

 * \todo Applications shall fully populate all fields of the
 * CameraConfiguration::sensorConfig class members before validating the
 * CameraConfiguration. If the SensorConfiguration is not fully populated, or if
 * any of its parameters cannot be applied to the sensor in use, the
 * CameraConfiguration validation process will fail and return
 * CameraConfiguration::Status::Invalid.

This is the relevant part

 * If the SensorConfiguration is not fully populated, or if
 * any of its parameters cannot be applied to the sensor in use, the
 * CameraConfiguration validation process will fail and return
 * CameraConfiguration::Status::Invalid.

Maybe it's all behind a todo because we currently allow populating
only bitDepth and outputSize, as documented by the isValid() function

 * \todo For now allow applications to populate the bitDepth and the outputSize
 * only as skipping and binnings factors are initialized to 1 and the analog
 * crop is ignored.

But the idea of never adjusting them is still valid.

I don't think we should adjust the sensor configuration.

Also the check for validity assumes only 8 10 and 12 are valid, while there
are sensors that can happily provide 14 or 16 bits.

If this is a limitation of the pipeline handler, of the driver or the
hardware itself, applications that specify a sensorConfig should know
about this as we assume they're highly specialized.

> +		}
> +	}
> +
>  	/* Cap the number of entries to the available streams. */
>  	if (config_.size() > pathCount) {
>  		config_.resize(pathCount);
> @@ -540,7 +566,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, sensorConfig, &tryCfg) == Valid) {
>  				mainPathAvailable = false;
>  				cfg = tryCfg;
>  				cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> @@ -550,7 +576,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>
>  		if (selfPathAvailable) {
>  			StreamConfiguration tryCfg = cfg;
> -			if (data_->selfPath_->validate(sensor, &tryCfg) == Valid) {
> +			if (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Valid) {
>  				selfPathAvailable = false;
>  				cfg = tryCfg;
>  				cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> @@ -561,7 +587,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, sensorConfig, &tryCfg) == Adjusted) {
>  				mainPathAvailable = false;
>  				cfg = tryCfg;
>  				cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_));
> @@ -572,7 +598,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>
>  		if (selfPathAvailable) {
>  			StreamConfiguration tryCfg = cfg;
> -			if (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) {
> +			if (data_->selfPath_->validate(sensor, sensorConfig, &tryCfg) == Adjusted) {
>  				selfPathAvailable = false;
>  				cfg = tryCfg;
>  				cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_));
> @@ -589,26 +615,63 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>
>  	/* Select the sensor format. */
>  	PixelFormat rawFormat;
> +	Size rawSize;
>  	Size maxSize;
>
>  	for (const StreamConfiguration &cfg : config_) {
> -		if (cfg.pixelFormat.isRaw())
> +		if (cfg.pixelFormat.isRaw()) {
>  			rawFormat = cfg.pixelFormat;
> +			rawSize = cfg.size;
> +		}
>
>  		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;
> +		}

As said above, never adjust the sensor configuration.

> +	}
> +
>  	std::vector<unsigned int> mbusCodes;
> +	Size sz = maxSize;
>
>  	if (rawFormat.isValid()) {
>  		mbusCodes = { rawFormats.at(rawFormat) };
> +		sz = 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);
> +		}
> +		sz = sensorConfig->outputSize;

You need to validate that sensorConfig->outputSize is actually
supported by the sensor. I would do that in RkISP1Path::validate() as
suggested below ?

>  	} 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, sz);
> +	if (sensorConfig) {
> +		int ret = data_->sensor_->applyConfiguration(*sensorConfig,
> +							     combinedTransform_,
> +							     &sensorFormat_);

I would avoid applying a sensor configuration at validate() but only
do so at configure(), check what RPi does if you need examples.

> +		if (ret) {
> +			LOG(RkISP1, Error)
> +				<< "Sensor Configuration not supported: "
> +				<< strerror(-ret);
> +
> +			return Invalid;
> +		}
> +	}
>
>  	if (sensorFormat_.size.isNull())
>  		sensorFormat_.size = sensor->resolution();
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 7c1f0c73..30becb75 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -253,8 +253,10 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
>  	return cfg;
>  }
>
> -CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> -						 StreamConfiguration *cfg)
> +CameraConfiguration::Status
> +RkISP1Path::validate(const CameraSensor *sensor,
> +		     std::optional<SensorConfiguration> sensorConfig,

Does copy-constructing an std::optional<> copy the managed instance ?

From my understanding of
https://en.cppreference.com/w/cpp/utility/optional/optional

Copy constructor: If other contains a value, initializes the contained
value as if direct-initializing (but not direct-list-initializing) an
object of type T with the expression *other

it does.

In other word, we're not only copying the std::optional<> wrapper
(which should in this case point to some dynamically allocated memory
for the managed object) but the whole object itself ?

I would pass it by reference ?

> +		     StreamConfiguration *cfg)
>  {
>  	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
>  	Size resolution = filterSensorResolution(sensor);
> @@ -281,12 +283,18 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>  			    mbusCodes.end())
>  				continue;
>
> -			/*
> -			 * Store the raw format with the highest bits per pixel
> -			 * for later usage.
> +			/* If the bits per pixel is supplied from the sensor

                        /*
			 * 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.
>  			 */
>  			const PixelFormatInfo &info = PixelFormatInfo::info(format);
> -			if (info.bitsPerPixel > rawBitsPerPixel) {
> +
> +			if (sensorConfig &&
> +			    info.bitsPerPixel == sensorConfig->bitDepth) {
> +				rawFormat = format;
> +				rawBitsPerPixel = info.bitsPerPixel;
> +			} else if (info.bitsPerPixel > rawBitsPerPixel) {
>  				rawBitsPerPixel = info.bitsPerPixel;
>  				rawFormat = format;

A few lines below we have

	if (isRaw) {
		/*
		 * Use the sensor output size closest to the requested stream
		 * size.
		 */
		uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
		V4L2SubdeviceFormat sensorFormat =
			sensor->getFormat({ mbusCode }, cfg->size);

		minResolution = sensorFormat.size;
		maxResolution = sensorFormat.size;

If a SensorConfig is specified, shouldn't the size there provided
be used ? Also, make sure that 'sensorFormat' size is the one
specified in the sensorConfig. IOW the sensorConfig.size should be
supported exactly by the sensor, otherwise we should fail as
sensorConfig shall not be adjusted.

Once we have validated that the (optional) sensorConfig.size is
supported by the sensor, the StreamConfig::size will be then adjusted
to it with:

	cfg->size.boundTo(maxResolution);
	cfg->size.expandTo(minResolution);

a few lines below

If a sensorFormat is there, the RAW stream should be adjusted to match
it, and you can use it to select the sensor format in
RkISP1CameraConfiguration::validate().

Thanks
  j


>  			}
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index 9f75fe1f..8aae00ef 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -27,6 +27,7 @@ namespace libcamera {
>  class CameraSensor;
>  class MediaDevice;
>  class V4L2Subdevice;
> +class SensorConfiguration;
>  struct StreamConfiguration;
>  struct V4L2SubdeviceFormat;
>
> @@ -45,6 +46,7 @@ public:
>  						  const Size &resolution,
>  						  StreamRole role);
>  	CameraConfiguration::Status validate(const CameraSensor *sensor,
> +					     std::optional<SensorConfiguration> sensorConfig,
>  					     StreamConfiguration *cfg);
>
>  	int configure(const StreamConfiguration &config,
> --
> 2.45.0
>