From: Daniel Vetter <daniel@ffwll.ch>
To: "Christian König" <christian.koenig@amd.com>
Cc: "Paul Cercueil" <paul@crapouillou.net>,
"Christian König" <ckoenig.leichtzumerken@gmail.com>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Jonathan Corbet" <corbet@lwn.net>,
"Sumit Semwal" <sumit.semwal@linaro.org>,
"Jonathan Cameron" <jic23@kernel.org>,
"Nuno Sá" <noname.nuno@gmail.com>,
"Michael Hennerich" <Michael.Hennerich@analog.com>,
linux-usb@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
"Christoph Hellwig" <hch@lst.de>
Subject: Re: [Linaro-mm-sig] Re: [PATCH v5 1/6] dma-buf: Add dma_buf_{begin,end}_access()
Date: Wed, 31 Jan 2024 10:07:22 +0100 [thread overview]
Message-ID: <ZboNyju8h4vfSd7v@phenom.ffwll.local> (raw)
In-Reply-To: <1d912523-b980-4386-82b2-8d79808398c1@amd.com>
On Tue, Jan 30, 2024 at 02:09:45PM +0100, Christian König wrote:
> Am 30.01.24 um 11:40 schrieb Daniel Vetter:
> > On Tue, Jan 30, 2024 at 10:48:23AM +0100, Paul Cercueil wrote:
> > > Le mardi 30 janvier 2024 à 10:23 +0100, Christian König a écrit :
> > > > I would say we start with the DMA-API by getting away from sg_tables
> > > > to something cleaner and state oriented.
> > > FYI I am already adding a 'dma_vec' object in my IIO DMABUF patchset,
> > > which is just a dead simple
> > >
> > > struct dma_vec {
> > > dma_addr_t addr;
> > > size_t len;
> > > };
> > >
> > > (The rationale for introducing it in the IIO DMABUF patchset was that
> > > the "scatterlist" wouldn't allow me to change the transfer size.)
> > >
> > > So I believe a new "sg_table"-like could just be an array of struct
> > > dma_vec + flags.
> > Yeah that's pretty much the proposal I've seen, split the sg table into
> > input data (struct page + len) and output data (which is the dma_addr_t +
> > len you have above).
>
> I would extend that a bit and say we have an array with
> dma_addr+power_of_two_order and a header structure with lower bit offset and
> some DMA transaction flags.
>
> But this is something which can be worked as an optimization later on. For a
> start this proposal here looks good to me as well.
>
> > The part I don't expect to ever happen, because it hasn't the past 20 or
> > so years, is that the dma-api will give us information about what is
> > needed to keep the buffers coherency between various devices and the cpu.
>
> Well maybe that's what we are doing wrong.
>
> Instead of asking the dma-api about the necessary information we should give
> the API the opportunity to work for us.
>
> In other words we don't need the information about buffer coherency what we
> need is that the API works for as and fulfills the requirements we have.
>
> So the question is really what should we propose to change on the DMA-api
> side to get this working as expected?
So one thing I've been pondering, kinda picking up your point about CXL,
is that we do make the coherency protocol more explicit by adding a
coherency mode to dma_buf that the exporter sets. Some ideas for values
this could have:
- ATTOMIC_COHERENT: Fully cache coherent, including device/cpu atomis.
This would be for CXL. Non-CXL devices could still participate with the
old model using explicit devices flushes, but must at comply with
CPU_COHERENT.
There's also the power9-only nvlink that would fit here, but I guess
going forward CXL (and cache-coherent integrated gpu) would really be
the only users of this flag.
Peer2peer would have the same rules, otherwise doesn't really make
sense. Also we might want to forbib non-CXL imports for these buffers
maybe even? Not sure on that.
- CPU_COHERENT: device transactions do snoop cpu devices caches, but
devices might do their own caching which isn't snooped by the cpu and
needs explicit device-side invalidate/flushing. This means pcie
importers are not allowed to use pcie no-snoop transactions, intel igpu
wouldn't be allowed to use MOCS that do the same, similar for arm
integrated devices.
Importers can skip all explicit cache management apis like
dma_buf_begin/end_cpu_access, or the newly proposed
dma_buf_begin/end_device_access here.
We'd need to figure out what exactly this means for peer2peer
transactions, I have no idea whether the no-snoop flag even does
anything for those.
We might also want to split up CPU_COHERENT into CPU_COHERENT_WB and
CPU_WOHERENT_WC, so that importers know whether cpu reads are going to
be crawling or not.
- MEMORY_COHERENT: devices transactions do not snoop any caches, but
promise that all transactions are fully flushed to system memory. Any
devices transactions which do fill cpu caches must call the proposed
dma_buf_begin/end_device_access functions proposed here. Any cpu access
must be braketed by calls to dma_buf_begin/end_cpu_access.
If your device does fill cpu caches, then essentially you'd not be able
to import such buffers. Not sure whether we need to put the
responsibility of checking that onto importers or exporters. Ideally
core dma-buf.c code would check this.
Also maybe the cpu WC mapping mode would actually need to be a sub-mode
for MEMORY_COHERENT, because all cpu wc achieves is to avoid the need to
call dma_buf_begin/end_cpu_access, you would still need your devices to
be memory coherent. And if they're not, then you cannot use that
dma-buf.
Or maybe alternatively we need to guarantee that exporters which set
MEMORY_COHERENT implement dma_buf_begin/end_device_access to make things
work for these cpu-coherent but not memory-coherent devices. This
becomes very tricky with device/arch/bus specific details I think.
- DMA_API_COHERENT: The memory is allocated or mapped by the dma-api, and
the exact coherency mode is not know. Importers _must_ braket all cpu
and device access with the respective dma_buf functions. This is
essentially the "we have no idea" default.
Note that exporters might export memory allocated with dma_map_alloc
with MEMORY_COHERENT or CPU_COHERENT if they know how the memory exactly
works. E.g. for most arm soc gpu/display drivers we can assume that the
dma-api gives us MEMORY_COHERENT or CPU_COHERENT_WC, and just use that.
Essentially this would make the current implicit assumptions explicit.
udmabuf would need to set this, definitely if Paul's patches to add the
explicit device flushes land.
- DEFAULT_COHERENT: This would be the backwards compat legacy yolo
behvaior. I'm not sure whether we should alias that with
DMA_API_COHERENT or leave it as a special value to mark exporters which
haven't been updated for the much more explicit coherency handling yet.
The specification for this coherency mode would be a flat out "who
knows, just don't break existing use-cases with actual users".
Essentially the only reason we'd have this would be to make sure we can
avoid regressions of these existing use-cases, by keeping whatever
horrible heuristics we have in current exporters.
It would also allow us to convert exporters and importers on a case by
case basis.
Note that all these coherency modes are defined in terms of bus-sepecific
device access and in terms of dma_buf apis the importer must call or can
skip. This way we'd avoid having to change the dma-api in a first step,
and if this all works out properly we could then use the resulting dma-api
as a baseline to propose dma-api extensions.
I think starting right out with designing dma-api extension is a few
bridges too far. Both from a "how do we convince upstream" pov, but maybe
even more from a "how do we figure out what we even need" pov.
> Regards,
> Christian.
>
>
>
>
>
> > -Sima
>
> _______________________________________________
> Linaro-mm-sig mailing list -- linaro-mm-sig@lists.linaro.org
> To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
next prev parent reply other threads:[~2024-01-31 9:07 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-19 14:13 [PATCH v5 0/6] usb: gadget: functionfs: DMABUF import interface Paul Cercueil
2024-01-19 14:13 ` [PATCH v5 1/6] dma-buf: Add dma_buf_{begin,end}_access() Paul Cercueil
2024-01-20 20:20 ` kernel test robot
2024-01-22 10:35 ` [Linaro-mm-sig] " Christian König
2024-01-22 11:01 ` Paul Cercueil
2024-01-22 13:41 ` Christian König
2024-01-23 10:10 ` Paul Cercueil
2024-01-23 11:52 ` Christian König
2024-01-23 13:02 ` Paul Cercueil
[not found] ` <577501f9-9d1c-4f8d-9882-7c71090e5ef3@amd.com>
2024-01-24 10:58 ` Paul Cercueil
2024-01-24 15:38 ` Andrew Davis
2024-01-24 15:52 ` Paul Cercueil
[not found] ` <2ac7562c-d221-409a-bfee-1b3cfcc0f1c6@amd.com>
2024-01-25 18:01 ` Daniel Vetter
2024-01-26 16:42 ` Christian König
2024-01-30 9:01 ` [Linaro-mm-sig] " Daniel Vetter
[not found] ` <a2346244-e22b-4ff6-b6cd-1da7138725ae@amd.com>
2024-01-30 9:48 ` Paul Cercueil
2024-01-30 10:40 ` Daniel Vetter
2024-01-30 13:09 ` Christian König
2024-01-31 9:07 ` Daniel Vetter [this message]
2024-02-06 13:28 ` Christian König
2024-02-06 13:57 ` Daniel Vetter
2024-01-30 10:38 ` Daniel Vetter
2024-01-25 18:10 ` Daniel Vetter
2024-01-19 14:13 ` [PATCH v5 2/6] dma-buf: udmabuf: Implement .{begin,end}_access Paul Cercueil
2024-02-07 17:10 ` [Linaro-mm-sig] " Daniel Vetter
2024-01-19 14:13 ` [PATCH v5 3/6] usb: gadget: Support already-mapped DMA SGs Paul Cercueil
2024-01-19 14:14 ` [PATCH v5 4/6] usb: gadget: functionfs: Factorize wait-for-endpoint code Paul Cercueil
2024-01-19 14:14 ` [PATCH v5 5/6] usb: gadget: functionfs: Add DMABUF import interface Paul Cercueil
2024-01-19 14:14 ` [PATCH v5 6/6] Documentation: usb: Document FunctionFS DMABUF API Paul Cercueil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZboNyju8h4vfSd7v@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=Michael.Hennerich@analog.com \
--cc=christian.koenig@amd.com \
--cc=ckoenig.leichtzumerken@gmail.com \
--cc=corbet@lwn.net \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=hch@lst.de \
--cc=jic23@kernel.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=noname.nuno@gmail.com \
--cc=paul@crapouillou.net \
--cc=sumit.semwal@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).