[libcamera-devel] [PATCH v2] v4l2: V4L2CameraProxy: Add support for DMABUF buffer I/O in REQBUF ioctl

[Laurent Pinchart]

On Tue, Nov 30, 2021 at 07:28:39PM +0900, paul.elder at ideasonboard.com wrote:
> On Tue, Nov 30, 2021 at 03:02:15PM +0530, Vedant Paranjape wrote:
> > Hi Laurent,
> > I wanted to send this as an RFC, my bad.
> 
> If it was an RFC it would've be nice to know it was an RFC :D
> 
> > 
> > On Tue, Nov 30, 2021 at 2:55 PM Laurent Pinchart
> > <laurent.pinchart at ideasonboard.com> wrote:
> > >
> > > Hi Vedant,
> > >
> > > On Tue, Nov 30, 2021 at 02:28:30PM +0530, Vedant Paranjape wrote:
> > > > To support importing DMABUF, we need to reqbufs with DMABUF mode.
> > > >
> > > > This patch enables using V4L2_MEMORY_DMABUF as one of the memory types
> > > > in vidioc_reqbuf to initialise DMA Buffer I/O
> > >
> > > Have you tested that this patch enables DMABUF support ?
> > >
> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=89
> > > >
> > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> > > > ---
> > > >  src/v4l2/v4l2_camera_proxy.cpp | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > > > index f194e06345b7..49db766d9f9d 100644
> > > > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > > @@ -160,7 +160,8 @@ bool V4L2CameraProxy::validateBufferType(uint32_t type)
> > > >
> > > >  bool V4L2CameraProxy::validateMemoryType(uint32_t memory)
> > > >  {
> > > > -     return memory == V4L2_MEMORY_MMAP;
> > > > +     return (memory == V4L2_MEMORY_MMAP) ||
> > > > +            (memory == V4L2_MEMORY_DMABUF);
> > >
> > > No need for parentheses.
> > >
> > > As pointed by Paul, this introduces an issue in the EXPBUF
> > > implementation.
> > >
> > > Please stop ignoring review comments when sending new versions, or I'll
> > > have to ignore your patches.
> > 
> > I'm extremely sorry, didn't mean to ignore. I had a chat with Paul
> > regarding solving this, I wanted to send this as an RFC instead.
> 
> So clearly this patch depends on the other stuff that you were planning
> to send separately from this patch. Which means that on this patch you
> need to (besides mentioning that it's RFC) mention that you depend on X
> features that you'll send patches for later.

And I don't think there's more feedback that can be provided on this
patch alone without the rest of the implementation, we need to look at
it in its entirety.

RFC submissions should make it clear what comments are requested, and
why they're sent as RFCs and not normal patches.

> > > >  }
> > > >
> > > >  void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
> > > > @@ -468,7 +469,8 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
> > > >       if (!hasOwnership(file) && owner_)
> > > >               return -EBUSY;
> > > >
> > > > -     arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
> > > > +     arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP |
> > > > +                         V4L2_BUF_CAP_SUPPORTS_DMABUF;
> > > >       memset(arg->reserved, 0, sizeof(arg->reserved));
> > > >
> > > >       if (arg->count == 0) {
> > > > @@ -511,7 +513,7 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
> > > >               struct v4l2_buffer buf = {};
> > > >               buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > > >               buf.length = v4l2PixFormat_.sizeimage;
> > > > -             buf.memory = V4L2_MEMORY_MMAP;
> > > > +             buf.memory = arg->memory;
> > > >               buf.m.offset = i * v4l2PixFormat_.sizeimage;
> > > >               buf.index = i;
> > > >               buf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;

-- 
Regards,

Laurent Pinchart