2018-08-09 1:46 GMT+03:00 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>: > Hi Matwey, > > Thank you for the patch. > > 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? > > 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 > > > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
