[libcamera-devel] [PATCH v3 10/13] pipeline: rkisp1: Query the driver for formats
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Oct 30 17:46:27 CET 2022
Hello,
On Fri, Oct 28, 2022 at 11:15:38PM +0100, Kieran Bingham wrote:
> Quoting Jacopo Mondi via libcamera-devel (2022-10-26 16:32:41)
> > 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
Sounds good, I'll go for Info.
> > 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.
Patches are welcome :-)
> > > + << "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