[libcamera-devel,v1] v4l2: V4L2CameraProxy: Add support for DMABUF buffer I/O in REQBUF ioctl
diff mbox series

Message ID 20211129194031.1340933-1-vedantparanjape160201@gmail.com
State Superseded
Headers show
Series
  • [libcamera-devel,v1] v4l2: V4L2CameraProxy: Add support for DMABUF buffer I/O in REQBUF ioctl
Related show

Commit Message

Vedant Paranjape Nov. 29, 2021, 7:40 p.m. UTC 
To support importing DMABUF, we need to reqbufs with DMABUF mode.

This patch enables using V4L2_MEMORY_DMABUF as one of the memory types
in vidioc_reqbuf to initialise DMA Buffer I/O

Bug: https://bugs.libcamera.org/show_bug.cgi?id=89

Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
---
 src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart Nov. 29, 2021, 11:56 p.m. UTC | #1 
Hi Vedant,

On Tue, Nov 30, 2021 at 01:10:31AM +0530, Vedant Paranjape wrote:
> To support importing DMABUF, we need to reqbufs with DMABUF mode.
> 
> This patch enables using V4L2_MEMORY_DMABUF as one of the memory types
> in vidioc_reqbuf to initialise DMA Buffer I/O
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=89
> 
> Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index f194e06345b7..c8861a531399 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);

https://git.libcamera.org/libcamera/libcamera.git/tree/Documentation/coding-style.rst#n421

>  }
>  
>  void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
> @@ -468,7 +469,8 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
>  	if (!hasOwnership(file) && owner_)
>  		return -EBUSY;
>  
> -	arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
> +	arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP |
> +						   V4L2_BUF_CAP_SUPPORTS_DMABUF;
>  	memset(arg->reserved, 0, sizeof(arg->reserved));
>  
>  	if (arg->count == 0) {
> @@ -511,11 +513,15 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
>  		struct v4l2_buffer buf = {};
>  		buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  		buf.length = v4l2PixFormat_.sizeimage;
> -		buf.memory = V4L2_MEMORY_MMAP;
> -		buf.m.offset = i * v4l2PixFormat_.sizeimage;
> +		buf.memory = arg->memory;
>  		buf.index = i;
>  		buf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  
> +		if (arg->memory == V4L2_MEMORY_MMAP)
> +			buf.m.offset = i * v4l2PixFormat_.sizeimage;
> +		else if (arg->memory == V4L2_MEMORY_DMABUF)
> +			buf.m.fd = vcam_->getBufferFd(i).fd();

This isn't right. Please test this patch with a V4L2 application that
uses the device in DMABUF mode.

> +
>  		buffers_[i] = buf;
>  	}
>
Paul Elder Nov. 30, 2021, 2:06 a.m. UTC | #2 
Hi Vedant,

On Tue, Nov 30, 2021 at 01:10:31AM +0530, Vedant Paranjape wrote:
> To support importing DMABUF, we need to reqbufs with DMABUF mode.
> 
> This patch enables using V4L2_MEMORY_DMABUF as one of the memory types
> in vidioc_reqbuf to initialise DMA Buffer I/O
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=89
> 
> Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index f194e06345b7..c8861a531399 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 is going to cause problems with expbuf if your memory type is
dmabuf.


Paul

>  }
>  
>  void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
> @@ -468,7 +469,8 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
>  	if (!hasOwnership(file) && owner_)
>  		return -EBUSY;
>  
> -	arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
> +	arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP |
> +						   V4L2_BUF_CAP_SUPPORTS_DMABUF;
>  	memset(arg->reserved, 0, sizeof(arg->reserved));
>  
>  	if (arg->count == 0) {
> @@ -511,11 +513,15 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
>  		struct v4l2_buffer buf = {};
>  		buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  		buf.length = v4l2PixFormat_.sizeimage;
> -		buf.memory = V4L2_MEMORY_MMAP;
> -		buf.m.offset = i * v4l2PixFormat_.sizeimage;
> +		buf.memory = arg->memory;
>  		buf.index = i;
>  		buf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  
> +		if (arg->memory == V4L2_MEMORY_MMAP)
> +			buf.m.offset = i * v4l2PixFormat_.sizeimage;
> +		else if (arg->memory == V4L2_MEMORY_DMABUF)
> +			buf.m.fd = vcam_->getBufferFd(i).fd();
> +
>  		buffers_[i] = buf;
>  	}
>  
> -- 
> 2.25.1
>
Vedant Paranjape Nov. 30, 2021, 6:50 a.m. UTC | #3 
Hi Laurent,

