Re: [PATCH] dma-direct: Set SG_DMA_SWIOTLB flag for dma-direct

[Catalin Marinas]

On Wed, May 08, 2024 at 01:14:41PM -0700, T.J. Mercier wrote:
> On Wed, May 8, 2024 at 10:19 AM Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
> > On Tue, May 07, 2024 at 01:07:25PM -0700, T.J. Mercier wrote:
> > > On Mon, May 6, 2024 at 10:43 PM Christoph Hellwig <hch@xxxxxx> wrote:
> > > > On Mon, May 06, 2024 at 09:39:53AM -0700, T.J. Mercier wrote:
> > > > > > You should not check, you simply must handle it by doing the proper
> > > > > > DMA API based ownership management.
> > > > >
> > > > > That doesn't really work for uncached buffers.
> > > >
> > > > What uncached buffers?
> > >
> > > For example these ones:
> > > https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/dma-buf/heaps/system_heap.c#141
> > >
> > > Vendors have their own drivers that also export uncached buffers in a
> > > similar way.
[...]
> > I think in general buffer sharing with multiple dma_map_*() calls on the
> > same buffer and DMA_ATTR_SKIP_CPU_SYNC is incompatible with bouncing,
> > irrespective of the kmalloc() minalign series. If you do this for a
> > 32-bit device and one of the pages is outside the ZONE_DMA32 range,
> > you'd get a similar behaviour.
> >
> > From the kmalloc() minumum alignment perspective, it makes sense to skip
> > the bouncing if DMA_ATTR_SKIP_CPU_SYNC is passed. We also skip the
> > bouncing if the direction is DMA_TO_DEVICE or the device is fully
> > coherent.
> >
> > A completely untested patch below. It doesn't solve other problems with
> > bouncing you may have with your out of tree patches and, as Christoph
> > said, checking in your driver whether the DMA address is a swiotlb
> > buffer is completely wrong.
> 
> This is where I must be missing something. Is the main opposition that
> the *driver* is checking for swiotlb use (instead of inside the DMA
> API)?

I see the swiotlb use as some internal detail of the DMA API
implementation that should not leak outside this framework.

> Because it sounds like we agree it's a bad idea to attempt
> bouncing + DMA_ATTR_SKIP_CPU_SYNC.

It's not necessarily the DMA_ATTR_SKIP_CPU_SYNC but rather the usage
model of sharing a buffer between multiple devices. The DMA API is
mostly tailored around CPU <-> single device ownership and the bouncing
works fine. When sharing the same buffer with multiple devices, calling
dma_map_*() on a buffer can potentially create multiple copies of the
original CPU buffer. It may be fine _if_ the devices don't communicate
between themselves using such buffer, otherwise the model is broken
(sync or no sync). The additional issue with DMA_ATTR_SKIP_CPU_SYNC is
when you use it on subsequent dma_map_*() calls assuming that the sync
was already done on the first dma_map_*() call but with bouncing it's
another dma location (ignoring the Android specific patches).

I think we should prevent bouncing if DMA_ATTR_SKIP_CPU_SYNC is passed.
However, this is not sufficient with a proper use of the DMA API since
the first dma_map_*() without this attribute can still do the bouncing.
IMHO what we need is a DMA_ATTR_NO_BOUNCE or DMA_ATTR_SHARED that will
be used on the first map and potentially on subsequent calls in
combination with DMA_ATTR_SKIP_CPU_SYNC (though we could use the latter
to imply "shared"). The downside is that mapping may fail if the
coherent mask is too narrow.

Anyway, the definitive answer should come from the DMA API maintainers.

> This code looks like it almost gets there, but it'd still reach
> swiotlb_map (instead of DMA_MAPPING_ERROR) with DMA_ATTR_SKIP_CPU_SYNC
> set for force_bounce or if the dma_capable check fails.

My quick patch was mainly to ensure the kmalloc() alignment patches do
not make the situation worse.

-- 
Catalin