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

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