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

Christian König
2023-02-22T15:14:42Z

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