2018-07-17 23:10 GMT+03:00 Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>: > Hi Matwey, > > First of all, sorry for the delay. > > Adding Alan and Hans. Guys, do you have any feedback here? > > See below for some feedback on my side. > > On Mon, 2018-06-18 at 10:10 +0300, Matwey V. Kornilov wrote: >> Hi Ezequiel, >> >> 2018-06-18 8:11 GMT+03:00 Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>: >> > + Laurent >> > >> > On Sun, 2018-06-17 at 17:36 +0300, 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 >> > > >> > >> > This is quite interesting! I have receive similar complaints >> > from users wanting to use stk1160 on BBB and Raspberrys, >> > without much luck on either, due to insufficient isoc bandwidth. >> > >> > I'm guessing other ARM platforms could be suffering >> > from the same issue. >> > >> > Note that stk1160 and uvcvideo drivers use kmalloc on platforms >> > where DMA_NONCOHERENT is defined, but this is not the case >> > on ARM platforms. >> >> There are some ARMv7 platforms that have coherent DMA (for instance >> Broadcome Horthstar Plus series), but the most of them don't have. It >> is defined in device tree file, and there is no way to recover this >> information at runtime in USB perepherial driver. >> >> > >> > So, what is the benefit of using consistent >> > for these URBs, as opposed to streaming? >> >> I don't know, I think there is no real benefit and all we see is a >> consequence of copy-pasta when some webcam drivers were inspired by >> others and development priparily was going at x86 platforms. > > You are probably right about the copy-pasta. > >> It would >> be great if somebody corrected me here. DMA Coherence is quite strong >> property and I cannot figure out how can it help when streaming video. >> The CPU host always reads from the buffer and never writes to. >> Hardware perepherial always writes to and never reads from. Moreover, >> buffer access is mutually exclusive and separated in time by Interrupt >> fireing and URB starting (when we reuse existing URB for new request). >> Only single one memory barrier is really required here. >> > > Yeah, and not setting URB_NO_TRANSFER_DMA_MAP makes the USB core > create DMA mappings and use the streaming API. Which makes more > sense in hardware without hardware coherency. > > The only thing that bothers me with this patch is that it's not > really something specific to this driver. If this fix is valid > for pwc, then it's valid for all the drivers allocating coherent > memory. > > And also, this path won't prevent further copy-paste spread > of the coherent allocation. > > Is there any chance we can introduce a helper to allocate > isoc URBs, and then change all drivers to use it? No need > to do all of them now, but it would be good to at least have > a plan for it. Well, basically I am agree with you. However, I don't have all possible hardware to test, so I can't fix all possible drivers. Also I can not figure out how could the helper looked like. What do you think about usb_alloc() (c.f. usb_alloc_coherent()) ? > > Chances are, only a handful of platforms would care to use > coherent mappings. > >> I understand that there are cases when DMA coherence is really needed, >> for instane VirtIO VRing when we accessing same data structure in both >> directions from the both sides, but this has nothing common with our >> case. >> >> > >> > If the choice is simply platform dependent, >> > can't we somehow detect which mapping should >> > be prefered? >> >> Now, we don't have this way. >> >> > >> > > 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 5775d1f60668..6a3cd9680a7f 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); >> > > 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; >> >> >> > -- With best regards, Matwey V. Kornilov. Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia 119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
