[libcamera-devel] [RFC PATCH v2 3/5] libcamera: v4l2 device: Store buffer info in planes

[Kieran Bingham]

Quoting Vedant Paranjape (2023-08-21 15:43:55)
> Hello Gabby,
> 
> Thanks for your patch. This patch looks good to me.
> 
> On Mon, Aug 21, 2023 at 6:40 PM Gabby George <gabbymg94 at gmail.com> wrote:
> >
> > To perform a memory mapping using mmap, the MappedFrameBuffer class
> > needs the plane offset and file descriptor information of the frame
> > buffer's plane(s). This information is provided in the response to
> > REQBUF, which happens during buffer allocation. Store the plane offset
> > and file descriptor information in the buffer's plane at the time of
> > allocation.
> >
> > Currently, there is a metadata buffer type (metadata format UVCH) that
> > does not support exporting buffers using EXPBUF, so this should only
> > be done if the buffer type is metadata capture.
> >
> > Signed-off-by: Gabby George <gabbymg94 at gmail.com>
> 
> Reviewed-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> 
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 31 +++++++++++++++++++-----------
> >  1 file changed, 20 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index a72ef64d..e57cb131 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1402,18 +1402,27 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> >
> >         std::vector<FrameBuffer::Plane> planes;
> >         for (unsigned int nplane = 0; nplane < numPlanes; nplane++) {
> > -               UniqueFD fd = exportDmabufFd(buf.index, nplane);
> > -               if (!fd.isValid())
> > -                       return nullptr;
> > -
> >                 FrameBuffer::Plane plane;
> > -               plane.fd = SharedFD(std::move(fd));
> > -               /*
> > -                * V4L2 API doesn't provide dmabuf offset information of plane.
> > -                * Set 0 as a placeholder offset.
> > -                * \todo Set the right offset once V4L2 API provides a way.
> > -                */
> > -               plane.offset = 0;
> > +
> > +               if (buf.type == V4L2_BUF_TYPE_META_CAPTURE) {
> > +                       /*
> > +                        * Dmabuf fd is not exported for metadata, so store
> > +                        * the offset from the querybuf call and this device's fd.
> > +                        */
> > +                       plane.fd = SharedFD(this->fd());
> > +                       plane.offset = buf.m.offset;
> > +               } else {
> > +                       UniqueFD fd = exportDmabufFd(buf.index, nplane);
> > +                       if (!fd.isValid())
> > +                               return nullptr;
> > +                       plane.fd = SharedFD(std::move(fd));
> > +                       /*
> > +                       * V4L2 API doesn't provide dmabuf offset information of plane.

This indent is still incorrect.
 /*
  *
instead of 
 /*
 *


> > +                       * Set 0 as a placeholder offset.
> > +                       * \todo Set the right offset once V4L2 API provides a way.
> > +                       */
> > +                       plane.offset = 0;

So are plane.offset's *always* zero ? Does anything else ever set it to
something else? That looks like it signals the same information as
'usePlaneOffset' to me in that case ?

> > +               }
> >                 plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;
> >
> >                 planes.push_back(std::move(plane));
> > --
> > 2.34.1
> >