Re: [PATCH 15/16] drm/i915: Power down any power well left on by BIOS

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

On Fri, Oct 12, 2018 at 02:52:17PM -0700, José Roberto de Souza wrote:
> Just not enable power wells is not enough as BIOS/firmware can turn
> on some power wells during boot, so is needed disable those to save
> power and to avoid mismatch state errors in
> intel_power_domains_verify_state().
> So here disabling every non-real power well first as it could have
> some dependency in a real power well and then disabling all power
> wells in reverse(power well 2 depends on power well 1 and so on)
> other as required by spec.

You can't disable a power well while the function depending on it is
still on, that would lead to a hang. Also some power wells can only be
enabled/disabled at a specific place in the modeset sequence, so
disabling them here would lead to timeouts. The correct way to get power
wells disbled is to disable the corresponding functions by doing a
modeset disabling any active outputs.

> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 59 +++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 56c65d921acd..0f5016b74228 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -3785,6 +3785,61 @@ static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
>  
>  static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv);
>  
> +static void
> +intel_power_domains_disable_leftovers(struct drm_i915_private *dev_priv)
> +{
> +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +	struct i915_power_well *power_well;
> +	int i;
> +
> +	mutex_lock(&power_domains->lock);
> +
> +	/* Disable everything that is enabled and is not a HW power_well */
> +	for_each_power_well(dev_priv, power_well) {
> +		WARN_ON(power_well->count);
> +
> +		/*
> +		 * Power wells not belonging to any domain (like the MISC_IO
> +		 * and PW1 power wells) are under FW control, so ignore them,
> +		 * since their state can change asynchronously.
> +		 */
> +		if (!power_well->desc->domains || power_well->desc->always_on)
> +			continue;
> +
> +		if (power_well->desc->id != DISP_PW_ID_NONE)
> +			continue;
> +
> +		if (!power_well->hw_enabled)
> +			continue;
> +
> +		intel_power_well_disable(dev_priv, power_well);
> +	}
> +
> +	/* Disabled HW power wells in reverse order, so power well 2 is
> +	 * disabled before power well 1 and so on as required by spec.
> +	 */
> +	for (i = power_domains->power_well_count - 1; i >= 0; i--) {
> +		power_well = &power_domains->power_wells[i];
> +
> +		WARN_ON(power_well->count);
> +
> +		if (!power_well->desc->domains || power_well->desc->always_on)
> +			continue;
> +
> +		if (power_well->desc->id == DISP_PW_ID_NONE)
> +			continue;
> +
> +		if (!power_well->hw_enabled)
> +			continue;
> +
> +		intel_power_well_disable(dev_priv, power_well);
> +	}
> +
> +	mutex_unlock(&power_domains->lock);
> +
> +	intel_power_domains_verify_state(dev_priv);
> +}
> +
>  /**
>   * intel_power_domains_init_hw - initialize hardware power domain state
>   * @dev_priv: i915 device instance
> @@ -3838,6 +3893,10 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
>  		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
>  	intel_power_domains_sync_hw(dev_priv);
>  
> +	/* Disable everything left enabled by BIOS/firmware */
> +	if (!INTEL_INFO(dev_priv)->num_pipes)
> +		intel_power_domains_disable_leftovers(dev_priv);
> +
>  	power_domains->initializing = false;
>  }
>  
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx