Danilo Krummrich
2023-Aug-09 22:40 UTC
[Nouveau] [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 <dakr at redhat.com> 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 <dakr at redhat.com> > > Reviewed-by: Boris Brezillon <boris.brezillon at collabora.com> > > 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 >> * &drm_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(&obj->gpuva.list); >> - drm_gem_gpuva_set_lock(obj, &obj->resv->lock.base); >> } >> >> /** >