[libcamera-devel] [RFC PATCH 08/10] libcamera: v4l2_videodevice: Create color-format planes in CreateBuffer()
Tomasz Figa
tfiga at chromium.org
Fri Aug 27 12:36:34 CEST 2021
On Thu, Aug 26, 2021 at 8:09 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Tomasz,
>
> On Thu, Aug 26, 2021 at 02:14:45PM +0900, Tomasz Figa wrote:
> > On Sat, Aug 21, 2021 at 2:34 AM Laurent Pinchart wrote:
> > > On Fri, Aug 20, 2021 at 07:32:54PM +0900, Hirokazu Honda wrote:
> > > > On Wed, Aug 18, 2021 at 10:40 AM Laurent Pinchart wrote:
> > > > > On Mon, Aug 16, 2021 at 01:31:36PM +0900, Hirokazu Honda wrote:
> > > > > > V4L2VideDevice::CreateBuffer() creates the same number of
> > > > >
> > > > > s/CreateBuffer/createBuffer/
> > > > >
> > > > > > FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2
> > > > > > format single is single-planar format, the created number of
> > > > > > FrameBuffer::Planes is 1.
> > > > > > It should rather create the same number of FrameBuffer::Planes as the
> > > > > > color format planes.
> > > > > >
> > > > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > > > ---
> > > > > > include/libcamera/internal/v4l2_videodevice.h | 4 ++-
> > > > > > src/libcamera/v4l2_videodevice.cpp | 28 +++++++++++++++++--
> > > > > > 2 files changed, 29 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > > > > index e767ec84..d0aeb614 100644
> > > > > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > > > > @@ -136,11 +136,13 @@ private:
> > > > > > private:
> > > > > > struct Plane {
> > > > > > Plane(const FrameBuffer::Plane &plane)
> > > > > > - : fd(plane.fd.fd()), length(plane.length)
> > > > > > + : fd(plane.fd.fd()), offset(plane.offset),
> > > > > > + length(plane.length)
> > > > > > {
> > > > > > }
> > > > > >
> > > > > > int fd;
> > > > > > + unsigned int offset;
> > > > > > unsigned int length;
> > > > > > };
> > > > > >
> > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > > > > index ce60dff6..549418c8 100644
> > > > > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > > > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > > > > @@ -25,6 +25,7 @@
> > > > > >
> > > > > > #include <libcamera/file_descriptor.h>
> > > > > >
> > > > > > +#include "libcamera/internal/formats.h"
> > > > > > #include "libcamera/internal/media_device.h"
> > > > > > #include "libcamera/internal/media_object.h"
> > > > > >
> > > > > > @@ -273,6 +274,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
> > > > > >
> > > > > > for (unsigned int i = 0; i < planes.size(); i++)
> > > > > > if (planes_[i].fd != planes[i].fd.fd() ||
> > > > > > + planes_[i].offset != planes_[i].offset ||
> > > > > > planes_[i].length != planes[i].length)
> > > > > > return false;
> > > > > > return true;
> > > > > > @@ -1283,12 +1285,34 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> > > > >
> > > > > It would be good to capture the fact that the FrameBuffer planes now
> > > > > match the colour planes and not the V4L2 planes. This function is
> > > > > private and isn't documented, so we can't handle it here, and should
> > > > > instead document this in V4L2VideoDevice::allocateBuffers() and
> > > > > V4L2VideoDevice::exportBuffers(). Both functions have a paragraph that
> > > > > state
> > > > >
> > > > > * The number of planes and the plane sizes for the allocation are determined
> > > > > * by the currently active format on the device as set by setFormat().
> > > > >
> > > > > It could be extended to
> > > > >
> > > > > * The number of planes and their offsets and sizes are determined by the
> > > > > * currently active format on the device as set by setFormat(). They do not map
> > > > > * to the V4L2 buffer planes, but to colour planes of the pixel format. For
> > > > > * instance, if the active format is formats::NV12, the allocated FrameBuffer
> > > > > * instances will have two planes, for the luma and chroma components,
> > > > > * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
> > > > > * V4L2_PIX_FMT_NV12M.
> > > > >
> > > > > >
> > > > > > FrameBuffer::Plane plane;
> > > > > > plane.fd = std::move(fd);
> > > > > > - plane.length = multiPlanar ?
> > > > > > - buf.m.planes[nplane].length : buf.length;
> > > > > > + plane.offset = multiPlanar ? buf.m.planes[nplane].data_offset : 0;
> > > > > > + plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;
> > > > >
> > > > > The data_offset field defined by V4L2 is, unfortunately, not the offset
> > > > > in the dmabuf, but the offset of the data from the start of the plane.
> > > > > It's used by codec drivers to indicate the size of the header stored at
> > > > > the beginning of the plane. The data_offset is actually contained in the
> > > > > length.
> > > > >
> > > > > I think we can ignore the data offset reported by V4L2, until the V4L2
> > > > > API is expanded with a "real" data offset. plane.offset can be hardcoded
> > > > > to 0. The change to operator== could also probably be dropped.
> > > >
> > > > Yeah, I have faced the problem in developing chromium video accelerated codecs.
> > > > Although I think setting offset to 0 is not correct either.. but no
> > > > way of avoiding problem here.
> > >
> > > Let's revisit this part of the code once V4L2 gets proper support for
> > > offsets :-)
> >
> > We'll probably have to carry a downstream patch to handle them
> > initially... Would it be possible to at least make it easy to do that
> > until proper V4L2 support gets merged?
> >
> > (v4l2_ext API is being worked on, but last time I checked it was stuck
> > in review, without any comments...)
>
> If there's significant progress with this development we can carry a
> downstream patch to avoid waiting for the API to be merged upstream, but
> it indeed seems stuck. What would be your use case for such a downstream
> patch though ? We would need drivers using the API to make it useful in
> libcamera.
We carry driver patches to use data_offset for plane offset.
The background is that for consistency with various graphics APIs,
Chrome OS always passes planes as duped FDs + offset + stride, despite
generally placing all the color planes inside the same DMA-buf. Also,
the offset doesn't necessarily match V4L2 non-M formats to achieve
more efficient line and/or plane alignment and so we often need to use
M formats with a way to pass plane offsets.
>
> > > > > >
> > > > > > planes.push_back(std::move(plane));
> > > > > > }
> > > > > >
> > > > > > + V4L2DeviceFormat format{};
> > > > > > + ret = getFormat(&format);
> > > > > > + if (ret < 0) {
> > > > > > + LOG(V4L2, Error)
> > > > > > + << "Failed to get format: " << strerror(-ret);
> > > > > > + return nullptr;
> > > > > > + }
> > > > > > +
> > > > >
> > > > > A comment to explain what's going on would be useful.
> > > > >
> > > > > > + const auto &info = PixelFormatInfo::info(format.fourcc);
> > > > > > + if (info.isValid() && info.numPlanes() != numPlanes) {
> > > > >
> > > > > If info isn't valid, should we return nullptr ?
> > > > >
> > > > > > + ASSERT(numPlanes == 1u);
> > > > > > + planes.resize(info.numPlanes());
> > > > > > + const FileDescriptor &fd = planes[0].fd;
> > > > > > + size_t offset = 0;
> > > > > > + for (size_t i = 0; i < info.numPlanes(); ++i) {
> > > > > > + planes[i].fd = fd;
> > > > > > + planes[i].offset = offset;
> > > > > > + planes[i].length = info.frameSize(format.size);
> > > > >
> > > > > Isn't it the plane size that we need here, not the full frame size ?
> > > > > Unless I'm mistaken, info.frameSize() on, for instance, an NV12 format,
> > > > > will return the total size of the image, including both colour planes.
> > > > >
> > > > > There's also one change missing. You're addressing the buffer allocation
> > > > > side, but not the buffer queuing side. Now that an NV12 FrameBuffer has
> > > > > two planes, we need to translate from two colour planes to one V4L2
> > > > > plane in V4L2VideoDevice::queueBuffer() if V4L2_PIX_FMT_NV12 is used.
> > > >
> > > > I am confused thanks to various "Plane" structures.
> > > > In queueBuffer(), FrameBuffer::Plane is used to get fd, isn't it?
> > > > FrameMetadata::Plane is used to fill other v4l2_buffer variables.
> > > > What shall I fix in queueBuffer()?
> > >
> > > A FrameBuffer containing V4L2_PIX_FMT_NV12 should have two colour
> > > planes, with the same dmabuf for both:
> > >
> > > planes[0].fd = dmabuf;
> > > planes[0].offset = 0;
> > > planes[0].length = stride * height;
> > > planes[1].fd = dmabuf;
> > > planes[1].offset = stride * height;
> > > planes[1].length = stride * height / 2;
> > >
> > > This is because V4L2_PIX_FMT_NV12 is contiguous in memory, unlike
> > > V4L2_PIX_FMT_NV12M. When queing such a buffer to the V4L2VideoDevice,
> > > V4L2 will expect a single buffer plane, with
> > >
> > > v4l2Planes[0].m.fd = dmabuf;
> > >
> > > Additionally, for output buffers, bytesused and length also need to be
> > > set, but we may not need to handle that right away.
> > >
> > > V4L2VideoDevice thus needs, for V4L2_PIX_FMT_NV12, to translate from the
> > > two colour planes in the FrameBuffer to the single V4L2 buffer plane.
> > > The translation is fairly easy, as it only requires reducing the number
> > > of planes (but the number of V4L2 buffer planes for V4L2_PIX_FMT_NV12
> > > isn't available anywhere, so we will need to add this information
> > > somewhere). We however needs to verify that the FrameBuffer is suitable
> > > for V4L2, and check that
> > >
> > > - planes[0].fd == planes[1].fd
> >
> > planes[0].fd != planes[1].fd doesn't always imply that they refer to a
> > different DMA-buf. I'd skip this check (or just emit a warning) and
> > rely on checking whether the offset matches.
>
> We can check the inode number with fstat() (I've tested that by dupping
> a dmabuf fd, and I get the same inode). It's probably overkill for now,
> but will likely be done at some point. The reason why I'd like this
> check to stay is to catch userspace that would give use different fds
> (whether or not they reference the same dmabuf), and then decide how to
> handle them.
>
> > Generally the two common patterns in V4L2 are:
> > 1) single plane with 1 DMA-buf for all color planes
> > 2) multiplane with 1 DMA-buf for _each_ color plane, with zero offset
> > within each
>
> I'd say that more than this being a common pattern, it's the only thing
> that V4L2 supports upstream :-)
>
> > I haven't seen any other case in practice, so it should be safe to
> > simply check based on the offset. The ext API will solve this by
> > moving the DMA-buf and offset check to the kernel.
> >
> > > - planes[0].offset == 0
> > > - planes[1].offset == planes[0].length
> > >
> > > and possibly also check the length. If the buffer isn't suitable,
> > > V4L2VideoDevice::queueBuffer() should return an error.
> > >
> > > > > > + offset += planes[i].length;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > return std::make_unique<FrameBuffer>(std::move(planes));
> > > > > > }
> > > > > >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list