[libcamera-devel] [PATCH v3 10/13] pipeline: rkisp1: Query the driver for formats
Kieran Bingham
kieran.bingham at ideasonboard.com
Sat Oct 29 00:15:38 CEST 2022
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
> >
More information about the libcamera-devel
mailing list