Message-ID: <95cca943bbfda6af07339fb8d2dc7f4da3aa0280.camel@pengutronix.de>
Date:   Thu, 23 Jun 2022 11:33:30 +0200
From:   Lucas Stach <l.stach@...gutronix.de>
To:     Christian König <christian.koenig@....com>,
        Pekka Paalanen <ppaalanen@...il.com>
Cc:     "Sharma, Shashank" <Shashank.Sharma@....com>,
        lkml <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Nicolas Dufresne <nicolas@...fresne.ca>,
        linaro-mm-sig@...ts.linaro.org,
        Sumit Semwal <sumit.semwal@...aro.org>,
        linux-media <linux-media@...r.kernel.org>
Subject: Re: DMA-buf and uncached system memory

Am Donnerstag, dem 23.06.2022 um 11:09 +0200 schrieb Christian König:
> Am 23.06.22 um 10:58 schrieb Lucas Stach:
> > Am Donnerstag, dem 23.06.2022 um 10:14 +0200 schrieb Christian König:
> > > Am 23.06.22 um 10:04 schrieb Lucas Stach:
> > > > Am Donnerstag, dem 23.06.2022 um 09:26 +0200 schrieb Christian König:
> > > > > Am 23.06.22 um 09:13 schrieb Pekka Paalanen:
> > > > > > On Thu, 23 Jun 2022 08:59:41 +0200
> > > > > > Christian König <christian.koenig@....com> wrote:
> > > > > > 
> > > > > > > The exporter isn't doing anything wrong here. DMA-buf are supposed to be
> > > > > > > CPU cached and can also be cache hot.
> > > > > > Hi,
> > > > > > 
> > > > > > what is that statement based on?
> > > > > On the design documentation of DMA-buf and the actual driver
> > > > > implementations.
> > > > > 
> > > > > Coherency and snooping of the CPU cache is mandatory for devices and
> > > > > root complexes in the PCI specification. Incoherent access is just an
> > > > > extension.
> > > > > 
> > > > > We inherited that by basing DMA-buf on the Linux kernel DMA-API which in
> > > > > turn is largely based on the PCI specification.
> > > > > 
> > > > > > Were the (mandatory for CPU access) cpu_access_begin/end functions &
> > > > > > ioctls not supposed to ensure that CPU cache is up-to-date / CPU cache
> > > > > > is fully flushed out?
> > > > > No, those functions are to inform the exporter that the importer has
> > > > > started and finished accessing the buffer using the CPU.
> > > > > 
> > > > > There is no signaling in the other direction. In other words the
> > > > > exporter doesn't inform the importer about CPU accesses because it is
> > > > > the owner of the buffer.
> > > > > 
> > > > > It's the responsibility of the importer to make sure that it can
> > > > > actually access the data in the buffer. If it can't guarantee that the
> > > > > importer shouldn't import the buffer in the first place.
> > > > This is not really correct. DMA-buf inherited the the map/unmap part
> > > > from the DMA API, which on cache coherent architecture is mostly a no-
> > > > op or ties into the IOMMU implementation to set up the pagetables for
> > > > the translation. On non cache coherent architectures this is the point
> > > > where any any necessary cache maintenance happens. DRM breaks this
> > > > model by caching the DMA-buf mapping for performance reasons.
> > > That's not only because of performance reasons, but also because of
> > > correctness.
> > > 
> > > At least the Vulkan API and a bunch of OpenGL extensions make it
> > > mandatory for the buffer to be cache coherent. The kernel is simply not
> > > informed about domain transfers.
> > > 
> > > For example you can just do a CPU copy to a ring buffer and the
> > > expectation is that an already running shader sees that.
> > Yes, that one is not really an issue as you know that at buffer
> > creation time and can make sure to map those buffers uncached on non
> > coherent arches. If there are no explicit domain transfer points non
> > coherent must bite the bullet and bypass the CPU caches, running
> > performance into the ground.
> 
> Yes, exactly that was what this mail thread was about. But this case is 
> currently not supported by DMA-buf.
> 
> In other words, cache coherency is currently mandatory for everybody 
> involved.
> 
> > > > In the DMA API keeping things mapped is also a valid use-case, but then
> > > > you need to do explicit domain transfers via the dma_sync_* family,
> > > > which DMA-buf has not inherited. Again those sync are no-ops on cache
> > > > coherent architectures, but do any necessary cache maintenance on non
> > > > coherent arches.
> > > Correct, yes. Coherency is mandatory for DMA-buf, you can't use
> > > dma_sync_* on it when you are the importer.
> > > 
> > > The exporter could of course make use of that because he is the owner of
> > > the buffer.
> > In the example given here with UVC video, you don't know that the
> > buffer will be exported and needs to be coherent without
> > synchronization points, due to the mapping cache at the DRM side. So
> > V4L2 naturally allocates the buffers from CPU cached memory. If the
> > expectation is that those buffers are device coherent without relying
> > on the map/unmap_attachment calls, then V4L2 needs to always
> > synchronize caches on DQBUF when the  buffer is allocated from CPU
> > cached memory and a single DMA-buf attachment exists. And while writing
> > this I realize that this is probably exactly what V4L2 should do...
> 
> No, the expectation is that the importer can deal with whatever the 
> exporter provides.
> 
> If the importer can't access the DMA-buf coherently it's his job to 
> handle that gracefully.

How does the importer know that the memory behind the DMA-buf is in CPU
cached memory?

If you now tell me that an importer always needs to assume this and
reject the import if it can't do snooping, then any DMA-buf usage on
most ARM SoCs is currently invalid usage. On most of the multimedia
targeted ARM SoCs being unable to snoop the cache is the norm, not an
exception.

> 
> See for example on AMD/Intel hardware most of the engines can perfectly 
> deal with cache coherent memory accesses. Only the display engines can't.
> 
> So on import time we can't even say if the access can be coherent and 
> snoop the CPU cache or not because we don't know how the imported 
> DMA-buf will be used later on.
> 
So for those mixed use cases, wouldn't it help to have something
similar to the dma_sync in the DMA-buf API, so your scanout usage can
tell the exporter that it's going to do non-snoop access and any dirty
cache lines must be cleaned? Signaling this to the exporter would allow
to skip the cache maintenance if the buffer is in CPU uncached memory,
which again is a default case for the ARM SoC world.

Regards,
Lucas

Powered by blists - more mailing lists