Re: [Linaro-mm-sig] Re: [PATCH 0/8] dma-buf: heaps: Support carved-out heaps and ECC related-flags

[Thierry Reding]

On Mon, Jul 08, 2024 at 09:14:14AM GMT, Christian König wrote:
> Am 05.07.24 um 17:35 schrieb Daniel Vetter:
> > Just figured I'll jump in on one detail here.
> > 
> > On Fri, Jul 05, 2024 at 04:31:34PM +0200, Thierry Reding wrote:
> > > On Thu, Jul 04, 2024 at 02:24:49PM GMT, Maxime Ripard wrote:
> > > > On Fri, Jun 28, 2024 at 04:42:35PM GMT, Thierry Reding wrote:
> > > > > On Fri, Jun 28, 2024 at 03:08:46PM GMT, Maxime Ripard wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Fri, Jun 28, 2024 at 01:29:17PM GMT, Thierry Reding wrote:
> > > > > > > On Tue, May 21, 2024 at 02:06:19PM GMT, Daniel Vetter wrote:
> > > > > > > > On Thu, May 16, 2024 at 09:51:35AM -0700, John Stultz wrote:
> > > > > > > > > On Thu, May 16, 2024 at 3:56 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
> > > > > > > > > > On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote:
> > > > > > > > > > > But it makes me a little nervous to add a new generic allocation flag
> > > > > > > > > > > for a feature most hardware doesn't support (yet, at least). So it's
> > > > > > > > > > > hard to weigh how common the actual usage will be across all the
> > > > > > > > > > > heaps.
> > > > > > > > > > > 
> > > > > > > > > > > I apologize as my worry is mostly born out of seeing vendors really
> > > > > > > > > > > push opaque feature flags in their old ion heaps, so in providing a
> > > > > > > > > > > flags argument, it was mostly intended as an escape hatch for
> > > > > > > > > > > obviously common attributes. So having the first be something that
> > > > > > > > > > > seems reasonable, but isn't actually that common makes me fret some.
> > > > > > > > > > > 
> > > > > > > > > > > So again, not an objection, just something for folks to stew on to
> > > > > > > > > > > make sure this is really the right approach.
> > > > > > > > > > Another good reason to go with full heap names instead of opaque flags on
> > > > > > > > > > existing heaps is that with the former we can use symlinks in sysfs to
> > > > > > > > > > specify heaps, with the latter we need a new idea. We haven't yet gotten
> > > > > > > > > > around to implement this anywhere, but it's been in the dma-buf/heap todo
> > > > > > > > > > since forever, and I like it as a design approach. So would be a good idea
> > > > > > > > > > to not toss it. With that display would have symlinks to cma-ecc and cma,
> > > > > > > > > > and rendering maybe cma-ecc, shmem, cma heaps (in priority order) for a
> > > > > > > > > > SoC where the display needs contig memory for scanout.
> > > > > > > > > So indeed that is a good point to keep in mind, but I also think it
> > > > > > > > > might re-inforce the choice of having ECC as a flag here.
> > > > > > > > > 
> > > > > > > > > Since my understanding of the sysfs symlinks to heaps idea is about
> > > > > > > > > being able to figure out a common heap from a collection of devices,
> > > > > > > > > it's really about the ability for the driver to access the type of
> > > > > > > > > memory. If ECC is just an attribute of the type of memory (as in this
> > > > > > > > > patch series), it being on or off won't necessarily affect
> > > > > > > > > compatibility of the buffer with the device.  Similarly "uncached"
> > > > > > > > > seems more of an attribute of memory type and not a type itself.
> > > > > > > > > Hardware that can access non-contiguous "system" buffers can access
> > > > > > > > > uncached system buffers.
> > > > > > > > Yeah, but in graphics there's a wide band where "shit performance" is
> > > > > > > > defacto "not useable (as intended at least)".
> > > > > > > > 
> > > > > > > > So if we limit the symlink idea to just making sure zero-copy access is
> > > > > > > > possible, then we might not actually solve the real world problem we need
> > > > > > > > to solve. And so the symlinks become somewhat useless, and we need to
> > > > > > > > somewhere encode which flags you need to use with each symlink.
> > > > > > > > 
> > > > > > > > But I also see the argument that there's a bit a combinatorial explosion
> > > > > > > > possible. So I guess the question is where we want to handle it ...
> > > > > > > Sorry for jumping into this discussion so late. But are we really
> > > > > > > concerned about this combinatorial explosion in practice? It may be
> > > > > > > theoretically possible to create any combination of these, but do we
> > > > > > > expect more than a couple of heaps to exist in any given system?
> > > > > > I don't worry too much about the number of heaps available in a given
> > > > > > system, it would indeed be fairly low.
> > > > > > 
> > > > > > My concern is about the semantics combinatorial explosion. So far, the
> > > > > > name has carried what semantics we were supposed to get from the buffer
> > > > > > we allocate from that heap.
> > > > > > 
> > > > > > The more variations and concepts we'll have, the more heap names we'll
> > > > > > need, and with confusing names since we wouldn't be able to change the
> > > > > > names of the heaps we already have.
> > > > > What I was trying to say is that none of this matters if we make these
> > > > > names opaque. If these names are contextual for the given system it
> > > > > doesn't matter what the exact capabilities are. It only matters that
> > > > > their purpose is known and that's what applications will be interested
> > > > > in.
> > > > If the names are opaque, and we don't publish what the exact
> > > > capabilities are, how can an application figure out which heap to use in
> > > > the first place?
> > > This would need to be based on conventions. The idea is to standardize
> > > on a set of names for specific, well-known use-cases.
> > > 
> > > > > > > Would it perhaps make more sense to let a platform override the heap
> > > > > > > name to make it more easily identifiable? Maybe this is a naive
> > > > > > > assumption, but aren't userspace applications and drivers not primarily
> > > > > > > interested in the "type" of heap rather than whatever specific flags
> > > > > > > have been set for it?
> > > > > > I guess it depends on what you call the type of a heap. Where we
> > > > > > allocate the memory from, sure, an application won't care about that.
> > > > > > How the buffer behaves on the other end is definitely something
> > > > > > applications are going to be interested in though.
> > > > > Most of these heaps will be very specific, I would assume.
> > > > We don't have any specific heap upstream at the moment, only generic
> > > > ones.
> > > But we're trying to add more specific ones, right?
> > > 
> > > > > For example a heap that is meant to be protected for protected video
> > > > > decoding is both going to be created in such a way as to allow that
> > > > > use-case (i.e. it doesn't make sense for it to be uncached, for
> > > > > example) and it's also not going to be useful for any other use-case
> > > > > (i.e. there's no reason to use that heap for GPU jobs or networking,
> > > > > or whatever).
> > > > Right. But also, libcamera has started to use dma-heaps to allocate
> > > > dma-capable buffers and do software processing on it before sending it
> > > > to some hardware controller.
> > > > 
> > > > Caches are critical here, and getting a non-cacheable buffer would be
> > > > a clear regression.
> > > I understand that. My point is that maybe we shouldn't try to design a
> > > complex mechanism that allows full discoverability of everything that a
> > > heap supports or is capable of. Instead if the camera has specific
> > > requirements, it could look for a heap named "camera". Or if it can
> > > share a heap with other multimedia devices, maybe call the heap
> > > "multimedia".
> > > 
> > > The idea is that heaps for these use-cases are quite specific, so you
> > > would likely not find an arbitrary number of processes try to use the
> > > same heap.
> > Yeah the idea to sort this out was to have symlinks in sysfs from the
> > device to each heap. We could then have priorities for each such link, so
> > that applications can pick the "best" heap that will work with all
> > devices. Or also special links for special use-cases, like for a
> > display+render drm device you might want to have separate links for the
> > display and the render-only use-case.
> > 
> > I think trying to encode this all into the name of a heap without linking
> > it to the device is not going to work well in general.
> > 
> > We still have that entire "make sysfs symlinks work for dma-buf heaps" on
> > our todos, and that idea is almost as old as dma-buf itself :-/
> 
> I still have the draft patches for that lying around on my harddisk
> somewhere with zero time to look into it.
> 
> If anybody wants to pick it up feel free to ping me, but be aware that you
> need to write more documentation than code.

I'm interested, so if you can dig those out that'd be a great reference.

Thanks,
Thierry

Attachment: signature.asc
Description: PGP signature