Re: [PATCH] drm/i915: use correct runtime get/put calls at init/teardown

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Thursday, September 17, 2015 02:23:32 PM Jesse Barnes wrote:
> According to the PCI docs and Rafael, we need to be using
> pm_runtime_put_noidle() and pm_runtime_get_noresume() in our init and
> teardown routines, rather than using a direct enable/disable pair (and
> we didn't even have the enable side, so never autosuspended after an
> unload).
> 
> This fixes one failure of the basic-pci-d3-state test on my BYT.  I'm
> still debugging why the device never autosuspends.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 85c35fd..1addb8a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1822,7 +1822,7 @@ static void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
>  
>  	/* Make sure we're not suspended first. */
>  	pm_runtime_get_sync(device);
> -	pm_runtime_disable(device);

That is correct IMO.  If you ensure that the usage counter is always above 0
and the device is "on", you don't need to disable runtime PM in addition to that.

> +	pm_runtime_get_noresume(device);

I'm not sure that this is needed.  You've already bumped up the usage counter.

Are you going to drop it going forward?

>  }
>  
>  /**
> @@ -2114,8 +2114,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
>  	if (!HAS_RUNTIME_PM(dev))
>  		return;
>  
> -	pm_runtime_set_active(device);
> -
>  	/*
>  	 * RPM depends on RC6 to save restore the GT HW context, so make RC6 a
>  	 * requirement.
> @@ -2130,5 +2128,6 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
>  	pm_runtime_use_autosuspend(device);
>  
>  	pm_runtime_put_autosuspend(device);
> +	pm_runtime_put_noidle(device);

That shouldn't be necessary.  The "put" is already done in pm_runtime_put_autosuspend().

>  }
>  
> 

The rule is that .probe() should do one extra decrementation of the usage
counter and .remove() should do one extra incrementation of it.  Enable/disable
should never be necessary.

Thanks,
Rafael

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