Re: [PATCH drm-misc-next v9 01/11] drm/gem: fix lockdep check for dma-resv lock
On 8/8/23 09:21, Boris Brezillon wrote:
On Thu, 3 Aug 2023 18:52:20 +0200
Danilo Krummrich wrote:
When no custom lock is set to protect a GEMs GPUVA list, lockdep checks
should fall back to the GEM objects dma-resv lock. With the current
implementation we're setting the lock_dep_map of the GEM objects 'resv'
pointer (in case no custom lock_dep_map is set yet) on
drm_gem_private_object_init().
However, the GEM objects 'resv' pointer might still change after
drm_gem_private_object_init() is called, e.g. through
ttm_bo_init_reserved(). This can result in the wrong lock being tracked.
To fix this, call dma_resv_held() directly from
drm_gem_gpuva_assert_lock_held() and fall back to the GEMs lock_dep_map
pointer only if an actual custom lock is set.
Fixes: e6303f323b1a ("drm: manager to keep track of GPUs VA mappings")
Signed-off-by: Danilo Krummrich
Reviewed-by: Boris Brezillon
but I'm wondering if it wouldn't be a good thing to add a
drm_gem_set_resv() helper, so the core can control drm_gem_object::resv
re-assignments (block them if it's happening after the GEM has been
exposed to the outside world or update auxiliary data if it's happening
before that).
I agree, this might be a good idea. There are quite a few places where
drm_gem_object::resv is set from external code.
---
include/drm/drm_gem.h | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index c0b13c43b459..bc9f6aa2f3fe 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -551,15 +551,17 @@ int drm_gem_evict(struct drm_gem_object *obj);
* @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 dma-resv lock, otherwise, drm_gem_gpuva_init() takes care
- * of initializing lock_dep_map for you.
+ * Call this if you're not proctecting access to the gpuva list with the
+ * dma-resv lock, but with a custom lock.
*/
#define drm_gem_gpuva_set_lock(obj, lock) \
- if (!(obj)->gpuva.lock_dep_map) \
+ if (!WARN((obj)->gpuva.lock_dep_map, \
+ "GEM GPUVA lock should be set only once.")) \
(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))
+ lockdep_assert((obj)->gpuva.lock_dep_map ? \
+ lock_is_held((obj)->gpuva.lock_dep_map) : \
+ dma_resv_held((obj)->resv))
#else
#define drm_gem_gpuva_set_lock(obj, lock) do {} while (0)
#define drm_gem_gpuva_assert_lock_held(obj) do {} while (0)
@@ -573,11 +575,12 @@ int drm_gem_evict(struct drm_gem_object *obj);
*
* Calling this function is only necessary for drivers intending to support
the
* _driver_feature DRIVER_GEM_GPUVA.
+ *
+ * See also drm_gem_gpuva_set_lock().
*/
static inline void drm_gem_gpuva_init(struct drm_gem_object *obj)
{
INIT_LIST_HEAD(>gpuva.list);
- drm_gem_gpuva_set_lock(obj, >resv->lock.base);
}
/**
Re: [PATCH drm-misc-next v9 01/11] drm/gem: fix lockdep check for dma-resv lock
On Thu, 3 Aug 2023 18:52:20 +0200
Danilo Krummrich wrote:
> When no custom lock is set to protect a GEMs GPUVA list, lockdep checks
> should fall back to the GEM objects dma-resv lock. With the current
> implementation we're setting the lock_dep_map of the GEM objects 'resv'
> pointer (in case no custom lock_dep_map is set yet) on
> drm_gem_private_object_init().
>
> However, the GEM objects 'resv' pointer might still change after
> drm_gem_private_object_init() is called, e.g. through
> ttm_bo_init_reserved(). This can result in the wrong lock being tracked.
>
> To fix this, call dma_resv_held() directly from
> drm_gem_gpuva_assert_lock_held() and fall back to the GEMs lock_dep_map
> pointer only if an actual custom lock is set.
>
> Fixes: e6303f323b1a ("drm: manager to keep track of GPUs VA mappings")
> Signed-off-by: Danilo Krummrich
Reviewed-by: Boris Brezillon
but I'm wondering if it wouldn't be a good thing to add a
drm_gem_set_resv() helper, so the core can control drm_gem_object::resv
re-assignments (block them if it's happening after the GEM has been
exposed to the outside world or update auxiliary data if it's happening
before that).
> ---
> include/drm/drm_gem.h | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index c0b13c43b459..bc9f6aa2f3fe 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -551,15 +551,17 @@ int drm_gem_evict(struct drm_gem_object *obj);
> * @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 dma-resv lock, otherwise, drm_gem_gpuva_init() takes care
> - * of initializing lock_dep_map for you.
> + * Call this if you're not proctecting access to the gpuva list with the
> + * dma-resv lock, but with a custom lock.
> */
> #define drm_gem_gpuva_set_lock(obj, lock) \
> - if (!(obj)->gpuva.lock_dep_map) \
> + if (!WARN((obj)->gpuva.lock_dep_map, \
> + "GEM GPUVA lock should be set only once.")) \
> (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))
> + lockdep_assert((obj)->gpuva.lock_dep_map ? \
> +lock_is_held((obj)->gpuva.lock_dep_map) : \
> +dma_resv_held((obj)->resv))
> #else
> #define drm_gem_gpuva_set_lock(obj, lock) do {} while (0)
> #define drm_gem_gpuva_assert_lock_held(obj) do {} while (0)
> @@ -573,11 +575,12 @@ int drm_gem_evict(struct drm_gem_object *obj);
> *
> * Calling this function is only necessary for drivers intending to support
> the
> * _driver_feature DRIVER_GEM_GPUVA.
> + *
> + * See also drm_gem_gpuva_set_lock().
> */
> static inline void drm_gem_gpuva_init(struct drm_gem_object *obj)
> {
> INIT_LIST_HEAD(>gpuva.list);
> - drm_gem_gpuva_set_lock(obj, >resv->lock.base);
> }
>
> /**
[PATCH drm-misc-next v9 01/11] drm/gem: fix lockdep check for dma-resv lock
When no custom lock is set to protect a GEMs GPUVA list, lockdep checks
should fall back to the GEM objects dma-resv lock. With the current
implementation we're setting the lock_dep_map of the GEM objects 'resv'
pointer (in case no custom lock_dep_map is set yet) on
drm_gem_private_object_init().
However, the GEM objects 'resv' pointer might still change after
drm_gem_private_object_init() is called, e.g. through
ttm_bo_init_reserved(). This can result in the wrong lock being tracked.
To fix this, call dma_resv_held() directly from
drm_gem_gpuva_assert_lock_held() and fall back to the GEMs lock_dep_map
pointer only if an actual custom lock is set.
Fixes: e6303f323b1a ("drm: manager to keep track of GPUs VA mappings")
Signed-off-by: Danilo Krummrich
---
include/drm/drm_gem.h | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index c0b13c43b459..bc9f6aa2f3fe 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -551,15 +551,17 @@ int drm_gem_evict(struct drm_gem_object *obj);
* @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 dma-resv lock, otherwise, drm_gem_gpuva_init() takes care
- * of initializing lock_dep_map for you.
+ * Call this if you're not proctecting access to the gpuva list with the
+ * dma-resv lock, but with a custom lock.
*/
#define drm_gem_gpuva_set_lock(obj, lock) \
- if (!(obj)->gpuva.lock_dep_map) \
+ if (!WARN((obj)->gpuva.lock_dep_map, \
+ "GEM GPUVA lock should be set only once.")) \
(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))
+ lockdep_assert((obj)->gpuva.lock_dep_map ? \
+ lock_is_held((obj)->gpuva.lock_dep_map) : \
+ dma_resv_held((obj)->resv))
#else
#define drm_gem_gpuva_set_lock(obj, lock) do {} while (0)
#define drm_gem_gpuva_assert_lock_held(obj) do {} while (0)
@@ -573,11 +575,12 @@ int drm_gem_evict(struct drm_gem_object *obj);
*
* Calling this function is only necessary for drivers intending to support the
* _driver_feature DRIVER_GEM_GPUVA.
+ *
+ * See also drm_gem_gpuva_set_lock().
*/
static inline void drm_gem_gpuva_init(struct drm_gem_object *obj)
{
INIT_LIST_HEAD(>gpuva.list);
- drm_gem_gpuva_set_lock(obj, >resv->lock.base);
}
/**
--
2.41.0
