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

[Tomasz Figa]

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

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

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

Best regards,
Tomasz