| Message ID | 20190403233539.31828-6-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] drm/i915/psr: Update PSR2 SU corruption workaround comment | expand |
On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de Souza wrote: > PSR is only supported in eDP transcoder and there is only one > instance of it, so lets drop all of this code. Is this sentence true? I mean, in the way it is written it seems like HW doesn't actually support it... Or should we re-phrase for we are not really enabling support for other transcoders than eDP and we do not have plans to do it so soon so let's clean the code... or something like that? > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 17 +--- > drivers/gpu/drm/i915/intel_psr.c | 147 ++++++++----------------------- > 2 files changed, 42 insertions(+), 122 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index c59cfa83dbaf..18e2991b376d 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4241,13 +4241,9 @@ enum { > /* Bspec claims those aren't shifted but stay at 0x64800 */ > #define EDP_PSR_IMR _MMIO(0x64834) > #define EDP_PSR_IIR _MMIO(0x64838) > -#define EDP_PSR_ERROR(shift) (1 << ((shift) + 2)) > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1)) > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift)) > -#define EDP_PSR_TRANSCODER_C_SHIFT 24 > -#define EDP_PSR_TRANSCODER_B_SHIFT 16 > -#define EDP_PSR_TRANSCODER_A_SHIFT 8 > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0 > +#define EDP_PSR_ERROR (1 << 2) > +#define EDP_PSR_POST_EXIT (1 << 1) > +#define EDP_PSR_PRE_ENTRY (1 << 0) > > #define EDP_PSR_AUX_CTL _MMIO(dev_priv->psr_mmio_base + 0x10) > #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26) > @@ -4312,12 +4308,7 @@ enum { > #define EDP_PSR2_IDLE_FRAME_MASK 0xf > #define EDP_PSR2_IDLE_FRAME_SHIFT 0 > > -#define _PSR_EVENT_TRANS_A 0x60848 > -#define _PSR_EVENT_TRANS_B 0x61848 > -#define _PSR_EVENT_TRANS_C 0x62848 > -#define _PSR_EVENT_TRANS_D 0x63848 > -#define _PSR_EVENT_TRANS_EDP 0x6F848 > -#define PSR_EVENT(trans) _MMIO_TRANS2(trans, _PSR_EVENT_TRANS_A) > +#define PSR_EVENT _MMIO(0x6F848) > #define PSR_EVENT_PSR2_WD_TIMER_EXPIRE (1 << 17) > #define PSR_EVENT_PSR2_DISABLED (1 << 16) > #define PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN (1 << 15) > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index bb97c1657493..b984e005b72e 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv, > } > } > > -static int edp_psr_shift(enum transcoder cpu_transcoder) > -{ > - switch (cpu_transcoder) { > - case TRANSCODER_A: > - return EDP_PSR_TRANSCODER_A_SHIFT; > - case TRANSCODER_B: > - return EDP_PSR_TRANSCODER_B_SHIFT; > - case TRANSCODER_C: > - return EDP_PSR_TRANSCODER_C_SHIFT; > - default: > - MISSING_CASE(cpu_transcoder); > - /* fallthrough */ > - case TRANSCODER_EDP: > - return EDP_PSR_TRANSCODER_EDP_SHIFT; > - } > -} > - > void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 debug) > { > - u32 debug_mask, mask; > - enum transcoder cpu_transcoder; > - u32 transcoders = BIT(TRANSCODER_EDP); > - > - if (INTEL_GEN(dev_priv) >= 8) > - transcoders |= BIT(TRANSCODER_A) | > - BIT(TRANSCODER_B) | > - BIT(TRANSCODER_C); > - > - debug_mask = 0; > - mask = 0; > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) { > - int shift = edp_psr_shift(cpu_transcoder); > - > - mask |= EDP_PSR_ERROR(shift); > - debug_mask |= EDP_PSR_POST_EXIT(shift) | > - EDP_PSR_PRE_ENTRY(shift); > - } > + u32 mask = EDP_PSR_ERROR; > > if (debug & I915_PSR_DEBUG_IRQ) > - mask |= debug_mask; > + mask |= EDP_PSR_POST_EXIT | EDP_PSR_PRE_ENTRY; > > I915_WRITE(EDP_PSR_IMR, ~mask); > } > @@ -167,62 +133,47 @@ static void psr_event_print(u32 val, bool psr2_enabled) > > void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir) > { > - u32 transcoders = BIT(TRANSCODER_EDP); > - enum transcoder cpu_transcoder; > - ktime_t time_ns = ktime_get(); > - u32 mask = 0; > + ktime_t time_ns = ktime_get(); > > - if (INTEL_GEN(dev_priv) >= 8) > - transcoders |= BIT(TRANSCODER_A) | > - BIT(TRANSCODER_B) | > - BIT(TRANSCODER_C); > - > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) { > - int shift = edp_psr_shift(cpu_transcoder); > - > - if (psr_iir & EDP_PSR_ERROR(shift)) { > - DRM_WARN("[transcoder %s] PSR aux error\n", > - transcoder_name(cpu_transcoder)); > - > - dev_priv->psr.irq_aux_error = true; > - > - /* > - * If this interruption is not masked it will keep > - * interrupting so fast that it prevents the scheduled > - * work to run. > - * Also after a PSR error, we don't want to arm PSR > - * again so we don't care about unmask the interruption > - * or unset irq_aux_error. > - */ > - mask |= EDP_PSR_ERROR(shift); > - } > + if (psr_iir & EDP_PSR_ERROR) { > + u32 mask; > > - if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) { > - dev_priv->psr.last_entry_attempt = time_ns; > - DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n", > - transcoder_name(cpu_transcoder)); > - } > + DRM_WARN("[transcoder %s] PSR aux error\n", > + transcoder_name(TRANSCODER_EDP)); > > - if (psr_iir & EDP_PSR_POST_EXIT(shift)) { > - dev_priv->psr.last_exit = time_ns; > - DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n", > - transcoder_name(cpu_transcoder)); > + dev_priv->psr.irq_aux_error = true; > > - if (INTEL_GEN(dev_priv) >= 9) { > - u32 val = I915_READ(PSR_EVENT(cpu_transcoder)); > - bool psr2_enabled = dev_priv->psr.psr2_enabled; > + /* > + * If this interruption is not masked it will keep > + * interrupting so fast that it prevents the scheduled > + * work to run. > + * Also after a PSR error, we don't want to arm PSR > + * again so we don't care about unmask the interruption > + * or unset irq_aux_error. > + */ > + mask = I915_READ(EDP_PSR_IMR) | EDP_PSR_ERROR; > + I915_WRITE(EDP_PSR_IMR, mask); > > - I915_WRITE(PSR_EVENT(cpu_transcoder), val); > - psr_event_print(val, psr2_enabled); > - } > - } > + schedule_work(&dev_priv->psr.work); > } > > - if (mask) { > - mask |= I915_READ(EDP_PSR_IMR); > - I915_WRITE(EDP_PSR_IMR, mask); > + if (psr_iir & EDP_PSR_PRE_ENTRY) { > + dev_priv->psr.last_entry_attempt = time_ns; > + DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n", > + transcoder_name(TRANSCODER_EDP)); > + } > > - schedule_work(&dev_priv->psr.work); > + if (psr_iir & EDP_PSR_POST_EXIT) { > + DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n", > + transcoder_name(TRANSCODER_EDP)); > + > + if (INTEL_GEN(dev_priv) >= 9) { > + u32 val = I915_READ(PSR_EVENT); > + bool psr2_enabled = dev_priv->psr.psr2_enabled; > + > + I915_WRITE(PSR_EVENT, val); > + psr_event_print(val, psr2_enabled); > + } > } > } > > @@ -669,30 +620,10 @@ static void intel_psr_activate(struct intel_dp *intel_dp) > dev_priv->psr.active = true; > } > > -static i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private *dev_priv, > - enum transcoder cpu_transcoder) > -{ > - static const i915_reg_t regs[] = { > - [TRANSCODER_A] = CHICKEN_TRANS_A, > - [TRANSCODER_B] = CHICKEN_TRANS_B, > - [TRANSCODER_C] = CHICKEN_TRANS_C, > - [TRANSCODER_EDP] = CHICKEN_TRANS_EDP, > - }; > - > - WARN_ON(INTEL_GEN(dev_priv) < 9); > - > - if (WARN_ON(cpu_transcoder >= ARRAY_SIZE(regs) || > - !regs[cpu_transcoder].reg)) > - cpu_transcoder = TRANSCODER_A; > - > - return regs[cpu_transcoder]; > -} > - > static void intel_psr_enable_source(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > - enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > u32 mask; > > /* Only HSW and BDW have PSR AUX registers that need to be setup. SKL+ > @@ -703,13 +634,11 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp, > > if (dev_priv->psr.psr2_enabled && (IS_GEN(dev_priv, 9) && > !IS_GEMINILAKE(dev_priv))) { > - i915_reg_t reg = gen9_chicken_trans_reg(dev_priv, > - cpu_transcoder); > - u32 chicken = I915_READ(reg); > + u32 chicken = I915_READ(CHICKEN_TRANS_EDP); > > chicken |= PSR2_VSC_ENABLE_PROG_HEADER | > PSR2_ADD_VERTICAL_LINE_COUNT; > - I915_WRITE(reg, chicken); > + I915_WRITE(CHICKEN_TRANS_EDP, chicken); > } > > /* > @@ -1222,7 +1151,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv) > * to avoid any rendering problems. > */ > val = I915_READ(EDP_PSR_IIR); > - val &= EDP_PSR_ERROR(edp_psr_shift(TRANSCODER_EDP)); > + val &= EDP_PSR_ERROR; > if (val) { > DRM_DEBUG_KMS("PSR interruption error set\n"); > dev_priv->psr.sink_not_reliable = true; > -- > 2.21.0 >
On Wed, 2019-04-03 at 17:31 -0700, Rodrigo Vivi wrote: > On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de Souza > wrote: > > PSR is only supported in eDP transcoder and there is only one > > instance of it, so lets drop all of this code. > > Is this sentence true? I mean, in the way it is written it > seems like HW doesn't actually support it... > Or should we re-phrase for we are not really enabling support > for other transcoders than eDP and we do not have plans to do > it so soon so let's clean the code... > or something like that? Okay, what about replace it for: Since BDW all transcoders have PSR registers but only EDP transcoder can drive a EDP panel and as PSR is only part of the EDP specification, for real usage PSR is only supported in EDP panel, so lets drop all of this useless code. > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 17 +--- > > drivers/gpu/drm/i915/intel_psr.c | 147 ++++++++------------------- > > ---- > > 2 files changed, 42 insertions(+), 122 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index c59cfa83dbaf..18e2991b376d 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -4241,13 +4241,9 @@ enum { > > /* Bspec claims those aren't shifted but stay at 0x64800 */ > > #define EDP_PSR_IMR _MMIO(0x64834) > > #define EDP_PSR_IIR _MMIO(0x64838) > > -#define EDP_PSR_ERROR(shift) (1 << ((shift) > > + 2)) > > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1)) > > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift)) > > -#define EDP_PSR_TRANSCODER_C_SHIFT 24 > > -#define EDP_PSR_TRANSCODER_B_SHIFT 16 > > -#define EDP_PSR_TRANSCODER_A_SHIFT 8 > > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0 > > +#define EDP_PSR_ERROR (1 << 2) > > +#define EDP_PSR_POST_EXIT (1 << 1) > > +#define EDP_PSR_PRE_ENTRY (1 << 0) > > > > #define EDP_PSR_AUX_CTL _MMIO(dev_priv- > > >psr_mmio_base + 0x10) > > #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26) > > @@ -4312,12 +4308,7 @@ enum { > > #define EDP_PSR2_IDLE_FRAME_MASK 0xf > > #define EDP_PSR2_IDLE_FRAME_SHIFT 0 > > > > -#define _PSR_EVENT_TRANS_A 0x60848 > > -#define _PSR_EVENT_TRANS_B 0x61848 > > -#define _PSR_EVENT_TRANS_C 0x62848 > > -#define _PSR_EVENT_TRANS_D 0x63848 > > -#define _PSR_EVENT_TRANS_EDP 0x6F848 > > -#define PSR_EVENT(trans) _MMIO_TRANS2(trans, > > _PSR_EVENT_TRANS_A) > > +#define PSR_EVENT _MMIO(0x6F848) > > #define PSR_EVENT_PSR2_WD_TIMER_EXPIRE (1 << 17) > > #define PSR_EVENT_PSR2_DISABLED (1 << 16) > > #define PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN (1 << 15) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index bb97c1657493..b984e005b72e 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct > > drm_i915_private *dev_priv, > > } > > } > > > > -static int edp_psr_shift(enum transcoder cpu_transcoder) > > -{ > > - switch (cpu_transcoder) { > > - case TRANSCODER_A: > > - return EDP_PSR_TRANSCODER_A_SHIFT; > > - case TRANSCODER_B: > > - return EDP_PSR_TRANSCODER_B_SHIFT; > > - case TRANSCODER_C: > > - return EDP_PSR_TRANSCODER_C_SHIFT; > > - default: > > - MISSING_CASE(cpu_transcoder); > > - /* fallthrough */ > > - case TRANSCODER_EDP: > > - return EDP_PSR_TRANSCODER_EDP_SHIFT; > > - } > > -} > > - > > void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 > > debug) > > { > > - u32 debug_mask, mask; > > - enum transcoder cpu_transcoder; > > - u32 transcoders = BIT(TRANSCODER_EDP); > > - > > - if (INTEL_GEN(dev_priv) >= 8) > > - transcoders |= BIT(TRANSCODER_A) | > > - BIT(TRANSCODER_B) | > > - BIT(TRANSCODER_C); > > - > > - debug_mask = 0; > > - mask = 0; > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, > > transcoders) { > > - int shift = edp_psr_shift(cpu_transcoder); > > - > > - mask |= EDP_PSR_ERROR(shift); > > - debug_mask |= EDP_PSR_POST_EXIT(shift) | > > - EDP_PSR_PRE_ENTRY(shift); > > - } > > + u32 mask = EDP_PSR_ERROR; > > > > if (debug & I915_PSR_DEBUG_IRQ) > > - mask |= debug_mask; > > + mask |= EDP_PSR_POST_EXIT | EDP_PSR_PRE_ENTRY; > > > > I915_WRITE(EDP_PSR_IMR, ~mask); > > } > > @@ -167,62 +133,47 @@ static void psr_event_print(u32 val, bool > > psr2_enabled) > > > > void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 > > psr_iir) > > { > > - u32 transcoders = BIT(TRANSCODER_EDP); > > - enum transcoder cpu_transcoder; > > - ktime_t time_ns = ktime_get(); > > - u32 mask = 0; > > + ktime_t time_ns = ktime_get(); > > > > - if (INTEL_GEN(dev_priv) >= 8) > > - transcoders |= BIT(TRANSCODER_A) | > > - BIT(TRANSCODER_B) | > > - BIT(TRANSCODER_C); > > - > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, > > transcoders) { > > - int shift = edp_psr_shift(cpu_transcoder); > > - > > - if (psr_iir & EDP_PSR_ERROR(shift)) { > > - DRM_WARN("[transcoder %s] PSR aux error\n", > > - transcoder_name(cpu_transcoder)); > > - > > - dev_priv->psr.irq_aux_error = true; > > - > > - /* > > - * If this interruption is not masked it will > > keep > > - * interrupting so fast that it prevents the > > scheduled > > - * work to run. > > - * Also after a PSR error, we don't want to arm > > PSR > > - * again so we don't care about unmask the > > interruption > > - * or unset irq_aux_error. > > - */ > > - mask |= EDP_PSR_ERROR(shift); > > - } > > + if (psr_iir & EDP_PSR_ERROR) { > > + u32 mask; > > > > - if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) { > > - dev_priv->psr.last_entry_attempt = time_ns; > > - DRM_DEBUG_KMS("[transcoder %s] PSR entry > > attempt in 2 vblanks\n", > > - transcoder_name(cpu_transcoder)); > > - } > > + DRM_WARN("[transcoder %s] PSR aux error\n", > > + transcoder_name(TRANSCODER_EDP)); > > > > - if (psr_iir & EDP_PSR_POST_EXIT(shift)) { > > - dev_priv->psr.last_exit = time_ns; > > - DRM_DEBUG_KMS("[transcoder %s] PSR exit > > completed\n", > > - transcoder_name(cpu_transcoder)); > > + dev_priv->psr.irq_aux_error = true; > > > > - if (INTEL_GEN(dev_priv) >= 9) { > > - u32 val = > > I915_READ(PSR_EVENT(cpu_transcoder)); > > - bool psr2_enabled = dev_priv- > > >psr.psr2_enabled; > > + /* > > + * If this interruption is not masked it will keep > > + * interrupting so fast that it prevents the scheduled > > + * work to run. > > + * Also after a PSR error, we don't want to arm PSR > > + * again so we don't care about unmask the interruption > > + * or unset irq_aux_error. > > + */ > > + mask = I915_READ(EDP_PSR_IMR) | EDP_PSR_ERROR; > > + I915_WRITE(EDP_PSR_IMR, mask); > > > > - I915_WRITE(PSR_EVENT(cpu_transcoder), > > val); > > - psr_event_print(val, psr2_enabled); > > - } > > - } > > + schedule_work(&dev_priv->psr.work); > > } > > > > - if (mask) { > > - mask |= I915_READ(EDP_PSR_IMR); > > - I915_WRITE(EDP_PSR_IMR, mask); > > + if (psr_iir & EDP_PSR_PRE_ENTRY) { > > + dev_priv->psr.last_entry_attempt = time_ns; > > + DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 > > vblanks\n", > > + transcoder_name(TRANSCODER_EDP)); > > + } > > > > - schedule_work(&dev_priv->psr.work); > > + if (psr_iir & EDP_PSR_POST_EXIT) { > > + DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n", > > + transcoder_name(TRANSCODER_EDP)); > > + > > + if (INTEL_GEN(dev_priv) >= 9) { > > + u32 val = I915_READ(PSR_EVENT); > > + bool psr2_enabled = dev_priv->psr.psr2_enabled; > > + > > + I915_WRITE(PSR_EVENT, val); > > + psr_event_print(val, psr2_enabled); > > + } > > } > > } > > > > @@ -669,30 +620,10 @@ static void intel_psr_activate(struct > > intel_dp *intel_dp) > > dev_priv->psr.active = true; > > } > > > > -static i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private > > *dev_priv, > > - enum transcoder > > cpu_transcoder) > > -{ > > - static const i915_reg_t regs[] = { > > - [TRANSCODER_A] = CHICKEN_TRANS_A, > > - [TRANSCODER_B] = CHICKEN_TRANS_B, > > - [TRANSCODER_C] = CHICKEN_TRANS_C, > > - [TRANSCODER_EDP] = CHICKEN_TRANS_EDP, > > - }; > > - > > - WARN_ON(INTEL_GEN(dev_priv) < 9); > > - > > - if (WARN_ON(cpu_transcoder >= ARRAY_SIZE(regs) || > > - !regs[cpu_transcoder].reg)) > > - cpu_transcoder = TRANSCODER_A; > > - > > - return regs[cpu_transcoder]; > > -} > > - > > static void intel_psr_enable_source(struct intel_dp *intel_dp, > > const struct intel_crtc_state > > *crtc_state) > > { > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > - enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > > u32 mask; > > > > /* Only HSW and BDW have PSR AUX registers that need to be > > setup. SKL+ > > @@ -703,13 +634,11 @@ static void intel_psr_enable_source(struct > > intel_dp *intel_dp, > > > > if (dev_priv->psr.psr2_enabled && (IS_GEN(dev_priv, 9) && > > !IS_GEMINILAKE(dev_priv))) { > > - i915_reg_t reg = gen9_chicken_trans_reg(dev_priv, > > - cpu_transcoder) > > ; > > - u32 chicken = I915_READ(reg); > > + u32 chicken = I915_READ(CHICKEN_TRANS_EDP); > > > > chicken |= PSR2_VSC_ENABLE_PROG_HEADER | > > PSR2_ADD_VERTICAL_LINE_COUNT; > > - I915_WRITE(reg, chicken); > > + I915_WRITE(CHICKEN_TRANS_EDP, chicken); > > } > > > > /* > > @@ -1222,7 +1151,7 @@ void intel_psr_init(struct drm_i915_private > > *dev_priv) > > * to avoid any rendering problems. > > */ > > val = I915_READ(EDP_PSR_IIR); > > - val &= EDP_PSR_ERROR(edp_psr_shift(TRANSCODER_EDP)); > > + val &= EDP_PSR_ERROR; > > if (val) { > > DRM_DEBUG_KMS("PSR interruption error set\n"); > > dev_priv->psr.sink_not_reliable = true; > > -- > > 2.21.0 > >
On Thu, Apr 04, 2019 at 12:40:34PM -0700, Souza, Jose wrote: > On Wed, 2019-04-03 at 17:31 -0700, Rodrigo Vivi wrote: > > On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de Souza > > wrote: > > > PSR is only supported in eDP transcoder and there is only one > > > instance of it, so lets drop all of this code. > > > > Is this sentence true? I mean, in the way it is written it > > seems like HW doesn't actually support it... > > Or should we re-phrase for we are not really enabling support > > for other transcoders than eDP and we do not have plans to do > > it so soon so let's clean the code... > > or something like that? > > Okay, what about replace it for: > > Since BDW all transcoders have PSR registers but only EDP transcoder > can drive a EDP panel and as PSR is only part of the EDP specification, > for real usage PSR is only supported in EDP panel, so lets drop all of > this useless code. well, this still looks like HW limitation. If PSR is supported by HW on different transcoders is because there's some possibility of adding eDP on other transcoders. They wouldn't waste so many register space if that wasn't the case. Even though we have a dedicated transcoder for eDP I don't believe we can assume that eDP is not supported on the other ones. > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_reg.h | 17 +--- > > > drivers/gpu/drm/i915/intel_psr.c | 147 ++++++++------------------- > > > ---- > > > 2 files changed, 42 insertions(+), 122 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > > b/drivers/gpu/drm/i915/i915_reg.h > > > index c59cfa83dbaf..18e2991b376d 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -4241,13 +4241,9 @@ enum { > > > /* Bspec claims those aren't shifted but stay at 0x64800 */ > > > #define EDP_PSR_IMR _MMIO(0x64834) > > > #define EDP_PSR_IIR _MMIO(0x64838) > > > -#define EDP_PSR_ERROR(shift) (1 << ((shift) > > > + 2)) > > > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1)) > > > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift)) > > > -#define EDP_PSR_TRANSCODER_C_SHIFT 24 > > > -#define EDP_PSR_TRANSCODER_B_SHIFT 16 > > > -#define EDP_PSR_TRANSCODER_A_SHIFT 8 > > > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0 > > > +#define EDP_PSR_ERROR (1 << 2) > > > +#define EDP_PSR_POST_EXIT (1 << 1) > > > +#define EDP_PSR_PRE_ENTRY (1 << 0) > > > > > > #define EDP_PSR_AUX_CTL _MMIO(dev_priv- > > > >psr_mmio_base + 0x10) > > > #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26) > > > @@ -4312,12 +4308,7 @@ enum { > > > #define EDP_PSR2_IDLE_FRAME_MASK 0xf > > > #define EDP_PSR2_IDLE_FRAME_SHIFT 0 > > > > > > -#define _PSR_EVENT_TRANS_A 0x60848 > > > -#define _PSR_EVENT_TRANS_B 0x61848 > > > -#define _PSR_EVENT_TRANS_C 0x62848 > > > -#define _PSR_EVENT_TRANS_D 0x63848 > > > -#define _PSR_EVENT_TRANS_EDP 0x6F848 > > > -#define PSR_EVENT(trans) _MMIO_TRANS2(trans, > > > _PSR_EVENT_TRANS_A) > > > +#define PSR_EVENT _MMIO(0x6F848) > > > #define PSR_EVENT_PSR2_WD_TIMER_EXPIRE (1 << 17) > > > #define PSR_EVENT_PSR2_DISABLED (1 << 16) > > > #define PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN (1 << 15) > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index bb97c1657493..b984e005b72e 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct > > > drm_i915_private *dev_priv, > > > } > > > } > > > > > > -static int edp_psr_shift(enum transcoder cpu_transcoder) > > > -{ > > > - switch (cpu_transcoder) { > > > - case TRANSCODER_A: > > > - return EDP_PSR_TRANSCODER_A_SHIFT; > > > - case TRANSCODER_B: > > > - return EDP_PSR_TRANSCODER_B_SHIFT; > > > - case TRANSCODER_C: > > > - return EDP_PSR_TRANSCODER_C_SHIFT; > > > - default: > > > - MISSING_CASE(cpu_transcoder); > > > - /* fallthrough */ > > > - case TRANSCODER_EDP: > > > - return EDP_PSR_TRANSCODER_EDP_SHIFT; > > > - } > > > -} > > > - > > > void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 > > > debug) > > > { > > > - u32 debug_mask, mask; > > > - enum transcoder cpu_transcoder; > > > - u32 transcoders = BIT(TRANSCODER_EDP); > > > - > > > - if (INTEL_GEN(dev_priv) >= 8) > > > - transcoders |= BIT(TRANSCODER_A) | > > > - BIT(TRANSCODER_B) | > > > - BIT(TRANSCODER_C); > > > - > > > - debug_mask = 0; > > > - mask = 0; > > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, > > > transcoders) { > > > - int shift = edp_psr_shift(cpu_transcoder); > > > - > > > - mask |= EDP_PSR_ERROR(shift); > > > - debug_mask |= EDP_PSR_POST_EXIT(shift) | > > > - EDP_PSR_PRE_ENTRY(shift); > > > - } > > > + u32 mask = EDP_PSR_ERROR; > > > > > > if (debug & I915_PSR_DEBUG_IRQ) > > > - mask |= debug_mask; > > > + mask |= EDP_PSR_POST_EXIT | EDP_PSR_PRE_ENTRY; > > > > > > I915_WRITE(EDP_PSR_IMR, ~mask); > > > } > > > @@ -167,62 +133,47 @@ static void psr_event_print(u32 val, bool > > > psr2_enabled) > > > > > > void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 > > > psr_iir) > > > { > > > - u32 transcoders = BIT(TRANSCODER_EDP); > > > - enum transcoder cpu_transcoder; > > > - ktime_t time_ns = ktime_get(); > > > - u32 mask = 0; > > > + ktime_t time_ns = ktime_get(); > > > > > > - if (INTEL_GEN(dev_priv) >= 8) > > > - transcoders |= BIT(TRANSCODER_A) | > > > - BIT(TRANSCODER_B) | > > > - BIT(TRANSCODER_C); > > > - > > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, > > > transcoders) { > > > - int shift = edp_psr_shift(cpu_transcoder); > > > - > > > - if (psr_iir & EDP_PSR_ERROR(shift)) { > > > - DRM_WARN("[transcoder %s] PSR aux error\n", > > > - transcoder_name(cpu_transcoder)); > > > - > > > - dev_priv->psr.irq_aux_error = true; > > > - > > > - /* > > > - * If this interruption is not masked it will > > > keep > > > - * interrupting so fast that it prevents the > > > scheduled > > > - * work to run. > > > - * Also after a PSR error, we don't want to arm > > > PSR > > > - * again so we don't care about unmask the > > > interruption > > > - * or unset irq_aux_error. > > > - */ > > > - mask |= EDP_PSR_ERROR(shift); > > > - } > > > + if (psr_iir & EDP_PSR_ERROR) { > > > + u32 mask; > > > > > > - if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) { > > > - dev_priv->psr.last_entry_attempt = time_ns; > > > - DRM_DEBUG_KMS("[transcoder %s] PSR entry > > > attempt in 2 vblanks\n", > > > - transcoder_name(cpu_transcoder)); > > > - } > > > + DRM_WARN("[transcoder %s] PSR aux error\n", > > > + transcoder_name(TRANSCODER_EDP)); > > > > > > - if (psr_iir & EDP_PSR_POST_EXIT(shift)) { > > > - dev_priv->psr.last_exit = time_ns; > > > - DRM_DEBUG_KMS("[transcoder %s] PSR exit > > > completed\n", > > > - transcoder_name(cpu_transcoder)); > > > + dev_priv->psr.irq_aux_error = true; > > > > > > - if (INTEL_GEN(dev_priv) >= 9) { > > > - u32 val = > > > I915_READ(PSR_EVENT(cpu_transcoder)); > > > - bool psr2_enabled = dev_priv- > > > >psr.psr2_enabled; > > > + /* > > > + * If this interruption is not masked it will keep > > > + * interrupting so fast that it prevents the scheduled > > > + * work to run. > > > + * Also after a PSR error, we don't want to arm PSR > > > + * again so we don't care about unmask the interruption > > > + * or unset irq_aux_error. > > > + */ > > > + mask = I915_READ(EDP_PSR_IMR) | EDP_PSR_ERROR; > > > + I915_WRITE(EDP_PSR_IMR, mask); > > > > > > - I915_WRITE(PSR_EVENT(cpu_transcoder), > > > val); > > > - psr_event_print(val, psr2_enabled); > > > - } > > > - } > > > + schedule_work(&dev_priv->psr.work); > > > } > > > > > > - if (mask) { > > > - mask |= I915_READ(EDP_PSR_IMR); > > > - I915_WRITE(EDP_PSR_IMR, mask); > > > + if (psr_iir & EDP_PSR_PRE_ENTRY) { > > > + dev_priv->psr.last_entry_attempt = time_ns; > > > + DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 > > > vblanks\n", > > > + transcoder_name(TRANSCODER_EDP)); > > > + } > > > > > > - schedule_work(&dev_priv->psr.work); > > > + if (psr_iir & EDP_PSR_POST_EXIT) { > > > + DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n", > > > + transcoder_name(TRANSCODER_EDP)); > > > + > > > + if (INTEL_GEN(dev_priv) >= 9) { > > > + u32 val = I915_READ(PSR_EVENT); > > > + bool psr2_enabled = dev_priv->psr.psr2_enabled; > > > + > > > + I915_WRITE(PSR_EVENT, val); > > > + psr_event_print(val, psr2_enabled); > > > + } > > > } > > > } > > > > > > @@ -669,30 +620,10 @@ static void intel_psr_activate(struct > > > intel_dp *intel_dp) > > > dev_priv->psr.active = true; > > > } > > > > > > -static i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private > > > *dev_priv, > > > - enum transcoder > > > cpu_transcoder) > > > -{ > > > - static const i915_reg_t regs[] = { > > > - [TRANSCODER_A] = CHICKEN_TRANS_A, > > > - [TRANSCODER_B] = CHICKEN_TRANS_B, > > > - [TRANSCODER_C] = CHICKEN_TRANS_C, > > > - [TRANSCODER_EDP] = CHICKEN_TRANS_EDP, > > > - }; > > > - > > > - WARN_ON(INTEL_GEN(dev_priv) < 9); > > > - > > > - if (WARN_ON(cpu_transcoder >= ARRAY_SIZE(regs) || > > > - !regs[cpu_transcoder].reg)) > > > - cpu_transcoder = TRANSCODER_A; > > > - > > > - return regs[cpu_transcoder]; > > > -} > > > - > > > static void intel_psr_enable_source(struct intel_dp *intel_dp, > > > const struct intel_crtc_state > > > *crtc_state) > > > { > > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > - enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > > > u32 mask; > > > > > > /* Only HSW and BDW have PSR AUX registers that need to be > > > setup. SKL+ > > > @@ -703,13 +634,11 @@ static void intel_psr_enable_source(struct > > > intel_dp *intel_dp, > > > > > > if (dev_priv->psr.psr2_enabled && (IS_GEN(dev_priv, 9) && > > > !IS_GEMINILAKE(dev_priv))) { > > > - i915_reg_t reg = gen9_chicken_trans_reg(dev_priv, > > > - cpu_transcoder) > > > ; > > > - u32 chicken = I915_READ(reg); > > > + u32 chicken = I915_READ(CHICKEN_TRANS_EDP); > > > > > > chicken |= PSR2_VSC_ENABLE_PROG_HEADER | > > > PSR2_ADD_VERTICAL_LINE_COUNT; > > > - I915_WRITE(reg, chicken); > > > + I915_WRITE(CHICKEN_TRANS_EDP, chicken); > > > } > > > > > > /* > > > @@ -1222,7 +1151,7 @@ void intel_psr_init(struct drm_i915_private > > > *dev_priv) > > > * to avoid any rendering problems. > > > */ > > > val = I915_READ(EDP_PSR_IIR); > > > - val &= EDP_PSR_ERROR(edp_psr_shift(TRANSCODER_EDP)); > > > + val &= EDP_PSR_ERROR; > > > if (val) { > > > DRM_DEBUG_KMS("PSR interruption error set\n"); > > > dev_priv->psr.sink_not_reliable = true; > > > -- > > > 2.21.0 > > >
On Thu, 2019-04-04 at 14:20 -0700, Rodrigo Vivi wrote: > On Thu, Apr 04, 2019 at 12:40:34PM -0700, Souza, Jose wrote: > > On Wed, 2019-04-03 at 17:31 -0700, Rodrigo Vivi wrote: > > > On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de Souza > > > wrote: > > > > PSR is only supported in eDP transcoder and there is only one > > > > instance of it, so lets drop all of this code. > > > > > > Is this sentence true? I mean, in the way it is written it > > > seems like HW doesn't actually support it... > > > Or should we re-phrase for we are not really enabling support > > > for other transcoders than eDP and we do not have plans to do > > > it so soon so let's clean the code... > > > or something like that? > > > > Okay, what about replace it for: > > > > Since BDW all transcoders have PSR registers but only EDP transcoder > > can drive a EDP panel and as PSR is only part of the EDP specification, > > for real usage PSR is only supported in EDP panel, so lets drop all of > > this useless code. > > well, this still looks like HW limitation. If PSR is supported by HW on > different transcoders is because there's some possibility of adding > eDP on other transcoders. They wouldn't waste so many register space > if that wasn't the case. > > Even though we have a dedicated transcoder for eDP I don't > believe we can assume that eDP is not supported on the other ones. > Why not write something like (or exactly) this "i915 does not support enabling PSR on any transcoder other than eDP. Clean up the misleading non-eDP code that currently exists to allow for the rework of PSR register definitions in the next patch" -DK > > > > > > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_reg.h | 17 +--- > > > > drivers/gpu/drm/i915/intel_psr.c | 147 ++++++++------------------- > > > > ---- > > > > 2 files changed, 42 insertions(+), 122 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > > > b/drivers/gpu/drm/i915/i915_reg.h > > > > index c59cfa83dbaf..18e2991b376d 100644 > > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > > @@ -4241,13 +4241,9 @@ enum { > > > > /* Bspec claims those aren't shifted but stay at 0x64800 */ > > > > #define EDP_PSR_IMR _MMIO(0x64834) > > > > #define EDP_PSR_IIR _MMIO(0x64838) > > > > -#define EDP_PSR_ERROR(shift) (1 << ((shift) > > > > + 2)) > > > > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1)) > > > > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift)) > > > > -#define EDP_PSR_TRANSCODER_C_SHIFT 24 > > > > -#define EDP_PSR_TRANSCODER_B_SHIFT 16 > > > > -#define EDP_PSR_TRANSCODER_A_SHIFT 8 > > > > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0 > > > > +#define EDP_PSR_ERROR (1 << 2) > > > > +#define EDP_PSR_POST_EXIT (1 << 1) > > > > +#define EDP_PSR_PRE_ENTRY (1 << 0) > > > > > > > > #define EDP_PSR_AUX_CTL _MMIO(dev_priv- > > > > > psr_mmio_base + 0x10) > > > > > > > > #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26) > > > > @@ -4312,12 +4308,7 @@ enum { > > > > #define EDP_PSR2_IDLE_FRAME_MASK 0xf > > > > #define EDP_PSR2_IDLE_FRAME_SHIFT 0 > > > > > > > > -#define _PSR_EVENT_TRANS_A 0x60848 > > > > -#define _PSR_EVENT_TRANS_B 0x61848 > > > > -#define _PSR_EVENT_TRANS_C 0x62848 > > > > -#define _PSR_EVENT_TRANS_D 0x63848 > > > > -#define _PSR_EVENT_TRANS_EDP 0x6F848 > > > > -#define PSR_EVENT(trans) _MMIO_TRANS2(trans, > > > > _PSR_EVENT_TRANS_A) > > > > +#define PSR_EVENT _MMIO(0x6F848) > > > > #define PSR_EVENT_PSR2_WD_TIMER_EXPIRE (1 << 17) > > > > #define PSR_EVENT_PSR2_DISABLED (1 << 16) > > > > #define PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN (1 << 15) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > > b/drivers/gpu/drm/i915/intel_psr.c > > > > index bb97c1657493..b984e005b72e 100644 > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct > > > > drm_i915_private *dev_priv, > > > > } > > > > } > > > > > > > > -static int edp_psr_shift(enum transcoder cpu_transcoder) > > > > -{ > > > > - switch (cpu_transcoder) { > > > > - case TRANSCODER_A: > > > > - return EDP_PSR_TRANSCODER_A_SHIFT; > > > > - case TRANSCODER_B: > > > > - return EDP_PSR_TRANSCODER_B_SHIFT; > > > > - case TRANSCODER_C: > > > > - return EDP_PSR_TRANSCODER_C_SHIFT; > > > > - default: > > > > - MISSING_CASE(cpu_transcoder); > > > > - /* fallthrough */ > > > > - case TRANSCODER_EDP: > > > > - return EDP_PSR_TRANSCODER_EDP_SHIFT; > > > > - } > > > > -} > > > > - > > > > void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 > > > > debug) > > > > { > > > > - u32 debug_mask, mask; > > > > - enum transcoder cpu_transcoder; > > > > - u32 transcoders = BIT(TRANSCODER_EDP); > > > > - > > > > - if (INTEL_GEN(dev_priv) >= 8) > > > > - transcoders |= BIT(TRANSCODER_A) | > > > > - BIT(TRANSCODER_B) | > > > > - BIT(TRANSCODER_C); > > > > - > > > > - debug_mask = 0; > > > > - mask = 0; > > > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, > > > > transcoders) { > > > > - int shift = edp_psr_shift(cpu_transcoder); > > > > - > > > > - mask |= EDP_PSR_ERROR(shift); > > > > - debug_mask |= EDP_PSR_POST_EXIT(shift) | > > > > - EDP_PSR_PRE_ENTRY(shift); > > > > - } > > > > + u32 mask = EDP_PSR_ERROR; > > > > > > > > if (debug & I915_PSR_DEBUG_IRQ) > > > > - mask |= debug_mask; > > > > + mask |= EDP_PSR_POST_EXIT | EDP_PSR_PRE_ENTRY; > > > > > > > > I915_WRITE(EDP_PSR_IMR, ~mask); > > > > } > > > > @@ -167,62 +133,47 @@ static void psr_event_print(u32 val, bool > > > > psr2_enabled) > > > > > > > > void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 > > > > psr_iir) > > > > { > > > > - u32 transcoders = BIT(TRANSCODER_EDP); > > > > - enum transcoder cpu_transcoder; > > > > - ktime_t time_ns = ktime_get(); > > > > - u32 mask = 0; > > > > + ktime_t time_ns = ktime_get(); > > > > > > > > - if (INTEL_GEN(dev_priv) >= 8) > > > > - transcoders |= BIT(TRANSCODER_A) | > > > > - BIT(TRANSCODER_B) | > > > > - BIT(TRANSCODER_C); > > > > - > > > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, > > > > transcoders) { > > > > - int shift = edp_psr_shift(cpu_transcoder); > > > > - > > > > - if (psr_iir & EDP_PSR_ERROR(shift)) { > > > > - DRM_WARN("[transcoder %s] PSR aux error\n", > > > > - transcoder_name(cpu_transcoder)); > > > > - > > > > - dev_priv->psr.irq_aux_error = true; > > > > - > > > > - /* > > > > - * If this interruption is not masked it will > > > > keep > > > > - * interrupting so fast that it prevents the > > > > scheduled > > > > - * work to run. > > > > - * Also after a PSR error, we don't want to arm > > > > PSR > > > > - * again so we don't care about unmask the > > > > interruption > > > > - * or unset irq_aux_error. > > > > - */ > > > > - mask |= EDP_PSR_ERROR(shift); > > > > - } > > > > + if (psr_iir & EDP_PSR_ERROR) { > > > > + u32 mask; > > > > > > > > - if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) { > > > > - dev_priv->psr.last_entry_attempt = time_ns; > > > > - DRM_DEBUG_KMS("[transcoder %s] PSR entry > > > > attempt in 2 vblanks\n", > > > > - transcoder_name(cpu_transcoder)); > > > > - } > > > > + DRM_WARN("[transcoder %s] PSR aux error\n", > > > > + transcoder_name(TRANSCODER_EDP)); > > > > > > > > - if (psr_iir & EDP_PSR_POST_EXIT(shift)) { > > > > - dev_priv->psr.last_exit = time_ns; > > > > - DRM_DEBUG_KMS("[transcoder %s] PSR exit > > > > completed\n", > > > > - transcoder_name(cpu_transcoder)); > > > > + dev_priv->psr.irq_aux_error = true; > > > > > > > > - if (INTEL_GEN(dev_priv) >= 9) { > > > > - u32 val = > > > > I915_READ(PSR_EVENT(cpu_transcoder)); > > > > - bool psr2_enabled = dev_priv- > > > > > psr.psr2_enabled; > > > > > > > > + /* > > > > + * If this interruption is not masked it will keep > > > > + * interrupting so fast that it prevents the scheduled > > > > + * work to run. > > > > + * Also after a PSR error, we don't want to arm PSR > > > > + * again so we don't care about unmask the interruption > > > > + * or unset irq_aux_error. > > > > + */ > > > > + mask = I915_READ(EDP_PSR_IMR) | EDP_PSR_ERROR; > > > > + I915_WRITE(EDP_PSR_IMR, mask); > > > > > > > > - I915_WRITE(PSR_EVENT(cpu_transcoder), > > > > val); > > > > - psr_event_print(val, psr2_enabled); > > > > - } > > > > - } > > > > + schedule_work(&dev_priv->psr.work); > > > > } > > > > > > > > - if (mask) { > > > > - mask |= I915_READ(EDP_PSR_IMR); > > > > - I915_WRITE(EDP_PSR_IMR, mask); > > > > + if (psr_iir & EDP_PSR_PRE_ENTRY) { > > > > + dev_priv->psr.last_entry_attempt = time_ns; > > > > + DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 > > > > vblanks\n", > > > > + transcoder_name(TRANSCODER_EDP)); > > > > + } > > > > > > > > - schedule_work(&dev_priv->psr.work); > > > > + if (psr_iir & EDP_PSR_POST_EXIT) { > > > > + DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n", > > > > + transcoder_name(TRANSCODER_EDP)); > > > > + > > > > + if (INTEL_GEN(dev_priv) >= 9) { > > > > + u32 val = I915_READ(PSR_EVENT); > > > > + bool psr2_enabled = dev_priv->psr.psr2_enabled; > > > > + > > > > + I915_WRITE(PSR_EVENT, val); > > > > + psr_event_print(val, psr2_enabled); > > > > + } > > > > } > > > > } > > > > > > > > @@ -669,30 +620,10 @@ static void intel_psr_activate(struct > > > > intel_dp *intel_dp) > > > > dev_priv->psr.active = true; > > > > } > > > > > > > > -static i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private > > > > *dev_priv, > > > > - enum transcoder > > > > cpu_transcoder) > > > > -{ > > > > - static const i915_reg_t regs[] = { > > > > - [TRANSCODER_A] = CHICKEN_TRANS_A, > > > > - [TRANSCODER_B] = CHICKEN_TRANS_B, > > > > - [TRANSCODER_C] = CHICKEN_TRANS_C, > > > > - [TRANSCODER_EDP] = CHICKEN_TRANS_EDP, > > > > - }; > > > > - > > > > - WARN_ON(INTEL_GEN(dev_priv) < 9); > > > > - > > > > - if (WARN_ON(cpu_transcoder >= ARRAY_SIZE(regs) || > > > > - !regs[cpu_transcoder].reg)) > > > > - cpu_transcoder = TRANSCODER_A; > > > > - > > > > - return regs[cpu_transcoder]; > > > > -} > > > > - > > > > static void intel_psr_enable_source(struct intel_dp *intel_dp, > > > > const struct intel_crtc_state > > > > *crtc_state) > > > > { > > > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > > - enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > > > > u32 mask; > > > > > > > > /* Only HSW and BDW have PSR AUX registers that need to be > > > > setup. SKL+ > > > > @@ -703,13 +634,11 @@ static void intel_psr_enable_source(struct > > > > intel_dp *intel_dp, > > > > > > > > if (dev_priv->psr.psr2_enabled && (IS_GEN(dev_priv, 9) && > > > > !IS_GEMINILAKE(dev_priv))) { > > > > - i915_reg_t reg = gen9_chicken_trans_reg(dev_priv, > > > > - cpu_transcoder) > > > > ; > > > > - u32 chicken = I915_READ(reg); > > > > + u32 chicken = I915_READ(CHICKEN_TRANS_EDP); > > > > > > > > chicken |= PSR2_VSC_ENABLE_PROG_HEADER | > > > > PSR2_ADD_VERTICAL_LINE_COUNT; > > > > - I915_WRITE(reg, chicken); > > > > + I915_WRITE(CHICKEN_TRANS_EDP, chicken); > > > > } > > > > > > > > /* > > > > @@ -1222,7 +1151,7 @@ void intel_psr_init(struct drm_i915_private > > > > *dev_priv) > > > > * to avoid any rendering problems. > > > > */ > > > > val = I915_READ(EDP_PSR_IIR); > > > > - val &= EDP_PSR_ERROR(edp_psr_shift(TRANSCODER_EDP)); > > > > + val &= EDP_PSR_ERROR; > > > > if (val) { > > > > DRM_DEBUG_KMS("PSR interruption error set\n"); > > > > dev_priv->psr.sink_not_reliable = true; > > > > -- > > > > 2.21.0 > > > > > >
On Thu, Apr 04, 2019 at 02:41:26PM -0700, Pandiyan, Dhinakaran wrote: > On Thu, 2019-04-04 at 14:20 -0700, Rodrigo Vivi wrote: > > On Thu, Apr 04, 2019 at 12:40:34PM -0700, Souza, Jose wrote: > > > On Wed, 2019-04-03 at 17:31 -0700, Rodrigo Vivi wrote: > > > > On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de Souza > > > > wrote: > > > > > PSR is only supported in eDP transcoder and there is only one > > > > > instance of it, so lets drop all of this code. > > > > > > > > Is this sentence true? I mean, in the way it is written it > > > > seems like HW doesn't actually support it... > > > > Or should we re-phrase for we are not really enabling support > > > > for other transcoders than eDP and we do not have plans to do > > > > it so soon so let's clean the code... > > > > or something like that? > > > > > > Okay, what about replace it for: > > > > > > Since BDW all transcoders have PSR registers but only EDP transcoder > > > can drive a EDP panel and as PSR is only part of the EDP specification, > > > for real usage PSR is only supported in EDP panel, so lets drop all of > > > this useless code. > > > > well, this still looks like HW limitation. If PSR is supported by HW on > > different transcoders is because there's some possibility of adding > > eDP on other transcoders. They wouldn't waste so many register space > > if that wasn't the case. > > > > Even though we have a dedicated transcoder for eDP I don't > > believe we can assume that eDP is not supported on the other ones. > > > > Why not write something like (or exactly) this > > "i915 does not support enabling PSR on any transcoder other than eDP. Clean up > the misleading non-eDP code that currently exists to allow for the rework of PSR > register definitions in the next patch" +1 > > > -DK > > > > > > > > > > > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_reg.h | 17 +--- > > > > > drivers/gpu/drm/i915/intel_psr.c | 147 ++++++++------------------- > > > > > ---- > > > > > 2 files changed, 42 insertions(+), 122 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > > > > b/drivers/gpu/drm/i915/i915_reg.h > > > > > index c59cfa83dbaf..18e2991b376d 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > > > @@ -4241,13 +4241,9 @@ enum { > > > > > /* Bspec claims those aren't shifted but stay at 0x64800 */ > > > > > #define EDP_PSR_IMR _MMIO(0x64834) > > > > > #define EDP_PSR_IIR _MMIO(0x64838) > > > > > -#define EDP_PSR_ERROR(shift) (1 << ((shift) > > > > > + 2)) > > > > > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1)) > > > > > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift)) > > > > > -#define EDP_PSR_TRANSCODER_C_SHIFT 24 > > > > > -#define EDP_PSR_TRANSCODER_B_SHIFT 16 > > > > > -#define EDP_PSR_TRANSCODER_A_SHIFT 8 > > > > > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0 > > > > > +#define EDP_PSR_ERROR (1 << 2) > > > > > +#define EDP_PSR_POST_EXIT (1 << 1) > > > > > +#define EDP_PSR_PRE_ENTRY (1 << 0) > > > > > > > > > > #define EDP_PSR_AUX_CTL _MMIO(dev_priv- > > > > > > psr_mmio_base + 0x10) > > > > > > > > > > #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26) > > > > > @@ -4312,12 +4308,7 @@ enum { > > > > > #define EDP_PSR2_IDLE_FRAME_MASK 0xf > > > > > #define EDP_PSR2_IDLE_FRAME_SHIFT 0 > > > > > > > > > > -#define _PSR_EVENT_TRANS_A 0x60848 > > > > > -#define _PSR_EVENT_TRANS_B 0x61848 > > > > > -#define _PSR_EVENT_TRANS_C 0x62848 > > > > > -#define _PSR_EVENT_TRANS_D 0x63848 > > > > > -#define _PSR_EVENT_TRANS_EDP 0x6F848 > > > > > -#define PSR_EVENT(trans) _MMIO_TRANS2(trans, > > > > > _PSR_EVENT_TRANS_A) > > > > > +#define PSR_EVENT _MMIO(0x6F848) > > > > > #define PSR_EVENT_PSR2_WD_TIMER_EXPIRE (1 << 17) > > > > > #define PSR_EVENT_PSR2_DISABLED (1 << 16) > > > > > #define PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN (1 << 15) > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > > > b/drivers/gpu/drm/i915/intel_psr.c > > > > > index bb97c1657493..b984e005b72e 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > > @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct > > > > > drm_i915_private *dev_priv, > > > > > } > > > > > } > > > > > > > > > > -static int edp_psr_shift(enum transcoder cpu_transcoder) > > > > > -{ > > > > > - switch (cpu_transcoder) { > > > > > - case TRANSCODER_A: > > > > > - return EDP_PSR_TRANSCODER_A_SHIFT; > > > > > - case TRANSCODER_B: > > > > > - return EDP_PSR_TRANSCODER_B_SHIFT; > > > > > - case TRANSCODER_C: > > > > > - return EDP_PSR_TRANSCODER_C_SHIFT; > > > > > - default: > > > > > - MISSING_CASE(cpu_transcoder); > > > > > - /* fallthrough */ > > > > > - case TRANSCODER_EDP: > > > > > - return EDP_PSR_TRANSCODER_EDP_SHIFT; > > > > > - } > > > > > -} > > > > > - > > > > > void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 > > > > > debug) > > > > > { > > > > > - u32 debug_mask, mask; > > > > > - enum transcoder cpu_transcoder; > > > > > - u32 transcoders = BIT(TRANSCODER_EDP); > > > > > - > > > > > - if (INTEL_GEN(dev_priv) >= 8) > > > > > - transcoders |= BIT(TRANSCODER_A) | > > > > > - BIT(TRANSCODER_B) | > > > > > - BIT(TRANSCODER_C); > > > > > - > > > > > - debug_mask = 0; > > > > > - mask = 0; > > > > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, > > > > > transcoders) { > > > > > - int shift = edp_psr_shift(cpu_transcoder); > > > > > - > > > > > - mask |= EDP_PSR_ERROR(shift); > > > > > - debug_mask |= EDP_PSR_POST_EXIT(shift) | > > > > > - EDP_PSR_PRE_ENTRY(shift); > > > > > - } > > > > > + u32 mask = EDP_PSR_ERROR; > > > > > > > > > > if (debug & I915_PSR_DEBUG_IRQ) > > > > > - mask |= debug_mask; > > > > > + mask |= EDP_PSR_POST_EXIT | EDP_PSR_PRE_ENTRY; > > > > > > > > > > I915_WRITE(EDP_PSR_IMR, ~mask); > > > > > } > > > > > @@ -167,62 +133,47 @@ static void psr_event_print(u32 val, bool > > > > > psr2_enabled) > > > > > > > > > > void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 > > > > > psr_iir) > > > > > { > > > > > - u32 transcoders = BIT(TRANSCODER_EDP); > > > > > - enum transcoder cpu_transcoder; > > > > > - ktime_t time_ns = ktime_get(); > > > > > - u32 mask = 0; > > > > > + ktime_t time_ns = ktime_get(); > > > > > > > > > > - if (INTEL_GEN(dev_priv) >= 8) > > > > > - transcoders |= BIT(TRANSCODER_A) | > > > > > - BIT(TRANSCODER_B) | > > > > > - BIT(TRANSCODER_C); > > > > > - > > > > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, > > > > > transcoders) { > > > > > - int shift = edp_psr_shift(cpu_transcoder); > > > > > - > > > > > - if (psr_iir & EDP_PSR_ERROR(shift)) { > > > > > - DRM_WARN("[transcoder %s] PSR aux error\n", > > > > > - transcoder_name(cpu_transcoder)); > > > > > - > > > > > - dev_priv->psr.irq_aux_error = true; > > > > > - > > > > > - /* > > > > > - * If this interruption is not masked it will > > > > > keep > > > > > - * interrupting so fast that it prevents the > > > > > scheduled > > > > > - * work to run. > > > > > - * Also after a PSR error, we don't want to arm > > > > > PSR > > > > > - * again so we don't care about unmask the > > > > > interruption > > > > > - * or unset irq_aux_error. > > > > > - */ > > > > > - mask |= EDP_PSR_ERROR(shift); > > > > > - } > > > > > + if (psr_iir & EDP_PSR_ERROR) { > > > > > + u32 mask; > > > > > > > > > > - if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) { > > > > > - dev_priv->psr.last_entry_attempt = time_ns; > > > > > - DRM_DEBUG_KMS("[transcoder %s] PSR entry > > > > > attempt in 2 vblanks\n", > > > > > - transcoder_name(cpu_transcoder)); > > > > > - } > > > > > + DRM_WARN("[transcoder %s] PSR aux error\n", > > > > > + transcoder_name(TRANSCODER_EDP)); > > > > > > > > > > - if (psr_iir & EDP_PSR_POST_EXIT(shift)) { > > > > > - dev_priv->psr.last_exit = time_ns; > > > > > - DRM_DEBUG_KMS("[transcoder %s] PSR exit > > > > > completed\n", > > > > > - transcoder_name(cpu_transcoder)); > > > > > + dev_priv->psr.irq_aux_error = true; > > > > > > > > > > - if (INTEL_GEN(dev_priv) >= 9) { > > > > > - u32 val = > > > > > I915_READ(PSR_EVENT(cpu_transcoder)); > > > > > - bool psr2_enabled = dev_priv- > > > > > > psr.psr2_enabled; > > > > > > > > > > + /* > > > > > + * If this interruption is not masked it will keep > > > > > + * interrupting so fast that it prevents the scheduled > > > > > + * work to run. > > > > > + * Also after a PSR error, we don't want to arm PSR > > > > > + * again so we don't care about unmask the interruption > > > > > + * or unset irq_aux_error. > > > > > + */ > > > > > + mask = I915_READ(EDP_PSR_IMR) | EDP_PSR_ERROR; > > > > > + I915_WRITE(EDP_PSR_IMR, mask); > > > > > > > > > > - I915_WRITE(PSR_EVENT(cpu_transcoder), > > > > > val); > > > > > - psr_event_print(val, psr2_enabled); > > > > > - } > > > > > - } > > > > > + schedule_work(&dev_priv->psr.work); > > > > > } > > > > > > > > > > - if (mask) { > > > > > - mask |= I915_READ(EDP_PSR_IMR); > > > > > - I915_WRITE(EDP_PSR_IMR, mask); > > > > > + if (psr_iir & EDP_PSR_PRE_ENTRY) { > > > > > + dev_priv->psr.last_entry_attempt = time_ns; > > > > > + DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 > > > > > vblanks\n", > > > > > + transcoder_name(TRANSCODER_EDP)); > > > > > + } > > > > > > > > > > - schedule_work(&dev_priv->psr.work); > > > > > + if (psr_iir & EDP_PSR_POST_EXIT) { > > > > > + DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n", > > > > > + transcoder_name(TRANSCODER_EDP)); > > > > > + > > > > > + if (INTEL_GEN(dev_priv) >= 9) { > > > > > + u32 val = I915_READ(PSR_EVENT); > > > > > + bool psr2_enabled = dev_priv->psr.psr2_enabled; > > > > > + > > > > > + I915_WRITE(PSR_EVENT, val); > > > > > + psr_event_print(val, psr2_enabled); > > > > > + } > > > > > } > > > > > } > > > > > > > > > > @@ -669,30 +620,10 @@ static void intel_psr_activate(struct > > > > > intel_dp *intel_dp) > > > > > dev_priv->psr.active = true; > > > > > } > > > > > > > > > > -static i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private > > > > > *dev_priv, > > > > > - enum transcoder > > > > > cpu_transcoder) > > > > > -{ > > > > > - static const i915_reg_t regs[] = { > > > > > - [TRANSCODER_A] = CHICKEN_TRANS_A, > > > > > - [TRANSCODER_B] = CHICKEN_TRANS_B, > > > > > - [TRANSCODER_C] = CHICKEN_TRANS_C, > > > > > - [TRANSCODER_EDP] = CHICKEN_TRANS_EDP, > > > > > - }; > > > > > - > > > > > - WARN_ON(INTEL_GEN(dev_priv) < 9); > > > > > - > > > > > - if (WARN_ON(cpu_transcoder >= ARRAY_SIZE(regs) || > > > > > - !regs[cpu_transcoder].reg)) > > > > > - cpu_transcoder = TRANSCODER_A; > > > > > - > > > > > - return regs[cpu_transcoder]; > > > > > -} > > > > > - > > > > > static void intel_psr_enable_source(struct intel_dp *intel_dp, > > > > > const struct intel_crtc_state > > > > > *crtc_state) > > > > > { > > > > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > > > - enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > > > > > u32 mask; > > > > > > > > > > /* Only HSW and BDW have PSR AUX registers that need to be > > > > > setup. SKL+ > > > > > @@ -703,13 +634,11 @@ static void intel_psr_enable_source(struct > > > > > intel_dp *intel_dp, > > > > > > > > > > if (dev_priv->psr.psr2_enabled && (IS_GEN(dev_priv, 9) && > > > > > !IS_GEMINILAKE(dev_priv))) { > > > > > - i915_reg_t reg = gen9_chicken_trans_reg(dev_priv, > > > > > - cpu_transcoder) > > > > > ; > > > > > - u32 chicken = I915_READ(reg); > > > > > + u32 chicken = I915_READ(CHICKEN_TRANS_EDP); > > > > > > > > > > chicken |= PSR2_VSC_ENABLE_PROG_HEADER | > > > > > PSR2_ADD_VERTICAL_LINE_COUNT; > > > > > - I915_WRITE(reg, chicken); > > > > > + I915_WRITE(CHICKEN_TRANS_EDP, chicken); > > > > > } > > > > > > > > > > /* > > > > > @@ -1222,7 +1151,7 @@ void intel_psr_init(struct drm_i915_private > > > > > *dev_priv) > > > > > * to avoid any rendering problems. > > > > > */ > > > > > val = I915_READ(EDP_PSR_IIR); > > > > > - val &= EDP_PSR_ERROR(edp_psr_shift(TRANSCODER_EDP)); > > > > > + val &= EDP_PSR_ERROR; > > > > > if (val) { > > > > > DRM_DEBUG_KMS("PSR interruption error set\n"); > > > > > dev_priv->psr.sink_not_reliable = true; > > > > > -- > > > > > 2.21.0 > > > > > > > > >
On Thu, 2019-04-04 at 14:41 -0700, Dhinakaran Pandiyan wrote: > On Thu, 2019-04-04 at 14:20 -0700, Rodrigo Vivi wrote: > > On Thu, Apr 04, 2019 at 12:40:34PM -0700, Souza, Jose wrote: > > > On Wed, 2019-04-03 at 17:31 -0700, Rodrigo Vivi wrote: > > > > On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de Souza > > > > wrote: > > > > > PSR is only supported in eDP transcoder and there is only one > > > > > instance of it, so lets drop all of this code. > > > > > > > > Is this sentence true? I mean, in the way it is written it > > > > seems like HW doesn't actually support it... > > > > Or should we re-phrase for we are not really enabling support > > > > for other transcoders than eDP and we do not have plans to do > > > > it so soon so let's clean the code... > > > > or something like that? > > > > > > Okay, what about replace it for: > > > > > > Since BDW all transcoders have PSR registers but only EDP transcoder > > > can drive a EDP panel and as PSR is only part of the EDP specification, > > > for real usage PSR is only supported in EDP panel, so lets drop all of > > > this useless code. > > > > well, this still looks like HW limitation. If PSR is supported by HW on > > different transcoders is because there's some possibility of adding > > eDP on other transcoders. They wouldn't waste so many register space > > if that wasn't the case. > > > > Even though we have a dedicated transcoder for eDP I don't > > believe we can assume that eDP is not supported on the other ones. > > > > Why not write something like (or exactly) this > > "i915 does not support enabling PSR on any transcoder other than eDP. Clean up > the misleading non-eDP code that currently exists to allow for the rework of > PSR > register definitions in the next patch" > > > -DK > > > > > > > > > > > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_reg.h | 17 +--- > > > > > drivers/gpu/drm/i915/intel_psr.c | 147 ++++++++------------------- > > > > > ---- > > > > > 2 files changed, 42 insertions(+), 122 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > > > > b/drivers/gpu/drm/i915/i915_reg.h > > > > > index c59cfa83dbaf..18e2991b376d 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > > > @@ -4241,13 +4241,9 @@ enum { > > > > > /* Bspec claims those aren't shifted but stay at 0x64800 */ > > > > > #define EDP_PSR_IMR _MMIO(0x64834) > > > > > #define EDP_PSR_IIR _MMIO(0x64838) > > > > > -#define EDP_PSR_ERROR(shift) (1 << ((shift) > > > > > + 2)) > > > > > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1)) > > > > > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift)) > > > > > -#define EDP_PSR_TRANSCODER_C_SHIFT 24 > > > > > -#define EDP_PSR_TRANSCODER_B_SHIFT 16 > > > > > -#define EDP_PSR_TRANSCODER_A_SHIFT 8 > > > > > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0 > > > > > +#define EDP_PSR_ERROR (1 << 2) > > > > > +#define EDP_PSR_POST_EXIT (1 << 1) > > > > > +#define EDP_PSR_PRE_ENTRY (1 << 0) > > > > > > > > > > #define EDP_PSR_AUX_CTL _MMIO(dev_priv- > > > > > > psr_mmio_base + 0x10) > > > > > > > > > > #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26) > > > > > @@ -4312,12 +4308,7 @@ enum { > > > > > #define EDP_PSR2_IDLE_FRAME_MASK 0xf > > > > > #define EDP_PSR2_IDLE_FRAME_SHIFT 0 > > > > > > > > > > -#define _PSR_EVENT_TRANS_A 0x60848 > > > > > -#define _PSR_EVENT_TRANS_B 0x61848 > > > > > -#define _PSR_EVENT_TRANS_C 0x62848 > > > > > -#define _PSR_EVENT_TRANS_D 0x63848 > > > > > -#define _PSR_EVENT_TRANS_EDP 0x6F848 > > > > > -#define PSR_EVENT(trans) _MMIO_TRANS2(trans, > > > > > _PSR_EVENT_TRANS_A) > > > > > +#define PSR_EVENT _MMIO(0x6F848) > > > > > #define PSR_EVENT_PSR2_WD_TIMER_EXPIRE (1 << 17) > > > > > #define PSR_EVENT_PSR2_DISABLED (1 << 16) > > > > > #define PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN (1 << 15) > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > > > b/drivers/gpu/drm/i915/intel_psr.c > > > > > index bb97c1657493..b984e005b72e 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > > @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct > > > > > drm_i915_private *dev_priv, > > > > > } > > > > > } > > > > > > > > > > -static int edp_psr_shift(enum transcoder cpu_transcoder) > > > > > -{ > > > > > - switch (cpu_transcoder) { > > > > > - case TRANSCODER_A: > > > > > - return EDP_PSR_TRANSCODER_A_SHIFT; > > > > > - case TRANSCODER_B: > > > > > - return EDP_PSR_TRANSCODER_B_SHIFT; > > > > > - case TRANSCODER_C: > > > > > - return EDP_PSR_TRANSCODER_C_SHIFT; > > > > > - default: > > > > > - MISSING_CASE(cpu_transcoder); > > > > > - /* fallthrough */ > > > > > - case TRANSCODER_EDP: > > > > > - return EDP_PSR_TRANSCODER_EDP_SHIFT; > > > > > - } > > > > > -} > > > > > - > > > > > void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 > > > > > debug) > > > > > { > > > > > - u32 debug_mask, mask; > > > > > - enum transcoder cpu_transcoder; > > > > > - u32 transcoders = BIT(TRANSCODER_EDP); > > > > > - > > > > > - if (INTEL_GEN(dev_priv) >= 8) > > > > > - transcoders |= BIT(TRANSCODER_A) | > > > > > - BIT(TRANSCODER_B) | > > > > > - BIT(TRANSCODER_C); > > > > > - > > > > > - debug_mask = 0; > > > > > - mask = 0; > > > > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, > > > > > transcoders) { > > > > > - int shift = edp_psr_shift(cpu_transcoder); > > > > > - > > > > > - mask |= EDP_PSR_ERROR(shift); > > > > > - debug_mask |= EDP_PSR_POST_EXIT(shift) | > > > > > - EDP_PSR_PRE_ENTRY(shift); > > > > > - } > > > > > + u32 mask = EDP_PSR_ERROR; > > > > > > > > > > if (debug & I915_PSR_DEBUG_IRQ) > > > > > - mask |= debug_mask; > > > > > + mask |= EDP_PSR_POST_EXIT | EDP_PSR_PRE_ENTRY; > > > > > > > > > > I915_WRITE(EDP_PSR_IMR, ~mask); > > > > > } > > > > > @@ -167,62 +133,47 @@ static void psr_event_print(u32 val, bool > > > > > psr2_enabled) > > > > > > > > > > void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 > > > > > psr_iir) > > > > > { > > > > > - u32 transcoders = BIT(TRANSCODER_EDP); > > > > > - enum transcoder cpu_transcoder; > > > > > - ktime_t time_ns = ktime_get(); > > > > > - u32 mask = 0; > > > > > + ktime_t time_ns = ktime_get(); > > > > > > > > > > - if (INTEL_GEN(dev_priv) >= 8) > > > > > - transcoders |= BIT(TRANSCODER_A) | > > > > > - BIT(TRANSCODER_B) | > > > > > - BIT(TRANSCODER_C); > > > > > - > > > > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, > > > > > transcoders) { > > > > > - int shift = edp_psr_shift(cpu_transcoder); > > > > > - > > > > > - if (psr_iir & EDP_PSR_ERROR(shift)) { > > > > > - DRM_WARN("[transcoder %s] PSR aux error\n", > > > > > - transcoder_name(cpu_transcoder)); > > > > > - > > > > > - dev_priv->psr.irq_aux_error = true; > > > > > - > > > > > - /* > > > > > - * If this interruption is not masked it will > > > > > keep > > > > > - * interrupting so fast that it prevents the > > > > > scheduled > > > > > - * work to run. > > > > > - * Also after a PSR error, we don't want to arm > > > > > PSR > > > > > - * again so we don't care about unmask the > > > > > interruption > > > > > - * or unset irq_aux_error. > > > > > - */ > > > > > - mask |= EDP_PSR_ERROR(shift); > > > > > - } What's really the strategy though? I hope we aren't planning to add this code back again.
On Thu, 2019-04-04 at 14:51 -0700, Rodrigo Vivi wrote: > On Thu, Apr 04, 2019 at 02:41:26PM -0700, Pandiyan, Dhinakaran wrote: > > On Thu, 2019-04-04 at 14:20 -0700, Rodrigo Vivi wrote: > > > On Thu, Apr 04, 2019 at 12:40:34PM -0700, Souza, Jose wrote: > > > > On Wed, 2019-04-03 at 17:31 -0700, Rodrigo Vivi wrote: > > > > > On Wed, Apr 03, 2019 at 04:35:38PM -0700, José Roberto de > > > > > Souza > > > > > wrote: > > > > > > PSR is only supported in eDP transcoder and there is only > > > > > > one > > > > > > instance of it, so lets drop all of this code. > > > > > > > > > > Is this sentence true? I mean, in the way it is written it > > > > > seems like HW doesn't actually support it... > > > > > Or should we re-phrase for we are not really enabling support > > > > > for other transcoders than eDP and we do not have plans to do > > > > > it so soon so let's clean the code... > > > > > or something like that? > > > > > > > > Okay, what about replace it for: > > > > > > > > Since BDW all transcoders have PSR registers but only EDP > > > > transcoder > > > > can drive a EDP panel and as PSR is only part of the EDP > > > > specification, > > > > for real usage PSR is only supported in EDP panel, so lets drop > > > > all of > > > > this useless code. > > > > > > well, this still looks like HW limitation. If PSR is supported by > > > HW on > > > different transcoders is because there's some possibility of > > > adding > > > eDP on other transcoders. They wouldn't waste so many register > > > space > > > if that wasn't the case. > > > > > > Even though we have a dedicated transcoder for eDP I don't > > > believe we can assume that eDP is not supported on the other > > > ones. > > > > > > > Why not write something like (or exactly) this > > > > "i915 does not support enabling PSR on any transcoder other than > > eDP. Clean up > > the misleading non-eDP code that currently exists to allow for the > > rework of PSR > > register definitions in the next patch" > > +1 Sure, going to change to that in the next version. > > > > > -DK > > > > > > > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/i915_reg.h | 17 +--- > > > > > > drivers/gpu/drm/i915/intel_psr.c | 147 ++++++++----------- > > > > > > -------- > > > > > > ---- > > > > > > 2 files changed, 42 insertions(+), 122 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > > > > > b/drivers/gpu/drm/i915/i915_reg.h > > > > > > index c59cfa83dbaf..18e2991b376d 100644 > > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > > > > @@ -4241,13 +4241,9 @@ enum { > > > > > > /* Bspec claims those aren't shifted but stay at 0x64800 > > > > > > */ > > > > > > #define EDP_PSR_IMR _MMIO(0 > > > > > > x64834) > > > > > > #define EDP_PSR_IIR _MMIO(0 > > > > > > x64838) > > > > > > -#define EDP_PSR_ERROR(shift) (1 << > > > > > > ((shift) > > > > > > + 2)) > > > > > > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) > > > > > > + 1)) > > > > > > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift)) > > > > > > -#define EDP_PSR_TRANSCODER_C_SHIFT 24 > > > > > > -#define EDP_PSR_TRANSCODER_B_SHIFT 16 > > > > > > -#define EDP_PSR_TRANSCODER_A_SHIFT 8 > > > > > > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0 > > > > > > +#define EDP_PSR_ERROR (1 << > > > > > > 2) > > > > > > +#define EDP_PSR_POST_EXIT (1 << > > > > > > 1) > > > > > > +#define EDP_PSR_PRE_ENTRY (1 << > > > > > > 0) > > > > > > > > > > > > #define EDP_PSR_AUX_CTL _MMIO(d > > > > > > ev_priv- > > > > > > > psr_mmio_base + 0x10) > > > > > > > > > > > > #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << > > > > > > 26) > > > > > > @@ -4312,12 +4308,7 @@ enum { > > > > > > #define EDP_PSR2_IDLE_FRAME_MASK 0xf > > > > > > #define EDP_PSR2_IDLE_FRAME_SHIFT 0 > > > > > > > > > > > > -#define _PSR_EVENT_TRANS_A 0x60848 > > > > > > -#define _PSR_EVENT_TRANS_B 0x61848 > > > > > > -#define _PSR_EVENT_TRANS_C 0x62848 > > > > > > -#define _PSR_EVENT_TRANS_D 0x63848 > > > > > > -#define _PSR_EVENT_TRANS_EDP 0x6F848 > > > > > > -#define PSR_EVENT(trans) _MMIO_TRANS2(tr > > > > > > ans, > > > > > > _PSR_EVENT_TRANS_A) > > > > > > +#define PSR_EVENT _MMIO(0x6F848) > > > > > > #define PSR_EVENT_PSR2_WD_TIMER_EXPIRE (1 << > > > > > > 17) > > > > > > #define PSR_EVENT_PSR2_DISABLED (1 << 16) > > > > > > #define PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN (1 << 15) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > > > > b/drivers/gpu/drm/i915/intel_psr.c > > > > > > index bb97c1657493..b984e005b72e 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > > > @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct > > > > > > drm_i915_private *dev_priv, > > > > > > } > > > > > > } > > > > > > > > > > > > -static int edp_psr_shift(enum transcoder cpu_transcoder) > > > > > > -{ > > > > > > - switch (cpu_transcoder) { > > > > > > - case TRANSCODER_A: > > > > > > - return EDP_PSR_TRANSCODER_A_SHIFT; > > > > > > - case TRANSCODER_B: > > > > > > - return EDP_PSR_TRANSCODER_B_SHIFT; > > > > > > - case TRANSCODER_C: > > > > > > - return EDP_PSR_TRANSCODER_C_SHIFT; > > > > > > - default: > > > > > > - MISSING_CASE(cpu_transcoder); > > > > > > - /* fallthrough */ > > > > > > - case TRANSCODER_EDP: > > > > > > - return EDP_PSR_TRANSCODER_EDP_SHIFT; > > > > > > - } > > > > > > -} > > > > > > - > > > > > > void intel_psr_irq_control(struct drm_i915_private > > > > > > *dev_priv, u32 > > > > > > debug) > > > > > > { > > > > > > - u32 debug_mask, mask; > > > > > > - enum transcoder cpu_transcoder; > > > > > > - u32 transcoders = BIT(TRANSCODER_EDP); > > > > > > - > > > > > > - if (INTEL_GEN(dev_priv) >= 8) > > > > > > - transcoders |= BIT(TRANSCODER_A) | > > > > > > - BIT(TRANSCODER_B) | > > > > > > - BIT(TRANSCODER_C); > > > > > > - > > > > > > - debug_mask = 0; > > > > > > - mask = 0; > > > > > > - for_each_cpu_transcoder_masked(dev_priv, > > > > > > cpu_transcoder, > > > > > > transcoders) { > > > > > > - int shift = edp_psr_shift(cpu_transcoder); > > > > > > - > > > > > > - mask |= EDP_PSR_ERROR(shift); > > > > > > - debug_mask |= EDP_PSR_POST_EXIT(shift) | > > > > > > - EDP_PSR_PRE_ENTRY(shift); > > > > > > - } > > > > > > + u32 mask = EDP_PSR_ERROR; > > > > > > > > > > > > if (debug & I915_PSR_DEBUG_IRQ) > > > > > > - mask |= debug_mask; > > > > > > + mask |= EDP_PSR_POST_EXIT | EDP_PSR_PRE_ENTRY; > > > > > > > > > > > > I915_WRITE(EDP_PSR_IMR, ~mask); > > > > > > } > > > > > > @@ -167,62 +133,47 @@ static void psr_event_print(u32 val, > > > > > > bool > > > > > > psr2_enabled) > > > > > > > > > > > > void intel_psr_irq_handler(struct drm_i915_private > > > > > > *dev_priv, u32 > > > > > > psr_iir) > > > > > > { > > > > > > - u32 transcoders = BIT(TRANSCODER_EDP); > > > > > > - enum transcoder cpu_transcoder; > > > > > > - ktime_t time_ns = ktime_get(); > > > > > > - u32 mask = 0; > > > > > > + ktime_t time_ns = ktime_get(); > > > > > > > > > > > > - if (INTEL_GEN(dev_priv) >= 8) > > > > > > - transcoders |= BIT(TRANSCODER_A) | > > > > > > - BIT(TRANSCODER_B) | > > > > > > - BIT(TRANSCODER_C); > > > > > > - > > > > > > - for_each_cpu_transcoder_masked(dev_priv, > > > > > > cpu_transcoder, > > > > > > transcoders) { > > > > > > - int shift = edp_psr_shift(cpu_transcoder); > > > > > > - > > > > > > - if (psr_iir & EDP_PSR_ERROR(shift)) { > > > > > > - DRM_WARN("[transcoder %s] PSR aux > > > > > > error\n", > > > > > > - transcoder_name(cpu_transcoder > > > > > > )); > > > > > > - > > > > > > - dev_priv->psr.irq_aux_error = true; > > > > > > - > > > > > > - /* > > > > > > - * If this interruption is not masked > > > > > > it will > > > > > > keep > > > > > > - * interrupting so fast that it > > > > > > prevents the > > > > > > scheduled > > > > > > - * work to run. > > > > > > - * Also after a PSR error, we don't > > > > > > want to arm > > > > > > PSR > > > > > > - * again so we don't care about unmask > > > > > > the > > > > > > interruption > > > > > > - * or unset irq_aux_error. > > > > > > - */ > > > > > > - mask |= EDP_PSR_ERROR(shift); > > > > > > - } > > > > > > + if (psr_iir & EDP_PSR_ERROR) { > > > > > > + u32 mask; > > > > > > > > > > > > - if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) { > > > > > > - dev_priv->psr.last_entry_attempt = > > > > > > time_ns; > > > > > > - DRM_DEBUG_KMS("[transcoder %s] PSR > > > > > > entry > > > > > > attempt in 2 vblanks\n", > > > > > > - transcoder_name(cpu_trans > > > > > > coder)); > > > > > > - } > > > > > > + DRM_WARN("[transcoder %s] PSR aux error\n", > > > > > > + transcoder_name(TRANSCODER_EDP)); > > > > > > > > > > > > - if (psr_iir & EDP_PSR_POST_EXIT(shift)) { > > > > > > - dev_priv->psr.last_exit = time_ns; > > > > > > - DRM_DEBUG_KMS("[transcoder %s] PSR exit > > > > > > completed\n", > > > > > > - transcoder_name(cpu_trans > > > > > > coder)); > > > > > > + dev_priv->psr.irq_aux_error = true; > > > > > > > > > > > > - if (INTEL_GEN(dev_priv) >= 9) { > > > > > > - u32 val = > > > > > > I915_READ(PSR_EVENT(cpu_transcoder)); > > > > > > - bool psr2_enabled = dev_priv- > > > > > > > psr.psr2_enabled; > > > > > > > > > > > > + /* > > > > > > + * If this interruption is not masked it will > > > > > > keep > > > > > > + * interrupting so fast that it prevents the > > > > > > scheduled > > > > > > + * work to run. > > > > > > + * Also after a PSR error, we don't want to arm > > > > > > PSR > > > > > > + * again so we don't care about unmask the > > > > > > interruption > > > > > > + * or unset irq_aux_error. > > > > > > + */ > > > > > > + mask = I915_READ(EDP_PSR_IMR) | EDP_PSR_ERROR; > > > > > > + I915_WRITE(EDP_PSR_IMR, mask); > > > > > > > > > > > > - I915_WRITE(PSR_EVENT(cpu_transc > > > > > > oder), > > > > > > val); > > > > > > - psr_event_print(val, > > > > > > psr2_enabled); > > > > > > - } > > > > > > - } > > > > > > + schedule_work(&dev_priv->psr.work); > > > > > > } > > > > > > > > > > > > - if (mask) { > > > > > > - mask |= I915_READ(EDP_PSR_IMR); > > > > > > - I915_WRITE(EDP_PSR_IMR, mask); > > > > > > + if (psr_iir & EDP_PSR_PRE_ENTRY) { > > > > > > + dev_priv->psr.last_entry_attempt = time_ns; > > > > > > + DRM_DEBUG_KMS("[transcoder %s] PSR entry > > > > > > attempt in 2 > > > > > > vblanks\n", > > > > > > + transcoder_name(TRANSCODER_EDP)); > > > > > > + } > > > > > > > > > > > > - schedule_work(&dev_priv->psr.work); > > > > > > + if (psr_iir & EDP_PSR_POST_EXIT) { > > > > > > + DRM_DEBUG_KMS("[transcoder %s] PSR exit > > > > > > completed\n", > > > > > > + transcoder_name(TRANSCODER_EDP)); > > > > > > + > > > > > > + if (INTEL_GEN(dev_priv) >= 9) { > > > > > > + u32 val = I915_READ(PSR_EVENT); > > > > > > + bool psr2_enabled = dev_priv- > > > > > > >psr.psr2_enabled; > > > > > > + > > > > > > + I915_WRITE(PSR_EVENT, val); > > > > > > + psr_event_print(val, psr2_enabled); > > > > > > + } > > > > > > } > > > > > > } > > > > > > > > > > > > @@ -669,30 +620,10 @@ static void intel_psr_activate(struct > > > > > > intel_dp *intel_dp) > > > > > > dev_priv->psr.active = true; > > > > > > } > > > > > > > > > > > > -static i915_reg_t gen9_chicken_trans_reg(struct > > > > > > drm_i915_private > > > > > > *dev_priv, > > > > > > - enum transcoder > > > > > > cpu_transcoder) > > > > > > -{ > > > > > > - static const i915_reg_t regs[] = { > > > > > > - [TRANSCODER_A] = CHICKEN_TRANS_A, > > > > > > - [TRANSCODER_B] = CHICKEN_TRANS_B, > > > > > > - [TRANSCODER_C] = CHICKEN_TRANS_C, > > > > > > - [TRANSCODER_EDP] = CHICKEN_TRANS_EDP, > > > > > > - }; > > > > > > - > > > > > > - WARN_ON(INTEL_GEN(dev_priv) < 9); > > > > > > - > > > > > > - if (WARN_ON(cpu_transcoder >= ARRAY_SIZE(regs) || > > > > > > - !regs[cpu_transcoder].reg)) > > > > > > - cpu_transcoder = TRANSCODER_A; > > > > > > - > > > > > > - return regs[cpu_transcoder]; > > > > > > -} > > > > > > - > > > > > > static void intel_psr_enable_source(struct intel_dp > > > > > > *intel_dp, > > > > > > const struct > > > > > > intel_crtc_state > > > > > > *crtc_state) > > > > > > { > > > > > > struct drm_i915_private *dev_priv = > > > > > > dp_to_i915(intel_dp); > > > > > > - enum transcoder cpu_transcoder = crtc_state- > > > > > > >cpu_transcoder; > > > > > > u32 mask; > > > > > > > > > > > > /* Only HSW and BDW have PSR AUX registers that need to > > > > > > be > > > > > > setup. SKL+ > > > > > > @@ -703,13 +634,11 @@ static void > > > > > > intel_psr_enable_source(struct > > > > > > intel_dp *intel_dp, > > > > > > > > > > > > if (dev_priv->psr.psr2_enabled && (IS_GEN(dev_priv, 9) > > > > > > && > > > > > > !IS_GEMINILAKE(dev_p > > > > > > riv))) { > > > > > > - i915_reg_t reg = > > > > > > gen9_chicken_trans_reg(dev_priv, > > > > > > - cpu_tra > > > > > > nscoder) > > > > > > ; > > > > > > - u32 chicken = I915_READ(reg); > > > > > > + u32 chicken = I915_READ(CHICKEN_TRANS_EDP); > > > > > > > > > > > > chicken |= PSR2_VSC_ENABLE_PROG_HEADER | > > > > > > PSR2_ADD_VERTICAL_LINE_COUNT; > > > > > > - I915_WRITE(reg, chicken); > > > > > > + I915_WRITE(CHICKEN_TRANS_EDP, chicken); > > > > > > } > > > > > > > > > > > > /* > > > > > > @@ -1222,7 +1151,7 @@ void intel_psr_init(struct > > > > > > drm_i915_private > > > > > > *dev_priv) > > > > > > * to avoid any rendering problems. > > > > > > */ > > > > > > val = I915_READ(EDP_PSR_IIR); > > > > > > - val &= EDP_PSR_ERROR(edp_psr_shift(TRANSCODER_EDP)); > > > > > > + val &= EDP_PSR_ERROR; > > > > > > if (val) { > > > > > > DRM_DEBUG_KMS("PSR interruption error set\n"); > > > > > > dev_priv->psr.sink_not_reliable = true; > > > > > > -- > > > > > > 2.21.0 > > > > > >
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index c59cfa83dbaf..18e2991b376d 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4241,13 +4241,9 @@ enum { /* Bspec claims those aren't shifted but stay at 0x64800 */ #define EDP_PSR_IMR _MMIO(0x64834) #define EDP_PSR_IIR _MMIO(0x64838) -#define EDP_PSR_ERROR(shift) (1 << ((shift) + 2)) -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1)) -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift)) -#define EDP_PSR_TRANSCODER_C_SHIFT 24 -#define EDP_PSR_TRANSCODER_B_SHIFT 16 -#define EDP_PSR_TRANSCODER_A_SHIFT 8 -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0 +#define EDP_PSR_ERROR (1 << 2) +#define EDP_PSR_POST_EXIT (1 << 1) +#define EDP_PSR_PRE_ENTRY (1 << 0) #define EDP_PSR_AUX_CTL _MMIO(dev_priv->psr_mmio_base + 0x10) #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26) @@ -4312,12 +4308,7 @@ enum { #define EDP_PSR2_IDLE_FRAME_MASK 0xf #define EDP_PSR2_IDLE_FRAME_SHIFT 0 -#define _PSR_EVENT_TRANS_A 0x60848 -#define _PSR_EVENT_TRANS_B 0x61848 -#define _PSR_EVENT_TRANS_C 0x62848 -#define _PSR_EVENT_TRANS_D 0x63848 -#define _PSR_EVENT_TRANS_EDP 0x6F848 -#define PSR_EVENT(trans) _MMIO_TRANS2(trans, _PSR_EVENT_TRANS_A) +#define PSR_EVENT _MMIO(0x6F848) #define PSR_EVENT_PSR2_WD_TIMER_EXPIRE (1 << 17) #define PSR_EVENT_PSR2_DISABLED (1 << 16) #define PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN (1 << 15) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index bb97c1657493..b984e005b72e 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -84,46 +84,12 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv, } } -static int edp_psr_shift(enum transcoder cpu_transcoder) -{ - switch (cpu_transcoder) { - case TRANSCODER_A: - return EDP_PSR_TRANSCODER_A_SHIFT; - case TRANSCODER_B: - return EDP_PSR_TRANSCODER_B_SHIFT; - case TRANSCODER_C: - return EDP_PSR_TRANSCODER_C_SHIFT; - default: - MISSING_CASE(cpu_transcoder); - /* fallthrough */ - case TRANSCODER_EDP: - return EDP_PSR_TRANSCODER_EDP_SHIFT; - } -} - void intel_psr_irq_control(struct drm_i915_private *dev_priv, u32 debug) { - u32 debug_mask, mask; - enum transcoder cpu_transcoder; - u32 transcoders = BIT(TRANSCODER_EDP); - - if (INTEL_GEN(dev_priv) >= 8) - transcoders |= BIT(TRANSCODER_A) | - BIT(TRANSCODER_B) | - BIT(TRANSCODER_C); - - debug_mask = 0; - mask = 0; - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) { - int shift = edp_psr_shift(cpu_transcoder); - - mask |= EDP_PSR_ERROR(shift); - debug_mask |= EDP_PSR_POST_EXIT(shift) | - EDP_PSR_PRE_ENTRY(shift); - } + u32 mask = EDP_PSR_ERROR; if (debug & I915_PSR_DEBUG_IRQ) - mask |= debug_mask; + mask |= EDP_PSR_POST_EXIT | EDP_PSR_PRE_ENTRY; I915_WRITE(EDP_PSR_IMR, ~mask); } @@ -167,62 +133,47 @@ static void psr_event_print(u32 val, bool psr2_enabled) void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir) { - u32 transcoders = BIT(TRANSCODER_EDP); - enum transcoder cpu_transcoder; - ktime_t time_ns = ktime_get(); - u32 mask = 0; + ktime_t time_ns = ktime_get(); - if (INTEL_GEN(dev_priv) >= 8) - transcoders |= BIT(TRANSCODER_A) | - BIT(TRANSCODER_B) | - BIT(TRANSCODER_C); - - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) { - int shift = edp_psr_shift(cpu_transcoder); - - if (psr_iir & EDP_PSR_ERROR(shift)) { - DRM_WARN("[transcoder %s] PSR aux error\n", - transcoder_name(cpu_transcoder)); - - dev_priv->psr.irq_aux_error = true; - - /* - * If this interruption is not masked it will keep - * interrupting so fast that it prevents the scheduled - * work to run. - * Also after a PSR error, we don't want to arm PSR - * again so we don't care about unmask the interruption - * or unset irq_aux_error. - */ - mask |= EDP_PSR_ERROR(shift); - } + if (psr_iir & EDP_PSR_ERROR) { + u32 mask; - if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) { - dev_priv->psr.last_entry_attempt = time_ns; - DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n", - transcoder_name(cpu_transcoder)); - } + DRM_WARN("[transcoder %s] PSR aux error\n", + transcoder_name(TRANSCODER_EDP)); - if (psr_iir & EDP_PSR_POST_EXIT(shift)) { - dev_priv->psr.last_exit = time_ns; - DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n", - transcoder_name(cpu_transcoder)); + dev_priv->psr.irq_aux_error = true; - if (INTEL_GEN(dev_priv) >= 9) { - u32 val = I915_READ(PSR_EVENT(cpu_transcoder)); - bool psr2_enabled = dev_priv->psr.psr2_enabled; + /* + * If this interruption is not masked it will keep + * interrupting so fast that it prevents the scheduled + * work to run. + * Also after a PSR error, we don't want to arm PSR + * again so we don't care about unmask the interruption + * or unset irq_aux_error. + */ + mask = I915_READ(EDP_PSR_IMR) | EDP_PSR_ERROR; + I915_WRITE(EDP_PSR_IMR, mask); - I915_WRITE(PSR_EVENT(cpu_transcoder), val); - psr_event_print(val, psr2_enabled); - } - } + schedule_work(&dev_priv->psr.work); } - if (mask) { - mask |= I915_READ(EDP_PSR_IMR); - I915_WRITE(EDP_PSR_IMR, mask); + if (psr_iir & EDP_PSR_PRE_ENTRY) { + dev_priv->psr.last_entry_attempt = time_ns; + DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n", + transcoder_name(TRANSCODER_EDP)); + } - schedule_work(&dev_priv->psr.work); + if (psr_iir & EDP_PSR_POST_EXIT) { + DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n", + transcoder_name(TRANSCODER_EDP)); + + if (INTEL_GEN(dev_priv) >= 9) { + u32 val = I915_READ(PSR_EVENT); + bool psr2_enabled = dev_priv->psr.psr2_enabled; + + I915_WRITE(PSR_EVENT, val); + psr_event_print(val, psr2_enabled); + } } } @@ -669,30 +620,10 @@ static void intel_psr_activate(struct intel_dp *intel_dp) dev_priv->psr.active = true; } -static i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private *dev_priv, - enum transcoder cpu_transcoder) -{ - static const i915_reg_t regs[] = { - [TRANSCODER_A] = CHICKEN_TRANS_A, - [TRANSCODER_B] = CHICKEN_TRANS_B, - [TRANSCODER_C] = CHICKEN_TRANS_C, - [TRANSCODER_EDP] = CHICKEN_TRANS_EDP, - }; - - WARN_ON(INTEL_GEN(dev_priv) < 9); - - if (WARN_ON(cpu_transcoder >= ARRAY_SIZE(regs) || - !regs[cpu_transcoder].reg)) - cpu_transcoder = TRANSCODER_A; - - return regs[cpu_transcoder]; -} - static void intel_psr_enable_source(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); - enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; u32 mask; /* Only HSW and BDW have PSR AUX registers that need to be setup. SKL+ @@ -703,13 +634,11 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp, if (dev_priv->psr.psr2_enabled && (IS_GEN(dev_priv, 9) && !IS_GEMINILAKE(dev_priv))) { - i915_reg_t reg = gen9_chicken_trans_reg(dev_priv, - cpu_transcoder); - u32 chicken = I915_READ(reg); + u32 chicken = I915_READ(CHICKEN_TRANS_EDP); chicken |= PSR2_VSC_ENABLE_PROG_HEADER | PSR2_ADD_VERTICAL_LINE_COUNT; - I915_WRITE(reg, chicken); + I915_WRITE(CHICKEN_TRANS_EDP, chicken); } /* @@ -1222,7 +1151,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv) * to avoid any rendering problems. */ val = I915_READ(EDP_PSR_IIR); - val &= EDP_PSR_ERROR(edp_psr_shift(TRANSCODER_EDP)); + val &= EDP_PSR_ERROR; if (val) { DRM_DEBUG_KMS("PSR interruption error set\n"); dev_priv->psr.sink_not_reliable = true;
PSR is only supported in eDP transcoder and there is only one instance of it, so lets drop all of this code. Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 17 +--- drivers/gpu/drm/i915/intel_psr.c | 147 ++++++++----------------------- 2 files changed, 42 insertions(+), 122 deletions(-)