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