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

[Jacopo Mondi]

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>


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