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

[PATCH v2 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>

---
v1 -> v2: using vmap and vunmap instead of vm_map_ram and vm_unmap_ram

 .../media/common/videobuf2/videobuf2-dma-sg.c    | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index fa69158a65b1fd..dcb8de5ab3e84a 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -496,11 +496,26 @@ 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 = vmap(buf->pages, buf->num_pages, VM_MAP,
+				  PAGE_KERNEL);
+
 	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)
+		vunmap(buf->vaddr);
+
+	buf->vaddr = NULL;
+}
+
 static int vb2_dma_sg_dmabuf_ops_mmap(struct dma_buf *dbuf,
 	struct vm_area_struct *vma)
 {
@@ -515,6 +530,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 v2 2/2] usb: gadget: uvc: limit isoc_sg to super speed gadgets

[Michael Grzeschik]

When calling uvc_video_encode_isoc_sg the function is preparing the sg payload
by setting the sglist pointers of the videobuffer for the request. The usb
gadget driver then is parsing the sg list and uses each sg entry to send in one
urb to the host. Because of the unrelated buffer of the uvc header that buffer
has to be send separately in an extra sg entry.

When it comes to transfers with an limited payload (e.g. the maximum of 3kB for
high-speed) this extra payload handling is not justified. A simple memcpy of
the header and payload is usually faster and does not come with that extra
runtime overhead.

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>

---
v1 -> v2: - left the sg assignment in uvc_buffer_sg under the test for use_sg
          - rephrased the commit message

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

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index 0aa3d7e1f3cc32..0abb1763faf1b6 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -87,9 +87,8 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
 	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->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 dd1c6b2ca7c6f3..b6ea600b011185 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -459,6 +459,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;
 
@@ -490,9 +493,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 v2 1/2] media: videobuf2-dma-sg: fix vmap and vunmap callbacks

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]

Gentle Ping!

On Fri, Nov 25, 2022 at 04:34:49PM +0100, 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>
>
>---
>v1 -> v2: using vmap and vunmap instead of vm_map_ram and vm_unmap_ram
>
> .../media/common/videobuf2/videobuf2-dma-sg.c    | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
>diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>index fa69158a65b1fd..dcb8de5ab3e84a 100644
>--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>@@ -496,11 +496,26 @@ 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 = vmap(buf->pages, buf->num_pages, VM_MAP,
>+				  PAGE_KERNEL);
>+
> 	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)
>+		vunmap(buf->vaddr);
>+
>+	buf->vaddr = NULL;
>+}
>+
> static int vb2_dma_sg_dmabuf_ops_mmap(struct dma_buf *dbuf,
> 	struct vm_area_struct *vma)
> {
>@@ -515,6 +530,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
>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 3205 bytes --]

Gentle Ping!

On Fri, Nov 25, 2022 at 04:34:50PM +0100, Michael Grzeschik wrote:
>When calling uvc_video_encode_isoc_sg the function is preparing the sg payload
>by setting the sglist pointers of the videobuffer for the request. The usb
>gadget driver then is parsing the sg list and uses each sg entry to send in one
>urb to the host. Because of the unrelated buffer of the uvc header that buffer
>has to be send separately in an extra sg entry.
>
>When it comes to transfers with an limited payload (e.g. the maximum of 3kB for
>high-speed) this extra payload handling is not justified. A simple memcpy of
>the header and payload is usually faster and does not come with that extra
>runtime overhead.
>
>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>
>
>---
>v1 -> v2: - left the sg assignment in uvc_buffer_sg under the test for use_sg
>          - rephrased the commit message
>
> drivers/usb/gadget/function/uvc_queue.c | 3 +--
> drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
>index 0aa3d7e1f3cc32..0abb1763faf1b6 100644
>--- a/drivers/usb/gadget/function/uvc_queue.c
>+++ b/drivers/usb/gadget/function/uvc_queue.c
>@@ -87,9 +87,8 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
> 	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->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 dd1c6b2ca7c6f3..b6ea600b011185 100644
>--- a/drivers/usb/gadget/function/uvc_video.c
>+++ b/drivers/usb/gadget/function/uvc_video.c
>@@ -459,6 +459,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;
>
>@@ -490,9 +493,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
>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Hans Verkuil]

