[Intel-gfx] [PATCH v3 2/7] drm/i915/tgl: Access the right register when handling PSR interruptions
Matt Roper
matthew.d.roper at intel.com
Tue Sep 3 16:52:48 UTC 2019
On Thu, Aug 29, 2019 at 02:25:49AM -0700, Lucas De Marchi wrote:
> From: José Roberto de Souza <jose.souza at intel.com>
>
> For older gens PSR IIR and IMR have fixed addresses. From TGL onwards those
> registers moved to each transcoder offset. The bits for the registers
> are defined without an offset per transcoder as right now we have one
> register per transcoder. So add a fake "trans_shift" when calculating
> the bits offsets: it will be 0 for gen12+ and psr.transcoder otherwise.
>
> v2 (Lucas): change the implementation to use trans_shift instead of
> getting each bit value with a different macro
>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_psr.c | 60 ++++++++++++++++++------
> drivers/gpu/drm/i915/i915_irq.c | 51 +++++++++++++++++---
> drivers/gpu/drm/i915/i915_reg.h | 10 +++-
> 3 files changed, 99 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 6f2bf50b6d80..e01c897ec9f9 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -90,17 +90,32 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
>
> static void psr_irq_control(struct drm_i915_private *dev_priv)
> {
> - enum transcoder trans = dev_priv->psr.transcoder;
> - u32 val, mask;
> + enum transcoder trans_shift;
> + u32 mask, val;
> + i915_reg_t imr_reg;
>
> - mask = EDP_PSR_ERROR(trans);
> + /*
> + * 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 (INTEL_GEN(dev_priv) >= 12) {
> + trans_shift = 0;
> + imr_reg = TRANS_PSR_IMR(dev_priv->psr.transcoder);
> + } else {
> + trans_shift = dev_priv->psr.transcoder;
> + imr_reg = EDP_PSR_IMR;
> + }
> +
> + mask = EDP_PSR_ERROR(trans_shift);
> if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
> - mask |= EDP_PSR_POST_EXIT(trans) | EDP_PSR_PRE_ENTRY(trans);
> + mask |= EDP_PSR_POST_EXIT(trans_shift) |
> + EDP_PSR_PRE_ENTRY(trans_shift);
>
> - val = I915_READ(EDP_PSR_IMR);
> - val &= ~EDP_PSR_TRANS_MASK(trans);
> + val = I915_READ(imr_reg);
> + val &= ~EDP_PSR_TRANS_MASK(trans_shift);
> val |= ~mask;
> - I915_WRITE(EDP_PSR_IMR, val);
> + I915_WRITE(imr_reg, val);
> }
>
> static void psr_event_print(u32 val, bool psr2_enabled)
> @@ -143,15 +158,25 @@ static void psr_event_print(u32 val, bool psr2_enabled)
> void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
> {
> enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
> + enum transcoder trans_shift;
> + i915_reg_t imr_reg;
> ktime_t time_ns = ktime_get();
>
> - if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> + if (INTEL_GEN(dev_priv) >= 12) {
> + trans_shift = 0;
> + imr_reg = TRANS_PSR_IMR(dev_priv->psr.transcoder);
> + } else {
> + trans_shift = dev_priv->psr.transcoder;
> + imr_reg = EDP_PSR_IMR;
> + }
> +
> + if (psr_iir & EDP_PSR_PRE_ENTRY(trans_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));
> }
>
> - if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> + if (psr_iir & EDP_PSR_POST_EXIT(trans_shift)) {
> dev_priv->psr.last_exit = time_ns;
> DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> transcoder_name(cpu_transcoder));
> @@ -165,7 +190,7 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
> }
> }
>
> - if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> + if (psr_iir & EDP_PSR_ERROR(trans_shift)) {
> u32 val;
>
> DRM_WARN("[transcoder %s] PSR aux error\n",
> @@ -181,9 +206,9 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
> * again so we don't care about unmask the interruption
> * or unset irq_aux_error.
> */
> - val = I915_READ(EDP_PSR_IMR);
> - val |= EDP_PSR_ERROR(cpu_transcoder);
> - I915_WRITE(EDP_PSR_IMR, val);
> + val = I915_READ(imr_reg);
> + val |= EDP_PSR_ERROR(trans_shift);
> + I915_WRITE(imr_reg, val);
>
> schedule_work(&dev_priv->psr.work);
> }
> @@ -730,8 +755,13 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
> * first time that PSR HW tries to activate so lets keep PSR disabled
> * to avoid any rendering problems.
> */
> - val = I915_READ(EDP_PSR_IIR);
> - val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
> + if (INTEL_GEN(dev_priv) >= 12) {
> + val = I915_READ(TRANS_PSR_IIR(dev_priv->psr.transcoder));
> + val &= EDP_PSR_ERROR(0);
> + } else {
> + val = I915_READ(EDP_PSR_IIR);
> + val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
> + }
> if (val) {
> dev_priv->psr.sink_not_reliable = true;
> DRM_DEBUG_KMS("PSR interruption error set, not enabling PSR\n");
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 3f1b6ee157ba..73f1bd60bc68 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2656,11 +2656,21 @@ gen8_de_misc_irq_handler(struct drm_i915_private *dev_priv, u32 iir)
> }
>
> if (iir & GEN8_DE_EDP_PSR) {
> - u32 psr_iir = I915_READ(EDP_PSR_IIR);
> + u32 psr_iir;
> + i915_reg_t iir_reg;
> +
> + if (INTEL_GEN(dev_priv) >= 12)
> + iir_reg = TRANS_PSR_IIR(dev_priv->psr.transcoder);
> + else
> + iir_reg = EDP_PSR_IIR;
> +
> + psr_iir = I915_READ(iir_reg);
> + I915_WRITE(iir_reg, psr_iir);
> +
> + if (psr_iir)
> + found = true;
>
> intel_psr_irq_handler(dev_priv, psr_iir);
> - I915_WRITE(EDP_PSR_IIR, psr_iir);
> - found = true;
> }
>
> if (!found)
> @@ -3280,8 +3290,23 @@ static void gen11_irq_reset(struct drm_i915_private *dev_priv)
>
> intel_uncore_write(uncore, GEN11_DISPLAY_INT_CTL, 0);
>
> - intel_uncore_write(uncore, EDP_PSR_IMR, 0xffffffff);
> - intel_uncore_write(uncore, EDP_PSR_IIR, 0xffffffff);
> + if (INTEL_GEN(dev_priv) >= 12) {
> + enum transcoder trans;
> +
> + for (trans = TRANSCODER_A; trans <= TRANSCODER_D; trans++) {
> + enum intel_display_power_domain domain;
> +
> + domain = POWER_DOMAIN_TRANSCODER(trans);
> + if (!intel_display_power_is_enabled(dev_priv, domain))
> + continue;
> +
> + intel_uncore_write(uncore, TRANS_PSR_IMR(trans), 0xffffffff);
> + intel_uncore_write(uncore, TRANS_PSR_IIR(trans), 0xffffffff);
> + }
> + } else {
> + intel_uncore_write(uncore, EDP_PSR_IMR, 0xffffffff);
> + intel_uncore_write(uncore, EDP_PSR_IIR, 0xffffffff);
> + }
>
> for_each_pipe(dev_priv, pipe)
> if (intel_display_power_is_enabled(dev_priv,
> @@ -3794,7 +3819,21 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> else if (IS_BROADWELL(dev_priv))
> de_port_enables |= GEN8_PORT_DP_A_HOTPLUG;
>
> - gen3_assert_iir_is_zero(uncore, EDP_PSR_IIR);
> + if (INTEL_GEN(dev_priv) >= 12) {
> + enum transcoder trans;
> +
> + for (trans = TRANSCODER_A; trans <= TRANSCODER_D; trans++) {
> + enum intel_display_power_domain domain;
> +
> + domain = POWER_DOMAIN_TRANSCODER(trans);
> + if (!intel_display_power_is_enabled(dev_priv, domain))
> + continue;
> +
> + gen3_assert_iir_is_zero(uncore, TRANS_PSR_IIR(trans));
> + }
> + } else {
> + gen3_assert_iir_is_zero(uncore, EDP_PSR_IIR);
> + }
>
> for_each_pipe(dev_priv, pipe) {
> dev_priv->de_irq_mask[pipe] = ~de_pipe_masked;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6e7db9c65981..33d9d141ee8f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4222,9 +4222,17 @@ enum {
> #define EDP_PSR_TP1_TIME_0us (3 << 4)
> #define EDP_PSR_IDLE_FRAME_SHIFT 0
>
> -/* Bspec claims those aren't shifted but stay at 0x64800 */
> +/*
> + * Until TGL, Bspec says IMR/IIR are fixed at 0x648xx. On TGL+ those registers
The way these comments are written (both old and new) makes it sound
like we don't completely trust the bspec for some reason. I'd just drop
the mention of the bspec and say "Until TGL, IMR/IIR are fixed..."
The patch itself looks correct, so
Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> + * are relative to transcoder and bits defined for each one as
> + * if using no shift (i.e. as if it was for TRANSCODER_EDP)
> + */
> #define EDP_PSR_IMR _MMIO(0x64834)
> #define EDP_PSR_IIR _MMIO(0x64838)
> +#define _PSR_IMR_A 0x60814
> +#define _PSR_IIR_A 0x60818
> +#define TRANS_PSR_IMR(tran) _MMIO_TRANS2(tran, _PSR_IMR_A)
> +#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))
> --
> 2.23.0
>
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
More information about the Intel-gfx
mailing list