On Tue, Nov 30, 2021 at 5:27 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Vedant,
>
> On Tue, Nov 30, 2021 at 01:10:31AM +0530, Vedant Paranjape wrote:
> > To support importing DMABUF, we need to reqbufs with DMABUF mode.
> >
> > This patch enables using V4L2_MEMORY_DMABUF as one of the memory types
> > in vidioc_reqbuf to initialise DMA Buffer I/O
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=89
> >
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index f194e06345b7..c8861a531399 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);
>
> https://git.libcamera.org/libcamera/libcamera.git/tree/Documentation/coding-style.rst#n421

There's a bug in checkstyle, that's why I had to ignore it on this
one. Checkstyle was changing some irrelevant part of the code too.
So, I manually tried to fix this, but checkstyle still won't budge. I
have setup the script long back :)

>
> >  }
> >
> >  void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
> > @@ -468,7 +469,8 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
> >       if (!hasOwnership(file) && owner_)
> >               return -EBUSY;
> >
> > -     arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
> > +     arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP |
> > +                                                V4L2_BUF_CAP_SUPPORTS_DMABUF;
> >       memset(arg->reserved, 0, sizeof(arg->reserved));
> >
> >       if (arg->count == 0) {
> > @@ -511,11 +513,15 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
> >               struct v4l2_buffer buf = {};
> >               buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >               buf.length = v4l2PixFormat_.sizeimage;
> > -             buf.memory = V4L2_MEMORY_MMAP;
> > -             buf.m.offset = i * v4l2PixFormat_.sizeimage;
> > +             buf.memory = arg->memory;
> >               buf.index = i;
> >               buf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> >
> > +             if (arg->memory == V4L2_MEMORY_MMAP)
> > +                     buf.m.offset = i * v4l2PixFormat_.sizeimage;
> > +             else if (arg->memory == V4L2_MEMORY_DMABUF)
> > +                     buf.m.fd = vcam_->getBufferFd(i).fd();
>
> This isn't right. Please test this patch with a V4L2 application that
> uses the device in DMABUF mode.

Okay, I'll do so.

>
> > +
> >               buffers_[i] = buf;
> >       }
> >
>
> --
> Regards,
>
> Laurent Pinchart

Regards,
Vedant Paranjape
Vedant Paranjape Nov. 30, 2021, 6:53 a.m. UTC | #4 
Hi Paul,

On Tue, Nov 30, 2021 at 7:36 AM <paul.elder@ideasonboard.com> wrote:
>
> Hi Vedant,
>
> On Tue, Nov 30, 2021 at 01:10:31AM +0530, Vedant Paranjape wrote:
> > To support importing DMABUF, we need to reqbufs with DMABUF mode.
> >
> > This patch enables using V4L2_MEMORY_DMABUF as one of the memory types
> > in vidioc_reqbuf to initialise DMA Buffer I/O
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=89
> >
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index f194e06345b7..c8861a531399 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 is going to cause problems with expbuf if your memory type is
> dmabuf.

I am not sure what needs to be done for importing dmabuf then, according to this
https://www.kernel.org/doc/html/v5.3/media/uapi/v4l/dmabuf.html#example-queueing-dmabuf-using-single-plane-api

What do you suggest ?

>
>
> Paul
>
> >  }
> >
> >  void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
> > @@ -468,7 +469,8 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
> >       if (!hasOwnership(file) && owner_)
> >               return -EBUSY;
> >
> > -     arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
> > +     arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP |
> > +                                                V4L2_BUF_CAP_SUPPORTS_DMABUF;
> >       memset(arg->reserved, 0, sizeof(arg->reserved));
> >
> >       if (arg->count == 0) {
> > @@ -511,11 +513,15 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
> >               struct v4l2_buffer buf = {};
> >               buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >               buf.length = v4l2PixFormat_.sizeimage;
> > -             buf.memory = V4L2_MEMORY_MMAP;
> > -             buf.m.offset = i * v4l2PixFormat_.sizeimage;
> > +             buf.memory = arg->memory;
> >               buf.index = i;
> >               buf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> >
> > +             if (arg->memory == V4L2_MEMORY_MMAP)
> > +                     buf.m.offset = i * v4l2PixFormat_.sizeimage;
> > +             else if (arg->memory == V4L2_MEMORY_DMABUF)
> > +                     buf.m.fd = vcam_->getBufferFd(i).fd();
> > +
> >               buffers_[i] = buf;
> >       }
> >
> > --
> > 2.25.1
> >

