[PATCH v4 1/1] libcamera: rkisp1: Integrate SensorConfiguration support
Umang Jain
umang.jain at ideasonboard.com
Tue Oct 8 11:47:14 CEST 2024
Hi Jacopo
On 08/10/24 2:24 pm, Jacopo Mondi wrote:
> 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 ?
you're absolutely correct here. It should be sensorFormat, not refCfg,
as reqCfg can get adjusted.
Will address in v5.
>
>> + 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