| Message ID | 20240603114008.16235-1-hailong.liu@oppo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v1] dma-buf: heaps: move the verification of heap_flags to the corresponding heap | expand |
On Mon, Jun 3, 2024 at 4:40 AM <hailong.liu@oppo.com> wrote: > > From: "Hailong.Liu" <hailong.liu@oppo.com> > > This help module use heap_flags to determine the type of dma-buf, > so that some mechanisms can be used to speed up allocation, such as > memory_pool, to optimize the allocation time of dma-buf. This feels like it's trying to introduce heap specific flags, but doesn't introduce any details about what those flags might be? This seems like it would re-allow the old opaque vendor specific heap flags that we saw in the ION days, which was problematic as different userspaces would use the same interface with potentially colliding heap flags with different meanings. Resulting in no way to properly move to an upstream solution. With the dma-heaps interface, we're trying to make sure it is well defined. One can register a number of heaps with different behaviors, and the heap name is used to differentiate the behavior. Any flags introduced will need to be well defined and behaviorally consistent between heaps. That way when an upstream solution lands, if necessary we can provide backwards compatibility via symlinks. So I don't think this is a good direction to go for dma-heaps. It would be better if you were able to clarify what flag requirements you need, so we can better understand how they might apply to other heaps, and see if it was something we would want to define as a flag (see the discussion here for similar thoughts: https://lore.kernel.org/lkml/CANDhNCoOKwtpstFE2VDcUvzdXUWkZ-Zx+fz6xrdPWTyciVXMXQ@mail.gmail.com/ ) But if your vendor heap really needs some sort of flags argument that you can't generalize, you can always implement your own dmabuf exporter driver with whatever ioctl interface you'd prefer. thanks -john
On Mon, 03. Jun 09:01, John Stultz wrote: > On Mon, Jun 3, 2024 at 4:40 AM <hailong.liu@oppo.com> wrote: > > > > From: "Hailong.Liu" <hailong.liu@oppo.com> > > > > This help module use heap_flags to determine the type of dma-buf, > > so that some mechanisms can be used to speed up allocation, such as > > memory_pool, to optimize the allocation time of dma-buf. > > This feels like it's trying to introduce heap specific flags, but > doesn't introduce any details about what those flags might be? > > This seems like it would re-allow the old opaque vendor specific heap > flags that we saw in the ION days, which was problematic as different > userspaces would use the same interface with potentially colliding > heap flags with different meanings. Resulting in no way to properly > move to an upstream solution. > > With the dma-heaps interface, we're trying to make sure it is well > defined. One can register a number of heaps with different behaviors, > and the heap name is used to differentiate the behavior. Any flags > introduced will need to be well defined and behaviorally consistent > between heaps. That way when an upstream solution lands, if necessary > we can provide backwards compatibility via symlinks. > > So I don't think this is a good direction to go for dma-heaps. > > It would be better if you were able to clarify what flag requirements > you need, so we can better understand how they might apply to other > heaps, and see if it was something we would want to define as a flag > (see the discussion here for similar thoughts: > https://lore.kernel.org/lkml/CANDhNCoOKwtpstFE2VDcUvzdXUWkZ-Zx+fz6xrdPWTyciVXMXQ@mail.gmail.com/ > ) > > But if your vendor heap really needs some sort of flags argument that > you can't generalize, you can always implement your own dmabuf > exporter driver with whatever ioctl interface you'd prefer. Thanks for your reply. Let’s continue our discussion here instead of on android-review. We aim to enhance memory allocation on each all heaps. Your pointer towards heap_flags used in /dev/ion for heap identification was helpful. We now aim to improve priority dma-buf allocation. Consider android animations scene: when device is in low memory, Allocating dma-buf as animation buffers enter direct_reclaimation, longer allocation time result in a laggy UI. But if we know the usage of the dma-buf, we can use some mechanisms to boost, e.g. animation-memory-pool. However, dma-buf usage identification becomes a challenge. A potential solution could be heap_flags. the use of heap_flags seems ugly and contrary to the intended design as you said, How aboult extending dma_heap_allocation_data as follows? struct dma_heap_allocation_data { __u64 len; __u32 fd; __u32 fd_flags; __u64 heap_flags; __u64 buf_flags: // buf usage }; Do you have any better suggestion? > > thanks > -john -- Best Regards, Hailong.
On Mon, Jun 3, 2024 at 10:21 AM Hailong Liu <hailong.liu@oppo.com> wrote: > On Mon, 03. Jun 09:01, John Stultz wrote: > > On Mon, Jun 3, 2024 at 4:40 AM <hailong.liu@oppo.com> wrote: > > > > > > From: "Hailong.Liu" <hailong.liu@oppo.com> > > > > > > This help module use heap_flags to determine the type of dma-buf, > > > so that some mechanisms can be used to speed up allocation, such as > > > memory_pool, to optimize the allocation time of dma-buf. > > > > This feels like it's trying to introduce heap specific flags, but > > doesn't introduce any details about what those flags might be? > > > > This seems like it would re-allow the old opaque vendor specific heap > > flags that we saw in the ION days, which was problematic as different > > userspaces would use the same interface with potentially colliding > > heap flags with different meanings. Resulting in no way to properly > > move to an upstream solution. > > > > With the dma-heaps interface, we're trying to make sure it is well > > defined. One can register a number of heaps with different behaviors, > > and the heap name is used to differentiate the behavior. Any flags > > introduced will need to be well defined and behaviorally consistent > > between heaps. That way when an upstream solution lands, if necessary > > we can provide backwards compatibility via symlinks. > > > > So I don't think this is a good direction to go for dma-heaps. > > > > It would be better if you were able to clarify what flag requirements > > you need, so we can better understand how they might apply to other > > heaps, and see if it was something we would want to define as a flag > > (see the discussion here for similar thoughts: > > https://lore.kernel.org/lkml/CANDhNCoOKwtpstFE2VDcUvzdXUWkZ-Zx+fz6xrdPWTyciVXMXQ@mail.gmail.com/ > > ) > > > > But if your vendor heap really needs some sort of flags argument that > > you can't generalize, you can always implement your own dmabuf > > exporter driver with whatever ioctl interface you'd prefer. > > Thanks for your reply. Let’s continue our discussion here instead > of on android-review. We aim to enhance memory allocation on each > all heaps. Your pointer towards heap_flags used in /dev/ion for heap > identification was helpful. > > We now aim to improve priority dma-buf allocation. Consider android > animations scene: > > when device is in low memory, Allocating dma-buf as animation > buffers enter direct_reclaimation, longer allocation time result in a > laggy UI. But if we know the usage of the dma-buf, we can use some > mechanisms to boost, e.g. animation-memory-pool. Can you generalize this a bit further? When would userland know to use this new flag? If it is aware, would it make sense to just use a separate heap name instead? (Also: These other mechanisms you mention should probably also be submitted upstream, however for upstream there's also the requirement that we have open users and are not just enabling proprietary blob userspace, which makes any changes to dma-buf heaps for out of tree code quite difficult) > However, dma-buf usage identification becomes a challenge. A potential > solution could be heap_flags. the use of heap_flags seems ugly and > contrary to the intended design as you said, How aboult extending > dma_heap_allocation_data as follows? > > struct dma_heap_allocation_data { > __u64 len; > __u32 fd; > __u32 fd_flags; > __u64 heap_flags; > __u64 buf_flags: // buf usage > }; This would affect the ABI (forcing a new ioctl number). And it's unclear what flags you envision as buffer specific (rather than heap specific as this patch suggested). I think we need more details about the specific problem you're seeing and trying to resolve. thanks -john
On 6/4/2024 2:06 AM, John Stultz wrote: > On Mon, Jun 3, 2024 at 10:21 AM Hailong Liu <hailong.liu@oppo.com> wrote: >> On Mon, 03. Jun 09:01, John Stultz wrote: >>> On Mon, Jun 3, 2024 at 4:40 AM <hailong.liu@oppo.com> wrote: >>>> >>>> From: "Hailong.Liu" <hailong.liu@oppo.com> >>>> >>>> This help module use heap_flags to determine the type of dma-buf, >>>> so that some mechanisms can be used to speed up allocation, such as >>>> memory_pool, to optimize the allocation time of dma-buf. >>> >>> This feels like it's trying to introduce heap specific flags, but >>> doesn't introduce any details about what those flags might be? >>> >>> This seems like it would re-allow the old opaque vendor specific heap >>> flags that we saw in the ION days, which was problematic as different >>> userspaces would use the same interface with potentially colliding >>> heap flags with different meanings. Resulting in no way to properly >>> move to an upstream solution. >>> >>> With the dma-heaps interface, we're trying to make sure it is well >>> defined. One can register a number of heaps with different behaviors, >>> and the heap name is used to differentiate the behavior. Any flags >>> introduced will need to be well defined and behaviorally consistent >>> between heaps. That way when an upstream solution lands, if necessary >>> we can provide backwards compatibility via symlinks. >>> >>> So I don't think this is a good direction to go for dma-heaps. >>> >>> It would be better if you were able to clarify what flag requirements >>> you need, so we can better understand how they might apply to other >>> heaps, and see if it was something we would want to define as a flag >>> (see the discussion here for similar thoughts: >>> https://lore.kernel.org/lkml/CANDhNCoOKwtpstFE2VDcUvzdXUWkZ-Zx+fz6xrdPWTyciVXMXQ@mail.gmail.com/ >>> ) >>> >>> But if your vendor heap really needs some sort of flags argument that >>> you can't generalize, you can always implement your own dmabuf >>> exporter driver with whatever ioctl interface you'd prefer. >> >> Thanks for your reply. Let’s continue our discussion here instead >> of on android-review. We aim to enhance memory allocation on each >> all heaps. Your pointer towards heap_flags used in /dev/ion for heap >> identification was helpful. >> >> We now aim to improve priority dma-buf allocation. Consider android >> animations scene: >> >> when device is in low memory, Allocating dma-buf as animation >> buffers enter direct_reclaimation, longer allocation time result in a >> laggy UI. But if we know the usage of the dma-buf, we can use some >> mechanisms to boost, e.g. animation-memory-pool. > > Can you generalize this a bit further? When would userland know to use > this new flag? > If it is aware, would it make sense to just use a separate heap name instead? > > (Also: These other mechanisms you mention should probably also be > submitted upstream, however for upstream there's also the requirement > that we have open users and are not just enabling proprietary blob > userspace, which makes any changes to dma-buf heaps for out of tree > code quite difficult) > >> However, dma-buf usage identification becomes a challenge. A potential >> solution could be heap_flags. the use of heap_flags seems ugly and >> contrary to the intended design as you said, How aboult extending >> dma_heap_allocation_data as follows? >> >> struct dma_heap_allocation_data { >> __u64 len; >> __u32 fd; >> __u32 fd_flags; >> __u64 heap_flags; >> __u64 buf_flags: // buf usage >> }; > > This would affect the ABI (forcing a new ioctl number). And it's > unclear what flags you envision as buffer specific (rather than heap > specific as this patch suggested). > > I think we need more details about the specific problem you're seeing > and trying to resolve. This patch mainly focuses on optimization for Android scenarios. Let’s discuss it on the issue website. Bug: 344501512 Brs, Hailong. > > thanks > -john
On Mon, Jun 3, 2024 at 11:30 PM Hailong Liu <hailong.liu@oppo.com> wrote: > On 6/4/2024 2:06 AM, John Stultz wrote: > > On Mon, Jun 3, 2024 at 10:21 AM Hailong Liu <hailong.liu@oppo.com> wrote: > >> We now aim to improve priority dma-buf allocation. Consider android > >> animations scene: > >> > >> when device is in low memory, Allocating dma-buf as animation > >> buffers enter direct_reclaimation, longer allocation time result in a > >> laggy UI. But if we know the usage of the dma-buf, we can use some > >> mechanisms to boost, e.g. animation-memory-pool. > > > > Can you generalize this a bit further? When would userland know to use > > this new flag? > > If it is aware, would it make sense to just use a separate heap name instead? > > > > (Also: These other mechanisms you mention should probably also be > > submitted upstream, however for upstream there's also the requirement > > that we have open users and are not just enabling proprietary blob > > userspace, which makes any changes to dma-buf heaps for out of tree > > code quite difficult) > > > >> However, dma-buf usage identification becomes a challenge. A potential > >> solution could be heap_flags. the use of heap_flags seems ugly and > >> contrary to the intended design as you said, How aboult extending > >> dma_heap_allocation_data as follows? > >> > >> struct dma_heap_allocation_data { > >> __u64 len; > >> __u32 fd; > >> __u32 fd_flags; > >> __u64 heap_flags; > >> __u64 buf_flags: // buf usage > >> }; > > > > This would affect the ABI (forcing a new ioctl number). And it's > > unclear what flags you envision as buffer specific (rather than heap > > specific as this patch suggested). > > > > I think we need more details about the specific problem you're seeing > > and trying to resolve. > This patch mainly focuses on optimization for Android scenarios. Let’s > discuss it on the issue website. > Bug: 344501512 Ok, we can do that if you need. But if this is ever going to go upstream (and it's more and more important that we minimize out of tree technical debt), conversations about how to generalize this will need to happen on the list. thanks -john
On 6/4/2024 11:33 PM, John Stultz wrote: > On Mon, Jun 3, 2024 at 11:30 PM Hailong Liu <hailong.liu@oppo.com> wrote: >> On 6/4/2024 2:06 AM, John Stultz wrote: >>> On Mon, Jun 3, 2024 at 10:21 AM Hailong Liu <hailong.liu@oppo.com> wrote: >>>> We now aim to improve priority dma-buf allocation. Consider android >>>> animations scene: >>>> >>>> when device is in low memory, Allocating dma-buf as animation >>>> buffers enter direct_reclaimation, longer allocation time result in a >>>> laggy UI. But if we know the usage of the dma-buf, we can use some >>>> mechanisms to boost, e.g. animation-memory-pool. >>> >>> Can you generalize this a bit further? When would userland know to use >>> this new flag? >>> If it is aware, would it make sense to just use a separate heap name instead? >>> >>> (Also: These other mechanisms you mention should probably also be >>> submitted upstream, however for upstream there's also the requirement >>> that we have open users and are not just enabling proprietary blob >>> userspace, which makes any changes to dma-buf heaps for out of tree >>> code quite difficult) >>> >>>> However, dma-buf usage identification becomes a challenge. A potential >>>> solution could be heap_flags. the use of heap_flags seems ugly and >>>> contrary to the intended design as you said, How aboult extending >>>> dma_heap_allocation_data as follows? >>>> >>>> struct dma_heap_allocation_data { >>>> __u64 len; >>>> __u32 fd; >>>> __u32 fd_flags; >>>> __u64 heap_flags; >>>> __u64 buf_flags: // buf usage >>>> }; >>> >>> This would affect the ABI (forcing a new ioctl number). And it's >>> unclear what flags you envision as buffer specific (rather than heap >>> specific as this patch suggested). >>> >>> I think we need more details about the specific problem you're seeing >>> and trying to resolve. >> This patch mainly focuses on optimization for Android scenarios. Let’s >> discuss it on the issue website. >> Bug: 344501512 > > Ok, we can do that if you need. > > But if this is ever going to go upstream (and it's more and more > important that we minimize out of tree technical debt), conversations > about how to generalize this will need to happen on the list. > We need a more complete design and test results to convince upstream to accept. Thank you again for your suggestion. Brs, Hailong. > thanks > -john
diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c index 84ae708fafe7..6c78ee6c7a58 100644 --- a/drivers/dma-buf/dma-heap.c +++ b/drivers/dma-buf/dma-heap.c @@ -105,9 +105,6 @@ static long dma_heap_ioctl_allocate(struct file *file, void *data) if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS) return -EINVAL; - if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS) - return -EINVAL; - fd = dma_heap_buffer_alloc(heap, heap_allocation->len, heap_allocation->fd_flags, heap_allocation->heap_flags); diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c index 4a63567e93ba..ae4fa974372b 100644 --- a/drivers/dma-buf/heaps/cma_heap.c +++ b/drivers/dma-buf/heaps/cma_heap.c @@ -288,6 +288,9 @@ static struct dma_buf *cma_heap_allocate(struct dma_heap *heap, int ret = -ENOMEM; pgoff_t pg; + if (heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS) + return ERR_PTR(-EINVAL); + buffer = kzalloc(sizeof(*buffer), GFP_KERNEL); if (!buffer) return ERR_PTR(-ENOMEM); diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 9076d47ed2ef..80858719a819 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -347,6 +347,9 @@ static struct dma_buf *system_heap_allocate(struct dma_heap *heap, struct page *page, *tmp_page; int i, ret = -ENOMEM; + if (heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS) + return ERR_PTR(-EINVAL); + buffer = kzalloc(sizeof(*buffer), GFP_KERNEL); if (!buffer) return ERR_PTR(-ENOMEM);