Re: [Intel-gfx] [RFC 1/2] drm/i915/dgfx: Release mmap on rpm suspend

From: Matthew Auld <matthew.auld@intel.com>
To: Anshuman Gupta <anshuman.gupta@intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: chris@chris-wilson.co.uk, rodrigo.vivi@intel.com
Subject: Re: [Intel-gfx] [RFC 1/2] drm/i915/dgfx: Release mmap on rpm suspend
Date: Wed, 17 Aug 2022 18:34:02 +0100	[thread overview]
Message-ID: <a802d213-cf66-db97-541c-727d6daf62ff@intel.com> (raw)
In-Reply-To: <20220817150941.25447-2-anshuman.gupta@intel.com>

On 17/08/2022 16:09, Anshuman Gupta wrote:
> Release all mmap mapping for all lmem objects which are associated
> with userfault such that, while pcie function in D3hot, any access
> to memory mappings will raise a userfault.
> 
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c  | 4 ++++
>   drivers/gpu/drm/i915/gt/intel_gt.c       | 1 +
>   drivers/gpu/drm/i915/gt/intel_gt_types.h | 2 ++
>   drivers/gpu/drm/i915/i915_gem.c          | 6 ++++++
>   4 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 5a5cf332d8a5..b49823d599e7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -1073,6 +1073,10 @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf)
>   	} else {
>   		ret = ttm_bo_vm_dummy_page(vmf, vmf->vma->vm_page_prot);
>   	}
> +
> +	if (i915_gem_object_is_lmem(obj))
> +		list_add(&obj->userfault_link, &to_gt(to_i915(obj->base.dev))->lmem_userfault_list);

Both patches need to be squashed together I think. Also AFAIK we need 
some kind of lock here to protect against concurrent list_add(). We also 
need something to prevent against double list_add(). And we should 
probably first check the ret value before touching the list.

> +
>   	if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index e4bac2431e41..f0d641c3153c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -39,6 +39,7 @@ static void __intel_gt_init_early(struct intel_gt *gt)
>   {
>   	spin_lock_init(&gt->irq_lock);
>   
> +	INIT_LIST_HEAD(&gt->lmem_userfault_list);
>   	INIT_LIST_HEAD(&gt->closed_vma);
>   	spin_lock_init(&gt->closed_lock);
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> index 4d56f7d5a3be..3e915df255f3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> @@ -132,6 +132,8 @@ struct intel_gt {
>   	struct intel_wakeref wakeref;
>   	atomic_t user_wakeref;
>   
> +	struct list_head lmem_userfault_list;
> +
>   	struct list_head closed_vma;
>   	spinlock_t closed_lock; /* guards the list of closed_vma */
>   
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 702e5b89be22..1e6ce6d06c11 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -842,6 +842,12 @@ void i915_gem_runtime_suspend(struct drm_i915_private *i915)
>   				 &to_gt(i915)->ggtt->userfault_list, userfault_link)
>   		__i915_gem_object_release_mmap_gtt(obj);
>   
> +	list_for_each_entry_safe(obj, on,
> +				 &to_gt(i915)->lmem_userfault_list, userfault_link) {
> +		i915_gem_object_release_mmap_offset(obj);
> +		list_del(&obj->userfault_link);
> +	}
> +
>   	/*
>   	 * The fence will be lost when the device powers down. If any were
>   	 * in use by hardware (i.e. they are pinned), we should not be powering