Re: [Intel-xe] [PATCH] drm/xe: add lockdep annotation for xe_device_mem_access_put()

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Jul 21, 2023 at 01:34:25PM +0100, Matthew Auld wrote:
> The main motivation is with d3cold which will make the suspend and
> resume callbacks even more scary, but is useful regardless. We already
> have the needed annotation on the acquire side with
> xe_device_mem_access_get(), and by adding the annotation on the release
> side we should have a lot more confidence that our locking hierarchy is
> correct.

good idea!

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c |  2 +-
>  drivers/gpu/drm/xe/xe_device.h |  4 ++++
>  drivers/gpu/drm/xe/xe_pm.c     | 24 ++++++++++++++++++++++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 1c57944014e0..fed3015e2d96 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -36,7 +36,7 @@
>  #include "xe_wait_user_fence.h"
>  
>  #ifdef CONFIG_LOCKDEP
> -static struct lockdep_map xe_device_mem_access_lockdep_map = {
> +struct lockdep_map xe_device_mem_access_lockdep_map = {
>  	.name = "xe_device_mem_access_lockdep_map"
>  };
>  #endif
> diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
> index 8b085ffdc5f8..593accb68281 100644
> --- a/drivers/gpu/drm/xe/xe_device.h
> +++ b/drivers/gpu/drm/xe/xe_device.h
> @@ -16,6 +16,10 @@ struct xe_file;
>  #include "xe_force_wake.h"
>  #include "xe_macros.h"
>  
> +#ifdef CONFIG_LOCKDEP
> +extern struct lockdep_map xe_device_mem_access_lockdep_map;
> +#endif
> +
>  static inline struct xe_device *to_xe_device(const struct drm_device *dev)
>  {
>  	return container_of(dev, struct xe_device, drm);
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index 17a69b7af155..d1b2aa52ea03 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -199,6 +199,29 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>  	/* Disable access_ongoing asserts and prevent recursive pm calls */
>  	xe_pm_write_callback_task(xe, current);
>  
> +	/*
> +	 * The actual xe_device_mem_access_put() is always async underneath, so
> +	 * exactly where that is called should makes no difference to us. However
> +	 * we still need to be very careful with the locks that this callback
> +	 * acquires and the locks that are acquired and held by any callers of
> +	 * xe_device_mem_access_get(). We already have the matching annotation
> +	 * on that side, but we also need it here. For example lockdep should be
> +	 * able to tell us if the following scenario is in theory possible:
> +	 *
> +	 * CPU0                          | CPU1 (kworker)
> +	 * lock(A)                       |
> +	 *                               | xe_pm_runtime_suspend()
> +	 *                               |      lock(A)
> +	 * xe_device_mem_access_get()    |
> +	 *
> +	 * This will clearly deadlock since rpm core needs to wait for
> +	 * xe_pm_runtime_suspend() to complete, but here we are holding lock(A)
> +	 * on CPU0 which prevents CPU1 making forward progress.  With the
> +	 * annotation here and in xe_device_mem_access_get() lockdep will see
> +	 * the potential lock inversion and give us a nice splat.
> +	 */
> +	lock_map_acquire(&xe_device_mem_access_lockdep_map);
> +
>  	if (xe->d3cold.allowed) {
>  		err = xe_bo_evict_all(xe);
>  		if (err)
> @@ -213,6 +236,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>  
>  	xe_irq_suspend(xe);
>  out:
> +	lock_map_release(&xe_device_mem_access_lockdep_map);
>  	xe_pm_write_callback_task(xe, NULL);
>  	return err;
>  }
> -- 
> 2.41.0
>