* [PATCH v4 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer @ 2018-08-09 18:11 Matwey V. Kornilov 2018-08-09 18:11 ` [PATCH v4 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() Matwey V. Kornilov ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Matwey V. Kornilov @ 2018-08-09 18:11 UTC (permalink / raw) To: linux-media, linux-kernel Cc: Matwey V. Kornilov, tfiga, laurent.pinchart, matwey, stern, ezequiel, hdegoede, hverkuil, mchehab, rostedt, mingo, isely, bhumirks, colin.king, kieran.bingham, keiichiw 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 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 | 63 ++++++++++++++++++++++++++++++++-------- include/trace/events/pwc.h | 65 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 12 deletions(-) create mode 100644 include/trace/events/pwc.h -- 2.16.4 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() 2018-08-09 18:11 [PATCH v4 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov @ 2018-08-09 18:11 ` Matwey V. Kornilov 2018-08-09 18:26 ` Steven Rostedt 2018-08-09 18:11 ` [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov 2018-08-09 19:31 ` [PATCH v4 0/2] " Linus Torvalds 2 siblings, 1 reply; 15+ messages in thread From: Matwey V. Kornilov @ 2018-08-09 18:11 UTC (permalink / raw) To: linux-media, linux-kernel Cc: 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 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..84807fea2217 --- /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(int, urb__status) + __field(u32, urb__actual_length) + __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; + __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)", + __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) + __string(name, pdev->v4l2_dev.name) + __field(struct pwc_frame_buf*, fbuf) + __field(int, fbuf__filled) + ), + TP_fast_assign( + __entry->urb = urb; + __assign_str(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", + __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] 15+ messages in thread
* Re: [PATCH v4 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() 2018-08-09 18:11 ` [PATCH v4 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() Matwey V. Kornilov @ 2018-08-09 18:26 ` Steven Rostedt 2018-08-09 18:39 ` Matwey V. Kornilov 0 siblings, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2018-08-09 18:26 UTC (permalink / raw) To: Matwey V. Kornilov Cc: linux-media, linux-kernel, tfiga, laurent.pinchart, stern, ezequiel, hdegoede, hverkuil, mchehab, mingo, isely, bhumirks, colin.king, kieran.bingham, keiichiw On Thu, 9 Aug 2018 21:11:02 +0300 "Matwey V. Kornilov" <matwey@sai.msu.ru> wrote: > 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 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..84807fea2217 > --- /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(int, urb__status) > + __field(u32, urb__actual_length) > + __string(name, pdev->v4l2_dev.name) > + __field(struct pwc_frame_buf*, fbuf) > + __field(int, fbuf__filled) I should have mentioned this before. The above is a structure, and the fields of that structure are created in the same order as this list. It's best to try to make sure there's no "holes". I would recommend: __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) Best to have names at the end, as in the future I may be able to optimize dynamic arrays (like strings) better, if there's only one and its at the end of the structure. > + ), > + TP_fast_assign( > + __entry->urb = urb; > + __entry->urb__status = urb->status; > + __entry->urb__actual_length = urb->actual_length; > + __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)", > + __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) > + __string(name, pdev->v4l2_dev.name) > + __field(struct pwc_frame_buf*, fbuf) > + __field(int, fbuf__filled) This I would just move name to the end. Thanks! -- Steve > + ), > + TP_fast_assign( > + __entry->urb = urb; > + __assign_str(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", > + __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] 15+ messages in thread
* Re: [PATCH v4 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() 2018-08-09 18:26 ` Steven Rostedt @ 2018-08-09 18:39 ` Matwey V. Kornilov 0 siblings, 0 replies; 15+ messages in thread From: Matwey V. Kornilov @ 2018-08-09 18:39 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 чт, 9 авг. 2018 г. в 21:26, Steven Rostedt <rostedt@goodmis.org>: > > On Thu, 9 Aug 2018 21:11:02 +0300 > "Matwey V. Kornilov" <matwey@sai.msu.ru> wrote: > > > 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 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..84807fea2217 > > --- /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(int, urb__status) > > + __field(u32, urb__actual_length) > > + __string(name, pdev->v4l2_dev.name) > > + __field(struct pwc_frame_buf*, fbuf) > > + __field(int, fbuf__filled) > > I should have mentioned this before. The above is a structure, and the > fields of that structure are created in the same order as this list. > It's best to try to make sure there's no "holes". I would recommend: > > __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) > > Best to have names at the end, as in the future I may be able to > optimize dynamic arrays (like strings) better, if there's only one and > its at the end of the structure. I see, thank you! > > > + ), > > + TP_fast_assign( > > + __entry->urb = urb; > > + __entry->urb__status = urb->status; > > + __entry->urb__actual_length = urb->actual_length; > > + __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)", > > + __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) > > + __string(name, pdev->v4l2_dev.name) > > + __field(struct pwc_frame_buf*, fbuf) > > + __field(int, fbuf__filled) > > This I would just move name to the end. > > Thanks! > > -- Steve > > > + ), > > + TP_fast_assign( > > + __entry->urb = urb; > > + __assign_str(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", > > + __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> > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer 2018-08-09 18:11 [PATCH v4 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov 2018-08-09 18:11 ` [PATCH v4 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() Matwey V. Kornilov @ 2018-08-09 18:11 ` Matwey V. Kornilov 2018-08-10 8:53 ` Laurent Pinchart 2018-08-09 19:31 ` [PATCH v4 0/2] " Linus Torvalds 2 siblings, 1 reply; 15+ messages in thread From: Matwey V. Kornilov @ 2018-08-09 18:11 UTC (permalink / raw) To: linux-media, linux-kernel Cc: 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 = (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 | 56 +++++++++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 12 deletions(-) diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c index 72d2897a4b9f..e9c826be1ba6 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 @@ -428,16 +459,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 +518,17 @@ 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer 2018-08-09 18:11 ` [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov @ 2018-08-10 8:53 ` Laurent Pinchart 2018-08-10 9:38 ` Matwey V. Kornilov 0 siblings, 1 reply; 15+ messages in thread From: Laurent Pinchart @ 2018-08-10 8:53 UTC (permalink / raw) To: Matwey V. Kornilov Cc: linux-media, linux-kernel, tfiga, stern, ezequiel, hdegoede, hverkuil, mchehab, rostedt, mingo, isely, bhumirks, colin.king, kieran.bingham, keiichiw Hi Matwey, Thank you for the patch. On Thursday, 9 August 2018 21:11:03 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 | 56 +++++++++++++++++++++++++++++++------- > 1 file changed, 44 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c > index 72d2897a4b9f..e9c826be1ba6 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); > + Aren't you're missing a dma_sync_single_for_device() call before submitting the URB ? IIRC that's required for correct operation of the DMA mapping API on some platforms, depending on the cache architecture. The additional sync can affect performances, so it would be useful to re-run the perf test. > /* vsync: 0 = don't copy data > 1 = sync-hunt > 2 = synched > @@ -428,16 +459,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 +518,17 @@ 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); > } No need for curly braces. > - usb_free_urb(pdev->urbs[i]); > + usb_free_urb(urb); > pdev->urbs[i] = NULL; > } > } -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer 2018-08-10 8:53 ` Laurent Pinchart @ 2018-08-10 9:38 ` Matwey V. Kornilov 2018-08-10 9:49 ` Laurent Pinchart 0 siblings, 1 reply; 15+ messages in thread From: Matwey V. Kornilov @ 2018-08-10 9:38 UTC (permalink / raw) To: Laurent Pinchart Cc: Linux Media Mailing List, open 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, keiichiw 2018-08-10 11:53 GMT+03:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > Hi Matwey, > > Thank you for the patch. > > On Thursday, 9 August 2018 21:11:03 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 | 56 +++++++++++++++++++++++++++++++------- >> 1 file changed, 44 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c >> index 72d2897a4b9f..e9c826be1ba6 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); >> + > > Aren't you're missing a dma_sync_single_for_device() call before submitting > the URB ? IIRC that's required for correct operation of the DMA mapping API on > some platforms, depending on the cache architecture. The additional sync can > affect performances, so it would be useful to re-run the perf test. This was already discussed: https://lkml.org/lkml/2018/7/23/1051 I rely on Alan's reply: >According to Documentation/DMA-API-HOWTO.txt, the CPU should not write >to a DMA_FROM_DEVICE-mapped area, so dma_sync_single_for_device() is >not needed. > >> /* vsync: 0 = don't copy data >> 1 = sync-hunt >> 2 = synched >> @@ -428,16 +459,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 +518,17 @@ 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); >> } > > No need for curly braces. > >> - usb_free_urb(pdev->urbs[i]); >> + usb_free_urb(urb); >> pdev->urbs[i] = NULL; >> } >> } > > -- > Regards, > > Laurent Pinchart > > > -- With best regards, Matwey V. Kornilov ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer 2018-08-10 9:38 ` Matwey V. Kornilov @ 2018-08-10 9:49 ` Laurent Pinchart 2018-08-10 14:27 ` Alan Stern 0 siblings, 1 reply; 15+ messages in thread From: Laurent Pinchart @ 2018-08-10 9:49 UTC (permalink / raw) To: Matwey V. Kornilov Cc: Linux Media Mailing List, open 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, keiichiw Hi Matwey, On Friday, 10 August 2018 12:38:39 EEST Matwey V. Kornilov wrote: > 2018-08-10 11:53 GMT+03:00 Laurent Pinchart: > > On Thursday, 9 August 2018 21:11:03 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 | 56 +++++++++++++++++++++++++++++------- > >> 1 file changed, 44 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/media/usb/pwc/pwc-if.c > >> b/drivers/media/usb/pwc/pwc-if.c index 72d2897a4b9f..e9c826be1ba6 100644 > >> --- a/drivers/media/usb/pwc/pwc-if.c > >> +++ b/drivers/media/usb/pwc/pwc-if.c [snip] > >> @@ -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); > >> + > > > > Aren't you're missing a dma_sync_single_for_device() call before > > submitting the URB ? IIRC that's required for correct operation of the DMA > > mapping API on some platforms, depending on the cache architecture. The > > additional sync can affect performances, so it would be useful to re-run > > the perf test. > > This was already discussed: > > https://lkml.org/lkml/2018/7/23/1051 > > I rely on Alan's reply: > > > According to Documentation/DMA-API-HOWTO.txt, the CPU should not write > > to a DMA_FROM_DEVICE-mapped area, so dma_sync_single_for_device() is > > not needed. I fully agree that the CPU should not write to the buffer. However, I think the sync call is still needed. It's been a long time since I touched this area, but IIRC, some cache architectures (VIVT ?) require both cache clean before the transfer and cache invalidation after the transfer. On platforms where no cache management operation is needed before the transfer in the DMA_FROM_DEVICE direction, the dma_sync_*_for_device() calls should be no-ops (and if they're not, it's a bug of the DMA mapping implementation). -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer 2018-08-10 9:49 ` Laurent Pinchart @ 2018-08-10 14:27 ` Alan Stern 2018-08-17 17:44 ` Matwey V. Kornilov 2018-09-20 13:55 ` Ezequiel Garcia 0 siblings, 2 replies; 15+ messages in thread From: Alan Stern @ 2018-08-10 14:27 UTC (permalink / raw) To: Laurent Pinchart Cc: Matwey V. Kornilov, Linux Media Mailing List, open list, Tomasz Figa, Ezequiel Garcia, Hans de Goede, Hans Verkuil, Mauro Carvalho Chehab, Steven Rostedt, mingo, Mike Isely, Bhumika Goyal, Colin King, Kieran Bingham, keiichiw On Fri, 10 Aug 2018, Laurent Pinchart wrote: > > > Aren't you're missing a dma_sync_single_for_device() call before > > > submitting the URB ? IIRC that's required for correct operation of the DMA > > > mapping API on some platforms, depending on the cache architecture. The > > > additional sync can affect performances, so it would be useful to re-run > > > the perf test. > > > > This was already discussed: > > > > https://lkml.org/lkml/2018/7/23/1051 > > > > I rely on Alan's reply: > > > > > According to Documentation/DMA-API-HOWTO.txt, the CPU should not write > > > to a DMA_FROM_DEVICE-mapped area, so dma_sync_single_for_device() is > > > not needed. > > I fully agree that the CPU should not write to the buffer. However, I think > the sync call is still needed. It's been a long time since I touched this > area, but IIRC, some cache architectures (VIVT ?) require both cache clean > before the transfer and cache invalidation after the transfer. On platforms > where no cache management operation is needed before the transfer in the > DMA_FROM_DEVICE direction, the dma_sync_*_for_device() calls should be no-ops > (and if they're not, it's a bug of the DMA mapping implementation). In general, I agree that the cache has to be clean before a transfer starts. This means some sort of mapping operation (like dma_sync_*_for-device) is indeed required at some point between the allocation and the first transfer. For subsequent transfers, however, the cache is already clean and it will remain clean because the CPU will not do any writes to the buffer. (Note: clean != empty. Rather, clean == !dirty.) Therefore transfers following the first should not need any dma_sync_*_for_device. If you don't accept this reasoning then you should ask the people who wrote DMA-API-HOWTO.txt. They certainly will know more about this issue than I do. Alan Stern ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer 2018-08-10 14:27 ` Alan Stern @ 2018-08-17 17:44 ` Matwey V. Kornilov 2018-08-21 8:36 ` Matwey V. Kornilov 2018-09-20 13:55 ` Ezequiel Garcia 1 sibling, 1 reply; 15+ messages in thread From: Matwey V. Kornilov @ 2018-08-17 17:44 UTC (permalink / raw) To: Alan Stern Cc: Laurent Pinchart, Linux Media Mailing List, open list, Tomasz Figa, Ezequiel Garcia, Hans de Goede, Hans Verkuil, Mauro Carvalho Chehab, Steven Rostedt, mingo, Mike Isely, Bhumika Goyal, Colin King, Kieran Bingham, keiichiw пт, 10 авг. 2018 г. в 17:27, Alan Stern <stern@rowland.harvard.edu>: > > On Fri, 10 Aug 2018, Laurent Pinchart wrote: > > > > > Aren't you're missing a dma_sync_single_for_device() call before > > > > submitting the URB ? IIRC that's required for correct operation of the DMA > > > > mapping API on some platforms, depending on the cache architecture. The > > > > additional sync can affect performances, so it would be useful to re-run > > > > the perf test. > > > > > > This was already discussed: > > > > > > https://lkml.org/lkml/2018/7/23/1051 > > > > > > I rely on Alan's reply: > > > > > > > According to Documentation/DMA-API-HOWTO.txt, the CPU should not write > > > > to a DMA_FROM_DEVICE-mapped area, so dma_sync_single_for_device() is > > > > not needed. > > > > I fully agree that the CPU should not write to the buffer. However, I think > > the sync call is still needed. It's been a long time since I touched this > > area, but IIRC, some cache architectures (VIVT ?) require both cache clean > > before the transfer and cache invalidation after the transfer. On platforms > > where no cache management operation is needed before the transfer in the > > DMA_FROM_DEVICE direction, the dma_sync_*_for_device() calls should be no-ops > > (and if they're not, it's a bug of the DMA mapping implementation). > > In general, I agree that the cache has to be clean before a transfer > starts. This means some sort of mapping operation (like > dma_sync_*_for-device) is indeed required at some point between the > allocation and the first transfer. > > For subsequent transfers, however, the cache is already clean and it > will remain clean because the CPU will not do any writes to the buffer. > (Note: clean != empty. Rather, clean == !dirty.) Therefore transfers > following the first should not need any dma_sync_*_for_device. > > If you don't accept this reasoning then you should ask the people who > wrote DMA-API-HOWTO.txt. They certainly will know more about this > issue than I do. Laurent, I have not seen any data corruption or glitches without dma_sync_single_for_device() on ARM and x86. It takes additional ~20usec for dma_sync_single_for_device() to run on ARM. So It would be great to ensure that we can reliable save another 15% running time. > > Alan Stern > -- With best regards, Matwey V. Kornilov ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer 2018-08-17 17:44 ` Matwey V. Kornilov @ 2018-08-21 8:36 ` Matwey V. Kornilov 0 siblings, 0 replies; 15+ messages in thread From: Matwey V. Kornilov @ 2018-08-21 8:36 UTC (permalink / raw) To: Alan Stern Cc: Laurent Pinchart, Linux Media Mailing List, open list, Tomasz Figa, Ezequiel Garcia, Hans de Goede, Hans Verkuil, Mauro Carvalho Chehab, Steven Rostedt, mingo, Mike Isely, Bhumika Goyal, Colin King, Kieran Bingham, keiichiw 2018-08-17 20:44 GMT+03:00 Matwey V. Kornilov <matwey.kornilov@gmail.com>: > пт, 10 авг. 2018 г. в 17:27, Alan Stern <stern@rowland.harvard.edu>: >> >> On Fri, 10 Aug 2018, Laurent Pinchart wrote: >> >> > > > Aren't you're missing a dma_sync_single_for_device() call before >> > > > submitting the URB ? IIRC that's required for correct operation of the DMA >> > > > mapping API on some platforms, depending on the cache architecture. The >> > > > additional sync can affect performances, so it would be useful to re-run >> > > > the perf test. >> > > >> > > This was already discussed: >> > > >> > > https://lkml.org/lkml/2018/7/23/1051 >> > > >> > > I rely on Alan's reply: >> > > >> > > > According to Documentation/DMA-API-HOWTO.txt, the CPU should not write >> > > > to a DMA_FROM_DEVICE-mapped area, so dma_sync_single_for_device() is >> > > > not needed. >> > >> > I fully agree that the CPU should not write to the buffer. However, I think >> > the sync call is still needed. It's been a long time since I touched this >> > area, but IIRC, some cache architectures (VIVT ?) require both cache clean >> > before the transfer and cache invalidation after the transfer. On platforms >> > where no cache management operation is needed before the transfer in the >> > DMA_FROM_DEVICE direction, the dma_sync_*_for_device() calls should be no-ops >> > (and if they're not, it's a bug of the DMA mapping implementation). >> >> In general, I agree that the cache has to be clean before a transfer >> starts. This means some sort of mapping operation (like >> dma_sync_*_for-device) is indeed required at some point between the >> allocation and the first transfer. >> >> For subsequent transfers, however, the cache is already clean and it >> will remain clean because the CPU will not do any writes to the buffer. >> (Note: clean != empty. Rather, clean == !dirty.) Therefore transfers >> following the first should not need any dma_sync_*_for_device. >> >> If you don't accept this reasoning then you should ask the people who >> wrote DMA-API-HOWTO.txt. They certainly will know more about this >> issue than I do. > > Laurent, > > I have not seen any data corruption or glitches without > dma_sync_single_for_device() on ARM and x86. > It takes additional ~20usec for dma_sync_single_for_device() to run on > ARM. So It would be great to ensure that we can reliable save another > 15% running time. DMA-API-HOWTO.txt has and example with my_card_interrupt_handler() function where it says that "CPU should not write to DMA_FROM_DEVICE-mapped area, so dma_sync_single_for_device() is not needed here." I've found that, for instance, drivers/crypto/caam/caamrng.c follows DMA-API-HOWTO.txt and don't use dma_sync_single_for_device() in the same case as we have in pwc. > >> >> Alan Stern >> > > > -- > With best regards, > Matwey V. Kornilov -- With best regards, Matwey V. Kornilov ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer 2018-08-10 14:27 ` Alan Stern 2018-08-17 17:44 ` Matwey V. Kornilov @ 2018-09-20 13:55 ` Ezequiel Garcia 2018-09-20 14:28 ` Alan Stern 1 sibling, 1 reply; 15+ messages in thread From: Ezequiel Garcia @ 2018-09-20 13:55 UTC (permalink / raw) To: Alan Stern, Laurent Pinchart Cc: Matwey V. Kornilov, Linux Media Mailing List, open list, Tomasz Figa, Hans de Goede, Hans Verkuil, Mauro Carvalho Chehab, Steven Rostedt, mingo, Mike Isely, Bhumika Goyal, Colin King, Kieran Bingham, keiichiw Alan, Laurent: On Fri, 2018-08-10 at 10:27 -0400, Alan Stern wrote: > On Fri, 10 Aug 2018, Laurent Pinchart wrote: > > > > > Aren't you're missing a dma_sync_single_for_device() call before > > > > submitting the URB ? IIRC that's required for correct operation of the DMA > > > > mapping API on some platforms, depending on the cache architecture. The > > > > additional sync can affect performances, so it would be useful to re-run > > > > the perf test. > > > > > > This was already discussed: > > > > > > https://lkml.org/lkml/2018/7/23/1051 > > > > > > I rely on Alan's reply: > > > > > > > According to Documentation/DMA-API-HOWTO.txt, the CPU should not write > > > > to a DMA_FROM_DEVICE-mapped area, so dma_sync_single_for_device() is > > > > not needed. > > > > I fully agree that the CPU should not write to the buffer. However, I think > > the sync call is still needed. It's been a long time since I touched this > > area, but IIRC, some cache architectures (VIVT ?) require both cache clean > > before the transfer and cache invalidation after the transfer. On platforms > > where no cache management operation is needed before the transfer in the > > DMA_FROM_DEVICE direction, the dma_sync_*_for_device() calls should be no-ops > > (and if they're not, it's a bug of the DMA mapping implementation). > > In general, I agree that the cache has to be clean before a transfer > starts. This means some sort of mapping operation (like > dma_sync_*_for-device) is indeed required at some point between the > allocation and the first transfer. > > For subsequent transfers, however, the cache is already clean and it > will remain clean because the CPU will not do any writes to the buffer. > (Note: clean != empty. Rather, clean == !dirty.) Therefore transfers > following the first should not need any dma_sync_*_for_device. > > If you don't accept this reasoning then you should ask the people who > wrote DMA-API-HOWTO.txt. They certainly will know more about this > issue than I do. > Can either of you ack or nack this change? I'd like to see this merged, or either re-worked, so we can merge it. Thanks! Eze ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer 2018-09-20 13:55 ` Ezequiel Garcia @ 2018-09-20 14:28 ` Alan Stern 0 siblings, 0 replies; 15+ messages in thread From: Alan Stern @ 2018-09-20 14:28 UTC (permalink / raw) To: Ezequiel Garcia Cc: Laurent Pinchart, Matwey V. Kornilov, Linux Media Mailing List, open list, Tomasz Figa, Hans de Goede, Hans Verkuil, Mauro Carvalho Chehab, Steven Rostedt, mingo, Mike Isely, Bhumika Goyal, Colin King, Kieran Bingham, keiichiw On Thu, 20 Sep 2018, Ezequiel Garcia wrote: > Alan, Laurent: > > On Fri, 2018-08-10 at 10:27 -0400, Alan Stern wrote: > > On Fri, 10 Aug 2018, Laurent Pinchart wrote: > > > > > > > Aren't you're missing a dma_sync_single_for_device() call before > > > > > submitting the URB ? IIRC that's required for correct operation of the DMA > > > > > mapping API on some platforms, depending on the cache architecture. The > > > > > additional sync can affect performances, so it would be useful to re-run > > > > > the perf test. > > > > > > > > This was already discussed: > > > > > > > > https://lkml.org/lkml/2018/7/23/1051 > > > > > > > > I rely on Alan's reply: > > > > > > > > > According to Documentation/DMA-API-HOWTO.txt, the CPU should not write > > > > > to a DMA_FROM_DEVICE-mapped area, so dma_sync_single_for_device() is > > > > > not needed. > > > > > > I fully agree that the CPU should not write to the buffer. However, I think > > > the sync call is still needed. It's been a long time since I touched this > > > area, but IIRC, some cache architectures (VIVT ?) require both cache clean > > > before the transfer and cache invalidation after the transfer. On platforms > > > where no cache management operation is needed before the transfer in the > > > DMA_FROM_DEVICE direction, the dma_sync_*_for_device() calls should be no-ops > > > (and if they're not, it's a bug of the DMA mapping implementation). > > > > In general, I agree that the cache has to be clean before a transfer > > starts. This means some sort of mapping operation (like > > dma_sync_*_for-device) is indeed required at some point between the > > allocation and the first transfer. > > > > For subsequent transfers, however, the cache is already clean and it > > will remain clean because the CPU will not do any writes to the buffer. > > (Note: clean != empty. Rather, clean == !dirty.) Therefore transfers > > following the first should not need any dma_sync_*_for_device. > > > > If you don't accept this reasoning then you should ask the people who > > wrote DMA-API-HOWTO.txt. They certainly will know more about this > > issue than I do. > > > > Can either of you ack or nack this change? I'd like to see this merged, > or either re-worked, so we can merge it. I don't have the hardware and I'm not terribly familiar with that part of the code, so it would be better for Laurent to respond. The basic idea seems good, but I'm not looking at the details. Alan Stern ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer 2018-08-09 18:11 [PATCH v4 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov 2018-08-09 18:11 ` [PATCH v4 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() Matwey V. Kornilov 2018-08-09 18:11 ` [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov @ 2018-08-09 19:31 ` Linus Torvalds 2018-08-10 8:28 ` Matwey V. Kornilov 2 siblings, 1 reply; 15+ messages in thread From: Linus Torvalds @ 2018-08-09 19:31 UTC (permalink / raw) To: matwey Cc: linux-media, lkml, matwey.kornilov, tfiga, laurent.pinchart, stern, ezequiel, hdegoede, hverkuil, mchehab, Steven Rostedt, mingo, isely, bhumirks, Colin Ian King, kieran.bingham, keiichiw Matwey, your DKIM signature is garbage, and it causes your emails to be marked as spam when they go through a mailing list. The reason is this: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:from:to:cc:subject:date:message-id; where the problem is that the "sender" field is included in the DKIM hash calculation. That is completely incorrect, since a mailing list will - by definition - change the sender to the list, not the original sender. This is not a mailing list bug. This is a bug in your DKIM setup on the sending side. I'm leaving everybody cc'd., because this problem is starting to be annoyingly common. We had two people with chromium.org addresses with the same misconfiguration, and I want people to be aware of this. I get too much email, and too much spam, and so when people have misc-onfigured email sending that causes problems for spam systems, it needs to get fixed. Your situation *may* be the same as the Chromium guys. Quoting Doug: "Looks like it's all fixed. Both Kees and I setup our chromium.org accounts a long time ago. IIRC during that time the suggested way to do things was that you'd use your @google.com SMTP settings even when you were sending as your @chroumium.org account. These days it doesn't appear that there's even any UI in gmail to configure things that way, so presumably nobody else will be stuck in the same hole that Kees and I were in" and he may have been wrong about that "presumably nobody else will be stuck in the same hole" guess. So you might want to check what the SMTP settings are for your setup. Linus ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer 2018-08-09 19:31 ` [PATCH v4 0/2] " Linus Torvalds @ 2018-08-10 8:28 ` Matwey V. Kornilov 0 siblings, 0 replies; 15+ messages in thread From: Matwey V. Kornilov @ 2018-08-10 8:28 UTC (permalink / raw) To: Linus Torvalds Cc: Linux Media Mailing List, lkml, Tomasz Figa, Laurent Pinchart, Alan Stern, Ezequiel Garcia, Hans de Goede, Hans Verkuil, Mauro Carvalho Chehab, Steven Rostedt, mingo, Mike Isely, Bhumika Goyal, Colin Ian King, Kieran Bingham, keiichiw Hi Linus, Thanks for pointing it out, I'll try to do my best to avoid this in future. As I see now, in my case, the 'sender' in DKIM is triggered when using GMail with alien domain in "Send From:". I would not say that it is configuration "bug" (at least I can imagine why do they do that), but it definitely must be avoided for maillists. It is also interesting that if I use alien "From" email address with google-apps (g suite) domain, then GMail generated two DKIM-Signature headers, both for gmail.com and for the google-apps domain. 2018-08-09 22:31 GMT+03:00 Linus Torvalds <torvalds@linux-foundation.org>: > Matwey, > your DKIM signature is garbage, and it causes your emails to be > marked as spam when they go through a mailing list. > > The reason is this: > > DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; > d=gmail.com; s=20161025; > h=sender:from:to:cc:subject:date:message-id; > > where the problem is that the "sender" field is included in the DKIM > hash calculation. > > That is completely incorrect, since a mailing list will - by > definition - change the sender to the list, not the original sender. > > This is not a mailing list bug. This is a bug in your DKIM setup on > the sending side. > > I'm leaving everybody cc'd., because this problem is starting to be > annoyingly common. We had two people with chromium.org addresses with > the same misconfiguration, and I want people to be aware of this. > > I get too much email, and too much spam, and so when people have > misc-onfigured email sending that causes problems for spam systems, it > needs to get fixed. > > Your situation *may* be the same as the Chromium guys. Quoting Doug: > > "Looks like it's all fixed. Both Kees and I setup our chromium.org > accounts a long time ago. IIRC during that time the suggested way to > do things was that you'd use your @google.com SMTP settings even when > you were sending as your @chroumium.org account. These days it > doesn't appear that there's even any UI in gmail to configure things > that way, so presumably nobody else will be stuck in the same hole > that Kees and I were in" > > and he may have been wrong about that "presumably nobody else will be > stuck in the same hole" guess. > > So you might want to check what the SMTP settings are for your setup. > > Linus -- With best regards, Matwey V. Kornilov ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-09-20 14:28 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-09 18:11 [PATCH v4 0/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov 2018-08-09 18:11 ` [PATCH v4 1/2] media: usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler() Matwey V. Kornilov 2018-08-09 18:26 ` Steven Rostedt 2018-08-09 18:39 ` Matwey V. Kornilov 2018-08-09 18:11 ` [PATCH v4 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer Matwey V. Kornilov 2018-08-10 8:53 ` Laurent Pinchart 2018-08-10 9:38 ` Matwey V. Kornilov 2018-08-10 9:49 ` Laurent Pinchart 2018-08-10 14:27 ` Alan Stern 2018-08-17 17:44 ` Matwey V. Kornilov 2018-08-21 8:36 ` Matwey V. Kornilov 2018-09-20 13:55 ` Ezequiel Garcia 2018-09-20 14:28 ` Alan Stern 2018-08-09 19:31 ` [PATCH v4 0/2] " Linus Torvalds 2018-08-10 8:28 ` 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).