[PATCH v2 0/2] Don't use coherent DMA buffers for ISO transfer

[PATCH v2 0/2] Don't use coherent DMA buffers for ISO transfer

[Matwey V. Kornilov]

From: "Matwey V. Kornilov" <matwey.kornilov@gmail.com>

DMA cocherency slows the transfer down on systems without hardware coherent
DMA. In order to demontrate this we introduce performance measurement
facilities in patch 1 and fix the performance issue in patch 2 in order to
obtain 5.5 times speedup.

Changes since v1:
 * trace_pwc_handler_exit() call moved to proper place
 * detailed description added for commit 1
 * additional output added to trace to track separate frames

Matwey V. Kornilov (2):
  media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler()
  media: usb: pwc: Don't use coherent DMA buffers for ISO transfer

 drivers/media/usb/pwc/pwc-if.c | 19 +++++++------
 include/trace/events/pwc.h     | 64 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 9 deletions(-)
 create mode 100644 include/trace/events/pwc.h

-- 
2.16.4

[PATCH v2 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler()

[Matwey V. Kornilov]

There were reports that PWC-based webcams don't work at some
embedded ARM platforms. [1] Isochronous transfer handler seems to
work too long leading to the issues in MUSB USB host subsystem.
Also note, that urb->giveback() handlers are still called with
disabled interrupts. In order to be able to measure performance of
PWC driver, traces are introduced in URB handler section.

[1] https://www.spinics.net/lists/linux-usb/msg165735.html

Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
 drivers/media/usb/pwc/pwc-if.c |  7 +++++
 include/trace/events/pwc.h     | 64 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)
 create mode 100644 include/trace/events/pwc.h

diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
index 54b036d39c5b..72d2897a4b9f 100644
--- a/drivers/media/usb/pwc/pwc-if.c
+++ b/drivers/media/usb/pwc/pwc-if.c
@@ -76,6 +76,9 @@
 #include "pwc-dec23.h"
 #include "pwc-dec1.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/pwc.h>
+
 /* Function prototypes and driver templates */
 
 /* hotplug device table support */
@@ -260,6 +263,8 @@ static void pwc_isoc_handler(struct urb *urb)
 	int i, fst, flen;
 	unsigned char *iso_buf = NULL;
 
