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

[Thomas Hellström]

Hi, Danilo

On 9/11/23 19:49, Danilo Krummrich wrote:
> 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@xxxxxxx>
> > > Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> >
> > 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

Now after looking a bit deeper, I think actually the question could be 
rephrased as, why don't we just use the
struct drm_gem_object::gpuva struct as the drm_gpuvm_bo in the spirit of 
keeping things simple? Drivers would then just embed it in their bo 
subclass and we'd avoid unnecessary fields in the struct drm_gem_object 
for drivers that don't do VM_BIND yet.

Sure, this won't be per bo and per vm, but it'd really only make a 
slight difference where we have multiple VMAs per bo, where per-vm 
per-bo state either needs to be duplicated or attached to a single vma 
(as in the case of the external bo list).

To me that looks like a substantial amount of less code / complexity?

/Thomas


> > Thanks,
> >
> > Thomas