[libcamera-devel,v3,23/30] cam: drm: Support per-plane stride values
diff mbox series

Message ID 20210906225636.14683-23-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Handle fallout of FrameBuffer offset support
Related show

Commit Message

Laurent Pinchart Sept. 6, 2021, 10:56 p.m. UTC 
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@ideasonboard.com>
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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(-)

Comments

Kieran Bingham Sept. 7, 2021, 11:42 a.m. UTC | #1 
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@ideasonboard.com>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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@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;
>  
>
Laurent Pinchart Sept. 7, 2021, 2:28 p.m. UTC | #2 
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@ideasonboard.com>
> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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@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;
> >

Patch
diff mbox series 

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 ? */
+	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;