[libcamera-devel,v3] v4l2: V4L2CameraProxy: Add support for PREPARE_BUF as one of the supported ioctl
diff mbox series

Message ID 20220119055512.31893-1-vedantparanjape160201@gmail.com
State Accepted
Commit b5225f00cde0f84437349955050c27d89a76e550
Headers show
Series
  • [libcamera-devel,v3] v4l2: V4L2CameraProxy: Add support for PREPARE_BUF as one of the supported ioctl
Related show

Commit Message

Vedant Paranjape Jan. 19, 2022, 5:55 a.m. UTC 
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 most likely don't depend on these conditions being
handled correctly.

Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/v4l2/v4l2_camera_proxy.cpp | 35 +++++++++++++++++++++++++++++++++-
 src/v4l2/v4l2_camera_proxy.h   |  1 +
 2 files changed, 35 insertions(+), 1 deletion(-)

Comments

Vedant Paranjape Jan. 21, 2022, 10:27 a.m. UTC | #1 
Hello Paul,
Thanks for the review.

On Fri, Jan 21, 2022 at 3:44 PM <paul.elder@ideasonboard.com> wrote:
>
> Hi Vedant,
>
> On Wed, Jan 19, 2022 at 11:25:12AM +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.
>
> + Thus, simply duplicate the checks done by vidioc_qbuf.
>
> , perhaps? You describe a (minor) issue but no solution.
>
> >
> > 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 most likely don't depend on these conditions being
> > handled correctly.
>
> Would we add these once we support importing buffers, just for
> correctness?
Sure I will do that
>
> >
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 35 +++++++++++++++++++++++++++++++++-
> >  src/v4l2/v4l2_camera_proxy.h   |  1 +
> >  2 files changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index ec6daac605fb..f3470a6d312a 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -559,6 +559,38 @@ 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)
> > +             << "[" << file->description() << "] " << __func__
> > +             << "(index=" << arg->index << ")";
> > +
> > +     if (!hasOwnership(file))
> > +             return -EBUSY;
> > +
> > +     if (arg->index >= bufferCount_)
> > +             return -EINVAL;
> > +
> > +     if (arg->flags & V4L2_BUF_FLAG_REQUEST_FD)
> > +             return -EINVAL;
> > +
> > +     if (!validateBufferType(arg->type) ||
> > +         !validateMemoryType(arg->memory))
> > +             return -EINVAL;
>
> I would've said that these checks should be in the same order as in
> qbuf, but I guess you've copied this order from the kernel. In which
> case qbuf should be fixed (in a separate patch; this one's fine).
>
> > +
> > +     struct v4l2_buffer &buffer = buffers_[arg->index];
> > +
> > +     if (buffer.flags & V4L2_BUF_FLAG_QUEUED ||
> > +         buffer.flags & V4L2_BUF_FLAG_PREPARED)
> > +             return -EINVAL;
>
> Huh, I didn't realize that preparing a buf that's already been prepared
> would be an error. I thought you could just nop it. v4l2 docs haven't
> kept up I suppose :/
>
> I would say that we could check the prepared flag in qbuf too, but these
> checks are few and constant time so it's probably find as-is.
I can address this in the qbuf patch you asked above.
>
>
> Looks good.
>
> With or without the suggested changes,
>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>
> > +
> > +     buffer.flags |= V4L2_BUF_FLAG_PREPARED;
> > +
> > +     arg->flags = buffer.flags;
> > +
> > +     return 0;
> > +}
> > +
> >  int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
> >  {
> >       LOG(V4L2Compat, Debug)
> > @@ -627,7 +659,7 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> >
> >       struct v4l2_buffer &buf = buffers_[currentBuf_];
> >
> > -     buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);
> > +     buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_PREPARED);
> >       buf.length = sizeimage_;
> >       *arg = buf;
> >
> > @@ -729,6 +761,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 a38b28c744d3..76ca2d8a5558 100644
> > --- a/src/v4l2/v4l2_camera_proxy.h
> > +++ b/src/v4l2/v4l2_camera_proxy.h
> > @@ -58,6 +58,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
> >

