* [PATCH v6 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
@ 2018-11-09 19:03 Matwey V. Kornilov
2018-11-09 19:03 ` [PATCH v6 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() Matwey V. Kornilov
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Matwey V. Kornilov @ 2018-11-09 19:03 UTC (permalink / raw)
To: linux-media, linux-kernel
Cc: matwey.kornilov, Matwey V. Kornilov, tfiga, laurent.pinchart,
stern, ezequiel, hdegoede, hverkuil, mchehab, rostedt, mingo,
isely, bhumirks, colin.king, kieran.bingham, keiichiw
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 3.3 times speedup.
Changes since v5:
* add dma_sync_single_for_device() as required by Laurent Pinchart
Changes since v4:
* fix fields order in trace events
* minor style fixes
Changes since v3:
* fix scripts/checkpatch.pl errors
* use __string to store name in trace events
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 | 69 ++++++++++++++++++++++++++++++++++--------
include/trace/events/pwc.h | 65 +++++++++++++++++++++++++++++++++++++++
2 files changed, 121 insertions(+), 13 deletions(-)
create mode 100644 include/trace/events/pwc.h
--
2.16.4
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v6 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler()
2018-11-09 19:03 [PATCH v6 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov
@ 2018-11-09 19:03 ` Matwey V. Kornilov
2018-11-09 19:32 ` Steven Rostedt
2018-12-12 17:36 ` [PATCH v6 3/2] MAINTAINERS: added include/trace/events/pwc.h Hans Verkuil
2018-11-09 19:03 ` [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov
2018-12-12 17:28 ` [PATCH v6 0/2] " Laurent Pinchart
2 siblings, 2 replies; 14+ messages in thread
From: Matwey V. Kornilov @ 2018-11-09 19:03 UTC (permalink / raw)
To: linux-media, linux-kernel
Cc: matwey.kornilov, Matwey V. Kornilov, tfiga, laurent.pinchart,
stern, ezequiel, hdegoede, hverkuil, mchehab, rostedt, mingo,
isely, bhumirks, colin.king, kieran.bingham, keiichiw
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 | 65 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 72 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 72704f4d5330..53c111bd5a22 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..a2da764a3b41
--- /dev/null
+++ b/include/trace/events/pwc.h
@@ -0,0 +1,65 @@
+/* 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(struct pwc_frame_buf*, fbuf)
+ __field(int, urb__status)
+ __field(u32, urb__actual_length)
+ __field(int, fbuf__filled)
+ __string(name, pdev->v4l2_dev.name)
+ ),
+ TP_fast_assign(
+ __entry->urb = urb;
+ __entry->fbuf = pdev->fill_buf;
+ __entry->urb__status = urb->status;
+ __entry->urb__actual_length = urb->actual_length;
+ __entry->fbuf__filled = (pdev->fill_buf
+ ? pdev->fill_buf->filled : 0);
+ __assign_str(name, pdev->v4l2_dev.name);
+ ),
+ TP_printk("dev=%s (fbuf=%p filled=%d) urb=%p (status=%d actual_length=%u)",
+ __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(struct pwc_frame_buf*, fbuf)
+ __field(int, fbuf__filled)
+ __string(name, pdev->v4l2_dev.name)
+ ),
+ TP_fast_assign(
+ __entry->urb = urb;
+ __entry->fbuf = pdev->fill_buf;
+ __entry->fbuf__filled = pdev->fill_buf->filled;
+ __assign_str(name, pdev->v4l2_dev.name);
+ ),
+ TP_printk(" dev=%s (fbuf=%p filled=%d) urb=%p",
+ __get_str(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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
2018-11-09 19:03 [PATCH v6 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov
2018-11-09 19:03 ` [PATCH v6 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() Matwey V. Kornilov
@ 2018-11-09 19:03 ` Matwey V. Kornilov
2018-11-21 18:15 ` Matwey V. Kornilov
2018-12-12 17:28 ` [PATCH v6 0/2] " Laurent Pinchart
2 siblings, 1 reply; 14+ messages in thread
From: Matwey V. Kornilov @ 2018-11-09 19:03 UTC (permalink / raw)
To: linux-media, linux-kernel
Cc: matwey.kornilov, Matwey V. Kornilov, tfiga, laurent.pinchart,
stern, ezequiel, hdegoede, hverkuil, mchehab, rostedt, mingo,
isely, bhumirks, colin.king, kieran.bingham, keiichiw
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 = ( 94 +- 4) MBps
time = (101 +- 4) usec
* usb_alloc_coherent: rate = (28.1 +- 0.1) MBps
time = (341 +- 2) usec
Note, that quantative difference leads (this commit leads to 3.3 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 | 62 +++++++++++++++++++++++++++++++++---------
1 file changed, 49 insertions(+), 13 deletions(-)
diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
index 53c111bd5a22..a81fb319b339 100644
--- a/drivers/media/usb/pwc/pwc-if.c
+++ b/drivers/media/usb/pwc/pwc-if.c
@@ -159,6 +159,32 @@ 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)
+ return NULL;
+
+ *dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
+ if (dma_mapping_error(dev, *dma_handle)) {
+ kfree(buffer);
+ return NULL;
+ }
+
+ return buffer;
+}
+
+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 +332,11 @@ 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
@@ -352,6 +383,11 @@ static void pwc_isoc_handler(struct urb *urb)
pdev->vlast_packet_size = flen;
}
+ dma_sync_single_for_device(&urb->dev->dev,
+ urb->transfer_dma,
+ urb->transfer_buffer_length,
+ DMA_FROM_DEVICE);
+
handler_end:
trace_pwc_handler_exit(urb, pdev);
@@ -428,16 +464,15 @@ 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 +523,16 @@ 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);
- }
- usb_free_urb(pdev->urbs[i]);
+ if (urb->transfer_buffer)
+ pwc_free_urb_buffer(&urb->dev->dev,
+ urb->transfer_buffer_length,
+ urb->transfer_buffer,
+ urb->transfer_dma);
+ usb_free_urb(urb);
pdev->urbs[i] = NULL;
}
}
--
2.16.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler()
2018-11-09 19:03 ` [PATCH v6 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() Matwey V. Kornilov
@ 2018-11-09 19:32 ` Steven Rostedt
2018-12-12 17:36 ` [PATCH v6 3/2] MAINTAINERS: added include/trace/events/pwc.h Hans Verkuil
1 sibling, 0 replies; 14+ messages in thread
From: Steven Rostedt @ 2018-11-09 19:32 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: linux-media, linux-kernel, matwey.kornilov, tfiga,
laurent.pinchart, stern, ezequiel, hdegoede, hverkuil, mchehab,
mingo, isely, bhumirks, colin.king, kieran.bingham, keiichiw
On Fri, 9 Nov 2018 22:03:26 +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
>From a tracing perspective, I don't see anything wrong with this patch.
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
-- Steve
>
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> ---
> drivers/media/usb/pwc/pwc-if.c | 7 +++++
> include/trace/events/pwc.h | 65 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 72 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 72704f4d5330..53c111bd5a22 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..a2da764a3b41
> --- /dev/null
> +++ b/include/trace/events/pwc.h
> @@ -0,0 +1,65 @@
> +/* 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(struct pwc_frame_buf*, fbuf)
> + __field(int, urb__status)
> + __field(u32, urb__actual_length)
> + __field(int, fbuf__filled)
> + __string(name, pdev->v4l2_dev.name)
> + ),
> + TP_fast_assign(
> + __entry->urb = urb;
> + __entry->fbuf = pdev->fill_buf;
> + __entry->urb__status = urb->status;
> + __entry->urb__actual_length = urb->actual_length;
> + __entry->fbuf__filled = (pdev->fill_buf
> + ? pdev->fill_buf->filled : 0);
> + __assign_str(name, pdev->v4l2_dev.name);
> + ),
> + TP_printk("dev=%s (fbuf=%p filled=%d) urb=%p (status=%d actual_length=%u)",
> + __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(struct pwc_frame_buf*, fbuf)
> + __field(int, fbuf__filled)
> + __string(name, pdev->v4l2_dev.name)
> + ),
> + TP_fast_assign(
> + __entry->urb = urb;
> + __entry->fbuf = pdev->fill_buf;
> + __entry->fbuf__filled = pdev->fill_buf->filled;
> + __assign_str(name, pdev->v4l2_dev.name);
> + ),
> + TP_printk(" dev=%s (fbuf=%p filled=%d) urb=%p",
> + __get_str(name),
> + __entry->fbuf,
> + __entry->fbuf__filled,
> + __entry->urb)
> +);
> +
> +#endif /* _TRACE_PWC_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
2018-11-09 19:03 ` [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov
@ 2018-11-21 18:15 ` Matwey V. Kornilov
2018-11-30 12:20 ` Matwey V. Kornilov
0 siblings, 1 reply; 14+ messages in thread
From: Matwey V. Kornilov @ 2018-11-21 18:15 UTC (permalink / raw)
To: Linux Media Mailing List, open list
Cc: Tomasz Figa, Laurent Pinchart, Alan Stern, Ezequiel Garcia,
Hans de Goede, Hans Verkuil, Mauro Carvalho Chehab,
Steven Rostedt, mingo, Mike Isely, Bhumika Goyal, Colin King,
Kieran Bingham, keiichiw
пт, 9 нояб. 2018 г. в 22:03, Matwey V. Kornilov <matwey@sai.msu.ru>:
>
> 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 = ( 94 +- 4) MBps
> time = (101 +- 4) usec
> * usb_alloc_coherent: rate = (28.1 +- 0.1) MBps
> time = (341 +- 2) usec
>
> Note, that quantative difference leads (this commit leads to 3.3 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>
Ping
> ---
> drivers/media/usb/pwc/pwc-if.c | 62 +++++++++++++++++++++++++++++++++---------
> 1 file changed, 49 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> index 53c111bd5a22..a81fb319b339 100644
> --- a/drivers/media/usb/pwc/pwc-if.c
> +++ b/drivers/media/usb/pwc/pwc-if.c
> @@ -159,6 +159,32 @@ 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)
> + return NULL;
> +
> + *dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
> + if (dma_mapping_error(dev, *dma_handle)) {
> + kfree(buffer);
> + return NULL;
> + }
> +
> + return buffer;
> +}
> +
> +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 +332,11 @@ 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
> @@ -352,6 +383,11 @@ static void pwc_isoc_handler(struct urb *urb)
> pdev->vlast_packet_size = flen;
> }
>
> + dma_sync_single_for_device(&urb->dev->dev,
> + urb->transfer_dma,
> + urb->transfer_buffer_length,
> + DMA_FROM_DEVICE);
> +
> handler_end:
> trace_pwc_handler_exit(urb, pdev);
>
> @@ -428,16 +464,15 @@ 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 +523,16 @@ 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);
> - }
> - usb_free_urb(pdev->urbs[i]);
> + if (urb->transfer_buffer)
> + pwc_free_urb_buffer(&urb->dev->dev,
> + urb->transfer_buffer_length,
> + urb->transfer_buffer,
> + urb->transfer_dma);
> + usb_free_urb(urb);
> pdev->urbs[i] = NULL;
> }
> }
> --
> 2.16.4
>
--
With best regards,
Matwey V. Kornilov
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
2018-11-21 18:15 ` Matwey V. Kornilov
@ 2018-11-30 12:20 ` Matwey V. Kornilov
2018-12-12 13:07 ` Matwey V. Kornilov
0 siblings, 1 reply; 14+ messages in thread
From: Matwey V. Kornilov @ 2018-11-30 12:20 UTC (permalink / raw)
To: Linux Media Mailing List, open list
Cc: Tomasz Figa, Laurent Pinchart, Alan Stern, Ezequiel Garcia,
Hans de Goede, Hans Verkuil, Mauro Carvalho Chehab,
Steven Rostedt, mingo, Mike Isely, Bhumika Goyal, Colin King,
Kieran Bingham, keiichiw
ср, 21 нояб. 2018 г. в 21:15, Matwey V. Kornilov <matwey.kornilov@gmail.com>:
>
> пт, 9 нояб. 2018 г. в 22:03, Matwey V. Kornilov <matwey@sai.msu.ru>:
> >
> > 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 = ( 94 +- 4) MBps
> > time = (101 +- 4) usec
> > * usb_alloc_coherent: rate = (28.1 +- 0.1) MBps
> > time = (341 +- 2) usec
> >
> > Note, that quantative difference leads (this commit leads to 3.3 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>
>
> Ping
Ping
>
> > ---
> > drivers/media/usb/pwc/pwc-if.c | 62 +++++++++++++++++++++++++++++++++---------
> > 1 file changed, 49 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> > index 53c111bd5a22..a81fb319b339 100644
> > --- a/drivers/media/usb/pwc/pwc-if.c
> > +++ b/drivers/media/usb/pwc/pwc-if.c
> > @@ -159,6 +159,32 @@ 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)
> > + return NULL;
> > +
> > + *dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
> > + if (dma_mapping_error(dev, *dma_handle)) {
> > + kfree(buffer);
> > + return NULL;
> > + }
> > +
> > + return buffer;
> > +}
> > +
> > +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 +332,11 @@ 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
> > @@ -352,6 +383,11 @@ static void pwc_isoc_handler(struct urb *urb)
> > pdev->vlast_packet_size = flen;
> > }
> >
> > + dma_sync_single_for_device(&urb->dev->dev,
> > + urb->transfer_dma,
> > + urb->transfer_buffer_length,
> > + DMA_FROM_DEVICE);
> > +
> > handler_end:
> > trace_pwc_handler_exit(urb, pdev);
> >
> > @@ -428,16 +464,15 @@ 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 +523,16 @@ 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);
> > - }
> > - usb_free_urb(pdev->urbs[i]);
> > + if (urb->transfer_buffer)
> > + pwc_free_urb_buffer(&urb->dev->dev,
> > + urb->transfer_buffer_length,
> > + urb->transfer_buffer,
> > + urb->transfer_dma);
> > + usb_free_urb(urb);
> > pdev->urbs[i] = NULL;
> > }
> > }
> > --
> > 2.16.4
> >
>
>
> --
> With best regards,
> Matwey V. Kornilov
--
With best regards,
Matwey V. Kornilov
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
2018-11-30 12:20 ` Matwey V. Kornilov
@ 2018-12-12 13:07 ` Matwey V. Kornilov
2018-12-12 13:56 ` Steven Rostedt
0 siblings, 1 reply; 14+ messages in thread
From: Matwey V. Kornilov @ 2018-12-12 13:07 UTC (permalink / raw)
To: Linux Media Mailing List, open list
Cc: Tomasz Figa, Laurent Pinchart, Alan Stern, Ezequiel Garcia,
Hans de Goede, Hans Verkuil, Mauro Carvalho Chehab,
Steven Rostedt, mingo, Mike Isely, Bhumika Goyal, Colin King,
Kieran Bingham, keiichiw
пт, 30 нояб. 2018 г. в 15:20, Matwey V. Kornilov <matwey.kornilov@gmail.com>:
>
> ср, 21 нояб. 2018 г. в 21:15, Matwey V. Kornilov <matwey.kornilov@gmail.com>:
> >
> > пт, 9 нояб. 2018 г. в 22:03, Matwey V. Kornilov <matwey@sai.msu.ru>:
> > >
> > > 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 = ( 94 +- 4) MBps
> > > time = (101 +- 4) usec
> > > * usb_alloc_coherent: rate = (28.1 +- 0.1) MBps
> > > time = (341 +- 2) usec
> > >
> > > Note, that quantative difference leads (this commit leads to 3.3 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>
> >
> > Ping
>
> Ping
Ping
>
> >
> > > ---
> > > drivers/media/usb/pwc/pwc-if.c | 62 +++++++++++++++++++++++++++++++++---------
> > > 1 file changed, 49 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> > > index 53c111bd5a22..a81fb319b339 100644
> > > --- a/drivers/media/usb/pwc/pwc-if.c
> > > +++ b/drivers/media/usb/pwc/pwc-if.c
> > > @@ -159,6 +159,32 @@ 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)
> > > + return NULL;
> > > +
> > > + *dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
> > > + if (dma_mapping_error(dev, *dma_handle)) {
> > > + kfree(buffer);
> > > + return NULL;
> > > + }
> > > +
> > > + return buffer;
> > > +}
> > > +
> > > +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 +332,11 @@ 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
> > > @@ -352,6 +383,11 @@ static void pwc_isoc_handler(struct urb *urb)
> > > pdev->vlast_packet_size = flen;
> > > }
> > >
> > > + dma_sync_single_for_device(&urb->dev->dev,
> > > + urb->transfer_dma,
> > > + urb->transfer_buffer_length,
> > > + DMA_FROM_DEVICE);
> > > +
> > > handler_end:
> > > trace_pwc_handler_exit(urb, pdev);
> > >
> > > @@ -428,16 +464,15 @@ 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 +523,16 @@ 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);
> > > - }
> > > - usb_free_urb(pdev->urbs[i]);
> > > + if (urb->transfer_buffer)
> > > + pwc_free_urb_buffer(&urb->dev->dev,
> > > + urb->transfer_buffer_length,
> > > + urb->transfer_buffer,
> > > + urb->transfer_dma);
> > > + usb_free_urb(urb);
> > > pdev->urbs[i] = NULL;
> > > }
> > > }
> > > --
> > > 2.16.4
> > >
> >
> >
> > --
> > With best regards,
> > Matwey V. Kornilov
>
>
>
> --
> With best regards,
> Matwey V. Kornilov
--
With best regards,
Matwey V. Kornilov
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
2018-12-12 13:07 ` Matwey V. Kornilov
@ 2018-12-12 13:56 ` Steven Rostedt
2018-12-12 14:01 ` Steven Rostedt
0 siblings, 1 reply; 14+ messages in thread
From: Steven Rostedt @ 2018-12-12 13:56 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: Linux Media Mailing List, open list, Tomasz Figa,
Laurent Pinchart, Alan Stern, Ezequiel Garcia, Hans de Goede,
Hans Verkuil, Mauro Carvalho Chehab, mingo, Mike Isely,
Bhumika Goyal, Colin King, Kieran Bingham, keiichiw,
Andrew Morton
Can someone please take this patch or at least say what's wrong with it
if you have a problem?
Matwey has been patiently pinging us once every other week for over a
month asking for a reply. I've already given my Reviewed-by from a
tracing perspective.
Ignoring patches is not a friendly gesture.
-- Steve
On Wed, 12 Dec 2018 16:07:38 +0300
"Matwey V. Kornilov" <matwey.kornilov@gmail.com> wrote:
> пт, 30 нояб. 2018 г. в 15:20, Matwey V. Kornilov <matwey.kornilov@gmail.com>:
> >
> > ср, 21 нояб. 2018 г. в 21:15, Matwey V. Kornilov <matwey.kornilov@gmail.com>:
> > >
> > > пт, 9 нояб. 2018 г. в 22:03, Matwey V. Kornilov <matwey@sai.msu.ru>:
> > > >
> > > > 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 = ( 94 +- 4) MBps
> > > > time = (101 +- 4) usec
> > > > * usb_alloc_coherent: rate = (28.1 +- 0.1) MBps
> > > > time = (341 +- 2) usec
> > > >
> > > > Note, that quantative difference leads (this commit leads to 3.3 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>
> > >
> > > Ping
> >
> > Ping
>
> Ping
>
> >
> > >
> > > > ---
> > > > drivers/media/usb/pwc/pwc-if.c | 62 +++++++++++++++++++++++++++++++++---------
> > > > 1 file changed, 49 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> > > > index 53c111bd5a22..a81fb319b339 100644
> > > > --- a/drivers/media/usb/pwc/pwc-if.c
> > > > +++ b/drivers/media/usb/pwc/pwc-if.c
> > > > @@ -159,6 +159,32 @@ 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)
> > > > + return NULL;
> > > > +
> > > > + *dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
> > > > + if (dma_mapping_error(dev, *dma_handle)) {
> > > > + kfree(buffer);
> > > > + return NULL;
> > > > + }
> > > > +
> > > > + return buffer;
> > > > +}
> > > > +
> > > > +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 +332,11 @@ 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
> > > > @@ -352,6 +383,11 @@ static void pwc_isoc_handler(struct urb *urb)
> > > > pdev->vlast_packet_size = flen;
> > > > }
> > > >
> > > > + dma_sync_single_for_device(&urb->dev->dev,
> > > > + urb->transfer_dma,
> > > > + urb->transfer_buffer_length,
> > > > + DMA_FROM_DEVICE);
> > > > +
> > > > handler_end:
> > > > trace_pwc_handler_exit(urb, pdev);
> > > >
> > > > @@ -428,16 +464,15 @@ 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 +523,16 @@ 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);
> > > > - }
> > > > - usb_free_urb(pdev->urbs[i]);
> > > > + if (urb->transfer_buffer)
> > > > + pwc_free_urb_buffer(&urb->dev->dev,
> > > > + urb->transfer_buffer_length,
> > > > + urb->transfer_buffer,
> > > > + urb->transfer_dma);
> > > > + usb_free_urb(urb);
> > > > pdev->urbs[i] = NULL;
> > > > }
> > > > }
> > > > --
> > > > 2.16.4
> > > >
> > >
> > >
> > > --
> > > With best regards,
> > > Matwey V. Kornilov
> >
> >
> >
> > --
> > With best regards,
> > Matwey V. Kornilov
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
2018-12-12 13:56 ` Steven Rostedt
@ 2018-12-12 14:01 ` Steven Rostedt
2018-12-12 15:15 ` Matwey V. Kornilov
0 siblings, 1 reply; 14+ messages in thread
From: Steven Rostedt @ 2018-12-12 14:01 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: Linux Media Mailing List, open list, Tomasz Figa,
Laurent Pinchart, Alan Stern, Ezequiel Garcia, Hans de Goede,
Hans Verkuil, Mauro Carvalho Chehab, mingo, Mike Isely,
Bhumika Goyal, Colin King, Kieran Bingham, keiichiw,
Andrew Morton
On Wed, 12 Dec 2018 08:56:22 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> Can someone please take this patch or at least say what's wrong with it
> if you have a problem?
>
> Matwey has been patiently pinging us once every other week for over a
> month asking for a reply. I've already given my Reviewed-by from a
> tracing perspective.
>
> Ignoring patches is not a friendly gesture.
>
Nevermind, it appears that v5 is still under discussion.
Matwey, does v6 address the comments made in v5?
-- Steve
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
2018-12-12 14:01 ` Steven Rostedt
@ 2018-12-12 15:15 ` Matwey V. Kornilov
0 siblings, 0 replies; 14+ messages in thread
From: Matwey V. Kornilov @ 2018-12-12 15:15 UTC (permalink / raw)
To: Steven Rostedt
Cc: Linux Media Mailing List, open list, Tomasz Figa,
Laurent Pinchart, Alan Stern, Ezequiel Garcia, Hans de Goede,
Hans Verkuil, Mauro Carvalho Chehab, mingo, Mike Isely,
Bhumika Goyal, Colin King, Kieran Bingham, keiichiw, akpm
ср, 12 дек. 2018 г. в 17:01, Steven Rostedt <rostedt@goodmis.org>:
>
> On Wed, 12 Dec 2018 08:56:22 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > Can someone please take this patch or at least say what's wrong with it
> > if you have a problem?
> >
> > Matwey has been patiently pinging us once every other week for over a
> > month asking for a reply. I've already given my Reviewed-by from a
> > tracing perspective.
> >
> > Ignoring patches is not a friendly gesture.
> >
>
> Nevermind, it appears that v5 is still under discussion.
>
> Matwey, does v6 address the comments made in v5?
Hi,
v6 addresses the comments made by Laurent Pinchart on Oct, 31:
https://www.spinics.net/lists/linux-media/msg142216.html
namely, dma_sync_single_for_device() is introduced in the proper place
>
> -- Steve
--
With best regards,
Matwey V. Kornilov
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
2018-11-09 19:03 [PATCH v6 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov
2018-11-09 19:03 ` [PATCH v6 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() Matwey V. Kornilov
2018-11-09 19:03 ` [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov
@ 2018-12-12 17:28 ` Laurent Pinchart
2018-12-12 17:41 ` Ezequiel Garcia
2 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2018-12-12 17:28 UTC (permalink / raw)
To: Matwey V. Kornilov
Cc: linux-media, linux-kernel, matwey.kornilov, tfiga, stern,
ezequiel, hdegoede, hverkuil, mchehab, rostedt, mingo, isely,
bhumirks, colin.king, kieran.bingham, keiichiw
Hi Matwey,
Thank you for the patches.
For the whole series,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
On Friday, 9 November 2018 21:03:25 EET Matwey V. Kornilov wrote:
> 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 3.3 times speedup.
>
> Changes since v5:
> * add dma_sync_single_for_device() as required by Laurent Pinchart
>
> Changes since v4:
> * fix fields order in trace events
> * minor style fixes
>
> Changes since v3:
> * fix scripts/checkpatch.pl errors
> * use __string to store name in trace events
>
> 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 | 69 +++++++++++++++++++++++++++++++--------
> include/trace/events/pwc.h | 65 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 121 insertions(+), 13 deletions(-)
> create mode 100644 include/trace/events/pwc.h
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v6 3/2] MAINTAINERS: added include/trace/events/pwc.h
2018-11-09 19:03 ` [PATCH v6 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() Matwey V. Kornilov
2018-11-09 19:32 ` Steven Rostedt
@ 2018-12-12 17:36 ` Hans Verkuil
1 sibling, 0 replies; 14+ messages in thread
From: Hans Verkuil @ 2018-12-12 17:36 UTC (permalink / raw)
To: Matwey V. Kornilov, linux-media, linux-kernel
Cc: matwey.kornilov, tfiga, laurent.pinchart, stern, ezequiel,
hdegoede, mchehab, rostedt, mingo, isely, bhumirks, colin.king,
kieran.bingham, keiichiw
Added include/trace/events/pwc.h to the list of files.
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
Will include this patch as part of the pull request.
---
MAINTAINERS | 1 +
1 file changed, 1 insertion(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 3e9f1710ed13..3ce9533bca26 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12191,6 +12191,7 @@ L: linux-media@vger.kernel.org
T: git git://linuxtv.org/media_tree.git
S: Odd Fixes
F: drivers/media/usb/pwc/*
+F: include/trace/events/pwc.h
PWM FAN DRIVER
M: Kamil Debski <kamil@wypas.org>
--
2.19.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
2018-12-12 17:28 ` [PATCH v6 0/2] " Laurent Pinchart
@ 2018-12-12 17:41 ` Ezequiel Garcia
2018-12-24 8:23 ` Matwey V. Kornilov
0 siblings, 1 reply; 14+ messages in thread
From: Ezequiel Garcia @ 2018-12-12 17:41 UTC (permalink / raw)
To: Laurent Pinchart
Cc: matwey, linux-media, Linux Kernel Mailing List, matwey.kornilov,
Tomasz Figa, Alan Stern, Ezequiel Garcia, Hans de Goede,
Hans Verkuil, Mauro Carvalho Chehab, rostedt, mingo, isely,
bhumirks, colin.king, kieran.bingham, Keiichi Watanabe
On Wed, 12 Dec 2018 at 14:27, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Matwey,
>
> Thank you for the patches.
>
> For the whole series,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
Thanks Laurent.
Matwey, given your detailed analysis of this issue,
and given you have pwc hardware to test, I think
you should consider co-maintaining this driver.
Thanks,
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
2018-12-12 17:41 ` Ezequiel Garcia
@ 2018-12-24 8:23 ` Matwey V. Kornilov
0 siblings, 0 replies; 14+ messages in thread
From: Matwey V. Kornilov @ 2018-12-24 8:23 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Laurent Pinchart, linux-media, Linux Kernel Mailing List,
Tomasz Figa, Alan Stern, Ezequiel Garcia, Hans de Goede,
Hans Verkuil, Mauro Carvalho Chehab, Steven Rostedt, mingo,
Mike Isely, Bhumika Goyal, Colin King, Kieran Bingham,
Keiichi Watanabe
ср, 12 дек. 2018 г. в 20:41, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>
> On Wed, 12 Dec 2018 at 14:27, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Matwey,
> >
> > Thank you for the patches.
> >
> > For the whole series,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
>
> Thanks Laurent.
>
> Matwey, given your detailed analysis of this issue,
> and given you have pwc hardware to test, I think
> you should consider co-maintaining this driver.
>
Well, It would be great if I could help. Is there some guide how to apply?
> Thanks,
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar
--
With best regards,
Matwey V. Kornilov
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-12-24 8:24 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-09 19:03 [PATCH v6 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov
2018-11-09 19:03 ` [PATCH v6 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() Matwey V. Kornilov
2018-11-09 19:32 ` Steven Rostedt
2018-12-12 17:36 ` [PATCH v6 3/2] MAINTAINERS: added include/trace/events/pwc.h Hans Verkuil
2018-11-09 19:03 ` [PATCH v6 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov
2018-11-21 18:15 ` Matwey V. Kornilov
2018-11-30 12:20 ` Matwey V. Kornilov
2018-12-12 13:07 ` Matwey V. Kornilov
2018-12-12 13:56 ` Steven Rostedt
2018-12-12 14:01 ` Steven Rostedt
2018-12-12 15:15 ` Matwey V. Kornilov
2018-12-12 17:28 ` [PATCH v6 0/2] " Laurent Pinchart
2018-12-12 17:41 ` Ezequiel Garcia
2018-12-24 8:23 ` Matwey V. Kornilov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).