Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 15/09/2022 11:33, Anshuman Gupta wrote:
> If i915 gem obj lies in lmem, then i915_gem_object_pin_map
> need to grab a rpm wakeref to make sure gfx PCIe endpoint
> function stays in D0 state during any access to mapping
> returned by i915_gem_object_pin_map().
> Subsequently i915_gem_object_upin_map will put the wakref as well.

Another thing to check are perma pinned contexts. Follow the flow from 
intel_engine_create_pinned_context to 
intel_engine_destroy_pinned_context. If you find out that kernel (&co) 
contexts are pinned for the duration of i915 load/bind and that they use 
lmem objects, that would mean wakeref is held for the duration of i915 
loaded state. Defeating the point and making the solution effectively 
equal to just disabling RPM.

Regards,

Tvrtko

> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_object.c       |  2 ++
>   drivers/gpu/drm/i915/gem/i915_gem_object.h       |  5 +++++
>   drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 14 ++++++++++++++
>   drivers/gpu/drm/i915/gem/i915_gem_pages.c        |  8 ++++++++
>   4 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 85482a04d158..f291f990838d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -95,6 +95,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
>   	mutex_init(&obj->mm.get_page.lock);
>   	INIT_RADIX_TREE(&obj->mm.get_dma_page.radix, GFP_KERNEL | __GFP_NOWARN);
>   	mutex_init(&obj->mm.get_dma_page.lock);
> +	mutex_init(&obj->wakeref_lock);
>   }
>   
>   /**
> @@ -110,6 +111,7 @@ void __i915_gem_object_fini(struct drm_i915_gem_object *obj)
>   {
>   	mutex_destroy(&obj->mm.get_page.lock);
>   	mutex_destroy(&obj->mm.get_dma_page.lock);
> +	mutex_destroy(&obj->wakeref_lock);
>   	dma_resv_fini(&obj->base._resv);
>   }
>   
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index 7317d4102955..b31ac6e4c272 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -501,6 +501,11 @@ static inline void i915_gem_object_flush_map(struct drm_i915_gem_object *obj)
>    */
>   static inline void i915_gem_object_unpin_map(struct drm_i915_gem_object *obj)
>   {
> +	mutext_lock(obj->wakeref_lock);
> +	if (!--obj->wakeref_count)
> +		intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref);
> +	mutext_unlock(obj->wakeref_lock);
> +
>   	i915_gem_object_unpin_pages(obj);
>   }
>   
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> index 9f6b14ec189a..34aff95a1984 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -657,6 +657,20 @@ struct drm_i915_gem_object {
>   
>   		void *gvt_info;
>   	};
> +
> +	/**
> +	 * wakeref to protect the i915 lmem iomem mappings.
> +	 * We don't pin_map an object partially that makes easy
> +	 * to track the wakeref cookie, if wakeref is already held
> +	 * then we don't need to grab it again for other pin_map.
> +	 * first pin_map will grab the wakeref and last unpin_map
> +	 * will put the wakeref.
> +	 */
> +	intel_wakeref_t wakeref;
> +	unsigned int wakeref_count;
> +
> +	/** protects the wakeref_count wakeref cookie against multiple pin_map and unpin_map */
> +	struct mutex wakeref_lock;
>   };
>   
>   static inline struct drm_i915_gem_object *
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index 4df50b049cea..b638b5413280 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -370,6 +370,14 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
>   
>   	assert_object_held(obj);
>   
> +	if (i915_gem_object_is_lmem(obj)) {
> +		mutex_lock(&obj->wakeref_lock);
> +		if (!obj->wakeref_count++)
> +			obj->wakeref =
> +				intel_runtime_pm_get(&to_i915(obj->base.dev)->runtime_pm);
> +		mutex_unlock(&obj->wakeref_lock);
> +	}
> +
>   	pinned = !(type & I915_MAP_OVERRIDE);
>   	type &= ~I915_MAP_OVERRIDE;
>