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@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
