Messages in this thread | | | Date | Thu, 14 Sep 2023 19:25:31 +0200 | Subject | Re: [PATCH drm-misc-next v3 6/7] drm/gpuvm: generalize dma_resv/extobj handling and GEM validation | From | Danilo Krummrich <> |
| |
On 9/14/23 19:21, Thomas Hellström wrote: > On Thu, 2023-09-14 at 18:36 +0200, Danilo Krummrich wrote: >> On 9/14/23 15:48, Thomas Hellström wrote: >>> Hi, Danilo >>> >>> Some additional minor comments as xe conversion progresses. >>> >>> On 9/9/23 17:31, Danilo Krummrich wrote: >>>> So far the DRM GPUVA manager offers common infrastructure to >>>> track GPU VA >>>> allocations and mappings, generically connect GPU VA mappings to >>>> their >>>> backing buffers and perform more complex mapping operations on >>>> the GPU VA >>>> space. >>>> >>>> However, there are more design patterns commonly used by drivers, >>>> which >>>> can potentially be generalized in order to make the DRM GPUVA >>>> manager >>>> represent a basic GPU-VM implementation. In this context, this >>>> patch aims >>>> at generalizing the following elements. >>>> >>>> 1) Provide a common dma-resv for GEM objects not being used >>>> outside of >>>> this GPU-VM. >>>> >>>> 2) Provide tracking of external GEM objects (GEM objects which >>>> are >>>> shared with other GPU-VMs). >>>> >>>> 3) Provide functions to efficiently lock all GEM objects dma-resv >>>> the >>>> GPU-VM contains mappings of. >>>> >>>> 4) Provide tracking of evicted GEM objects the GPU-VM contains >>>> mappings >>>> of, such that validation of evicted GEM objects is >>>> accelerated. >>>> >>>> 5) Provide some convinience functions for common patterns. >>>> >>>> Rather than being designed as a "framework", the target is to >>>> make all >>>> features appear as a collection of optional helper functions, >>>> such that >>>> drivers are free to make use of the DRM GPUVA managers basic >>>> functionality and opt-in for other features without setting any >>>> feature >>>> flags, just by making use of the corresponding functions. >>>> >>>> Big kudos to Boris Brezillon for his help to figure out locking >>>> for drivers >>>> updating the GPU VA space within the fence signalling path. >>>> >>>> Suggested-by: Matthew Brost <matthew.brost@intel.com> >>>> Signed-off-by: Danilo Krummrich <dakr@redhat.com> >>>> --- >>>> >>>> +/** >>>> + * drm_gpuvm_bo_evict() - add / remove a &drm_gem_object to / >>>> from a >>>> + * &drm_gpuvms evicted list >>>> + * @obj: the &drm_gem_object to add or remove >>>> + * @evict: indicates whether the object is evicted >>>> + * >>>> + * Adds a &drm_gem_object to or removes it from all &drm_gpuvms >>>> evicted >>>> + * list containing a mapping of this &drm_gem_object. >>>> + */ >>>> +void >>>> +drm_gpuvm_bo_evict(struct drm_gem_object *obj, bool evict) >>>> +{ >>>> + struct drm_gpuvm_bo *vm_bo; >>>> + >>>> + drm_gem_for_each_gpuvm_bo(vm_bo, obj) { >>>> + if (evict) >>>> + drm_gpuvm_bo_list_add(vm_bo, evict); >>>> + else >>>> + drm_gpuvm_bo_list_del(vm_bo, evict); >>>> + } >>>> +} >>>> +EXPORT_SYMBOL_GPL(drm_gpuvm_bo_evict); >>>> + >>> >>> We need a drm_gpuvm_bo_evict(struct drm_gpuvm_bo *vm_bo, ...) that >>> puts a single gpuvm_bo on the list, the above function could >>> perhaps be renamed as drm_gpuvm_gem_obj_evict(obj, ....). >> >> Makes sense - gonna change that. >> >>> >>> Reason is some vm's are faulting vms which don't have an evict >>> list, but validate from the pagefault handler. Also evict == false >>> is dangerous because if called from within an exec, it might remove >>> the obj from other vm's evict list before they've had a chance to >>> rebind their VMAs. >>> >>>> static int >>>> __drm_gpuva_insert(struct drm_gpuvm *gpuvm, >>>> struct drm_gpuva *va) >>>> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h >>>> index afa50b9059a2..834bb6d6617e 100644 >>>> --- a/include/drm/drm_gpuvm.h >>>> +++ b/include/drm/drm_gpuvm.h >>>> @@ -26,10 +26,12 @@ >>>> */ >>>> #include <linux/list.h> >>>> +#include <linux/dma-resv.h> >>>> #include <linux/rbtree.h> >>>> #include <linux/types.h> >>>> #include <drm/drm_gem.h> >>>> +#include <drm/drm_exec.h> >>>> struct drm_gpuvm; >>>> struct drm_gpuvm_bo; >>>> @@ -259,6 +261,38 @@ struct drm_gpuvm { >>>> * space >>>> */ >>>> struct dma_resv *resv; >>>> + >>>> + /** >>>> + * @extobj: structure holding the extobj list >>>> + */ >>>> + struct { >>>> + /** >>>> + * @list: &list_head storing &drm_gpuvm_bos serving as >>>> + * external object >>>> + */ >>>> + struct list_head list; >>>> + >>>> + /** >>>> + * @lock: spinlock to protect the extobj list >>>> + */ >>>> + spinlock_t lock; >>>> + } extobj; >>>> + >>>> + /** >>>> + * @evict: structure holding the evict list and evict list >>>> lock >>>> + */ >>>> + struct { >>>> + /** >>>> + * @list: &list_head storing &drm_gpuvm_bos currently >>>> being >>>> + * evicted >>>> + */ >>>> + struct list_head list; >>>> + >>>> + /** >>>> + * @lock: spinlock to protect the evict list >>>> + */ >>>> + spinlock_t lock; >>>> + } evict; >>>> }; >>>> void drm_gpuvm_init(struct drm_gpuvm *gpuvm, struct drm_device >>>> *drm, >>>> @@ -268,6 +302,21 @@ void drm_gpuvm_init(struct drm_gpuvm *gpuvm, >>>> struct drm_device *drm, >>>> const struct drm_gpuvm_ops *ops); >>>> void drm_gpuvm_destroy(struct drm_gpuvm *gpuvm); >>>> +/** >>>> + * drm_gpuvm_is_extobj() - indicates whether the given >>>> &drm_gem_object is an >>>> + * external object >>>> + * @gpuvm: the &drm_gpuvm to check >>>> + * @obj: the &drm_gem_object to check >>>> + * >>>> + * Returns: true if the &drm_gem_object &dma_resv differs from >>>> the >>>> + * &drm_gpuvms &dma_resv, false otherwise >>>> + */ >>>> +static inline bool drm_gpuvm_is_extobj(struct drm_gpuvm *gpuvm, >>>> + struct drm_gem_object *obj) >>>> +{ >>>> + return obj && obj->resv != gpuvm->resv; >>>> +} >>>> + >>>> static inline struct drm_gpuva * >>>> __drm_gpuva_next(struct drm_gpuva *va) >>>> { >>>> @@ -346,6 +395,128 @@ __drm_gpuva_next(struct drm_gpuva *va) >>>> #define drm_gpuvm_for_each_va_safe(va__, next__, gpuvm__) \ >>>> list_for_each_entry_safe(va__, next__, &(gpuvm__)->rb.list, >>>> rb.entry) >>>> +/** >>>> + * struct drm_gpuvm_exec - &drm_gpuvm abstraction of &drm_exec >>>> + * >>>> + * This structure should be created on the stack as &drm_exec >>>> should be. >>>> + * >>>> + * Optionally, @extra can be set in order to lock additional >>>> &drm_gem_objects. >>>> + */ >>>> +struct drm_gpuvm_exec { >>>> + /** >>>> + * @exec: the &drm_exec structure >>>> + */ >>>> + struct drm_exec exec; >>>> + >>>> + /** >>>> + * @vm: the &drm_gpuvm to lock its DMA reservations >>>> + */ >>>> + struct drm_gpuvm *vm; >>>> + >>>> + /** >>>> + * @extra: Callback and corresponding private data for the >>>> driver to >>>> + * lock arbitrary additional &drm_gem_objects. >>>> + */ >>>> + struct { >>>> + /** >>>> + * @fn: The driver callback to lock additional >>>> &drm_gem_objects. >>>> + */ >>>> + int (*fn)(struct drm_gpuvm_exec *vm_exec, >>>> + unsigned int num_fences); >>>> + >>>> + /** >>>> + * @priv: driver private data for the @fn callback >>>> + */ >>>> + void *priv; >>>> + } extra; >>>> +}; >>>> + >>>> +/** >>>> + * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv >>>> + * @gpuvm: the &drm_gpuvm >>>> + * @exec: the &drm_exec context >>>> + * @num_fences: the amount of &dma_fences to reserve >>>> + * >>>> + * Calls drm_exec_prepare_obj() for the GPUVMs dummy >>>> &drm_gem_object. >>>> + * >>>> + * Using this function directly, it is the drivers >>>> responsibility to call >>>> + * drm_exec_init() and drm_exec_fini() accordingly. >>>> + * >>>> + * Returns: 0 on success, negative error code on failure. >>>> + */ >>>> +static inline int >>>> +drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm, >>>> + struct drm_exec *exec, >>>> + unsigned int num_fences) >>>> +{ >>>> + return drm_exec_prepare_obj(exec, &gpuvm->d_obj, >>>> num_fences); >>>> +} >>>> + >>>> +int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm, >>>> + struct drm_exec *exec, >>>> + unsigned int num_fences); >>>> + >>>> +int drm_gpuvm_prepare_range(struct drm_gpuvm *gpuvm, >>>> + struct drm_exec *exec, >>>> + u64 addr, u64 range, >>>> + unsigned int num_fences); >>>> + >>>> +int drm_gpuvm_exec_lock(struct drm_gpuvm_exec *vm_exec, >>>> + unsigned int num_fences, >>>> + bool interruptible); >>>> + >>>> +int drm_gpuvm_exec_lock_array(struct drm_gpuvm_exec *vm_exec, >>>> + struct drm_gem_object **objs, >>>> + unsigned int num_objs, >>>> + unsigned int num_fences, >>>> + bool interruptible); >>>> + >>>> +int drm_gpuvm_exec_lock_range(struct drm_gpuvm_exec *vm_exec, >>>> + u64 addr, u64 range, >>>> + unsigned int num_fences, >>>> + bool interruptible); >>>> + >>>> +/** >>>> + * drm_gpuvm_lock() - lock all dma-resv of all assoiciated BOs >>>> + * @gpuvm: the &drm_gpuvm >>>> + * >>>> + * Releases all dma-resv locks of all &drm_gem_objects >>>> previously acquired >>>> + * through drm_gpuvm_lock() or its variants. >>>> + * >>>> + * Returns: 0 on success, negative error code on failure. >>>> + */ >>>> +static inline void >>>> +drm_gpuvm_exec_unlock(struct drm_gpuvm_exec *vm_exec) >>>> +{ >>>> + drm_exec_fini(&vm_exec->exec); >>>> +} >>>> + >>>> +int drm_gpuvm_validate(struct drm_gpuvm *gpuvm); >>>> +void drm_gpuvm_resv_add_fence(struct drm_gpuvm *gpuvm, >>>> + struct drm_exec *exec, >>>> + struct dma_fence *fence, >>>> + enum dma_resv_usage private_usage, >>>> + enum dma_resv_usage extobj_usage); >>>> + >>>> +/** >>>> + * drm_gpuvm_exec_resv_add_fence() >>>> + * @vm_exec: the &drm_gpuvm_exec abstraction >>>> + * @fence: fence to add >>>> + * @private_usage: private dma-resv usage >>>> + * @extobj_usage: extobj dma-resv usage >>>> + * >>>> + * See drm_gpuvm_resv_add_fence(). >>>> + */ >>>> +static inline void >>>> +drm_gpuvm_exec_resv_add_fence(struct drm_gpuvm_exec *vm_exec, >>>> + struct dma_fence *fence, >>>> + enum dma_resv_usage private_usage, >>>> + enum dma_resv_usage extobj_usage) >>>> +{ >>>> + drm_gpuvm_resv_add_fence(vm_exec->vm, &vm_exec->exec, fence, >>>> + private_usage, extobj_usage); >>>> +} >>>> + >>>> /** >>>> * struct drm_gpuvm_bo - structure representing a &drm_gpuvm >>>> and >>>> * &drm_gem_object combination >>>> @@ -398,6 +569,18 @@ struct drm_gpuvm_bo { >>>> * gpuva list. >>>> */ >>>> struct list_head gem; >>>> + >>>> + /** >>>> + * @evict: List entry to attach to the &drm_gpuvms >>>> + * extobj list. >>>> + */ >>>> + struct list_head extobj; >>>> + >>>> + /** >>>> + * @evict: List entry to attach to the &drm_gpuvms >>>> evict >>>> + * list. >>>> + */ >>>> + struct list_head evict; >>>> } entry; >>>> } list; >>>> }; >>>> @@ -432,6 +615,9 @@ struct drm_gpuvm_bo * >>>> drm_gpuvm_bo_find(struct drm_gpuvm *gpuvm, >>>> struct drm_gem_object *obj); >>>> +void drm_gpuvm_bo_evict(struct drm_gem_object *obj, bool evict); >>>> +void drm_gpuvm_bo_extobj_add(struct drm_gpuvm_bo *vm_bo); >>>> + >>>> /** >>>> * drm_gpuvm_bo_for_each_va() - iterator to walk over a list of >>>> &drm_gpuva >>>> * @va__: &drm_gpuva structure to assign to in each iteration >>>> step >>>> @@ -837,6 +1023,17 @@ struct drm_gpuvm_ops { >>>> * used. >>>> */ >>>> int (*sm_step_unmap)(struct drm_gpuva_op *op, void *priv); >>>> + >>>> + /** >>>> + * @bo_validate: called from drm_gpuvm_validate() >>>> + * >>>> + * Drivers receive this callback for every evicted >>>> &drm_gem_object being >>>> + * mapped in the corresponding &drm_gpuvm. >>>> + * >>>> + * Typically, drivers would call their driver specific >>>> variant of >>>> + * ttm_bo_validate() from within this callback. >>>> + */ >>>> + int (*bo_validate)(struct drm_gem_object *obj); >>> >>> Same here. Could we have a vm_bo as an argument instead, so that >>> the callback knows what gpuvm we're targeting and can mark all its >>> gpu_vas for revalidation? Or is that intended to be done elsewhere? >> >> Makes sense as well. I'll change that too. > > I forgot, drm_gpuvm_validate() would preferably take an drm_gpuvm_exec > argument because we need it in the validate callback. It's also easy > for the driver to subclass further if needed, to pass even more > arguments to its validate callback.
Hm.. that implies that a driver open coding the drm_exec loop, still needs to use a struct drm_gpuvm_exec rather than just a struct drm_exec. What is this needed for in Xe? Do we expect other drivers needing it? Might a priv void pointer maybe make more sense?
> > /Thomas > > >> >>> >>>> }; >>>> int drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm, void *priv, >>> >>> Thanks, >>> >>> Thomas >>> >>> >> >
| |