Message ID | 20221024000356.29521-12-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
On Mon, Oct 24, 2022 at 03:03:54AM +0300, Laurent Pinchart wrote: > Unlike RkISP1Path::generateConfiguration(), the validate() function > doesn't take the camera sensor resolution into account but only > considers the absolute minimum and maximum sizes support by the ISP to s/support/supported/ > validate the stream size. Fix it by using the same logic as when > generating the configuration. > > Instead of passing the sensor resolution to the validate() function, > pass the CameraSensor pointer to prepare for subsequent changes that > will require access to more camera sensor data. While at it, update the > generateConfiguration() function similarly for the same reason. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++++------- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 22 +++++++++++++++---- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 6 +++-- > 3 files changed, 31 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index cca89cc13bff..dcab5286aa56 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -404,14 +404,15 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg) > { > + const CameraSensor *sensor = data_->sensor_.get(); > StreamConfiguration config; > > config = cfg; > - if (data_->mainPath_->validate(&config) != Valid) > + if (data_->mainPath_->validate(sensor, &config) != Valid) > return false; > > config = cfg; > - if (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid) > + if (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid) > return false; > > return true; > @@ -459,7 +460,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > /* Try to match stream without adjusting configuration. */ > if (mainPathAvailable) { > StreamConfiguration tryCfg = cfg; > - if (data_->mainPath_->validate(&tryCfg) == Valid) { > + if (data_->mainPath_->validate(sensor, &tryCfg) == Valid) { > mainPathAvailable = false; > cfg = tryCfg; > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); > @@ -469,7 +470,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > if (selfPathAvailable) { > StreamConfiguration tryCfg = cfg; > - if (data_->selfPath_->validate(&tryCfg) == Valid) { > + if (data_->selfPath_->validate(sensor, &tryCfg) == Valid) { > selfPathAvailable = false; > cfg = tryCfg; > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); > @@ -480,7 +481,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > /* Try to match stream allowing adjusting configuration. */ > if (mainPathAvailable) { > StreamConfiguration tryCfg = cfg; > - if (data_->mainPath_->validate(&tryCfg) == Adjusted) { > + if (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) { > mainPathAvailable = false; > cfg = tryCfg; > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); > @@ -491,7 +492,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > if (selfPathAvailable) { > StreamConfiguration tryCfg = cfg; > - if (data_->selfPath_->validate(&tryCfg) == Adjusted) { > + if (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) { > selfPathAvailable = false; > cfg = tryCfg; > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); > @@ -603,11 +604,11 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera, > StreamConfiguration cfg; > if (useMainPath) { > cfg = data->mainPath_->generateConfiguration( > - data->sensor_->resolution()); > + data->sensor_.get()); > mainPathAvailable = false; > } else { > cfg = data->selfPath_->generateConfiguration( > - data->sensor_->resolution()); > + data->sensor_.get()); > selfPathAvailable = false; > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index 8077a54494a5..cc2ac66e6939 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -12,6 +12,7 @@ > #include <libcamera/formats.h> > #include <libcamera/stream.h> > > +#include "libcamera/internal/camera_sensor.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/v4l2_subdevice.h" > #include "libcamera/internal/v4l2_videodevice.h" > @@ -85,8 +86,10 @@ void RkISP1Path::populateFormats() > } > } > > -StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > +StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor) > { > + const Size &resolution = sensor->resolution(); > + > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > .boundedTo(resolution); > Size minResolution = minResolution_.expandedToAspectRatio(resolution); > @@ -104,8 +107,11 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > return cfg; > } > > -CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > +CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > + StreamConfiguration *cfg) > { > + const Size &resolution = sensor->resolution(); > + > const StreamConfiguration reqCfg = *cfg; > CameraConfiguration::Status status = CameraConfiguration::Valid; > > @@ -117,8 +123,16 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > if (!streamFormats_.count(cfg->pixelFormat)) > cfg->pixelFormat = formats::NV12; > > - cfg->size.boundTo(maxResolution_); > - cfg->size.expandTo(minResolution_); > + /* > + * Adjust the size based on the sensor resolution and absolute limits > + * of the ISP. > + */ > + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > + .boundedTo(resolution); > + Size minResolution = minResolution_.expandedToAspectRatio(resolution); > + > + cfg->size.boundTo(maxResolution); > + cfg->size.expandTo(minResolution); > cfg->bufferCount = RKISP1_BUFFER_COUNT; > > V4L2DeviceFormat format; > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index d88effbb6f56..bf4ad18fbbf2 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -23,6 +23,7 @@ > > namespace libcamera { > > +class CameraSensor; > class MediaDevice; > class V4L2Subdevice; > struct StreamConfiguration; > @@ -39,8 +40,9 @@ public: > int setEnabled(bool enable) { return link_->setEnabled(enable); } > bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; } > > - StreamConfiguration generateConfiguration(const Size &resolution); > - CameraConfiguration::Status validate(StreamConfiguration *cfg); > + StreamConfiguration generateConfiguration(const CameraSensor *sensor); > + CameraConfiguration::Status validate(const CameraSensor *sensor, > + StreamConfiguration *cfg); > > int configure(const StreamConfiguration &config, > const V4L2SubdeviceFormat &inputFormat); > -- > Regards, > > Laurent Pinchart >
Hi Laurent On Mon, Oct 24, 2022 at 03:03:54AM +0300, Laurent Pinchart via libcamera-devel wrote: > Unlike RkISP1Path::generateConfiguration(), the validate() function > doesn't take the camera sensor resolution into account but only > considers the absolute minimum and maximum sizes support by the ISP to > validate the stream size. Fix it by using the same logic as when > generating the configuration. > > Instead of passing the sensor resolution to the validate() function, > pass the CameraSensor pointer to prepare for subsequent changes that > will require access to more camera sensor data. While at it, update the > generateConfiguration() function similarly for the same reason. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++++------- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 22 +++++++++++++++---- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 6 +++-- > 3 files changed, 31 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index cca89cc13bff..dcab5286aa56 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -404,14 +404,15 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg) > { > + const CameraSensor *sensor = data_->sensor_.get(); > StreamConfiguration config; > > config = cfg; > - if (data_->mainPath_->validate(&config) != Valid) > + if (data_->mainPath_->validate(sensor, &config) != Valid) > return false; > > config = cfg; > - if (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid) > + if (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid) > return false; > > return true; > @@ -459,7 +460,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > /* Try to match stream without adjusting configuration. */ > if (mainPathAvailable) { > StreamConfiguration tryCfg = cfg; > - if (data_->mainPath_->validate(&tryCfg) == Valid) { > + if (data_->mainPath_->validate(sensor, &tryCfg) == Valid) { > mainPathAvailable = false; > cfg = tryCfg; > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); > @@ -469,7 +470,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > if (selfPathAvailable) { > StreamConfiguration tryCfg = cfg; > - if (data_->selfPath_->validate(&tryCfg) == Valid) { > + if (data_->selfPath_->validate(sensor, &tryCfg) == Valid) { > selfPathAvailable = false; > cfg = tryCfg; > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); > @@ -480,7 +481,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > /* Try to match stream allowing adjusting configuration. */ > if (mainPathAvailable) { > StreamConfiguration tryCfg = cfg; > - if (data_->mainPath_->validate(&tryCfg) == Adjusted) { > + if (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) { > mainPathAvailable = false; > cfg = tryCfg; > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); > @@ -491,7 +492,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > if (selfPathAvailable) { > StreamConfiguration tryCfg = cfg; > - if (data_->selfPath_->validate(&tryCfg) == Adjusted) { > + if (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) { > selfPathAvailable = false; > cfg = tryCfg; > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); > @@ -603,11 +604,11 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera, > StreamConfiguration cfg; > if (useMainPath) { > cfg = data->mainPath_->generateConfiguration( > - data->sensor_->resolution()); > + data->sensor_.get()); > mainPathAvailable = false; > } else { > cfg = data->selfPath_->generateConfiguration( > - data->sensor_->resolution()); > + data->sensor_.get()); > selfPathAvailable = false; > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index 8077a54494a5..cc2ac66e6939 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -12,6 +12,7 @@ > #include <libcamera/formats.h> > #include <libcamera/stream.h> > > +#include "libcamera/internal/camera_sensor.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/v4l2_subdevice.h" > #include "libcamera/internal/v4l2_videodevice.h" > @@ -85,8 +86,10 @@ void RkISP1Path::populateFormats() > } > } > > -StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > +StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor) > { > + const Size &resolution = sensor->resolution(); > + > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > .boundedTo(resolution); > Size minResolution = minResolution_.expandedToAspectRatio(resolution); > @@ -104,8 +107,11 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > return cfg; > } > > -CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > +CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > + StreamConfiguration *cfg) > { > + const Size &resolution = sensor->resolution(); > + > const StreamConfiguration reqCfg = *cfg; > CameraConfiguration::Status status = CameraConfiguration::Valid; > > @@ -117,8 +123,16 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > if (!streamFormats_.count(cfg->pixelFormat)) > cfg->pixelFormat = formats::NV12; > > - cfg->size.boundTo(maxResolution_); > - cfg->size.expandTo(minResolution_); > + /* > + * Adjust the size based on the sensor resolution and absolute limits > + * of the ISP. > + */ > + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > + .boundedTo(resolution); > + Size minResolution = minResolution_.expandedToAspectRatio(resolution); > + > + cfg->size.boundTo(maxResolution); > + cfg->size.expandTo(minResolution); > cfg->bufferCount = RKISP1_BUFFER_COUNT; > > V4L2DeviceFormat format; > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index d88effbb6f56..bf4ad18fbbf2 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -23,6 +23,7 @@ > > namespace libcamera { > > +class CameraSensor; > class MediaDevice; > class V4L2Subdevice; > struct StreamConfiguration; > @@ -39,8 +40,9 @@ public: > int setEnabled(bool enable) { return link_->setEnabled(enable); } > bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; } > > - StreamConfiguration generateConfiguration(const Size &resolution); > - CameraConfiguration::Status validate(StreamConfiguration *cfg); > + StreamConfiguration generateConfiguration(const CameraSensor *sensor); > + CameraConfiguration::Status validate(const CameraSensor *sensor, > + StreamConfiguration *cfg); > > int configure(const StreamConfiguration &config, > const V4L2SubdeviceFormat &inputFormat); > -- > Regards, > > Laurent Pinchart >
Quoting Laurent Pinchart via libcamera-devel (2022-10-24 01:03:54) > Unlike RkISP1Path::generateConfiguration(), the validate() function > doesn't take the camera sensor resolution into account but only > considers the absolute minimum and maximum sizes support by the ISP to > validate the stream size. Fix it by using the same logic as when > generating the configuration. > > Instead of passing the sensor resolution to the validate() function, > pass the CameraSensor pointer to prepare for subsequent changes that > will require access to more camera sensor data. While at it, update the > generateConfiguration() function similarly for the same reason. I have quite the surprising curveball on this patch. While the validation seems to aim to restrict sizes to the capabilities of the ISP and the Sensor size, it seems to miss something. I've hooked up an Arducam 16MP to test this - and the set up fails because the sensor size selected is larger than the maximum input size of the ISP. Can this somehow take the input limtiations of the ISP into account when selecting and filtering sensor sizes easily ? -- Kieran > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++++------- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 22 +++++++++++++++---- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 6 +++-- > 3 files changed, 31 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index cca89cc13bff..dcab5286aa56 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -404,14 +404,15 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg) > { > + const CameraSensor *sensor = data_->sensor_.get(); > StreamConfiguration config; > > config = cfg; > - if (data_->mainPath_->validate(&config) != Valid) > + if (data_->mainPath_->validate(sensor, &config) != Valid) > return false; > > config = cfg; > - if (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid) > + if (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid) > return false; > > return true; > @@ -459,7 +460,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > /* Try to match stream without adjusting configuration. */ > if (mainPathAvailable) { > StreamConfiguration tryCfg = cfg; > - if (data_->mainPath_->validate(&tryCfg) == Valid) { > + if (data_->mainPath_->validate(sensor, &tryCfg) == Valid) { > mainPathAvailable = false; > cfg = tryCfg; > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); > @@ -469,7 +470,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > if (selfPathAvailable) { > StreamConfiguration tryCfg = cfg; > - if (data_->selfPath_->validate(&tryCfg) == Valid) { > + if (data_->selfPath_->validate(sensor, &tryCfg) == Valid) { > selfPathAvailable = false; > cfg = tryCfg; > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); > @@ -480,7 +481,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > /* Try to match stream allowing adjusting configuration. */ > if (mainPathAvailable) { > StreamConfiguration tryCfg = cfg; > - if (data_->mainPath_->validate(&tryCfg) == Adjusted) { > + if (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) { > mainPathAvailable = false; > cfg = tryCfg; > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); > @@ -491,7 +492,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > if (selfPathAvailable) { > StreamConfiguration tryCfg = cfg; > - if (data_->selfPath_->validate(&tryCfg) == Adjusted) { > + if (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) { > selfPathAvailable = false; > cfg = tryCfg; > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); > @@ -603,11 +604,11 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera, > StreamConfiguration cfg; > if (useMainPath) { > cfg = data->mainPath_->generateConfiguration( > - data->sensor_->resolution()); > + data->sensor_.get()); > mainPathAvailable = false; > } else { > cfg = data->selfPath_->generateConfiguration( > - data->sensor_->resolution()); > + data->sensor_.get()); > selfPathAvailable = false; > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index 8077a54494a5..cc2ac66e6939 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -12,6 +12,7 @@ > #include <libcamera/formats.h> > #include <libcamera/stream.h> > > +#include "libcamera/internal/camera_sensor.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/v4l2_subdevice.h" > #include "libcamera/internal/v4l2_videodevice.h" > @@ -85,8 +86,10 @@ void RkISP1Path::populateFormats() > } > } > > -StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > +StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor) > { > + const Size &resolution = sensor->resolution(); > + > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > .boundedTo(resolution); > Size minResolution = minResolution_.expandedToAspectRatio(resolution); > @@ -104,8 +107,11 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > return cfg; > } > > -CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > +CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > + StreamConfiguration *cfg) > { > + const Size &resolution = sensor->resolution(); > + > const StreamConfiguration reqCfg = *cfg; > CameraConfiguration::Status status = CameraConfiguration::Valid; > > @@ -117,8 +123,16 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > if (!streamFormats_.count(cfg->pixelFormat)) > cfg->pixelFormat = formats::NV12; > > - cfg->size.boundTo(maxResolution_); > - cfg->size.expandTo(minResolution_); > + /* > + * Adjust the size based on the sensor resolution and absolute limits > + * of the ISP. > + */ > + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > + .boundedTo(resolution); I played around here and expected that limiting this maxResolution to the maximum Input resolution should help - but I'm missing something and haven't delved deep enough to figure out what yet. > + Size minResolution = minResolution_.expandedToAspectRatio(resolution); > + > + cfg->size.boundTo(maxResolution); > + cfg->size.expandTo(minResolution); > cfg->bufferCount = RKISP1_BUFFER_COUNT; > > V4L2DeviceFormat format; > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index d88effbb6f56..bf4ad18fbbf2 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -23,6 +23,7 @@ > > namespace libcamera { > > +class CameraSensor; > class MediaDevice; > class V4L2Subdevice; > struct StreamConfiguration; > @@ -39,8 +40,9 @@ public: > int setEnabled(bool enable) { return link_->setEnabled(enable); } > bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; } > > - StreamConfiguration generateConfiguration(const Size &resolution); > - CameraConfiguration::Status validate(StreamConfiguration *cfg); > + StreamConfiguration generateConfiguration(const CameraSensor *sensor); > + CameraConfiguration::Status validate(const CameraSensor *sensor, > + StreamConfiguration *cfg); > > int configure(const StreamConfiguration &config, > const V4L2SubdeviceFormat &inputFormat); > -- > Regards, > > Laurent Pinchart >
Hi Kieran, On Fri, Oct 28, 2022 at 11:11:06PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart via libcamera-devel (2022-10-24 01:03:54) > > Unlike RkISP1Path::generateConfiguration(), the validate() function > > doesn't take the camera sensor resolution into account but only > > considers the absolute minimum and maximum sizes support by the ISP to > > validate the stream size. Fix it by using the same logic as when > > generating the configuration. > > > > Instead of passing the sensor resolution to the validate() function, > > pass the CameraSensor pointer to prepare for subsequent changes that > > will require access to more camera sensor data. While at it, update the > > generateConfiguration() function similarly for the same reason. > > I have quite the surprising curveball on this patch. > > While the validation seems to aim to restrict sizes to the capabilities > of the ISP and the Sensor size, it seems to miss something. > > I've hooked up an Arducam 16MP to test this - and the set up fails > because the sensor size selected is larger than the maximum input size > of the ISP. > > Can this somehow take the input limtiations of the ISP into account when > selecting and filtering sensor sizes easily ? Is this a regression ? > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++++------- > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 22 +++++++++++++++---- > > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 6 +++-- > > 3 files changed, 31 insertions(+), 14 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index cca89cc13bff..dcab5286aa56 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -404,14 +404,15 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > > > bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg) > > { > > + const CameraSensor *sensor = data_->sensor_.get(); > > StreamConfiguration config; > > > > config = cfg; > > - if (data_->mainPath_->validate(&config) != Valid) > > + if (data_->mainPath_->validate(sensor, &config) != Valid) > > return false; > > > > config = cfg; > > - if (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid) > > + if (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid) > > return false; > > > > return true; > > @@ -459,7 +460,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > /* Try to match stream without adjusting configuration. */ > > if (mainPathAvailable) { > > StreamConfiguration tryCfg = cfg; > > - if (data_->mainPath_->validate(&tryCfg) == Valid) { > > + if (data_->mainPath_->validate(sensor, &tryCfg) == Valid) { > > mainPathAvailable = false; > > cfg = tryCfg; > > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); > > @@ -469,7 +470,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > > if (selfPathAvailable) { > > StreamConfiguration tryCfg = cfg; > > - if (data_->selfPath_->validate(&tryCfg) == Valid) { > > + if (data_->selfPath_->validate(sensor, &tryCfg) == Valid) { > > selfPathAvailable = false; > > cfg = tryCfg; > > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); > > @@ -480,7 +481,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > /* Try to match stream allowing adjusting configuration. */ > > if (mainPathAvailable) { > > StreamConfiguration tryCfg = cfg; > > - if (data_->mainPath_->validate(&tryCfg) == Adjusted) { > > + if (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) { > > mainPathAvailable = false; > > cfg = tryCfg; > > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); > > @@ -491,7 +492,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > > if (selfPathAvailable) { > > StreamConfiguration tryCfg = cfg; > > - if (data_->selfPath_->validate(&tryCfg) == Adjusted) { > > + if (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) { > > selfPathAvailable = false; > > cfg = tryCfg; > > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); > > @@ -603,11 +604,11 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera, > > StreamConfiguration cfg; > > if (useMainPath) { > > cfg = data->mainPath_->generateConfiguration( > > - data->sensor_->resolution()); > > + data->sensor_.get()); > > mainPathAvailable = false; > > } else { > > cfg = data->selfPath_->generateConfiguration( > > - data->sensor_->resolution()); > > + data->sensor_.get()); > > selfPathAvailable = false; > > } > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > index 8077a54494a5..cc2ac66e6939 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > @@ -12,6 +12,7 @@ > > #include <libcamera/formats.h> > > #include <libcamera/stream.h> > > > > +#include "libcamera/internal/camera_sensor.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/v4l2_subdevice.h" > > #include "libcamera/internal/v4l2_videodevice.h" > > @@ -85,8 +86,10 @@ void RkISP1Path::populateFormats() > > } > > } > > > > -StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > > +StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor) > > { > > + const Size &resolution = sensor->resolution(); > > + > > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > > .boundedTo(resolution); > > Size minResolution = minResolution_.expandedToAspectRatio(resolution); > > @@ -104,8 +107,11 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > > return cfg; > > } > > > > -CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > > +CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > > + StreamConfiguration *cfg) > > { > > + const Size &resolution = sensor->resolution(); > > + > > const StreamConfiguration reqCfg = *cfg; > > CameraConfiguration::Status status = CameraConfiguration::Valid; > > > > @@ -117,8 +123,16 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > > if (!streamFormats_.count(cfg->pixelFormat)) > > cfg->pixelFormat = formats::NV12; > > > > - cfg->size.boundTo(maxResolution_); > > - cfg->size.expandTo(minResolution_); > > + /* > > + * Adjust the size based on the sensor resolution and absolute limits > > + * of the ISP. > > + */ > > + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > > + .boundedTo(resolution); > > I played around here and expected that limiting this maxResolution to > the maximum Input resolution should help - but I'm missing something and > haven't delved deep enough to figure out what yet. > > > + Size minResolution = minResolution_.expandedToAspectRatio(resolution); > > + > > + cfg->size.boundTo(maxResolution); > > + cfg->size.expandTo(minResolution); > > cfg->bufferCount = RKISP1_BUFFER_COUNT; > > > > V4L2DeviceFormat format; > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > index d88effbb6f56..bf4ad18fbbf2 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > @@ -23,6 +23,7 @@ > > > > namespace libcamera { > > > > +class CameraSensor; > > class MediaDevice; > > class V4L2Subdevice; > > struct StreamConfiguration; > > @@ -39,8 +40,9 @@ public: > > int setEnabled(bool enable) { return link_->setEnabled(enable); } > > bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; } > > > > - StreamConfiguration generateConfiguration(const Size &resolution); > > - CameraConfiguration::Status validate(StreamConfiguration *cfg); > > + StreamConfiguration generateConfiguration(const CameraSensor *sensor); > > + CameraConfiguration::Status validate(const CameraSensor *sensor, > > + StreamConfiguration *cfg); > > > > int configure(const StreamConfiguration &config, > > const V4L2SubdeviceFormat &inputFormat);
Quoting Laurent Pinchart (2022-10-30 17:24:49) > Hi Kieran, > > On Fri, Oct 28, 2022 at 11:11:06PM +0100, Kieran Bingham wrote: > > Quoting Laurent Pinchart via libcamera-devel (2022-10-24 01:03:54) > > > Unlike RkISP1Path::generateConfiguration(), the validate() function > > > doesn't take the camera sensor resolution into account but only > > > considers the absolute minimum and maximum sizes support by the ISP to > > > validate the stream size. Fix it by using the same logic as when > > > generating the configuration. > > > > > > Instead of passing the sensor resolution to the validate() function, > > > pass the CameraSensor pointer to prepare for subsequent changes that > > > will require access to more camera sensor data. While at it, update the > > > generateConfiguration() function similarly for the same reason. > > > > I have quite the surprising curveball on this patch. > > > > While the validation seems to aim to restrict sizes to the capabilities > > of the ISP and the Sensor size, it seems to miss something. > > > > I've hooked up an Arducam 16MP to test this - and the set up fails > > because the sensor size selected is larger than the maximum input size > > of the ISP. > > > > Can this somehow take the input limtiations of the ISP into account when > > selecting and filtering sensor sizes easily ? > > Is this a regression ? I wasn't able to test this camera before on this platform. So it's not a regression for me, it's just never worked... But ... if we're "fixing the validation of the stream size", I'd call this a bug report that this patch doesn't seem to account for the ISP limitations when doing so. I've worked around this by removing the larger image sizes from the sensor driver currently... (Then I hit an issue where the Pixelrate of the sensor is not supported by the ISP ... so something else is wrong there). > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++++------- > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 22 +++++++++++++++---- > > > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 6 +++-- > > > 3 files changed, 31 insertions(+), 14 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index cca89cc13bff..dcab5286aa56 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -404,14 +404,15 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > > > > > bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg) > > > { > > > + const CameraSensor *sensor = data_->sensor_.get(); > > > StreamConfiguration config; > > > > > > config = cfg; > > > - if (data_->mainPath_->validate(&config) != Valid) > > > + if (data_->mainPath_->validate(sensor, &config) != Valid) > > > return false; > > > > > > config = cfg; > > > - if (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid) > > > + if (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid) > > > return false; > > > > > > return true; > > > @@ -459,7 +460,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > /* Try to match stream without adjusting configuration. */ > > > if (mainPathAvailable) { > > > StreamConfiguration tryCfg = cfg; > > > - if (data_->mainPath_->validate(&tryCfg) == Valid) { > > > + if (data_->mainPath_->validate(sensor, &tryCfg) == Valid) { > > > mainPathAvailable = false; > > > cfg = tryCfg; > > > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); > > > @@ -469,7 +470,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > > > > if (selfPathAvailable) { > > > StreamConfiguration tryCfg = cfg; > > > - if (data_->selfPath_->validate(&tryCfg) == Valid) { > > > + if (data_->selfPath_->validate(sensor, &tryCfg) == Valid) { > > > selfPathAvailable = false; > > > cfg = tryCfg; > > > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); > > > @@ -480,7 +481,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > /* Try to match stream allowing adjusting configuration. */ > > > if (mainPathAvailable) { > > > StreamConfiguration tryCfg = cfg; > > > - if (data_->mainPath_->validate(&tryCfg) == Adjusted) { > > > + if (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) { > > > mainPathAvailable = false; > > > cfg = tryCfg; > > > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); > > > @@ -491,7 +492,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > > > > if (selfPathAvailable) { > > > StreamConfiguration tryCfg = cfg; > > > - if (data_->selfPath_->validate(&tryCfg) == Adjusted) { > > > + if (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) { > > > selfPathAvailable = false; > > > cfg = tryCfg; > > > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); > > > @@ -603,11 +604,11 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera, > > > StreamConfiguration cfg; > > > if (useMainPath) { > > > cfg = data->mainPath_->generateConfiguration( > > > - data->sensor_->resolution()); > > > + data->sensor_.get()); > > > mainPathAvailable = false; > > > } else { > > > cfg = data->selfPath_->generateConfiguration( > > > - data->sensor_->resolution()); > > > + data->sensor_.get()); > > > selfPathAvailable = false; > > > } > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > index 8077a54494a5..cc2ac66e6939 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > @@ -12,6 +12,7 @@ > > > #include <libcamera/formats.h> > > > #include <libcamera/stream.h> > > > > > > +#include "libcamera/internal/camera_sensor.h" > > > #include "libcamera/internal/media_device.h" > > > #include "libcamera/internal/v4l2_subdevice.h" > > > #include "libcamera/internal/v4l2_videodevice.h" > > > @@ -85,8 +86,10 @@ void RkISP1Path::populateFormats() > > > } > > > } > > > > > > -StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > > > +StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor) > > > { > > > + const Size &resolution = sensor->resolution(); > > > + > > > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > > > .boundedTo(resolution); > > > Size minResolution = minResolution_.expandedToAspectRatio(resolution); > > > @@ -104,8 +107,11 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > > > return cfg; > > > } > > > > > > -CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > > > +CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > > > + StreamConfiguration *cfg) > > > { > > > + const Size &resolution = sensor->resolution(); > > > + > > > const StreamConfiguration reqCfg = *cfg; > > > CameraConfiguration::Status status = CameraConfiguration::Valid; > > > > > > @@ -117,8 +123,16 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > > > if (!streamFormats_.count(cfg->pixelFormat)) > > > cfg->pixelFormat = formats::NV12; > > > > > > - cfg->size.boundTo(maxResolution_); > > > - cfg->size.expandTo(minResolution_); > > > + /* > > > + * Adjust the size based on the sensor resolution and absolute limits > > > + * of the ISP. > > > + */ > > > + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > > > + .boundedTo(resolution); > > > > I played around here and expected that limiting this maxResolution to > > the maximum Input resolution should help - but I'm missing something and > > haven't delved deep enough to figure out what yet. > > > > > + Size minResolution = minResolution_.expandedToAspectRatio(resolution); > > > + > > > + cfg->size.boundTo(maxResolution); > > > + cfg->size.expandTo(minResolution); > > > cfg->bufferCount = RKISP1_BUFFER_COUNT; > > > > > > V4L2DeviceFormat format; > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > > index d88effbb6f56..bf4ad18fbbf2 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > > @@ -23,6 +23,7 @@ > > > > > > namespace libcamera { > > > > > > +class CameraSensor; > > > class MediaDevice; > > > class V4L2Subdevice; > > > struct StreamConfiguration; > > > @@ -39,8 +40,9 @@ public: > > > int setEnabled(bool enable) { return link_->setEnabled(enable); } > > > bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; } > > > > > > - StreamConfiguration generateConfiguration(const Size &resolution); > > > - CameraConfiguration::Status validate(StreamConfiguration *cfg); > > > + StreamConfiguration generateConfiguration(const CameraSensor *sensor); > > > + CameraConfiguration::Status validate(const CameraSensor *sensor, > > > + StreamConfiguration *cfg); > > > > > > int configure(const StreamConfiguration &config, > > > const V4L2SubdeviceFormat &inputFormat); > > -- > Regards, > > Laurent Pinchart
Hi Kieran, On Wed, Nov 23, 2022 at 09:56:06AM +0000, Kieran Bingham wrote: > Quoting Laurent Pinchart (2022-10-30 17:24:49) > > On Fri, Oct 28, 2022 at 11:11:06PM +0100, Kieran Bingham wrote: > > > Quoting Laurent Pinchart via libcamera-devel (2022-10-24 01:03:54) > > > > Unlike RkISP1Path::generateConfiguration(), the validate() function > > > > doesn't take the camera sensor resolution into account but only > > > > considers the absolute minimum and maximum sizes support by the ISP to > > > > validate the stream size. Fix it by using the same logic as when > > > > generating the configuration. > > > > > > > > Instead of passing the sensor resolution to the validate() function, > > > > pass the CameraSensor pointer to prepare for subsequent changes that > > > > will require access to more camera sensor data. While at it, update the > > > > generateConfiguration() function similarly for the same reason. > > > > > > I have quite the surprising curveball on this patch. > > > > > > While the validation seems to aim to restrict sizes to the capabilities > > > of the ISP and the Sensor size, it seems to miss something. > > > > > > I've hooked up an Arducam 16MP to test this - and the set up fails > > > because the sensor size selected is larger than the maximum input size > > > of the ISP. > > > > > > Can this somehow take the input limtiations of the ISP into account when > > > selecting and filtering sensor sizes easily ? > > > > Is this a regression ? > > I wasn't able to test this camera before on this platform. So it's not a > regression for me, it's just never worked... > > But ... if we're "fixing the validation of the stream size", I'd call > this a bug report that this patch doesn't seem to account for the ISP > limitations when doing so. No disagreement there :-) What I'm wondering is if it has to be fixed in here, or could be addressed on top. And of course if someone with access to a 16MP camera module could do it, that would be even better ;-) > I've worked around this by removing the larger image sizes from the > sensor driver currently... (Then I hit an issue where the Pixelrate of > the sensor is not supported by the ISP ... so something else is wrong > there). > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++++------- > > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 22 +++++++++++++++---- > > > > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 6 +++-- > > > > 3 files changed, 31 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > index cca89cc13bff..dcab5286aa56 100644 > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > @@ -404,14 +404,15 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > > > > > > > bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg) > > > > { > > > > + const CameraSensor *sensor = data_->sensor_.get(); > > > > StreamConfiguration config; > > > > > > > > config = cfg; > > > > - if (data_->mainPath_->validate(&config) != Valid) > > > > + if (data_->mainPath_->validate(sensor, &config) != Valid) > > > > return false; > > > > > > > > config = cfg; > > > > - if (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid) > > > > + if (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid) > > > > return false; > > > > > > > > return true; > > > > @@ -459,7 +460,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > > /* Try to match stream without adjusting configuration. */ > > > > if (mainPathAvailable) { > > > > StreamConfiguration tryCfg = cfg; > > > > - if (data_->mainPath_->validate(&tryCfg) == Valid) { > > > > + if (data_->mainPath_->validate(sensor, &tryCfg) == Valid) { > > > > mainPathAvailable = false; > > > > cfg = tryCfg; > > > > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); > > > > @@ -469,7 +470,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > > > > > > if (selfPathAvailable) { > > > > StreamConfiguration tryCfg = cfg; > > > > - if (data_->selfPath_->validate(&tryCfg) == Valid) { > > > > + if (data_->selfPath_->validate(sensor, &tryCfg) == Valid) { > > > > selfPathAvailable = false; > > > > cfg = tryCfg; > > > > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); > > > > @@ -480,7 +481,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > > /* Try to match stream allowing adjusting configuration. */ > > > > if (mainPathAvailable) { > > > > StreamConfiguration tryCfg = cfg; > > > > - if (data_->mainPath_->validate(&tryCfg) == Adjusted) { > > > > + if (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) { > > > > mainPathAvailable = false; > > > > cfg = tryCfg; > > > > cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); > > > > @@ -491,7 +492,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > > > > > > if (selfPathAvailable) { > > > > StreamConfiguration tryCfg = cfg; > > > > - if (data_->selfPath_->validate(&tryCfg) == Adjusted) { > > > > + if (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) { > > > > selfPathAvailable = false; > > > > cfg = tryCfg; > > > > cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); > > > > @@ -603,11 +604,11 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera, > > > > StreamConfiguration cfg; > > > > if (useMainPath) { > > > > cfg = data->mainPath_->generateConfiguration( > > > > - data->sensor_->resolution()); > > > > + data->sensor_.get()); > > > > mainPathAvailable = false; > > > > } else { > > > > cfg = data->selfPath_->generateConfiguration( > > > > - data->sensor_->resolution()); > > > > + data->sensor_.get()); > > > > selfPathAvailable = false; > > > > } > > > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > > index 8077a54494a5..cc2ac66e6939 100644 > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > > @@ -12,6 +12,7 @@ > > > > #include <libcamera/formats.h> > > > > #include <libcamera/stream.h> > > > > > > > > +#include "libcamera/internal/camera_sensor.h" > > > > #include "libcamera/internal/media_device.h" > > > > #include "libcamera/internal/v4l2_subdevice.h" > > > > #include "libcamera/internal/v4l2_videodevice.h" > > > > @@ -85,8 +86,10 @@ void RkISP1Path::populateFormats() > > > > } > > > > } > > > > > > > > -StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > > > > +StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor) > > > > { > > > > + const Size &resolution = sensor->resolution(); > > > > + > > > > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > > > > .boundedTo(resolution); > > > > Size minResolution = minResolution_.expandedToAspectRatio(resolution); > > > > @@ -104,8 +107,11 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) > > > > return cfg; > > > > } > > > > > > > > -CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > > > > +CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > > > > + StreamConfiguration *cfg) > > > > { > > > > + const Size &resolution = sensor->resolution(); > > > > + > > > > const StreamConfiguration reqCfg = *cfg; > > > > CameraConfiguration::Status status = CameraConfiguration::Valid; > > > > > > > > @@ -117,8 +123,16 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) > > > > if (!streamFormats_.count(cfg->pixelFormat)) > > > > cfg->pixelFormat = formats::NV12; > > > > > > > > - cfg->size.boundTo(maxResolution_); > > > > - cfg->size.expandTo(minResolution_); > > > > + /* > > > > + * Adjust the size based on the sensor resolution and absolute limits > > > > + * of the ISP. > > > > + */ > > > > + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > > > > + .boundedTo(resolution); > > > > > > I played around here and expected that limiting this maxResolution to > > > the maximum Input resolution should help - but I'm missing something and > > > haven't delved deep enough to figure out what yet. > > > > > > > + Size minResolution = minResolution_.expandedToAspectRatio(resolution); > > > > + > > > > + cfg->size.boundTo(maxResolution); > > > > + cfg->size.expandTo(minResolution); > > > > cfg->bufferCount = RKISP1_BUFFER_COUNT; > > > > > > > > V4L2DeviceFormat format; > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > > > index d88effbb6f56..bf4ad18fbbf2 100644 > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > > > @@ -23,6 +23,7 @@ > > > > > > > > namespace libcamera { > > > > > > > > +class CameraSensor; > > > > class MediaDevice; > > > > class V4L2Subdevice; > > > > struct StreamConfiguration; > > > > @@ -39,8 +40,9 @@ public: > > > > int setEnabled(bool enable) { return link_->setEnabled(enable); } > > > > bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; } > > > > > > > > - StreamConfiguration generateConfiguration(const Size &resolution); > > > > - CameraConfiguration::Status validate(StreamConfiguration *cfg); > > > > + StreamConfiguration generateConfiguration(const CameraSensor *sensor); > > > > + CameraConfiguration::Status validate(const CameraSensor *sensor, > > > > + StreamConfiguration *cfg); > > > > > > > > int configure(const StreamConfiguration &config, > > > > const V4L2SubdeviceFormat &inputFormat);
Quoting Laurent Pinchart (2022-11-23 11:06:09) > Hi Kieran, > > On Wed, Nov 23, 2022 at 09:56:06AM +0000, Kieran Bingham wrote: > > Quoting Laurent Pinchart (2022-10-30 17:24:49) > > > On Fri, Oct 28, 2022 at 11:11:06PM +0100, Kieran Bingham wrote: > > > > Quoting Laurent Pinchart via libcamera-devel (2022-10-24 01:03:54) > > > > > Unlike RkISP1Path::generateConfiguration(), the validate() function > > > > > doesn't take the camera sensor resolution into account but only > > > > > considers the absolute minimum and maximum sizes support by the ISP to > > > > > validate the stream size. Fix it by using the same logic as when > > > > > generating the configuration. > > > > > > > > > > Instead of passing the sensor resolution to the validate() function, > > > > > pass the CameraSensor pointer to prepare for subsequent changes that > > > > > will require access to more camera sensor data. While at it, update the > > > > > generateConfiguration() function similarly for the same reason. > > > > > > > > I have quite the surprising curveball on this patch. > > > > > > > > While the validation seems to aim to restrict sizes to the capabilities > > > > of the ISP and the Sensor size, it seems to miss something. > > > > > > > > I've hooked up an Arducam 16MP to test this - and the set up fails > > > > because the sensor size selected is larger than the maximum input size > > > > of the ISP. > > > > > > > > Can this somehow take the input limtiations of the ISP into account when > > > > selecting and filtering sensor sizes easily ? > > > > > > Is this a regression ? > > > > I wasn't able to test this camera before on this platform. So it's not a > > regression for me, it's just never worked... > > > > But ... if we're "fixing the validation of the stream size", I'd call > > this a bug report that this patch doesn't seem to account for the ISP > > limitations when doing so. > > No disagreement there :-) What I'm wondering is if it has to be fixed in > here, or could be addressed on top. And of course if someone with access > to a 16MP camera module could do it, that would be even better ;-) It can be done on top. -- Kieran
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index cca89cc13bff..dcab5286aa56 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -404,14 +404,15 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg) { + const CameraSensor *sensor = data_->sensor_.get(); StreamConfiguration config; config = cfg; - if (data_->mainPath_->validate(&config) != Valid) + if (data_->mainPath_->validate(sensor, &config) != Valid) return false; config = cfg; - if (data_->selfPath_ && data_->selfPath_->validate(&config) != Valid) + if (data_->selfPath_ && data_->selfPath_->validate(sensor, &config) != Valid) return false; return true; @@ -459,7 +460,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() /* Try to match stream without adjusting configuration. */ if (mainPathAvailable) { StreamConfiguration tryCfg = cfg; - if (data_->mainPath_->validate(&tryCfg) == Valid) { + if (data_->mainPath_->validate(sensor, &tryCfg) == Valid) { mainPathAvailable = false; cfg = tryCfg; cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); @@ -469,7 +470,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() if (selfPathAvailable) { StreamConfiguration tryCfg = cfg; - if (data_->selfPath_->validate(&tryCfg) == Valid) { + if (data_->selfPath_->validate(sensor, &tryCfg) == Valid) { selfPathAvailable = false; cfg = tryCfg; cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); @@ -480,7 +481,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() /* Try to match stream allowing adjusting configuration. */ if (mainPathAvailable) { StreamConfiguration tryCfg = cfg; - if (data_->mainPath_->validate(&tryCfg) == Adjusted) { + if (data_->mainPath_->validate(sensor, &tryCfg) == Adjusted) { mainPathAvailable = false; cfg = tryCfg; cfg.setStream(const_cast<Stream *>(&data_->mainPathStream_)); @@ -491,7 +492,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() if (selfPathAvailable) { StreamConfiguration tryCfg = cfg; - if (data_->selfPath_->validate(&tryCfg) == Adjusted) { + if (data_->selfPath_->validate(sensor, &tryCfg) == Adjusted) { selfPathAvailable = false; cfg = tryCfg; cfg.setStream(const_cast<Stream *>(&data_->selfPathStream_)); @@ -603,11 +604,11 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera, StreamConfiguration cfg; if (useMainPath) { cfg = data->mainPath_->generateConfiguration( - data->sensor_->resolution()); + data->sensor_.get()); mainPathAvailable = false; } else { cfg = data->selfPath_->generateConfiguration( - data->sensor_->resolution()); + data->sensor_.get()); selfPathAvailable = false; } diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp index 8077a54494a5..cc2ac66e6939 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp @@ -12,6 +12,7 @@ #include <libcamera/formats.h> #include <libcamera/stream.h> +#include "libcamera/internal/camera_sensor.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/v4l2_subdevice.h" #include "libcamera/internal/v4l2_videodevice.h" @@ -85,8 +86,10 @@ void RkISP1Path::populateFormats() } } -StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) +StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor) { + const Size &resolution = sensor->resolution(); + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) .boundedTo(resolution); Size minResolution = minResolution_.expandedToAspectRatio(resolution); @@ -104,8 +107,11 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution) return cfg; } -CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) +CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, + StreamConfiguration *cfg) { + const Size &resolution = sensor->resolution(); + const StreamConfiguration reqCfg = *cfg; CameraConfiguration::Status status = CameraConfiguration::Valid; @@ -117,8 +123,16 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg) if (!streamFormats_.count(cfg->pixelFormat)) cfg->pixelFormat = formats::NV12; - cfg->size.boundTo(maxResolution_); - cfg->size.expandTo(minResolution_); + /* + * Adjust the size based on the sensor resolution and absolute limits + * of the ISP. + */ + Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) + .boundedTo(resolution); + Size minResolution = minResolution_.expandedToAspectRatio(resolution); + + cfg->size.boundTo(maxResolution); + cfg->size.expandTo(minResolution); cfg->bufferCount = RKISP1_BUFFER_COUNT; V4L2DeviceFormat format; diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h index d88effbb6f56..bf4ad18fbbf2 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h @@ -23,6 +23,7 @@ namespace libcamera { +class CameraSensor; class MediaDevice; class V4L2Subdevice; struct StreamConfiguration; @@ -39,8 +40,9 @@ public: int setEnabled(bool enable) { return link_->setEnabled(enable); } bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; } - StreamConfiguration generateConfiguration(const Size &resolution); - CameraConfiguration::Status validate(StreamConfiguration *cfg); + StreamConfiguration generateConfiguration(const CameraSensor *sensor); + CameraConfiguration::Status validate(const CameraSensor *sensor, + StreamConfiguration *cfg); int configure(const StreamConfiguration &config, const V4L2SubdeviceFormat &inputFormat);
Unlike RkISP1Path::generateConfiguration(), the validate() function doesn't take the camera sensor resolution into account but only considers the absolute minimum and maximum sizes support by the ISP to validate the stream size. Fix it by using the same logic as when generating the configuration. Instead of passing the sensor resolution to the validate() function, pass the CameraSensor pointer to prepare for subsequent changes that will require access to more camera sensor data. While at it, update the generateConfiguration() function similarly for the same reason. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++++------- src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 22 +++++++++++++++---- src/libcamera/pipeline/rkisp1/rkisp1_path.h | 6 +++-- 3 files changed, 31 insertions(+), 14 deletions(-)