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

17673 diff mbox series

From: Paul Elder <paul.elder@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@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@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(-)

Hi Laurent

On Mon, Oct 24, 2022 at 03:03:53AM +0300, Laurent Pinchart via libcamera-devel wrote:
> From: Paul Elder <paul.elder@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@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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@jmondi.org>


> +			<< "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
>

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@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@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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@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
> >

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@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@ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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@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_;