[PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline

[Laurent Pinchart]

On Wed, Sep 11, 2024 at 11:52:36AM +0100, Kieran Bingham wrote:
> Quoting Umang Jain (2024-09-09 17:37:19)
> > It is possible that the maximum sensor size (returned by
> > CameraSensor::resolution()) is not supported by the pipeline. In such
> > cases, a filter function is required to filter out resolutions of the
> > camera sensor, which cannot be supported by the pipeline.
> > 
> > Introduce the filter function filterSensorResolution() in RkISP1Path
> > class and use it for validate() and generateConfiguration(). The
> > maximum sensor resolution is picked from that filtered set of
> > resolutions instead of CameraSensor::resolution().
> > 
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++-
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  3 ++
> >  2 files changed, 49 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > index 9053af18..b3b27aa5 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats()
> >         }
> >  }
> >  
> > +/**
> > + * \brief Filter the sensor resolutions that can be supported
> > + * \param[in] sensor The camera sensor
> > + *
> > + * This function retrieves all the sizes supported by the sensor and
> > + * filters all the resolutions that can be supported on the pipeline.
> > + * It is possible that the sensor's maximum output resolution is higher
> > + * than the ISP maximum input. In that case, this function filters out all
> > + * the resolution incapable of being supported and returns the maximum
> > + * sensor resolution that can be supported by the pipeline.
> > + *
> > + * \return Maximum sensor size supported on the pipeline
> > + */
> > +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor)
> > +{
> > +       if (!sensorSizes_.empty())
> > +               return sensorSizes_.back();

There are systems based on rkisp1 where multiple sensors are connected
to the same ISP. Obviously only one of them can be used at a time, but
applications can switch between them. This will be broken by caching
sensorSizes_.

> I like that we keep the full list of supported sensorSizes_. My first
> thought was that this list should be generated when the path is created,
> but this also seems fine to me.
> 
> I think there's no need to complicate / split this until there's
> actually a user of the full sensorSizes list ?
> 
> I'm a little surprised that's not not used actually. Do we report the
> raw modes we support somewhere? Should that iterate the 'sensorSizes_'
> as the supported sizes ?
> 
> Perhaps that's an improvement/fix on top though.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > +
> > +       std::vector<Size> sensorSizes;
> > +       const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> > +       for (const auto iter : mbusCodes) {
> > +               std::vector<Size> sizes = sensor->sizes(iter);
> > +               for (Size sz : sizes)
> > +                       sensorSizes.push_back(sz);
> > +       }
> > +
> > +       std::sort(sensorSizes.begin(), sensorSizes.end());
> > +
> > +       /* Remove duplicates. */
> > +       auto last = std::unique(sensorSizes.begin(), sensorSizes.end());
> > +       sensorSizes.erase(last, sensorSizes.end());
> > +
> > +       /* Discard any sizes that the pipeline is unable to support. */
> > +       for (auto sz : sensorSizes) {
> > +               if (sz.width > maxResolution_.width ||
> > +                   sz.height > maxResolution_.height)
> > +                       continue;
> > +
> > +               sensorSizes_.push_back(sz);
> > +       }
> > +
> > +       return sensorSizes_.back();
> > +}
> > +
> >  StreamConfiguration
> >  RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
> >                                   StreamRole role)
> >  {
> >         const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> > -       const Size &resolution = sensor->resolution();
> > +       Size resolution = filterSensorResolution(sensor);
> >  
> >         /* Min and max resolutions to populate the available stream formats. */
> >         Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> >                                                  StreamConfiguration *cfg)
> >  {
> >         const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> > -       const Size &resolution = sensor->resolution();
> > +       Size resolution = filterSensorResolution(sensor);
> >  
> >         const StreamConfiguration reqCfg = *cfg;
> >         CameraConfiguration::Status status = CameraConfiguration::Valid;
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > index 13ba4b62..03ff9543 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > @@ -63,6 +63,7 @@ public:
> >  
> >  private:
> >         void populateFormats();
> > +       Size filterSensorResolution(const CameraSensor *sensor);
> >  
> >         static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> >  
> > @@ -77,6 +78,8 @@ private:
> >         std::unique_ptr<V4L2Subdevice> resizer_;
> >         std::unique_ptr<V4L2VideoDevice> video_;
> >         MediaLink *link_;
> > +
> > +       std::vector<Size> sensorSizes_;
> >  };
> >  
> >  class RkISP1MainPath : public RkISP1Path

-- 
Regards,

Laurent Pinchart