Re: [PATCH 10/15] drm/i915: disable runtime PM until delayed RPS/RC6 enabling completes

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

On Wed, 2014-04-09 at 16:19 +0200, Daniel Vetter wrote:
> On Tue, Apr 08, 2014 at 07:57:51PM +0300, Imre Deak wrote:
> > At least on some platforms we depend on RC6 being enabled for RPM, so
> > disable RPM until the delayed RC6 enabling completes. For simplicity
> > don't differentiate between platforms, those that don't have this
> > dependency will enable RC6 only rarely during driver init and system
> > resume.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c  |  5 ++++-
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c  | 17 +++++++++++++++++
> >  3 files changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 8dee34c..0948228 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -755,7 +755,7 @@ int i915_reset(struct drm_device *dev)
> >  		 * of re-init after reset. */
> >  		if (INTEL_INFO(dev)->gen > 5) {
> >  			mutex_lock(&dev->struct_mutex);
> > -			intel_enable_gt_powersave(dev);
> > +			intel_reset_gt_powersave(dev);
> >  			mutex_unlock(&dev->struct_mutex);
> >  		}
> >  
> > @@ -958,6 +958,9 @@ static int intel_runtime_suspend(struct device *device)
> >  	struct drm_device *dev = pci_get_drvdata(pdev);
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > +	if (WARN_ON_ONCE(!dev_priv->rps.enabled))
> > +		return -ENODEV;
> > +
> >  	WARN_ON(!HAS_RUNTIME_PM(dev));
> >  	assert_force_wake_inactive(dev_priv);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 42762b7..7f4e873 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -901,6 +901,7 @@ void intel_init_gt_powersave(struct drm_device *dev);
> >  void intel_cleanup_gt_powersave(struct drm_device *dev);
> >  void intel_enable_gt_powersave(struct drm_device *dev);
> >  void intel_disable_gt_powersave(struct drm_device *dev);
> > +void intel_reset_gt_powersave(struct drm_device *dev);
> >  void ironlake_teardown_rc6(struct drm_device *dev);
> >  void gen6_update_ring_freq(struct drm_device *dev);
> >  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 5728238..c082936 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4519,6 +4519,8 @@ static void intel_gen6_powersave_work(struct work_struct *work)
> >  	}
> >  	dev_priv->rps.enabled = true;
> >  	mutex_unlock(&dev_priv->rps.hw_lock);
> > +
> > +	intel_runtime_pm_put(dev_priv);
> >  }
> >  
> >  void intel_enable_gt_powersave(struct drm_device *dev)
> > @@ -4534,12 +4536,27 @@ void intel_enable_gt_powersave(struct drm_device *dev)
> >  		 * PCU communication is slow and this doesn't need to be
> >  		 * done at any specific time, so do this out of our fast path
> >  		 * to make resume and init faster.
> > +		 *
> > +		 * On platforms like Valleyview we depend on the HW RC6 power
> > +		 * context save/restore mechanism when entering D3 through
> > +		 * runtime PM suspend. So disable RPM until RPS/RC6 is
> > +		 * properly setup.
> >  		 */
> 
> I'm pretty sure that depency also exists on other platforms since I did
> wonder quite a bit how the current runtime pm stuff resurrects the render
> state ;-)

Yes, that's possible, I haven't checked this for other platforms, but
they should be covered too after this patch. Well, we'll still need to
re-enable RPS for those during RPM resume. Perhaps that can be done in
the context of the refactoring work you asked for 14/15.

> Imo the right approach here is to grab a runtime pm reference in the
> function that launches the async work and drop it again when that work has
> completed.

Do you mean something different than what this patch does? I get here
the RPM reference when rps.delayed_resume_work is scheduled and drop it
when the work is completed. Note that after we enable RPM for VLV we
have to do this same sequence when re-enabling RPS from runtime resume,
hence the _noresume version.

> We already temporarily disable runtime pm in driver load and
> suspend/resume code, so that should cover us.

As stated above, we need the same re-enabling sequence during runtime
resume.

--Imre 


> -Daniel
> 
> 
> > +		if (!cancel_delayed_work(&dev_priv->rps.delayed_resume_work))
> > +			intel_runtime_pm_get_noresume(dev_priv);
> >  		schedule_delayed_work(&dev_priv->rps.delayed_resume_work,
> >  				      round_jiffies_up_relative(HZ));
> >  	}
> >  }
> >  
> > +void intel_reset_gt_powersave(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	dev_priv->rps.enabled = false;
> > +	intel_enable_gt_powersave(dev);
> > +}
> > +
> >  static void ibx_init_clock_gating(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>