Re: [PATCH 1/3] drm/i915: Kill intel_runtime_pm_disable()

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On 11/06/2015 05:08 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> intel_runtime_pm_disable() takes an extra rpm reference which combined
> with the one we leak from intel_display_set_init_power() leaves the
> usage count at <original>+1 after the driver has been unloaded.
> The original ref is dropped explicitly in intel_runtime_pm_enable().
> So the next time we load the driver we can no longer do runtime PM ever.
> 
> This used to work, but
> commit 292b990e86ab ("drm/i915: Update power domains on readout.")
> broke things by not dropping the init power domain during fbdev
> teardown. Based on the comment in intel_power_domains_fini(), the
> way it used to to work wasn't intentional. As in we weren't supposed
> to drop the init power during driver unload. And since we no longer
> do, we now leak an extra rpm reference.
> 
> So fix things by throwing intel_runtime_pm_disable() to the bin, so
> that the only leaked reference comes from the init power domain.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Fixes: 292b990e86ab ("drm/i915: Update power domains on readout.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 17 -----------------
>  1 file changed, 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 1017555..bdc9ed4 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1847,21 +1847,6 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
>  	return 0;
>  }
>  
> -static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
> -{
> -	struct drm_device *dev = dev_priv->dev;
> -	struct device *device = &dev->pdev->dev;
> -
> -	if (!HAS_RUNTIME_PM(dev))
> -		return;
> -
> -	if (!intel_enable_rc6(dev))
> -		return;
> -
> -	/* Make sure we're not suspended first. */
> -	pm_runtime_get_sync(device);
> -}
> -
>  /**
>   * intel_power_domains_fini - finalizes the power domain structures
>   * @dev_priv: i915 device instance
> @@ -1872,8 +1857,6 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
>   */
>  void intel_power_domains_fini(struct drm_i915_private *dev_priv)
>  {
> -	intel_runtime_pm_disable(dev_priv);
> -
>  	/* The i915.ko module is still not prepared to be loaded when
>  	 * the power well is not enabled, so just enable it in case
>  	 * we're going to unload/reload. */
> 

Yeah I guess this is fine.  Will we still disable RPM on unload?  What's
the expected behavior here?  Cc'ing Rafael.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx