| Message ID | 20241004084128.14672-2-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
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) { + std::optional<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()); + } + 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; + } else if (info.bitsPerPixel > rawBitsPerPixel) { 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); + 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); } 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,
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@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(-)