[libcamera-devel] [PATCH v1] v4l2: V4L2CameraProxy: Add support for PREPARE_BUF as one of the supported ioctl

[Vedant Paranjape]

Ah, I see.

On Wed, 12 Jan, 2022, 01:05 Laurent Pinchart, <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Vedant,
>
> On Wed, Jan 12, 2022 at 01:03:23AM +0530, Vedant Paranjape wrote:
> > On Wed, Jan 12, 2022 at 12:52 AM Laurent Pinchart wrote:
> > > On Tue, Jan 11, 2022 at 09:10:53PM +0200, Laurent Pinchart wrote:
> > > > On Tue, Jan 11, 2022 at 10:03:29PM +0530, Vedant Paranjape wrote:
> > > > > Add support for PREPARE_BUF as one of the ioctl. Since this is a
> compat
> > > > > layer, there doesn't seem to be an equivalent to the "transfer
> ownership
> > > > > of the buffer to kernel driver" in V4L2Camera class.
> > > > >
> > > > > To match the error checks done by kernel implementation, we'd have
> to
> > > > > check if dmabuf fd is valid and that the buffer size is large
> enough.
> > > > > Doing so will not add any particular value to the program as
> > > > > applications don't depend on these conditions being handled
> correctly.
> > > >
> > > > s/don't depend/most likely don't depend/ (there can always be corner
> > > > cases, developers sometimes get inventive)
> > > >
> > > > > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> > > > > ---
> > > > > Source builds and tests fine, also passes the v4l2-compliance
> > > > >
> > > > > <snip>
> > > > > vedant at pc ~/libcamera$ ./test/v4l2_compat/v4l2_compat_test.py
> ./build/src/v4l2/v4l2-compat.so
> > > > > Testing /dev/video0 with uvcvideo driver... success
> > > > > </snip>
> > > > >
> > > > > Still figuring out how to test this with v4l2-ctl, if you have
> hints
> > > > > please let me know :)
> > > >
> > > > v4l2-compliance tests VIDIOC_PREPARE buf, that should be good enough
> as
> > > > a test.
> > > >
> > > > > ---
> > > > > src/v4l2/v4l2_camera_proxy.cpp | 28 ++++++++++++++++++++++++++++
> > > > >  src/v4l2/v4l2_camera_proxy.h   |  1 +
> > > > >  2 files changed, 29 insertions(+)
> > > > >
> > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp
> b/src/v4l2/v4l2_camera_proxy.cpp
> > > > > index 4d529bc29a4d..8c28e8738a06 100644
> > > > > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > > > @@ -544,6 +544,33 @@ int
> V4L2CameraProxy::vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *a
> > > > >     return 0;
> > > > >  }
> > > > >
> > > > > +int V4L2CameraProxy::vidioc_prepare_buf(V4L2CameraFile *file,
> struct v4l2_buffer *arg)
> > > > > +{
> > > > > +   LOG(V4L2Compat, Debug) << "Servicing vidioc_prepare_buf, index
> = "
> > > > > +                          << arg->index << " fd = " <<
> file->efd();
> > > > > +
> > > > > +   if (!hasOwnership(file))
> > > > > +           return -EBUSY;
> > > > > +
> > > > > +   if (arg->index >= bufferCount_)
> > > > > +           return -EINVAL;
> > > > > +
> > > >
> > > >       struct v4l2_buffer &buffer = buffers_[arg->index];
> > > >
> > > > and use buffer below.
> > > >
> > > > With this change, and v4l2-compliance passing,
> > > >
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > >
> > > > > +   if (buffers_[arg->index].flags & V4L2_BUF_FLAG_REQUEST_FD ||
> > >
> > > Actually, this isn't correct, you need to check arg->flags here.
> >
> > I missed this, but I can't figure out the reason why this is incorrect ?
>
> Because V4L2_BUF_FLAG_REQUEST_FD is a flag that is passed to
> VIDIOC_PREPARE_BUF, not a flag we set internally.
>
> > > > > +       buffers_[arg->index].flags & V4L2_BUF_FLAG_PREPARED)
> > > > > +           return -EINVAL;
> > > > > +
> > > > > +   if (buffers_[arg->index].flags & V4L2_BUF_FLAG_QUEUED ||
> > > > > +       !validateBufferType(arg->type) ||
> > > > > +       !validateMemoryType(arg->memory))
> > > > > +           return -EINVAL;
> > >
> > > And let's group all the flags check:
> > >
> > >         if (arg->flags & V4L2_BUF_FLAG_REQUEST_FD)
> > >                 return -EINVAL;
> > >
> > >         if (!validateBufferType(arg->type) ||
> !validateMemoryType(arg->memory))
> > >                 return -EINVAL;
> > >
> > >         struct v4l2_buffer &buffer = buffers_[arg->index];
> > >
> > >         if (buffer.flags & V4L2_BUF_FLAG_PREPARED ||
> > >             buffer.flags & V4L2_BUF_FLAG_QUEUED ||
> > >                 return -EINVAL;
> > >
> > > > > +
> > > > > +   buffers_[arg->index].flags |= V4L2_BUF_FLAG_PREPARED;
> > > > > +
> > > > > +   arg->flags = buffers_[arg->index].flags;
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > > > +
> > > > >  int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct
> v4l2_buffer *arg)
> > > > >  {
> > > > >     LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = "
> > > > > @@ -709,6 +736,7 @@ const std::set<unsigned long>
> V4L2CameraProxy::supportedIoctls_ = {
> > > > >     VIDIOC_S_INPUT,
> > > > >     VIDIOC_REQBUFS,
> > > > >     VIDIOC_QUERYBUF,
> > > > > +   VIDIOC_PREPARE_BUF,
> > > > >     VIDIOC_QBUF,
> > > > >     VIDIOC_DQBUF,
> > > > >     VIDIOC_EXPBUF,
> > > > > diff --git a/src/v4l2/v4l2_camera_proxy.h
> b/src/v4l2/v4l2_camera_proxy.h
> > > > > index 14e027c3e7d1..6baba94262a9 100644
> > > > > --- a/src/v4l2/v4l2_camera_proxy.h
> > > > > +++ b/src/v4l2/v4l2_camera_proxy.h
> > > > > @@ -57,6 +57,7 @@ private:
> > > > >     int vidioc_s_input(V4L2CameraFile *file, int *arg);
> > > > >     int vidioc_reqbufs(V4L2CameraFile *file, struct
> v4l2_requestbuffers *arg);
> > > > >     int vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer
> *arg);
> > > > > +   int vidioc_prepare_buf(V4L2CameraFile *file, struct
> v4l2_buffer *arg);
> > > > >     int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> > > > >     int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> > > > >                      libcamera::Mutex *lock)
> LIBCAMERA_TSA_REQUIRES(*lock);
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220112/d172d4b8/attachment.htm>