Danilo Krummrich
2023-Jul-07 12:32 UTC
[Nouveau] [PATCH drm-next v6 02/13] drm: manager to keep track of GPUs VA mappings
On 7/7/23 09:57, Boris Brezillon wrote:> On Thu, 6 Jul 2023 20:26:42 +0200 > Boris Brezillon <boris.brezillon at collabora.com> wrote: > >> On Fri, 30 Jun 2023 00:25:18 +0200 >> Danilo Krummrich <dakr at redhat.com> wrote: >> >>> +#ifdef CONFIG_LOCKDEP >>> +typedef struct lockdep_map *lockdep_map_p; >>> +#define drm_gpuva_manager_ext_assert_held(mgr) \ >>> + lockdep_assert(lock_is_held((mgr)->ext_lock) != LOCK_STATE_NOT_HELD) >>> +/** >>> + * drm_gpuva_manager_set_ext_lock - set the external lock according to >>> + * @DRM_GPUVA_MANAGER_LOCK_EXTERN >>> + * @mgr: the &drm_gpuva_manager to set the lock for >>> + * @lock: the lock to set >>> + * >>> + * If @DRM_GPUVA_MANAGER_LOCK_EXTERN is set, drivers need to call this function >>> + * to provide the lock used to lock linking and unlinking of &drm_gpuvas to the >>> + * &drm_gem_objects GPUVA list. >>> + */ >>> +#define drm_gpuva_manager_set_ext_lock(mgr, lock) \ >>> + (mgr)->ext_lock = &(lock)->dep_map >> >> Okay, so, IIUC, this is the lock protecting the GEM's active mappings >> list, meaning the lock is likely to be attached to the GEM object. Are >> we expected to call drm_gpuva_manager_set_ext_lock() every time we call >> drm_gpuva_[un]link(), or are we supposed to have some lock at the >> device level serializing all drm_gpuva_[un]link() calls across VMs? The >> later doesn't sound like a good option to me, and the former feels a bit >> weird. I'm wondering if we shouldn't just drop this assert_held() check >> when DRM_GPUVA_MANAGER_LOCK_EXTERN is set. Alternatively, we could say >> that any driver wanting to use a custom lock (which is basically all >> drivers modifying the VA space asynchronously in the ::run_job() path) >> has to provide its own variant of drm_gpuva_[un]link() (maybe with its >> own VA list too), which doesn't sound like a good idea either. > > Or we could just attach the dep_map to drm_gem_object::gpuva::lock, and > let drivers overload the default lock in their GEM creation function if > they want to use a custom lock (see the following diff).Uh, I like that. Will pick it up, thanks!> > --- > > diff --git a/drivers/gpu/drm/drm_gpuva_mgr.c b/drivers/gpu/drm/drm_gpuva_mgr.c > index e47747f22126..6427c88c22ba 100644 > --- a/drivers/gpu/drm/drm_gpuva_mgr.c > +++ b/drivers/gpu/drm/drm_gpuva_mgr.c > @@ -675,8 +675,7 @@ drm_gpuva_manager_init(struct drm_gpuva_manager *mgr, > const char *name, > u64 start_offset, u64 range, > u64 reserve_offset, u64 reserve_range, > - const struct drm_gpuva_fn_ops *ops, > - enum drm_gpuva_manager_flags flags) > + const struct drm_gpuva_fn_ops *ops) > { > mgr->rb.tree = RB_ROOT_CACHED; > INIT_LIST_HEAD(&mgr->rb.list); > @@ -686,7 +685,6 @@ drm_gpuva_manager_init(struct drm_gpuva_manager *mgr, > mgr->mm_range = range; > > mgr->name = name ? name : "unknown"; > - mgr->flags = flags; > mgr->ops = ops; > > memset(&mgr->kernel_alloc_node, 0, sizeof(struct drm_gpuva)); > @@ -822,16 +820,12 @@ EXPORT_SYMBOL(drm_gpuva_remove); > void > drm_gpuva_link(struct drm_gpuva *va) > { > - struct drm_gpuva_manager *mgr = va->mgr; > struct drm_gem_object *obj = va->gem.obj; > > if (unlikely(!obj)) > return; > > - if (drm_gpuva_manager_external_lock(mgr)) > - drm_gpuva_manager_ext_assert_held(mgr); > - else > - dma_resv_assert_held(obj->resv); > + drm_gem_gpuva_assert_lock_held(obj); > > list_add_tail(&va->gem.entry, &obj->gpuva.list); > } > @@ -850,16 +844,12 @@ EXPORT_SYMBOL(drm_gpuva_link); > void > drm_gpuva_unlink(struct drm_gpuva *va) > { > - struct drm_gpuva_manager *mgr = va->mgr; > struct drm_gem_object *obj = va->gem.obj; > > if (unlikely(!obj)) > return; > > - if (drm_gpuva_manager_external_lock(mgr)) > - drm_gpuva_manager_ext_assert_held(mgr); > - else > - dma_resv_assert_held(obj->resv); > + drm_gem_gpuva_assert_lock_held(obj); > > list_del_init(&va->gem.entry); > } > @@ -1680,10 +1670,7 @@ drm_gpuva_gem_unmap_ops_create(struct drm_gpuva_manager *mgr, > struct drm_gpuva *va; > int ret; > > - if (drm_gpuva_manager_external_lock(mgr)) > - drm_gpuva_manager_ext_assert_held(mgr); > - else > - dma_resv_assert_held(obj->resv); > + drm_gem_gpuva_assert_lock_held(obj); > > ops = kzalloc(sizeof(*ops), GFP_KERNEL); > if (!ops) > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 5ec8148a30ee..572d7a538324 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -387,10 +387,14 @@ struct drm_gem_object { > * 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). > + * (&drm_gem_object.resv) or a custom lock if one is provided. > */ > struct { > struct list_head list; > + > +#ifdef CONFIG_LOCKDEP > + struct lockdep_map *lock_dep_map; > +#endif > } gpuva; > > /** > @@ -540,6 +544,26 @@ unsigned long drm_gem_lru_scan(struct drm_gem_lru *lru, > > int drm_gem_evict(struct drm_gem_object *obj); > > +#ifdef CONFIG_LOCKDEP > +/* > + * drm_gem_gpuva_set_lock() - Set the lock protecting accesses to the gpuva list. > + * @obj: the &drm_gem_object > + * @lock: the lock used to protect the gpuva list. The locking primitive > + * must contain a dep_map field. > + * > + * Call this if you're not proctecting access to the gpuva list > + * with the resv lock, otherwise, drm_gem_gpuva_init() takes case > + * of initializing the lock_dep_map for you. > + */ > +#define drm_gem_gpuva_set_lock(obj, lock) \ > + obj->gpuva.lock_dep_map = &(lock)->dep_map > +#define drm_gem_gpuva_assert_lock_held(obj) \ > + lockdep_assert(lock_is_held(obj->gpuva.lock_dep_map)) > +#else > +#define drm_gem_gpuva_set_lock(obj, lock) do {} while(0) > +#define drm_gem_gpuva_assert_lock_held(obj) do {} while(0) > +#endif > + > /** > * drm_gem_gpuva_init - initialize the gpuva list of a GEM object > * @obj: the &drm_gem_object > @@ -552,6 +576,7 @@ int drm_gem_evict(struct drm_gem_object *obj); > static inline void drm_gem_gpuva_init(struct drm_gem_object *obj) > { > INIT_LIST_HEAD(&obj->gpuva.list); > + drm_gem_gpuva_set_lock(obj, &obj->resv->lock.base); > } > > /** > diff --git a/include/drm/drm_gpuva_mgr.h b/include/drm/drm_gpuva_mgr.h > index 4f23aaf726dd..4ad56b67e244 100644 > --- a/include/drm/drm_gpuva_mgr.h > +++ b/include/drm/drm_gpuva_mgr.h > @@ -185,44 +185,6 @@ static inline bool drm_gpuva_invalidated(struct drm_gpuva *va) > return va->flags & DRM_GPUVA_INVALIDATED; > } > > -#ifdef CONFIG_LOCKDEP > -typedef struct lockdep_map *lockdep_map_p; > -#define drm_gpuva_manager_ext_assert_held(mgr) \ > - lockdep_assert(lock_is_held((mgr)->ext_lock) != LOCK_STATE_NOT_HELD) > -/** > - * drm_gpuva_manager_set_ext_lock - set the external lock according to > - * @DRM_GPUVA_MANAGER_LOCK_EXTERN > - * @mgr: the &drm_gpuva_manager to set the lock for > - * @lock: the lock to set > - * > - * If @DRM_GPUVA_MANAGER_LOCK_EXTERN is set, drivers need to call this function > - * to provide the lock used to lock linking and unlinking of &drm_gpuvas to the > - * &drm_gem_objects GPUVA list. > - */ > -#define drm_gpuva_manager_set_ext_lock(mgr, lock) \ > - (mgr)->ext_lock = &(lock)->dep_map > -#else > -typedef struct { /* nothing */ } lockdep_map_p; > -#define drm_gpuva_manager_ext_assert_held(mgr) do { (void)(mgr); } while (0) > -#define drm_gpuva_manager_set_ext_lock(mgr, lock) do { } while (0) > -#endif > - > -/** > - * enum drm_gpuva_manager_flags - the feature flags for the &drm_gpuva_manager > - */ > -enum drm_gpuva_manager_flags { > - /** > - * @DRM_GPUVA_MANAGER_LOCK_EXTERN: > - * > - * Indicates the driver has it's own external lock for linking and > - * unlinking &drm_gpuvas to the &drm_gem_objects GPUVA list. > - * > - * When setting this flag it is rquired to set a lock via > - * drm_gpuva_set_ext_lock(). > - */ > - DRM_GPUVA_MANAGER_LOCK_EXTERN = (1 << 0), > -}; > - > /** > * struct drm_gpuva_manager - DRM GPU VA Manager > * > @@ -241,11 +203,6 @@ struct drm_gpuva_manager { > */ > const char *name; > > - /** > - * @flags: the feature flags of the &drm_gpuva_manager > - */ > - enum drm_gpuva_manager_flags flags; > - > /** > * @mm_start: start of the VA space > */ > @@ -283,31 +240,15 @@ struct drm_gpuva_manager { > * @ops: &drm_gpuva_fn_ops providing the split/merge steps to drivers > */ > const struct drm_gpuva_fn_ops *ops; > - > - /** > - * @ext_lock: &lockdep_map according to @DRM_GPUVA_MANAGER_LOCK_EXTERN > - */ > - lockdep_map_p ext_lock; > }; > > void drm_gpuva_manager_init(struct drm_gpuva_manager *mgr, > const char *name, > u64 start_offset, u64 range, > u64 reserve_offset, u64 reserve_range, > - const struct drm_gpuva_fn_ops *ops, > - enum drm_gpuva_manager_flags flags); > + const struct drm_gpuva_fn_ops *ops); > void drm_gpuva_manager_destroy(struct drm_gpuva_manager *mgr); > > -/** > - * drm_gpuva_manager_external_lock - indicates whether the > - * @DRM_GPUVA_MANAGER_LOCK_EXTERN flag is set > - * @mgr: the &drm_gpuva_manager to check the flag for > - */ > -static inline bool drm_gpuva_manager_external_lock(struct drm_gpuva_manager *mgr) > -{ > - return mgr->flags & DRM_GPUVA_MANAGER_LOCK_EXTERN; > -} > - > /** > * drm_gpuva_for_each_va_range - iternator to walk over a range of &drm_gpuvas > * @va__: &drm_gpuva structure to assign to in each iteration step >
Maybe Matching Threads
- [PATCH drm-next v6 02/13] drm: manager to keep track of GPUs VA mappings
- [PATCH drm-next v6 02/13] drm: manager to keep track of GPUs VA mappings
- [PATCH drm-next v7 02/13] drm: manager to keep track of GPUs VA mappings
- [PATCH drm-misc-next v8 01/12] drm: manager to keep track of GPUs VA mappings
- [PATCH drm-misc-next 2/3] drm/gpuva_mgr: generalize dma_resv/extobj handling and GEM validation