[Intel-gfx] [PATCH v3 03/23] drm/i915/psr: Only handle interruptions of the transcoder in use
Lucas De Marchi
lucas.demarchi at intel.com
Tue Aug 27 16:50:24 UTC 2019
On Mon, Aug 26, 2019 at 08:28:33PM +0300, Imre Deak wrote:
>On Fri, Aug 23, 2019 at 01:20:35AM -0700, Lucas De Marchi wrote:
>> From: José Roberto de Souza <jose.souza at intel.com>
>>
>> It was enabling and checking PSR interruptions in every transcoder
>> while it should keep the interruptions on the non-used transcoders
>> masked.
>>
>> This also already prepares for future when more than one PSR instance
>> will be allowed.
>>
>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan 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 | 140 +++++++++--------------
>> drivers/gpu/drm/i915/i915_reg.h | 13 +--
>> 2 files changed, 59 insertions(+), 94 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> index 28b62e587204..81e3619cd905 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> @@ -88,48 +88,23 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
>> }
>> }
>>
>> -static int edp_psr_shift(enum transcoder cpu_transcoder)
>> +static void psr_irq_control(struct drm_i915_private *dev_priv)
>> {
>> - 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;
>> - }
>> -}
>> + enum transcoder trans = dev_priv->psr.transcoder;
>
>This is called from intel_psr_debug_set() where psr.transcoder may be
>uninited.
>
>> + u32 val, mask;
>>
>> -static void 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);
>> - }
>> + mask = EDP_PSR_ERROR(trans);
>> + if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
>> + mask |= EDP_PSR_POST_EXIT(trans) | EDP_PSR_PRE_ENTRY(trans);
>>
>> - if (debug & I915_PSR_DEBUG_IRQ)
>> - mask |= debug_mask;
>> -
>> - I915_WRITE(EDP_PSR_IMR, ~mask);
>> + /*
>> + * 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 |= ~mask;
>> + I915_WRITE(EDP_PSR_IMR, val);
>> }
>>
>> static void psr_event_print(u32 val, bool psr2_enabled)
>> @@ -171,63 +146,54 @@ 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;
>> + enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
>> ktime_t time_ns = ktime_get();
>> - u32 mask = 0;
>>
>> - if (INTEL_GEN(dev_priv) >= 8)
>> - transcoders |= BIT(TRANSCODER_A) |
>> - BIT(TRANSCODER_B) |
>> - BIT(TRANSCODER_C);
>> + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
>> + u32 val;
>>
>> - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
>
>I think we should still catch all interrupts, log the unexpected ones
>and react only on the expected one in intel_psr_work().
could you expand more on this? there is only one PSR instance hence only
one possible transcoder coming from dev_priv->psr.transcoder. Looping here just to
warn seems wasteful.
>
>> - int shift = edp_psr_shift(cpu_transcoder);
>> + DRM_WARN("[transcoder %s] PSR aux error\n",
>> + transcoder_name(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;
>>
>> - 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.
>> + *
>> + * TODO: when handling multiple PSR instances a global spinlock
>> + * will be needed to synchronize the value of shared register
I'm not really a fan of these TODO for multiple PSR instances. When/if
we add them, we won't really be able to rely on these TODO comments and
will rather need to evaluate the whole scenario.
>> + */
>> + val = I915_READ(EDP_PSR_IMR);
>> + val |= EDP_PSR_ERROR(cpu_transcoder);
>> + I915_WRITE(EDP_PSR_IMR, val);
>>
>> - /*
>> - * 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);
>> - }
>> + schedule_work(&dev_priv->psr.work);
>
>Would be better not to reorder intel_psr_work() and printing the events
>below.
>
>> + }
>>
>> - 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));
>> - }
>> + if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
>> + 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(shift)) {
>> - dev_priv->psr.last_exit = time_ns;
>> - DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
>> - transcoder_name(cpu_transcoder));
>> + if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
>> + dev_priv->psr.last_exit = time_ns;
>> + DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
>> + transcoder_name(cpu_transcoder));
>>
>> - if (INTEL_GEN(dev_priv) >= 9) {
>> - u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
>> - bool psr2_enabled = dev_priv->psr.psr2_enabled;
>> + if (INTEL_GEN(dev_priv) >= 9) {
>> + u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
>> + bool psr2_enabled = dev_priv->psr.psr2_enabled;
>>
>> - I915_WRITE(PSR_EVENT(cpu_transcoder), val);
>> - psr_event_print(val, psr2_enabled);
>> - }
>> + I915_WRITE(PSR_EVENT(cpu_transcoder), val);
>> + psr_event_print(val, psr2_enabled);
>> }
>> }
>> -
>> - if (mask) {
>> - mask |= I915_READ(EDP_PSR_IMR);
>> - I915_WRITE(EDP_PSR_IMR, mask);
>> -
>> - schedule_work(&dev_priv->psr.work);
>> - }
>> }
>>
>> static bool intel_dp_get_alpm_status(struct intel_dp *intel_dp)
>> @@ -737,7 +703,7 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
>>
>> I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
>>
>> - psr_irq_control(dev_priv, dev_priv->psr.debug);
>> + psr_irq_control(dev_priv);
>> }
>>
>> static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
>> @@ -762,7 +728,7 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
>> * to avoid any rendering problems.
>> */
>> val = I915_READ(EDP_PSR_IIR);
>> - val &= EDP_PSR_ERROR(edp_psr_shift(dev_priv->psr.transcoder));
>> + 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");
>> @@ -1110,7 +1076,7 @@ int intel_psr_debug_set(struct drm_i915_private *dev_priv, u64 val)
>>
>> old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK;
>> dev_priv->psr.debug = val;
>> - psr_irq_control(dev_priv, dev_priv->psr.debug);
>> + psr_irq_control(dev_priv);
>>
>> mutex_unlock(&dev_priv->psr.lock);
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 02e1ef10c47e..1c6d99944630 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4225,13 +4225,12 @@ 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_TRANS_SHIFT(trans) ((trans) == TRANSCODER_EDP ? \
>> + 0 : ((trans) + 1) * 8)
>
>(trans - TRANSCODER_A) + 1
>
>to not depend on the enum value of TRANSCODER_A.
agreed
thanks
Lucas De Marchi
>
>> +#define EDP_PSR_TRANS_MASK(trans) (0x7 << _EDP_PSR_TRANS_SHIFT(trans))
>> +#define EDP_PSR_ERROR(trans) (0x4 << _EDP_PSR_TRANS_SHIFT(trans))
>> +#define EDP_PSR_POST_EXIT(trans) (0x2 << _EDP_PSR_TRANS_SHIFT(trans))
>> +#define EDP_PSR_PRE_ENTRY(trans) (0x1 << _EDP_PSR_TRANS_SHIFT(trans))
>>
>> #define _SRD_AUX_CTL_A 0x60810
>> #define _SRD_AUX_CTL_EDP 0x6f810
>> --
>> 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