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

[PATCH v3 0/2] media: usb: pwc: 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 4 times speedup.

Changes since v2:
 * use dma_sync_single_for_cpu() to achive better performance
 * remeasured performance

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 | 54 +++++++++++++++++++++++++++--------
 include/trace/events/pwc.h     | 64 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+), 12 deletions(-)
 create mode 100644 include/trace/events/pwc.h

-- 
2.16.4

[PATCH v3 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..71ba98770537
--- /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(const char*, name)
+		__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->name = pdev->v4l2_dev.name;
+		__entry->fbuf = pdev->fill_buf;
+		__entry->fbuf__filled = (pdev->fill_buf ? pdev->fill_buf->filled : 0);
+	),
+	TP_printk("dev=%s (fbuf=%p filled=%d) urb=%p (status=%d actual_length=%u)",
+		__entry->name,
+		__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(const char*, name)
+		__field(struct pwc_frame_buf*, fbuf)
+		__field(int, fbuf__filled)
+	),
+	TP_fast_assign(
+		__entry->urb = urb;
+		__entry->name = pdev->v4l2_dev.name;
+		__entry->fbuf = pdev->fill_buf;
+		__entry->fbuf__filled = pdev->fill_buf->filled;
+	),
+	TP_printk(" dev=%s (fbuf=%p filled=%d) urb=%p",
+		__entry->name,
+		__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 v3 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler()

[Steven Rostedt]

On Thu,  9 Aug 2018 12:33:06 +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>
> ---
>  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..71ba98770537
> --- /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(const char*, name)

name needs to be a __string. Never save pointers that you will
dereference in the print_fmt, as you never know if those pointers will
exist later. Not to mention, userspace tools like trace-cmd and perf
have no idea how to display them.

You want:

		__string( name, pdev->v4l2_dev.name ),


> +		__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->name = pdev->v4l2_dev.name;

And here you assign it with:

		__assign_str(name, pdev->v4l2_dev.name);

> +		__entry->fbuf = pdev->fill_buf;
> +		__entry->fbuf__filled = (pdev->fill_buf ? pdev->fill_buf->filled : 0);
> +	),
> +	TP_printk("dev=%s (fbuf=%p filled=%d) urb=%p (status=%d actual_length=%u)",
> +		__entry->name,

And display it with:

		__get_str(name),

> +		__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(const char*, name)
> +		__field(struct pwc_frame_buf*, fbuf)
> +		__field(int, fbuf__filled)
> +	),
> +	TP_fast_assign(
> +		__entry->urb = urb;
> +		__entry->name = pdev->v4l2_dev.name;
> +		__entry->fbuf = pdev->fill_buf;
> +		__entry->fbuf__filled = pdev->fill_buf->filled;
> +	),
> +	TP_printk(" dev=%s (fbuf=%p filled=%d) urb=%p",
> +		__entry->name,

Same thing here.

-- Steve

> +		__entry->fbuf,
> +		__entry->fbuf__filled,
> +		__entry->urb)
> +);
> +
> +#endif /* _TRACE_PWC_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>

