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

[Boris Brezillon <boris.brezillon@collabora.com>]

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

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