| Message ID | 20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | dma-buf: heaps: Support carved-out heaps and ECC related-flags | expand |
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 thing to discuss, that I didn't see in your mail: Do we have an open-source user of this new flag? thanks -john
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. > 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
Hi John, Thanks for your feedback 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. I understand your hesitation and concern :) Is there anything we could provide that would help moving the discussion forward? > Another thing to discuss, that I didn't see in your mail: Do we have > an open-source user of this new flag? Not at the moment. I guess it would be one of the things that would reduce your concerns, but is it a requirement? Thanks! Maxime
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
On Thu, May 16, 2024 at 3:56 AM Daniel Vetter <daniel@ffwll.ch> 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. thanks -john
On Thu, May 16, 2024 at 5:22 AM Maxime Ripard <mripard@kernel.org> wrote: > On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote: > > 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. > > I understand your hesitation and concern :) Is there anything we could > provide that would help moving the discussion forward? > Mostly I just want to make sure it's discussed, which is why I raise it as a concern. Getting APIs right is hard, and I know I've made my share of mistakes (for instance, a situation that very much echoes this current question: the *_ALARM clockids probably should have used a flag). So I've found highlighting the trade-offs and getting other folk's perspectives useful to avoid such issues. But I also don't intend to needlessly delay things. > > Another thing to discuss, that I didn't see in your mail: Do we have > > an open-source user of this new flag? > > Not at the moment. I guess it would be one of the things that would > reduce your concerns, but is it a requirement? So I'd defer to Sima on this. There have been a number of heap releated changes that have had to be held out of tree on this requirement, but I'm hoping recent efforts on upstream device support will eventually unblock those. thanks -john
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@ffwll.ch> 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 ... Also wondering whether we should get the symlink/allocator idea off the ground first, but given that that hasn't moved in a decade it might be too much. But then the question is, what userspace are we going to use for all these new heaps (or heaps with new flags)? Cheers, Sima
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@ffwll.ch> 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)". Right, but "not useable" is still kind of usage dependent, which reinforces the need for flags (and possibly some way to discover what the heap supports). Like, if I just want to allocate a buffer for a single writeback frame, then I probably don't have the same requirements than a compositor that needs to output a frame at 120Hz. The former probably doesn't care about the buffer attributes aside that it's accessible by the device. The latter probably can't make any kind of compromise over what kind of memory characteristics it uses. If we look into the current discussions we have, a compositor would probably need a buffer without ECC, non-secure, and probably wouldn't care about caching and being physically contiguous. Libcamera's SoftISP would probably require that the buffer is cacheable, non-secure, without ECC and might ask for physically contiguous buffers. As we add more memory types / attributes, I think being able to discover and enforce a particular set of flags will be more and more important, even more so if we tie heaps to devices, because it just gives a hint about the memory being reachable from the device, but as you said, you can still get a buffer with shit performance that won't be what you want. > 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 ... > > Also wondering whether we should get the symlink/allocator idea off the > ground first, but given that that hasn't moved in a decade it might be too > much. But then the question is, what userspace are we going to use for all > these new heaps (or heaps with new flags)? For ECC here, the compositors are the obvious target. Which loops backs into the discussion with John. Do you consider dma-buf code have the same uapi requirements as DRM? Maxime
On Wed, May 22, 2024 at 03:18:02PM +0200, Maxime Ripard 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@ffwll.ch> 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)". > > Right, but "not useable" is still kind of usage dependent, which > reinforces the need for flags (and possibly some way to discover what > the heap supports). > > Like, if I just want to allocate a buffer for a single writeback frame, > then I probably don't have the same requirements than a compositor that > needs to output a frame at 120Hz. > > The former probably doesn't care about the buffer attributes aside that > it's accessible by the device. The latter probably can't make any kind > of compromise over what kind of memory characteristics it uses. > > If we look into the current discussions we have, a compositor would > probably need a buffer without ECC, non-secure, and probably wouldn't > care about caching and being physically contiguous. > > Libcamera's SoftISP would probably require that the buffer is cacheable, > non-secure, without ECC and might ask for physically contiguous buffers. > > As we add more memory types / attributes, I think being able to discover > and enforce a particular set of flags will be more and more important, > even more so if we tie heaps to devices, because it just gives a hint > about the memory being reachable from the device, but as you said, you > can still get a buffer with shit performance that won't be what you > want. > > > 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 ... > > > > Also wondering whether we should get the symlink/allocator idea off the > > ground first, but given that that hasn't moved in a decade it might be too > > much. But then the question is, what userspace are we going to use for all > > these new heaps (or heaps with new flags)? > > For ECC here, the compositors are the obvious target. Which loops backs > into the discussion with John. Do you consider dma-buf code have the > same uapi requirements as DRM? Imo yes, otherwise we'll get really funny stuff like people bypass drm's userspace requirement for e.g. content protected buffers by just shipping the feature in a dma-buf heap ... Been there, done that. Also I think especially with interop across components there's a huge difference between a quick test program toy and the real thing. And dma-buf heaps are kinda all about cross component interop. -Sima
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@ffwll.ch> 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? 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? For example, if an applications wants to use a protected buffer, the application doesn't (and shouldn't need to) care about whether the heap for that buffer supports ECC or is backed by CMA. All it really needs to know is that it's the system's "protected" heap. This rather than try to represent every possible combination we basically make this a "configuration" issue. System designers need to settle on whatever combination of flags work for all the desired use- cases and then we expose that combination as a named heap. One problem that this doesn't solve is that we still don't have a way of retrieving these flags in drivers which may need them. Perhaps one way to address this would be to add in-kernel APIs to allocate from a heap. That way a DRM/KMS driver (for example) could find a named heap, allocate from it and implicitly store flags about the heap/buffer. Or maybe we could add in-kernel API to retrieve flags, which would be a bit better than having to expose them to userspace. Thierry
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@ffwll.ch> 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. > 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. And if we allow any platform to change a given heap name, then a generic application won't be able to support that without some kind of platform-specific configuration. > For example, if an applications wants to use a protected buffer, the > application doesn't (and shouldn't need to) care about whether the heap > for that buffer supports ECC or is backed by CMA. All it really needs to > know is that it's the system's "protected" heap. I mean... "protected" very much means backed by CMA already, it's pretty much the only thing we document, and we call it as such in Kconfig. But yeah, I agree that being backed by CMA is probably not what an application cares about (and we even have might some discussions about that), but if the ECC protection comes at a performance cost then it will very much care about it. Or if it comes with caches enabled or not. > This rather than try to represent every possible combination we > basically make this a "configuration" issue. System designers need to > settle on whatever combination of flags work for all the desired use- > cases and then we expose that combination as a named heap. This just pushes the problem down to applications, and carry the flags mentioned earlier in the heap name. So the same information, but harder to process or discover for an application. > One problem that this doesn't solve is that we still don't have a way of > retrieving these flags in drivers which may need them. I'm not sure drivers should actually need to allocate from heaps, but we could do it just like I suggested we'd do it for applications: we add a new function that allows to discover what a given heap capabilities are. And then we just have to iterate and choose the best suited for our needs. Maxime
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@ffwll.ch> 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. > > 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. 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). > And if we allow any platform to change a given heap name, then a generic > application won't be able to support that without some kind of > platform-specific configuration. We could still standardize on common use-cases so that applications would know what heaps to allocate from. But there's also no need to arbitrarily restrict this. For example there could be cases that are very specific to a particular platform and which just doesn't exist anywhere else. Platform designers could then still use this mechanism to define that very particular heap and have a very specialized userspace application use that heap for their purpose. > > For example, if an applications wants to use a protected buffer, the > > application doesn't (and shouldn't need to) care about whether the heap > > for that buffer supports ECC or is backed by CMA. All it really needs to > > know is that it's the system's "protected" heap. > > I mean... "protected" very much means backed by CMA already, it's pretty > much the only thing we document, and we call it as such in Kconfig. Well, CMA is really just an implementation detail, right? It doesn't make sense to advertise that to anything outside the kernel. Maybe it's an interesting fact that buffers allocated from these heaps will be physically contiguous? In the majority of cases that's probably not even something that matters because we get a DMA-BUF anyway and we can map that any way we want. Irrespective of that, physically contigous buffers could be allocated in any number of ways, CMA is just a convenient implementation of one such allocator. > But yeah, I agree that being backed by CMA is probably not what an > application cares about (and we even have might some discussions about > that), but if the ECC protection comes at a performance cost then it > will very much care about it. Or if it comes with caches enabled or not. True, no doubt about that. However, I'm saying there may be advantages in hiding all of this from applications. Let's say we're trying to implement video decoding. We can create a special "protected-video" heap that is specifically designed to allocate encrypted/protected scanout buffers from. When you design that system, you would most certainly not enable ECC protection on that heap because it leads to bad performance. You would also want to make sure that all of the buffers in that heap are cached and whatever other optimizations your chip may provide. Your application doesn't have to care about this, though, because it can simply look for a heap named "protected-video" and allocate buffers from it. > > This rather than try to represent every possible combination we > > basically make this a "configuration" issue. System designers need to > > settle on whatever combination of flags work for all the desired use- > > cases and then we expose that combination as a named heap. > > This just pushes the problem down to applications, and carry the flags > mentioned earlier in the heap name. So the same information, but harder > to process or discover for an application. Yes, this pushes the problem down to the application. But given the above I don't think it becomes at all hard to process. We may sacrifice some flexibility, but I'm arguing that it's flexibility that we don't need anyway. > > One problem that this doesn't solve is that we still don't have a way of > > retrieving these flags in drivers which may need them. > > I'm not sure drivers should actually need to allocate from heaps, but we > could do it just like I suggested we'd do it for applications: we add a > new function that allows to discover what a given heap capabilities are. > And then we just have to iterate and choose the best suited for our > needs. Yeah, that's an interesting option as well. I think contrary to userspace it makes more sense to work off of a set of flags at the kernel level. The obvious downside to this is that userspace now also needs driver- specific implementations for the allocation. Similar to the above it gives us a lot of flexibility at the cost of simplicity. Thierry
Hi, 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. I submitted a draft PR to the DT schema for the bindings used in this PR: https://github.com/devicetree-org/dt-schema/pull/138 Let me know what you think, Maxime Signed-off-by: Maxime Ripard <mripard@kernel.org> --- Maxime Ripard (8): dma-buf: heaps: Introduce a new heap for reserved memory of: Add helper to retrieve ECC memory bits dma-buf: heaps: Import uAPI header dma-buf: heaps: Add ECC protection flags dma-buf: heaps: system: Remove global variable dma-buf: heaps: system: Handle ECC flags dma-buf: heaps: cma: Handle ECC flags dma-buf: heaps: carveout: Handle ECC flags drivers/dma-buf/dma-heap.c | 4 + drivers/dma-buf/heaps/Kconfig | 8 + drivers/dma-buf/heaps/Makefile | 1 + drivers/dma-buf/heaps/carveout_heap.c | 330 ++++++++++++++++++++++++++++++++++ drivers/dma-buf/heaps/cma_heap.c | 10 ++ drivers/dma-buf/heaps/system_heap.c | 29 ++- include/linux/dma-heap.h | 2 + include/linux/of.h | 25 +++ include/uapi/linux/dma-heap.h | 5 +- 9 files changed, 407 insertions(+), 7 deletions(-) --- base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6 change-id: 20240515-dma-buf-ecc-heap-28a311d2c94e Best regards,