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

[Laurent Pinchart]

On Sun, Nov 28, 2021 at 02:11:28AM +0530, Vedant Paranjape wrote:
> On Sun, Nov 28, 2021 at 1:58 AM Laurent Pinchart wrote:
> > On Sun, Nov 28, 2021 at 01:52:55AM +0530, Vedant Paranjape wrote:
> > > On Sun, 28 Nov, 2021, 01:30 Laurent Pinchart wrote:
> > > > 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.
> >
> > Isn't there ?
> 
> I am a bit confused as to how this will work. Can you hint it out please.

You already check if arg->index >= bufferCount_ and fail if it is. This
will catch cases where REQBUFS hasn't been called, as bufferCount_ will
be equal to 0 in that case.

For EXPBUF to succeed, REQBUFS must have been called, so someone has
ownership already. There's no need to acquire it here, you can check if
you're the owner, and if not, return -EBUSY.

To match the behaviour of the kernel, the ownership check should come
first.

> > > > 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