From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 7/9] drm/i915/display/panel: use intel_de_rmw if possible in panel related code
Date: Mon, 9 Jan 2023 14:22:12 -0500 [thread overview]
Message-ID: <Y7xpZOkbqzaLzExo@intel.com> (raw)
In-Reply-To: <20230105131046.2173431-7-andrzej.hajda@intel.com>
On Thu, Jan 05, 2023 at 02:10:44PM +0100, Andrzej Hajda wrote:
> The helper makes the code more compact and readable.
>
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
> .../gpu/drm/i915/display/intel_backlight.c | 59 +++++++------------
> drivers/gpu/drm/i915/display/intel_pps.c | 14 ++---
> drivers/gpu/drm/i915/display/intel_psr.c | 40 ++++---------
> 3 files changed, 37 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index 5b7da72c95b8c5..b088921c543eaa 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -349,8 +349,7 @@ static void lpt_disable_backlight(const struct drm_connector_state *old_conn_sta
> intel_de_write(i915, BLC_PWM_CPU_CTL2, tmp & ~BLM_PWM_ENABLE);
> }
>
> - tmp = intel_de_read(i915, BLC_PWM_PCH_CTL1);
> - intel_de_write(i915, BLC_PWM_PCH_CTL1, tmp & ~BLM_PCH_PWM_ENABLE);
> + tmp = intel_de_rmw(i915, BLC_PWM_PCH_CTL1, BLM_PCH_PWM_ENABLE, 0);
> }
>
> static void pch_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
> @@ -361,11 +360,9 @@ static void pch_disable_backlight(const struct drm_connector_state *old_conn_sta
>
> intel_backlight_set_pwm_level(old_conn_state, val);
>
> - tmp = intel_de_read(i915, BLC_PWM_CPU_CTL2);
> - intel_de_write(i915, BLC_PWM_CPU_CTL2, tmp & ~BLM_PWM_ENABLE);
> + intel_de_rmw(i915, BLC_PWM_CPU_CTL2, BLM_PWM_ENABLE, 0);
>
> - tmp = intel_de_read(i915, BLC_PWM_PCH_CTL1);
> - intel_de_write(i915, BLC_PWM_PCH_CTL1, tmp & ~BLM_PCH_PWM_ENABLE);
> + tmp = intel_de_rmw(i915, BLC_PWM_PCH_CTL1, BLM_PCH_PWM_ENABLE, 0);
> }
>
> static void i9xx_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
> @@ -380,8 +377,7 @@ static void i965_disable_backlight(const struct drm_connector_state *old_conn_st
>
> intel_backlight_set_pwm_level(old_conn_state, val);
>
> - tmp = intel_de_read(i915, BLC_PWM_CTL2);
> - intel_de_write(i915, BLC_PWM_CTL2, tmp & ~BLM_PWM_ENABLE);
> + tmp = intel_de_rmw(i915, BLC_PWM_CTL2, BLM_PWM_ENABLE, 0);
> }
>
> static void vlv_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
> @@ -393,8 +389,7 @@ static void vlv_disable_backlight(const struct drm_connector_state *old_conn_sta
>
> intel_backlight_set_pwm_level(old_conn_state, val);
>
> - tmp = intel_de_read(i915, VLV_BLC_PWM_CTL2(pipe));
> - intel_de_write(i915, VLV_BLC_PWM_CTL2(pipe), tmp & ~BLM_PWM_ENABLE);
> + tmp = intel_de_rmw(i915, VLV_BLC_PWM_CTL2(pipe), BLM_PWM_ENABLE, 0);
> }
>
> static void bxt_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
> @@ -402,19 +397,14 @@ static void bxt_disable_backlight(const struct drm_connector_state *old_conn_sta
> struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
> struct drm_i915_private *i915 = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
> - u32 tmp;
>
> intel_backlight_set_pwm_level(old_conn_state, val);
>
> - tmp = intel_de_read(i915, BXT_BLC_PWM_CTL(panel->backlight.controller));
> - intel_de_write(i915, BXT_BLC_PWM_CTL(panel->backlight.controller),
> - tmp & ~BXT_BLC_PWM_ENABLE);
> + intel_de_rmw(i915, BXT_BLC_PWM_CTL(panel->backlight.controller),
> + BXT_BLC_PWM_ENABLE, 0);
>
> - if (panel->backlight.controller == 1) {
> - val = intel_de_read(i915, UTIL_PIN_CTL);
> - val &= ~UTIL_PIN_ENABLE;
> - intel_de_write(i915, UTIL_PIN_CTL, val);
> - }
> + if (panel->backlight.controller == 1)
> + intel_de_rmw(i915, UTIL_PIN_CTL, UTIL_PIN_ENABLE, 0);
> }
>
> static void cnp_disable_backlight(const struct drm_connector_state *old_conn_state, u32 val)
> @@ -422,13 +412,11 @@ static void cnp_disable_backlight(const struct drm_connector_state *old_conn_sta
> struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
> struct drm_i915_private *i915 = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
> - u32 tmp;
>
> intel_backlight_set_pwm_level(old_conn_state, val);
>
> - tmp = intel_de_read(i915, BXT_BLC_PWM_CTL(panel->backlight.controller));
> - intel_de_write(i915, BXT_BLC_PWM_CTL(panel->backlight.controller),
> - tmp & ~BXT_BLC_PWM_ENABLE);
> + intel_de_rmw(i915, BXT_BLC_PWM_CTL(panel->backlight.controller),
> + BXT_BLC_PWM_ENABLE, 0);
> }
>
> static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn_state, u32 level)
> @@ -478,7 +466,7 @@ static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
> struct intel_connector *connector = to_intel_connector(conn_state->connector);
> struct drm_i915_private *i915 = to_i915(connector->base.dev);
> struct intel_panel *panel = &connector->panel;
> - u32 pch_ctl1, pch_ctl2, schicken;
> + u32 pch_ctl1, pch_ctl2;
>
> pch_ctl1 = intel_de_read(i915, BLC_PWM_PCH_CTL1);
> if (pch_ctl1 & BLM_PCH_PWM_ENABLE) {
> @@ -487,21 +475,14 @@ static void lpt_enable_backlight(const struct intel_crtc_state *crtc_state,
> intel_de_write(i915, BLC_PWM_PCH_CTL1, pch_ctl1);
> }
>
> - if (HAS_PCH_LPT(i915)) {
> - schicken = intel_de_read(i915, SOUTH_CHICKEN2);
> - if (panel->backlight.alternate_pwm_increment)
> - schicken |= LPT_PWM_GRANULARITY;
> - else
> - schicken &= ~LPT_PWM_GRANULARITY;
> - intel_de_write(i915, SOUTH_CHICKEN2, schicken);
> - } else {
> - schicken = intel_de_read(i915, SOUTH_CHICKEN1);
> - if (panel->backlight.alternate_pwm_increment)
> - schicken |= SPT_PWM_GRANULARITY;
> - else
> - schicken &= ~SPT_PWM_GRANULARITY;
> - intel_de_write(i915, SOUTH_CHICKEN1, schicken);
> - }
> + if (HAS_PCH_LPT(i915))
> + intel_de_rmw(i915, SOUTH_CHICKEN2, LPT_PWM_GRANULARITY,
> + panel->backlight.alternate_pwm_increment ?
> + LPT_PWM_GRANULARITY : 0);
> + else
> + intel_de_rmw(i915, SOUTH_CHICKEN1, SPT_PWM_GRANULARITY,
> + panel->backlight.alternate_pwm_increment ?
> + SPT_PWM_GRANULARITY : 0);
this chunck has a risk of behavior change, but this looks the
right thing to do.
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
(we do need to get the CI in this series)
>
> pch_ctl2 = panel->backlight.pwm_level_max << 16;
> intel_de_write(i915, BLC_PWM_PCH_CTL2, pch_ctl2);
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
> index 7b21438edd9bc5..a4e00cab5f0ed8 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -1534,17 +1534,13 @@ static void pps_init_registers(struct intel_dp *intel_dp, bool force_disable_vdd
> /*
> * Compute the divisor for the pp clock, simply match the Bspec formula.
> */
> - if (i915_mmio_reg_valid(regs.pp_div)) {
> + if (i915_mmio_reg_valid(regs.pp_div))
> intel_de_write(dev_priv, regs.pp_div,
> REG_FIELD_PREP(PP_REFERENCE_DIVIDER_MASK, (100 * div) / 2 - 1) | REG_FIELD_PREP(PANEL_POWER_CYCLE_DELAY_MASK, DIV_ROUND_UP(seq->t11_t12, 1000)));
> - } else {
> - u32 pp_ctl;
> -
> - pp_ctl = intel_de_read(dev_priv, regs.pp_ctrl);
> - pp_ctl &= ~BXT_POWER_CYCLE_DELAY_MASK;
> - pp_ctl |= REG_FIELD_PREP(BXT_POWER_CYCLE_DELAY_MASK, DIV_ROUND_UP(seq->t11_t12, 1000));
> - intel_de_write(dev_priv, regs.pp_ctrl, pp_ctl);
> - }
> + else
> + intel_de_rmw(dev_priv, regs.pp_ctrl, BXT_POWER_CYCLE_DELAY_MASK,
> + REG_FIELD_PREP(BXT_POWER_CYCLE_DELAY_MASK,
> + DIV_ROUND_UP(seq->t11_t12, 1000)));
>
> drm_dbg_kms(&dev_priv->drm,
> "panel power sequencer register settings: PP_ON %#x, PP_OFF %#x, PP_DIV %#x\n",
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index d0d774219cc5ea..a0518c2f2668ce 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -153,7 +153,7 @@ static void psr_irq_control(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> i915_reg_t imr_reg;
> - u32 mask, val;
> + u32 mask;
>
> if (DISPLAY_VER(dev_priv) >= 12)
> imr_reg = TRANS_PSR_IMR(intel_dp->psr.transcoder);
> @@ -165,10 +165,7 @@ static void psr_irq_control(struct intel_dp *intel_dp)
> mask |= psr_irq_post_exit_bit_get(intel_dp) |
> psr_irq_pre_entry_bit_get(intel_dp);
>
> - val = intel_de_read(dev_priv, imr_reg);
> - val &= ~psr_irq_mask_get(intel_dp);
> - val |= ~mask;
> - intel_de_write(dev_priv, imr_reg, val);
> + intel_de_rmw(dev_priv, imr_reg, psr_irq_mask_get(intel_dp), ~mask);
> }
>
> static void psr_event_print(struct drm_i915_private *i915,
> @@ -246,8 +243,6 @@ void intel_psr_irq_handler(struct intel_dp *intel_dp, u32 psr_iir)
> }
>
> if (psr_iir & psr_irq_psr_error_bit_get(intel_dp)) {
> - u32 val;
> -
> drm_warn(&dev_priv->drm, "[transcoder %s] PSR aux error\n",
> transcoder_name(cpu_transcoder));
>
> @@ -261,9 +256,7 @@ void intel_psr_irq_handler(struct intel_dp *intel_dp, u32 psr_iir)
> * again so we don't care about unmask the interruption
> * or unset irq_aux_error.
> */
> - val = intel_de_read(dev_priv, imr_reg);
> - val |= psr_irq_psr_error_bit_get(intel_dp);
> - intel_de_write(dev_priv, imr_reg, val);
> + intel_de_rmw(dev_priv, imr_reg, 0, psr_irq_psr_error_bit_get(intel_dp));
>
> schedule_work(&intel_dp->psr.work);
> }
> @@ -638,13 +631,10 @@ static void psr2_program_idle_frames(struct intel_dp *intel_dp,
> u32 idle_frames)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> - u32 val;
>
> idle_frames <<= EDP_PSR2_IDLE_FRAME_SHIFT;
> - val = intel_de_read(dev_priv, EDP_PSR2_CTL(intel_dp->psr.transcoder));
> - val &= ~EDP_PSR2_IDLE_FRAME_MASK;
> - val |= idle_frames;
> - intel_de_write(dev_priv, EDP_PSR2_CTL(intel_dp->psr.transcoder), val);
> + intel_de_rmw(dev_priv, EDP_PSR2_CTL(intel_dp->psr.transcoder),
> + EDP_PSR2_IDLE_FRAME_MASK, idle_frames);
> }
>
> static void tgl_psr2_enable_dc3co(struct intel_dp *intel_dp)
> @@ -1144,19 +1134,13 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
>
> psr_irq_control(intel_dp);
>
> - if (intel_dp->psr.dc3co_exitline) {
> - u32 val;
> -
> - /*
> - * TODO: if future platforms supports DC3CO in more than one
> - * transcoder, EXITLINE will need to be unset when disabling PSR
> - */
> - val = intel_de_read(dev_priv, EXITLINE(cpu_transcoder));
> - val &= ~EXITLINE_MASK;
> - val |= intel_dp->psr.dc3co_exitline << EXITLINE_SHIFT;
> - val |= EXITLINE_ENABLE;
> - intel_de_write(dev_priv, EXITLINE(cpu_transcoder), val);
> - }
> + /*
> + * TODO: if future platforms supports DC3CO in more than one
> + * transcoder, EXITLINE will need to be unset when disabling PSR
> + */
> + if (intel_dp->psr.dc3co_exitline)
> + intel_de_rmw(dev_priv, EXITLINE(cpu_transcoder), EXITLINE_MASK,
> + intel_dp->psr.dc3co_exitline << EXITLINE_SHIFT | EXITLINE_ENABLE);
>
> if (HAS_PSR_HW_TRACKING(dev_priv) && HAS_PSR2_SEL_FETCH(dev_priv))
> intel_de_rmw(dev_priv, CHICKEN_PAR1_1, IGNORE_PSR2_HW_TRACKING,
> --
> 2.34.1
>
next prev parent reply other threads:[~2023-01-09 19:22 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-05 13:10 [Intel-gfx] [PATCH v2 1/9] drm/i915/display/core: use intel_de_rmw if possible Andrzej Hajda
2023-01-05 13:10 ` [Intel-gfx] [PATCH v2 2/9] drm/i915/display/power: " Andrzej Hajda
2023-01-05 20:27 ` Rodrigo Vivi
2023-02-16 16:27 ` Jani Nikula
2023-01-05 13:10 ` [Intel-gfx] [PATCH v2 3/9] drm/i915/display/dpll: " Andrzej Hajda
2023-01-05 20:32 ` Rodrigo Vivi
2023-01-05 13:10 ` [Intel-gfx] [PATCH v2 4/9] drm/i915/display/phys: " Andrzej Hajda
2023-01-06 15:26 ` Rodrigo Vivi
2023-01-05 13:10 ` [Intel-gfx] [PATCH v2 5/9] drm/i915/display/pch: " Andrzej Hajda
2023-01-06 15:28 ` Rodrigo Vivi
2023-01-05 13:10 ` [Intel-gfx] [PATCH v2 6/9] drm/i915/display/hdmi: " Andrzej Hajda
2023-01-06 15:35 ` Rodrigo Vivi
2023-01-09 10:51 ` Andrzej Hajda
2023-01-09 11:45 ` Jani Nikula
2023-01-05 13:10 ` [Intel-gfx] [PATCH v2 7/9] drm/i915/display/panel: use intel_de_rmw if possible in panel related code Andrzej Hajda
2023-01-09 19:22 ` Rodrigo Vivi [this message]
2023-01-05 13:10 ` [Intel-gfx] [PATCH v2 8/9] drm/i915/display/interfaces: use intel_de_rmw if possible Andrzej Hajda
2023-01-09 19:24 ` Rodrigo Vivi
2023-01-05 13:10 ` [Intel-gfx] [PATCH v2 9/9] drm/i915/display/misc: " Andrzej Hajda
2023-01-09 19:27 ` Rodrigo Vivi
2023-01-10 9:28 ` Andrzej Hajda
2023-01-10 11:36 ` [Intel-gfx] [PATCH v3] " Andrzej Hajda
2023-01-10 16:15 ` Rodrigo Vivi
2023-01-05 20:21 ` [Intel-gfx] [PATCH v2 1/9] drm/i915/display/core: " Rodrigo Vivi
2023-01-09 11:32 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [v2,1/9] drm/i915/display/core: use intel_de_rmw if possible (rev2) Patchwork
2023-01-09 11:54 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2023-01-09 13:38 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2023-01-10 15:45 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [v2,1/9] drm/i915/display/core: use intel_de_rmw if possible (rev3) Patchwork
2023-01-11 12:09 ` Andrzej Hajda
2023-01-11 12:12 ` Veesam, RavitejaX
2023-02-06 12:35 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [v2,1/9] drm/i915/display/core: use intel_de_rmw if possible (rev4) Patchwork
2023-02-06 16:11 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
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=Y7xpZOkbqzaLzExo@intel.com \
--to=rodrigo.vivi@intel.com \
--cc=andrzej.hajda@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox