by Daniel Vetter

[permalink] [raw]

Subject: Re: [PATCH 01/12] dma-buf: add dynamic caching of sg_table

On Thu, May 23, 2019 at 1:30 PM Daniel Vetter <[email protected]> wrote:
>
> On Thu, May 23, 2019 at 1:21 PM Koenig, Christian
> <[email protected]> wrote:
> >
> > Am 22.05.19 um 20:30 schrieb Daniel Vetter:
> > > [SNIP]
> > >> Well, it seems you are making incorrect assumptions about the cache
> > >> maintenance of DMA-buf here.
> > >>
> > >> At least for all DRM devices I'm aware of mapping/unmapping an
> > >> attachment does *NOT* have any cache maintenance implications.
> > >>
> > >> E.g. the use case you describe above would certainly fail with amdgpu,
> > >> radeon, nouveau and i915 because mapping a DMA-buf doesn't stop the
> > >> exporter from reading/writing to that buffer (just the opposite actually).
> > >>
> > >> All of them assume perfectly coherent access to the underlying memory.
> > >> As far as I know there is no documented cache maintenance requirements
> > >> for DMA-buf.
> > > I think it is documented. It's just that on x86, we ignore that
> > > because the dma-api pretends there's never a need for cache flushing
> > > on x86, and that everything snoops the cpu caches. Which isn't true
> > > since over 20 ago when AGP happened. The actual rules for x86 dma-buf
> > > are very much ad-hoc (and we occasionally reapply some duct-tape when
> > > cacheline noise shows up somewhere).
> >
> > Well I strongly disagree on this. Even on x86 at least AMD GPUs are also
> > not fully coherent.
> >
> > For example you have the texture cache and the HDP read/write cache. So
> > when both amdgpu as well as i915 would write to the same buffer at the
> > same time we would get a corrupted data as well.
> >
> > The key point is that it is NOT DMA-buf in it's map/unmap call who is
> > defining the coherency, but rather the reservation object and its
> > attached dma_fence instances.
> >
> > So for example as long as a exclusive reservation object fence is still
> > not signaled I can't assume that all caches are flushed and so can't
> > start with my own operation/access to the data in question.
>
> The dma-api doesn't flush device caches, ever. It might flush some
> iommu caches or some other bus cache somewhere in-between. So it also
> won't ever make sure that multiple devices don't trample on each
> another. For that you need something else (like reservation object,
> but I think that's not really followed outside of drm much).
>
> The other bit is the coherent vs. non-coherent thing, which in the
> dma-api land just talks about whether cpu/device access need extra
> flushing or not. Now in practice that extra flushing is always only
> cpu side, i.e. will cpu writes/reads go through the cpu cache, and
> will device reads/writes snoop the cpu caches. That's (afaik at least,
> an in practice, not the abstract spec) the _only_ thing dma-api's
> cache maintenance does. For 0 copy that's all completely irrelevant,
> because as soon as you pick a mode where you need to do manual cache
> management you've screwed up, it's not 0-copy anymore really.
>
> The other hilarious stuff is that on x86 we let userspace (at least
> with i915) do that cache management, so the kernel doesn't even have a
> clue. I think what we need in dma-buf (and dma-api people will scream
> about the "abstraction leak") is some notition about whether an
> importer should snoop or not (or if that device always uses non-snoop
> or snooped transactions). But that would shred the illusion the
> dma-api tries to keep up that all that matters is whether a mapping is
> coherent from the cpu's pov or not, and you can achieve coherence both
> with a cache cpu mapping + snooped transactions, or with wc cpu side
> and non-snooped transactions. Trying to add cache managment (which
> some dma-buf exporter do indeed attempt to) will be even worse.
>
> Again, none of this is about preventing concurrent writes, or making
> sure device caches are flushed correctly around batches.

btw I just grepped for reservation_object, no one outside of
drivers/gpu is using that. So for device access synchronization
everyone else is relying on userspace ordering requests correctly on
its own. Iirc v4l/media is pondering adding dma-fence support, but
that's not going anywhere.

Also, for correctness reservations aren't needed, we allow explicit
syncing userspace to managed dma-fences/drm_syncobj on their own, and
they are allowed to get this wrong.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

2019-05-27 11:28:29

by Christian König

[permalink] [raw]

Subject: Re: [PATCH 02/12] dma-buf: add dma_buf_(un)map_attachment_locked variants v3

Thanks for the comments, but you are looking at a completely outdated
patchset.

If you are interested in the newest one please ping me and I'm going to
CC you when I send out the next version.

Christian.

Am 25.05.19 um 03:04 schrieb Hillf Danton:
> On Tue, 16 Apr 2019 20:38:31 +0200 Christian König wrote:
>> Add function variants which can be called with the reservation lock
>> already held.
>>
>> v2: reordered, add lockdep asserts, fix kerneldoc
>> v3: rebased on sgt caching
>>
>> Signed-off-by: Christian König <[email protected]>
>> ---
>> drivers/dma-buf/dma-buf.c | 63 +++++++++++++++++++++++++++++++++++++++
>> include/linux/dma-buf.h | 5 ++++
>> 2 files changed, 68 insertions(+)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index 65161a82d4d5..ef480e5fb239 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -623,6 +623,43 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>> }
>> EXPORT_SYMBOL_GPL(dma_buf_detach);
>>
>> +/**
>> + * dma_buf_map_attachment_locked - Maps the buffer into _device_ address space
>> + * with the reservation lock held. Is a wrapper for map_dma_buf() of the
> Something is missing; seems to be s/of the/of the dma_buf_ops./
>> + *
>> + * Returns the scatterlist table of the attachment;
>> + * dma_buf_ops.
> Oh it is sitting here!
>
>> + * @attach: [in] attachment whose scatterlist is to be returned
>> + * @direction: [in] direction of DMA transfer
>> + *
>> + * Returns sg_table containing the scatterlist to be returned; returns ERR_PTR
>> + * on error. May return -EINTR if it is interrupted by a signal.
>> + *
> EINTR looks impossible in the code.
>
>> + * A mapping must be unmapped by using dma_buf_unmap_attachment_locked(). Note
>> + * that the underlying backing storage is pinned for as long as a mapping
>> + * exists, therefore users/importers should not hold onto a mapping for undue
>> + * amounts of time.
>> + */
>> +struct sg_table *
>> +dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
>> + enum dma_data_direction direction)
>> +{
>> + struct sg_table *sg_table;
>> +
>> + might_sleep();
>> + reservation_object_assert_held(attach->dmabuf->resv);
>> +
>> + if (attach->sgt)
>> + return attach->sgt;
>> +
>> + sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction);
>> + if (!sg_table)
>> + sg_table = ERR_PTR(-ENOMEM);
>> +
>> + return sg_table;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_buf_map_attachment_locked);
>> +
> Best Regards
> Hillf
>
>
> _______________________________________________
> amd-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx