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

[Tomasz Figa]

On Thu, Dec 13, 2018 at 11:03 PM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>
> On Thu, Dec 13, 2018 at 12:13:29PM +0900, Tomasz Figa wrote:
> > Putting aside the problem of memory without struct page, one thing to
> > note here that what is a contiguous DMA range for device X, may not be
> > mappable contiguously for device Y and it would still need something
> > like a scatter list to fully describe the buffer.
>
> I think we need to define contiguous here.
>
> If the buffer always is physically contiguous, as it is in the currently
> posted series, we can always map it with a single dma_map_single call
> (if the hardware can handle that in a single segment is a different
> question, but out of scope here).

Are you sure the buffer is always physically contiguous? At least the
ARM IOMMU dma_ops [1] and the DMA-IOMMU dma_ops [2] will simply
allocate pages without any continuity guarantees and remap the pages
into a contiguous kernel VA (unless DMA_ATTR_NO_KERNEL_MAPPING is
given, which makes them return an opaque cookie instead of the kernel
VA).

[1] http://git.infradead.org/users/hch/misc.git/blob/2dbb028e4a3017e1b71a6ae3828a3548545eba24:/arch/arm/mm/dma-mapping.c#l1291
[2] http://git.infradead.org/users/hch/misc.git/blob/2dbb028e4a3017e1b71a6ae3828a3548545eba24:/drivers/iommu/dma-iommu.c#l450

>
> If on the other hand we have multiple discontiguous physical address
> range that are mapped using the iommu and vmap this interface doesn't
> work anyway.
>
> But in that case you should just do multiple allocations and then use
> dma_map_sg coalescing on the hardware side, and vmap [1] if really
> needed.  I guess for this we want to gurantee that dma_alloc_attrs
> with the DMA_ATTR_NON_CONSISTENT allows virt_to_page to be used on
> the return value, which the currently posted implementation does,
> although I'm a it reluctant about the API guarantee.
>
>
> > Do we already have a structure that would work for this purposes? I'd
> > assume that we need something like the existing scatterlist but with
> > page links replaced with something that doesn't require the memory to
> > have struct page, possibly just PFN?
>
> The problem is that just the PFN / physical address isn't enough for
> most use cases as you also need a kernel virtual address.  But moving
> struct scatterlist to store a pfn instead of a struct page would be
> pretty nice for various reasons anyway.
>
> >
> > >
> > > It would also be great to use that opportunity to get rid of all the
> > > code duplication of almost the same dmabug provides backed by the
> > > DMA API.
> >
> > Could you sched some more light on this? I'm curious what is the code
> > duplication you're referring to.
>
> It seems like a lot of the dmabuf ops are just slight various of
> dma_alloc + dma_get_sttable + dma_map_sg / dma_unmap_sg.  There must be
> a void to not duplicate that over and over.

Device/kernel/userspace maps/unmaps shouldn't really be
exporter-specific indeed, as long as one provides a uniform way of
describing a buffer and then have dma_map_*() work on that. Possibly a
part that manages the CPU cache maintenance either. There is still
some space for some special device caches (or other synchronization),
though.

>
> [1] and use invalidate_kernel_vmap_range and flush_kernel_vmap_range
>     to manually take care of cache flushing.