Re: [Intel-gfx] [PATCH] drm/i915: Disable unused power wells left enabled by BIOS

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Feb 02, 2022 at 12:42:49PM +0200, Imre Deak wrote:
> Make sure all unused power wells left enabled by BIOS get disabled
> during driver loading and system resume.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5028
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  |  2 ++
>  .../drm/i915/display/intel_display_power.c    | 31 +++++++++++++++++++
>  .../drm/i915/display/intel_display_power.h    |  1 +
>  3 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c431076f98a15..df347329d90e5 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -10664,6 +10664,8 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
>  	}
>  
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT, wakeref);
> +
> +	intel_power_domains_sanitize_state(dev_priv);

One slight concern is intel_vga_redisable() which is called after
this during resume. Dunno if there's any way we could end up
disabling the power well that houses the VGA logic before actually 
turning off the VGA plane?

Hmm. Maybe we should just reorder the intel_modeset_setup_hw_state()
vs. intel_vga_redisable() anyway. We already do those in the opposite
order during driver probe. No idea why we have a different order for
resume.

Apart from that this seems sane enough to me.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  void intel_display_resume(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 369317805d245..d2102cc17bb4c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -6213,6 +6213,37 @@ void intel_power_domains_driver_remove(struct drm_i915_private *i915)
>  	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
>  }
>  
> +/**
> + * intel_power_domains_sanitize_state - sanitize power domains state
> + * @i915: i915 device instance
> + *
> + * Sanitize the power domains state during driver loading and system resume.
> + * The function will disable all display power wells that BIOS has enabled
> + * without a user for it (any user for a power well has taken a reference
> + * on it by the time this function is called, after the state of all the
> + * pipe, encoder, etc. HW resources have been sanitized).
> + */
> +void intel_power_domains_sanitize_state(struct drm_i915_private *i915)
> +{
> +	struct i915_power_domains *power_domains = &i915->power_domains;
> +	struct i915_power_well *power_well;
> +
> +	mutex_lock(&power_domains->lock);
> +
> +	for_each_power_well_reverse(i915, power_well) {
> +		if (power_well->desc->always_on || power_well->count ||
> +		    !power_well->desc->ops->is_enabled(i915, power_well))
> +			continue;
> +
> +		drm_dbg_kms(&i915->drm,
> +			    "BIOS left unused %s power well enabled, disabling it\n",
> +			    power_well->desc->name);
> +		intel_power_well_disable(i915, power_well);
> +	}
> +
> +	mutex_unlock(&power_domains->lock);
> +}
> +
>  /**
>   * intel_power_domains_enable - enable toggling of display power wells
>   * @i915: i915 device instance
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> index 686d18eaa24c8..a985f0e7ef78b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -219,6 +219,7 @@ void intel_power_domains_disable(struct drm_i915_private *dev_priv);
>  void intel_power_domains_suspend(struct drm_i915_private *dev_priv,
>  				 enum i915_drm_suspend_mode);
>  void intel_power_domains_resume(struct drm_i915_private *dev_priv);
> +void intel_power_domains_sanitize_state(struct drm_i915_private *dev_priv);
>  
>  void intel_display_power_suspend_late(struct drm_i915_private *i915);
>  void intel_display_power_resume_early(struct drm_i915_private *i915);
> -- 
> 2.27.0

-- 
Ville Syrjälä
Intel