Re: [Intel-xe] [PATCH v2 15/31] drm: manager to keep track of GPUs VA mappings

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On 5/2/23 02:17, Matthew Brost wrote:
> From: Danilo Krummrich <dakr@redhat.com>
>
> Add infrastructure to keep track of GPU virtual address (VA) mappings
> with a decicated VA space manager implementation.
>
> New UAPIs, motivated by Vulkan sparse memory bindings graphics drivers
> start implementing, allow userspace applications to request multiple and
> arbitrary GPU VA mappings of buffer objects. The DRM GPU VA manager is
> intended to serve the following purposes in this context.
>
> 1) Provide infrastructure to track GPU VA allocations and mappings,
>     making use of the maple_tree.
>
> 2) Generically connect GPU VA mappings to their backing buffers, in
>     particular DRM GEM objects.
>
> 3) Provide a common implementation to perform more complex mapping
>     operations on the GPU VA space. In particular splitting and merging
>     of GPU VA mappings, e.g. for intersecting mapping requests or partial
>     unmap requests.
>
> Suggested-by: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>

Danilo, Matthew

Before embarking on a full review of this (saving this for last) I heard 
there might be plans to add userptr support, rebind_work interaction and 
such and resolve any driver differences using vfuncs.

Just wanted to raise a warning that helpers that attempt to "do it all" 
and depend on vfuncs are easy traps to start creating middle layers 
(like TTM) which are typically frowned upon. (See for example the 
discussion on the partly rejected patch series on the TTM shrinker).

So just as a recommendation to avoid redoing a lot of stuff, please be 
careful with additional helpers that require vfuncs and check if they 
can be implemented in another way by rethinking the layering.

Thanks,

Thomas