Received-SPF: pass (google.com: domain of linux-kernel+bounces-200110-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Received-SPF: Pass (protection.outlook.com: domain of oppo.com designates
 58.252.5.68 as permitted sender) receiver=protection.outlook.com;
 client-ip=58.252.5.68; helo=mail.oppo.com; pr=C
Message-ID: <cff79c75-4c9c-46e0-a3ac-b9c0e8cad6f0@oppo.com>
Date: Tue, 4 Jun 2024 14:30:24 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v1] dma-buf: heaps: move the verification of
 heap_flags to the corresponding heap
To: John Stultz <jstultz@google.com>
CC: Sumit Semwal <sumit.semwal@linaro.org>, Benjamin Gaignard
	<benjamin.gaignard@collabora.com>, Brian Starkey <Brian.Starkey@arm.com>,
	"T.J. Mercier" <tjmercier@google.com>, =?UTF-8?Q?Christian_K=C3=B6nig?=
	<christian.koenig@amd.com>, <21cnbao@gmail.com>,
	<linux-media@vger.kernel.org>, <dri-devel@lists.freedesktop.org>,
	<linaro-mm-sig@lists.linaro.org>, <linux-kernel@vger.kernel.org>
References: <20240603114008.16235-1-hailong.liu@oppo.com>
 <CANDhNCq50zPB+TS+_Oo0HY0aUuBAdik2KrC8eJRTygbis293sw@mail.gmail.com>
 <20240603172148.gb7txpg2ya43jyxn@oppo.com>
 <CANDhNCrwgce7G5_-4tNfgTHcdL12zt3JKBg=o3bHrzMmfFMctg@mail.gmail.com>
Content-Language: en-US
From: Hailong Liu <hailong.liu@oppo.com>
In-Reply-To: <CANDhNCrwgce7G5_-4tNfgTHcdL12zt3JKBg=o3bHrzMmfFMctg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jun 2024 06:30:25.8596
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 2e0853c1-991d-4d2b-5aca-08dc845fd206
X-MS-Exchange-CrossTenant-Id: f1905eb1-c353-41c5-9516-62b4a54b5ee6
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=f1905eb1-c353-41c5-9516-62b4a54b5ee6;Ip=[58.252.5.68];Helo=[mail.oppo.com]
X-MS-Exchange-CrossTenant-AuthSource:
	HK3PEPF00000221.apcprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: KL1PR02MB7310

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