[PATCH v3 1/1] libcamera: rkisp1: Integrate SensorConfiguration support
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Fri Oct 4 12:09:35 CEST 2024
Hi Umang
On Fri, Oct 04, 2024 at 02:11:27PM 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.
>
> The camera configuration will be marked as invalid when the sensor
> configuration is supplied, if:
> - Invalid sensor configuration (SensorConfiguration::isValid())
> - Bit depth not supported by RkISP1 pipeline
> - RAW stream configuration size doesn't matches sensor configuration
> output size
> - Sensor configuration output size is larger than maximum supported
> sensor configuration on RkISP1 pipeline
> - No matching sensor configuration output size supplied by the sensor
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 43 ++++++++++++++---
> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 47 ++++++++++++++++---
> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +
> 3 files changed, 79 insertions(+), 13 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index c02c7cf3..8c3d78dc 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -447,11 +447,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;
> @@ -468,6 +469,27 @@ 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) {
Can we unify the two if branches ?
> + std::optional<unsigned int> bitDepth = sensorConfig->bitDepth;
Why an optional ?
> + if (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {
> + LOG(RkISP1, Error)
> + << "Invalid sensor configuration bit depth";
> +
> + return Invalid;
> + }
> + }
> +
What about:
/*
* Make sure that if a sensor configuration has been requested it
* is valid.
*/
if (sensorConfig) {
if (!sensorConfig->isValid()) {
LOG(RkISP1, Error)
<< "Invalid sensor configuration request";
return Invalid;
}
unsigned int bitDepth = sensorConfig->bitDepth;
if (bitDepth != 8 && bitDepth != 10 && bitDepth != 12) {
LOG(RkISP1, Error)
<< "Invalid sensor configuration bit depth";
return Invalid;
}
}
> /* Cap the number of entries to the available streams. */
> if (config_.size() > pathCount) {
> config_.resize(pathCount);
> @@ -514,7 +536,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_));
> @@ -524,7 +546,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_));
> @@ -535,7 +557,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_));
> @@ -546,7 +568,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_));
> @@ -723,7 +745,14 @@ 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 {} I think
> if (ret < 0)
> return ret;
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index feb6d89f..20bff0a4 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -251,8 +251,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,
> + StreamConfiguration *cfg)
> {
> const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> Size resolution = filterSensorResolution(sensor);
> @@ -282,10 +284,16 @@ 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) {
> + if (sensorConfig &&
> + info.bitsPerPixel == sensorConfig->bitDepth) {
> + rawFormat = format;
> + rawBitsPerPixel = info.bitsPerPixel;
Should this be
if (sensorConfig && info.bitsPerPixel != sensorConfig->bitDepth)
continue;
Otherwise if (sensorConfig) and
(info.bitsPerPixel != sensorConfig->bitDepth)
> + } else if (info.bitsPerPixel > rawBitsPerPixel) {
We can still enter this branch and wrongly pick a format with a
bitDepth != than the one specified in sensorConfig ?
Then, if we exit the for() loop with "found == false && sensorConfig"
you should return invalid ?
> rawBitsPerPixel = info.bitsPerPixel;
> rawFormat = format;
> }
> @@ -319,13 +327,40 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> * size while ensuring the output size doesn't exceed ISP limits.
> */
> uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
> + Size rawSize = sensorConfig ? sensorConfig->outputSize
> + : cfg->size;
> cfg->size.boundTo(resolution);
>
> V4L2SubdeviceFormat sensorFormat =
> - sensor->getFormat({ mbusCode }, cfg->size);
> + sensor->getFormat({ mbusCode }, rawSize);
> +
> + if (sensorConfig &&
> + reqCfg.size != sensorConfig->outputSize)
> + return CameraConfiguration::Invalid;
>
> minResolution = sensorFormat.size;
> maxResolution = sensorFormat.size;
> + } else if (sensorConfig) {
> + /*
> + * We have already ensured 'rawFormat' has the matching bit
> + * depth with sensorConfig.bitDepth hence, only validate the
> + * sensorConfig's output size here.
> + */
> + uint32_t mbusCode = formatToMediaBus.at(rawFormat);
nit: you can declare this a few lines below before using it
> + Size sensorSize = sensorConfig->outputSize;
> +
> + if (sensorSize > resolution)
> + return CameraConfiguration::Invalid;
> +
> + V4L2SubdeviceFormat sensorFormat =
> + sensor->getFormat({ mbusCode }, sensorSize);
> +
> + if (sensorFormat.size != sensorSize)
> + return CameraConfiguration::Invalid;
> +
> + minResolution = minResolution_.expandedToAspectRatio(sensorSize);
> + maxResolution = maxResolution_.boundedToAspectRatio(sensorSize)
> + .boundedTo(sensorSize);
Isn't it better to first "boundedTo" to make sure maxResolutions_ is
smaller than sensorSize and then "boundedToAspectRatio" to further
shrink it to the aspect ratio of the sensorSize ?
Except the bitDepth handling at the beginning of this function, most
comments are nits, I presume the next version should be good to go
Thanks
j
> } else {
> /*
> * Adjust the size based on the sensor resolution and absolute
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index 777fcae7..ce9a5666 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;
>
> @@ -44,6 +45,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
>
More information about the libcamera-devel
mailing list