[PATCH 1/2] media: videobuf2-dma-sg: fix vmap and vunmap callbacks

[PATCH 1/2] media: videobuf2-dma-sg: fix vmap and vunmap callbacks

[Michael Grzeschik]

For dmabuf import users to be able to use the vaddr from another
videobuf2-dma-sg source, the exporter needs to set a proper vaddr on
vb2_dma_sg_dmabuf_ops_vmap callback.

This patch adds vm_map_ram on map if buf->vaddr was not set, and also
adds the vb2_dma_sg_dmabuf_ops_vunmap function to remove the mapping
afterwards.

Cc: stable <stable@kernel.org>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index fa69158a65b1fd..8d6e154bbbc8b0 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -496,11 +496,25 @@ static int vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf,
 {
 	struct vb2_dma_sg_buf *buf = dbuf->priv;
 
+	if (!buf->vaddr)
+		buf->vaddr = vm_map_ram(buf->pages, buf->num_pages, -1);
+
 	iosys_map_set_vaddr(map, buf->vaddr);
 
 	return 0;
 }
 
+static void vb2_dma_sg_dmabuf_ops_vunmap(struct dma_buf *dbuf,
+				      struct iosys_map *map)
+{
+	struct vb2_dma_sg_buf *buf = dbuf->priv;
+
+	if (buf->vaddr)
+		vm_unmap_ram(buf->vaddr, buf->num_pages);
+
+	buf->vaddr = NULL;
+}
+
 static int vb2_dma_sg_dmabuf_ops_mmap(struct dma_buf *dbuf,
 	struct vm_area_struct *vma)
 {
@@ -515,6 +529,7 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = {
 	.begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access,
 	.end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access,
 	.vmap = vb2_dma_sg_dmabuf_ops_vmap,
+	.vunmap = vb2_dma_sg_dmabuf_ops_vunmap,
 	.mmap = vb2_dma_sg_dmabuf_ops_mmap,
 	.release = vb2_dma_sg_dmabuf_ops_release,
 };
-- 
2.30.2

[PATCH RESEND 2/2] usb: gadget: uvc: limit isoc_sg to super speed gadgets

[Michael Grzeschik]

The overhead of preparing sg data is high for transfers with limited
payload. When transferring isoc over high-speed usb the maximum payload
is rather small which is a good argument no to use sg. This patch is
changing the uvc_video_encode_isoc_sg encode function only to be used
for super speed gadgets.

Cc: stable <stable@kernel.org>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
Since the last time the patch was send, I saw some issues regarding
the use of the vaddr. I thought there was a fix needed in this code.

But the problem was to be found in the videobuf2-dma-sg vmap/vunmap
implementation. So this patch stays unchanged and is save to be applied,
if the corresponding videobuf2-dma-sg patch is applied before it.

 drivers/usb/gadget/function/uvc_queue.c | 9 +++------
 drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index ec500ee499eed1..31c50ba1774f0d 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -84,12 +84,9 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
 		return -ENODEV;
 
 	buf->state = UVC_BUF_STATE_QUEUED;
-	if (queue->use_sg) {
-		buf->sgt = vb2_dma_sg_plane_desc(vb, 0);
-		buf->sg = buf->sgt->sgl;
-	} else {
-		buf->mem = vb2_plane_vaddr(vb, 0);
-	}
+	buf->sgt = vb2_dma_sg_plane_desc(vb, 0);
+	buf->sg = buf->sgt->sgl;
+	buf->mem = vb2_plane_vaddr(vb, 0);
 	buf->length = vb2_plane_size(vb, 0);
 	if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		buf->bytesused = 0;
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index bb037fcc90e69e..5081eb3bc5484c 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -448,6 +448,9 @@ static void uvcg_video_pump(struct work_struct *work)
  */
 int uvcg_video_enable(struct uvc_video *video, int enable)
 {
+	struct uvc_device *uvc = video->uvc;
+	struct usb_composite_dev *cdev = uvc->func.config->cdev;
+	struct usb_gadget *gadget = cdev->gadget;
 	unsigned int i;
 	int ret;
 
@@ -479,9 +482,11 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
 	if (video->max_payload_size) {
 		video->encode = uvc_video_encode_bulk;
 		video->payload_size = 0;
-	} else
-		video->encode = video->queue.use_sg ?
+	} else {
+		video->encode = (video->queue.use_sg &&
+				 !(gadget->speed <= USB_SPEED_HIGH)) ?
 			uvc_video_encode_isoc_sg : uvc_video_encode_isoc;
+	}
 
 	video->req_int_count = 0;
 
-- 
2.30.2

Re: [PATCH 1/2] media: videobuf2-dma-sg: fix vmap and vunmap callbacks

[Hans Verkuil]

Marek,

Can you review this? It looks good to me, but I wonder if videobuf2-dma-config.c
has a similar problem. That looks to be mapping as well, but there is no vunmap.

Michael, I have a comment below:

On 26/10/2022 20:42, Michael Grzeschik wrote:
> For dmabuf import users to be able to use the vaddr from another
> videobuf2-dma-sg source, the exporter needs to set a proper vaddr on
> vb2_dma_sg_dmabuf_ops_vmap callback.
> 
> This patch adds vm_map_ram on map if buf->vaddr was not set, and also
> adds the vb2_dma_sg_dmabuf_ops_vunmap function to remove the mapping
> afterwards.
> 
> Cc: stable <stable@kernel.org>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> index fa69158a65b1fd..8d6e154bbbc8b0 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -496,11 +496,25 @@ static int vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf,
>  {
>  	struct vb2_dma_sg_buf *buf = dbuf->priv;
>  
> +	if (!buf->vaddr)
> +		buf->vaddr = vm_map_ram(buf->pages, buf->num_pages, -1);

The comments before the vm_map_ram function state that it should be used for
up to 256 KB only, and video buffers are definitely much larger. It recommends
using vmap in that case. Any reason for not switching to vmap()?

Regards,

	Hans

> +
>  	iosys_map_set_vaddr(map, buf->vaddr);
>  
>  	return 0;
>  }
>  
> +static void vb2_dma_sg_dmabuf_ops_vunmap(struct dma_buf *dbuf,
> +				      struct iosys_map *map)
> +{
> +	struct vb2_dma_sg_buf *buf = dbuf->priv;
> +
> +	if (buf->vaddr)
> +		vm_unmap_ram(buf->vaddr, buf->num_pages);
> +
> +	buf->vaddr = NULL;
> +}
> +
>  static int vb2_dma_sg_dmabuf_ops_mmap(struct dma_buf *dbuf,
>  	struct vm_area_struct *vma)
>  {
> @@ -515,6 +529,7 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = {
>  	.begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access,
>  	.end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access,
>  	.vmap = vb2_dma_sg_dmabuf_ops_vmap,
> +	.vunmap = vb2_dma_sg_dmabuf_ops_vunmap,
>  	.mmap = vb2_dma_sg_dmabuf_ops_mmap,
>  	.release = vb2_dma_sg_dmabuf_ops_release,
>  };

Re: [PATCH 1/2] media: videobuf2-dma-sg: fix vmap and vunmap callbacks

[Laurent Pinchart]

On Fri, Nov 04, 2022 at 04:55:25PM +0100, Hans Verkuil wrote:
> Marek,
> 
> Can you review this? It looks good to me, but I wonder if videobuf2-dma-config.c
> has a similar problem. That looks to be mapping as well, but there is no vunmap.
> 
> Michael, I have a comment below:
> 
> On 26/10/2022 20:42, Michael Grzeschik wrote:
> > For dmabuf import users to be able to use the vaddr from another
> > videobuf2-dma-sg source, the exporter needs to set a proper vaddr on
> > vb2_dma_sg_dmabuf_ops_vmap callback.
> > 
> > This patch adds vm_map_ram on map if buf->vaddr was not set, and also
> > adds the vb2_dma_sg_dmabuf_ops_vunmap function to remove the mapping
> > afterwards.
> > 
> > Cc: stable <stable@kernel.org>
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > index fa69158a65b1fd..8d6e154bbbc8b0 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > @@ -496,11 +496,25 @@ static int vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf,
> >  {
> >  	struct vb2_dma_sg_buf *buf = dbuf->priv;
> >  
> > +	if (!buf->vaddr)
> > +		buf->vaddr = vm_map_ram(buf->pages, buf->num_pages, -1);
> 
> The comments before the vm_map_ram function state that it should be used for
> up to 256 KB only, and video buffers are definitely much larger. It recommends
> using vmap in that case. Any reason for not switching to vmap()?

vb2_dma_sg_vaddr() already uses vm_map_ram(), so that would need to be
fixed too. I assume the code is copied from there.

> > +
> >  	iosys_map_set_vaddr(map, buf->vaddr);
> >  
> >  	return 0;
> >  }
> >  
> > +static void vb2_dma_sg_dmabuf_ops_vunmap(struct dma_buf *dbuf,
> > +				      struct iosys_map *map)
> > +{
> > +	struct vb2_dma_sg_buf *buf = dbuf->priv;
> > +
> > +	if (buf->vaddr)
> > +		vm_unmap_ram(buf->vaddr, buf->num_pages);
> > +
> > +	buf->vaddr = NULL;
> > +}
> > +
> >  static int vb2_dma_sg_dmabuf_ops_mmap(struct dma_buf *dbuf,
> >  	struct vm_area_struct *vma)
> >  {
> > @@ -515,6 +529,7 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = {
> >  	.begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access,
> >  	.end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access,
> >  	.vmap = vb2_dma_sg_dmabuf_ops_vmap,
> > +	.vunmap = vb2_dma_sg_dmabuf_ops_vunmap,
> >  	.mmap = vb2_dma_sg_dmabuf_ops_mmap,
> >  	.release = vb2_dma_sg_dmabuf_ops_release,
> >  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH RESEND 2/2] usb: gadget: uvc: limit isoc_sg to super speed gadgets

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Wed, Oct 26, 2022 at 08:42:12PM +0200, Michael Grzeschik wrote:
> The overhead of preparing sg data is high for transfers with limited
> payload. When transferring isoc over high-speed usb the maximum payload

What exactly is causing a high overhead, and how big is it ?

> is rather small which is a good argument no to use sg. This patch is
> changing the uvc_video_encode_isoc_sg encode function only to be used
> for super speed gadgets.
> 
> Cc: stable <stable@kernel.org>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> ---
> Since the last time the patch was send, I saw some issues regarding
> the use of the vaddr. I thought there was a fix needed in this code.
> 
> But the problem was to be found in the videobuf2-dma-sg vmap/vunmap
> implementation. So this patch stays unchanged and is save to be applied,
> if the corresponding videobuf2-dma-sg patch is applied before it.
> 
>  drivers/usb/gadget/function/uvc_queue.c | 9 +++------
>  drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index ec500ee499eed1..31c50ba1774f0d 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -84,12 +84,9 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
>  		return -ENODEV;
>  
>  	buf->state = UVC_BUF_STATE_QUEUED;
> -	if (queue->use_sg) {
> -		buf->sgt = vb2_dma_sg_plane_desc(vb, 0);
> -		buf->sg = buf->sgt->sgl;
> -	} else {
> -		buf->mem = vb2_plane_vaddr(vb, 0);
> -	}
> +	buf->sgt = vb2_dma_sg_plane_desc(vb, 0);
> +	buf->sg = buf->sgt->sgl;

vb2_dma_sg_plane_desc() is defined as

static inline struct sg_table *vb2_dma_sg_plane_desc(
		struct vb2_buffer *vb, unsigned int plane_no)
{
	return (struct sg_table *)vb2_plane_cookie(vb, plane_no);
}

and vb2_plane_cookie() as

void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int plane_no)
{
	if (plane_no >= vb->num_planes || !vb->planes[plane_no].mem_priv)
		return NULL;

	return call_ptr_memop(cookie, vb, vb->planes[plane_no].mem_priv);
}

If the queue isn't using scatter-gather (use_sg == false), the cookie
operation will not be defined, and buf->sgt will be NULL. Dereferencing
it will thus segfault.

> +	buf->mem = vb2_plane_vaddr(vb, 0);
>  	buf->length = vb2_plane_size(vb, 0);
>  	if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>  		buf->bytesused = 0;
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index bb037fcc90e69e..5081eb3bc5484c 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -448,6 +448,9 @@ static void uvcg_video_pump(struct work_struct *work)
>   */
>  int uvcg_video_enable(struct uvc_video *video, int enable)
>  {
> +	struct uvc_device *uvc = video->uvc;
> +	struct usb_composite_dev *cdev = uvc->func.config->cdev;
> +	struct usb_gadget *gadget = cdev->gadget;
>  	unsigned int i;
>  	int ret;
>  
> @@ -479,9 +482,11 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
>  	if (video->max_payload_size) {
>  		video->encode = uvc_video_encode_bulk;
>  		video->payload_size = 0;
> -	} else
> -		video->encode = video->queue.use_sg ?
> +	} else {
> +		video->encode = (video->queue.use_sg &&
> +				 !(gadget->speed <= USB_SPEED_HIGH)) ?
>  			uvc_video_encode_isoc_sg : uvc_video_encode_isoc;
> +	}
>  
>  	video->req_int_count = 0;
>  

-- 
Regards,

Laurent Pinchart