Hi Michael,

I'm going through some old patches in patchwork and found this one.
Is this patch specifically for the uvc gadget driver, or is it a
generic fix? If it is the latter (I suspect it is), then can you
post this as a v3 by itself and rebased to the latest kernel?

The fact that it was combined with the uvc gadget patch in the patch
series is the main reason for this delay (see also my upcoming reply
to patch 2/2).

Regards,

	Hans

On 24/01/2023 23:34, Michael Grzeschik wrote:
> Gentle Ping!
> 
> On Fri, Nov 25, 2022 at 04:34:49PM +0100, 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>
>>
>> ---
>> v1 -> v2: using vmap and vunmap instead of vm_map_ram and vm_unmap_ram
>>
>> .../media/common/videobuf2/videobuf2-dma-sg.c    | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>> index fa69158a65b1fd..dcb8de5ab3e84a 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>> @@ -496,11 +496,26 @@ 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 = vmap(buf->pages, buf->num_pages, VM_MAP,
>> +                  PAGE_KERNEL);
>> +
>>     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)
>> +        vunmap(buf->vaddr);
>> +
>> +    buf->vaddr = NULL;
>> +}
>> +
>> static int vb2_dma_sg_dmabuf_ops_mmap(struct dma_buf *dbuf,
>>     struct vm_area_struct *vma)
>> {
>> @@ -515,6 +530,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
>>
> 

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

[Hans Verkuil]

Laurent, Dan,

On 24/01/2023 23:35, Michael Grzeschik wrote:
> Gentle Ping!

Can one of you look at this series? I see that Dan was added as UVC Gadget maintainer
earlier this year, so perhaps Dan can look at this? And also other UVC Gadget patches
from Michael:

https://patchwork.linuxtv.org/project/linux-media/list/?submitter=545

Patchwork is messy: think several of the patches in that list are either superseded
or are already merged, but the status was never updated.

Regards,

	Hans

> 
> On Fri, Nov 25, 2022 at 04:34:50PM +0100, Michael Grzeschik wrote:
>> When calling uvc_video_encode_isoc_sg the function is preparing the sg payload
>> by setting the sglist pointers of the videobuffer for the request. The usb
>> gadget driver then is parsing the sg list and uses each sg entry to send in one
>> urb to the host. Because of the unrelated buffer of the uvc header that buffer
>> has to be send separately in an extra sg entry.
>>
>> When it comes to transfers with an limited payload (e.g. the maximum of 3kB for
>> high-speed) this extra payload handling is not justified. A simple memcpy of
>> the header and payload is usually faster and does not come with that extra
>> runtime overhead.
>>
>> 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>
>>
>> ---
>> v1 -> v2: - left the sg assignment in uvc_buffer_sg under the test for use_sg
>>          - rephrased the commit message
>>
>> drivers/usb/gadget/function/uvc_queue.c | 3 +--
>> drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
>> 2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
>> index 0aa3d7e1f3cc32..0abb1763faf1b6 100644
>> --- a/drivers/usb/gadget/function/uvc_queue.c
>> +++ b/drivers/usb/gadget/function/uvc_queue.c
>> @@ -87,9 +87,8 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
>>     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->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 dd1c6b2ca7c6f3..b6ea600b011185 100644
>> --- a/drivers/usb/gadget/function/uvc_video.c
>> +++ b/drivers/usb/gadget/function/uvc_video.c
>> @@ -459,6 +459,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;
>>
>> @@ -490,9 +493,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 v2 2/2] usb: gadget: uvc: limit isoc_sg to super speed gadgets

