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

[Christoph Hellwig <hch@infradead.org>]

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

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.

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