[libcamera-devel] [PATCH] v4l2: V4L2CameraProxy: Add EXPBUF as one of the supported ioctl

[Vedant Paranjape]

Hello Laurent,
Thanks for the review.

On Sun, 28 Nov, 2021, 01:30 Laurent Pinchart,
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Vedant,
>
> Thank you for the patch.
>
> On Sun, Nov 28, 2021 at 12:54:40AM +0530, Vedant Paranjape wrote:
> > To support DMABUF as one of the memory buffer, we need to implement
> > EXPBUF in the v4l2 compat layer.
> >
> > This patch implements vidioc_expbuf as one of the supported ioctls.
> >
> > Reference issue: https://bugs.libcamera.org/show_bug.cgi?id=89
>
> We use
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=89

Noted.

>
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 41 +++++++++++++++++++++++++++++++++-
> >  src/v4l2/v4l2_camera_proxy.h   |  1 +
> >  2 files changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index 3610e63cade3..03e3564bfed5 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);
>
> This has the side effect that, for instance, REQBUFS will accept the
> memory type DMABUF, but without DMABUF support actually implemented. Why
> do you need this change as part of this patch ?

Right, will remove it.

> >  }
> >
> >  void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
> > @@ -624,6 +625,40 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> >       return 0;
> >  }
> >
> > +int V4L2CameraProxy::vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg)
> > +{
> > +     LOG(V4L2Compat, Debug) << "Servicing vidioc_expbuf fd = " << file->efd();
> > +     /* \todo Verify that the memory type is MMAP when adding DMABUF support */
> > +     if (!validateBufferType(arg->type))
> > +             return -EINVAL;
> > +
> > +     if (arg->index >= bufferCount_)
> > +             return -EINVAL;
> > +
> > +     if (arg->flags & ~(O_CLOEXEC | O_ACCMODE))
> > +             return -EINVAL;
> > +
> > +     LOG(V4L2Compat, Debug) << arg->index << " : index of the buffer";
> > +
> > +     if (file->priority() < maxPriority())
> > +             return -EBUSY;
> > +
> > +     if (!hasOwnership(file) && owner_)
> > +             return -EBUSY;
>
> I don't think this is right, you can't EXPBUF without calling REQBUFS
> first, which has an impact on ownership.

But, there's no way to know that reqbuf is the owner and was called before this.

> No cargo cult copy&paste please.
LMAO, okay ;)

> > +
> > +     memset(arg->reserved, 0, sizeof(arg->reserved));
> > +
> > +     arg->fd = fcntl(buffers_[arg->index].m.fd,
> > +                     arg->flags & O_CLOEXEC ? F_DUPFD_CLOEXEC : F_DUPFD, 0);
> > +     arg->fd = fcntl(arg->fd, F_SETFL, 0);
>
> 0 ?
>
> > +
> > +     LOG(V4L2Compat, Debug) << "Exported buffer at index: " << arg->index;
> > +
> > +     acquire(file);
>
> Same comment regarding ownership.
>
> > +
> > +     return 0;
> > +}
> > +
> >  int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *file, int *arg)
> >  {
> >       LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << file->efd();
> > @@ -685,6 +720,7 @@ const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
> >       VIDIOC_QUERYBUF,
> >       VIDIOC_QBUF,
> >       VIDIOC_DQBUF,
> > +     VIDIOC_EXPBUF,
> >       VIDIOC_STREAMON,
> >       VIDIOC_STREAMOFF,
> >  };
> > @@ -755,6 +791,9 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar
> >       case VIDIOC_DQBUF:
> >               ret = vidioc_dqbuf(file, static_cast<struct v4l2_buffer *>(arg), &locker);
> >               break;
> > +     case VIDIOC_EXPBUF:
> > +             ret = vidioc_expbuf(file, static_cast<struct v4l2_exportbuffer *>(arg));
> > +             break;
> >       case VIDIOC_STREAMON:
> >               ret = vidioc_streamon(file, static_cast<int *>(arg));
> >               break;
> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > index fccec241879d..81ef7788e9fe 100644
> > --- a/src/v4l2/v4l2_camera_proxy.h
> > +++ b/src/v4l2/v4l2_camera_proxy.h
> > @@ -58,6 +58,7 @@ private:
> >       int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> >       int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> >                        libcamera::MutexLocker *locker);
> > +     int vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg);
> >       int vidioc_streamon(V4L2CameraFile *file, int *arg);
> >       int vidioc_streamoff(V4L2CameraFile *file, int *arg);
> >
>
> --
> Regards,
>
> Laurent Pinchart

Regards,
Vedant Paranjape