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

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4018 bytes --]

On Thu, May 16, 2024 at 12:56:27PM +0200, Daniel Vetter wrote:
> On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote:
> > On Wed, May 15, 2024 at 6:57 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > This series is the follow-up of the discussion that John and I had a few
> > > months ago here:
> > >
> > > https://lore.kernel.org/all/CANDhNCquJn6bH3KxKf65BWiTYLVqSd9892-xtFDHHqqyrroCMQ@mail.gmail.com/
> > >
> > > The initial problem we were discussing was that I'm currently working on
> > > a platform which has a memory layout with ECC enabled. However, enabling
> > > the ECC has a number of drawbacks on that platform: lower performance,
> > > increased memory usage, etc. So for things like framebuffers, the
> > > trade-off isn't great and thus there's a memory region with ECC disabled
> > > to allocate from for such use cases.
> > >
> > > After a suggestion from John, I chose to start using heap allocations
> > > flags to allow for userspace to ask for a particular ECC setup. This is
> > > then backed by a new heap type that runs from reserved memory chunks
> > > flagged as such, and the existing DT properties to specify the ECC
> > > properties.
> > >
> > > We could also easily extend this mechanism to support more flags, or
> > > through a new ioctl to discover which flags a given heap supports.
> > 
> > Hey! Thanks for sending this along! I'm eager to see more heap related
> > work being done upstream.
> > 
> > The only thing that makes me a bit hesitant, is the introduction of
> > allocation flags (as opposed to a uniquely specified/named "ecc"
> > heap).
> > 
> > We did talk about this earlier, and my earlier press that only if the
> > ECC flag was general enough to apply to the majority of heaps then it
> > makes sense as a flag, and your patch here does apply it to all the
> > heaps. So I don't have an objection.
> > 
> > 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.

I guess it depends what we want to use the heaps for exactly. If we
create a heap by type, then the number of heaps is going to explode and
their name is going to be super weird and inconsistent.

Using the ECC setup here as an example, it means that we would need to
create system (with the default ECC setup for the system), system-ecc,
system-no-ecc, cma, cma-ecc, cma-no-ecc.

Let's say we introduce caching next. do we want to triple the number of
heaps again?

So I guess it all boils down to whether we want to consider heaps as
allocators, and then we need the flags to fine-tune the attributes/exact
semantics, or the combination of an allocator and the semantics which
will make the number of heaps explode (and reduce their general
usefulness, I guess).

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]