+	trace_pwc_handler_enter(urb, pdev);
+
 	if (urb->status == -ENOENT || urb->status == -ECONNRESET ||
 	    urb->status == -ESHUTDOWN) {
 		PWC_DEBUG_OPEN("URB (%p) unlinked %ssynchronously.\n",
@@ -348,6 +353,8 @@ static void pwc_isoc_handler(struct urb *urb)
 	}
 
 handler_end:
+	trace_pwc_handler_exit(urb, pdev);
+
 	i = usb_submit_urb(urb, GFP_ATOMIC);
 	if (i != 0)
 		PWC_ERROR("Error (%d) re-submitting urb in pwc_isoc_handler.\n", i);
diff --git a/include/trace/events/pwc.h b/include/trace/events/pwc.h
new file mode 100644
index 000000000000..d5bcb1314d0e
--- /dev/null
+++ b/include/trace/events/pwc.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#if !defined(_TRACE_PWC_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_PWC_H
+
+#include <linux/usb.h>
+#include <linux/tracepoint.h>
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM pwc
+
+TRACE_EVENT(pwc_handler_enter,
+	TP_PROTO(struct urb *urb, struct pwc_device *pdev),
+	TP_ARGS(urb, pdev),
+	TP_STRUCT__entry(
+		__field(struct urb*, urb)
+		__field(int, urb__status)
+		__field(u32, urb__actual_length)
+		__field(char*, devpath)
+		__field(struct pwc_frame_buf*, fbuf)
+		__field(int, fbuf__filled)
+	),
+	TP_fast_assign(
+		__entry->urb = urb;
+		__entry->urb__status = urb->status;
+		__entry->urb__actual_length = urb->actual_length;
+		__entry->devpath = pdev->udev->devpath;
+		__entry->fbuf = pdev->fill_buf;
+		__entry->fbuf__filled = pdev->fill_buf->filled;
+	),
+	TP_printk("devpath=%s (fbuf=%p filled=%d) urb=%p (status=%d actual_length=%u)",
+		__entry->devpath,
+		__entry->fbuf,
+		__entry->fbuf__filled,
+		__entry->urb,
+		__entry->urb__status,
+		__entry->urb__actual_length)
+);
+
+TRACE_EVENT(pwc_handler_exit,
+	TP_PROTO(struct urb *urb, struct pwc_device* pdev),
+	TP_ARGS(urb, pdev),
+	TP_STRUCT__entry(
+		__field(struct urb*, urb)
+		__field(char*, devpath)
+		__field(struct pwc_frame_buf*, fbuf)
+		__field(int, fbuf__filled)
+	),
+	TP_fast_assign(
+		__entry->urb = urb;
+		__entry->devpath = pdev->udev->devpath;
+		__entry->fbuf = pdev->fill_buf;
+		__entry->fbuf__filled = pdev->fill_buf->filled;
+	),
+	TP_printk("devpath=%s (fbuf=%p filled=%d) urb=%p",
+		__entry->devpath,
+		__entry->fbuf,
+		__entry->fbuf__filled,
+		__entry->urb)
+);
+
+#endif /* _TRACE_PWC_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
-- 
2.16.4

Re: [PATCH v2 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler()

[Steven Rostedt]

On Fri, 22 Jun 2018 15:04:18 +0300
"Matwey V. Kornilov" <matwey@sai.msu.ru> wrote:

> There were reports that PWC-based webcams don't work at some
> embedded ARM platforms. [1] Isochronous transfer handler seems to
> work too long leading to the issues in MUSB USB host subsystem.
> Also note, that urb->giveback() handlers are still called with
> disabled interrupts. In order to be able to measure performance of
> PWC driver, traces are introduced in URB handler section.
> 
> [1] https://www.spinics.net/lists/linux-usb/msg165735.html
> 
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>

From a tracing point of view:

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> ---
>  drivers/media/usb/pwc/pwc-if.c |  7 +++++
>  include/trace/events/pwc.h     | 64 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)
>  create mode 100644 include/trace/events/pwc.h
> 
>

Re: [PATCH v2 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler()

[Steven Rostedt]

On Fri, 22 Jun 2018 15:04:18 +0300
"Matwey V. Kornilov" <matwey@sai.msu.ru> wrote:

> There were reports that PWC-based webcams don't work at some
> embedded ARM platforms. [1] Isochronous transfer handler seems to
> work too long leading to the issues in MUSB USB host subsystem.
> Also note, that urb->giveback() handlers are still called with
> disabled interrupts. In order to be able to measure performance of
> PWC driver, traces are introduced in URB handler section.
> 
> [1] https://www.spinics.net/lists/linux-usb/msg165735.html
> 
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>

>From a tracing point of view:

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> ---
>  drivers/media/usb/pwc/pwc-if.c |  7 +++++
>  include/trace/events/pwc.h     | 64 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)
>  create mode 100644 include/trace/events/pwc.h
> 
>

[PATCH v2 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer

[Matwey V. Kornilov]

DMA cocherency slows the transfer down on systems without hardware
coherent DMA.

Based on previous commit the following performance benchmarks have been
carried out. Average memcpy() data transfer rate (rate) and handler
completion time (time) have been measured when running video stream at
640x480 resolution at 10fps.

x86_64 based system (Intel Core i5-3470). This platform has hardware
coherent DMA support and proposed change doesn't make big difference here.

 * kmalloc:            rate = (4.4 +- 1.0) GBps
                       time = (2.4 +- 1.2) usec
 * usb_alloc_coherent: rate = (4.1 +- 0.9) GBps
                       time = (2.5 +- 1.0) usec

We see that the measurements agree well within error ranges in this case.
So no performance downgrade is introduced.

armv7l based system (TI AM335x BeagleBone Black). This platform has no
hardware coherent DMA support. DMA coherence is implemented via disabled
page caching that slows down memcpy() due to memory controller behaviour.

 * kmalloc:            rate =  (190 +-  30) MBps
                       time =   (50 +-  10) usec
 * usb_alloc_coherent: rate =   (33 +-   4) MBps
                       time = (3000 +- 400) usec

Note, that quantative difference leads (this commit leads to 5 times
acceleration) to qualitative behavior change in this case. As it was
stated before, the video stream can not be successfully received at AM335x
platforms with MUSB based USB host controller due to performance issues
[1].

[1] https://www.spinics.net/lists/linux-usb/msg165735.html

Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
 drivers/media/usb/pwc/pwc-if.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
index 72d2897a4b9f..339a285600d1 100644
--- a/drivers/media/usb/pwc/pwc-if.c
+++ b/drivers/media/usb/pwc/pwc-if.c
@@ -427,11 +427,8 @@ static int pwc_isoc_init(struct pwc_device *pdev)
 		urb->interval = 1; // devik
 		urb->dev = udev;
 		urb->pipe = usb_rcvisocpipe(udev, pdev->vendpoint);
-		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
-		urb->transfer_buffer = usb_alloc_coherent(udev,
-							  ISO_BUFFER_SIZE,
-							  GFP_KERNEL,
-							  &urb->transfer_dma);
+		urb->transfer_flags = URB_ISO_ASAP;
+		urb->transfer_buffer = kmalloc(ISO_BUFFER_SIZE, GFP_KERNEL);
 		if (urb->transfer_buffer == NULL) {
 			PWC_ERROR("Failed to allocate urb buffer %d\n", i);
 			pwc_isoc_cleanup(pdev);
@@ -491,10 +488,7 @@ static void pwc_iso_free(struct pwc_device *pdev)
 		if (pdev->urbs[i]) {
 			PWC_DEBUG_MEMORY("Freeing URB\n");
 			if (pdev->urbs[i]->transfer_buffer) {
-				usb_free_coherent(pdev->udev,
-					pdev->urbs[i]->transfer_buffer_length,
-					pdev->urbs[i]->transfer_buffer,
-					pdev->urbs[i]->transfer_dma);
+				kfree(pdev->urbs[i]->transfer_buffer);
 			}
 			usb_free_urb(pdev->urbs[i]);
 			pdev->urbs[i] = NULL;
-- 
2.16.4

Re: [PATCH v2 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer

[Matwey V. Kornilov]

2018-06-22 15:04 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
> DMA cocherency slows the transfer down on systems without hardware
> coherent DMA.
>

Ping.

> Based on previous commit the following performance benchmarks have been
> carried out. Average memcpy() data transfer rate (rate) and handler
> completion time (time) have been measured when running video stream at
> 640x480 resolution at 10fps.
>
> x86_64 based system (Intel Core i5-3470). This platform has hardware
> coherent DMA support and proposed change doesn't make big difference here.
>
>  * kmalloc:            rate = (4.4 +- 1.0) GBps
>                        time = (2.4 +- 1.2) usec
>  * usb_alloc_coherent: rate = (4.1 +- 0.9) GBps
>                        time = (2.5 +- 1.0) usec
>
> We see that the measurements agree well within error ranges in this case.
> So no performance downgrade is introduced.
>
> armv7l based system (TI AM335x BeagleBone Black). This platform has no
> hardware coherent DMA support. DMA coherence is implemented via disabled
> page caching that slows down memcpy() due to memory controller behaviour.
>
>  * kmalloc:            rate =  (190 +-  30) MBps
>                        time =   (50 +-  10) usec
>  * usb_alloc_coherent: rate =   (33 +-   4) MBps
>                        time = (3000 +- 400) usec
>
> Note, that quantative difference leads (this commit leads to 5 times
> acceleration) to qualitative behavior change in this case. As it was
> stated before, the video stream can not be successfully received at AM335x
> platforms with MUSB based USB host controller due to performance issues
> [1].
>
> [1] https://www.spinics.net/lists/linux-usb/msg165735.html
>
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> ---
>  drivers/media/usb/pwc/pwc-if.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> index 72d2897a4b9f..339a285600d1 100644
> --- a/drivers/media/usb/pwc/pwc-if.c
> +++ b/drivers/media/usb/pwc/pwc-if.c
> @@ -427,11 +427,8 @@ static int pwc_isoc_init(struct pwc_device *pdev)
>                 urb->interval = 1; // devik
>                 urb->dev = udev;
>                 urb->pipe = usb_rcvisocpipe(udev, pdev->vendpoint);
> -               urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
> -               urb->transfer_buffer = usb_alloc_coherent(udev,
> -                                                         ISO_BUFFER_SIZE,
> -                                                         GFP_KERNEL,
> -                                                         &urb->transfer_dma);
> +               urb->transfer_flags = URB_ISO_ASAP;
> +               urb->transfer_buffer = kmalloc(ISO_BUFFER_SIZE, GFP_KERNEL);
>                 if (urb->transfer_buffer == NULL) {
>                         PWC_ERROR("Failed to allocate urb buffer %d\n", i);
>                         pwc_isoc_cleanup(pdev);
> @@ -491,10 +488,7 @@ static void pwc_iso_free(struct pwc_device *pdev)
>                 if (pdev->urbs[i]) {
>                         PWC_DEBUG_MEMORY("Freeing URB\n");
>                         if (pdev->urbs[i]->transfer_buffer) {
> -                               usb_free_coherent(pdev->udev,
> -                                       pdev->urbs[i]->transfer_buffer_length,
> -                                       pdev->urbs[i]->transfer_buffer,
> -                                       pdev->urbs[i]->transfer_dma);
> +                               kfree(pdev->urbs[i]->transfer_buffer);
>                         }
>                         usb_free_urb(pdev->urbs[i]);
>                         pdev->urbs[i] = NULL;
> --
> 2.16.4
>



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382

Re: [PATCH v2 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer

[Laurent Pinchart]

Hi Matwey,

Thank you for the patch.

On Friday, 22 June 2018 15:04:19 EEST Matwey V. Kornilov wrote:
> DMA cocherency slows the transfer down on systems without hardware
> coherent DMA.
> 
> Based on previous commit the following performance benchmarks have been
> carried out. Average memcpy() data transfer rate (rate) and handler
> completion time (time) have been measured when running video stream at
> 640x480 resolution at 10fps.
> 
> x86_64 based system (Intel Core i5-3470). This platform has hardware
> coherent DMA support and proposed change doesn't make big difference here.
> 
>  * kmalloc:            rate = (4.4 +- 1.0) GBps
>                        time = (2.4 +- 1.2) usec
>  * usb_alloc_coherent: rate = (4.1 +- 0.9) GBps
>                        time = (2.5 +- 1.0) usec
> 
> We see that the measurements agree well within error ranges in this case.
> So no performance downgrade is introduced.
> 
> armv7l based system (TI AM335x BeagleBone Black). This platform has no
> hardware coherent DMA support. DMA coherence is implemented via disabled
> page caching that slows down memcpy() due to memory controller behaviour.
> 
>  * kmalloc:            rate =  (190 +-  30) MBps
>                        time =   (50 +-  10) usec
>  * usb_alloc_coherent: rate =   (33 +-   4) MBps
>                        time = (3000 +- 400) usec
> 
> Note, that quantative difference leads (this commit leads to 5 times
> acceleration) to qualitative behavior change in this case. As it was
> stated before, the video stream can not be successfully received at AM335x
> platforms with MUSB based USB host controller due to performance issues
> [1].
> 
> [1] https://www.spinics.net/lists/linux-usb/msg165735.html
> 
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> ---
>  drivers/media/usb/pwc/pwc-if.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> index 72d2897a4b9f..339a285600d1 100644
> --- a/drivers/media/usb/pwc/pwc-if.c
> +++ b/drivers/media/usb/pwc/pwc-if.c
> @@ -427,11 +427,8 @@ static int pwc_isoc_init(struct pwc_device *pdev)
>  		urb->interval = 1; // devik
>  		urb->dev = udev;
>  		urb->pipe = usb_rcvisocpipe(udev, pdev->vendpoint);
> -		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
> -		urb->transfer_buffer = usb_alloc_coherent(udev,
> -							  ISO_BUFFER_SIZE,
> -							  GFP_KERNEL,
> -							  &urb->transfer_dma);
> +		urb->transfer_flags = URB_ISO_ASAP;
> +		urb->transfer_buffer = kmalloc(ISO_BUFFER_SIZE, GFP_KERNEL);

ISO_BUFFER_SIZE is 970 bytes, well below a page size, so this should be fine. 
However, for other USB camera drivers, we might require larger buffers 
spanning multiple pages, and kmalloc() wouldn't be a very good choice there. I 
thus believe we should implement a helper function, possibly in the for of 
usb_alloc_noncoherent(), to allow picking the right allocation mechanism based 
on the buffer size (and possibly other parameters). Ideally the helper and the 
USB core should cooperate to avoid any overhead from DMA operations when DMA 
is coherent on the platform (on x86 and some ARM platforms).

That being said, I don't see a reason why this patch should be blocked until 
we get such a helper function, we can also implement it when needed for 
another USB webcam driver (likely uvcvideo given the recent discussions) and 
then use it in the pwc driver.

We have also determined that performances can be further improved by keeping 
mappings around and using the dma_sync_* operations at runtime. That's again 
not a reason to block this patch, as the performance improvement is already 
impressive, so

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

But I would still like to see the above two issues addressed. If you'd like to 
give them a go, with or without getting v2 of this series merged first, please 
do so, and I'll happily review patches.

>  		if (urb->transfer_buffer == NULL) {
>  			PWC_ERROR("Failed to allocate urb buffer %d\n", i);
>  			pwc_isoc_cleanup(pdev);
> @@ -491,10 +488,7 @@ static void pwc_iso_free(struct pwc_device *pdev)
>  		if (pdev->urbs[i]) {
>  			PWC_DEBUG_MEMORY("Freeing URB\n");
>  			if (pdev->urbs[i]->transfer_buffer) {
> -				usb_free_coherent(pdev->udev,
> -					pdev->urbs[i]->transfer_buffer_length,
> -					pdev->urbs[i]->transfer_buffer,
> -					pdev->urbs[i]->transfer_dma);
> +				kfree(pdev->urbs[i]->transfer_buffer);
>  			}
>  			usb_free_urb(pdev->urbs[i]);
>  			pdev->urbs[i] = NULL;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer

[Matwey V. Kornilov]

2018-08-09 1:46 GMT+03:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Matwey,
>
> Thank you for the patch.
>
> On Friday, 22 June 2018 15:04:19 EEST Matwey V. Kornilov wrote:
>> DMA cocherency slows the transfer down on systems without hardware
>> coherent DMA.
>>
>> Based on previous commit the following performance benchmarks have been
>> carried out. Average memcpy() data transfer rate (rate) and handler
>> completion time (time) have been measured when running video stream at
>> 640x480 resolution at 10fps.
>>
>> x86_64 based system (Intel Core i5-3470). This platform has hardware
>> coherent DMA support and proposed change doesn't make big difference here.
>>
>>  * kmalloc:            rate = (4.4 +- 1.0) GBps
>>                        time = (2.4 +- 1.2) usec
>>  * usb_alloc_coherent: rate = (4.1 +- 0.9) GBps
>>                        time = (2.5 +- 1.0) usec
>>
>> We see that the measurements agree well within error ranges in this case.
>> So no performance downgrade is introduced.
>>
>> armv7l based system (TI AM335x BeagleBone Black). This platform has no
>> hardware coherent DMA support. DMA coherence is implemented via disabled
>> page caching that slows down memcpy() due to memory controller behaviour.
>>
>>  * kmalloc:            rate =  (190 +-  30) MBps
>>                        time =   (50 +-  10) usec
>>  * usb_alloc_coherent: rate =   (33 +-   4) MBps
>>                        time = (3000 +- 400) usec
>>
>> Note, that quantative difference leads (this commit leads to 5 times
>> acceleration) to qualitative behavior change in this case. As it was
>> stated before, the video stream can not be successfully received at AM335x
>> platforms with MUSB based USB host controller due to performance issues
>> [1].
>>
>> [1] https://www.spinics.net/lists/linux-usb/msg165735.html
>>
>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>> ---
>>  drivers/media/usb/pwc/pwc-if.c | 12 +++---------
>>  1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
>> index 72d2897a4b9f..339a285600d1 100644
>> --- a/drivers/media/usb/pwc/pwc-if.c
>> +++ b/drivers/media/usb/pwc/pwc-if.c
>> @@ -427,11 +427,8 @@ static int pwc_isoc_init(struct pwc_device *pdev)
>>               urb->interval = 1; // devik
>>               urb->dev = udev;
>>               urb->pipe = usb_rcvisocpipe(udev, pdev->vendpoint);
>> -             urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
>> -             urb->transfer_buffer = usb_alloc_coherent(udev,
>> -                                                       ISO_BUFFER_SIZE,
>> -                                                       GFP_KERNEL,
>> -                                                       &urb->transfer_dma);
>> +             urb->transfer_flags = URB_ISO_ASAP;
>> +             urb->transfer_buffer = kmalloc(ISO_BUFFER_SIZE, GFP_KERNEL);
>
> ISO_BUFFER_SIZE is 970 bytes, well below a page size, so this should be fine.
> However, for other USB camera drivers, we might require larger buffers
> spanning multiple pages, and kmalloc() wouldn't be a very good choice there. I
> thus believe we should implement a helper function, possibly in the for of
> usb_alloc_noncoherent(), to allow picking the right allocation mechanism based
> on the buffer size (and possibly other parameters). Ideally the helper and the
> USB core should cooperate to avoid any overhead from DMA operations when DMA
> is coherent on the platform (on x86 and some ARM platforms).

pwc/pwc.h:

#define ISO_FRAMES_PER_DESC     10
#define ISO_MAX_FRAME_SIZE      960
#define ISO_BUFFER_SIZE         (ISO_FRAMES_PER_DESC * ISO_MAX_FRAME_SIZE)

The buffer size is 9600 bytes, where did 970 come from?

>
> That being said, I don't see a reason why this patch should be blocked until
> we get such a helper function, we can also implement it when needed for
> another USB webcam driver (likely uvcvideo given the recent discussions) and
> then use it in the pwc driver.
>
> We have also determined that performances can be further improved by keeping
> mappings around and using the dma_sync_* operations at runtime. That's again
> not a reason to block this patch, as the performance improvement is already
> impressive, so

I've incorporated this changes in v3, I'll send it shortly.

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> But I would still like to see the above two issues addressed. If you'd like to
> give them a go, with or without getting v2 of this series merged first, please
> do so, and I'll happily review patches.
>
>>               if (urb->transfer_buffer == NULL) {
>>                       PWC_ERROR("Failed to allocate urb buffer %d\n", i);
>>                       pwc_isoc_cleanup(pdev);
>> @@ -491,10 +488,7 @@ static void pwc_iso_free(struct pwc_device *pdev)
>>               if (pdev->urbs[i]) {
>>                       PWC_DEBUG_MEMORY("Freeing URB\n");
>>                       if (pdev->urbs[i]->transfer_buffer) {
>> -                             usb_free_coherent(pdev->udev,
>> -                                     pdev->urbs[i]->transfer_buffer_length,
>> -                                     pdev->urbs[i]->transfer_buffer,
>> -                                     pdev->urbs[i]->transfer_dma);
>> +                             kfree(pdev->urbs[i]->transfer_buffer);
>>                       }
>>                       usb_free_urb(pdev->urbs[i]);
>>                       pdev->urbs[i] = NULL;
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382

Re: [PATCH v2 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer

[Laurent Pinchart]

Hi Matwey,

On Thursday, 9 August 2018 12:18:13 EEST Matwey V. Kornilov wrote:
> 2018-08-09 1:46 GMT+03:00 Laurent Pinchart:
> > On Friday, 22 June 2018 15:04:19 EEST Matwey V. Kornilov wrote:
> >> DMA cocherency slows the transfer down on systems without hardware
> >> coherent DMA.
> >> 
> >> Based on previous commit the following performance benchmarks have been
> >> carried out. Average memcpy() data transfer rate (rate) and handler
> >> completion time (time) have been measured when running video stream at
> >> 640x480 resolution at 10fps.
> >> 
> >> x86_64 based system (Intel Core i5-3470). This platform has hardware
> >> coherent DMA support and proposed change doesn't make big difference
> >> here.
> >> 
> >>  * kmalloc:            rate = (4.4 +- 1.0) GBps
> >>                        time = (2.4 +- 1.2) usec
> >>  
> >>  * usb_alloc_coherent: rate = (4.1 +- 0.9) GBps
> >>                        time = (2.5 +- 1.0) usec
> >> 
> >> We see that the measurements agree well within error ranges in this case.
> >> So no performance downgrade is introduced.
> >> 
> >> armv7l based system (TI AM335x BeagleBone Black). This platform has no
> >> hardware coherent DMA support. DMA coherence is implemented via disabled
> >> page caching that slows down memcpy() due to memory controller behaviour.
> >> 
> >>  * kmalloc:            rate =  (190 +-  30) MBps
> >>                        time =   (50 +-  10) usec
> >>  
> >>  * usb_alloc_coherent: rate =   (33 +-   4) MBps
> >>                        time = (3000 +- 400) usec
> >> 
> >> Note, that quantative difference leads (this commit leads to 5 times
> >> acceleration) to qualitative behavior change in this case. As it was
> >> stated before, the video stream can not be successfully received at
> >> AM335x platforms with MUSB based USB host controller due to performance
> >> issues [1].
> >> 
> >> [1] https://www.spinics.net/lists/linux-usb/msg165735.html
> >> 
> >> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> >> ---
> >> 
> >>  drivers/media/usb/pwc/pwc-if.c | 12 +++---------
> >>  1 file changed, 3 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/drivers/media/usb/pwc/pwc-if.c
> >> b/drivers/media/usb/pwc/pwc-if.c index 72d2897a4b9f..339a285600d1 100644
> >> --- a/drivers/media/usb/pwc/pwc-if.c
> >> +++ b/drivers/media/usb/pwc/pwc-if.c
> >> @@ -427,11 +427,8 @@ static int pwc_isoc_init(struct pwc_device *pdev)
> >>               urb->interval = 1; // devik
> >>               urb->dev = udev;
> >>               urb->pipe = usb_rcvisocpipe(udev, pdev->vendpoint);
> >> -             urb->transfer_flags = URB_ISO_ASAP |
> >> URB_NO_TRANSFER_DMA_MAP;
> >> -             urb->transfer_buffer = usb_alloc_coherent(udev,
> >> -                                                       ISO_BUFFER_SIZE,
> >> -                                                       GFP_KERNEL,
> >> -                                                      
> >> &urb->transfer_dma);
> >> +             urb->transfer_flags = URB_ISO_ASAP;
> >> +             urb->transfer_buffer = kmalloc(ISO_BUFFER_SIZE,
> >> GFP_KERNEL);
> > 
> > ISO_BUFFER_SIZE is 970 bytes, well below a page size, so this should be
> > fine. However, for other USB camera drivers, we might require larger
> > buffers spanning multiple pages, and kmalloc() wouldn't be a very good
> > choice there. I thus believe we should implement a helper function,
> > possibly in the for of usb_alloc_noncoherent(), to allow picking the
> > right allocation mechanism based on the buffer size (and possibly other
> > parameters). Ideally the helper and the USB core should cooperate to
> > avoid any overhead from DMA operations when DMA is coherent on the
> > platform (on x86 and some ARM platforms).
> 
> pwc/pwc.h:
> 
> #define ISO_FRAMES_PER_DESC     10
> #define ISO_MAX_FRAME_SIZE      960
> #define ISO_BUFFER_SIZE         (ISO_FRAMES_PER_DESC * ISO_MAX_FRAME_SIZE)
> 
> The buffer size is 9600 bytes, where did 970 come from?

From mistaking * for + at 2:00am :-/

That's three pages, so to reduce pressure on memory allocation it would be 
best to use an allocator that can be backed by CMA. A helper function to 
abstract this has just become more important.

> > That being said, I don't see a reason why this patch should be blocked
> > until we get such a helper function, we can also implement it when needed
> > for another USB webcam driver (likely uvcvideo given the recent
> > discussions) and then use it in the pwc driver.
> > 
> > We have also determined that performances can be further improved by
> > keeping mappings around and using the dma_sync_* operations at runtime.
> > That's again not a reason to block this patch, as the performance
> > improvement is already impressive, so
> 
> I've incorporated this changes in v3, I'll send it shortly.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > But I would still like to see the above two issues addressed. If you'd
> > like to give them a go, with or without getting v2 of this series merged
> > first, please do so, and I'll happily review patches.
> > 
> >>               if (urb->transfer_buffer == NULL) {
> >>                       PWC_ERROR("Failed to allocate urb buffer %d\n", i);
> >>                       pwc_isoc_cleanup(pdev);
> >> @@ -491,10 +488,7 @@ static void pwc_iso_free(struct pwc_device *pdev)
> >>               if (pdev->urbs[i]) {
> >>                       PWC_DEBUG_MEMORY("Freeing URB\n");
> >>                       if (pdev->urbs[i]->transfer_buffer) {
> >> -                             usb_free_coherent(pdev->udev,
> >> -                                  pdev->urbs[i]->transfer_buffer_length,
> >> -                                  pdev->urbs[i]->transfer_buffer,
> >> -                                  pdev->urbs[i]->transfer_dma);
> >> +                             kfree(pdev->urbs[i]->transfer_buffer);
> >>                       }
> >>                       usb_free_urb(pdev->urbs[i]);
> >>                       pdev->urbs[i] = NULL;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer

[Laurent Pinchart]

Hi Matwey,

On Thursday, 9 August 2018 12:18:13 EEST Matwey V. Kornilov wrote:
> 2018-08-09 1:46 GMT+03:00 Laurent Pinchart:
> > On Friday, 22 June 2018 15:04:19 EEST Matwey V. Kornilov wrote:
> >> DMA cocherency slows the transfer down on systems without hardware
> >> coherent DMA.
> >> 
> >> Based on previous commit the following performance benchmarks have been
> >> carried out. Average memcpy() data transfer rate (rate) and handler
> >> completion time (time) have been measured when running video stream at
> >> 640x480 resolution at 10fps.
> >> 
> >> x86_64 based system (Intel Core i5-3470). This platform has hardware
> >> coherent DMA support and proposed change doesn't make big difference
> >> here.
> >> 
> >>  * kmalloc:            rate = (4.4 +- 1.0) GBps
> >>                        time = (2.4 +- 1.2) usec
> >>  
> >>  * usb_alloc_coherent: rate = (4.1 +- 0.9) GBps
> >>                        time = (2.5 +- 1.0) usec
> >> 
> >> We see that the measurements agree well within error ranges in this case.
> >> So no performance downgrade is introduced.
> >> 
> >> armv7l based system (TI AM335x BeagleBone Black). This platform has no
> >> hardware coherent DMA support. DMA coherence is implemented via disabled
> >> page caching that slows down memcpy() due to memory controller behaviour.
> >> 
> >>  * kmalloc:            rate =  (190 +-  30) MBps
> >>                        time =   (50 +-  10) usec
> >>  
> >>  * usb_alloc_coherent: rate =   (33 +-   4) MBps
> >>                        time = (3000 +- 400) usec
> >> 
> >> Note, that quantative difference leads (this commit leads to 5 times
> >> acceleration) to qualitative behavior change in this case. As it was
> >> stated before, the video stream can not be successfully received at
> >> AM335x platforms with MUSB based USB host controller due to performance
> >> issues [1].
> >> 
> >> [1] https://www.spinics.net/lists/linux-usb/msg165735.html
> >> 
> >> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> >> ---
> >> 
> >>  drivers/media/usb/pwc/pwc-if.c | 12 +++---------
> >>  1 file changed, 3 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/drivers/media/usb/pwc/pwc-if.c
> >> b/drivers/media/usb/pwc/pwc-if.c index 72d2897a4b9f..339a285600d1 100644
> >> --- a/drivers/media/usb/pwc/pwc-if.c
> >> +++ b/drivers/media/usb/pwc/pwc-if.c
> >> @@ -427,11 +427,8 @@ static int pwc_isoc_init(struct pwc_device *pdev)
> >>               urb->interval = 1; // devik
> >>               urb->dev = udev;
> >>               urb->pipe = usb_rcvisocpipe(udev, pdev->vendpoint);
> >> -             urb->transfer_flags = URB_ISO_ASAP |
> >> URB_NO_TRANSFER_DMA_MAP;
> >> -             urb->transfer_buffer = usb_alloc_coherent(udev,
> >> -                                                       ISO_BUFFER_SIZE,
> >> -                                                       GFP_KERNEL,
> >> -                                                      
> >> &urb->transfer_dma);
> >> +             urb->transfer_flags = URB_ISO_ASAP;
> >> +             urb->transfer_buffer = kmalloc(ISO_BUFFER_SIZE,
> >> GFP_KERNEL);
> > 
> > ISO_BUFFER_SIZE is 970 bytes, well below a page size, so this should be
> > fine. However, for other USB camera drivers, we might require larger
> > buffers spanning multiple pages, and kmalloc() wouldn't be a very good
> > choice there. I thus believe we should implement a helper function,
> > possibly in the for of usb_alloc_noncoherent(), to allow picking the
> > right allocation mechanism based on the buffer size (and possibly other
> > parameters). Ideally the helper and the USB core should cooperate to
> > avoid any overhead from DMA operations when DMA is coherent on the
> > platform (on x86 and some ARM platforms).
> 
> pwc/pwc.h:
> 
> #define ISO_FRAMES_PER_DESC     10
> #define ISO_MAX_FRAME_SIZE      960
> #define ISO_BUFFER_SIZE         (ISO_FRAMES_PER_DESC * ISO_MAX_FRAME_SIZE)
> 
> The buffer size is 9600 bytes, where did 970 come from?

>From mistaking * for + at 2:00am :-/

That's three pages, so to reduce pressure on memory allocation it would be 
best to use an allocator that can be backed by CMA. A helper function to 
abstract this has just become more important.

> > That being said, I don't see a reason why this patch should be blocked
> > until we get such a helper function, we can also implement it when needed
> > for another USB webcam driver (likely uvcvideo given the recent
> > discussions) and then use it in the pwc driver.
> > 
> > We have also determined that performances can be further improved by
> > keeping mappings around and using the dma_sync_* operations at runtime.
> > That's again not a reason to block this patch, as the performance
> > improvement is already impressive, so
> 
> I've incorporated this changes in v3, I'll send it shortly.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > But I would still like to see the above two issues addressed. If you'd
> > like to give them a go, with or without getting v2 of this series merged
> > first, please do so, and I'll happily review patches.
> > 
> >>               if (urb->transfer_buffer == NULL) {
> >>                       PWC_ERROR("Failed to allocate urb buffer %d\n", i);
> >>                       pwc_isoc_cleanup(pdev);
> >> @@ -491,10 +488,7 @@ static void pwc_iso_free(struct pwc_device *pdev)
> >>               if (pdev->urbs[i]) {
> >>                       PWC_DEBUG_MEMORY("Freeing URB\n");
> >>                       if (pdev->urbs[i]->transfer_buffer) {
> >> -                             usb_free_coherent(pdev->udev,
> >> -                                  pdev->urbs[i]->transfer_buffer_length,
> >> -                                  pdev->urbs[i]->transfer_buffer,
> >> -                                  pdev->urbs[i]->transfer_dma);
> >> +                             kfree(pdev->urbs[i]->transfer_buffer);
> >>                       }
> >>                       usb_free_urb(pdev->urbs[i]);
> >>                       pdev->urbs[i] = NULL;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer

[Tomasz Figa]

On Thu, Aug 9, 2018 at 6:44 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Matwey,
>
> On Thursday, 9 August 2018 12:18:13 EEST Matwey V. Kornilov wrote:
> > 2018-08-09 1:46 GMT+03:00 Laurent Pinchart:
> > > On Friday, 22 June 2018 15:04:19 EEST Matwey V. Kornilov wrote:
> > >> DMA cocherency slows the transfer down on systems without hardware
> > >> coherent DMA.
> > >>
> > >> Based on previous commit the following performance benchmarks have been
> > >> carried out. Average memcpy() data transfer rate (rate) and handler
> > >> completion time (time) have been measured when running video stream at
> > >> 640x480 resolution at 10fps.
> > >>
> > >> x86_64 based system (Intel Core i5-3470). This platform has hardware
> > >> coherent DMA support and proposed change doesn't make big difference
> > >> here.
> > >>
> > >>  * kmalloc:            rate = (4.4 +- 1.0) GBps
> > >>                        time = (2.4 +- 1.2) usec
> > >>
> > >>  * usb_alloc_coherent: rate = (4.1 +- 0.9) GBps
> > >>                        time = (2.5 +- 1.0) usec
> > >>
> > >> We see that the measurements agree well within error ranges in this case.
> > >> So no performance downgrade is introduced.
> > >>
> > >> armv7l based system (TI AM335x BeagleBone Black). This platform has no
> > >> hardware coherent DMA support. DMA coherence is implemented via disabled
> > >> page caching that slows down memcpy() due to memory controller behaviour.
> > >>
> > >>  * kmalloc:            rate =  (190 +-  30) MBps
> > >>                        time =   (50 +-  10) usec
> > >>
> > >>  * usb_alloc_coherent: rate =   (33 +-   4) MBps
> > >>                        time = (3000 +- 400) usec
> > >>
> > >> Note, that quantative difference leads (this commit leads to 5 times
> > >> acceleration) to qualitative behavior change in this case. As it was
> > >> stated before, the video stream can not be successfully received at
> > >> AM335x platforms with MUSB based USB host controller due to performance
> > >> issues [1].
> > >>
> > >> [1] https://www.spinics.net/lists/linux-usb/msg165735.html
> > >>
> > >> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> > >> ---
> > >>
> > >>  drivers/media/usb/pwc/pwc-if.c | 12 +++---------
> > >>  1 file changed, 3 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/drivers/media/usb/pwc/pwc-if.c
> > >> b/drivers/media/usb/pwc/pwc-if.c index 72d2897a4b9f..339a285600d1 100644
> > >> --- a/drivers/media/usb/pwc/pwc-if.c
> > >> +++ b/drivers/media/usb/pwc/pwc-if.c
> > >> @@ -427,11 +427,8 @@ static int pwc_isoc_init(struct pwc_device *pdev)
> > >>               urb->interval = 1; // devik
> > >>               urb->dev = udev;
> > >>               urb->pipe = usb_rcvisocpipe(udev, pdev->vendpoint);
> > >> -             urb->transfer_flags = URB_ISO_ASAP |
> > >> URB_NO_TRANSFER_DMA_MAP;
> > >> -             urb->transfer_buffer = usb_alloc_coherent(udev,
> > >> -                                                       ISO_BUFFER_SIZE,
> > >> -                                                       GFP_KERNEL,
> > >> -
> > >> &urb->transfer_dma);
> > >> +             urb->transfer_flags = URB_ISO_ASAP;
> > >> +             urb->transfer_buffer = kmalloc(ISO_BUFFER_SIZE,
> > >> GFP_KERNEL);
> > >
> > > ISO_BUFFER_SIZE is 970 bytes, well below a page size, so this should be
> > > fine. However, for other USB camera drivers, we might require larger
> > > buffers spanning multiple pages, and kmalloc() wouldn't be a very good
> > > choice there. I thus believe we should implement a helper function,
> > > possibly in the for of usb_alloc_noncoherent(), to allow picking the
> > > right allocation mechanism based on the buffer size (and possibly other
> > > parameters). Ideally the helper and the USB core should cooperate to
> > > avoid any overhead from DMA operations when DMA is coherent on the
> > > platform (on x86 and some ARM platforms).
> >
> > pwc/pwc.h:
> >
> > #define ISO_FRAMES_PER_DESC     10
> > #define ISO_MAX_FRAME_SIZE      960
> > #define ISO_BUFFER_SIZE         (ISO_FRAMES_PER_DESC * ISO_MAX_FRAME_SIZE)
> >
> > The buffer size is 9600 bytes, where did 970 come from?
>
> From mistaking * for + at 2:00am :-/
>
> That's three pages, so to reduce pressure on memory allocation it would be
> best to use an allocator that can be backed by CMA. A helper function to
> abstract this has just become more important.

Yes, that's a very valid point (which I raised earlier wrt possible
problems with replacing current usb_alloc_coherent() with kmalloc().
Not only it wouldn't be able to deal efficiently with contiguous
allocations, it would actually enforce contiguous allocations, even
for devices, which don't really need it, e.g. equipped with IOMMU.

Sounds like dma_alloc_noncoherent() (or dma_alloc_streaming())? In
theory, there is already DMA_ATTR_NON_CONSISTENT flag for
dma_alloc_attrs(), but almost no archs implement it (for sure ARM
doesn't and neither do the generic dma-mapping helpers).

Best regards,
Tomasz