[PATCH v5 1/1] libcamera: rkisp1: Integrate SensorConfiguration support
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Thu Oct 10 08:43:46 CEST 2024
Hi Laurent
On Wed, Oct 09, 2024 at 07:24:20PM GMT, 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 ?
>
> > + 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 ?
>
Right, I suggested this but this should have been
if (sensorConfig && !rawFormat)
return CameraConfiguration::Invalid;
instead
> > +
> > 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 ?
>
If the sensorConfig requested output size cannot be processed by the
ISP, I think we should fail yes
> > +
> > + 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 suggested this as well, as my thinking was that it is correct to first bound
down to a smaller res and then further reduce down to the desired aspect
ratio.
However, if we bound down to a smaller res, we already got the desired
aspect ratio, so the two ops are interchangeable probably ?
>
> > } 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,
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list