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 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.