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

[Matwey V. Kornilov]

2018-07-20 2:36 GMT+03:00 Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>:
> 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?

It would be fine for me, if there are no objections. I know that some
maintainers don't like module parameters per se.

>
> 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
>



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382