[Dan Scally]

Morning Hans

On 18/07/2023 09:46, Hans Verkuil wrote:
> Laurent, Dan,
>
> On 24/01/2023 23:35, Michael Grzeschik wrote:
>> Gentle Ping!
> Can one of you look at this series? I see that Dan was added as UVC Gadget maintainer
> earlier this year, so perhaps Dan can look at this? And also other UVC Gadget patches
> from Michael:
>
> https://patchwork.linuxtv.org/project/linux-media/list/?submitter=545
>
> Patchwork is messy: think several of the patches in that list are either superseded
> or are already merged, but the status was never updated.


Sorry; I'll put some time aside and look at the list this week.


Thanks

Dan

>
> Regards,
>
> 	Hans
>
>> On Fri, Nov 25, 2022 at 04:34:50PM +0100, Michael Grzeschik wrote:
>>> When calling uvc_video_encode_isoc_sg the function is preparing the sg payload
>>> by setting the sglist pointers of the videobuffer for the request. The usb
>>> gadget driver then is parsing the sg list and uses each sg entry to send in one
>>> urb to the host. Because of the unrelated buffer of the uvc header that buffer
>>> has to be send separately in an extra sg entry.
>>>
>>> When it comes to transfers with an limited payload (e.g. the maximum of 3kB for
>>> high-speed) this extra payload handling is not justified. A simple memcpy of
>>> the header and payload is usually faster and does not come with that extra
>>> runtime overhead.
>>>
>>> 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>
>>>
>>> ---
>>> v1 -> v2: - left the sg assignment in uvc_buffer_sg under the test for use_sg
>>>           - rephrased the commit message
>>>
>>> drivers/usb/gadget/function/uvc_queue.c | 3 +--
>>> drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
>>> 2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
>>> index 0aa3d7e1f3cc32..0abb1763faf1b6 100644
>>> --- a/drivers/usb/gadget/function/uvc_queue.c
>>> +++ b/drivers/usb/gadget/function/uvc_queue.c
>>> @@ -87,9 +87,8 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
>>>      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->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 dd1c6b2ca7c6f3..b6ea600b011185 100644
>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>> @@ -459,6 +459,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;
>>>
>>> @@ -490,9 +493,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 v2 2/2] usb: gadget: uvc: limit isoc_sg to super speed gadgets

[Dan Scally]

Hi Michael

On 25/11/2022 15:34, Michael Grzeschik wrote:
> When calling uvc_video_encode_isoc_sg the function is preparing the sg payload
> by setting the sglist pointers of the videobuffer for the request. The usb
> gadget driver then is parsing the sg list and uses each sg entry to send in one
> urb to the host. Because of the unrelated buffer of the uvc header that buffer
> has to be send separately in an extra sg entry.
>
> When it comes to transfers with an limited payload (e.g. the maximum of 3kB for
> high-speed) this extra payload handling is not justified. A simple memcpy of
> the header and payload is usually faster and does not come with that extra
> runtime overhead.


Sorry for the delay with this one. I don't suppose you benchmarked this at all? I'd be curious to 
see the effect.

> 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>
> ---
> v1 -> v2: - left the sg assignment in uvc_buffer_sg under the test for use_sg
>            - rephrased the commit message
>
>   drivers/usb/gadget/function/uvc_queue.c | 3 +--
>   drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
>   2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index 0aa3d7e1f3cc32..0abb1763faf1b6 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -87,9 +87,8 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
>   	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->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 dd1c6b2ca7c6f3..b6ea600b011185 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -459,6 +459,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;
>   
> @@ -490,9 +493,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;
> +	}


I think it'd be better to just set video->queue.use_sg n uvcg_queue_init() by checking 
cdev->gadget->speed as well as cdev->gadget->sg_supported; can we do that?

>   
>   	video->req_int_count = 0;
>   

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

[Dan Scally]

Hello Hans

