[PATCH drm-next v6 02/13] drm: manager to keep track of GPUs VA mappings
Danilo Krummrich
dakr at redhat.com
Fri Jul 7 12:32:30 UTC 2023
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
>
More information about the dri-devel
mailing list