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. > +#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