Re: [Intel-gfx] [PATCH v2 07/26] drm/i915: Add functions to get a power well's state/name/domains/mask/refcount

["Hogander, Jouni" <jouni.hogander@intel.com>]

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@intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  .../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
> >