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

[Laurent Pinchart]

Hi Kieran,

On Tue, Sep 07, 2021 at 12:42:13PM +0100, Kieran Bingham wrote:
> 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?

We do report the stride in the stream configuration, but that's a single
value, not a per-plane value. That's what the comment is about, should
libcamera make it per-plane, or leave it to applications to compute
plane strides ?

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

-- 
Regards,

Laurent Pinchart