Re: [PATCH v2 19/25] drm/i915: reinit GT power save during resume

[Imre Deak <imre.deak@intel.com>]

On Wed, 2014-04-16 at 20:46 +0300, Ville Syrjälä wrote:
> On Mon, Apr 14, 2014 at 08:24:40PM +0300, Imre Deak wrote:
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index b87109c..1f88917 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -920,6 +920,7 @@ static int intel_runtime_suspend(struct device *device)
> >  	DRM_DEBUG_KMS("Suspending device\n");
> >  
> >  	intel_runtime_pm_disable_interrupts(dev);
> > +	cancel_work_sync(&dev_priv->rps.work);
> 
> What happens if the rps work still runs after
> intel_runtime_pm_disable_interrupts()? To me it looks like it can hit
> the WARN in snb_update_pm_irq(), or there's a race and it'll miss the
> dev_priv->pm.irqs_disabled update and unmask something in PMIMR.
> 
> Hmm, I guess the same issue already exists in intel_disable_gt_powersave().
> 
> But since it's just PMIMR I guess it's not really dangerous since IER
> isn't touched so we don't actually get any interrupts. But it feels a bit
> fragile.

Ok, this was subtle thanks for pointing it out.

I see now that the race you describe is present already before this
change both here and on the system suspend/driver cleanup path calling
intel_disable_gt_powersave(). I think it's not an issue during 
intel_disable_gt_powersave(), since it's called with all i915 interrupts
disabled.

Here we may get that WARN right after the
intel_runtime_pm_disable_interrupts() call, but other than that it is
the same scenario as with intel_disable_gt_powersave(), since we disable
all interrupts.

Otoh, it's also guaranteed that we don't get any RPS interrupts at this
point, since we get here only after gen6_rps_idle() has been called
after which the HW is supposed to stay quiescent with the minimum RPS
freq set. The next possible RPS interrupt is only at a subsequent GT
command we submit, but before that we'll get an RPM ref. So what I'd
suggest here to avoid the WARN (and the above race it entails) is to
change the order of intel_runtime_pm_disable_interrupts() and
cancel_work_sync(). After cancel_work_sync() it's guaranteed that the
work won't be rearmed since we can't get any RPS interrupts here. I
could also add a comment explaining this.

--Imre

> 
> >  
> >  	if (IS_GEN6(dev))
> >  		;
> > @@ -968,6 +969,7 @@ static int intel_runtime_resume(struct device *device)
> >  
> >  	i915_gem_init_swizzling(dev);
> >  	gen6_update_ring_freq(dev);
> > +	intel_reset_gt_powersave(dev);
> >  
> >  	intel_runtime_pm_restore_interrupts(dev);
> >  
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx