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.so...@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.pandi...@intel.com>
Signed-off-by: José Roberto de Souza <jose.so...@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demar...@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
