Messages in this thread | | | Date | Wed, 22 Feb 2023 16:14:25 +0100 | Subject | Re: [PATCH drm-next v2 05/16] drm: manager to keep track of GPUs VA mappings | From | Christian König <> |
| |
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: > > <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.
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. >> >
| |