| Message ID | 20181012215218.5119-15-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/16] drm/i915: Properly set PCH as NOP when display is disabled | expand |
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
On Mon, 2018-10-15 at 14:06 +0300, Imre Deak wrote: > 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. Do a modeset disabling would require initialize some display stuff and that is what this PR is avoiding when display is disabled by parameter. Also I did not reproduced any hang and CI did not too as somes IGT tests load i915 with display off. > > > 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
On Tue, Oct 16, 2018 at 03:05:35AM +0300, Souza, Jose wrote: > On Mon, 2018-10-15 at 14:06 +0300, Imre Deak wrote: > > 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. > > Do a modeset disabling would require initialize some display stuff and > that is what this PR is avoiding when display is disabled by parameter. Right, but it's not the good approach to add support for the .disable_display parameter. A proper modeset disable sequence has to be run, we can't simply disable power while dependent functionality is still active as I wrote. Chris has posted a patch to do such a disable sequence we would need something along that line. > Also I did not reproduced any hang and CI did not too as somes IGT > tests load i915 with display off. > > > > > > 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
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; }
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. 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(+)