[Intel-gfx] [PATCH 05/41] drm/i915: Move fence cancellation to runtime suspend

[Chris Wilson]

On Fri, Oct 21, 2016 at 03:48:27PM +0300, Imre Deak wrote:
> On to, 2016-10-20 at 16:03 +0100, Chris Wilson wrote:
> > At the moment, we have dependency on the RPM as a barrier itself in both
> > i915_gem_release_all_mmaps() and i915_gem_restore_fences().
> > i915_gem_restore_fences() is also called along !runtime pm paths, but we
> > can move the markup of lost fences alongside releasing the mmaps into a
> > common i915_gem_runtime_suspend(). This has the advantage of locating
> > all the tricky barrier dependencies into one location.
> > 
> > Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c       |  6 ++----
> >  drivers/gpu/drm/i915/i915_drv.h       |  3 ++-
> >  drivers/gpu/drm/i915/i915_gem.c       | 25 +++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/i915_gem_fence.c | 12 +++++-------
> >  4 files changed, 32 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 885d33f341f3..99e4e044e958 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -2278,10 +2278,8 @@ static int vlv_resume_prepare(struct drm_i915_private *dev_priv,
> >  
> >  	vlv_check_no_gt_access(dev_priv);
> >  
> > -	if (rpm_resume) {
> > +	if (rpm_resume)
> >  		intel_init_clock_gating(dev);
> > -		i915_gem_restore_fences(dev);
> > -	}
> >  
> >  	return ret;
> >  }
> > @@ -2307,7 +2305,7 @@ static int intel_runtime_suspend(struct device *kdev)
> >  	 * We are safe here against re-faults, since the fault handler takes
> >  	 * an RPM reference.
> >  	 */
> > -	i915_gem_release_all_mmaps(dev_priv);
> > +	i915_gem_runtime_suspend(dev_priv);
> >  
> >  	intel_guc_suspend(dev);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c388361ad717..9434734176a3 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3130,9 +3130,10 @@ void i915_vma_destroy(struct i915_vma *vma);
> >  
> >  int i915_gem_object_unbind(struct drm_i915_gem_object *obj);
> >  int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
> > -void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
> >  void i915_gem_release_mmap(struct drm_i915_gem_object *obj);
> >  
> > +void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv);
> > +
> >  int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
> >  
> >  static inline int __sg_page_count(struct scatterlist *sg)
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 64a88ce4b3c6..41088c31ada6 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1952,10 +1952,10 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
> >  	intel_runtime_pm_put(i915);
> >  }
> >  
> > -void
> > -i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
> > +void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
> >  {
> >  	struct drm_i915_gem_object *obj, *on;
> > +	int i;
> >  
> >  	/*
> >  	 * Only called during RPM suspend. All users of the userfault_list
> > @@ -1970,6 +1970,27 @@ i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv)
> >  		drm_vma_node_unmap(&obj->base.vma_node,
> >  				   obj->base.dev->anon_inode->i_mapping);
> >  	}
> > +
> > +	/* 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
> > +	 * down! All other fences will be reacquired by the user upon waking.
> > +	 */
> > +	for (i = 0; i < dev_priv->num_fence_regs; i++) {
> > +		struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i];
> > +		struct i915_vma *vma;
> > +
> > +		if (WARN_ON(reg->pin_count))
> > +			continue;
> > +
> > +		vma = fetch_and_zero(®->vma);
> > +		if (!vma)
> > +			continue;
> > +
> > +		GEM_BUG_ON(!list_empty(&vma->obj->userfault_link));
> > +
> > +		list_move(®->link, &dev_priv->mm.fence_list);
> > +		vma->fence = NULL;
> 
> I suppose the obj can be still bound, so shouldn't the fence reg itself
> be zeroed as well? Or are you counting with these regs getting reset
> due to suspend? Not sure if that's guaranteed.

No, they would be rewritten before use to a bound object. The problem lies
not with the current set of objects, but a new object that is allocated
into the region of the stale fence.

Hmm. rather than removing the link between the object and fence, all I
need to do is mark the fence as dirty... Then if we relinquish the fence
later (rpm will be active) we will clear it. If we try to use the
object, we will rewrite the fence register.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre