Message-ID: <29ea3705-5634-c204-c1da-d356b6dfbafc@amd.com>
Date:   Wed, 22 Feb 2023 16:14:25 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH drm-next v2 05/16] drm: manager to keep track of GPUs VA
 mappings
Content-Language: en-US
To:     Danilo Krummrich <dakr@redhat.com>
Cc:     airlied@gmail.com, daniel@ffwll.ch, tzimmermann@suse.de,
        mripard@kernel.org, corbet@lwn.net, bskeggs@redhat.com,
        Liam.Howlett@oracle.com, matthew.brost@intel.com,
        boris.brezillon@collabora.com, alexdeucher@gmail.com,
        ogabbay@kernel.org, bagasdotme@gmail.com, willy@infradead.org,
        jason@jlekstrand.net, linux-doc@vger.kernel.org,
        nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, linux-mm@kvack.org,
        Dave Airlie <airlied@redhat.com>
References: <20230217134422.14116-1-dakr@redhat.com>
 <20230217134422.14116-6-dakr@redhat.com>
 <70ba382f-1559-289a-4922-ca9c371aaf59@amd.com>
 <cc8eeaf4-31e7-98e4-a712-012fc604e985@redhat.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <cc8eeaf4-31e7-98e4-a712-012fc604e985@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UU12bjNBT3V1aVdQS3pnblR0RkJDMFIxd2F4T3o1dElTUjJTcjREcHRFN2or?=
 =?utf-8?B?UWtRVUlXdXNWM1MwRXlSSWV0NXlNeU9jM3RjOXlTYlZrMjlHNWMvRlNxZURx?=
 =?utf-8?B?WWdET0w2cHNjdTBvU2dFMlBHTGt3NkcrZlh6QXZUYVBBalgyK2VCUlJhOUlE?=
 =?utf-8?B?bmZTUWdrVVFDSlNJSUlHb0lFNG1qOWxhNUhvL0hhZENaUTY4RTVHNHVjRnJ5?=
 =?utf-8?B?cUVVUVB0RnFpaFFFbkdFMVl4Z253dER4dkFMWGRTM2c1SU5OWGxTbXE0ejI3?=
 =?utf-8?B?WXl4Um96d25GRFdDK29JR2c3bjRyWUlaN3NuMUtvZ2drOGVzcVdsSjZVQkpO?=
 =?utf-8?B?UUJJMjg5ZVFUbWRDNG1PK3U5dVpTOC92UGJvaTgvY01oL3pDMG01RHJraTRm?=
 =?utf-8?B?d1NrYy8yMkxRa2xSeFgrMnNyM2RJWmlveVpwbkIyMkxqR21BdWNBdGtPdWxV?=
 =?utf-8?B?K2ZBYk41WDA3VjkyZGY3QW90OGUyb2VsTUNCLzVTc3Bua2dVeFZ4VW5CcmFJ?=
 =?utf-8?B?R3lGM28rQU5BdDFid2psMkZwbW9haFFKc0VTZis4Q2ZscmRDRXIzMzE2Q0hH?=
 =?utf-8?B?dUNXL3JyM0Q1SG1sZkpUbEgvK0hzdHF5S0tpWTNjRTE4bWFnOFNVbEtaREE0?=
 =?utf-8?B?SEs4V0xUTjBBb2EwaUQzOG1LUXhTbkFpM1ptMDdNUllHZWVHdlVOWXNLa0Fj?=
 =?utf-8?B?Wkd5OEY1N0JjdVZDRis3UGpiTENHbjFEN3dlQUp3b2FSUGN5aVZZdCtPL0hu?=
 =?utf-8?B?bm45UGhkS1YvRXArZFh6eEw4RENGOFQzK2p4MXJ2RDMxZEg3amFCNDkwNWZ5?=
 =?utf-8?B?eGN1UWpRR084dmx4WU94U1ppblo3bVpZVmRDUHJHUURWcHgyUVZjMFk2Zkpz?=
 =?utf-8?B?VVpUKzZScGpwQVE0K242K0I4VzJ5dnBQV3VvVG1ubFdSc2IxQmw4RVJPVU1m?=
 =?utf-8?B?c2xBQ2lSelZOVVMxM0RHd001MGlRaWd1cFVtRStLcVRCb2VLVHBNbWVNQUM0?=
 =?utf-8?B?TFd5OVdzMUVVUmQvZ1J6a2JQVDg3MVJDRHAyaVR6Ymd6ZE9iTnZUb2EwRkhO?=
 =?utf-8?B?dlQvd2hpdTRTR3RReko3N0RmTWRCZE9tTnBMdDVYNWhVVzNBVDQ5OGtad200?=
 =?utf-8?B?M2Z4VEdhV1hpZWNBa2JoeWVyWS9SMnZFSjdMS1VrSFNUdC9XUms1aFZYRDRT?=
 =?utf-8?B?eUllOERhWHpUbjFsb2l3SlRrSmdGMDNzMFZoN0szSzEzYTZ1Q0lWMHd6azZ0?=
 =?utf-8?B?M1VlK3ZFR2NGSDk0OTYwbXM5SnhDd2IrWTV0R3d0TGpXaGxydDhuZk9UNWZu?=
 =?utf-8?B?djBGSkJkQlZpUmJZa2loWHJlMkh1UnZLOE1oa3RYMEI4bUFEWXQ2bXVXbGtm?=
 =?utf-8?B?OU44UUZhNFk4VzU3YjFFaldpMDZLYXIyblQxejg1clJTaXd4cTQ5RW1NS3R0?=
 =?utf-8?B?MjJ5Uld0R0grdG9udWhYODNkR2JpWkFHMFZxZnArSXlPM3lWZWlCdDAvc2tV?=
 =?utf-8?B?MUNRQlRueXRuTVVGby9aY0tIY3UyZU5WdEhLcE5VT29LM1FxYUEzQkxaczcv?=
 =?utf-8?B?bjAwSzFCUlRieVFzc2RyOXFpY2o0TDNPVHNVSGQvR29zdS9sdnJtR1JtR0gy?=
 =?utf-8?B?MkZqM29DQ2F2Nmh5VjNvS2ZnT0JQSVAzZzZSeUh3S2RDb1E0OXVURHk2MWpZ?=
 =?utf-8?B?NkVWdGFCa1JOVSt5MVpLa01jWHBGRzA3dnZPUE9ocm5pL3lGUlpIbjlsQnln?=
 =?utf-8?B?UzNPTHo4N0o3L0R1SW5QWEkwMGdNZlVMeHFGaTlQa0I2dGxiMVZBVlhtT3N1?=
 =?utf-8?B?ajRteC9tV21wK1hndzBOcTF3NjZaMDJRemludURNMHoxd3pNVEJ5OU8wbEov?=
 =?utf-8?B?QlZTVXhKVWlyS2VBS3Y4aDZMdlpEd3QrZThnajNyS3FZSDgzcWI4OXg1L01u?=
 =?utf-8?B?bjNGYm55T1pBKzk4S1hRK0ludWgrZzNDa2luRndOa1pGSlBTRndoa3N0MVIv?=
 =?utf-8?B?YmVqWDVaMGFBSDQ4NE1CL0tLdnBBa0RGZ0g4dy9vVDM0bFk4K095NDRXMS9s?=
 =?utf-8?B?bXFPTXlET3N0Wnk5aGVTay9GOE4vN2dHSHBETHJHM3RXQy9NVGZPelgyelBU?=
 =?utf-8?B?NllDdjNIMTZjdEErOU9KYkVDSlI5bFhtSUtkSDh0TCtzYnA0emdxbnU0SDh3?=
 =?utf-8?Q?gMQbxXmrtwGLRmKz7P87SEgTr489F7dLMirW2Eqerpmt?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5d4f2792-b799-4fa6-f3c0-08db14e77fa5
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2023 15:14:31.6671
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 345VGiA10iftcGENcMj4XLr/sZVpsA815b/MkB9eRSUSnimiHMXwZSw9t3cVLEi4
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR12MB5234
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 22.02.23 um 16:07 schrieb Danilo Krummrich:
> On 2/22/23 11:25, Christian König wrote:
>> Am 17.02.23 um 14:44 schrieb Danilo Krummrich:
>
> <snip>
>
>>> +/**
>>> + * DOC: Overview
>>> + *
>>> + * The DRM GPU VA Manager, represented by struct drm_gpuva_manager 
>>> keeps track
>>> + * of a GPU's virtual address (VA) space and manages the 
>>> corresponding virtual
>>> + * mappings represented by &drm_gpuva objects. It also keeps track 
>>> of the
>>> + * mapping's backing &drm_gem_object buffers.
>>> + *
>>> + * &drm_gem_object buffers maintain a list (and a corresponding 
>>> list lock) of
>>> + * &drm_gpuva objects representing all existent GPU VA mappings 
>>> using this
>>> + * &drm_gem_object as backing buffer.
>>> + *
>>> + * If the &DRM_GPUVA_MANAGER_REGIONS feature is enabled, a GPU VA 
>>> mapping can
>>> + * only be created within a previously allocated &drm_gpuva_region, 
>>> which
>>> + * represents a reserved portion of the GPU VA space. GPU VA 
>>> mappings are not
>>> + * allowed to span over a &drm_gpuva_region's boundary.
>>> + *
>>> + * GPU VA regions can also be flagged as sparse, which allows 
>>> drivers to create
>>> + * sparse mappings for a whole GPU VA region in order to support 
>>> Vulkan
>>> + * 'Sparse Resources'.
>>
>> Well since we have now found that there is absolutely no technical 
>> reason for having those regions could we please drop them?
>
> I disagree this was the outcome of our previous discussion.
>
> In nouveau I still need them to track the separate sparse page tables 
> and, as you confirmed previously, Nvidia cards are not the only cards 
> supporting this feature.
>
> The second reason is that with regions we can avoid merging between 
> buffers, which saves some effort. However, I agree that this argument 
> by itself probably doesn't hold too much, since you've pointed out in 
> a previous mail that:
>
> <cite>
> 1) If we merge and decide to only do that inside certain boundaries 
> then those boundaries needs to be provided and checked against. This 
> burns quite some CPU cycles
>
> 2) If we just merge what we can we might have extra page table updates 
> which cost time and could result in undesired side effects.
>
> 3) If we don't merge at all we have additional housekeeping for the 
> mappings and maybe hw restrictions.
> </cite>
>
> However, if a driver uses regions to track its separate sparse page 
> tables anyway it gets 1) for free, which is a nice synergy.
>
> I totally agree that regions aren't for everyone though. Hence, I made 
> them an optional feature and by default regions are disabled. In order 
> to use them drm_gpuva_manager_init() must be called with the 
> DRM_GPUVA_MANAGER_REGIONS feature flag.
>
> I really would not want to open code regions or have two GPUVA manager 
> instances in nouveau to track sparse page tables. That would be really 
> messy, hence I hope we can agree on this to be an optional feature.

I absolutely don't think that this is a good idea then. This separate 
handling of sparse page tables is completely Nouveau specific.

Even when it's optional feature mixing this into the common handling is 
exactly what I pointed out as not properly separating between hardware 
specific and hardware agnostic functionality.

This is exactly the problem we ran into with TTM as well and I've spend 
a massive amount of time to clean that up again.

Regards,
Christian.

>
>>
>> I don't really see a need for them any more.
>>
>> Regards,
>> Christian.
>>
>