On Thu, 2022-09-29 at 06:16 -0700, José Roberto de Souza wrote: > On Tue, 2022-09-27 at 14:09 +0300, Jouni Högander wrote: > > Current PSR code is supposed to use TRANSCODER_EDP to force 0 shift for > > bits in PSR_IMR/IIR registers: > > > > /* > > * gen12+ has registers relative to transcoder and one per transcoder > > * using the same bit definition: handle it as TRANSCODER_EDP to force > > * 0 shift in bit definition > > */ > > > > At the time of writing the code assumption "TRANSCODER_EDP == 0" was made. > > This is not the case and all fields in PSR_IMR and PSR_IIR are shifted > > incorrectly if DISPLAY_VER >= 12. > > > > Fix this by adding separate register field defines for >=12 and add bit > > getter functions to keep code readability. > > > > v3: > > - Add separate register field defines (José) > > - Add bit getter functions (José) > > v2: > > - Improve commit message (José)
Also missing the Fixes tag, so this gets backported to stable kernels. > > > > Signed-off-by: Jouni Högander <[email protected]> > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 84 ++++++++++++++---------- > > drivers/gpu/drm/i915/i915_reg.h | 16 +++-- > > 2 files changed, 62 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index 9def8d9fade6..d7b08a7da9e9 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -116,34 +116,56 @@ static bool psr2_global_enabled(struct intel_dp > > *intel_dp) > > } > > } > > > > +static u32 psr_irq_psr_error_bit_get(struct intel_dp *intel_dp) > > +{ > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + > > + return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_ERROR : > > + EDP_PSR_ERROR(intel_dp->psr.transcoder); > > Drop the "_EDP", just go with TGL_PSR_ERROR... there is no reference to EDP > or any transcoder in TGL+ it is one register per transcoder. > > > +} > > + > > +static u32 psr_irq_post_exit_bit_get(struct intel_dp *intel_dp) > > +{ > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + > > + return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_POST_EXIT : > > + EDP_PSR_POST_EXIT(intel_dp->psr.transcoder); > > +} > > + > > +static u32 psr_irq_pre_entry_bit_get(struct intel_dp *intel_dp) > > +{ > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + > > + return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_PRE_ENTRY : > > + EDP_PSR_PRE_ENTRY(intel_dp->psr.transcoder); > > +} > > + > > +static u32 psr_irq_mask_get(struct intel_dp *intel_dp) > > +{ > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + > > + return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_MASK : > > + EDP_PSR_MASK(intel_dp->psr.transcoder); > > +} > > + > > static void psr_irq_control(struct intel_dp *intel_dp) > > { > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > - enum transcoder trans_shift; > > i915_reg_t imr_reg; > > u32 mask, val; > > > > - /* > > - * gen12+ has registers relative to transcoder and one per transcoder > > - * using the same bit definition: handle it as TRANSCODER_EDP to force > > - * 0 shift in bit definition > > - */ > > - if (DISPLAY_VER(dev_priv) >= 12) { > > - trans_shift = 0; > > + if (DISPLAY_VER(dev_priv) >= 12) > > imr_reg = TRANS_PSR_IMR(intel_dp->psr.transcoder); > > - } else { > > - trans_shift = intel_dp->psr.transcoder; > > + else > > imr_reg = EDP_PSR_IMR; > > - } > > > > - mask = EDP_PSR_ERROR(trans_shift); > > + mask = psr_irq_psr_error_bit_get(intel_dp); > > if (intel_dp->psr.debug & I915_PSR_DEBUG_IRQ) > > - mask |= EDP_PSR_POST_EXIT(trans_shift) | > > - EDP_PSR_PRE_ENTRY(trans_shift); > > + mask |= psr_irq_post_exit_bit_get(intel_dp) | > > + psr_irq_pre_entry_bit_get(intel_dp); > > > > - /* Warning: it is masking/setting reserved bits too */ > > val = intel_de_read(dev_priv, imr_reg); > > - val &= ~EDP_PSR_TRANS_MASK(trans_shift); > > + val &= ~psr_irq_mask_get(intel_dp); > > val |= ~mask; > > intel_de_write(dev_priv, imr_reg, val); > > } > > @@ -191,25 +213,21 @@ void intel_psr_irq_handler(struct intel_dp *intel_dp, > > u32 psr_iir) > > enum transcoder cpu_transcoder = intel_dp->psr.transcoder; > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > ktime_t time_ns = ktime_get(); > > - enum transcoder trans_shift; > > i915_reg_t imr_reg; > > > > - if (DISPLAY_VER(dev_priv) >= 12) { > > - trans_shift = 0; > > + if (DISPLAY_VER(dev_priv) >= 12) > > imr_reg = TRANS_PSR_IMR(intel_dp->psr.transcoder); > > - } else { > > - trans_shift = intel_dp->psr.transcoder; > > + else > > imr_reg = EDP_PSR_IMR; > > - } > > > > - if (psr_iir & EDP_PSR_PRE_ENTRY(trans_shift)) { > > + if (psr_iir & psr_irq_pre_entry_bit_get(intel_dp)) { > > intel_dp->psr.last_entry_attempt = time_ns; > > drm_dbg_kms(&dev_priv->drm, > > "[transcoder %s] PSR entry attempt in 2 vblanks\n", > > transcoder_name(cpu_transcoder)); > > } > > > > - if (psr_iir & EDP_PSR_POST_EXIT(trans_shift)) { > > + if (psr_iir & psr_irq_post_exit_bit_get(intel_dp)) { > > intel_dp->psr.last_exit = time_ns; > > drm_dbg_kms(&dev_priv->drm, > > "[transcoder %s] PSR exit completed\n", > > @@ -226,7 +244,7 @@ void intel_psr_irq_handler(struct intel_dp *intel_dp, > > u32 psr_iir) > > } > > } > > > > - if (psr_iir & EDP_PSR_ERROR(trans_shift)) { > > + if (psr_iir & psr_irq_psr_error_bit_get(intel_dp)) { > > u32 val; > > > > drm_warn(&dev_priv->drm, "[transcoder %s] PSR aux error\n", > > @@ -243,7 +261,7 @@ void intel_psr_irq_handler(struct intel_dp *intel_dp, > > u32 psr_iir) > > * or unset irq_aux_error. > > */ > > val = intel_de_read(dev_priv, imr_reg); > > - val |= EDP_PSR_ERROR(trans_shift); > > + val |= psr_irq_psr_error_bit_get(intel_dp); > > intel_de_write(dev_priv, imr_reg, val); > > > > schedule_work(&intel_dp->psr.work); > > @@ -1194,14 +1212,12 @@ static bool psr_interrupt_error_check(struct > > intel_dp *intel_dp) > > * first time that PSR HW tries to activate so lets keep PSR disabled > > * to avoid any rendering problems. > > */ > > - if (DISPLAY_VER(dev_priv) >= 12) { > > + if (DISPLAY_VER(dev_priv) >= 12) > > val = intel_de_read(dev_priv, > > TRANS_PSR_IIR(intel_dp->psr.transcoder)); > > - val &= EDP_PSR_ERROR(0); > > - } else { > > + else > > val = intel_de_read(dev_priv, EDP_PSR_IIR); > > - val &= EDP_PSR_ERROR(intel_dp->psr.transcoder); > > - } > > + val &= psr_irq_psr_error_bit_get(intel_dp); > > if (val) { > > intel_dp->psr.sink_not_reliable = true; > > drm_dbg_kms(&dev_priv->drm, > > @@ -2158,9 +2174,9 @@ static void intel_psr_work(struct work_struct *work) > > > > /* > > * We have to make sure PSR is ready for re-enable > > - * otherwise it keeps disabled until next full enable/disable cycle. > > - * PSR might take some time to get fully disabled > > - * and be ready for re-enable. > > + * otherwise it keeps disabled until next full enable/disable > > + * cycle. PSR might take some time to get fully disabled and > > + * be ready for re-enable. > > Non-related change. > > > */ > > if (!__psr_wait_for_idle_locked(intel_dp)) > > goto unlock; > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index 5003a5ffbc6a..3c103aeaa2e4 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -2168,10 +2168,18 @@ > > #define TRANS_PSR_IIR(tran) _MMIO_TRANS2(tran, > > _PSR_IIR_A) > > #define _EDP_PSR_TRANS_SHIFT(trans) ((trans) == > > TRANSCODER_EDP ? \ > > 0 : ((trans) - TRANSCODER_A + > > 1) * 8) > > -#define EDP_PSR_TRANS_MASK(trans) (0x7 << > > _EDP_PSR_TRANS_SHIFT(trans)) > > -#define EDP_PSR_ERROR(trans) (0x4 << > > _EDP_PSR_TRANS_SHIFT(trans)) > > -#define EDP_PSR_POST_EXIT(trans) (0x2 << > > _EDP_PSR_TRANS_SHIFT(trans)) > > -#define EDP_PSR_PRE_ENTRY(trans) (0x1 << > > _EDP_PSR_TRANS_SHIFT(trans)) > > +#define TGL_EDP_PSR_MASK (0x7) > > +#define TGL_EDP_PSR_ERROR (1 << 2) > > +#define TGL_EDP_PSR_POST_EXIT (1 << 1) > > +#define TGL_EDP_PSR_PRE_ENTRY (1 << 0) > > For new stuff REG_BIT() should be used. > > > +#define EDP_PSR_MASK(trans) (TGL_EDP_PSR_MASK << > > \ > > + _EDP_PSR_TRANS_SHIFT(trans)) > > +#define EDP_PSR_ERROR(trans) (TGL_EDP_PSR_ERROR << > > \ > > + _EDP_PSR_TRANS_SHIFT(trans)) > > +#define EDP_PSR_POST_EXIT(trans) (TGL_EDP_PSR_POST_EXIT << \ > > + _EDP_PSR_TRANS_SHIFT(trans)) > > +#define EDP_PSR_PRE_ENTRY(trans) (TGL_EDP_PSR_PRE_ENTRY << \ > > + _EDP_PSR_TRANS_SHIFT(trans)) > > > > #define _SRD_AUX_DATA_A 0x60814 > > #define _SRD_AUX_DATA_EDP 0x6f814 >
