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@xxxxxxxxxx> 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@xxxxxxxxxxxxxx/ > > > > 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. > Another thing to discuss, that I didn't see in your mail: Do we have > an open-source user of this new flag? I think one option might be to just start using these internally, but not sure the dma-api would understand a fallback cadence of allocators (afaik you can specify specific cma regions already, but that doesn't really covere the case where you can fall back to pages and iommu to remap to contig dma space) ... And I don't think abandonding the dma-api for allocating cma buffers is going to be a popular proposal. -Sima -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
