[Intel-gfx] [PATCH v3 08/23] drm/i915/tgl: Access the right register when handling PSR interruptions
Lucas De Marchi
lucas.demarchi at intel.com
Mon Aug 26 16:56:58 UTC 2019
On Mon, Aug 26, 2019 at 03:23:55PM +0530, Anshuman Gupta wrote:
>On 2019-08-23 at 01:20:40 -0700, Lucas De Marchi wrote:
>> From: José Roberto de Souza <jose.souza at intel.com>
>>
>> For older gens PSR IIR and IMR had a fixed address that was not
>> relative to anything, but from TGL those registers moved to each
>> transcoder offset.
>>
>> So here adding a new macro and a new PSR irq handler with the
>> transcoder parameter.
>>
>There are few minor comments below, apart from below comments
>patch is looks ok to me.
Those are fixed and queued for next revision.
>Reviewed-by: Anshuman Gupta <anshuman.gupta at intel.com>
Thanks
Lucas De Marchi
>> 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 | 67 ++++++++++++++++++------
>> drivers/gpu/drm/i915/display/intel_psr.h | 1 +
>> drivers/gpu/drm/i915/i915_irq.c | 52 +++++++++++++++---
>> drivers/gpu/drm/i915/i915_reg.h | 10 +++-
>> 4 files changed, 107 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> index 2429328f963e..c33aa16ed038 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> @@ -91,20 +91,33 @@ 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;
>> + u32 psr_error, psr_entry, psr_exit, mask, val;
>> + i915_reg_t mask_reg;
>> +
>> + if (INTEL_GEN(dev_priv) >= 12) {
>> + psr_error = TRANS_PSR_ERROR;
>> + psr_entry = TRANS_PSR_PRE_ENTRY;
>> + psr_exit = TRANS_PSR_POST_EXIT;
>> + mask_reg = TRANS_PSR_IMR(trans);
>> + } else {
>> + psr_error = EDP_PSR_ERROR(trans);
>> + psr_entry = EDP_PSR_PRE_ENTRY(trans);
>> + psr_exit = EDP_PSR_POST_EXIT(trans);
>> + mask_reg = EDP_PSR_IMR;
>> + }
>>
>> - mask = EDP_PSR_ERROR(trans);
>> + mask = psr_error;
>> if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
>> - mask |= EDP_PSR_POST_EXIT(trans) | EDP_PSR_PRE_ENTRY(trans);
>> + mask |= psr_exit | psr_entry;
>>
>> /*
>> * TODO: when handling multiple PSR instances a global spinlock will be
>> * needed to synchronize the value of shared register
>> */
>> - val = I915_READ(EDP_PSR_IMR);
>> - val &= ~EDP_PSR_TRANS_MASK(trans);
>> + val = I915_READ(mask_reg);
>> + val &= ~(psr_error | psr_entry | psr_exit);
>> val |= ~mask;
>> - I915_WRITE(EDP_PSR_IMR, val);
>> + I915_WRITE(mask_reg, val);
>> }
>>
>> static void psr_event_print(u32 val, bool psr2_enabled)
>> @@ -147,9 +160,21 @@ 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;
>> + u32 psr_error, psr_entry, psr_exit;
>> ktime_t time_ns = ktime_get();
>>
>> - if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
>> + if (INTEL_GEN(dev_priv) >= 12) {
>> + psr_error = TRANS_PSR_ERROR;
>> + psr_entry = TRANS_PSR_PRE_ENTRY;
>> + psr_exit = TRANS_PSR_POST_EXIT;
>> + } else {
>> + psr_error = EDP_PSR_ERROR(cpu_transcoder);
>> + psr_entry = EDP_PSR_PRE_ENTRY(cpu_transcoder);
>> + psr_exit = EDP_PSR_POST_EXIT(cpu_transcoder);
>> + }
>> +
>> + if (psr_iir & psr_error) {
>> + i915_reg_t mask_reg;
>> u32 val;
>>
>> DRM_WARN("[transcoder %s] PSR aux error\n",
>> @@ -168,20 +193,25 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>> * TODO: when handling multiple PSR instances a global spinlock
>> * will be needed to synchronize the value of shared register
>> */
>> - val = I915_READ(EDP_PSR_IMR);
>> - val |= EDP_PSR_ERROR(cpu_transcoder);
>> - I915_WRITE(EDP_PSR_IMR, val);
>> + if (INTEL_GEN(dev_priv) >= 12)
>> + mask_reg = TRANS_PSR_IMR(cpu_transcoder);
>> + else
>> + mask_reg = EDP_PSR_IMR;
>> +
>> + val = I915_READ(mask_reg);
>> + val |= psr_error;
>> + I915_WRITE(mask_reg, val);
>>
>> schedule_work(&dev_priv->psr.work);
>> }
>>
>> - if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
>> + if (psr_iir & psr_entry) {
>> 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 & psr_exit) {
>> dev_priv->psr.last_exit = time_ns;
>> DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
>> transcoder_name(cpu_transcoder));
>> @@ -632,7 +662,7 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
>> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>>
>> if (INTEL_GEN(dev_priv) >= 9 &&
>> - psr2_supported(dev_priv, dev_priv->psr.transcoder))
>> + transcoder_has_psr2(dev_priv, dev_priv->psr.transcoder))
>> WARN_ON(I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder)) & EDP_PSR2_ENABLE);
>> WARN_ON(I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder)) & EDP_PSR_ENABLE);
>> WARN_ON(dev_priv->psr.active);
>> @@ -730,8 +760,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 &= TRANS_PSR_ERROR;
>> + } 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");
>> @@ -787,7 +822,7 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
>>
>> if (!dev_priv->psr.active) {
>> if (INTEL_GEN(dev_priv) >= 9 &&
>> - psr2_supported(dev_priv, dev_priv->psr.transcoder)) {
>> + transcoder_has_psr2(dev_priv, dev_priv->psr.transcoder)) {
>> val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
>> WARN_ON(val & EDP_PSR2_ENABLE);
>> }
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
>> index 46e4de8b8cd5..6570a23a68b2 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.h
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
>> @@ -6,6 +6,7 @@
>> #ifndef __INTEL_PSR_H__
>> #define __INTEL_PSR_H__
>>
>> +#include "intel_display.h"
>IMO we don't need to include this header.
>> #include "intel_frontbuffer.h"
>>
>> struct drm_i915_private;
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 77391d8325bf..6024a6ef1c76 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -2655,11 +2655,22 @@ 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;
>> +
>> + if (INTEL_GEN(dev_priv) >= 12) {
>> + enum transcoder trans = dev_priv->psr.transcoder;
>> +
>> + psr_iir = I915_READ(TRANS_PSR_IIR(trans));
>> + I915_WRITE(TRANS_PSR_IIR(trans), psr_iir);
>> + } else {
>> + psr_iir = I915_READ(EDP_PSR_IIR);
>> + I915_WRITE(EDP_PSR_IIR, 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)
>> @@ -3279,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);
>Lines over 80 char.
>> + }
>> + } 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,
>> @@ -3793,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 1c6d99944630..3de02683d856 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4222,7 +4222,7 @@ 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 */
>> +/* Bspec claims those aren't shifted but stay at 0x64800 until TGL */
>> #define EDP_PSR_IMR _MMIO(0x64834)
>> #define EDP_PSR_IIR _MMIO(0x64838)
>> #define _EDP_PSR_TRANS_SHIFT(trans) ((trans) == TRANSCODER_EDP ? \
>> @@ -4232,6 +4232,14 @@ enum {
>> #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 _PSR_IMR_A 0x60814
>> +#define _PSR_IIR_A 0x60818
>> +#define TRANS_PSR_IMR(tran) _MMIO_TRANS2(tran, _PSR_IMR_A) /* TGL+ */
>> +#define TRANS_PSR_IIR(tran) _MMIO_TRANS2(tran, _PSR_IIR_A) /* TGL+ */
>IMO if /* TGL+ */ comment shifted above, it will satisfy the 80 char limit.
>> +#define TRANS_PSR_ERROR (1 << 2)
>> +#define TRANS_PSR_POST_EXIT (1 << 1)
>> +#define TRANS_PSR_PRE_ENTRY (1 << 0)
>> +
>> #define _SRD_AUX_CTL_A 0x60810
>> #define _SRD_AUX_CTL_EDP 0x6f810
>> #define EDP_PSR_AUX_CTL(tran) _MMIO(_PSR_ADJ(tran, _SRD_AUX_CTL_A))
>> --
>> 2.23.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list