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

[Daniel Vetter <daniel@ffwll.ch>]

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 ;-)

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. We already temporarily disable runtime pm in driver load and
suspend/resume code, so that should cover us.
-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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch