Re: [PATCH drm-next v6 02/13] drm: manager to keep track of GPUs VA mappings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 7/7/23 09:57, Boris Brezillon wrote:
On Thu, 6 Jul 2023 20:26:42 +0200
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:

On Fri, 30 Jun 2023 00:25:18 +0200
Danilo Krummrich <dakr@xxxxxxxxxx> 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





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux