From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jani Nikula <jani.nikula@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915/display: use intel_de_*() functions for register access
Date: Fri, 14 Feb 2020 16:24:14 +0200 [thread overview]
Message-ID: <20200214142414.GJ13686@intel.com> (raw)
In-Reply-To: <20200214140910.23194-2-jani.nikula@intel.com>
On Fri, Feb 14, 2020 at 04:09:10PM +0200, Jani Nikula wrote:
> The implicit "dev_priv" local variable use has been a long-standing pain
> point in the register access macros I915_READ(), I915_WRITE(),
> POSTING_READ(), I915_READ_FW(), and I915_WRITE_FW().
>
> Replace them with the corresponding new display engine register
> accessors intel_de_read(), intel_de_write(), intel_de_posting_read(),
> intel_de_read_fw(), and intel_de_write_fw().
>
> No functional changes.
>
> Generated using the following semantic patch:
>
> @@
> expression REG, OFFSET;
> @@
> - I915_READ(REG)
> + intel_de_read(dev_priv, REG)
>
> @@
> expression REG, OFFSET;
> @@
> - POSTING_READ(REG)
> + intel_de_posting_read(dev_priv, REG)
>
> @@
> expression REG, OFFSET;
> @@
> - I915_WRITE(REG, OFFSET)
> + intel_de_write(dev_priv, REG, OFFSET)
>
> @@
> expression REG;
> @@
> - I915_READ_FW(REG)
> + intel_de_read_fw(dev_priv, REG)
>
> @@
> expression REG, OFFSET;
> @@
> - I915_WRITE_FW(REG, OFFSET)
> + intel_de_write_fw(dev_priv, REG, OFFSET)
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> .../drm/i915/display/intel_display_debugfs.c | 65 +++++++++++--------
> 1 file changed, 37 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 50baac726e70..287442bbcb46 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -57,15 +57,15 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
> u32 mask;
>
> if (INTEL_GEN(dev_priv) >= 8)
> - mask = I915_READ(IVB_FBC_STATUS2) & BDW_FBC_COMP_SEG_MASK;
> + mask = intel_de_read(dev_priv, IVB_FBC_STATUS2) & BDW_FBC_COMP_SEG_MASK;
> else if (INTEL_GEN(dev_priv) >= 7)
> - mask = I915_READ(IVB_FBC_STATUS2) & IVB_FBC_COMP_SEG_MASK;
> + mask = intel_de_read(dev_priv, IVB_FBC_STATUS2) & IVB_FBC_COMP_SEG_MASK;
> else if (INTEL_GEN(dev_priv) >= 5)
> - mask = I915_READ(ILK_DPFC_STATUS) & ILK_DPFC_COMP_SEG_MASK;
> + mask = intel_de_read(dev_priv, ILK_DPFC_STATUS) & ILK_DPFC_COMP_SEG_MASK;
> else if (IS_G4X(dev_priv))
> - mask = I915_READ(DPFC_STATUS) & DPFC_COMP_SEG_MASK;
> + mask = intel_de_read(dev_priv, DPFC_STATUS) & DPFC_COMP_SEG_MASK;
> else
> - mask = I915_READ(FBC_STATUS) & (FBC_STAT_COMPRESSING |
> + mask = intel_de_read(dev_priv, FBC_STATUS) & (FBC_STAT_COMPRESSING |
> FBC_STAT_COMPRESSED);
Seesm to want some manual work to fix the alignment afterwards.
Probably should wrap after & actually.
>
> seq_printf(m, "Compressing: %s\n", yesno(mask));
> @@ -99,12 +99,11 @@ static int i915_fbc_false_color_set(void *data, u64 val)
>
> mutex_lock(&dev_priv->fbc.lock);
>
> - reg = I915_READ(ILK_DPFC_CONTROL);
> + reg = intel_de_read(dev_priv, ILK_DPFC_CONTROL);
> dev_priv->fbc.false_color = val;
>
> - I915_WRITE(ILK_DPFC_CONTROL, val ?
> - (reg | FBC_CTL_FALSE_COLOR) :
> - (reg & ~FBC_CTL_FALSE_COLOR));
> + intel_de_write(dev_priv, ILK_DPFC_CONTROL,
> + val ? (reg | FBC_CTL_FALSE_COLOR) : (reg & ~FBC_CTL_FALSE_COLOR));
Did we introduce some kind of rmw() variant? Could use it here to get
rid of this rather ugly construct.
For the series
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> mutex_unlock(&dev_priv->fbc.lock);
> return 0;
> @@ -130,7 +129,7 @@ static int i915_ips_status(struct seq_file *m, void *unused)
> if (INTEL_GEN(dev_priv) >= 8) {
> seq_puts(m, "Currently: unknown\n");
> } else {
> - if (I915_READ(IPS_CTL) & IPS_ENABLE)
> + if (intel_de_read(dev_priv, IPS_CTL) & IPS_ENABLE)
> seq_puts(m, "Currently: enabled\n");
> else
> seq_puts(m, "Currently: disabled\n");
> @@ -152,16 +151,16 @@ static int i915_sr_status(struct seq_file *m, void *unused)
> if (INTEL_GEN(dev_priv) >= 9)
> /* no global SR status; inspect per-plane WM */;
> else if (HAS_PCH_SPLIT(dev_priv))
> - sr_enabled = I915_READ(WM1_LP_ILK) & WM1_LP_SR_EN;
> + sr_enabled = intel_de_read(dev_priv, WM1_LP_ILK) & WM1_LP_SR_EN;
> else if (IS_I965GM(dev_priv) || IS_G4X(dev_priv) ||
> IS_I945G(dev_priv) || IS_I945GM(dev_priv))
> - sr_enabled = I915_READ(FW_BLC_SELF) & FW_BLC_SELF_EN;
> + sr_enabled = intel_de_read(dev_priv, FW_BLC_SELF) & FW_BLC_SELF_EN;
> else if (IS_I915GM(dev_priv))
> - sr_enabled = I915_READ(INSTPM) & INSTPM_SELF_EN;
> + sr_enabled = intel_de_read(dev_priv, INSTPM) & INSTPM_SELF_EN;
> else if (IS_PINEVIEW(dev_priv))
> - sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN;
> + sr_enabled = intel_de_read(dev_priv, DSPFW3) & PINEVIEW_SELF_REFRESH_EN;
> else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> - sr_enabled = I915_READ(FW_BLC_SELF_VLV) & FW_CSPWRDWNEN;
> + sr_enabled = intel_de_read(dev_priv, FW_BLC_SELF_VLV) & FW_CSPWRDWNEN;
>
> intel_display_power_put(dev_priv, POWER_DOMAIN_INIT, wakeref);
>
> @@ -298,7 +297,8 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
> "BUF_ON",
> "TG_ON"
> };
> - val = I915_READ(EDP_PSR2_STATUS(dev_priv->psr.transcoder));
> + val = intel_de_read(dev_priv,
> + EDP_PSR2_STATUS(dev_priv->psr.transcoder));
> status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
> EDP_PSR2_STATUS_STATE_SHIFT;
> if (status_val < ARRAY_SIZE(live_status))
> @@ -314,7 +314,8 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
> "SRDOFFACK",
> "SRDENT_ON",
> };
> - val = I915_READ(EDP_PSR_STATUS(dev_priv->psr.transcoder));
> + val = intel_de_read(dev_priv,
> + EDP_PSR_STATUS(dev_priv->psr.transcoder));
> status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
> EDP_PSR_STATUS_STATE_SHIFT;
> if (status_val < ARRAY_SIZE(live_status))
> @@ -361,10 +362,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> }
>
> if (psr->psr2_enabled) {
> - val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
> + val = intel_de_read(dev_priv,
> + EDP_PSR2_CTL(dev_priv->psr.transcoder));
> enabled = val & EDP_PSR2_ENABLE;
> } else {
> - val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
> + val = intel_de_read(dev_priv,
> + EDP_PSR_CTL(dev_priv->psr.transcoder));
> enabled = val & EDP_PSR_ENABLE;
> }
> seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> @@ -377,7 +380,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> * SKL+ Perf counter is reset to 0 everytime DC state is entered
> */
> if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> - val = I915_READ(EDP_PSR_PERF_CNT(dev_priv->psr.transcoder));
> + val = intel_de_read(dev_priv,
> + EDP_PSR_PERF_CNT(dev_priv->psr.transcoder));
> val &= EDP_PSR_PERF_CNT_MASK;
> seq_printf(m, "Performance counter: %u\n", val);
> }
> @@ -397,8 +401,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> * frame boundary between register reads
> */
> for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame += 3) {
> - val = I915_READ(PSR2_SU_STATUS(dev_priv->psr.transcoder,
> - frame));
> + val = intel_de_read(dev_priv,
> + PSR2_SU_STATUS(dev_priv->psr.transcoder, frame));
> su_frames_val[frame / 3] = val;
> }
>
> @@ -518,7 +522,8 @@ static int i915_dmc_info(struct seq_file *m, void *unused)
> * reg for DC3CO debugging and validation,
> * but TGL DMC f/w is using DMC_DEBUG3 reg for DC3CO counter.
> */
> - seq_printf(m, "DC3CO count: %d\n", I915_READ(DMC_DEBUG3));
> + seq_printf(m, "DC3CO count: %d\n",
> + intel_de_read(dev_priv, DMC_DEBUG3));
> } else {
> dc5_reg = IS_BROXTON(dev_priv) ? BXT_CSR_DC3_DC5_COUNT :
> SKL_CSR_DC3_DC5_COUNT;
> @@ -526,14 +531,18 @@ static int i915_dmc_info(struct seq_file *m, void *unused)
> dc6_reg = SKL_CSR_DC5_DC6_COUNT;
> }
>
> - seq_printf(m, "DC3 -> DC5 count: %d\n", I915_READ(dc5_reg));
> + seq_printf(m, "DC3 -> DC5 count: %d\n",
> + intel_de_read(dev_priv, dc5_reg));
> if (dc6_reg.reg)
> - seq_printf(m, "DC5 -> DC6 count: %d\n", I915_READ(dc6_reg));
> + seq_printf(m, "DC5 -> DC6 count: %d\n",
> + intel_de_read(dev_priv, dc6_reg));
>
> out:
> - seq_printf(m, "program base: 0x%08x\n", I915_READ(CSR_PROGRAM(0)));
> - seq_printf(m, "ssp base: 0x%08x\n", I915_READ(CSR_SSP_BASE));
> - seq_printf(m, "htp: 0x%08x\n", I915_READ(CSR_HTP_SKL));
> + seq_printf(m, "program base: 0x%08x\n",
> + intel_de_read(dev_priv, CSR_PROGRAM(0)));
> + seq_printf(m, "ssp base: 0x%08x\n",
> + intel_de_read(dev_priv, CSR_SSP_BASE));
> + seq_printf(m, "htp: 0x%08x\n", intel_de_read(dev_priv, CSR_HTP_SKL));
>
> intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref);
>
> --
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-02-14 14:24 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-14 14:09 [Intel-gfx] [PATCH 1/2] drm/i915/csr: use intel_de_*() functions for register access Jani Nikula
2020-02-14 14:09 ` [Intel-gfx] [PATCH 2/2] drm/i915/display: " Jani Nikula
2020-02-14 14:24 ` Ville Syrjälä [this message]
2020-02-14 14:27 ` Jani Nikula
2020-02-18 9:31 ` Jani Nikula
2020-02-14 20:54 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/csr: " Patchwork
2020-02-18 2:09 ` [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=20200214142414.GJ13686@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).