From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: intel-gfx@lists.freedesktop.org,
"Thomas Hellström" <thomas.hellstrom@intel.com>,
chris.p.wilson@linux.intel.com,
"Matthew Auld" <matthew.auld@intel.com>
Subject: Re: [Intel-gfx] [PATCH v3] drm/i915/display/power: use intel_de_rmw if possible
Date: Tue, 21 Feb 2023 15:00:00 -0500 [thread overview]
Message-ID: <Y/UiwJPNMJUdKKkp@intel.com> (raw)
In-Reply-To: <20230217111836.864959-1-andrzej.hajda@intel.com>
On Fri, Feb 17, 2023 at 12:18:36PM +0100, Andrzej Hajda wrote:
> The helper makes the code more compact and readable.
>
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
pushed, thanks
> ---
> v3:
> - just rebased
> ---
> .../drm/i915/display/intel_display_power.c | 49 ++++-------
> .../i915/display/intel_display_power_well.c | 82 ++++++-------------
> 2 files changed, 39 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 7222502a760ccb..743b919bb2cfd7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -1260,9 +1260,7 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
> drm_err(&dev_priv->drm, "D_COMP RCOMP still in progress\n");
>
> if (allow_power_down) {
> - val = intel_de_read(dev_priv, LCPLL_CTL);
> - val |= LCPLL_POWER_DOWN_ALLOW;
> - intel_de_write(dev_priv, LCPLL_CTL, val);
> + intel_de_rmw(dev_priv, LCPLL_CTL, 0, LCPLL_POWER_DOWN_ALLOW);
> intel_de_posting_read(dev_priv, LCPLL_CTL);
> }
> }
> @@ -1306,9 +1304,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> drm_err(&dev_priv->drm, "LCPLL not locked yet\n");
>
> if (val & LCPLL_CD_SOURCE_FCLK) {
> - val = intel_de_read(dev_priv, LCPLL_CTL);
> - val &= ~LCPLL_CD_SOURCE_FCLK;
> - intel_de_write(dev_priv, LCPLL_CTL, val);
> + intel_de_rmw(dev_priv, LCPLL_CTL, LCPLL_CD_SOURCE_FCLK, 0);
>
> if (wait_for_us((intel_de_read(dev_priv, LCPLL_CTL) &
> LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
> @@ -1347,15 +1343,11 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> */
> static void hsw_enable_pc8(struct drm_i915_private *dev_priv)
> {
> - u32 val;
> -
> drm_dbg_kms(&dev_priv->drm, "Enabling package C8+\n");
>
> - if (HAS_PCH_LPT_LP(dev_priv)) {
> - val = intel_de_read(dev_priv, SOUTH_DSPCLK_GATE_D);
> - val &= ~PCH_LP_PARTITION_LEVEL_DISABLE;
> - intel_de_write(dev_priv, SOUTH_DSPCLK_GATE_D, val);
> - }
> + if (HAS_PCH_LPT_LP(dev_priv))
> + intel_de_rmw(dev_priv, SOUTH_DSPCLK_GATE_D,
> + PCH_LP_PARTITION_LEVEL_DISABLE, 0);
>
> lpt_disable_clkout_dp(dev_priv);
> hsw_disable_lcpll(dev_priv, true, true);
> @@ -1363,25 +1355,21 @@ static void hsw_enable_pc8(struct drm_i915_private *dev_priv)
>
> static void hsw_disable_pc8(struct drm_i915_private *dev_priv)
> {
> - u32 val;
> -
> drm_dbg_kms(&dev_priv->drm, "Disabling package C8+\n");
>
> hsw_restore_lcpll(dev_priv);
> intel_init_pch_refclk(dev_priv);
>
> - if (HAS_PCH_LPT_LP(dev_priv)) {
> - val = intel_de_read(dev_priv, SOUTH_DSPCLK_GATE_D);
> - val |= PCH_LP_PARTITION_LEVEL_DISABLE;
> - intel_de_write(dev_priv, SOUTH_DSPCLK_GATE_D, val);
> - }
> + if (HAS_PCH_LPT_LP(dev_priv))
> + intel_de_rmw(dev_priv, SOUTH_DSPCLK_GATE_D,
> + 0, PCH_LP_PARTITION_LEVEL_DISABLE);
> }
>
> static void intel_pch_reset_handshake(struct drm_i915_private *dev_priv,
> bool enable)
> {
> i915_reg_t reg;
> - u32 reset_bits, val;
> + u32 reset_bits;
>
> if (IS_IVYBRIDGE(dev_priv)) {
> reg = GEN7_MSG_CTL;
> @@ -1394,14 +1382,7 @@ static void intel_pch_reset_handshake(struct drm_i915_private *dev_priv,
> if (DISPLAY_VER(dev_priv) >= 14)
> reset_bits |= MTL_RESET_PICA_HANDSHAKE_EN;
>
> - val = intel_de_read(dev_priv, reg);
> -
> - if (enable)
> - val |= reset_bits;
> - else
> - val &= ~reset_bits;
> -
> - intel_de_write(dev_priv, reg, val);
> + intel_de_rmw(dev_priv, reg, reset_bits, enable ? reset_bits : 0);
> }
>
> static void skl_display_core_init(struct drm_i915_private *dev_priv,
> @@ -1616,7 +1597,6 @@ static void icl_display_core_init(struct drm_i915_private *dev_priv,
> {
> struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
> struct i915_power_well *well;
> - u32 val;
>
> gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>
> @@ -1668,11 +1648,10 @@ static void icl_display_core_init(struct drm_i915_private *dev_priv,
> intel_dmc_load_program(dev_priv);
>
> /* Wa_14011508470:tgl,dg1,rkl,adl-s,adl-p */
> - if (DISPLAY_VER(dev_priv) >= 12) {
> - val = DCPR_CLEAR_MEMSTAT_DIS | DCPR_SEND_RESP_IMM |
> - DCPR_MASK_LPMODE | DCPR_MASK_MAXLATENCY_MEMUP_CLR;
> - intel_de_rmw(dev_priv, GEN11_CHICKEN_DCPR_2, 0, val);
> - }
> + if (DISPLAY_VER(dev_priv) >= 12)
> + intel_de_rmw(dev_priv, GEN11_CHICKEN_DCPR_2, 0,
> + DCPR_CLEAR_MEMSTAT_DIS | DCPR_SEND_RESP_IMM |
> + DCPR_MASK_LPMODE | DCPR_MASK_MAXLATENCY_MEMUP_CLR);
>
> /* Wa_14011503030:xelpd */
> if (DISPLAY_VER(dev_priv) >= 13)
> 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 56a20bf5825b4a..6d1d3284eddd3a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> @@ -333,7 +333,6 @@ static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
> {
> const struct i915_power_well_regs *regs = power_well->desc->ops->regs;
> int pw_idx = i915_power_well_instance(power_well)->hsw.idx;
> - u32 val;
>
> if (power_well->desc->has_fuses) {
> enum skl_power_gate pg;
> @@ -356,9 +355,7 @@ static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
> gen9_wait_for_power_well_fuses(dev_priv, SKL_PG0);
> }
>
> - val = intel_de_read(dev_priv, regs->driver);
> - intel_de_write(dev_priv, regs->driver,
> - val | HSW_PWR_WELL_CTL_REQ(pw_idx));
> + intel_de_rmw(dev_priv, regs->driver, 0, HSW_PWR_WELL_CTL_REQ(pw_idx));
>
> hsw_wait_for_power_well_enable(dev_priv, power_well, false);
>
> @@ -380,14 +377,11 @@ static void hsw_power_well_disable(struct drm_i915_private *dev_priv,
> {
> const struct i915_power_well_regs *regs = power_well->desc->ops->regs;
> int pw_idx = i915_power_well_instance(power_well)->hsw.idx;
> - u32 val;
>
> hsw_power_well_pre_disable(dev_priv,
> power_well->desc->irq_pipe_mask);
>
> - val = intel_de_read(dev_priv, regs->driver);
> - intel_de_write(dev_priv, regs->driver,
> - val & ~HSW_PWR_WELL_CTL_REQ(pw_idx));
> + intel_de_rmw(dev_priv, regs->driver, HSW_PWR_WELL_CTL_REQ(pw_idx), 0);
> hsw_wait_for_power_well_disable(dev_priv, power_well);
> }
>
> @@ -411,29 +405,22 @@ icl_combo_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
> const struct i915_power_well_regs *regs = power_well->desc->ops->regs;
> int pw_idx = i915_power_well_instance(power_well)->hsw.idx;
> enum phy phy = icl_aux_pw_to_phy(dev_priv, power_well);
> - u32 val;
>
> drm_WARN_ON(&dev_priv->drm, !IS_ICELAKE(dev_priv));
>
> - val = intel_de_read(dev_priv, regs->driver);
> - intel_de_write(dev_priv, regs->driver,
> - val | HSW_PWR_WELL_CTL_REQ(pw_idx));
> + intel_de_rmw(dev_priv, regs->driver, 0, HSW_PWR_WELL_CTL_REQ(pw_idx));
>
> - if (DISPLAY_VER(dev_priv) < 12) {
> - val = intel_de_read(dev_priv, ICL_PORT_CL_DW12(phy));
> - intel_de_write(dev_priv, ICL_PORT_CL_DW12(phy),
> - val | ICL_LANE_ENABLE_AUX);
> - }
> + if (DISPLAY_VER(dev_priv) < 12)
> + intel_de_rmw(dev_priv, ICL_PORT_CL_DW12(phy),
> + 0, ICL_LANE_ENABLE_AUX);
>
> hsw_wait_for_power_well_enable(dev_priv, power_well, false);
>
> /* Display WA #1178: icl */
> if (pw_idx >= ICL_PW_CTL_IDX_AUX_A && pw_idx <= ICL_PW_CTL_IDX_AUX_B &&
> - !intel_port_is_edp(dev_priv, (enum port)phy)) {
> - val = intel_de_read(dev_priv, ICL_AUX_ANAOVRD1(pw_idx));
> - val |= ICL_AUX_ANAOVRD1_ENABLE | ICL_AUX_ANAOVRD1_LDO_BYPASS;
> - intel_de_write(dev_priv, ICL_AUX_ANAOVRD1(pw_idx), val);
> - }
> + !intel_port_is_edp(dev_priv, (enum port)phy))
> + intel_de_rmw(dev_priv, ICL_AUX_ANAOVRD1(pw_idx),
> + 0, ICL_AUX_ANAOVRD1_ENABLE | ICL_AUX_ANAOVRD1_LDO_BYPASS);
> }
>
> static void
> @@ -443,17 +430,12 @@ icl_combo_phy_aux_power_well_disable(struct drm_i915_private *dev_priv,
> const struct i915_power_well_regs *regs = power_well->desc->ops->regs;
> int pw_idx = i915_power_well_instance(power_well)->hsw.idx;
> enum phy phy = icl_aux_pw_to_phy(dev_priv, power_well);
> - u32 val;
>
> drm_WARN_ON(&dev_priv->drm, !IS_ICELAKE(dev_priv));
>
> - val = intel_de_read(dev_priv, ICL_PORT_CL_DW12(phy));
> - intel_de_write(dev_priv, ICL_PORT_CL_DW12(phy),
> - val & ~ICL_LANE_ENABLE_AUX);
> + intel_de_rmw(dev_priv, ICL_PORT_CL_DW12(phy), ICL_LANE_ENABLE_AUX, 0);
>
> - val = intel_de_read(dev_priv, regs->driver);
> - intel_de_write(dev_priv, regs->driver,
> - val & ~HSW_PWR_WELL_CTL_REQ(pw_idx));
> + intel_de_rmw(dev_priv, regs->driver, HSW_PWR_WELL_CTL_REQ(pw_idx), 0);
>
> hsw_wait_for_power_well_disable(dev_priv, power_well);
> }
> @@ -515,19 +497,15 @@ icl_tc_phy_aux_power_well_enable(struct drm_i915_private *dev_priv,
> const struct i915_power_well_regs *regs = power_well->desc->ops->regs;
> bool is_tbt = power_well->desc->is_tc_tbt;
> bool timeout_expected;
> - u32 val;
>
> icl_tc_port_assert_ref_held(dev_priv, power_well, dig_port);
>
> - val = intel_de_read(dev_priv, DP_AUX_CH_CTL(aux_ch));
> - val &= ~DP_AUX_CH_CTL_TBT_IO;
> - if (is_tbt)
> - val |= DP_AUX_CH_CTL_TBT_IO;
> - intel_de_write(dev_priv, DP_AUX_CH_CTL(aux_ch), val);
> + intel_de_rmw(dev_priv, DP_AUX_CH_CTL(aux_ch),
> + DP_AUX_CH_CTL_TBT_IO, is_tbt ? DP_AUX_CH_CTL_TBT_IO : 0);
>
> - val = intel_de_read(dev_priv, regs->driver);
> - intel_de_write(dev_priv, regs->driver,
> - val | HSW_PWR_WELL_CTL_REQ(i915_power_well_instance(power_well)->hsw.idx));
> + intel_de_rmw(dev_priv, regs->driver,
> + 0,
> + HSW_PWR_WELL_CTL_REQ(i915_power_well_instance(power_well)->hsw.idx));
>
> /*
> * An AUX timeout is expected if the TBT DP tunnel is down,
> @@ -789,12 +767,8 @@ static void tgl_enable_dc3co(struct drm_i915_private *dev_priv)
>
> static void tgl_disable_dc3co(struct drm_i915_private *dev_priv)
> {
> - u32 val;
> -
> drm_dbg_kms(&dev_priv->drm, "Disabling DC3CO\n");
> - val = intel_de_read(dev_priv, DC_STATE_EN);
> - val &= ~DC_STATE_DC3CO_STATUS;
> - intel_de_write(dev_priv, DC_STATE_EN, val);
> + intel_de_rmw(dev_priv, DC_STATE_EN, DC_STATE_DC3CO_STATUS, 0);
> gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> /*
> * Delay of 200us DC3CO Exit time B.Spec 49196
> @@ -833,8 +807,8 @@ void gen9_enable_dc5(struct drm_i915_private *dev_priv)
>
> /* Wa Display #1183: skl,kbl,cfl */
> if (DISPLAY_VER(dev_priv) == 9 && !IS_BROXTON(dev_priv))
> - intel_de_write(dev_priv, GEN8_CHICKEN_DCPR_1,
> - intel_de_read(dev_priv, GEN8_CHICKEN_DCPR_1) | SKL_SELECT_ALTERNATE_DC_EXIT);
> + intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
> + 0, SKL_SELECT_ALTERNATE_DC_EXIT);
>
> gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
> }
> @@ -860,8 +834,8 @@ void skl_enable_dc6(struct drm_i915_private *dev_priv)
>
> /* Wa Display #1183: skl,kbl,cfl */
> if (DISPLAY_VER(dev_priv) == 9 && !IS_BROXTON(dev_priv))
> - intel_de_write(dev_priv, GEN8_CHICKEN_DCPR_1,
> - intel_de_read(dev_priv, GEN8_CHICKEN_DCPR_1) | SKL_SELECT_ALTERNATE_DC_EXIT);
> + intel_de_rmw(dev_priv, GEN8_CHICKEN_DCPR_1,
> + 0, SKL_SELECT_ALTERNATE_DC_EXIT);
>
> gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
> }
> @@ -1162,18 +1136,14 @@ static bool vlv_power_well_enabled(struct drm_i915_private *dev_priv,
>
> static void vlv_init_display_clock_gating(struct drm_i915_private *dev_priv)
> {
> - u32 val;
> -
> /*
> * On driver load, a pipe may be active and driving a DSI display.
> * Preserve DPOUNIT_CLOCK_GATE_DISABLE to avoid the pipe getting stuck
> * (and never recovering) in this case. intel_dsi_post_disable() will
> * clear it when we turn off the display.
> */
> - val = intel_de_read(dev_priv, DSPCLK_GATE_D(dev_priv));
> - val &= DPOUNIT_CLOCK_GATE_DISABLE;
> - val |= VRHUNIT_CLOCK_GATE_DISABLE;
> - intel_de_write(dev_priv, DSPCLK_GATE_D(dev_priv), val);
> + intel_de_rmw(dev_priv, DSPCLK_GATE_D(dev_priv),
> + ~DPOUNIT_CLOCK_GATE_DISABLE, VRHUNIT_CLOCK_GATE_DISABLE);
>
> /*
> * Disable trickle feed and enable pnd deadline calculation
> @@ -1289,8 +1259,7 @@ static void vlv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
> * both PLLs disabled, or we risk losing DPIO and PLL
> * synchronization.
> */
> - intel_de_write(dev_priv, DPIO_CTL,
> - intel_de_read(dev_priv, DPIO_CTL) | DPIO_CMNRST);
> + intel_de_rmw(dev_priv, DPIO_CTL, 0, DPIO_CMNRST);
> }
>
> static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> @@ -1302,8 +1271,7 @@ static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> assert_pll_disabled(dev_priv, pipe);
>
> /* Assert common reset */
> - intel_de_write(dev_priv, DPIO_CTL,
> - intel_de_read(dev_priv, DPIO_CTL) & ~DPIO_CMNRST);
> + intel_de_rmw(dev_priv, DPIO_CTL, DPIO_CMNRST, 0);
>
> vlv_set_power_well(dev_priv, power_well, false);
> }
> --
> 2.34.1
>
prev parent reply other threads:[~2023-02-21 20:00 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-17 11:18 [Intel-gfx] [PATCH v3] drm/i915/display/power: use intel_de_rmw if possible Andrzej Hajda
2023-02-17 15:36 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2023-02-18 0:29 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2023-02-21 20:00 ` Rodrigo Vivi [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y/UiwJPNMJUdKKkp@intel.com \
--to=rodrigo.vivi@intel.com \
--cc=andrzej.hajda@intel.com \
--cc=chris.p.wilson@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=matthew.auld@intel.com \
--cc=thomas.hellstrom@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.