Nouveau - Sep 2023 - [PATCH drm-misc-next v3 5/7] drm/gpuvm: add an abstraction for a VM / BO combination

Hi, Danilo

On 9/9/23 17:31, Danilo Krummrich wrote:
> This patch adds an abstraction layer between the drm_gpuva mappings of
> a particular drm_gem_object and this GEM object itself. The abstraction
> represents a combination of a drm_gem_object and drm_gpuvm. The
> drm_gem_object holds a list of drm_gpuvm_bo structures (the structure
> representing this abstraction), while each drm_gpuvm_bo contains list of
> mappings of this GEM object.
>
> This has multiple advantages:
>
> 1) We can use the drm_gpuvm_bo structure to attach it to various lists
>     of the drm_gpuvm. This is useful for tracking external and evicted
>     objects per VM, which is introduced in subsequent patches.
>
> 2) Finding mappings of a certain drm_gem_object mapped in a certain
>     drm_gpuvm becomes much cheaper.
>
> 3) Drivers can derive and extend the structure to easily represent
>     driver specific states of a BO for a certain GPUVM.
>
> The idea of this abstraction was taken from amdgpu, hence the credit for
> this idea goes to the developers of amdgpu.
>
> Cc: Christian K?nig <christian.koenig at amd.com>
> Signed-off-by: Danilo Krummrich <dakr at redhat.com>
Did you consider having the drivers embed the struct drm_gpuvm_bo in 
their own bo definition? I figure that would mean using the gem bo's 
refcounting and providing a helper to call from the driver's bo release. 
Looks like that could potentially save a lot of code? Or is there 
something that won't work with that approach?

Thanks,

Thomas

Hi Thomas,

On 9/11/23 19:19, Thomas Hellstr?m wrote:
> Hi, Danilo
> 
> On 9/9/23 17:31, Danilo Krummrich wrote:
>> This patch adds an abstraction layer between the drm_gpuva mappings of
>> a particular drm_gem_object and this GEM object itself. The abstraction
>> represents a combination of a drm_gem_object and drm_gpuvm. The
>> drm_gem_object holds a list of drm_gpuvm_bo structures (the structure
>> representing this abstraction), while each drm_gpuvm_bo contains list
of
>> mappings of this GEM object.
>>
>> This has multiple advantages:
>>
>> 1) We can use the drm_gpuvm_bo structure to attach it to various lists
>> ??? of the drm_gpuvm. This is useful for tracking external and evicted
>> ??? objects per VM, which is introduced in subsequent patches.
>>
>> 2) Finding mappings of a certain drm_gem_object mapped in a certain
>> ??? drm_gpuvm becomes much cheaper.
>>
>> 3) Drivers can derive and extend the structure to easily represent
>> ??? driver specific states of a BO for a certain GPUVM.
>>
>> The idea of this abstraction was taken from amdgpu, hence the credit
for
>> this idea goes to the developers of amdgpu.
>>
>> Cc: Christian K?nig <christian.koenig at amd.com>
>> Signed-off-by: Danilo Krummrich <dakr at redhat.com>
> 
> Did you consider having the drivers embed the struct drm_gpuvm_bo in their
own bo definition? I figure that would mean using the gem bo's refcounting
and providing a helper to call from the driver's bo release. Looks like that
could potentially save a lot of code? Or is there something that won't work
with that approach?
There are drm_gpuvm_ops::vm_bo_alloc and drm_gpuvm_ops::vm_bo_free callback for
drivers to register for that purpose.

- Danilo
> 
> Thanks,
> 
> Thomas
> 
>