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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Ezequiel,

On Friday, 20 July 2018 02:36:40 EEST Ezequiel Garcia wrote:
> On Wed, 2018-07-18 at 15:10 +0300, Matwey V. Kornilov wrote:
> > 2018-07-17 23:10 GMT+03:00 Ezequiel Garcia <ezequiel@collabora.com>:
> >> On Mon, 2018-06-18 at 10:10 +0300, Matwey V. Kornilov wrote:
> >>> 2018-06-18 8:11 GMT+03:00 Ezequiel Garcia <ezequiel@collabora.com>:
> >>>> 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.

A generic solution would be much better though. Could we still try to achieve 
one, and only go for a hack as a last resort ? With an analysis of code flows 
and performances on x86 vs. ARM I don't think it would be too difficult to 
decide what to do.

-- 
Regards,

Laurent Pinchart