[PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration support
Umang Jain
umang.jain at ideasonboard.com
Fri Oct 11 11:27:42 CEST 2024
Hi Laurent,
On 09/10/24 9:54 pm, Laurent Pinchart wrote:
> On Tue, Oct 08, 2024 at 03:49:18PM +0530, 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 | 48 +++++++++++++++++--
>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 +
>> 3 files changed, 80 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 da8d25c3..3b5bea96 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,
> Shouldn't this be const ?
sent a fix
>
>> + 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;
> Why is this ? For a non-raw stream, why would we reject an invalid
> format instead of adjusting it when there's a sensor configuration ?
I misunderstood found, oops. Sent a fix to the list.
>
>> +
>> bool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==
>> PixelFormatInfo::ColourEncodingRAW;
>>
>> @@ -319,11 +329,39 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>> * size.
>> */
>> uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
>> + Size rawSize = sensorConfig ? sensorConfig->outputSize
>> + : cfg->size;
>> +
>> V4L2SubdeviceFormat sensorFormat =
>> - sensor->getFormat({ mbusCode }, cfg->size);
>> + sensor->getFormat({ mbusCode }, rawSize);
>> +
>> + if (sensorConfig &&
>> + sensorConfig->outputSize != sensorFormat.size)
>> + 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;
> Is this check needed ?
My idea is if sensor config size is passed such that it exceeds the
limits of the pipeline, it should be invalidated.
resolution here is the highest sensor resolution that's supported on the
pipeline.
>> +
>> + uint32_t mbusCode = formatToMediaBus.at(rawFormat);
>> + V4L2SubdeviceFormat sensorFormat =
>> + sensor->getFormat({ mbusCode }, sensorSize);
>> +
>> + if (sensorFormat.size != sensorSize)
>> + return CameraConfiguration::Invalid;
>> +
>> + minResolution = minResolution_.expandedToAspectRatio(sensorSize);
>> + maxResolution = maxResolution_.boundedTo(sensorSize)
>> + .boundedToAspectRatio(sensorSize);
> Below we have
>
> maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> .boundedTo(resolution);
>
> Is there a reason why you swap the calls here ?
I think it was Jacopo's review comment from v3:
```
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 ?
```
which made sense to me. Should we swap the below calls instead ?
>
>> } 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;
> Alphabatical order.
>
>> 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,
More information about the libcamera-devel
mailing list