[libcamera-devel] [PATCH v3 11/13] pipeline: rkisp1: Fix stream size validation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Nov 23 12:06:09 CET 2022
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 at 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
More information about the libcamera-devel
mailing list