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

On 2/22/23 16:14, Christian König wrote:

Am 22.02.23 um 16:07 schrieb Danilo Krummrich:

quoted

On 2/22/23 11:25, Christian König wrote:

quoted

Am 17.02.23 um 14:44 schrieb Danilo Krummrich:

<snip>

quoted

quoted

+/**
+ * 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.

Actually, I rely on what you said in a previous mail when I say it's, 
potentially, not specific to nouveau.

<cite>
This sounds similar to what AMD hw used to have up until gfx8 (I think), 
basically sparse resources where defined through a separate mechanism to 
the address resolution of the page tables. I won't rule out that other 
hardware has similar approaches.
</cite>

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.

Optionally having regions is *not* a hardware specific concept, drivers 
might use it for a hardware specific purpose though. Which potentially 
is is the case for almost every DRM helper.

Drivers can use regions only for the sake of not merging between buffer 
boundaries as well. Some drivers might prefer this over "never merge" or 
"always merge", depending on the cost of re-organizing page tables for 
unnecessary splits/merges, without having the need of tracking separate 
sparse page tables.

Its just that I think *if* a driver needs to track separate sparse page 
tables anyways its a nice synergy since then there is no extra cost for 
getting this optimization.

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

Admittedly, I don't know what problems you are referring to. However, I 
don't see which kind of trouble it could cause by allowing drivers to 
track regions optionally.

Regards,
Christian.

quoted

quoted

I don't really see a need for them any more.

Regards,
Christian.