On 18/07/2023 09:46, Hans Verkuil wrote:
> Laurent, Dan,
>
> On 24/01/2023 23:35, Michael Grzeschik wrote:
>> Gentle Ping!
> Can one of you look at this series? I see that Dan was added as UVC Gadget maintainer
> earlier this year, so perhaps Dan can look at this? And also other UVC Gadget patches
> from Michael:
>
> https://patchwork.linuxtv.org/project/linux-media/list/?submitter=545
>
> Patchwork is messy: think several of the patches in that list are either superseded
> or are already merged, but the status was never updated.


I think in the end there's only one other in that list that was not either merged / superseded / 
nacked / reviewed already, which I'll review ASAP. I don't think I can update patchwork for the 
others though...or at least if I can I have no idea how.


Dan

>
> Regards,
>
> 	Hans
>
>> On Fri, Nov 25, 2022 at 04:34:50PM +0100, Michael Grzeschik wrote:
>>> When calling uvc_video_encode_isoc_sg the function is preparing the sg payload
>>> by setting the sglist pointers of the videobuffer for the request. The usb
>>> gadget driver then is parsing the sg list and uses each sg entry to send in one
>>> urb to the host. Because of the unrelated buffer of the uvc header that buffer
>>> has to be send separately in an extra sg entry.
>>>
>>> When it comes to transfers with an limited payload (e.g. the maximum of 3kB for
>>> high-speed) this extra payload handling is not justified. A simple memcpy of
>>> the header and payload is usually faster and does not come with that extra
>>> runtime overhead.
>>>
>>> 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>
>>>
>>> ---
>>> v1 -> v2: - left the sg assignment in uvc_buffer_sg under the test for use_sg
>>>           - rephrased the commit message
>>>
>>> drivers/usb/gadget/function/uvc_queue.c | 3 +--
>>> drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
>>> 2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
>>> index 0aa3d7e1f3cc32..0abb1763faf1b6 100644
>>> --- a/drivers/usb/gadget/function/uvc_queue.c
>>> +++ b/drivers/usb/gadget/function/uvc_queue.c
>>> @@ -87,9 +87,8 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
>>>      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->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 dd1c6b2ca7c6f3..b6ea600b011185 100644
>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>> @@ -459,6 +459,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;
>>>
>>> @@ -490,9 +493,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 v2 2/2] usb: gadget: uvc: limit isoc_sg to super speed gadgets

[Laurent Pinchart]

Hi Michael,

On Mon, Jul 24, 2023 at 01:47:09PM +0100, Dan Scally wrote:
> On 25/11/2022 15:34, Michael Grzeschik wrote:
> > When calling uvc_video_encode_isoc_sg the function is preparing the sg payload
> > by setting the sglist pointers of the videobuffer for the request. The usb
> > gadget driver then is parsing the sg list and uses each sg entry to send in one
> > urb to the host. Because of the unrelated buffer of the uvc header that buffer
> > has to be send separately in an extra sg entry.
> >
> > When it comes to transfers with an limited payload (e.g. the maximum of 3kB for
> > high-speed) this extra payload handling is not justified. A simple memcpy of
> > the header and payload is usually faster and does not come with that extra
> > runtime overhead.
> 
> Sorry for the delay with this one. I don't suppose you benchmarked
> this at all? I'd be curious to see the effect.

That's a good question. Michael, do you have numbers ?

> > 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>
> > ---
> > v1 -> v2: - left the sg assignment in uvc_buffer_sg under the test for use_sg
> >            - rephrased the commit message
> >
> >   drivers/usb/gadget/function/uvc_queue.c | 3 +--
> >   drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
> >   2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> > index 0aa3d7e1f3cc32..0abb1763faf1b6 100644
> > --- a/drivers/usb/gadget/function/uvc_queue.c
> > +++ b/drivers/usb/gadget/function/uvc_queue.c
> > @@ -87,9 +87,8 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
> >   	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->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 dd1c6b2ca7c6f3..b6ea600b011185 100644
> > --- a/drivers/usb/gadget/function/uvc_video.c
> > +++ b/drivers/usb/gadget/function/uvc_video.c
> > @@ -459,6 +459,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;
> >   
> > @@ -490,9 +493,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;
> > +	}
> 
> I think it'd be better to just set video->queue.use_sg n uvcg_queue_init() by checking 
> cdev->gadget->speed as well as cdev->gadget->sg_supported; can we do that?