[PATCH v3 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.
Instead we use noncocherent DMA memory and explicit sync at data receive
handler.

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 = (2.0 +- 0.4) GBps
                       time = (5.0 +- 3.0) usec
 * usb_alloc_coherent: rate = (3.4 +- 1.2) GBps
                       time = (3.5 +- 3.0) usec

We see that the measurements agree within error ranges in this case.
So theoretically predicted performance downgrade cannot be reliably measured here.

armv7l based system (TI AM335x BeagleBone Black @ 300MHz). 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 =  (114 +- 5) MBps
                       time =   (84 +- 4) usec
 * usb_alloc_coherent: rate = (28.1 +- 0.1) MBps
                       time =  (341 +- 2) usec

Note, that quantative difference leads (this commit leads to 4 times
acceleration) to qualitative behavior change in this case. As it was
stated before, the video stream cannot 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 | 47 +++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
index 72d2897a4b9f..17f2015a75bb 100644
--- a/drivers/media/usb/pwc/pwc-if.c
+++ b/drivers/media/usb/pwc/pwc-if.c
@@ -159,6 +159,31 @@ static const struct video_device pwc_template = {
 /***************************************************************************/
 /* Private functions */
 
+static void* pwc_alloc_urb_buffer(struct device *dev, size_t size, dma_addr_t *dma_handle) {
+	void* buffer = kmalloc(size, GFP_KERNEL);
+
+	if (buffer == NULL) {
+		goto fail_kmalloc;
+	}
+
+	*dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
+	if (dma_mapping_error(dev, *dma_handle)) {
+		goto fail_dma_map_single;
+	}
+
+	return buffer;
+
+fail_dma_map_single:
+	kfree(buffer);
+fail_kmalloc:
+	return NULL;
+}
+
+static void pwc_free_urb_buffer(struct device *dev, size_t size, void* buffer, dma_addr_t dma_handle) {
+	dma_unmap_single(dev, dma_handle, size, DMA_FROM_DEVICE);
+	kfree(buffer);
+}
+
 static struct pwc_frame_buf *pwc_get_next_fill_buf(struct pwc_device *pdev)
 {
 	unsigned long flags = 0;
@@ -306,6 +331,8 @@ static void pwc_isoc_handler(struct urb *urb)
 	/* Reset ISOC error counter. We did get here, after all. */
 	pdev->visoc_errors = 0;
 
+	dma_sync_single_for_cpu(&urb->dev->dev, urb->transfer_dma, urb->transfer_buffer_length, DMA_FROM_DEVICE);
+
 	/* vsync: 0 = don't copy data
 		  1 = sync-hunt
 		  2 = synched
@@ -428,16 +455,13 @@ static int pwc_isoc_init(struct pwc_device *pdev)
 		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_buffer_length = ISO_BUFFER_SIZE;
+		urb->transfer_buffer = pwc_alloc_urb_buffer(&udev->dev, urb->transfer_buffer_length, &urb->transfer_dma);
 		if (urb->transfer_buffer == NULL) {
 			PWC_ERROR("Failed to allocate urb buffer %d\n", i);
 			pwc_isoc_cleanup(pdev);
 			return -ENOMEM;
 		}
-		urb->transfer_buffer_length = ISO_BUFFER_SIZE;
 		urb->complete = pwc_isoc_handler;
 		urb->context = pdev;
 		urb->start_frame = 0;
@@ -488,15 +512,14 @@ static void pwc_iso_free(struct pwc_device *pdev)
 
 	/* Freeing ISOC buffers one by one */
 	for (i = 0; i < MAX_ISO_BUFS; i++) {
-		if (pdev->urbs[i]) {
+		struct urb* urb = pdev->urbs[i];
+
+		if (urb) {
 			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);
+			if (urb->transfer_buffer) {
+				pwc_free_urb_buffer(&urb->dev->dev, urb->transfer_buffer_length, urb->transfer_buffer, urb->transfer_dma);
 			}
-			usb_free_urb(pdev->urbs[i]);
+			usb_free_urb(urb);
 			pdev->urbs[i] = NULL;
 		}
 	}
-- 
2.16.4

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

[Laurent Pinchart]

Hi Matwey,

Thank you for the patch.

On Thursday, 9 August 2018 12:33:07 EEST Matwey V. Kornilov wrote:
> DMA cocherency slows the transfer down on systems without hardware
> coherent DMA. Instead we use noncocherent DMA memory and explicit sync at
> data receive handler.
> 
> 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 = (2.0 +- 0.4) GBps
>                        time = (5.0 +- 3.0) usec
>  * usb_alloc_coherent: rate = (3.4 +- 1.2) GBps
>                        time = (3.5 +- 3.0) usec
> 
> We see that the measurements agree within error ranges in this case.
> So theoretically predicted performance downgrade cannot be reliably measured
> here.
> 
> armv7l based system (TI AM335x BeagleBone Black @ 300MHz). 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 =  (114 +- 5) MBps
>                        time =   (84 +- 4) usec
>  * usb_alloc_coherent: rate = (28.1 +- 0.1) MBps
>                        time =  (341 +- 2) usec
> 
> Note, that quantative difference leads (this commit leads to 4 times
> acceleration) to qualitative behavior change in this case. As it was
> stated before, the video stream cannot 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 | 47 ++++++++++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> index 72d2897a4b9f..17f2015a75bb 100644
> --- a/drivers/media/usb/pwc/pwc-if.c
> +++ b/drivers/media/usb/pwc/pwc-if.c
> @@ -159,6 +159,31 @@ static const struct video_device pwc_template = {
>  /**************************************************************************
> */ /* Private functions */
> 
> +static void* pwc_alloc_urb_buffer(struct device *dev, size_t size,
> dma_addr_t *dma_handle) {

There are several violations of the Linux coding style here, could you please 
run your patch through checkpatch.pl to fix them ?

> +	void* buffer = kmalloc(size, GFP_KERNEL);
> +
> +	if (buffer == NULL) {
> +		goto fail_kmalloc;
> +	}

No need for a goto, you can just return NULL here.

> +	*dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
> +	if (dma_mapping_error(dev, *dma_handle)) {
> +		goto fail_dma_map_single;

And you can inline the error handling code here as it's used from a single 
location.

> +	}
> +
> +	return buffer;
> +
> +fail_dma_map_single:
> +	kfree(buffer);
> +fail_kmalloc:
> +	return NULL;
> +}
> +
> +static void pwc_free_urb_buffer(struct device *dev, size_t size, void*
> buffer, dma_addr_t dma_handle) {
> +	dma_unmap_single(dev, dma_handle, size, DMA_FROM_DEVICE);
> +	kfree(buffer);
> +}
> +
>  static struct pwc_frame_buf *pwc_get_next_fill_buf(struct pwc_device *pdev)
> {
>  	unsigned long flags = 0;
> @@ -306,6 +331,8 @@ static void pwc_isoc_handler(struct urb *urb)
>  	/* Reset ISOC error counter. We did get here, after all. */
>  	pdev->visoc_errors = 0;
> 
> +	dma_sync_single_for_cpu(&urb->dev->dev, urb->transfer_dma,
> urb->transfer_buffer_length, DMA_FROM_DEVICE);
> +
>  	/* vsync: 0 = don't copy data
>  		  1 = sync-hunt
>  		  2 = synched
> @@ -428,16 +455,13 @@ static int pwc_isoc_init(struct pwc_device *pdev)
>  		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_buffer_length = ISO_BUFFER_SIZE;
> +		urb->transfer_buffer = pwc_alloc_urb_buffer(&udev->dev,
> urb->transfer_buffer_length, &urb->transfer_dma);
>  		if (urb->transfer_buffer == NULL) {
>  			PWC_ERROR("Failed to allocate urb buffer %d\n", i);
>  			pwc_isoc_cleanup(pdev);
>  			return -ENOMEM;
>  		}
> -		urb->transfer_buffer_length = ISO_BUFFER_SIZE;
>  		urb->complete = pwc_isoc_handler;
>  		urb->context = pdev;
>  		urb->start_frame = 0;
> @@ -488,15 +512,14 @@ static void pwc_iso_free(struct pwc_device *pdev)
> 
>  	/* Freeing ISOC buffers one by one */
>  	for (i = 0; i < MAX_ISO_BUFS; i++) {
> -		if (pdev->urbs[i]) {
> +		struct urb* urb = pdev->urbs[i];
> +
> +		if (urb) {
>  			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);
> +			if (urb->transfer_buffer) {
> +				pwc_free_urb_buffer(&urb->dev->dev, urb->transfer_buffer_length,
> urb->transfer_buffer, urb->transfer_dma); }
> -			usb_free_urb(pdev->urbs[i]);
> +			usb_free_urb(urb);
>  			pdev->urbs[i] = NULL;
>  		}
>  	}

-- 
Regards,

Laurent Pinchart

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

[Matwey V. Kornilov]

2018-08-09 12:51 GMT+03:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Matwey,
>
> Thank you for the patch.
>
> On Thursday, 9 August 2018 12:33:07 EEST Matwey V. Kornilov wrote:
>> DMA cocherency slows the transfer down on systems without hardware
>> coherent DMA. Instead we use noncocherent DMA memory and explicit sync at
>> data receive handler.
>>
>> 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 = (2.0 +- 0.4) GBps
>>                        time = (5.0 +- 3.0) usec
>>  * usb_alloc_coherent: rate = (3.4 +- 1.2) GBps
>>                        time = (3.5 +- 3.0) usec
>>
>> We see that the measurements agree within error ranges in this case.
>> So theoretically predicted performance downgrade cannot be reliably measured
>> here.
>>
>> armv7l based system (TI AM335x BeagleBone Black @ 300MHz). 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 =  (114 +- 5) MBps
>>                        time =   (84 +- 4) usec
>>  * usb_alloc_coherent: rate = (28.1 +- 0.1) MBps
>>                        time =  (341 +- 2) usec
>>
>> Note, that quantative difference leads (this commit leads to 4 times
>> acceleration) to qualitative behavior change in this case. As it was
>> stated before, the video stream cannot 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 | 47 ++++++++++++++++++++++++++++-----------
>>  1 file changed, 35 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
>> index 72d2897a4b9f..17f2015a75bb 100644
>> --- a/drivers/media/usb/pwc/pwc-if.c
>> +++ b/drivers/media/usb/pwc/pwc-if.c
>> @@ -159,6 +159,31 @@ static const struct video_device pwc_template = {
>>  /**************************************************************************
>> */ /* Private functions */
>>
>> +static void* pwc_alloc_urb_buffer(struct device *dev, size_t size,
>> dma_addr_t *dma_handle) {
>
> There are several violations of the Linux coding style here, could you please
> run your patch through checkpatch.pl to fix them ?

Thanks. I'll fix them. I really need to configure git hooks with checkpatch.

>
>> +     void* buffer = kmalloc(size, GFP_KERNEL);
>> +
>> +     if (buffer == NULL) {
>> +             goto fail_kmalloc;
>> +     }
>
> No need for a goto, you can just return NULL here.
>
>> +     *dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
>> +     if (dma_mapping_error(dev, *dma_handle)) {
>> +             goto fail_dma_map_single;
>
> And you can inline the error handling code here as it's used from a single
> location.
>
>> +     }
>> +
>> +     return buffer;
>> +
>> +fail_dma_map_single:
>> +     kfree(buffer);
>> +fail_kmalloc:
>> +     return NULL;
>> +}
>> +
>> +static void pwc_free_urb_buffer(struct device *dev, size_t size, void*
>> buffer, dma_addr_t dma_handle) {
>> +     dma_unmap_single(dev, dma_handle, size, DMA_FROM_DEVICE);
>> +     kfree(buffer);
>> +}
>> +
>>  static struct pwc_frame_buf *pwc_get_next_fill_buf(struct pwc_device *pdev)
>> {
>>       unsigned long flags = 0;
>> @@ -306,6 +331,8 @@ static void pwc_isoc_handler(struct urb *urb)
>>       /* Reset ISOC error counter. We did get here, after all. */
>>       pdev->visoc_errors = 0;
>>
>> +     dma_sync_single_for_cpu(&urb->dev->dev, urb->transfer_dma,
>> urb->transfer_buffer_length, DMA_FROM_DEVICE);
>> +
>>       /* vsync: 0 = don't copy data
>>                 1 = sync-hunt
>>                 2 = synched
>> @@ -428,16 +455,13 @@ static int pwc_isoc_init(struct pwc_device *pdev)
>>               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_buffer_length = ISO_BUFFER_SIZE;
>> +             urb->transfer_buffer = pwc_alloc_urb_buffer(&udev->dev,
>> urb->transfer_buffer_length, &urb->transfer_dma);
>>               if (urb->transfer_buffer == NULL) {
>>                       PWC_ERROR("Failed to allocate urb buffer %d\n", i);
>>                       pwc_isoc_cleanup(pdev);
>>                       return -ENOMEM;
>>               }
>> -             urb->transfer_buffer_length = ISO_BUFFER_SIZE;
>>               urb->complete = pwc_isoc_handler;
>>               urb->context = pdev;
>>               urb->start_frame = 0;
>> @@ -488,15 +512,14 @@ static void pwc_iso_free(struct pwc_device *pdev)
>>
>>       /* Freeing ISOC buffers one by one */
>>       for (i = 0; i < MAX_ISO_BUFS; i++) {
>> -             if (pdev->urbs[i]) {
>> +             struct urb* urb = pdev->urbs[i];
>> +
>> +             if (urb) {
>>                       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);
>> +                     if (urb->transfer_buffer) {
>> +                             pwc_free_urb_buffer(&urb->dev->dev, urb->transfer_buffer_length,
>> urb->transfer_buffer, urb->transfer_dma); }
>> -                     usb_free_urb(pdev->urbs[i]);
>> +                     usb_free_urb(urb);
>>                       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