[libcamera-devel] [PATCH v3 23/30] cam: drm: Support per-plane stride values
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 7 16:28:16 CEST 2021
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
More information about the libcamera-devel
mailing list