On Fri, 2022-02-11 at 17:29 +0200, Imre Deak wrote:
> On Fri, Feb 11, 2022 at 04:26:27PM +0200, Hogander, Jouni wrote:
> > On Tue, 2022-02-08 at 13:36 +0200, Imre Deak wrote:
> > > Add functions to get a power well's actual- and cached-enabled
> > > state,
> > > name, domain mask and refcount, as a step towards making the low-
> > > level
> > > power well internals (i915_power_well_ops/desc structs) hidden.
> >
> > It's not really in scope of this patch, but still: Why this cached-
> > enabled state is needed on the first hand? Are we expecting seeing
> > hw_state as enabled while count == 0 or vice versa?
>
> It was added for VLV/CHV where PUNIT accesses to determine the actual
> HW
> state had too much overhead (20ms per access). After the initial
> sync_hw() call for the given power well and the read-out of its
> enabled
> HW state during driver loading and system resume, the cached value
> provides the state regardless of the refcount of the power well.
> During
> the sanitization of pipes/encoders etc and during the verification of
> HW
> vs. SW state after modesets, the driver gets (multiple) if_enabled()
> reference where the cached value should speed things up.
>
> For debugging purposes (intel_power_domains_verify_state()) it still
> makes sense to check the actual HW state, hence the presence of
> functions to get both the cached and non-cached state.
Thank you for the explanation.
>
> > > No functional change.
> > >
> > > Suggested-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > ---
> > > .../drm/i915/display/intel_display_power.c | 69 +++++++++--
> > > ----
> > > ----
> > > .../i915/display/intel_display_power_well.c | 31 +++++++++
> > > .../i915/display/intel_display_power_well.h | 7 ++
> > > 3 files changed, 72 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > index 056965248a3b2..321b271c4b674 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > @@ -191,10 +191,10 @@ bool
> > > __intel_display_power_is_enabled(struct
> > > drm_i915_private *dev_priv,
> > > is_enabled = true;
> > >
> > > for_each_power_domain_well_reverse(dev_priv, power_well,
> > > BIT_ULL(domain)) {
> > > - if (power_well->desc->always_on)
> > > + if (intel_power_well_is_always_on(power_well))
> > > continue;
> > >
> > > - if (!power_well->hw_enabled) {
> > > + if
> > > (!intel_power_well_is_enabled_cached(power_well)) {
> > > is_enabled = false;
> > > break;
> > > }
> > > @@ -330,7 +330,7 @@ static void
> > > hsw_wait_for_power_well_enable(struct
> > > drm_i915_private *dev_priv,
> > > if (intel_de_wait_for_set(dev_priv, regs->driver,
> > > HSW_PWR_WELL_CTL_STATE(pw_idx),
> > > 1)) {
> > > drm_dbg_kms(&dev_priv->drm, "%s power well enable
> > > timeout\n",
> > > - power_well->desc->name);
> > > + intel_power_well_name(power_well));
> > >
> > > drm_WARN_ON(&dev_priv->drm, !timeout_expected);
> > >
> > > @@ -378,7 +378,7 @@ static void
> > > hsw_wait_for_power_well_disable(struct drm_i915_private
> > > *dev_priv,
> > >
> > > drm_dbg_kms(&dev_priv->drm,
> > > "%s forced on (bios:%d driver:%d kvmr:%d
> > > debug:%d)\n",
> > > - power_well->desc->name,
> > > + intel_power_well_name(power_well),
> > > !!(reqs & 1), !!(reqs & 2), !!(reqs & 4),
> > > !!(reqs &
> > > 8));
> > > }
> > >
> > > @@ -967,8 +967,7 @@ void
> > > intel_display_power_set_target_dc_state(struct drm_i915_private
> > > *dev_priv,
> > > if (state == dev_priv->dmc.target_dc_state)
> > > goto unlock;
> > >
> > > - dc_off_enabled = power_well->desc->ops-
> > > >is_enabled(dev_priv,
> > > - power_we
> > > ll);
> > > + dc_off_enabled = intel_power_well_is_enabled(dev_priv,
> > > power_well);
> > > /*
> > > * If DC off power well is disabled, need to enable and
> > > disable
> > > the
> > > * DC off power well to effect target DC state.
> > > @@ -1090,17 +1089,17 @@ static void
> > > bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv)
> > > struct i915_power_well *power_well;
> > >
> > > power_well = lookup_power_well(dev_priv,
> > > BXT_DISP_PW_DPIO_CMN_A);
> > > - if (power_well->count > 0)
> > > + if (intel_power_well_refcount(power_well) > 0)
> > > bxt_ddi_phy_verify_state(dev_priv, power_well-
> > > >desc-
> > > > bxt.phy);
> > >
> > > power_well = lookup_power_well(dev_priv,
> > > VLV_DISP_PW_DPIO_CMN_BC);
> > > - if (power_well->count > 0)
> > > + if (intel_power_well_refcount(power_well) > 0)
> > > bxt_ddi_phy_verify_state(dev_priv, power_well-
> > > >desc-
> > > > bxt.phy);
> > >
> > > if (IS_GEMINILAKE(dev_priv)) {
> > > power_well = lookup_power_well(dev_priv,
> > > GLK_DISP_PW_DPIO_CMN
> > > _C);
> > > - if (power_well->count > 0)
> > > + if (intel_power_well_refcount(power_well) > 0)
> > > bxt_ddi_phy_verify_state(dev_priv,
> > > power_well->desc-
> > > > bxt.phy);
> > > }
> > > @@ -1226,7 +1225,7 @@ static bool
> > > i830_pipes_power_well_enabled(struct drm_i915_private *dev_priv,
> > > static void i830_pipes_power_well_sync_hw(struct
> > > drm_i915_private
> > > *dev_priv,
> > > struct i915_power_well
> > > *power_well)
> > > {
> > > - if (power_well->count > 0)
> > > + if (intel_power_well_refcount(power_well) > 0)
> > > i830_pipes_power_well_enable(dev_priv, power_well);
> > > else
> > > i830_pipes_power_well_disable(dev_priv,
> > > power_well);
> > > @@ -1499,7 +1498,7 @@ static void assert_chv_phy_status(struct
> > > drm_i915_private *dev_priv)
> > > PHY_STATUS_SPLINE_LDO(DPIO_PHY
> > > 1,
> > > DPIO_CH0, 0) |
> > > PHY_STATUS_SPLINE_LDO(DPIO_PHY
> > > 1,
> > > DPIO_CH0, 1));
> > >
> > > - if (cmn_bc->desc->ops->is_enabled(dev_priv, cmn_bc)) {
> > > + if (intel_power_well_is_enabled(dev_priv, cmn_bc)) {
> > > phy_status |= PHY_POWERGOOD(DPIO_PHY0);
> > >
> > > /* this assumes override is only used to enable
> > > lanes
> > > */
> > > @@ -1540,7 +1539,7 @@ static void assert_chv_phy_status(struct
> > > drm_i915_private *dev_priv)
> > > phy_status |=
> > > PHY_STATUS_SPLINE_LDO(DPIO_PHY0,
> > > DPIO_CH1, 1);
> > > }
> > >
> > > - if (cmn_d->desc->ops->is_enabled(dev_priv, cmn_d)) {
> > > + if (intel_power_well_is_enabled(dev_priv, cmn_d)) {
> > > phy_status |= PHY_POWERGOOD(DPIO_PHY1);
> > >
> > > /* this assumes override is only used to enable
> > > lanes
> > > */
> > > @@ -3334,12 +3333,10 @@ bool
> > > intel_display_power_well_is_enabled(struct drm_i915_private
> > > *dev_priv,
> > > enum i915_power_well_id
> > > power_well_id)
> > > {
> > > struct i915_power_well *power_well;
> > > - bool ret;
> > >
> > > power_well = lookup_power_well(dev_priv, power_well_id);
> > > - ret = power_well->desc->ops->is_enabled(dev_priv,
> > > power_well);
> > >
> > > - return ret;
> > > + return intel_power_well_is_enabled(dev_priv, power_well);
> > > }
> > >
> > > static const struct i915_power_well_desc skl_power_wells[] = {
> > > @@ -3909,7 +3906,7 @@ static void
> > > tgl_tc_cold_off_power_well_sync_hw(struct drm_i915_private
> > > *i915,
> > > struct i915_power_well
> > > *power_well)
> > > {
> > > - if (power_well->count > 0)
> > > + if (intel_power_well_refcount(power_well) > 0)
> > > tgl_tc_cold_off_power_well_enable(i915,
> > > power_well);
> > > else
> > > tgl_tc_cold_off_power_well_disable(i915,
> > > power_well);
> > > @@ -3923,7 +3920,7 @@
> > > tgl_tc_cold_off_power_well_is_enabled(struct
> > > drm_i915_private *dev_priv,
> > > * Not the correctly implementation but there is no way to
> > > just
> > > read it
> > > * from PCODE, so returning count to avoid state mismatch
> > > errors
> > > */
> > > - return power_well->count;
> > > + return intel_power_well_refcount(power_well);
> > > }
> > >
> > > static const struct i915_power_well_ops tgl_tc_cold_off_ops = {
> > > @@ -5729,7 +5726,7 @@ static void chv_phy_control_init(struct
> > > drm_i915_private *dev_priv)
> > > * override and set the lane powerdown bits accding to the
> > > * current lane status.
> > > */
> > > - if (cmn_bc->desc->ops->is_enabled(dev_priv, cmn_bc)) {
> > > + if (intel_power_well_is_enabled(dev_priv, cmn_bc)) {
> > > u32 status = intel_de_read(dev_priv, DPLL(PIPE_A));
> > > unsigned int mask;
> > >
> > > @@ -5760,7 +5757,7 @@ static void chv_phy_control_init(struct
> > > drm_i915_private *dev_priv)
> > > dev_priv->chv_phy_assert[DPIO_PHY0] = true;
> > > }
> > >
> > > - if (cmn_d->desc->ops->is_enabled(dev_priv, cmn_d)) {
> > > + if (intel_power_well_is_enabled(dev_priv, cmn_d)) {
> > > u32 status = intel_de_read(dev_priv,
> > > DPIO_PHY_STATUS);
> > > unsigned int mask;
> > >
> > > @@ -5796,8 +5793,8 @@ static void vlv_cmnlane_wa(struct
> > > drm_i915_private *dev_priv)
> > > lookup_power_well(dev_priv, VLV_DISP_PW_DISP2D);
> > >
> > > /* If the display might be already active skip this */
> > > - if (cmn->desc->ops->is_enabled(dev_priv, cmn) &&
> > > - disp2d->desc->ops->is_enabled(dev_priv, disp2d) &&
> > > + if (intel_power_well_is_enabled(dev_priv, cmn) &&
> > > + intel_power_well_is_enabled(dev_priv, disp2d) &&
> > > intel_de_read(dev_priv, DPIO_CTL) & DPIO_CMNRST)
> > > return;
> > >
> > > @@ -5964,12 +5961,12 @@ void
> > > intel_power_domains_sanitize_state(struct drm_i915_private *i915)
> > >
> > > 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))
> > > + !intel_power_well_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_name(power_well));
> > > intel_power_well_disable(i915, power_well);
> > > }
> > >
> > > @@ -6108,9 +6105,9 @@ static void
> > > intel_power_domains_dump_info(struct drm_i915_private *i915)
> > > enum intel_display_power_domain domain;
> > >
> > > drm_dbg(&i915->drm, "%-25s %d\n",
> > > - power_well->desc->name, power_well->count);
> > > + intel_power_well_name(power_well),
> > > intel_power_well_refcount(power_well));
> > >
> > > - for_each_power_domain(domain, power_well->desc-
> > > > domains)
> > > + for_each_power_domain(domain,
> > > intel_power_well_domains(power_well))
> > > drm_dbg(&i915->drm, " %-23s %d\n",
> > > intel_display_power_domain_str(doma
> > > in),
> > > power_domains-
> > > > domain_use_count[domain]);
> > > @@ -6143,23 +6140,25 @@ static void
> > > intel_power_domains_verify_state(struct drm_i915_private *i915)
> > > int domains_count;
> > > bool enabled;
> > >
> > > - enabled = power_well->desc->ops->is_enabled(i915,
> > > power_well);
> > > - if ((power_well->count || power_well->desc-
> > > >always_on)
> > > !=
> > > + enabled = intel_power_well_is_enabled(i915,
> > > power_well);
> > > + if ((intel_power_well_refcount(power_well) ||
> > > + intel_power_well_is_always_on(power_well)) !=
> > > enabled)
> > > drm_err(&i915->drm,
> > > "power well %s state mismatch
> > > (refcount
> > > %d/enabled %d)",
> > > - power_well->desc->name,
> > > - power_well->count, enabled);
> > > + intel_power_well_name(power_well),
> > > + intel_power_well_refcount(power_wel
> > > l),
> > > enabled);
> > >
> > > domains_count = 0;
> > > - for_each_power_domain(domain, power_well->desc-
> > > > domains)
> > > + for_each_power_domain(domain,
> > > intel_power_well_domains(power_well))
> > > domains_count += power_domains-
> > > > domain_use_count[domain];
> > >
> > > - if (power_well->count != domains_count) {
> > > + if (intel_power_well_refcount(power_well) !=
> > > domains_count) {
> > > drm_err(&i915->drm,
> > > "power well %s refcount/domain
> > > refcount
> > > mismatch "
> > > "(refcount %d/domains refcount
> > > %d)\n",
> > > - power_well->desc->name, power_well-
> > > > count,
> > > + intel_power_well_name(power_well),
> > > + intel_power_well_refcount(power_wel
> > > l),
> > > domains_count);
> > > dump_domain_info = true;
> > > }
> > > @@ -6264,10 +6263,10 @@ void intel_display_power_debug(struct
> > > drm_i915_private *i915, struct seq_file *m
> > > enum intel_display_power_domain power_domain;
> > >
> > > power_well = &power_domains->power_wells[i];
> > > - seq_printf(m, "%-25s %d\n", power_well->desc->name,
> > > - power_well->count);
> > > + seq_printf(m, "%-25s %d\n",
> > > intel_power_well_name(power_well),
> > > + intel_power_well_refcount(power_well));
> > >
> > > - for_each_power_domain(power_domain, power_well-
> > > >desc-
> > > > domains)
> > > + for_each_power_domain(power_domain,
> > > intel_power_well_domains(power_well))
> > > seq_printf(m, " %-23s %d\n",
> > > intel_display_power_domain_str(p
> > > ower
> > > _domain),
> > > power_domains-
> > > > domain_use_count[power_domain]);
> > > diff --git
> > > a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > index 63b97bcc64bc3..415ad193a8e83 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > @@ -47,3 +47,34 @@ void intel_power_well_put(struct
> > > drm_i915_private
> > > *i915,
> > > if (!--power_well->count)
> > > intel_power_well_disable(i915, power_well);
> > > }
> > > +
> > > +bool intel_power_well_is_enabled(struct drm_i915_private *i915,
> > > + struct i915_power_well
> > > *power_well)
> > > +{
> > > + return power_well->desc->ops->is_enabled(i915, power_well);
> > > +}
> > > +
> > > +bool intel_power_well_is_enabled_cached(struct i915_power_well
> > > *power_well)
> > > +{
> > > + return power_well->hw_enabled;
> > > +}
> > > +
> > > +bool intel_power_well_is_always_on(struct i915_power_well
> > > *power_well)
> > > +{
> > > + return power_well->desc->always_on;
> > > +}
> > > +
> > > +const char *intel_power_well_name(struct i915_power_well
> > > *power_well)
> > > +{
> > > + return power_well->desc->name;
> > > +}
> > > +
> > > +u64 intel_power_well_domains(struct i915_power_well *power_well)
> > > +{
> > > + return power_well->desc->domains;
> > > +}
> > > +
> > > +int intel_power_well_refcount(struct i915_power_well
> > > *power_well)
> > > +{
> > > + return power_well->count;
> > > +}
> > > diff --git
> > > a/drivers/gpu/drm/i915/display/intel_display_power_well.h
> > > b/drivers/gpu/drm/i915/display/intel_display_power_well.h
> > > index ba5bbd36f7fc0..43affbdbc48c1 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.h
> > > @@ -113,5 +113,12 @@ void intel_power_well_get(struct
> > > drm_i915_private *i915,
> > > struct i915_power_well *power_well);
> > > void intel_power_well_put(struct drm_i915_private *i915,
> > > struct i915_power_well *power_well);
> > > +bool intel_power_well_is_enabled(struct drm_i915_private *i915,
> > > + struct i915_power_well
> > > *power_well);
> > > +bool intel_power_well_is_enabled_cached(struct i915_power_well
> > > *power_well);
> > > +bool intel_power_well_is_always_on(struct i915_power_well
> > > *power_well);
> > > +const char *intel_power_well_name(struct i915_power_well
> > > *power_well);
> > > +u64 intel_power_well_domains(struct i915_power_well
> > > *power_well);
> > > +int intel_power_well_refcount(struct i915_power_well
> > > *power_well);
> > >
> > > #endif
> >
> > BR,
> >
> > Jouni Högander
> >
