| Message ID | 20220111163329.363418-1-vedantparanjape160201@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Vedant, Thank you for the patch. 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@gmail.com> > --- > Source builds and tests fine, also passes the v4l2-compliance > > <snip> > vedant@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@ideasonboard.com> > + if (buffers_[arg->index].flags & V4L2_BUF_FLAG_REQUEST_FD || > + 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; > + > + 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); > -- > 2.25.1 >
On Tue, Jan 11, 2022 at 09:10:53PM +0200, Laurent Pinchart wrote: > Hi Vedant, > > Thank you for the patch. > > 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@gmail.com> > > --- > > Source builds and tests fine, also passes the v4l2-compliance > > > > <snip> > > vedant@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@ideasonboard.com> > > > + if (buffers_[arg->index].flags & V4L2_BUF_FLAG_REQUEST_FD || Actually, this isn't correct, you need to check arg->flags here. > > + 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);
Hello Laurent, On Wed, Jan 12, 2022 at 12:52 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Jan 11, 2022 at 09:10:53PM +0200, Laurent Pinchart wrote: > > Hi Vedant, > > > > Thank you for the patch. > > > > 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@gmail.com> > > > --- > > > Source builds and tests fine, also passes the v4l2-compliance > > > > > > <snip> > > > vedant@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@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 ? > > > + 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 Regards, Vedant Paranjape
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@gmail.com> > > > > --- > > > > Source builds and tests fine, also passes the v4l2-compliance > > > > > > > > <snip> > > > > vedant@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@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);
Ah, I see. On Wed, 12 Jan, 2022, 01:05 Laurent Pinchart, < laurent.pinchart@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@gmail.com> > > > > > --- > > > > > Source builds and tests fine, also passes the v4l2-compliance > > > > > > > > > > <snip> > > > > > vedant@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@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 >
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; + + if (buffers_[arg->index].flags & V4L2_BUF_FLAG_REQUEST_FD || + 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; + + 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);
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. Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> --- Source builds and tests fine, also passes the v4l2-compliance <snip> vedant@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 :) --- src/v4l2/v4l2_camera_proxy.cpp | 28 ++++++++++++++++++++++++++++ src/v4l2/v4l2_camera_proxy.h | 1 + 2 files changed, 29 insertions(+)