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?
>
> 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_well);
> + 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_PHY1,
> DPIO_CH0, 0) |
> PHY_STATUS_SPLINE_LDO(DPIO_PHY1,
> 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(domain),
> 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_well),
> 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_well),
> 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(power
> _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
