On 6/4/2024 2:06 AM, John Stultz wrote: > On Mon, Jun 3, 2024 at 10:21 AM Hailong Liu <hailong.liu@xxxxxxxx> wrote: >> On Mon, 03. Jun 09:01, John Stultz wrote: >>> On Mon, Jun 3, 2024 at 4:40 AM <hailong.liu@xxxxxxxx> wrote: >>>> >>>> From: "Hailong.Liu" <hailong.liu@xxxxxxxx> >>>> >>>> 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@xxxxxxxxxxxxxx/ >>> ) >>> >>> 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