Agreed, otherwise use_sg will be a misnommer (it could be set to true
without actually using SG). Furthermore, we should not create a CPU
mapping of the buffer when using SG, as that's a waste of resources.

> >   
> >   	video->req_int_count = 0;
> >   

-- 
Regards,

Laurent Pinchart

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

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

Hi Hans,

On Tue, Jul 18, 2023 at 10:40:57AM +0200, Hans Verkuil wrote:
>I'm going through some old patches in patchwork and found this one.
>Is this patch specifically for the uvc gadget driver, or is it a
>generic fix? If it is the latter (I suspect it is), then can you
>post this as a v3 by itself and rebased to the latest kernel?

I will just send v3 then. It is totally working without other
dependencies. This is long overdue.

>The fact that it was combined with the uvc gadget patch in the patch
>series is the main reason for this delay (see also my upcoming reply
>to patch 2/2).

I will look into this aswell.

Thanks,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 4136 bytes --]

Hi,

On Mon, Jul 24, 2023 at 01:47:09PM +0100, Dan Scally wrote:
>On 25/11/2022 15:34, Michael Grzeschik wrote:
>>When calling uvc_video_encode_isoc_sg the function is preparing the sg payload
>>by setting the sglist pointers of the videobuffer for the request. The usb
>>gadget driver then is parsing the sg list and uses each sg entry to send in one
>>urb to the host. Because of the unrelated buffer of the uvc header that buffer
>>has to be send separately in an extra sg entry.
>>
>>When it comes to transfers with an limited payload (e.g. the maximum of 3kB for
>>high-speed) this extra payload handling is not justified. A simple memcpy of
>>the header and payload is usually faster and does not come with that extra
>>runtime overhead.
>
>
>Sorry for the delay with this one. I don't suppose you benchmarked
>this at all? I'd be curious to see the effect.

No, I don't think that this was benchmarked. However since the last
discussions about overall smaller request sizes to increase the
performance on the fifo side of the usb-controller this whole
topic needs to be rethought. I will look over this once we have
a solution for the worker/queue_req situation as discussed in this
thread:

https://lore.kernel.org/linux-usb/ZToOJhyOFeGCGUFj@pengutronix.de/T/#t


>>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>
>>---
>>v1 -> v2: - left the sg assignment in uvc_buffer_sg under the test for use_sg
>>           - rephrased the commit message
>>
>>  drivers/usb/gadget/function/uvc_queue.c | 3 +--
>>  drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>
>>diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
>>index 0aa3d7e1f3cc32..0abb1763faf1b6 100644
>>--- a/drivers/usb/gadget/function/uvc_queue.c
>>+++ b/drivers/usb/gadget/function/uvc_queue.c
>>@@ -87,9 +87,8 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
>>  	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->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 dd1c6b2ca7c6f3..b6ea600b011185 100644
>>--- a/drivers/usb/gadget/function/uvc_video.c
>>+++ b/drivers/usb/gadget/function/uvc_video.c
>>@@ -459,6 +459,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;
>>@@ -490,9 +493,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;
>>+	}
>
>
>I think it'd be better to just set video->queue.use_sg n 
>uvcg_queue_init() by checking cdev->gadget->speed as well as 
>cdev->gadget->sg_supported; can we do that?

Yes, this is the better place to check this.

In case this patch will be send again, I will work this in sure.

>>  	video->req_int_count = 0;

Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]