[libcamera-devel] [RFC PATCH v2] pipeline: rkisp1: Query the driver for formats

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Thu, Jul 21, 2022 at 01:40:04AM +0900, Paul Elder via libcamera-devel wrote:
> Query the driver for the output formats that it supports, instead of
> hardcoding it. While at it, cache the framesizes as well.

The cached sizes are not used, and I don't foresee a use for them in the
future. I would drop them, but I think you should use the enumerated
sizes in RkISP1Path::populateFormats() to replace the minResolution_ and
maxResolution_ values.

> 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>
> 
> ---
> Changes in v2:
> - 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 | 50 +++++++++++++++++--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  3 ++
>  2 files changed, 48 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 6f175758..cf5feebe 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(video_);

No need to pass it to the function, you can access video_ internally.

> +
>  	link_ = media->link("rkisp1_isp", 2, resizer, 0);
>  	if (!link_)
>  		return false;
> @@ -48,15 +50,44 @@ bool RkISP1Path::init(MediaDevice *media)
>  	return true;
>  }
>  
> +void RkISP1Path::populateFormats(std::unique_ptr<V4L2VideoDevice> &video)
> +{
> +	V4L2VideoDevice::Formats v4l2Formats = video->formats();
> +	if (v4l2Formats.empty()) {
> +		LOG(RkISP1, Warning)
> +			<< "Failed to enumerate framesizes. Loading default framesizes";
> +
> +		for (const PixelFormat &format : formats_)
> +			streamFormats_[format] = { { minResolution_, maxResolution_ } };
> +		return;
> +	}
> +
> +	std::vector<PixelFormat> formats;
> +	for (auto pair : v4l2Formats) {
> +		const PixelFormat pixelFormat = pair.first.toPixelFormat();
> +		const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
> +
> +		/* \todo Add support for RAW formats. */
> +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> +			continue;
> +
> +		streamFormats_[pixelFormat] = pair.second;
> +	}
> +}
> +
>  StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)
>  {
>  	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
>  					   .boundedTo(resolution);
>  	Size minResolution = minResolution_.expandedToAspectRatio(resolution);
>  
> +	/*
> +	 * Can we use the global min/max resolutions here or does it need to be
> +	 * resized to the same aspect ratio as the requested resolution?
> +	 */

Is this a comment that explains the code below, or a question to the
reader to figure out if the code should be fixed ?

>  	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> -	for (const PixelFormat &format : formats_)
> -		streamFormats[format] = { { minResolution, maxResolution } };
> +	for (auto pair : streamFormats_)

	for (const auto &[format, sizes] : streamFormats_)

> +		streamFormats[pair.first] = { { minResolution, maxResolution } };
>  
>  	StreamFormats formats(streamFormats);
>  	StreamConfiguration cfg(formats);
> @@ -72,8 +103,14 @@ 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 (std::find_if(streamFormats_.begin(), streamFormats_.end(),
> +			 [cfg](auto pair) { return pair.first == cfg->pixelFormat; }) ==
> +	    streamFormats_.end())

It's a map :-)

	if (!streamFormats.count(cfg->pixelFormat))

>  		cfg->pixelFormat = formats::NV12;
>  
>  	cfg->size.boundTo(maxResolution_);
> @@ -207,6 +244,8 @@ void RkISP1Path::stop()
>  namespace {
>  constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };
>  constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };
> +
> +/* \todo Remove this eventually. */

/*
 * \todo Remove the hardcoded resolutions and formats once all users will have 
 * migrated to a recent enough kernel.
 */

and you can move this just before the namespace, and drop the duplicated
\todo below.

>  constexpr std::array<PixelFormat, 6> RKISP1_RSZ_MP_FORMATS{
>  	formats::YUYV,
>  	formats::NV16,
> @@ -214,11 +253,12 @@ constexpr std::array<PixelFormat, 6> RKISP1_RSZ_MP_FORMATS{
>  	formats::NV21,
>  	formats::NV12,
>  	formats::R8,
> -	/* \todo Add support for RAW formats. */
>  };
>  
>  constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
>  constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };
> +
> +/* \todo Remove this eventually. */
>  constexpr std::array<PixelFormat, 8> RKISP1_RSZ_SP_FORMATS{
>  	formats::YUYV,
>  	formats::NV16,
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index f3f1ae39..42db158f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -57,11 +57,14 @@ public:
>  	Signal<FrameBuffer *> &bufferReady() { return video_->bufferReady; }
>  
>  private:
> +	void populateFormats(std::unique_ptr<V4L2VideoDevice> &video);
> +
>  	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
>  
>  	const char *name_;
>  	bool running_;
>  
> +	std::map<PixelFormat, std::vector<SizeRange>> streamFormats_;
>  	const Span<const PixelFormat> formats_;
>  	const Size minResolution_;
>  	const Size maxResolution_;

-- 
Regards,

Laurent Pinchart