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

[Ezequiel Garcia]

On Wed, 2018-07-18 at 15:10 +0300, Matwey V. Kornilov wrote:
> 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.

Sure. And keep in mind this is more about the USB host controller,
than about this specific driver. So it's the controller what we
would have to test!

> Also I can not figure out how could the helper looked like. What do
> you think about usb_alloc() (c.f. usb_alloc_coherent()) ?
> 

I do not know that either. But it's something we can think about.

Meanwhile, it would be a shame to loose or stall this excellent
effort (which is effectively enabling a cameras on a bunch of devices).

How about you introduce a driver parameter (or device attribute),
to avoid changing the behavior for USB host controllers we don't know
about?

Something like 'alloc_coherent_urbs=y/n'. Perhaps set that
to 'yes' by default in x86, and 'no' by default in the rest?

We can think about a generic solution later.

Thanks,
Eze