| Message ID | 20220109073330.923675-1-vedantparanjape160201@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Vedant, Thank you for the patch. On Sun, Jan 09, 2022 at 01:03:30PM +0530, Vedant Paranjape wrote: > Add support for PREPARE_BUF as one of the ioctl. This needs adding the > prepare_buf function to the v4l2_camera class as this operation involves The class is called V4L2Camera, not v4l2_camera. > passing on the ownership of buffers to the driver. > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> > --- > I have not cargo copied the error checking or the function, I actually > went to kernel source and checked what parameters are being checked to > give out a error and checked those in my definition as well. > > https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L729 Good idea ! > Please review the v4l2_camera_proxy part, if the function looks good > enough hypothetically assuming vcam_->prepare_buf() has a correct > implementation. > > As for the part in v4l2_camera, prepare_buf. Seeing the pattern of code, > any operation that needs exchanging buffers with the drivers needs to > have a function to be implemented inside v4l2_camera just like qbuf is. > Let me know if my assumption is wrong, and this change is absolutely not > needed. > > I am unable to figure out what is the equivalent of transfering ownership > of the buffer in the libcamera compat world. Just like queueing a buffer > has an equivalent, camera_->queueRequest(request) (not comparing apples > to apples, but to some extent). That's probably because there's no equivalent :-) I think you can just drop the V4L2Camera::prepare_buf() function, and just mark the buffer with V4L2_BUF_FLAG_PREPARED in V4L2CameraProxy::vidioc_prepare_buf() as you're doing already. A V4L2 application will have no way to figure out if the buffer has actually been prepared or not. > --- > src/v4l2/v4l2_camera.cpp | 10 ++++++++++ > src/v4l2/v4l2_camera.h | 1 + > src/v4l2/v4l2_camera_proxy.cpp | 32 ++++++++++++++++++++++++++++++++ > src/v4l2/v4l2_camera_proxy.h | 1 + > 4 files changed, 44 insertions(+) > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > index e922b9e6aab2..2c155a63f836 100644 > --- a/src/v4l2/v4l2_camera.cpp > +++ b/src/v4l2/v4l2_camera.cpp > @@ -275,6 +275,16 @@ int V4L2Camera::qbuf(unsigned int index) > return 0; > } > > +int V4L2Camera::prepare_buf(unsigned int index, unsigned int memory) > +{ > + // just a stub for now > + // referring to > + // https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L729 > + // https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-core.c#L1341 > + > + return 0; > +} > + > void V4L2Camera::waitForBufferAvailable() > { > MutexLocker locker(bufferMutex_); > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h > index 03e741180e8f..b4e4b65ed701 100644 > --- a/src/v4l2/v4l2_camera.h > +++ b/src/v4l2/v4l2_camera.h > @@ -57,6 +57,7 @@ public: > int streamOff(); > > int qbuf(unsigned int index); > + int prepare_buf(unsigned int index, unsigned int memory); > > void waitForBufferAvailable(); > bool isBufferAvailable(); > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index 4d529bc29a4d..ae0b0691ca4e 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -544,6 +544,37 @@ 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 (arg->index >= bufferCount_) > + return -EINVAL; > + > + if (arg->flags & V4L2_BUF_FLAG_REQUEST_FD || > + arg->flags & V4L2_BUF_FLAG_PREPARED) > + return -EINVAL; > + > + if (!hasOwnership(file)) > + return -EBUSY; This check should go first, to mirror the order of vb2_ioctl_prepare_buf(). > + > + if (!validateBufferType(arg->type) || > + !validateMemoryType(arg->memory)) > + return -EINVAL; You need to check the buffer state here (buffers_[arg->index].flags, not arg->flags) to make sure it's currently dequeued, as it's invalid to prepare a buffer that has already been prepared (see the first checks in vb2_core_prepare_buf()). If we wanted to fully match the error checks done by the kernel, we'd have to check here that the buffer size is large enough, and that the dmabuf fd is valid. I don't think it's worth it (especially the dmabuf check), as those are error conditions that I can't imagine an application would rely on being handled correctly. A comment to explain this may be useful though. > + > + /* \todo when DMABUF memory type is supported, add support here as well */ > + int ret = vcam_->prepare_buf(arg->index, arg->memory); > + if (ret < 0) > + return ret; > + > + buffers_[arg->index].flags |= V4L2_BUF_FLAG_PREPARED; > + > + arg->flags = buffers_[arg->index].flags; > + > + return ret; > +} > + > int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = " > @@ -709,6 +740,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);
diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp index e922b9e6aab2..2c155a63f836 100644 --- a/src/v4l2/v4l2_camera.cpp +++ b/src/v4l2/v4l2_camera.cpp @@ -275,6 +275,16 @@ int V4L2Camera::qbuf(unsigned int index) return 0; } +int V4L2Camera::prepare_buf(unsigned int index, unsigned int memory) +{ + // just a stub for now + // referring to + // https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L729 + // https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-core.c#L1341 + + return 0; +} + void V4L2Camera::waitForBufferAvailable() { MutexLocker locker(bufferMutex_); diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h index 03e741180e8f..b4e4b65ed701 100644 --- a/src/v4l2/v4l2_camera.h +++ b/src/v4l2/v4l2_camera.h @@ -57,6 +57,7 @@ public: int streamOff(); int qbuf(unsigned int index); + int prepare_buf(unsigned int index, unsigned int memory); void waitForBufferAvailable(); bool isBufferAvailable(); diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index 4d529bc29a4d..ae0b0691ca4e 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -544,6 +544,37 @@ 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 (arg->index >= bufferCount_) + return -EINVAL; + + if (arg->flags & V4L2_BUF_FLAG_REQUEST_FD || + arg->flags & V4L2_BUF_FLAG_PREPARED) + return -EINVAL; + + if (!hasOwnership(file)) + return -EBUSY; + + if (!validateBufferType(arg->type) || + !validateMemoryType(arg->memory)) + return -EINVAL; + + /* \todo when DMABUF memory type is supported, add support here as well */ + int ret = vcam_->prepare_buf(arg->index, arg->memory); + if (ret < 0) + return ret; + + buffers_[arg->index].flags |= V4L2_BUF_FLAG_PREPARED; + + arg->flags = buffers_[arg->index].flags; + + return ret; +} + int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = " @@ -709,6 +740,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. This needs adding the prepare_buf function to the v4l2_camera class as this operation involves passing on the ownership of buffers to the driver. Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> --- I have not cargo copied the error checking or the function, I actually went to kernel source and checked what parameters are being checked to give out a error and checked those in my definition as well. https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L729 Please review the v4l2_camera_proxy part, if the function looks good enough hypothetically assuming vcam_->prepare_buf() has a correct implementation. As for the part in v4l2_camera, prepare_buf. Seeing the pattern of code, any operation that needs exchanging buffers with the drivers needs to have a function to be implemented inside v4l2_camera just like qbuf is. Let me know if my assumption is wrong, and this change is absolutely not needed. I am unable to figure out what is the equivalent of transfering ownership of the buffer in the libcamera compat world. Just like queueing a buffer has an equivalent, camera_->queueRequest(request) (not comparing apples to apples, but to some extent). --- src/v4l2/v4l2_camera.cpp | 10 ++++++++++ src/v4l2/v4l2_camera.h | 1 + src/v4l2/v4l2_camera_proxy.cpp | 32 ++++++++++++++++++++++++++++++++ src/v4l2/v4l2_camera_proxy.h | 1 + 4 files changed, 44 insertions(+)