Regards,
Vedant Paranjape
Paul Elder Nov. 30, 2021, 7:04 a.m. UTC | #5 
On Tue, Nov 30, 2021 at 12:23:19PM +0530, Vedant Paranjape wrote:
> Hi Paul,
> 
> On Tue, Nov 30, 2021 at 7:36 AM <paul.elder@ideasonboard.com> wrote:
> >
> > Hi Vedant,
> >
> > On Tue, Nov 30, 2021 at 01:10:31AM +0530, Vedant Paranjape wrote:
> > > To support importing DMABUF, we need to reqbufs with DMABUF mode.
> > >
> > > This patch enables using V4L2_MEMORY_DMABUF as one of the memory types
> > > in vidioc_reqbuf to initialise DMA Buffer I/O
> > >
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=89
> > >
> > > Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
> > > ---
> > >  src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++----
> > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > > index f194e06345b7..c8861a531399 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 is going to cause problems with expbuf if your memory type is
> > dmabuf.
> 
> I am not sure what needs to be done for importing dmabuf then, according to this
> https://www.kernel.org/doc/html/v5.3/media/uapi/v4l/dmabuf.html#example-queueing-dmabuf-using-single-plane-api

Well the issue with importing isn't here, it's the hunk below, which
Laurent has reviewed (and we're discussing now on irc).

The issue here is that the memory type validator needs to be expanded.
expbuf for example should error out if the device is configured for
dmabuf. You'll have to check the docs for the other ioctls.

> 
> What do you suggest ?

You can come up with a design/patch, and we can discuss it.


Paul

> 
> >
> > >  }
> > >
> > >  void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
> > > @@ -468,7 +469,8 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
> > >       if (!hasOwnership(file) && owner_)
> > >               return -EBUSY;
> > >
> > > -     arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
> > > +     arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP |
> > > +                                                V4L2_BUF_CAP_SUPPORTS_DMABUF;
> > >       memset(arg->reserved, 0, sizeof(arg->reserved));
> > >
> > >       if (arg->count == 0) {
> > > @@ -511,11 +513,15 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
> > >               struct v4l2_buffer buf = {};
> > >               buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > >               buf.length = v4l2PixFormat_.sizeimage;
> > > -             buf.memory = V4L2_MEMORY_MMAP;
> > > -             buf.m.offset = i * v4l2PixFormat_.sizeimage;
> > > +             buf.memory = arg->memory;
> > >               buf.index = i;
> > >               buf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > >
> > > +             if (arg->memory == V4L2_MEMORY_MMAP)
> > > +                     buf.m.offset = i * v4l2PixFormat_.sizeimage;
> > > +             else if (arg->memory == V4L2_MEMORY_DMABUF)
> > > +                     buf.m.fd = vcam_->getBufferFd(i).fd();
> > > +
> > >               buffers_[i] = buf;
> > >       }
> > >
> > > --
> > > 2.25.1
> > >

Patch
diff mbox series 

diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
index f194e06345b7..c8861a531399 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);
 }
 
 void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
@@ -468,7 +469,8 @@  int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
 	if (!hasOwnership(file) && owner_)
 		return -EBUSY;
 
-	arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP;
+	arg->capabilities = V4L2_BUF_CAP_SUPPORTS_MMAP |
+						   V4L2_BUF_CAP_SUPPORTS_DMABUF;
 	memset(arg->reserved, 0, sizeof(arg->reserved));
 
 	if (arg->count == 0) {
@@ -511,11 +513,15 @@  int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
 		struct v4l2_buffer buf = {};
 		buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 		buf.length = v4l2PixFormat_.sizeimage;
-		buf.memory = V4L2_MEMORY_MMAP;
-		buf.m.offset = i * v4l2PixFormat_.sizeimage;
+		buf.memory = arg->memory;
 		buf.index = i;
 		buf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
+		if (arg->memory == V4L2_MEMORY_MMAP)
+			buf.m.offset = i * v4l2PixFormat_.sizeimage;
+		else if (arg->memory == V4L2_MEMORY_DMABUF)
+			buf.m.fd = vcam_->getBufferFd(i).fd();
+
 		buffers_[i] = buf;
 	}