[PATCH v4 1/1] libcamera: rkisp1: Integrate SensorConfiguration support
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Oct 8 10:54:14 CEST 2024
Hi Umang
On Fri, Oct 04, 2024 at 05:51:03PM 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 | 42 ++++++++++++++---
> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 47 +++++++++++++++++--
> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +
> 3 files changed, 79 insertions(+), 12 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index c02c7cf3..42961c12 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) {
> + 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,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 feb6d89f..c51554bc 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,9 +284,14 @@ 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 (sensorConfig && info.bitsPerPixel != sensorConfig->bitDepth)
> + continue;
> +
> if (info.bitsPerPixel > rawBitsPerPixel) {
> rawBitsPerPixel = info.bitsPerPixel;
> rawFormat = format;
> @@ -297,6 +304,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> }
> }
>
> + if (sensorConfig && !found)
> + return CameraConfiguration::Invalid;
> +
> bool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==
> PixelFormatInfo::ColourEncodingRAW;
>
> @@ -319,13 +329,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)
Should this be reqCfg or sensorFormat ?
If checking sensorConfig->outputSize agains "reqCfg" you're validating
that the requested RAW stream configuration has the desired sensor
output size.
If checking sensorFormat->size instead you're validating that the
requested sensorConfig->outputSize is supported by the sensor
natively.
I think this should be the latter, if reqCfg->size doesn't match the
sensorConfig->outputSize it will later be adjusted to it, and I think
that's fine (as long as we return Adjusted), but we should make sure
sensorFormat->size is exactly what has been requested with
sensorConfig ?
> + 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.
> + */
> + Size sensorSize = sensorConfig->outputSize;
> +
> + if (sensorSize > resolution)
> + return CameraConfiguration::Invalid;
Ah ok, I was about to suggest to move this just after initializing
'resolution', but in case of isRaw we don't go through the ISP, so the
ISP max input constraint doesn't apply
> +
> + uint32_t mbusCode = formatToMediaBus.at(rawFormat);
> + V4L2SubdeviceFormat sensorFormat =
> + sensor->getFormat({ mbusCode }, sensorSize);
> +
> + if (sensorFormat.size != sensorSize)
> + return CameraConfiguration::Invalid;
That's what I was suggesting to do in the raw case as well
One small push and we should be there!
Thanks
j
> +
> + minResolution = minResolution_.expandedToAspectRatio(sensorSize);
> + maxResolution = maxResolution_.boundedTo(sensorSize)
> + .boundedToAspectRatio(sensorSize);
> } 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.2
>
More information about the libcamera-devel
mailing list