[libcamera-devel] [PATCH v3 10/13] pipeline: rkisp1: Query the driver for formats

[Kieran Bingham]

Quoting Jacopo Mondi via libcamera-devel (2022-10-26 16:32:41)
> Hi Laurent
> 
> On Mon, Oct 24, 2022 at 03:03:53AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > From: Paul Elder <paul.elder at ideasonboard.com>
> >
> > Query the driver for the output formats and sizes that it supports,
> > instead of hardcoding them. This allows future-proofing for formats that
> > are supported by some but not all versions of the driver.
> >
> > As the rkisp1 driver currently does not support VIDIOC_ENUM_FRAMESIZES,
> > fallback to the hardcoded list of supported formats and framesizes. This
> > feature will be added to the driver in parallel, though we cannot
> > guarantee that users will have a new enough kernel for it.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > Changes since v2:
> >
> > - Don't pass V4L2VideoDevice to RkISP1Path::populateFormats()
> > - Use structured binding in for range loop
> > - Use std::set::count() to replace std::find_if()
> > - Don't cache sizes, use std::set instead of std::map
> > - Update min and max resolutions based on enumerated sizes
> > - Drop comment about aspect ratio
> >
> > Changes since v1:
> >
> > - Enumerate and cache framesizes as well
> > - Massage generateConfiguration accordingly
> > - This lets us skip modifying V4L2VideoDevice::formats() to support lack
> >   of ENUM_FRAMESIZES
> > - Also requires us to keep the list of hardcoded formats for backward
> >   compatibility
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 51 +++++++++++++++++--
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  8 ++-
> >  2 files changed, 54 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > index 2d38f0fb37ab..8077a54494a5 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > @@ -41,6 +41,8 @@ bool RkISP1Path::init(MediaDevice *media)
> >       if (video_->open() < 0)
> >               return false;
> >
> > +     populateFormats();
> > +
> >       link_ = media->link("rkisp1_isp", 2, resizer, 0);
> >       if (!link_)
> >               return false;
> > @@ -48,6 +50,41 @@ bool RkISP1Path::init(MediaDevice *media)
> >       return true;
> >  }
> >
> > +void RkISP1Path::populateFormats()
> > +{
> > +     V4L2VideoDevice::Formats v4l2Formats = video_->formats();
> > +     if (v4l2Formats.empty()) {
> > +             LOG(RkISP1, Warning)
> 
> This might warn users that something went wrong, while as the driver
> has been upstream without this feature for some time and there's
> nothing wrong. I would demote the error
> 
> This apart
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

It may be helpful to detect any input size limitations too ... but I
think they also need hardcoding in as defaults too.
--
Kieran


> 
> 
> > +                     << "Failed to enumerate framesizes. Loading default framesizes";
> > +
> > +             for (const PixelFormat &format : formats_)
> > +                     streamFormats_.insert(format);
> > +             return;
> > +     }
> > +
> > +     minResolution_ = { 65535, 65535 };
> > +     maxResolution_ = { 0, 0 };
> > +
> > +     std::vector<PixelFormat> formats;
> > +     for (const auto &[format, sizes] : v4l2Formats) {
> > +             const PixelFormat pixelFormat = format.toPixelFormat();
> > +             const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
> > +
> > +             /* \todo Add support for RAW formats. */
> > +             if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > +                     continue;
> > +
> > +             streamFormats_.insert(pixelFormat);
> > +
> > +             for (const auto &size : sizes) {
> > +                     if (minResolution_ > size.min)
> > +                             minResolution_ = size.min;
> > +                     if (maxResolution_ < size.max)
> > +                             maxResolution_ = size.max;
> > +             }
> > +     }
> > +}
> > +
> >  StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)
> >  {
> >       Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > @@ -55,7 +92,7 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)
> >       Size minResolution = minResolution_.expandedToAspectRatio(resolution);
> >
> >       std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> > -     for (const PixelFormat &format : formats_)
> > +     for (const auto &format : streamFormats_)
> >               streamFormats[format] = { { minResolution, maxResolution } };
> >
> >       StreamFormats formats(streamFormats);
> > @@ -72,8 +109,12 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg)
> >       const StreamConfiguration reqCfg = *cfg;
> >       CameraConfiguration::Status status = CameraConfiguration::Valid;
> >
> > -     if (std::find(formats_.begin(), formats_.end(), cfg->pixelFormat) ==
> > -         formats_.end())
> > +     /*
> > +      * Default to NV12 if the requested format is not supported. All
> > +      * versions of the ISP are guaranteed to support NV12 on both the main
> > +      * and self paths.
> > +      */
> > +     if (!streamFormats_.count(cfg->pixelFormat))
> >               cfg->pixelFormat = formats::NV12;
> >
> >       cfg->size.boundTo(maxResolution_);
> > @@ -204,6 +245,10 @@ void RkISP1Path::stop()
> >       running_ = false;
> >  }
> >
> > +/*
> > + * \todo Remove the hardcoded resolutions and formats once all users will have
> > + * migrated to a recent enough kernel.
> > + */
> >  namespace {
> >  constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };
> >  constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > index f3f1ae391d65..d88effbb6f56 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > @@ -8,6 +8,7 @@
> >  #pragma once
> >
> >  #include <memory>
> > +#include <set>
> >  #include <vector>
> >
> >  #include <libcamera/base/signal.h>
> > @@ -57,14 +58,17 @@ public:
> >       Signal<FrameBuffer *> &bufferReady() { return video_->bufferReady; }
> >
> >  private:
> > +     void populateFormats();
> > +
> >       static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> >
> >       const char *name_;
> >       bool running_;
> >
> >       const Span<const PixelFormat> formats_;
> > -     const Size minResolution_;
> > -     const Size maxResolution_;
> > +     std::set<PixelFormat> streamFormats_;
> > +     Size minResolution_;
> > +     Size maxResolution_;
> >
> >       std::unique_ptr<V4L2Subdevice> resizer_;
> >       std::unique_ptr<V4L2VideoDevice> video_;
> > --
> > Regards,
> >
> > Laurent Pinchart
> >