[libcamera-devel] [PATCH v3 23/30] cam: drm: Support per-plane stride values

[Kieran Bingham]

On 06/09/2021 23:56, Laurent Pinchart wrote:
> The stride is not always identical for all planes for multi-planar
> formats. Semi-planar YUV formats without horizontal subsampling often
> have a chroma stride equal to twice the luma stride, and tri-planar YUV
> formats with a 1/2 horizontal subsampling often have a chroma stride
> equal to half the luma stride. This isn't correctly taken into account
> when creating a DRM frame buffer, as the same stride is set for all
> planes.
> 
> libcamera doesn't report per-plane stride values yet, but uses chroma
> strides that match the above description for all currently supported
> platforms. Calculation the chrome strides appropriately in the KMSSink
> class, and pass them to DRM::createFrameBuffer().
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/cam/drm.cpp      |  7 +++----
>  src/cam/drm.h        |  5 ++++-
>  src/cam/kms_sink.cpp | 28 +++++++++++++++++++++++++++-
>  3 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> index da317e27cb19..ac47b8bd3287 100644
> --- a/src/cam/drm.cpp
> +++ b/src/cam/drm.cpp
> @@ -595,12 +595,12 @@ const Object *Device::object(uint32_t id)
>  std::unique_ptr<FrameBuffer> Device::createFrameBuffer(
>  	const libcamera::FrameBuffer &buffer,
>  	const libcamera::PixelFormat &format,
> -	const libcamera::Size &size, unsigned int stride)
> +	const libcamera::Size &size,
> +	const std::array<uint32_t, 4> &strides)
>  {
>  	std::unique_ptr<FrameBuffer> fb{ new FrameBuffer(this) };
>  
>  	uint32_t handles[4] = {};
> -	uint32_t pitches[4] = {};
>  	uint32_t offsets[4] = {};
>  	int ret;
>  
> @@ -623,13 +623,12 @@ std::unique_ptr<FrameBuffer> Device::createFrameBuffer(
>  		fb->planes_.push_back({ handle });
>  
>  		handles[i] = handle;
> -		pitches[i] = stride;
>  		offsets[i] = 0; /* TODO */
>  		++i;
>  	}
>  
>  	ret = drmModeAddFB2(fd_, size.width, size.height, format.fourcc(), handles,
> -			    pitches, offsets, &fb->id_, 0);
> +			    strides.data(), offsets, &fb->id_, 0);
>  	if (ret < 0) {
>  		ret = -errno;
>  		std::cerr
> diff --git a/src/cam/drm.h b/src/cam/drm.h
> index ee2304025208..00f7e798b771 100644
> --- a/src/cam/drm.h
> +++ b/src/cam/drm.h
> @@ -7,9 +7,11 @@
>  #ifndef __CAM_DRM_H__
>  #define __CAM_DRM_H__
>  
> +#include <array>
>  #include <list>
>  #include <map>
>  #include <memory>
> +#include <stdint.h>
>  #include <string>
>  #include <vector>
>  
> @@ -298,7 +300,8 @@ public:
>  	std::unique_ptr<FrameBuffer> createFrameBuffer(
>  		const libcamera::FrameBuffer &buffer,
>  		const libcamera::PixelFormat &format,
> -		const libcamera::Size &size, unsigned int stride);
> +		const libcamera::Size &size,
> +		const std::array<uint32_t, 4> &strides);
>  
>  	libcamera::Signal<AtomicRequest *> requestComplete;
>  
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> index 8c0b79c63922..658192efc105 100644
> --- a/src/cam/kms_sink.cpp
> +++ b/src/cam/kms_sink.cpp
> @@ -7,10 +7,12 @@
>  
>  #include "kms_sink.h"
>  
> +#include <array>
>  #include <algorithm>
>  #include <assert.h>
>  #include <iostream>
>  #include <memory>
> +#include <stdint.h>
>  #include <string.h>
>  
>  #include <libcamera/camera.h>
> @@ -65,8 +67,32 @@ KMSSink::KMSSink(const std::string &connectorName)
>  
>  void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)
>  {
> +	std::array<uint32_t, 4> strides = {};
> +
> +	/* \todo Should libcamera report per-plane strides ? */

Do you mean from the FrameBufferAllocator here?
Or to report the stride enforced from the V4L2 Device in some way?

But yes - if there are underlying stride requirements, they'll need to
be reported or discoverable right ?

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +	unsigned int uvStrideMultiplier;
> +
> +	switch (format_) {
> +	case libcamera::formats::NV24:
> +	case libcamera::formats::NV42:
> +		uvStrideMultiplier = 4;
> +		break;
> +	case libcamera::formats::YUV420:
> +	case libcamera::formats::YVU420:
> +	case libcamera::formats::YUV422:
> +		uvStrideMultiplier = 1;
> +		break;
> +	default:
> +		uvStrideMultiplier = 2;
> +		break;
> +	}
> +
> +	strides[0] = stride_;
> +	for (unsigned int i = 1; i < buffer->planes().size(); ++i)
> +		strides[i] = stride_ * uvStrideMultiplier / 2;
> +
>  	std::unique_ptr<DRM::FrameBuffer> drmBuffer =
> -		dev_.createFrameBuffer(*buffer, format_, size_, stride_);
> +		dev_.createFrameBuffer(*buffer, format_, size_, strides);
>  	if (!drmBuffer)
>  		return;
>  
>