Re: [PATCH v2 1/2] drm/i915: Encapsulate lmem rpm stuff in intel_runtime_pm

[Matthew Auld <matthew.auld@xxxxxxxxx>]

On 21/10/2022 10:55, Anshuman Gupta wrote:
> Runtime pm is not really per GT, therefore it make sense to
> move lmem_userfault_list, lmem_userfault_lock and
> userfault_wakeref from intel_gt to intel_runtime_pm structure,
> which is embedded to i915.
>
> No functional change.
>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_mman.c |  6 +++---
>   drivers/gpu/drm/i915/gem/i915_gem_pm.c   |  2 +-
>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c  |  8 ++++----
>   drivers/gpu/drm/i915/gt/intel_gt.c       |  3 ---
>   drivers/gpu/drm/i915/gt/intel_gt_types.h | 17 -----------------
>   drivers/gpu/drm/i915/i915_gem.c          |  4 +---
>   drivers/gpu/drm/i915/intel_runtime_pm.c  |  5 +++++
>   drivers/gpu/drm/i915/intel_runtime_pm.h  | 22 ++++++++++++++++++++++
>   8 files changed, 36 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 73d9eda1d6b7..fd29a9053582 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -413,7 +413,7 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf)
>   	vma->mmo = mmo;
>   
>   	if (CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)
> -		intel_wakeref_auto(&to_gt(i915)->userfault_wakeref,
> +		intel_wakeref_auto(&i915->runtime_pm.userfault_wakeref,
>   				   msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND));
>   
>   	if (write) {
> @@ -589,9 +589,9 @@ void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object *obj)
>   	spin_unlock(&obj->mmo.lock);
>   
>   	if (obj->userfault_count) {
> -		mutex_lock(&to_gt(to_i915(obj->base.dev))->lmem_userfault_lock);
> +		mutex_lock(&to_i915(obj->base.dev)->runtime_pm.lmem_userfault_lock);
>   		list_del(&obj->userfault_link);
> -		mutex_unlock(&to_gt(to_i915(obj->base.dev))->lmem_userfault_lock);
> +		mutex_unlock(&to_i915(obj->base.dev)->runtime_pm.lmem_userfault_lock);
>   		obj->userfault_count = 0;
>   	}
>   }
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> index e5bfb6be9f7a..0d812f4d787d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> @@ -27,7 +27,7 @@ void i915_gem_suspend(struct drm_i915_private *i915)
>   
>   	GEM_TRACE("%s\n", dev_name(i915->drm.dev));
>   
> -	intel_wakeref_auto(&to_gt(i915)->userfault_wakeref, 0);
> +	intel_wakeref_auto(&i915->runtime_pm.userfault_wakeref, 0);
>   	flush_workqueue(i915->wq);
>   
>   	/*
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 84c91a4228a1..50c70796ca38 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -1101,13 +1101,13 @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf)
>   	/* ttm_bo_vm_reserve() already has dma_resv_lock */
>   	if (ret == VM_FAULT_NOPAGE && wakeref && !obj->userfault_count) {
>   		obj->userfault_count = 1;
> -		mutex_lock(&to_gt(to_i915(obj->base.dev))->lmem_userfault_lock);
> -		list_add(&obj->userfault_link, &to_gt(to_i915(obj->base.dev))->lmem_userfault_list);
> -		mutex_unlock(&to_gt(to_i915(obj->base.dev))->lmem_userfault_lock);
> +		mutex_lock(&to_i915(obj->base.dev)->runtime_pm.lmem_userfault_lock);
> +		list_add(&obj->userfault_link, &to_i915(obj->base.dev)->runtime_pm.lmem_userfault_list);
> +		mutex_unlock(&to_i915(obj->base.dev)->runtime_pm.lmem_userfault_lock);
>   	}
>   
>   	if (wakeref & CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)
> -		intel_wakeref_auto(&to_gt(to_i915(obj->base.dev))->userfault_wakeref,
> +		intel_wakeref_auto(&to_i915(obj->base.dev)->runtime_pm.userfault_wakeref,
>   				   msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND));
>   
>   	i915_ttm_adjust_lru(obj);
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 27dbb9e4bd6c..0ad8118ccbee 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -40,8 +40,6 @@ void intel_gt_common_init_early(struct intel_gt *gt)
>   {
>   	spin_lock_init(gt->irq_lock);
>   
> -	INIT_LIST_HEAD(&gt->lmem_userfault_list);
> -	mutex_init(&gt->lmem_userfault_lock);
>   	INIT_LIST_HEAD(&gt->closed_vma);
>   	spin_lock_init(&gt->closed_lock);
>   
> @@ -859,7 +857,6 @@ static int intel_gt_tile_setup(struct intel_gt *gt, phys_addr_t phys_addr)
>   	}
>   
>   	intel_uncore_init_early(gt->uncore, gt);
> -	intel_wakeref_auto_init(&gt->userfault_wakeref, gt->uncore->rpm);
>   
>   	ret = intel_uncore_setup_mmio(gt->uncore, phys_addr);
>   	if (ret)
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> index 64aa2ba624fc..ff9251807f5c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> @@ -144,20 +144,6 @@ struct intel_gt {
>   	struct intel_wakeref wakeref;
>   	atomic_t user_wakeref;
>   
> -	/**
> -	 *  Protects access to lmem usefault list.
> -	 *  It is required, if we are outside of the runtime suspend path,
> -	 *  access to @lmem_userfault_list requires always first grabbing the
> -	 *  runtime pm, to ensure we can't race against runtime suspend.
> -	 *  Once we have that we also need to grab @lmem_userfault_lock,
> -	 *  at which point we have exclusive access.
> -	 *  The runtime suspend path is special since it doesn't really hold any locks,
> -	 *  but instead has exclusive access by virtue of all other accesses requiring
> -	 *  holding the runtime pm wakeref.
> -	 */
> -	struct mutex lmem_userfault_lock;
> -	struct list_head lmem_userfault_list;
> -
>   	struct list_head closed_vma;
>   	spinlock_t closed_lock; /* guards the list of closed_vma */
>   
> @@ -173,9 +159,6 @@ struct intel_gt {
>   	 */
>   	intel_wakeref_t awake;
>   
> -	/* Manual runtime pm autosuspend delay for user GGTT/lmem mmaps */
> -	struct intel_wakeref_auto userfault_wakeref;
> -
>   	u32 clock_frequency;
>   	u32 clock_period_ns;
>   
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 55d605c0c55d..299f94a9fb87 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -843,7 +843,7 @@ void i915_gem_runtime_suspend(struct drm_i915_private *i915)
>   		__i915_gem_object_release_mmap_gtt(obj);
>   
>   	list_for_each_entry_safe(obj, on,
> -				 &to_gt(i915)->lmem_userfault_list, userfault_link)
> +				 &i915->runtime_pm.lmem_userfault_list, userfault_link)
>   		i915_gem_object_runtime_pm_release_mmap_offset(obj);
>   
>   	/*
> @@ -1227,8 +1227,6 @@ void i915_gem_driver_remove(struct drm_i915_private *dev_priv)
>   	struct intel_gt *gt;
>   	unsigned int i;
>   
> -	intel_wakeref_auto_fini(&to_gt(dev_priv)->userfault_wakeref);
> -
>   	i915_gem_suspend_late(dev_priv);
>   	for_each_gt(gt, dev_priv, i)
>   		intel_gt_driver_remove(gt);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 744cca507946..bb74d4975cc8 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -633,6 +633,8 @@ void intel_runtime_pm_driver_release(struct intel_runtime_pm *rpm)
>   						     runtime_pm);
>   	int count = atomic_read(&rpm->wakeref_count);
>   
> +	intel_wakeref_auto_fini(&rpm->userfault_wakeref);
> +
>   	drm_WARN(&i915->drm, count,
>   		 "i915 raw-wakerefs=%d wakelocks=%d on cleanup\n",
>   		 intel_rpm_raw_wakeref_count(count),
> @@ -652,4 +654,7 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm *rpm)
>   	rpm->available = HAS_RUNTIME_PM(i915);
>   
>   	init_intel_runtime_pm_wakeref(rpm);
> +	INIT_LIST_HEAD(&rpm->lmem_userfault_list);
> +	mutex_init(&rpm->lmem_userfault_lock);
> +	intel_wakeref_auto_init(&rpm->userfault_wakeref, rpm);
>   }
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
> index d9160e3ff4af..d0c04af2a6f3 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> @@ -53,6 +53,28 @@ struct intel_runtime_pm {
>   	bool irqs_enabled;
>   	bool no_wakeref_tracking;
>   
> +	/**

Nit: that's not properly formatted kernel-doc.

Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>

> +	 *  Protects access to lmem usefault list.
> +	 *  It is required, if we are outside of the runtime suspend path,
> +	 *  access to @lmem_userfault_list requires always first grabbing the
> +	 *  runtime pm, to ensure we can't race against runtime suspend.
> +	 *  Once we have that we also need to grab @lmem_userfault_lock,
> +	 *  at which point we have exclusive access.
> +	 *  The runtime suspend path is special since it doesn't really hold any locks,
> +	 *  but instead has exclusive access by virtue of all other accesses requiring
> +	 *  holding the runtime pm wakeref.
> +	 */
> +	struct mutex lmem_userfault_lock;
> +
> +	/*
> +	 *  Keep list of userfaulted gem obj, which require to release their
> +	 *  mmap mappings at runtime suspend path.
> +	 */
> +	struct list_head lmem_userfault_list;
> +
> +	/* Manual runtime pm autosuspend delay for user GGTT/lmem mmaps */
> +	struct intel_wakeref_auto userfault_wakeref;
> +
>   #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
>   	/*
>   	 * To aide detection of wakeref leaks and general misuse, we