Regards,
Vedant Paranjape
Laurent Pinchart Jan. 21, 2022, 2:28 p.m. UTC | #2 
On Fri, Jan 21, 2022 at 07:13:56PM +0900, paul.elder@ideasonboard.com wrote:
> On Wed, Jan 19, 2022 at 11:25:12AM +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.
> 
> + Thus, simply duplicate the checks done by vidioc_qbuf.
> 
> , perhaps? You describe a (minor) issue but no solution.
> 
> > 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 most likely don't depend on these conditions being
> > handled correctly.
> 
> Would we add these once we support importing buffers, just for
> correctness?
> 
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 35 +++++++++++++++++++++++++++++++++-
> >  src/v4l2/v4l2_camera_proxy.h   |  1 +
> >  2 files changed, 35 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index ec6daac605fb..f3470a6d312a 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -559,6 +559,38 @@ 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)
> > +		<< "[" << file->description() << "] " << __func__
> > +		<< "(index=" << arg->index << ")";
> > +
> > +	if (!hasOwnership(file))
> > +		return -EBUSY;
> > +
> > +	if (arg->index >= bufferCount_)
> > +		return -EINVAL;
> > +
> > +	if (arg->flags & V4L2_BUF_FLAG_REQUEST_FD)
> > +		return -EINVAL;
> > +
> > +	if (!validateBufferType(arg->type) ||
> > +	    !validateMemoryType(arg->memory))
> > +		return -EINVAL;
> 
> I would've said that these checks should be in the same order as in
> qbuf, but I guess you've copied this order from the kernel. In which
> case qbuf should be fixed (in a separate patch; this one's fine).
> 
> > +
> > +	struct v4l2_buffer &buffer = buffers_[arg->index];
> > +
> > +	if (buffer.flags & V4L2_BUF_FLAG_QUEUED ||
> > +	    buffer.flags & V4L2_BUF_FLAG_PREPARED)
> > +		return -EINVAL;
> 
> Huh, I didn't realize that preparing a buf that's already been prepared
> would be an error. I thought you could just nop it. v4l2 docs haven't
> kept up I suppose :/
> 
> I would say that we could check the prepared flag in qbuf too, but these
> checks are few and constant time so it's probably find as-is.

You can call QBUF on a buffer that is prepared, so that shouldn't be a
problem. What I now realize is missing, however, is setting the
V4L2_BUF_FLAG_PREPARED flag in V4L2CameraProxy::vidioc_qbuf(). It can be
done in a separate patch, when fixing the order of checks in qbuf.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Looks good.
> 
> With or without the suggested changes,
> 
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> > +
> > +	buffer.flags |= V4L2_BUF_FLAG_PREPARED;
> > +
> > +	arg->flags = buffer.flags;
> > +
> > +	return 0;
> > +}
> > +
> >  int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
> >  {
> >  	LOG(V4L2Compat, Debug)
> > @@ -627,7 +659,7 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> >  
> >  	struct v4l2_buffer &buf = buffers_[currentBuf_];
> >  
> > -	buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);
> > +	buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_PREPARED);
> >  	buf.length = sizeimage_;
> >  	*arg = buf;
> >  
> > @@ -729,6 +761,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 a38b28c744d3..76ca2d8a5558 100644
> > --- a/src/v4l2/v4l2_camera_proxy.h
> > +++ b/src/v4l2/v4l2_camera_proxy.h
> > @@ -58,6 +58,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);

Patch
diff mbox series 

diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
index ec6daac605fb..f3470a6d312a 100644
--- a/src/v4l2/v4l2_camera_proxy.cpp
+++ b/src/v4l2/v4l2_camera_proxy.cpp
@@ -559,6 +559,38 @@  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)
+		<< "[" << file->description() << "] " << __func__
+		<< "(index=" << arg->index << ")";
+
+	if (!hasOwnership(file))
+		return -EBUSY;
+
+	if (arg->index >= bufferCount_)
+		return -EINVAL;
+
+	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_QUEUED ||
+	    buffer.flags & V4L2_BUF_FLAG_PREPARED)
+		return -EINVAL;
+
+	buffer.flags |= V4L2_BUF_FLAG_PREPARED;
+
+	arg->flags = buffer.flags;
+
+	return 0;
+}
+
 int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
 {
 	LOG(V4L2Compat, Debug)
@@ -627,7 +659,7 @@  int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
 
 	struct v4l2_buffer &buf = buffers_[currentBuf_];
 
-	buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);
+	buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_PREPARED);
 	buf.length = sizeimage_;
 	*arg = buf;
 
@@ -729,6 +761,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 a38b28c744d3..76ca2d8a5558 100644
--- a/src/v4l2/v4l2_camera_proxy.h
+++ b/src/v4l2/v4l2_camera_proxy.h
@@ -58,6 +58,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);