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

[Gupta, Anshuman]

> -----Original Message-----
> From: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Sent: Thursday, September 15, 2022 10:37 PM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: Auld, Matthew <matthew.auld at intel.com>; Vivi, Rodrigo
> <rodrigo.vivi at intel.com>
> Subject: Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against
> rpm
> 
> 
> 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.
That’s correct  intel_ring_pin can pin_map the lmem object.
      if (i915_vma_is_map_and_fenceable(vma)) {
                addr = (void __force *)i915_vma_pin_iomap(vma);
        } else {
                int type = i915_coherent_map_type(vma->vm->i915, vma->obj, false);

                addr = i915_gem_object_pin_map(vma->obj, type);
        }

If that is the case this RFC proposal will not work and in that case every caller of  i915_gem_object_pin_map need to grab the wakreref before
accessing the retuned address by pin_map. Any inputs from you side for any other approach.

Thanks,
Anshuman Gupta.

> 
> Regards,
> 
> Tvrtko
> 
> > Cc: Matthew Auld <matthew.auld at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: Andi Shyti <andi.shyti at linux.intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta at 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;
> >