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

16710 diff mbox series

Query the driver for the output formats that it supports, instead of
hardcoding it. While at it, cache the framesizes as well.

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>

---
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(-)

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@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_;