| Message ID | 20200214140910.23194-2-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/csr: use intel_de_*() functions for register access | expand |
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
On Fri, 14 Feb 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > 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. Hmmh, was too hasty glancing over the cocci generated patch. > >> >> 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. I wonder if it would be possible to come up with a magic cocci spell to convert those. > > For the series > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks. BR, Jani. > >> >> 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
On Fri, 14 Feb 2020, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > 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. Thanks for the review, pushed with this whitespace change applied. BR, Jani. > >> >> 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
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); 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)); 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);
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(-)