Re: [PATCH drm-next v2 05/16] drm: manager to keep track of GPUs VA mappings

From: "Christian König" <christian.koenig@amd.com>
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>
Subject: Re: [PATCH drm-next v2 05/16] drm: manager to keep track of GPUs VA mappings
Date: Wed, 22 Feb 2023 16:14:25 +0100	[thread overview]
Message-ID: <29ea3705-5634-c204-c1da-d356b6dfbafc@amd.com> (raw)
In-Reply-To: <cc8eeaf4-31e7-98e4-a712-012fc604e985@redhat.com>

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.
>>
>