Christian König
2023-Jun-22 13:54 UTC
[Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
Am 20.06.23 um 14:23 schrieb Danilo Krummrich:> Hi Christian, > > On 6/20/23 08:45, Christian K?nig wrote: >> Hi Danilo, >> >> sorry for the delayed reply. I've trying to dig myself out of a hole >> at the moment. > > No worries, thank you for taking a look anyway! > >> >> Am 20.06.23 um 02:42 schrieb Danilo Krummrich: >>> [SNIP] >>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h >>> index bbc721870c13..5ec8148a30ee 100644 >>> --- a/include/drm/drm_gem.h >>> +++ b/include/drm/drm_gem.h >>> @@ -36,6 +36,8 @@ >>> ? #include <linux/kref.h> >>> ? #include <linux/dma-resv.h> >>> +#include <linux/list.h> >>> +#include <linux/mutex.h> >>> ? #include <drm/drm_vma_manager.h> >>> @@ -379,6 +381,18 @@ struct drm_gem_object { >>> ?????? */ >>> ????? struct dma_resv _resv; >>> +??? /** >>> +???? * @gpuva: >>> +???? * >>> +???? * Provides the list of GPU VAs attached to this GEM object. >>> +???? * >>> +???? * Drivers should lock list accesses with the GEMs &dma_resv lock >>> +???? * (&drm_gem_object.resv). >>> +???? */ >>> +??? struct { >>> +??????? struct list_head list; >>> +??? } gpuva; >>> + >>> ????? /** >>> ?????? * @funcs: >>> ?????? * >> >> I'm pretty sure that it's not a good idea to attach this directly to >> the GEM object. > > Why do you think so? IMHO having a common way to connect mappings to > their backing buffers is a good thing, since every driver needs this > connection anyway. > > E.g. when a BO gets evicted, drivers can just iterate the list of > mappings and, as the circumstances require, invalidate the > corresponding mappings or to unmap all existing mappings of a given > buffer. > > What would be the advantage to let every driver implement a driver > specific way of keeping this connection?Flexibility. For example on amdgpu the mappings of a BO are groups by VM address spaces. E.g. the BO points to multiple bo_vm structures which in turn have lists of their mappings. Additional to that there is a state maschine associated with the mappings, e.g. if the page tables are up to date or need to be updated etc....> Do you see cases where this kind of connection between mappings and > backing buffers wouldn't be good enough? If so, which cases do you > have in mind? Maybe we can cover them in a common way as well?Yeah, we have tons of cases like that. But I have no idea how to generalize them.> >> >> As you wrote in the commit message it's highly driver specific what >> to map and where to map it. > > In the end the common case should be that in a VA space at least every > mapping being backed by a BO is represented by a struct drm_gpuva.Oh, no! We also have mappings not backed by a BO at all! For example for partial resident textures or data routing to internal hw etc... You can't be sure that a mapping is backed by a BO at all.> >> >> Instead I suggest to have a separate structure for mappings in a VA >> space which driver can then add to their GEM objects or whatever they >> want to map into their VMs. > > Which kind of separate structure for mappings? Another one analogous > to struct drm_gpuva?Well similar to what amdgpu uses BO -> one structure for each combination of BO and VM -> mappings inside that VM Regards, Christian.> > - Danilo > >> >> Regards, >> Christian. >> >> >
Danilo Krummrich
2023-Jun-22 14:22 UTC
[Nouveau] [PATCH drm-next v5 03/14] drm: manager to keep track of GPUs VA mappings
On 6/22/23 15:54, Christian K?nig wrote:> Am 20.06.23 um 14:23 schrieb Danilo Krummrich: >> Hi Christian, >> >> On 6/20/23 08:45, Christian K?nig wrote: >>> Hi Danilo, >>> >>> sorry for the delayed reply. I've trying to dig myself out of a hole >>> at the moment. >> >> No worries, thank you for taking a look anyway! >> >>> >>> Am 20.06.23 um 02:42 schrieb Danilo Krummrich: >>>> [SNIP] >>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h >>>> index bbc721870c13..5ec8148a30ee 100644 >>>> --- a/include/drm/drm_gem.h >>>> +++ b/include/drm/drm_gem.h >>>> @@ -36,6 +36,8 @@ >>>> ? #include <linux/kref.h> >>>> ? #include <linux/dma-resv.h> >>>> +#include <linux/list.h> >>>> +#include <linux/mutex.h> >>>> ? #include <drm/drm_vma_manager.h> >>>> @@ -379,6 +381,18 @@ struct drm_gem_object { >>>> ?????? */ >>>> ????? struct dma_resv _resv; >>>> +??? /** >>>> +???? * @gpuva: >>>> +???? * >>>> +???? * Provides the list of GPU VAs attached to this GEM object. >>>> +???? * >>>> +???? * Drivers should lock list accesses with the GEMs &dma_resv lock >>>> +???? * (&drm_gem_object.resv). >>>> +???? */ >>>> +??? struct { >>>> +??????? struct list_head list; >>>> +??? } gpuva; >>>> + >>>> ????? /** >>>> ?????? * @funcs: >>>> ?????? * >>> >>> I'm pretty sure that it's not a good idea to attach this directly to >>> the GEM object. >> >> Why do you think so? IMHO having a common way to connect mappings to >> their backing buffers is a good thing, since every driver needs this >> connection anyway. >> >> E.g. when a BO gets evicted, drivers can just iterate the list of >> mappings and, as the circumstances require, invalidate the >> corresponding mappings or to unmap all existing mappings of a given >> buffer. >> >> What would be the advantage to let every driver implement a driver >> specific way of keeping this connection? > > Flexibility. For example on amdgpu the mappings of a BO are groups by VM > address spaces. > > E.g. the BO points to multiple bo_vm structures which in turn have lists > of their mappings.Isn't this (almost) the same relationship I introduce with the GPUVA manager? If you would switch over to the GPUVA manager right now, it would be that every GEM has a list of it's mappings (the gpuva list). The mapping is represented by struct drm_gpuva (of course embedded in driver specific structure(s)) which has a pointer to the VM address space it is part of, namely the GPUVA manager instance. And the GPUVA manager keeps a maple tree of it's mappings as well. If you still would like to *directly* (indirectly you already have that relationship) keep a list of GPUVA managers (VM address spaces) per GEM, you could still do that in a driver specific way. Do I miss something?> > Additional to that there is a state maschine associated with the > mappings, e.g. if the page tables are up to date or need to be updated > etc.... > >> Do you see cases where this kind of connection between mappings and >> backing buffers wouldn't be good enough? If so, which cases do you >> have in mind? Maybe we can cover them in a common way as well? > > Yeah, we have tons of cases like that. But I have no idea how to > generalize them.They could still remain to be driver specific then, right?> >> >>> >>> As you wrote in the commit message it's highly driver specific what >>> to map and where to map it. >> >> In the end the common case should be that in a VA space at least every >> mapping being backed by a BO is represented by a struct drm_gpuva. > > Oh, no! We also have mappings not backed by a BO at all! For example for > partial resident textures or data routing to internal hw etc... > > You can't be sure that a mapping is backed by a BO at all.I fully agree, that's why I wrote "the common case should be that in a VA space at least every mapping *being backed by a BO* is represented by a struct drm_gpuva". Mappings not being backed by an actual BO would not be linked to a GEM of course.> >> >>> >>> Instead I suggest to have a separate structure for mappings in a VA >>> space which driver can then add to their GEM objects or whatever they >>> want to map into their VMs. >> >> Which kind of separate structure for mappings? Another one analogous >> to struct drm_gpuva? > > Well similar to what amdgpu uses BO -> one structure for each > combination of BO and VM -> mappings inside that VMAs explained above, I think that's exactly what the GPUVA manager does, just in another order: BO has list of mappings, mappings have pointer to VM, VM has list (or actually a maple tree) of mappings. You see any advantages or disadvantages of either order of relationships? For me it looks like it doesn't really matter which one to pick. - Danilo> > Regards, > Christian. > >> >> - Danilo >> >>> >>> Regards, >>> Christian. >>> >>> >> >