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

[Laurent Pinchart]

Hi Tomasz,

On Friday, 20 July 2018 14:33:33 EEST Tomasz Figa wrote:
> On Fri, Jul 20, 2018 at 8:23 PM Matwey V. Kornilov wrote:
> > 2018-07-20 13:55 GMT+03:00 Tomasz Figa:
> >> On Wed, Jul 18, 2018 at 5:51 AM Alan Stern wrote:
> >>> On Tue, 17 Jul 2018, Ezequiel Garcia wrote:
> >>>> Hi Matwey,
> >>>> 
> >>>> First of all, sorry for the delay.
> >>>> 
> >>>> Adding Alan and Hans. Guys, do you have any feedback here?
> >>> 
> >>> ...
> >>> 
> >>>>>> 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.
> >>> 
> >>> As far as I know, the _only_ advantage to using coherent DMA in this
> >>> situation is that you then do not have to pay the overhead of
> >>> constantly setting up and tearing down the streaming mappings.  So it
> >>> depends very much on the platform: If coherent buffers are cached then
> >>> it's a slight win and otherwise it's a big lose.
> >> 
> >> Isn't it about usb_alloc_coherent() being backed by DMA coherent API
> >> (dma_alloc_coherent/attrs()) and ending up allocating uncached (or
> >> write-combine) memory for devices with non-coherent DMAs? I'm not sure
> > 
> > Yes, this is what exactly happens at armv7l platforms.
> 
> Okay, thanks. So this seems to be exactly the same thing that is
> happening in the UVC driver. There is quite a bit of random accesses
> to extract some header fields and then a big memcpy into VB2 buffer to
> assemble final frame.
> 
> If we don't want to pay the cost of creating and destroying the
> streaming mapping, we could map (dma_map_single()) once, set
> transfer_dma of URB and URB_NO_TRANSFER_DMA_MAP and then just
> synchronize the caches (dma_sync_single()) before submitting/after
> completing the URB.

The problem is that the dma_sync_single() calls can end up being quite costly, 
depending on the platform, its cache architecture, and the buffer size. For a 
given system and use case we should always be able to decide which option is 
best, but finding that dynamically at runtime isn't an easy task. I remember 
that when developing the OMAP3 ISP driver at Nokia we had a heuristics that 
forced a full D-cache clean above a certain picture resolution as that was 
faster than selectively cleaning cache lines.

Furthermore, the DMA mapping API doesn't help us here, as it doesn't allow a 
platform to optimize operations based on the buffer mappings. An easy example 
is that there's no way for the DMA mapping implementation on ARM to find out 
in the dma_sync_single() operation that the buffer isn't mapped to the CPU and 
that the CPU cache clean can be skipped. This doesn't affect USB drivers as a 
CPU mapping always exists, but there could be some limitations there too when 
we'll try to optimize the implementation.

-- 
Regards,

Laurent Pinchart