* [PATCH 0/5] PSR interrupts
@ 2018-03-20 22:41 Dhinakaran Pandiyan
2018-03-20 22:41 ` [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw Dhinakaran Pandiyan
` (8 more replies)
0 siblings, 9 replies; 25+ messages in thread
From: Dhinakaran Pandiyan @ 2018-03-20 22:41 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan
I have revived the PSR interrupt patches that Daniel and Ville had worked
on [1], added debugfs control for the interrupts and exposed interrupt
timestamps via debugfs. The idea is to provide a mechanism for CI tests to
know whether a PSR exit event succeeded and if PSR re-enters when it is
expected to do so. This series was tested on a SKL laptop.
Daniel Vetter (2):
drm/i915: Enable edp psr error interrupts on hsw
drm/i915: Enable edp psr error interrupts on bdw+
Note: This patch has Daniel's name as the author but has Ville's
Signed-off, I have left it as it is.
Dhinakaran Pandiyan (2):
drm/i915/psr: Control PSR interrupts via debugfs
drm/i915/psr: Timestamps for PSR entry and exit interrupts.
Ville Syrjälä (1):
drm/i915: Drop reg_write from the PSR mask
[1] https://github.com/vsyrjala/linux/tree/psr_interrupts_2
drivers/gpu/drm/i915/i915_debugfs.c | 48 +++++++++++++++++++++++-
drivers/gpu/drm/i915/i915_drv.h | 4 ++
drivers/gpu/drm/i915/i915_irq.c | 61 ++++++++++++++++++++++++++++--
drivers/gpu/drm/i915/i915_reg.h | 9 +++++
drivers/gpu/drm/i915/intel_display.h | 4 ++
drivers/gpu/drm/i915/intel_drv.h | 2 +
drivers/gpu/drm/i915/intel_psr.c | 73 ++++++++++++++++++++++++++++++++++++
7 files changed, 197 insertions(+), 4 deletions(-)
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw
2018-03-20 22:41 [PATCH 0/5] PSR interrupts Dhinakaran Pandiyan
@ 2018-03-20 22:41 ` Dhinakaran Pandiyan
2018-03-21 18:59 ` Ville Syrjälä
2018-03-20 22:41 ` [PATCH 2/5] drm/i915: Drop reg_write from the PSR mask Dhinakaran Pandiyan
` (7 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Dhinakaran Pandiyan @ 2018-03-20 22:41 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Daniel Vetter, Dhinakaran Pandiyan, Rodrigo Vivi
From: Daniel Vetter <daniel.vetter@ffwll.ch>
The definitions for the error register should be valid on bdw/skl too,
but there we haven't even enabled DE_MISC handling yet.
Somewhat confusing the the moved register offset on bdw is only for
the _CTL/_AUX register, and that _IIR/IMR stayed where they have been
on bdw.
v2: Fixes from Ville.
v3: From DK
* Rebased on drm-tip
* Removed BDW IIR bit definition, looks like an unintentional change that
should be in the following patch.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++
drivers/gpu/drm/i915/intel_psr.c | 3 ++-
3 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 44eef355e12c..e94a835b7515 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2392,6 +2392,26 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
ironlake_rps_change_irq_handler(dev_priv);
}
+static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
+{
+ u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
+
+ if (edp_psr_iir & EDP_PSR_ERROR)
+ DRM_DEBUG_KMS("PSR error\n");
+
+ if (edp_psr_iir & EDP_PSR_PRE_ENTRY) {
+ DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n");
+ I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY);
+ }
+
+ if (edp_psr_iir & EDP_PSR_POST_EXIT) {
+ DRM_DEBUG_KMS("PSR exit completed\n");
+ I915_WRITE(EDP_PSR_IMR, 0);
+ }
+
+ I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
+}
+
static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
u32 de_iir)
{
@@ -2404,6 +2424,9 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
if (de_iir & DE_ERR_INT_IVB)
ivb_err_int_handler(dev_priv);
+ if (de_iir & DE_EDP_PSR_INT_HSW)
+ hsw_edp_psr_irq_handler(dev_priv);
+
if (de_iir & DE_AUX_CHANNEL_A_IVB)
dp_aux_irq_handler(dev_priv);
@@ -3252,6 +3275,11 @@ static void ironlake_irq_reset(struct drm_device *dev)
if (IS_GEN7(dev_priv))
I915_WRITE(GEN7_ERR_INT, 0xffffffff);
+ if (IS_HASWELL(dev_priv)) {
+ I915_WRITE(EDP_PSR_IMR, 0xffffffff);
+ I915_WRITE(EDP_PSR_IIR, 0xffffffff);
+ }
+
gen5_gt_irq_reset(dev_priv);
ibx_irq_reset(dev_priv);
@@ -3663,6 +3691,12 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
DE_DP_A_HOTPLUG);
}
+ if (IS_HASWELL(dev_priv)) {
+ gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
+ I915_WRITE(EDP_PSR_IMR, 0);
+ display_mask |= DE_EDP_PSR_INT_HSW;
+ }
+
dev_priv->irq_mask = ~display_mask;
ibx_irq_pre_postinstall(dev);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1e000f3004cb..3447f03eac3d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3828,6 +3828,13 @@ 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 */
+#define EDP_PSR_IMR _MMIO(0x64834)
+#define EDP_PSR_IIR _MMIO(0x64838)
+#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_DATA(i) _MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
@@ -6628,6 +6635,7 @@ enum {
#define DE_PCH_EVENT_IVB (1<<28)
#define DE_DP_A_HOTPLUG_IVB (1<<27)
#define DE_AUX_CHANNEL_A_IVB (1<<26)
+#define DE_EDP_PSR_INT_HSW (1<<19)
#define DE_SPRITEC_FLIP_DONE_IVB (1<<14)
#define DE_PLANEC_FLIP_DONE_IVB (1<<13)
#define DE_PIPEC_VBLANK_IVB (1<<10)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 317cb4a12693..27dfd507a4f7 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -613,7 +613,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
I915_WRITE(EDP_PSR_DEBUG,
EDP_PSR_DEBUG_MASK_MEMUP |
EDP_PSR_DEBUG_MASK_HPD |
- EDP_PSR_DEBUG_MASK_LPSP);
+ EDP_PSR_DEBUG_MASK_LPSP |
+ EDP_PSR_DEBUG_MASK_REG_WRITE);
}
}
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/5] drm/i915: Drop reg_write from the PSR mask
2018-03-20 22:41 [PATCH 0/5] PSR interrupts Dhinakaran Pandiyan
2018-03-20 22:41 ` [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw Dhinakaran Pandiyan
@ 2018-03-20 22:41 ` Dhinakaran Pandiyan
2018-03-20 22:41 ` [PATCH 3/5] drm/i915: Enable edp psr error interrupts on bdw+ Dhinakaran Pandiyan
` (6 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Dhinakaran Pandiyan @ 2018-03-20 22:41 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
v2: from DK
* Rebased on drm-tip
* Explananation in commit message.
REG_WRITE is necessary for HW to exit PSR when plane/pipe registers are
written to.
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/i915/intel_psr.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 27dfd507a4f7..317cb4a12693 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -613,8 +613,7 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
I915_WRITE(EDP_PSR_DEBUG,
EDP_PSR_DEBUG_MASK_MEMUP |
EDP_PSR_DEBUG_MASK_HPD |
- EDP_PSR_DEBUG_MASK_LPSP |
- EDP_PSR_DEBUG_MASK_REG_WRITE);
+ EDP_PSR_DEBUG_MASK_LPSP);
}
}
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 3/5] drm/i915: Enable edp psr error interrupts on bdw+
2018-03-20 22:41 [PATCH 0/5] PSR interrupts Dhinakaran Pandiyan
2018-03-20 22:41 ` [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw Dhinakaran Pandiyan
2018-03-20 22:41 ` [PATCH 2/5] drm/i915: Drop reg_write from the PSR mask Dhinakaran Pandiyan
@ 2018-03-20 22:41 ` Dhinakaran Pandiyan
2018-03-20 22:41 ` [PATCH 4/5] drm/i915/psr: Control PSR interrupts via debugfs Dhinakaran Pandiyan
` (5 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Dhinakaran Pandiyan @ 2018-03-20 22:41 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Dhinakaran Pandiyan, Rodrigo Vivi
From: Daniel Vetter <daniel.vetter@ffwll.ch>
Plug in the bdw+ irq handling for PSR interrupts. bdw+ supports psr on
any transcoder in theory, though the we don't currenty enable PSR except
on the EDP transcoder.
v2: From DK
* Rebased on drm-tip
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/i915/i915_irq.c | 57 ++++++++++++++++++++++++++++--------
drivers/gpu/drm/i915/i915_reg.h | 7 +++--
drivers/gpu/drm/i915/intel_display.h | 4 +++
3 files changed, 52 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e94a835b7515..56228e8ed980 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2395,20 +2395,34 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
{
u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
+ u32 edp_psr_imr = I915_READ(EDP_PSR_IMR);
+ u32 mask = BIT(TRANSCODER_EDP);
+ enum transcoder cpu_transcoder;
- if (edp_psr_iir & EDP_PSR_ERROR)
- DRM_DEBUG_KMS("PSR error\n");
-
- if (edp_psr_iir & EDP_PSR_PRE_ENTRY) {
- DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n");
- I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY);
- }
+ if (INTEL_GEN(dev_priv) >= 8)
+ mask |= BIT(TRANSCODER_A) |
+ BIT(TRANSCODER_B) |
+ BIT(TRANSCODER_C);
+
+ for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, mask) {
+ if (edp_psr_iir & EDP_PSR_ERROR(cpu_transcoder))
+ DRM_DEBUG_KMS("Transcoder %s PSR error\n",
+ transcoder_name(cpu_transcoder));
+
+ if (edp_psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
+ DRM_DEBUG_KMS("Transcoder %s PSR prepare entry in 2 vblanks\n",
+ transcoder_name(cpu_transcoder));
+ edp_psr_imr |= EDP_PSR_PRE_ENTRY(cpu_transcoder);
+ }
- if (edp_psr_iir & EDP_PSR_POST_EXIT) {
- DRM_DEBUG_KMS("PSR exit completed\n");
- I915_WRITE(EDP_PSR_IMR, 0);
+ if (edp_psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
+ DRM_DEBUG_KMS("Transcoder %s PSR exit completed\n",
+ transcoder_name(cpu_transcoder));
+ edp_psr_imr &= ~EDP_PSR_PRE_ENTRY(cpu_transcoder);
+ }
}
+ I915_WRITE(EDP_PSR_IMR, edp_psr_imr);
I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
}
@@ -2556,11 +2570,22 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
if (master_ctl & GEN8_DE_MISC_IRQ) {
iir = I915_READ(GEN8_DE_MISC_IIR);
if (iir) {
+ bool found = false;
+
I915_WRITE(GEN8_DE_MISC_IIR, iir);
ret = IRQ_HANDLED;
- if (iir & GEN8_DE_MISC_GSE)
+
+ if (iir & GEN8_DE_MISC_GSE) {
intel_opregion_asle_intr(dev_priv);
- else
+ found = true;
+ }
+
+ if (iir & GEN8_DE_EDP_PSR) {
+ hsw_edp_psr_irq_handler(dev_priv);
+ found = true;
+ }
+
+ if (!found)
DRM_ERROR("Unexpected DE Misc interrupt\n");
}
else
@@ -3318,6 +3343,9 @@ static void gen8_irq_reset(struct drm_device *dev)
gen8_gt_irq_reset(dev_priv);
+ I915_WRITE(EDP_PSR_IMR, 0xffffffff);
+ I915_WRITE(EDP_PSR_IIR, 0xffffffff);
+
for_each_pipe(dev_priv, pipe)
if (intel_display_power_is_enabled(dev_priv,
POWER_DOMAIN_PIPE(pipe)))
@@ -3807,7 +3835,7 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
uint32_t de_pipe_enables;
u32 de_port_masked = GEN8_AUX_CHANNEL_A;
u32 de_port_enables;
- u32 de_misc_masked = GEN8_DE_MISC_GSE;
+ u32 de_misc_masked = GEN8_DE_MISC_GSE | GEN8_DE_EDP_PSR;
enum pipe pipe;
if (INTEL_GEN(dev_priv) >= 9) {
@@ -3832,6 +3860,9 @@ 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(dev_priv, EDP_PSR_IIR);
+ I915_WRITE(EDP_PSR_IMR, 0);
+
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 3447f03eac3d..578a0ac74bfb 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3831,9 +3831,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 (1<<2)
-#define EDP_PSR_POST_EXIT (1<<1)
-#define EDP_PSR_PRE_ENTRY (1<<0)
+#define EDP_PSR_ERROR(trans) (1 << (((trans) * 8 + 10) & 31))
+#define EDP_PSR_POST_EXIT(trans) (1 << (((trans) * 8 + 9) & 31))
+#define EDP_PSR_PRE_ENTRY(trans) (1 << (((trans) * 8 + 8) & 31))
#define EDP_PSR_AUX_CTL _MMIO(dev_priv->psr_mmio_base + 0x10)
#define EDP_PSR_AUX_DATA(i) _MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
@@ -6760,6 +6760,7 @@ enum {
#define GEN8_DE_MISC_IIR _MMIO(0x44468)
#define GEN8_DE_MISC_IER _MMIO(0x4446c)
#define GEN8_DE_MISC_GSE (1 << 27)
+#define GEN8_DE_EDP_PSR (1 << 19)
#define GEN8_PCU_ISR _MMIO(0x444e0)
#define GEN8_PCU_IMR _MMIO(0x444e4)
diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index 4e7418b345bc..2ef31617614a 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -218,6 +218,10 @@ struct intel_link_m_n {
for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++) \
for_each_if((__mask) & BIT(__p))
+#define for_each_cpu_transcoder_masked(__dev_priv, __t, __mask) \
+ for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \
+ for_each_if ((__mask) & (1 << (__t)))
+
#define for_each_universal_plane(__dev_priv, __pipe, __p) \
for ((__p) = 0; \
(__p) < INTEL_INFO(__dev_priv)->num_sprites[(__pipe)] + 1; \
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 4/5] drm/i915/psr: Control PSR interrupts via debugfs
2018-03-20 22:41 [PATCH 0/5] PSR interrupts Dhinakaran Pandiyan
` (2 preceding siblings ...)
2018-03-20 22:41 ` [PATCH 3/5] drm/i915: Enable edp psr error interrupts on bdw+ Dhinakaran Pandiyan
@ 2018-03-20 22:41 ` Dhinakaran Pandiyan
2018-03-21 19:45 ` Ville Syrjälä
2018-03-20 22:41 ` [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts Dhinakaran Pandiyan
` (4 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Dhinakaran Pandiyan @ 2018-03-20 22:41 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Dhinakaran Pandiyan, Rodrigo Vivi
Interrupts other than the one for AUX errors are required only for debug,
so unmask them via debugfs when the user requests debug.
User can make such a request with
echo 1 > <DEBUG_FS>/dri/0/i915_edp_psr_debug
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 36 +++++++++++++++++++-
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_irq.c | 68 ++++++++++++++++---------------------
drivers/gpu/drm/i915/intel_drv.h | 2 ++
drivers/gpu/drm/i915/intel_psr.c | 56 ++++++++++++++++++++++++++++++
5 files changed, 123 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 964ea1a12357..669f3d56054a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2690,6 +2690,39 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
return 0;
}
+static int
+i915_edp_psr_debug_set(void *data, u64 val)
+{
+ struct drm_i915_private *dev_priv = data;
+
+ if (!CAN_PSR(dev_priv))
+ return -ENODEV;
+
+ if (val < 0 || val > 1)
+ return -EINVAL;
+
+ DRM_DEBUG_KMS("%s PSR debug\n", val == 1 ? "Enabling" : "Disabling");
+ intel_psr_debug_control(dev_priv, val);
+
+ return 0;
+}
+
+static int
+i915_edp_psr_debug_get(void *data, u64 *val)
+{
+ struct drm_i915_private *dev_priv = data;
+
+ if (!CAN_PSR(dev_priv))
+ return -ENODEV;
+
+ *val = READ_ONCE(dev_priv->psr.debug);
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops,
+ i915_edp_psr_debug_get, i915_edp_psr_debug_set,
+ "%llu\n");
+
static int i915_sink_crc(struct seq_file *m, void *data)
{
struct drm_i915_private *dev_priv = node_to_i915(m->private);
@@ -4811,7 +4844,8 @@ static const struct i915_debugfs_files {
{"i915_guc_log_relay", &i915_guc_log_relay_fops},
{"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops},
{"i915_ipc_status", &i915_ipc_status_fops},
- {"i915_drrs_ctl", &i915_drrs_ctl_fops}
+ {"i915_drrs_ctl", &i915_drrs_ctl_fops},
+ {"i915_edp_psr_debug", &i915_edp_psr_debug_fops}
};
int i915_debugfs_register(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e27ba8fb64e6..136fa2267a66 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -608,6 +608,7 @@ struct i915_psr {
bool colorimetry_support;
bool alpm;
bool has_hw_tracking;
+ bool debug;
void (*enable_source)(struct intel_dp *,
const struct intel_crtc_state *);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 56228e8ed980..94941b52f1cf 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2392,40 +2392,6 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
ironlake_rps_change_irq_handler(dev_priv);
}
-static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
-{
- u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
- u32 edp_psr_imr = I915_READ(EDP_PSR_IMR);
- u32 mask = BIT(TRANSCODER_EDP);
- enum transcoder cpu_transcoder;
-
- if (INTEL_GEN(dev_priv) >= 8)
- mask |= BIT(TRANSCODER_A) |
- BIT(TRANSCODER_B) |
- BIT(TRANSCODER_C);
-
- for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, mask) {
- if (edp_psr_iir & EDP_PSR_ERROR(cpu_transcoder))
- DRM_DEBUG_KMS("Transcoder %s PSR error\n",
- transcoder_name(cpu_transcoder));
-
- if (edp_psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
- DRM_DEBUG_KMS("Transcoder %s PSR prepare entry in 2 vblanks\n",
- transcoder_name(cpu_transcoder));
- edp_psr_imr |= EDP_PSR_PRE_ENTRY(cpu_transcoder);
- }
-
- if (edp_psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
- DRM_DEBUG_KMS("Transcoder %s PSR exit completed\n",
- transcoder_name(cpu_transcoder));
- edp_psr_imr &= ~EDP_PSR_PRE_ENTRY(cpu_transcoder);
- }
- }
-
- I915_WRITE(EDP_PSR_IMR, edp_psr_imr);
- I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
-}
-
static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
u32 de_iir)
{
@@ -2438,8 +2404,12 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
if (de_iir & DE_ERR_INT_IVB)
ivb_err_int_handler(dev_priv);
- if (de_iir & DE_EDP_PSR_INT_HSW)
- hsw_edp_psr_irq_handler(dev_priv);
+ if (de_iir & DE_EDP_PSR_INT_HSW) {
+ u32 psr_iir = I915_READ(EDP_PSR_IIR);
+
+ intel_psr_irq_handler(dev_priv, psr_iir);
+ I915_WRITE(EDP_PSR_IIR, psr_iir);
+ }
if (de_iir & DE_AUX_CHANNEL_A_IVB)
dp_aux_irq_handler(dev_priv);
@@ -2581,7 +2551,10 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
}
if (iir & GEN8_DE_EDP_PSR) {
- hsw_edp_psr_irq_handler(dev_priv);
+ u32 psr_iir = I915_READ(EDP_PSR_IIR);
+
+ intel_psr_irq_handler(dev_priv, psr_iir);
+ I915_WRITE(EDP_PSR_IIR, psr_iir);
found = true;
}
@@ -3699,6 +3672,23 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
}
}
+static inline u32 psr_mask(struct drm_i915_private *dev_priv)
+{
+ u32 transcoders = BIT(TRANSCODER_EDP);
+ u32 mask = 0;
+ enum transcoder cpu_transcoder;
+
+ 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)
+ mask |= EDP_PSR_ERROR(cpu_transcoder);
+
+ return ~mask;
+}
+
static int ironlake_irq_postinstall(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -3721,7 +3711,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
if (IS_HASWELL(dev_priv)) {
gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
- I915_WRITE(EDP_PSR_IMR, 0);
+ I915_WRITE(EDP_PSR_IMR, psr_mask(dev_priv));
display_mask |= DE_EDP_PSR_INT_HSW;
}
@@ -3861,7 +3851,7 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
de_port_enables |= GEN8_PORT_DP_A_HOTPLUG;
gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
- I915_WRITE(EDP_PSR_IMR, 0);
+ I915_WRITE(EDP_PSR_IMR, psr_mask(dev_priv));
for_each_pipe(dev_priv, pipe) {
dev_priv->de_irq_mask[pipe] = ~de_pipe_masked;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a215aa78b0be..8be75cc5bf24 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1887,6 +1887,8 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
unsigned frontbuffer_bits);
void intel_psr_compute_config(struct intel_dp *intel_dp,
struct intel_crtc_state *crtc_state);
+void intel_psr_debug_control(struct drm_i915_private *dev_priv, bool enable);
+void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
/* intel_runtime_pm.c */
int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 317cb4a12693..64ecea80438d 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -93,6 +93,62 @@ static void psr_aux_io_power_put(struct intel_dp *intel_dp)
intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
}
+void intel_psr_debug_control(struct drm_i915_private *dev_priv, bool enable)
+{
+ u32 transcoders = BIT(TRANSCODER_EDP);
+ u32 mask = 0;
+ enum transcoder cpu_transcoder;
+
+ /* No PSR interrupts on VLV/CHV */
+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+ return;
+
+ 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)
+ mask = EDP_PSR_POST_EXIT(cpu_transcoder) |
+ EDP_PSR_PRE_ENTRY(cpu_transcoder);
+
+ if (enable) {
+ WRITE_ONCE(dev_priv->psr.debug, true);
+ I915_WRITE(EDP_PSR_IMR, ~mask);
+ } else {
+ I915_WRITE(EDP_PSR_IMR, mask);
+ WRITE_ONCE(dev_priv->psr.debug, false);
+ }
+}
+
+void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
+{
+ u32 transcoders = BIT(TRANSCODER_EDP);
+ enum transcoder cpu_transcoder;
+
+ 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) {
+ /* FIXME: Exit PSR when this happens. */
+ if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
+ DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
+ transcoder_name(cpu_transcoder));
+
+ if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
+ DRM_DEBUG_KMS("[transcoder %s] PSR entry in 2 vblanks\n",
+ transcoder_name(cpu_transcoder));
+ }
+
+ if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
+ DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
+ transcoder_name(cpu_transcoder));
+ }
+ }
+}
+
static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
{
uint8_t psr_caps = 0;
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts.
2018-03-20 22:41 [PATCH 0/5] PSR interrupts Dhinakaran Pandiyan
` (3 preceding siblings ...)
2018-03-20 22:41 ` [PATCH 4/5] drm/i915/psr: Control PSR interrupts via debugfs Dhinakaran Pandiyan
@ 2018-03-20 22:41 ` Dhinakaran Pandiyan
2018-03-21 19:48 ` Ville Syrjälä
2018-03-22 21:08 ` Chris Wilson
2018-03-21 8:02 ` ✗ Fi.CI.CHECKPATCH: warning for PSR interrupts Patchwork
` (3 subsequent siblings)
8 siblings, 2 replies; 25+ messages in thread
From: Dhinakaran Pandiyan @ 2018-03-20 22:41 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Dhinakaran Pandiyan, Rodrigo Vivi
Timestamps are useful for IGT tests that trigger PSR exit and/or wait for
PSR entry.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++++
drivers/gpu/drm/i915/i915_drv.h | 3 +++
drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++++++--
3 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 669f3d56054a..d28dc4d8388e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2686,6 +2686,18 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
}
mutex_unlock(&dev_priv->psr.lock);
+ if (READ_ONCE(dev_priv->psr.debug)) {
+ unsigned int seq;
+
+ do {
+ seq = read_seqbegin(&dev_priv->psr.debug_lock);
+ seq_printf(m, "Last attempted entry at: %lld\n",
+ dev_priv->psr.last_entry_attempt);
+ seq_printf(m, "Last exit at: %lld\n",
+ dev_priv->psr.last_exit);
+ } while (read_seqretry(&dev_priv->psr.debug_lock, seq));
+ }
+
intel_runtime_pm_put(dev_priv);
return 0;
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 136fa2267a66..b8170882e1ab 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -609,6 +609,9 @@ struct i915_psr {
bool alpm;
bool has_hw_tracking;
bool debug;
+ ktime_t last_entry_attempt;
+ ktime_t last_exit;
+ seqlock_t debug_lock;
void (*enable_source)(struct intel_dp *,
const struct intel_crtc_state *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 64ecea80438d..a83d95b1b587 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -125,28 +125,44 @@ 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();
+ unsigned long flags = 0;
if (INTEL_GEN(dev_priv) >= 8)
transcoders |= BIT(TRANSCODER_A) |
BIT(TRANSCODER_B) |
BIT(TRANSCODER_C);
+ write_seqlock_irqsave(&dev_priv->psr.debug_lock, flags);
for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
+ bool handled = false;
+
+ /* PSR supported only on one transcoder currently */
+ WARN_ON_ONCE(handled);
+
/* FIXME: Exit PSR when this happens. */
- if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
+ if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
+ handled = true;
DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
transcoder_name(cpu_transcoder));
+ }
+
if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
- DRM_DEBUG_KMS("[transcoder %s] PSR entry in 2 vblanks\n",
+ handled = true;
+ 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)) {
+ handled = true;
+ dev_priv->psr.last_exit = time_ns;
DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
transcoder_name(cpu_transcoder));
}
}
+ write_sequnlock_irqrestore(&dev_priv->psr.debug_lock, flags);
}
static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
@@ -1160,6 +1176,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
mutex_init(&dev_priv->psr.lock);
+ seqlock_init(&dev_priv->psr.debug_lock);
if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
dev_priv->psr.enable_source = vlv_psr_enable_source;
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 25+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for PSR interrupts
2018-03-20 22:41 [PATCH 0/5] PSR interrupts Dhinakaran Pandiyan
` (4 preceding siblings ...)
2018-03-20 22:41 ` [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts Dhinakaran Pandiyan
@ 2018-03-21 8:02 ` Patchwork
2018-03-21 8:03 ` ✗ Fi.CI.SPARSE: " Patchwork
` (2 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Patchwork @ 2018-03-21 8:02 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: intel-gfx
== Series Details ==
Series: PSR interrupts
URL : https://patchwork.freedesktop.org/series/40332/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
3cde2d1d81fc drm/i915: Enable edp psr error interrupts on hsw
-:106: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#106: FILE: drivers/gpu/drm/i915/i915_reg.h:3843:
+#define EDP_PSR_ERROR (1<<2)
^
-:107: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#107: FILE: drivers/gpu/drm/i915/i915_reg.h:3844:
+#define EDP_PSR_POST_EXIT (1<<1)
^
-:108: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#108: FILE: drivers/gpu/drm/i915/i915_reg.h:3845:
+#define EDP_PSR_PRE_ENTRY (1<<0)
^
-:117: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#117: FILE: drivers/gpu/drm/i915/i915_reg.h:6647:
+#define DE_EDP_PSR_INT_HSW (1<<19)
^
total: 0 errors, 0 warnings, 4 checks, 87 lines checked
cf7b3fb92e87 drm/i915: Drop reg_write from the PSR mask
3bfc83341cb4 drm/i915: Enable edp psr error interrupts on bdw+
-:156: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#156: FILE: drivers/gpu/drm/i915/intel_display.h:221:
+#define for_each_cpu_transcoder_masked(__dev_priv, __t, __mask) \
+ for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \
+ for_each_if ((__mask) & (1 << (__t)))
-:156: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__t' - possible side-effects?
#156: FILE: drivers/gpu/drm/i915/intel_display.h:221:
+#define for_each_cpu_transcoder_masked(__dev_priv, __t, __mask) \
+ for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \
+ for_each_if ((__mask) & (1 << (__t)))
-:157: CHECK:SPACING: No space is necessary after a cast
#157: FILE: drivers/gpu/drm/i915/intel_display.h:222:
+ for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \
-:158: WARNING:SPACING: space prohibited between function name and open parenthesis '('
#158: FILE: drivers/gpu/drm/i915/intel_display.h:223:
+ for_each_if ((__mask) & (1 << (__t)))
total: 1 errors, 1 warnings, 2 checks, 123 lines checked
6dfdcd19a387 drm/i915/psr: Control PSR interrupts via debugfs
d72b667b69d5 drm/i915/psr: Timestamps for PSR entry and exit interrupts.
-:84: CHECK:BRACES: Blank lines aren't necessary before a close brace '}'
#84: FILE: drivers/gpu/drm/i915/intel_psr.c:149:
+ }
total: 0 errors, 0 warnings, 1 checks, 80 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* ✗ Fi.CI.SPARSE: warning for PSR interrupts
2018-03-20 22:41 [PATCH 0/5] PSR interrupts Dhinakaran Pandiyan
` (5 preceding siblings ...)
2018-03-21 8:02 ` ✗ Fi.CI.CHECKPATCH: warning for PSR interrupts Patchwork
@ 2018-03-21 8:03 ` Patchwork
2018-03-21 8:16 ` ✓ Fi.CI.BAT: success " Patchwork
2018-03-21 9:13 ` ✓ Fi.CI.IGT: " Patchwork
8 siblings, 0 replies; 25+ messages in thread
From: Patchwork @ 2018-03-21 8:03 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: intel-gfx
== Series Details ==
Series: PSR interrupts
URL : https://patchwork.freedesktop.org/series/40332/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Commit: drm/i915: Enable edp psr error interrupts on hsw
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: expected void [noderef] <asn:4>**slot
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: expected void **slot
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: expected void **slot
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: expected void **slot
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: got void [noderef] <asn:4>**
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: got void [noderef] <asn:4>**
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: got void [noderef] <asn:4>**
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: got void **slot
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: warning: incorrect type in argument 1 (different address spaces)
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: warning: incorrect type in assignment (different address spaces)
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: warning: incorrect type in assignment (different address spaces)
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: warning: incorrect type in assignment (different address spaces)
-drivers/gpu/drm/i915/gvt/gtt.c:662:45: expected void [noderef] <asn:4>**slot
-drivers/gpu/drm/i915/gvt/gtt.c:662:45: got void **slot
-drivers/gpu/drm/i915/gvt/gtt.c:662:45: warning: incorrect type in argument 1 (different address spaces)
-drivers/gpu/drm/i915/gvt/mmio.c:255:23: warning: memcpy with byte count of 279040
-drivers/gpu/drm/i915/gvt/mmio.c:256:23: warning: memcpy with byte count of 279040
-drivers/gpu/drm/i915/i915_perf.c:1365:15: warning: memset with byte count of 16777216
-drivers/gpu/drm/i915/i915_perf.c:1423:15: warning: memset with byte count of 16777216
+ ^
+ ^
+ ^~~
+ ^~~
+ #define I915_WRITE(reg, val) dev_priv->uncore.funcs.mmio_writel(dev_priv, (reg), (val), true)
+ #define I915_WRITE(reg, val) dev_priv->uncore.funcs.mmio_writel(dev_priv, (reg), (val), true)
+drivers/gpu/drm/i915/i915_drv.h:3537:83: note: in definition of macro ‘I915_WRITE’
+drivers/gpu/drm/i915/i915_drv.h:3537:83: note: in definition of macro ‘I915_WRITE’
+drivers/gpu/drm/i915/intel_psr.c:613:17: error: undefined identifier 'EDP_PSR_DEBUG_MASK_REG_WRITE'
+drivers/gpu/drm/i915/intel_psr.c:613:17: warning: call with no type!
+drivers/gpu/drm/i915/intel_psr.c:617:7: error: ‘EDP_PSR_DEBUG_MASK_REG_WRITE’ undeclared (first use in this function)
+drivers/gpu/drm/i915/intel_psr.c:617:7: note: each undeclared identifier is reported only once for each function it appears in
+drivers/gpu/drm/i915/intel_psr.c: In function ‘hsw_psr_enable_source’:
+ EDP_PSR_DEBUG_MASK_REG_WRITE);
+ EDP_PSR_DEBUG_MASK_REG_WRITE);
+ from drivers/gpu/drm/i915/intel_psr.c:56:
+In file included from drivers/gpu/drm/i915/intel_drv.h:33:0,
+make[1]: *** [drivers/gpu/drm/i915] Error 2
+make[2]: *** [drivers/gpu/drm/i915/intel_psr.o] Error 1
+make[2]: *** Waiting for unfinished jobs....
+make[2]: *** wait: No child processes. Stop.
+make: *** [drivers/gpu/drm/] Error 2
Commit: drm/i915: Drop reg_write from the PSR mask
- ^
- ^
- ^~~
- ^~~
- #define I915_WRITE(reg, val) dev_priv->uncore.funcs.mmio_writel(dev_priv, (reg), (val), true)
- #define I915_WRITE(reg, val) dev_priv->uncore.funcs.mmio_writel(dev_priv, (reg), (val), true)
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: expected void [noderef] <asn:4>**slot
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: expected void **slot
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: expected void **slot
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: expected void **slot
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: got void [noderef] <asn:4>**
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: got void [noderef] <asn:4>**
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: got void [noderef] <asn:4>**
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: got void **slot
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: warning: incorrect type in argument 1 (different address spaces)
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: warning: incorrect type in assignment (different address spaces)
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: warning: incorrect type in assignment (different address spaces)
-drivers/gpu/drm/i915/gvt/gtt.c:661:9: warning: incorrect type in assignment (different address spaces)
-drivers/gpu/drm/i915/gvt/gtt.c:662:45: expected void [noderef] <asn:4>**slot
-drivers/gpu/drm/i915/gvt/gtt.c:662:45: got void **slot
-drivers/gpu/drm/i915/gvt/gtt.c:662:45: warning: incorrect type in argument 1 (different address spaces)
-drivers/gpu/drm/i915/gvt/mmio.c:255:23: warning: memcpy with byte count of 279040
-drivers/gpu/drm/i915/gvt/mmio.c:256:23: warning: memcpy with byte count of 279040
-drivers/gpu/drm/i915/i915_drv.h:3537:83: note: in definition of macro ‘I915_WRITE’
-drivers/gpu/drm/i915/i915_drv.h:3537:83: note: in definition of macro ‘I915_WRITE’
-drivers/gpu/drm/i915/i915_perf.c:1365:15: warning: memset with byte count of 16777216
-drivers/gpu/drm/i915/i915_perf.c:1423:15: warning: memset with byte count of 16777216
-O:drivers/gpu/drm/i915/intel_psr.c:613:17: error: undefined identifier 'EDP_PSR_DEBUG_MASK_REG_WRITE'
-O:drivers/gpu/drm/i915/intel_psr.c:613:17: warning: call with no type!
-O:drivers/gpu/drm/i915/intel_psr.c:617:7: error: ‘EDP_PSR_DEBUG_MASK_REG_WRITE’ undeclared (first use in this function)
-O:drivers/gpu/drm/i915/intel_psr.c:617:7: note: each undeclared identifier is reported only once for each function it appears in
-drivers/gpu/drm/i915/intel_psr.c: In function ‘hsw_psr_enable_source’:
- EDP_PSR_DEBUG_MASK_REG_WRITE);
- EDP_PSR_DEBUG_MASK_REG_WRITE);
-drivers/gpu/drm/i915/intel_psr.c:56:
-In file included from drivers/gpu/drm/i915/intel_drv.h:33:0,
-make[1]: *** [drivers/gpu/drm/i915] Error 2
-make[2]: *** [drivers/gpu/drm/i915/intel_psr.o] Error 1
-make[2]: *** Waiting for unfinished jobs....
-make: *** [drivers/gpu/drm/] Error 2
+
Commit: drm/i915: Enable edp psr error interrupts on bdw+
Okay!
Commit: drm/i915/psr: Control PSR interrupts via debugfs
Okay!
Commit: drm/i915/psr: Timestamps for PSR entry and exit interrupts.
Okay!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* ✓ Fi.CI.BAT: success for PSR interrupts
2018-03-20 22:41 [PATCH 0/5] PSR interrupts Dhinakaran Pandiyan
` (6 preceding siblings ...)
2018-03-21 8:03 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2018-03-21 8:16 ` Patchwork
2018-03-21 9:13 ` ✓ Fi.CI.IGT: " Patchwork
8 siblings, 0 replies; 25+ messages in thread
From: Patchwork @ 2018-03-21 8:16 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: intel-gfx
== Series Details ==
Series: PSR interrupts
URL : https://patchwork.freedesktop.org/series/40332/
State : success
== Summary ==
Series 40332v1 PSR interrupts
https://patchwork.freedesktop.org/api/1.0/series/40332/revisions/1/mbox/
---- Possible new issues:
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-a:
incomplete -> PASS (fi-cfl-s2)
---- Known issues:
Test kms_busy:
Subgroup basic-flip-a:
pass -> DMESG-WARN (fi-skl-6700hq) fdo#101144 +3
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-c:
pass -> INCOMPLETE (fi-bxt-dsi) fdo#103927
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
fi-bdw-5557u total:285 pass:264 dwarn:0 dfail:0 fail:0 skip:21 time:430s
fi-bdw-gvtdvm total:285 pass:261 dwarn:0 dfail:0 fail:0 skip:24 time:437s
fi-blb-e6850 total:285 pass:220 dwarn:1 dfail:0 fail:0 skip:64 time:384s
fi-bsw-n3050 total:285 pass:239 dwarn:0 dfail:0 fail:0 skip:46 time:536s
fi-bwr-2160 total:285 pass:180 dwarn:0 dfail:0 fail:0 skip:105 time:297s
fi-bxt-dsi total:243 pass:216 dwarn:0 dfail:0 fail:0 skip:26
fi-bxt-j4205 total:285 pass:256 dwarn:0 dfail:0 fail:0 skip:29 time:511s
fi-byt-j1900 total:285 pass:250 dwarn:0 dfail:0 fail:0 skip:35 time:517s
fi-byt-n2820 total:285 pass:246 dwarn:0 dfail:0 fail:0 skip:39 time:503s
fi-cfl-8700k total:285 pass:257 dwarn:0 dfail:0 fail:0 skip:28 time:408s
fi-cfl-s2 total:285 pass:259 dwarn:0 dfail:0 fail:0 skip:26 time:581s
fi-cfl-u total:285 pass:259 dwarn:0 dfail:0 fail:0 skip:26 time:512s
fi-cnl-drrs total:285 pass:254 dwarn:3 dfail:0 fail:0 skip:28 time:528s
fi-elk-e7500 total:285 pass:225 dwarn:1 dfail:0 fail:0 skip:59 time:426s
fi-gdg-551 total:285 pass:177 dwarn:0 dfail:0 fail:0 skip:108 time:314s
fi-glk-1 total:285 pass:257 dwarn:0 dfail:0 fail:0 skip:28 time:537s
fi-hsw-4770 total:285 pass:258 dwarn:0 dfail:0 fail:0 skip:27 time:400s
fi-ilk-650 total:285 pass:225 dwarn:0 dfail:0 fail:0 skip:60 time:424s
fi-ivb-3520m total:285 pass:256 dwarn:0 dfail:0 fail:0 skip:29 time:472s
fi-ivb-3770 total:285 pass:252 dwarn:0 dfail:0 fail:0 skip:33 time:430s
fi-kbl-7500u total:285 pass:260 dwarn:1 dfail:0 fail:0 skip:24 time:479s
fi-kbl-7567u total:285 pass:265 dwarn:0 dfail:0 fail:0 skip:20 time:465s
fi-kbl-r total:285 pass:258 dwarn:0 dfail:0 fail:0 skip:27 time:513s
fi-pnv-d510 total:285 pass:219 dwarn:1 dfail:0 fail:0 skip:65 time:653s
fi-skl-6260u total:285 pass:265 dwarn:0 dfail:0 fail:0 skip:20 time:442s
fi-skl-6600u total:285 pass:258 dwarn:0 dfail:0 fail:0 skip:27 time:537s
fi-skl-6700hq total:285 pass:255 dwarn:4 dfail:0 fail:0 skip:26 time:554s
fi-skl-6700k2 total:285 pass:261 dwarn:0 dfail:0 fail:0 skip:24 time:499s
fi-skl-6770hq total:285 pass:265 dwarn:0 dfail:0 fail:0 skip:20 time:504s
fi-skl-guc total:285 pass:257 dwarn:0 dfail:0 fail:0 skip:28 time:429s
fi-snb-2520m total:285 pass:245 dwarn:0 dfail:0 fail:0 skip:40 time:569s
fi-snb-2600 total:285 pass:245 dwarn:0 dfail:0 fail:0 skip:40 time:401s
9d737cebc219c821989021a3115424165ff7b052 drm-tip: 2018y-03m-20d-14h-56m-05s UTC integration manifest
d72b667b69d5 drm/i915/psr: Timestamps for PSR entry and exit interrupts.
6dfdcd19a387 drm/i915/psr: Control PSR interrupts via debugfs
3bfc83341cb4 drm/i915: Enable edp psr error interrupts on bdw+
cf7b3fb92e87 drm/i915: Drop reg_write from the PSR mask
3cde2d1d81fc drm/i915: Enable edp psr error interrupts on hsw
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8424/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* ✓ Fi.CI.IGT: success for PSR interrupts
2018-03-20 22:41 [PATCH 0/5] PSR interrupts Dhinakaran Pandiyan
` (7 preceding siblings ...)
2018-03-21 8:16 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-03-21 9:13 ` Patchwork
8 siblings, 0 replies; 25+ messages in thread
From: Patchwork @ 2018-03-21 9:13 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: intel-gfx
== Series Details ==
Series: PSR interrupts
URL : https://patchwork.freedesktop.org/series/40332/
State : success
== Summary ==
---- Known issues:
Test kms_flip:
Subgroup 2x-flip-vs-expired-vblank:
pass -> FAIL (shard-hsw) fdo#102887
Subgroup 2x-plain-flip-ts-check-interruptible:
pass -> FAIL (shard-hsw) fdo#100368
Test kms_vblank:
Subgroup pipe-b-ts-continuation-dpms-suspend:
incomplete -> PASS (shard-hsw) fdo#105054
Subgroup pipe-b-ts-continuation-suspend:
pass -> SKIP (shard-snb) fdo#105411
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#105054 https://bugs.freedesktop.org/show_bug.cgi?id=105054
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
shard-hsw total:3478 pass:1766 dwarn:1 dfail:0 fail:3 skip:1707 time:11761s
shard-snb total:3478 pass:1357 dwarn:1 dfail:0 fail:2 skip:2118 time:7235s
Blacklisted hosts:
shard-kbl total:3478 pass:1911 dwarn:26 dfail:0 fail:10 skip:1531 time:9917s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8424/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw
2018-03-20 22:41 ` [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw Dhinakaran Pandiyan
@ 2018-03-21 18:59 ` Ville Syrjälä
2018-03-21 19:19 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 25+ messages in thread
From: Ville Syrjälä @ 2018-03-21 18:59 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: Daniel Vetter, Daniel Vetter, intel-gfx, Rodrigo Vivi
On Tue, Mar 20, 2018 at 03:41:47PM -0700, Dhinakaran Pandiyan wrote:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> The definitions for the error register should be valid on bdw/skl too,
> but there we haven't even enabled DE_MISC handling yet.
>
> Somewhat confusing the the moved register offset on bdw is only for
> the _CTL/_AUX register, and that _IIR/IMR stayed where they have been
> on bdw.
>
> v2: Fixes from Ville.
>
> v3: From DK
> * Rebased on drm-tip
> * Removed BDW IIR bit definition, looks like an unintentional change that
> should be in the following patch.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++
> drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> 3 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 44eef355e12c..e94a835b7515 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2392,6 +2392,26 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
> ironlake_rps_change_irq_handler(dev_priv);
> }
>
> +static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
> +{
> + u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
> +
> + if (edp_psr_iir & EDP_PSR_ERROR)
> + DRM_DEBUG_KMS("PSR error\n");
> +
> + if (edp_psr_iir & EDP_PSR_PRE_ENTRY) {
> + DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n");
I doubt we want to have the entry/exit interrupts generate all this
noise in dmesg all the time. We should probably only enable the
interrupts for testing. The error interrupt I suppose we want always,
might even want DRM_ERROR on it.
> + I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY);
> + }
> +
> + if (edp_psr_iir & EDP_PSR_POST_EXIT) {
> + DRM_DEBUG_KMS("PSR exit completed\n");
> + I915_WRITE(EDP_PSR_IMR, 0);
> + }
> +
> + I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
> +}
> +
> static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> u32 de_iir)
> {
> @@ -2404,6 +2424,9 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> if (de_iir & DE_ERR_INT_IVB)
> ivb_err_int_handler(dev_priv);
>
> + if (de_iir & DE_EDP_PSR_INT_HSW)
> + hsw_edp_psr_irq_handler(dev_priv);
> +
> if (de_iir & DE_AUX_CHANNEL_A_IVB)
> dp_aux_irq_handler(dev_priv);
>
> @@ -3252,6 +3275,11 @@ static void ironlake_irq_reset(struct drm_device *dev)
> if (IS_GEN7(dev_priv))
> I915_WRITE(GEN7_ERR_INT, 0xffffffff);
>
> + if (IS_HASWELL(dev_priv)) {
> + I915_WRITE(EDP_PSR_IMR, 0xffffffff);
> + I915_WRITE(EDP_PSR_IIR, 0xffffffff);
> + }
> +
> gen5_gt_irq_reset(dev_priv);
>
> ibx_irq_reset(dev_priv);
> @@ -3663,6 +3691,12 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> DE_DP_A_HOTPLUG);
> }
>
> + if (IS_HASWELL(dev_priv)) {
> + gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> + I915_WRITE(EDP_PSR_IMR, 0);
> + display_mask |= DE_EDP_PSR_INT_HSW;
> + }
> +
> dev_priv->irq_mask = ~display_mask;
>
> ibx_irq_pre_postinstall(dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1e000f3004cb..3447f03eac3d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3828,6 +3828,13 @@ 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 */
> +#define EDP_PSR_IMR _MMIO(0x64834)
> +#define EDP_PSR_IIR _MMIO(0x64838)
> +#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_DATA(i) _MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
>
> @@ -6628,6 +6635,7 @@ enum {
> #define DE_PCH_EVENT_IVB (1<<28)
> #define DE_DP_A_HOTPLUG_IVB (1<<27)
> #define DE_AUX_CHANNEL_A_IVB (1<<26)
> +#define DE_EDP_PSR_INT_HSW (1<<19)
> #define DE_SPRITEC_FLIP_DONE_IVB (1<<14)
> #define DE_PLANEC_FLIP_DONE_IVB (1<<13)
> #define DE_PIPEC_VBLANK_IVB (1<<10)
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 317cb4a12693..27dfd507a4f7 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -613,7 +613,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> I915_WRITE(EDP_PSR_DEBUG,
> EDP_PSR_DEBUG_MASK_MEMUP |
> EDP_PSR_DEBUG_MASK_HPD |
> - EDP_PSR_DEBUG_MASK_LPSP);
> + EDP_PSR_DEBUG_MASK_LPSP |
> + EDP_PSR_DEBUG_MASK_REG_WRITE);
I don't think Daniel's original had this actually. I think I added it
because I didn't want mmio to mess up my testing. In general I'd really
prefer to mask everything so that we can be sure that the PSR exits
happen when we want/need them and not randomly due to other activity.
If any random mmio can cause a PSR exit it's going to hard to prove that
we're doing the correct thing. Also seems pretty wasteful if eg. GT
interrupts can cause PSR exits for no good reason. I never tested to see
which registers cause the PSR exit. That would be good to know.
> }
> }
>
> --
> 2.14.1
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw
2018-03-21 18:59 ` Ville Syrjälä
@ 2018-03-21 19:19 ` Pandiyan, Dhinakaran
2018-03-21 19:29 ` Ville Syrjälä
0 siblings, 1 reply; 25+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-21 19:19 UTC (permalink / raw)
To: ville.syrjala; +Cc: Vetter, Daniel, daniel.vetter, intel-gfx, Vivi, Rodrigo
On Wed, 2018-03-21 at 20:59 +0200, Ville Syrjälä wrote:
> On Tue, Mar 20, 2018 at 03:41:47PM -0700, Dhinakaran Pandiyan wrote:
> > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > The definitions for the error register should be valid on bdw/skl too,
> > but there we haven't even enabled DE_MISC handling yet.
> >
> > Somewhat confusing the the moved register offset on bdw is only for
> > the _CTL/_AUX register, and that _IIR/IMR stayed where they have been
> > on bdw.
> >
> > v2: Fixes from Ville.
> >
> > v3: From DK
> > * Rebased on drm-tip
> > * Removed BDW IIR bit definition, looks like an unintentional change that
> > should be in the following patch.
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++
> > drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> > 3 files changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 44eef355e12c..e94a835b7515 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2392,6 +2392,26 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
> > ironlake_rps_change_irq_handler(dev_priv);
> > }
> >
> > +static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
> > +{
> > + u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
> > +
> > + if (edp_psr_iir & EDP_PSR_ERROR)
> > + DRM_DEBUG_KMS("PSR error\n");
> > +
> > + if (edp_psr_iir & EDP_PSR_PRE_ENTRY) {
> > + DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n");
>
> I doubt we want to have the entry/exit interrupts generate all this
> noise in dmesg all the time. We should probably only enable the
> interrupts for testing. The error interrupt I suppose we want always,
> might even want DRM_ERROR on it.
I implement debugfs control in Patch 4/5, agreed on DRM_ERROR.
>
> > + I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY);
> > + }
> > +
> > + if (edp_psr_iir & EDP_PSR_POST_EXIT) {
> > + DRM_DEBUG_KMS("PSR exit completed\n");
> > + I915_WRITE(EDP_PSR_IMR, 0);
> > + }
> > +
> > + I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
> > +}
> > +
> > static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > u32 de_iir)
> > {
> > @@ -2404,6 +2424,9 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > if (de_iir & DE_ERR_INT_IVB)
> > ivb_err_int_handler(dev_priv);
> >
> > + if (de_iir & DE_EDP_PSR_INT_HSW)
> > + hsw_edp_psr_irq_handler(dev_priv);
> > +
> > if (de_iir & DE_AUX_CHANNEL_A_IVB)
> > dp_aux_irq_handler(dev_priv);
> >
> > @@ -3252,6 +3275,11 @@ static void ironlake_irq_reset(struct drm_device *dev)
> > if (IS_GEN7(dev_priv))
> > I915_WRITE(GEN7_ERR_INT, 0xffffffff);
> >
> > + if (IS_HASWELL(dev_priv)) {
> > + I915_WRITE(EDP_PSR_IMR, 0xffffffff);
> > + I915_WRITE(EDP_PSR_IIR, 0xffffffff);
> > + }
> > +
> > gen5_gt_irq_reset(dev_priv);
> >
> > ibx_irq_reset(dev_priv);
> > @@ -3663,6 +3691,12 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> > DE_DP_A_HOTPLUG);
> > }
> >
> > + if (IS_HASWELL(dev_priv)) {
> > + gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> > + I915_WRITE(EDP_PSR_IMR, 0);
> > + display_mask |= DE_EDP_PSR_INT_HSW;
> > + }
> > +
> > dev_priv->irq_mask = ~display_mask;
> >
> > ibx_irq_pre_postinstall(dev);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 1e000f3004cb..3447f03eac3d 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3828,6 +3828,13 @@ 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 */
> > +#define EDP_PSR_IMR _MMIO(0x64834)
> > +#define EDP_PSR_IIR _MMIO(0x64838)
> > +#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_DATA(i) _MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
> >
> > @@ -6628,6 +6635,7 @@ enum {
> > #define DE_PCH_EVENT_IVB (1<<28)
> > #define DE_DP_A_HOTPLUG_IVB (1<<27)
> > #define DE_AUX_CHANNEL_A_IVB (1<<26)
> > +#define DE_EDP_PSR_INT_HSW (1<<19)
> > #define DE_SPRITEC_FLIP_DONE_IVB (1<<14)
> > #define DE_PLANEC_FLIP_DONE_IVB (1<<13)
> > #define DE_PIPEC_VBLANK_IVB (1<<10)
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 317cb4a12693..27dfd507a4f7 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -613,7 +613,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > I915_WRITE(EDP_PSR_DEBUG,
> > EDP_PSR_DEBUG_MASK_MEMUP |
> > EDP_PSR_DEBUG_MASK_HPD |
> > - EDP_PSR_DEBUG_MASK_LPSP);
> > + EDP_PSR_DEBUG_MASK_LPSP |
> > + EDP_PSR_DEBUG_MASK_REG_WRITE);
>
> I don't think Daniel's original had this actually. I think I added it
> because I didn't want mmio to mess up my testing.
It gets removed in a separate patch (2/5). So I assumed Daniel added it
and you removed it.
> In general I'd really
> prefer to mask everything so that we can be sure that the PSR exits
> happen when we want/need them and not randomly due to other activity.
>
> If any random mmio can cause a PSR exit it's going to hard to prove that
> we're doing the correct thing. Also seems pretty wasteful if eg. GT
> interrupts can cause PSR exits for no good reason. I never tested to see
> which registers cause the PSR exit. That would be good to know.
>
The same bit on SKL+ is called "Mask Display Reg Write", so it either
means
1) HSW and BDW exit only on display register writes and the bspec
description is ambiguous or
2) exiting specifically on display MMIO writes was added in SKL.
I'll see if I can get a HSW/BDW with PSR panel to test, but assuming the
former is safer.
> > }
> > }
> >
> > --
> > 2.14.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw
2018-03-21 19:19 ` Pandiyan, Dhinakaran
@ 2018-03-21 19:29 ` Ville Syrjälä
2018-03-22 1:19 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 25+ messages in thread
From: Ville Syrjälä @ 2018-03-21 19:29 UTC (permalink / raw)
To: Pandiyan, Dhinakaran
Cc: Vetter, Daniel, daniel.vetter, intel-gfx, Vivi, Rodrigo
On Wed, Mar 21, 2018 at 07:19:53PM +0000, Pandiyan, Dhinakaran wrote:
>
>
>
> On Wed, 2018-03-21 at 20:59 +0200, Ville Syrjälä wrote:
> > On Tue, Mar 20, 2018 at 03:41:47PM -0700, Dhinakaran Pandiyan wrote:
> > > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >
> > > The definitions for the error register should be valid on bdw/skl too,
> > > but there we haven't even enabled DE_MISC handling yet.
> > >
> > > Somewhat confusing the the moved register offset on bdw is only for
> > > the _CTL/_AUX register, and that _IIR/IMR stayed where they have been
> > > on bdw.
> > >
> > > v2: Fixes from Ville.
> > >
> > > v3: From DK
> > > * Rebased on drm-tip
> > > * Removed BDW IIR bit definition, looks like an unintentional change that
> > > should be in the following patch.
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++
> > > drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> > > 3 files changed, 44 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 44eef355e12c..e94a835b7515 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -2392,6 +2392,26 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
> > > ironlake_rps_change_irq_handler(dev_priv);
> > > }
> > >
> > > +static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
> > > +{
> > > + u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
> > > +
> > > + if (edp_psr_iir & EDP_PSR_ERROR)
> > > + DRM_DEBUG_KMS("PSR error\n");
> > > +
> > > + if (edp_psr_iir & EDP_PSR_PRE_ENTRY) {
> > > + DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n");
> >
> > I doubt we want to have the entry/exit interrupts generate all this
> > noise in dmesg all the time. We should probably only enable the
> > interrupts for testing. The error interrupt I suppose we want always,
> > might even want DRM_ERROR on it.
>
> I implement debugfs control in Patch 4/5, agreed on DRM_ERROR.
Right. It's a bit hard to read this with stuff getting
added/remove/moved more or less randomly.
>
> >
> > > + I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY);
> > > + }
> > > +
> > > + if (edp_psr_iir & EDP_PSR_POST_EXIT) {
> > > + DRM_DEBUG_KMS("PSR exit completed\n");
> > > + I915_WRITE(EDP_PSR_IMR, 0);
> > > + }
> > > +
> > > + I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
> > > +}
> > > +
> > > static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > > u32 de_iir)
> > > {
> > > @@ -2404,6 +2424,9 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > > if (de_iir & DE_ERR_INT_IVB)
> > > ivb_err_int_handler(dev_priv);
> > >
> > > + if (de_iir & DE_EDP_PSR_INT_HSW)
> > > + hsw_edp_psr_irq_handler(dev_priv);
> > > +
> > > if (de_iir & DE_AUX_CHANNEL_A_IVB)
> > > dp_aux_irq_handler(dev_priv);
> > >
> > > @@ -3252,6 +3275,11 @@ static void ironlake_irq_reset(struct drm_device *dev)
> > > if (IS_GEN7(dev_priv))
> > > I915_WRITE(GEN7_ERR_INT, 0xffffffff);
> > >
> > > + if (IS_HASWELL(dev_priv)) {
> > > + I915_WRITE(EDP_PSR_IMR, 0xffffffff);
> > > + I915_WRITE(EDP_PSR_IIR, 0xffffffff);
> > > + }
> > > +
> > > gen5_gt_irq_reset(dev_priv);
> > >
> > > ibx_irq_reset(dev_priv);
> > > @@ -3663,6 +3691,12 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> > > DE_DP_A_HOTPLUG);
> > > }
> > >
> > > + if (IS_HASWELL(dev_priv)) {
> > > + gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> > > + I915_WRITE(EDP_PSR_IMR, 0);
> > > + display_mask |= DE_EDP_PSR_INT_HSW;
> > > + }
> > > +
> > > dev_priv->irq_mask = ~display_mask;
> > >
> > > ibx_irq_pre_postinstall(dev);
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 1e000f3004cb..3447f03eac3d 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -3828,6 +3828,13 @@ 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 */
> > > +#define EDP_PSR_IMR _MMIO(0x64834)
> > > +#define EDP_PSR_IIR _MMIO(0x64838)
> > > +#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_DATA(i) _MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
> > >
> > > @@ -6628,6 +6635,7 @@ enum {
> > > #define DE_PCH_EVENT_IVB (1<<28)
> > > #define DE_DP_A_HOTPLUG_IVB (1<<27)
> > > #define DE_AUX_CHANNEL_A_IVB (1<<26)
> > > +#define DE_EDP_PSR_INT_HSW (1<<19)
> > > #define DE_SPRITEC_FLIP_DONE_IVB (1<<14)
> > > #define DE_PLANEC_FLIP_DONE_IVB (1<<13)
> > > #define DE_PIPEC_VBLANK_IVB (1<<10)
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > index 317cb4a12693..27dfd507a4f7 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -613,7 +613,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > I915_WRITE(EDP_PSR_DEBUG,
> > > EDP_PSR_DEBUG_MASK_MEMUP |
> > > EDP_PSR_DEBUG_MASK_HPD |
> > > - EDP_PSR_DEBUG_MASK_LPSP);
> > > + EDP_PSR_DEBUG_MASK_LPSP |
> > > + EDP_PSR_DEBUG_MASK_REG_WRITE);
> >
> > I don't think Daniel's original had this actually. I think I added it
> > because I didn't want mmio to mess up my testing.
>
> It gets removed in a separate patch (2/5). So I assumed Daniel added it
> and you removed it.
Hmm. That could indeed be the case.
>
>
> > In general I'd really
> > prefer to mask everything so that we can be sure that the PSR exits
> > happen when we want/need them and not randomly due to other activity.
> >
> > If any random mmio can cause a PSR exit it's going to hard to prove that
> > we're doing the correct thing. Also seems pretty wasteful if eg. GT
> > interrupts can cause PSR exits for no good reason. I never tested to see
> > which registers cause the PSR exit. That would be good to know.
> >
>
> The same bit on SKL+ is called "Mask Display Reg Write", so it either
> means
> 1) HSW and BDW exit only on display register writes and the bspec
> description is ambiguous or
The problem is that eg. some of the interrupt registers live in the
display block. Hence any interrupt could potentially cause the PSR exit.
As could any register read we do for debug purpose etc. So at least I
would feel safer if PSR can be made to work reliably with more or less
everything masked.
> 2) exiting specifically on display MMIO writes was added in SKL.
>
> I'll see if I can get a HSW/BDW with PSR panel to test, but assuming the
> former is safer.
>
>
>
> > > }
> > > }
> > >
> > > --
> > > 2.14.1
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/5] drm/i915/psr: Control PSR interrupts via debugfs
2018-03-20 22:41 ` [PATCH 4/5] drm/i915/psr: Control PSR interrupts via debugfs Dhinakaran Pandiyan
@ 2018-03-21 19:45 ` Ville Syrjälä
2018-03-22 0:59 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 25+ messages in thread
From: Ville Syrjälä @ 2018-03-21 19:45 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: Daniel Vetter, intel-gfx, Rodrigo Vivi
On Tue, Mar 20, 2018 at 03:41:50PM -0700, Dhinakaran Pandiyan wrote:
> Interrupts other than the one for AUX errors are required only for debug,
> so unmask them via debugfs when the user requests debug.
>
> User can make such a request with
> echo 1 > <DEBUG_FS>/dri/0/i915_edp_psr_debug
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 36 +++++++++++++++++++-
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_irq.c | 68 ++++++++++++++++---------------------
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_psr.c | 56 ++++++++++++++++++++++++++++++
> 5 files changed, 123 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 964ea1a12357..669f3d56054a 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2690,6 +2690,39 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> return 0;
> }
>
> +static int
> +i915_edp_psr_debug_set(void *data, u64 val)
> +{
> + struct drm_i915_private *dev_priv = data;
> +
> + if (!CAN_PSR(dev_priv))
> + return -ENODEV;
> +
> + if (val < 0 || val > 1)
> + return -EINVAL;
> +
> + DRM_DEBUG_KMS("%s PSR debug\n", val == 1 ? "Enabling" : "Disabling");
> + intel_psr_debug_control(dev_priv, val);
> +
> + return 0;
> +}
> +
> +static int
> +i915_edp_psr_debug_get(void *data, u64 *val)
> +{
> + struct drm_i915_private *dev_priv = data;
> +
> + if (!CAN_PSR(dev_priv))
> + return -ENODEV;
> +
> + *val = READ_ONCE(dev_priv->psr.debug);
> + return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops,
> + i915_edp_psr_debug_get, i915_edp_psr_debug_set,
> + "%llu\n");
> +
> static int i915_sink_crc(struct seq_file *m, void *data)
> {
> struct drm_i915_private *dev_priv = node_to_i915(m->private);
> @@ -4811,7 +4844,8 @@ static const struct i915_debugfs_files {
> {"i915_guc_log_relay", &i915_guc_log_relay_fops},
> {"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops},
> {"i915_ipc_status", &i915_ipc_status_fops},
> - {"i915_drrs_ctl", &i915_drrs_ctl_fops}
> + {"i915_drrs_ctl", &i915_drrs_ctl_fops},
> + {"i915_edp_psr_debug", &i915_edp_psr_debug_fops}
> };
>
> int i915_debugfs_register(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e27ba8fb64e6..136fa2267a66 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -608,6 +608,7 @@ struct i915_psr {
> bool colorimetry_support;
> bool alpm;
> bool has_hw_tracking;
> + bool debug;
>
> void (*enable_source)(struct intel_dp *,
> const struct intel_crtc_state *);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 56228e8ed980..94941b52f1cf 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2392,40 +2392,6 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
> ironlake_rps_change_irq_handler(dev_priv);
> }
>
> -static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
> -{
> - u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
> - u32 edp_psr_imr = I915_READ(EDP_PSR_IMR);
> - u32 mask = BIT(TRANSCODER_EDP);
> - enum transcoder cpu_transcoder;
> -
> - if (INTEL_GEN(dev_priv) >= 8)
> - mask |= BIT(TRANSCODER_A) |
> - BIT(TRANSCODER_B) |
> - BIT(TRANSCODER_C);
> -
> - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, mask) {
> - if (edp_psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> - DRM_DEBUG_KMS("Transcoder %s PSR error\n",
> - transcoder_name(cpu_transcoder));
> -
> - if (edp_psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> - DRM_DEBUG_KMS("Transcoder %s PSR prepare entry in 2 vblanks\n",
> - transcoder_name(cpu_transcoder));
> - edp_psr_imr |= EDP_PSR_PRE_ENTRY(cpu_transcoder);
> - }
> -
> - if (edp_psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> - DRM_DEBUG_KMS("Transcoder %s PSR exit completed\n",
> - transcoder_name(cpu_transcoder));
> - edp_psr_imr &= ~EDP_PSR_PRE_ENTRY(cpu_transcoder);
> - }
> - }
> -
> - I915_WRITE(EDP_PSR_IMR, edp_psr_imr);
> - I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
> -}
> -
> static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> u32 de_iir)
> {
> @@ -2438,8 +2404,12 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> if (de_iir & DE_ERR_INT_IVB)
> ivb_err_int_handler(dev_priv);
>
> - if (de_iir & DE_EDP_PSR_INT_HSW)
> - hsw_edp_psr_irq_handler(dev_priv);
> + if (de_iir & DE_EDP_PSR_INT_HSW) {
> + u32 psr_iir = I915_READ(EDP_PSR_IIR);
> +
> + intel_psr_irq_handler(dev_priv, psr_iir);
> + I915_WRITE(EDP_PSR_IIR, psr_iir);
> + }
>
> if (de_iir & DE_AUX_CHANNEL_A_IVB)
> dp_aux_irq_handler(dev_priv);
> @@ -2581,7 +2551,10 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> }
>
> if (iir & GEN8_DE_EDP_PSR) {
> - hsw_edp_psr_irq_handler(dev_priv);
> + u32 psr_iir = I915_READ(EDP_PSR_IIR);
> +
> + intel_psr_irq_handler(dev_priv, psr_iir);
> + I915_WRITE(EDP_PSR_IIR, psr_iir);
> found = true;
> }
>
> @@ -3699,6 +3672,23 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
> }
> }
>
> +static inline u32 psr_mask(struct drm_i915_private *dev_priv)
> +{
> + u32 transcoders = BIT(TRANSCODER_EDP);
> + u32 mask = 0;
> + enum transcoder cpu_transcoder;
> +
> + 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)
> + mask |= EDP_PSR_ERROR(cpu_transcoder);
> +
> + return ~mask;
> +}
> +
> static int ironlake_irq_postinstall(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -3721,7 +3711,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>
> if (IS_HASWELL(dev_priv)) {
> gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> - I915_WRITE(EDP_PSR_IMR, 0);
> + I915_WRITE(EDP_PSR_IMR, psr_mask(dev_priv));
> display_mask |= DE_EDP_PSR_INT_HSW;
> }
>
> @@ -3861,7 +3851,7 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> de_port_enables |= GEN8_PORT_DP_A_HOTPLUG;
>
> gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> - I915_WRITE(EDP_PSR_IMR, 0);
> + I915_WRITE(EDP_PSR_IMR, psr_mask(dev_priv));
psr_mask() seems somewhat pointless to me. Could perhaps just
write the full mask by hand in these cases.
>
> for_each_pipe(dev_priv, pipe) {
> dev_priv->de_irq_mask[pipe] = ~de_pipe_masked;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a215aa78b0be..8be75cc5bf24 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1887,6 +1887,8 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
> unsigned frontbuffer_bits);
> void intel_psr_compute_config(struct intel_dp *intel_dp,
> struct intel_crtc_state *crtc_state);
> +void intel_psr_debug_control(struct drm_i915_private *dev_priv, bool enable);
> +void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
>
> /* intel_runtime_pm.c */
> int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 317cb4a12693..64ecea80438d 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -93,6 +93,62 @@ static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> }
>
> +void intel_psr_debug_control(struct drm_i915_private *dev_priv, bool enable)
> +{
> + u32 transcoders = BIT(TRANSCODER_EDP);
> + u32 mask = 0;
> + enum transcoder cpu_transcoder;
> +
> + /* No PSR interrupts on VLV/CHV */
> + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> + return;
> +
> + 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)
> + mask = EDP_PSR_POST_EXIT(cpu_transcoder) |
> + EDP_PSR_PRE_ENTRY(cpu_transcoder);
This too could be done w/o the loop I think.
> +
> + if (enable) {
> + WRITE_ONCE(dev_priv->psr.debug, true);
> + I915_WRITE(EDP_PSR_IMR, ~mask);
> + } else {
> + I915_WRITE(EDP_PSR_IMR, mask);
> + WRITE_ONCE(dev_priv->psr.debug, false);
> + }
> +}
> +
> +void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
> +{
> + u32 transcoders = BIT(TRANSCODER_EDP);
> + enum transcoder cpu_transcoder;
> +
> + 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) {
> + /* FIXME: Exit PSR when this happens. */
> + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> + DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
> + transcoder_name(cpu_transcoder));
> +
> + if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> + DRM_DEBUG_KMS("[transcoder %s] PSR entry in 2 vblanks\n",
> + transcoder_name(cpu_transcoder));
> + }
> +
> + if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> + DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> + transcoder_name(cpu_transcoder));
> + }
> + }
> +}
> +
> static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> {
> uint8_t psr_caps = 0;
> --
> 2.14.1
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts.
2018-03-20 22:41 ` [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts Dhinakaran Pandiyan
@ 2018-03-21 19:48 ` Ville Syrjälä
2018-03-22 1:05 ` Pandiyan, Dhinakaran
2018-03-22 21:08 ` Chris Wilson
1 sibling, 1 reply; 25+ messages in thread
From: Ville Syrjälä @ 2018-03-21 19:48 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: Daniel Vetter, intel-gfx, Rodrigo Vivi
On Tue, Mar 20, 2018 at 03:41:51PM -0700, Dhinakaran Pandiyan wrote:
> Timestamps are useful for IGT tests that trigger PSR exit and/or wait for
> PSR entry.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++++
> drivers/gpu/drm/i915/i915_drv.h | 3 +++
> drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++++++--
> 3 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 669f3d56054a..d28dc4d8388e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2686,6 +2686,18 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> }
> mutex_unlock(&dev_priv->psr.lock);
>
> + if (READ_ONCE(dev_priv->psr.debug)) {
> + unsigned int seq;
> +
> + do {
> + seq = read_seqbegin(&dev_priv->psr.debug_lock);
> + seq_printf(m, "Last attempted entry at: %lld\n",
> + dev_priv->psr.last_entry_attempt);
> + seq_printf(m, "Last exit at: %lld\n",
> + dev_priv->psr.last_exit);
> + } while (read_seqretry(&dev_priv->psr.debug_lock, seq));
What does the seqlock buy us?
> + }
> +
> intel_runtime_pm_put(dev_priv);
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 136fa2267a66..b8170882e1ab 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -609,6 +609,9 @@ struct i915_psr {
> bool alpm;
> bool has_hw_tracking;
> bool debug;
> + ktime_t last_entry_attempt;
> + ktime_t last_exit;
> + seqlock_t debug_lock;
>
> void (*enable_source)(struct intel_dp *,
> const struct intel_crtc_state *);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 64ecea80438d..a83d95b1b587 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -125,28 +125,44 @@ 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();
> + unsigned long flags = 0;
>
> if (INTEL_GEN(dev_priv) >= 8)
> transcoders |= BIT(TRANSCODER_A) |
> BIT(TRANSCODER_B) |
> BIT(TRANSCODER_C);
>
> + write_seqlock_irqsave(&dev_priv->psr.debug_lock, flags);
> for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> + bool handled = false;
> +
> + /* PSR supported only on one transcoder currently */
> + WARN_ON_ONCE(handled);
> +
> /* FIXME: Exit PSR when this happens. */
> - if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> + handled = true;
> DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
> transcoder_name(cpu_transcoder));
>
> + }
> +
> if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> - DRM_DEBUG_KMS("[transcoder %s] PSR entry in 2 vblanks\n",
> + handled = true;
> + 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)) {
> + handled = true;
> + dev_priv->psr.last_exit = time_ns;
> DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> transcoder_name(cpu_transcoder));
> }
> }
> + write_sequnlock_irqrestore(&dev_priv->psr.debug_lock, flags);
> }
>
> static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> @@ -1160,6 +1176,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>
> INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
> mutex_init(&dev_priv->psr.lock);
> + seqlock_init(&dev_priv->psr.debug_lock);
>
> if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> dev_priv->psr.enable_source = vlv_psr_enable_source;
> --
> 2.14.1
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/5] drm/i915/psr: Control PSR interrupts via debugfs
2018-03-21 19:45 ` Ville Syrjälä
@ 2018-03-22 0:59 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 25+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-22 0:59 UTC (permalink / raw)
To: ville.syrjala; +Cc: Vetter, Daniel, intel-gfx, Vivi, Rodrigo
On Wed, 2018-03-21 at 21:45 +0200, Ville Syrjälä wrote:
> On Tue, Mar 20, 2018 at 03:41:50PM -0700, Dhinakaran Pandiyan wrote:
> > Interrupts other than the one for AUX errors are required only for debug,
> > so unmask them via debugfs when the user requests debug.
> >
> > User can make such a request with
> > echo 1 > <DEBUG_FS>/dri/0/i915_edp_psr_debug
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 36 +++++++++++++++++++-
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/i915_irq.c | 68 ++++++++++++++++---------------------
> > drivers/gpu/drm/i915/intel_drv.h | 2 ++
> > drivers/gpu/drm/i915/intel_psr.c | 56 ++++++++++++++++++++++++++++++
> > 5 files changed, 123 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 964ea1a12357..669f3d56054a 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2690,6 +2690,39 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> > return 0;
> > }
> >
> > +static int
> > +i915_edp_psr_debug_set(void *data, u64 val)
> > +{
> > + struct drm_i915_private *dev_priv = data;
> > +
> > + if (!CAN_PSR(dev_priv))
> > + return -ENODEV;
> > +
> > + if (val < 0 || val > 1)
> > + return -EINVAL;
> > +
> > + DRM_DEBUG_KMS("%s PSR debug\n", val == 1 ? "Enabling" : "Disabling");
> > + intel_psr_debug_control(dev_priv, val);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +i915_edp_psr_debug_get(void *data, u64 *val)
> > +{
> > + struct drm_i915_private *dev_priv = data;
> > +
> > + if (!CAN_PSR(dev_priv))
> > + return -ENODEV;
> > +
> > + *val = READ_ONCE(dev_priv->psr.debug);
> > + return 0;
> > +}
> > +
> > +DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops,
> > + i915_edp_psr_debug_get, i915_edp_psr_debug_set,
> > + "%llu\n");
> > +
> > static int i915_sink_crc(struct seq_file *m, void *data)
> > {
> > struct drm_i915_private *dev_priv = node_to_i915(m->private);
> > @@ -4811,7 +4844,8 @@ static const struct i915_debugfs_files {
> > {"i915_guc_log_relay", &i915_guc_log_relay_fops},
> > {"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops},
> > {"i915_ipc_status", &i915_ipc_status_fops},
> > - {"i915_drrs_ctl", &i915_drrs_ctl_fops}
> > + {"i915_drrs_ctl", &i915_drrs_ctl_fops},
> > + {"i915_edp_psr_debug", &i915_edp_psr_debug_fops}
> > };
> >
> > int i915_debugfs_register(struct drm_i915_private *dev_priv)
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index e27ba8fb64e6..136fa2267a66 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -608,6 +608,7 @@ struct i915_psr {
> > bool colorimetry_support;
> > bool alpm;
> > bool has_hw_tracking;
> > + bool debug;
> >
> > void (*enable_source)(struct intel_dp *,
> > const struct intel_crtc_state *);
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 56228e8ed980..94941b52f1cf 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2392,40 +2392,6 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
> > ironlake_rps_change_irq_handler(dev_priv);
> > }
> >
> > -static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
> > -{
> > - u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
> > - u32 edp_psr_imr = I915_READ(EDP_PSR_IMR);
> > - u32 mask = BIT(TRANSCODER_EDP);
> > - enum transcoder cpu_transcoder;
> > -
> > - if (INTEL_GEN(dev_priv) >= 8)
> > - mask |= BIT(TRANSCODER_A) |
> > - BIT(TRANSCODER_B) |
> > - BIT(TRANSCODER_C);
> > -
> > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, mask) {
> > - if (edp_psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > - DRM_DEBUG_KMS("Transcoder %s PSR error\n",
> > - transcoder_name(cpu_transcoder));
> > -
> > - if (edp_psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > - DRM_DEBUG_KMS("Transcoder %s PSR prepare entry in 2 vblanks\n",
> > - transcoder_name(cpu_transcoder));
> > - edp_psr_imr |= EDP_PSR_PRE_ENTRY(cpu_transcoder);
> > - }
> > -
> > - if (edp_psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> > - DRM_DEBUG_KMS("Transcoder %s PSR exit completed\n",
> > - transcoder_name(cpu_transcoder));
> > - edp_psr_imr &= ~EDP_PSR_PRE_ENTRY(cpu_transcoder);
> > - }
> > - }
> > -
> > - I915_WRITE(EDP_PSR_IMR, edp_psr_imr);
> > - I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
> > -}
> > -
> > static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > u32 de_iir)
> > {
> > @@ -2438,8 +2404,12 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > if (de_iir & DE_ERR_INT_IVB)
> > ivb_err_int_handler(dev_priv);
> >
> > - if (de_iir & DE_EDP_PSR_INT_HSW)
> > - hsw_edp_psr_irq_handler(dev_priv);
> > + if (de_iir & DE_EDP_PSR_INT_HSW) {
> > + u32 psr_iir = I915_READ(EDP_PSR_IIR);
> > +
> > + intel_psr_irq_handler(dev_priv, psr_iir);
> > + I915_WRITE(EDP_PSR_IIR, psr_iir);
> > + }
> >
> > if (de_iir & DE_AUX_CHANNEL_A_IVB)
> > dp_aux_irq_handler(dev_priv);
> > @@ -2581,7 +2551,10 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> > }
> >
> > if (iir & GEN8_DE_EDP_PSR) {
> > - hsw_edp_psr_irq_handler(dev_priv);
> > + u32 psr_iir = I915_READ(EDP_PSR_IIR);
> > +
> > + intel_psr_irq_handler(dev_priv, psr_iir);
> > + I915_WRITE(EDP_PSR_IIR, psr_iir);
> > found = true;
> > }
> >
> > @@ -3699,6 +3672,23 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
> > }
> > }
> >
> > +static inline u32 psr_mask(struct drm_i915_private *dev_priv)
> > +{
> > + u32 transcoders = BIT(TRANSCODER_EDP);
> > + u32 mask = 0;
> > + enum transcoder cpu_transcoder;
> > +
> > + 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)
> > + mask |= EDP_PSR_ERROR(cpu_transcoder);
> > +
> > + return ~mask;
> > +}
> > +
> > static int ironlake_irq_postinstall(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -3721,7 +3711,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> >
> > if (IS_HASWELL(dev_priv)) {
> > gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> > - I915_WRITE(EDP_PSR_IMR, 0);
> > + I915_WRITE(EDP_PSR_IMR, psr_mask(dev_priv));
> > display_mask |= DE_EDP_PSR_INT_HSW;
> > }
> >
> > @@ -3861,7 +3851,7 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> > de_port_enables |= GEN8_PORT_DP_A_HOTPLUG;
> >
> > gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> > - I915_WRITE(EDP_PSR_IMR, 0);
> > + I915_WRITE(EDP_PSR_IMR, psr_mask(dev_priv));
>
> psr_mask() seems somewhat pointless to me. Could perhaps just
> write the full mask by hand in these cases.
>
Okay. I'll replace these with
EDP_PSR_ERROR(TRANSCODER_EDP)
and
psr_mask = PSR_ERROR(TRANSCODER_EDP) |
PSR_ERROR(TRANSCODER_A) |
PSR_ERROR(TRANSCODER_B) |
PSR_ERROR(TRANSCODER_C) |
> >
> > for_each_pipe(dev_priv, pipe) {
> > dev_priv->de_irq_mask[pipe] = ~de_pipe_masked;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index a215aa78b0be..8be75cc5bf24 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1887,6 +1887,8 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
> > unsigned frontbuffer_bits);
> > void intel_psr_compute_config(struct intel_dp *intel_dp,
> > struct intel_crtc_state *crtc_state);
> > +void intel_psr_debug_control(struct drm_i915_private *dev_priv, bool enable);
> > +void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
> >
> > /* intel_runtime_pm.c */
> > int intel_power_domains_init(struct drm_i915_private *);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 317cb4a12693..64ecea80438d 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -93,6 +93,62 @@ static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> > intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> > }
> >
> > +void intel_psr_debug_control(struct drm_i915_private *dev_priv, bool enable)
> > +{
> > + u32 transcoders = BIT(TRANSCODER_EDP);
> > + u32 mask = 0;
> > + enum transcoder cpu_transcoder;
> > +
> > + /* No PSR interrupts on VLV/CHV */
> > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > + return;
> > +
> > + 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)
> > + mask = EDP_PSR_POST_EXIT(cpu_transcoder) |
> > + EDP_PSR_PRE_ENTRY(cpu_transcoder);
>
> This too could be done w/o the loop I think.
>
Okay, will write it as.
mask = EDP_PSR_POST_EXIT(TRANSCODER_EDP) |
DP_PSR_PRE_ENTRY(TRANSCODER_EDP);
if (INTEL_GEN(dev_priv) >= 8)
mask |= EDP_PSR_POST_EXIT(TRANSCODER_A) |
EDP_PSR_PRE_ENTRY(TRANSCODER_A) |
EDP_PSR_POST_EXIT(TRANSCODER_B) | EDP_PSR_PRE_ENTRY(TRANSCODER_B) |
EDP_PSR_POST_EXIT(TRANSCODER_C) | EDP_PSR_PRE_ENTRY(TRANSCODER_C);
> > +
> > + if (enable) {
> > + WRITE_ONCE(dev_priv->psr.debug, true);
> > + I915_WRITE(EDP_PSR_IMR, ~mask);
> > + } else {
> > + I915_WRITE(EDP_PSR_IMR, mask);
> > + WRITE_ONCE(dev_priv->psr.debug, false);
> > + }
> > +}
> > +
> > +void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
> > +{
> > + u32 transcoders = BIT(TRANSCODER_EDP);
> > + enum transcoder cpu_transcoder;
> > +
> > + 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) {
> > + /* FIXME: Exit PSR when this happens. */
> > + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > + DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
> > + transcoder_name(cpu_transcoder));
> > +
> > + if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > + DRM_DEBUG_KMS("[transcoder %s] PSR entry in 2 vblanks\n",
> > + transcoder_name(cpu_transcoder));
> > + }
> > +
> > + if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> > + DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> > + transcoder_name(cpu_transcoder));
> > + }
> > + }
> > +}
> > +
> > static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > {
> > uint8_t psr_caps = 0;
> > --
> > 2.14.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts.
2018-03-21 19:48 ` Ville Syrjälä
@ 2018-03-22 1:05 ` Pandiyan, Dhinakaran
2018-03-22 9:21 ` Ville Syrjälä
0 siblings, 1 reply; 25+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-22 1:05 UTC (permalink / raw)
To: ville.syrjala; +Cc: Vetter, Daniel, intel-gfx, Vivi, Rodrigo
On Wed, 2018-03-21 at 21:48 +0200, Ville Syrjälä wrote:
> On Tue, Mar 20, 2018 at 03:41:51PM -0700, Dhinakaran Pandiyan wrote:
> > Timestamps are useful for IGT tests that trigger PSR exit and/or wait for
> > PSR entry.
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++++
> > drivers/gpu/drm/i915/i915_drv.h | 3 +++
> > drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++++++--
> > 3 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 669f3d56054a..d28dc4d8388e 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2686,6 +2686,18 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> > }
> > mutex_unlock(&dev_priv->psr.lock);
> >
> > + if (READ_ONCE(dev_priv->psr.debug)) {
> > + unsigned int seq;
> > +
> > + do {
> > + seq = read_seqbegin(&dev_priv->psr.debug_lock);
> > + seq_printf(m, "Last attempted entry at: %lld\n",
> > + dev_priv->psr.last_entry_attempt);
> > + seq_printf(m, "Last exit at: %lld\n",
> > + dev_priv->psr.last_exit);
> > + } while (read_seqretry(&dev_priv->psr.debug_lock, seq));
>
> What does the seqlock buy us?
Reading debugfs wouldn't block the update inside the irq handler,
compared to using a spinlock. We need to serialize the read and update
parts, don't we? Otherwise tests might end up reading partial updates.
>
> > + }
> > +
> > intel_runtime_pm_put(dev_priv);
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 136fa2267a66..b8170882e1ab 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -609,6 +609,9 @@ struct i915_psr {
> > bool alpm;
> > bool has_hw_tracking;
> > bool debug;
> > + ktime_t last_entry_attempt;
> > + ktime_t last_exit;
> > + seqlock_t debug_lock;
> >
> > void (*enable_source)(struct intel_dp *,
> > const struct intel_crtc_state *);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 64ecea80438d..a83d95b1b587 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -125,28 +125,44 @@ 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();
> > + unsigned long flags = 0;
> >
> > if (INTEL_GEN(dev_priv) >= 8)
> > transcoders |= BIT(TRANSCODER_A) |
> > BIT(TRANSCODER_B) |
> > BIT(TRANSCODER_C);
> >
> > + write_seqlock_irqsave(&dev_priv->psr.debug_lock, flags);
> > for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> > + bool handled = false;
> > +
> > + /* PSR supported only on one transcoder currently */
> > + WARN_ON_ONCE(handled);
> > +
> > /* FIXME: Exit PSR when this happens. */
> > - if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > + handled = true;
> > DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
> > transcoder_name(cpu_transcoder));
> >
> > + }
> > +
> > if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > - DRM_DEBUG_KMS("[transcoder %s] PSR entry in 2 vblanks\n",
> > + handled = true;
> > + 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)) {
> > + handled = true;
> > + dev_priv->psr.last_exit = time_ns;
> > DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> > transcoder_name(cpu_transcoder));
> > }
> > }
> > + write_sequnlock_irqrestore(&dev_priv->psr.debug_lock, flags);
> > }
> >
> > static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > @@ -1160,6 +1176,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
> >
> > INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
> > mutex_init(&dev_priv->psr.lock);
> > + seqlock_init(&dev_priv->psr.debug_lock);
> >
> > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > dev_priv->psr.enable_source = vlv_psr_enable_source;
> > --
> > 2.14.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw
2018-03-21 19:29 ` Ville Syrjälä
@ 2018-03-22 1:19 ` Pandiyan, Dhinakaran
2018-03-22 11:33 ` Ville Syrjälä
0 siblings, 1 reply; 25+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-22 1:19 UTC (permalink / raw)
To: ville.syrjala; +Cc: daniel.vetter, Vetter, Daniel, intel-gfx, Vivi, Rodrigo
On Wed, 2018-03-21 at 21:29 +0200, Ville Syrjälä wrote:
> On Wed, Mar 21, 2018 at 07:19:53PM +0000, Pandiyan, Dhinakaran wrote:
> >
> >
> >
> > On Wed, 2018-03-21 at 20:59 +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 20, 2018 at 03:41:47PM -0700, Dhinakaran Pandiyan wrote:
> > > > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > >
> > > > The definitions for the error register should be valid on bdw/skl too,
> > > > but there we haven't even enabled DE_MISC handling yet.
> > > >
> > > > Somewhat confusing the the moved register offset on bdw is only for
> > > > the _CTL/_AUX register, and that _IIR/IMR stayed where they have been
> > > > on bdw.
> > > >
> > > > v2: Fixes from Ville.
> > > >
> > > > v3: From DK
> > > > * Rebased on drm-tip
> > > > * Removed BDW IIR bit definition, looks like an unintentional change that
> > > > should be in the following patch.
> > > >
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++++++++
> > > > drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++
> > > > drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> > > > 3 files changed, 44 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > > index 44eef355e12c..e94a835b7515 100644
> > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > @@ -2392,6 +2392,26 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
> > > > ironlake_rps_change_irq_handler(dev_priv);
> > > > }
> > > >
> > > > +static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
> > > > +{
> > > > + u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
> > > > +
> > > > + if (edp_psr_iir & EDP_PSR_ERROR)
> > > > + DRM_DEBUG_KMS("PSR error\n");
> > > > +
> > > > + if (edp_psr_iir & EDP_PSR_PRE_ENTRY) {
> > > > + DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n");
> > >
> > > I doubt we want to have the entry/exit interrupts generate all this
> > > noise in dmesg all the time. We should probably only enable the
> > > interrupts for testing. The error interrupt I suppose we want always,
> > > might even want DRM_ERROR on it.
> >
> > I implement debugfs control in Patch 4/5, agreed on DRM_ERROR.
>
> Right. It's a bit hard to read this with stuff getting
> added/remove/moved more or less randomly.
>
> >
> > >
> > > > + I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY);
> > > > + }
> > > > +
> > > > + if (edp_psr_iir & EDP_PSR_POST_EXIT) {
> > > > + DRM_DEBUG_KMS("PSR exit completed\n");
> > > > + I915_WRITE(EDP_PSR_IMR, 0);
> > > > + }
> > > > +
> > > > + I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
> > > > +}
> > > > +
> > > > static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > > > u32 de_iir)
> > > > {
> > > > @@ -2404,6 +2424,9 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > > > if (de_iir & DE_ERR_INT_IVB)
> > > > ivb_err_int_handler(dev_priv);
> > > >
> > > > + if (de_iir & DE_EDP_PSR_INT_HSW)
> > > > + hsw_edp_psr_irq_handler(dev_priv);
> > > > +
> > > > if (de_iir & DE_AUX_CHANNEL_A_IVB)
> > > > dp_aux_irq_handler(dev_priv);
> > > >
> > > > @@ -3252,6 +3275,11 @@ static void ironlake_irq_reset(struct drm_device *dev)
> > > > if (IS_GEN7(dev_priv))
> > > > I915_WRITE(GEN7_ERR_INT, 0xffffffff);
> > > >
> > > > + if (IS_HASWELL(dev_priv)) {
> > > > + I915_WRITE(EDP_PSR_IMR, 0xffffffff);
> > > > + I915_WRITE(EDP_PSR_IIR, 0xffffffff);
> > > > + }
> > > > +
> > > > gen5_gt_irq_reset(dev_priv);
> > > >
> > > > ibx_irq_reset(dev_priv);
> > > > @@ -3663,6 +3691,12 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> > > > DE_DP_A_HOTPLUG);
> > > > }
> > > >
> > > > + if (IS_HASWELL(dev_priv)) {
> > > > + gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> > > > + I915_WRITE(EDP_PSR_IMR, 0);
> > > > + display_mask |= DE_EDP_PSR_INT_HSW;
> > > > + }
> > > > +
> > > > dev_priv->irq_mask = ~display_mask;
> > > >
> > > > ibx_irq_pre_postinstall(dev);
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 1e000f3004cb..3447f03eac3d 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -3828,6 +3828,13 @@ 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 */
> > > > +#define EDP_PSR_IMR _MMIO(0x64834)
> > > > +#define EDP_PSR_IIR _MMIO(0x64838)
> > > > +#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_DATA(i) _MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
> > > >
> > > > @@ -6628,6 +6635,7 @@ enum {
> > > > #define DE_PCH_EVENT_IVB (1<<28)
> > > > #define DE_DP_A_HOTPLUG_IVB (1<<27)
> > > > #define DE_AUX_CHANNEL_A_IVB (1<<26)
> > > > +#define DE_EDP_PSR_INT_HSW (1<<19)
> > > > #define DE_SPRITEC_FLIP_DONE_IVB (1<<14)
> > > > #define DE_PLANEC_FLIP_DONE_IVB (1<<13)
> > > > #define DE_PIPEC_VBLANK_IVB (1<<10)
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 317cb4a12693..27dfd507a4f7 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -613,7 +613,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > > I915_WRITE(EDP_PSR_DEBUG,
> > > > EDP_PSR_DEBUG_MASK_MEMUP |
> > > > EDP_PSR_DEBUG_MASK_HPD |
> > > > - EDP_PSR_DEBUG_MASK_LPSP);
> > > > + EDP_PSR_DEBUG_MASK_LPSP |
> > > > + EDP_PSR_DEBUG_MASK_REG_WRITE);
> > >
> > > I don't think Daniel's original had this actually. I think I added it
> > > because I didn't want mmio to mess up my testing.
> >
> > It gets removed in a separate patch (2/5). So I assumed Daniel added it
> > and you removed it.
>
> Hmm. That could indeed be the case.
>
> >
> >
> > > In general I'd really
> > > prefer to mask everything so that we can be sure that the PSR exits
> > > happen when we want/need them and not randomly due to other activity.
> > >
> > > If any random mmio can cause a PSR exit it's going to hard to prove that
> > > we're doing the correct thing. Also seems pretty wasteful if eg. GT
> > > interrupts can cause PSR exits for no good reason. I never tested to see
> > > which registers cause the PSR exit. That would be good to know.
> > >
> >
> > The same bit on SKL+ is called "Mask Display Reg Write", so it either
> > means
> > 1) HSW and BDW exit only on display register writes and the bspec
> > description is ambiguous or
>
> The problem is that eg. some of the interrupt registers live in the
> display block. Hence any interrupt could potentially cause the PSR exit.
> As could any register read we do for debug purpose etc. So at least I
> would feel safer if PSR can be made to work reliably with more or less
> everything masked.
>
The idea is to move towards hardware triggered exits since that's what
is necessary for PSR2 selective updates[1]. More importantly, there are
places in the driver where we can't manually exit PSR without
circumventing locking issues - from vblank enable for instance. Relying
on hardware triggered exits in the modeset/commit path keeps it
consistent IMO.
[1] Rodrigo found a register (PSR2_MAN_TRK_CTL) that allows manual
control of selective updates. I don't know how well it works though.
> > 2) exiting specifically on display MMIO writes was added in SKL.
> >
> > I'll see if I can get a HSW/BDW with PSR panel to test, but assuming the
> > former is safer.
> >
> >
> >
> > > > }
> > > > }
> > > >
> > > > --
> > > > 2.14.1
> > >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts.
2018-03-22 1:05 ` Pandiyan, Dhinakaran
@ 2018-03-22 9:21 ` Ville Syrjälä
2018-03-22 20:59 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 25+ messages in thread
From: Ville Syrjälä @ 2018-03-22 9:21 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: Vetter, Daniel, intel-gfx, Vivi, Rodrigo
On Thu, Mar 22, 2018 at 01:05:24AM +0000, Pandiyan, Dhinakaran wrote:
> On Wed, 2018-03-21 at 21:48 +0200, Ville Syrjälä wrote:
> > On Tue, Mar 20, 2018 at 03:41:51PM -0700, Dhinakaran Pandiyan wrote:
> > > Timestamps are useful for IGT tests that trigger PSR exit and/or wait for
> > > PSR entry.
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++++
> > > drivers/gpu/drm/i915/i915_drv.h | 3 +++
> > > drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++++++--
> > > 3 files changed, 34 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 669f3d56054a..d28dc4d8388e 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2686,6 +2686,18 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> > > }
> > > mutex_unlock(&dev_priv->psr.lock);
> > >
> > > + if (READ_ONCE(dev_priv->psr.debug)) {
> > > + unsigned int seq;
> > > +
> > > + do {
> > > + seq = read_seqbegin(&dev_priv->psr.debug_lock);
> > > + seq_printf(m, "Last attempted entry at: %lld\n",
> > > + dev_priv->psr.last_entry_attempt);
> > > + seq_printf(m, "Last exit at: %lld\n",
> > > + dev_priv->psr.last_exit);
> > > + } while (read_seqretry(&dev_priv->psr.debug_lock, seq));
> >
> > What does the seqlock buy us?
>
> Reading debugfs wouldn't block the update inside the irq handler,
> compared to using a spinlock. We need to serialize the read and update
> parts, don't we? Otherwise tests might end up reading partial updates.
Hmm. ktime_t is 64 bits so I guess on 32bit systems it could be a slight
issue. Not sure we should care that much about PSR debug on 32bit though.
It's a rather unlikely configuration, and at least we don't do that in
the ci.
>
> >
> > > + }
> > > +
> > > intel_runtime_pm_put(dev_priv);
> > > return 0;
> > > }
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 136fa2267a66..b8170882e1ab 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -609,6 +609,9 @@ struct i915_psr {
> > > bool alpm;
> > > bool has_hw_tracking;
> > > bool debug;
> > > + ktime_t last_entry_attempt;
> > > + ktime_t last_exit;
> > > + seqlock_t debug_lock;
> > >
> > > void (*enable_source)(struct intel_dp *,
> > > const struct intel_crtc_state *);
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > index 64ecea80438d..a83d95b1b587 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -125,28 +125,44 @@ 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();
> > > + unsigned long flags = 0;
> > >
> > > if (INTEL_GEN(dev_priv) >= 8)
> > > transcoders |= BIT(TRANSCODER_A) |
> > > BIT(TRANSCODER_B) |
> > > BIT(TRANSCODER_C);
> > >
> > > + write_seqlock_irqsave(&dev_priv->psr.debug_lock, flags);
> > > for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> > > + bool handled = false;
> > > +
> > > + /* PSR supported only on one transcoder currently */
> > > + WARN_ON_ONCE(handled);
> > > +
> > > /* FIXME: Exit PSR when this happens. */
> > > - if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > > + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > > + handled = true;
> > > DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
> > > transcoder_name(cpu_transcoder));
> > >
> > > + }
> > > +
> > > if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > > - DRM_DEBUG_KMS("[transcoder %s] PSR entry in 2 vblanks\n",
> > > + handled = true;
> > > + 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)) {
> > > + handled = true;
> > > + dev_priv->psr.last_exit = time_ns;
> > > DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> > > transcoder_name(cpu_transcoder));
> > > }
> > > }
> > > + write_sequnlock_irqrestore(&dev_priv->psr.debug_lock, flags);
> > > }
> > >
> > > static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > > @@ -1160,6 +1176,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
> > >
> > > INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
> > > mutex_init(&dev_priv->psr.lock);
> > > + seqlock_init(&dev_priv->psr.debug_lock);
> > >
> > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > dev_priv->psr.enable_source = vlv_psr_enable_source;
> > > --
> > > 2.14.1
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw
2018-03-22 1:19 ` Pandiyan, Dhinakaran
@ 2018-03-22 11:33 ` Ville Syrjälä
2018-03-22 20:39 ` Rodrigo Vivi
2018-03-23 0:01 ` Pandiyan, Dhinakaran
0 siblings, 2 replies; 25+ messages in thread
From: Ville Syrjälä @ 2018-03-22 11:33 UTC (permalink / raw)
To: Pandiyan, Dhinakaran
Cc: daniel.vetter, Vetter, Daniel, intel-gfx, Vivi, Rodrigo
On Thu, Mar 22, 2018 at 01:19:19AM +0000, Pandiyan, Dhinakaran wrote:
>
>
>
> On Wed, 2018-03-21 at 21:29 +0200, Ville Syrjälä wrote:
> > On Wed, Mar 21, 2018 at 07:19:53PM +0000, Pandiyan, Dhinakaran wrote:
> > >
> > >
> > >
> > > On Wed, 2018-03-21 at 20:59 +0200, Ville Syrjälä wrote:
> > > > On Tue, Mar 20, 2018 at 03:41:47PM -0700, Dhinakaran Pandiyan wrote:
> > > > > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > >
> > > > > The definitions for the error register should be valid on bdw/skl too,
> > > > > but there we haven't even enabled DE_MISC handling yet.
> > > > >
> > > > > Somewhat confusing the the moved register offset on bdw is only for
> > > > > the _CTL/_AUX register, and that _IIR/IMR stayed where they have been
> > > > > on bdw.
> > > > >
> > > > > v2: Fixes from Ville.
> > > > >
> > > > > v3: From DK
> > > > > * Rebased on drm-tip
> > > > > * Removed BDW IIR bit definition, looks like an unintentional change that
> > > > > should be in the following patch.
> > > > >
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++++++++
> > > > > drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++
> > > > > drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> > > > > 3 files changed, 44 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > > > index 44eef355e12c..e94a835b7515 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > @@ -2392,6 +2392,26 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
> > > > > ironlake_rps_change_irq_handler(dev_priv);
> > > > > }
> > > > >
> > > > > +static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
> > > > > +{
> > > > > + u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
> > > > > +
> > > > > + if (edp_psr_iir & EDP_PSR_ERROR)
> > > > > + DRM_DEBUG_KMS("PSR error\n");
> > > > > +
> > > > > + if (edp_psr_iir & EDP_PSR_PRE_ENTRY) {
> > > > > + DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n");
> > > >
> > > > I doubt we want to have the entry/exit interrupts generate all this
> > > > noise in dmesg all the time. We should probably only enable the
> > > > interrupts for testing. The error interrupt I suppose we want always,
> > > > might even want DRM_ERROR on it.
> > >
> > > I implement debugfs control in Patch 4/5, agreed on DRM_ERROR.
> >
> > Right. It's a bit hard to read this with stuff getting
> > added/remove/moved more or less randomly.
> >
> > >
> > > >
> > > > > + I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY);
> > > > > + }
> > > > > +
> > > > > + if (edp_psr_iir & EDP_PSR_POST_EXIT) {
> > > > > + DRM_DEBUG_KMS("PSR exit completed\n");
> > > > > + I915_WRITE(EDP_PSR_IMR, 0);
> > > > > + }
> > > > > +
> > > > > + I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
> > > > > +}
> > > > > +
> > > > > static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > > > > u32 de_iir)
> > > > > {
> > > > > @@ -2404,6 +2424,9 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > > > > if (de_iir & DE_ERR_INT_IVB)
> > > > > ivb_err_int_handler(dev_priv);
> > > > >
> > > > > + if (de_iir & DE_EDP_PSR_INT_HSW)
> > > > > + hsw_edp_psr_irq_handler(dev_priv);
> > > > > +
> > > > > if (de_iir & DE_AUX_CHANNEL_A_IVB)
> > > > > dp_aux_irq_handler(dev_priv);
> > > > >
> > > > > @@ -3252,6 +3275,11 @@ static void ironlake_irq_reset(struct drm_device *dev)
> > > > > if (IS_GEN7(dev_priv))
> > > > > I915_WRITE(GEN7_ERR_INT, 0xffffffff);
> > > > >
> > > > > + if (IS_HASWELL(dev_priv)) {
> > > > > + I915_WRITE(EDP_PSR_IMR, 0xffffffff);
> > > > > + I915_WRITE(EDP_PSR_IIR, 0xffffffff);
> > > > > + }
> > > > > +
> > > > > gen5_gt_irq_reset(dev_priv);
> > > > >
> > > > > ibx_irq_reset(dev_priv);
> > > > > @@ -3663,6 +3691,12 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> > > > > DE_DP_A_HOTPLUG);
> > > > > }
> > > > >
> > > > > + if (IS_HASWELL(dev_priv)) {
> > > > > + gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> > > > > + I915_WRITE(EDP_PSR_IMR, 0);
> > > > > + display_mask |= DE_EDP_PSR_INT_HSW;
> > > > > + }
> > > > > +
> > > > > dev_priv->irq_mask = ~display_mask;
> > > > >
> > > > > ibx_irq_pre_postinstall(dev);
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index 1e000f3004cb..3447f03eac3d 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -3828,6 +3828,13 @@ 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 */
> > > > > +#define EDP_PSR_IMR _MMIO(0x64834)
> > > > > +#define EDP_PSR_IIR _MMIO(0x64838)
> > > > > +#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_DATA(i) _MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
> > > > >
> > > > > @@ -6628,6 +6635,7 @@ enum {
> > > > > #define DE_PCH_EVENT_IVB (1<<28)
> > > > > #define DE_DP_A_HOTPLUG_IVB (1<<27)
> > > > > #define DE_AUX_CHANNEL_A_IVB (1<<26)
> > > > > +#define DE_EDP_PSR_INT_HSW (1<<19)
> > > > > #define DE_SPRITEC_FLIP_DONE_IVB (1<<14)
> > > > > #define DE_PLANEC_FLIP_DONE_IVB (1<<13)
> > > > > #define DE_PIPEC_VBLANK_IVB (1<<10)
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index 317cb4a12693..27dfd507a4f7 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -613,7 +613,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > > > I915_WRITE(EDP_PSR_DEBUG,
> > > > > EDP_PSR_DEBUG_MASK_MEMUP |
> > > > > EDP_PSR_DEBUG_MASK_HPD |
> > > > > - EDP_PSR_DEBUG_MASK_LPSP);
> > > > > + EDP_PSR_DEBUG_MASK_LPSP |
> > > > > + EDP_PSR_DEBUG_MASK_REG_WRITE);
> > > >
> > > > I don't think Daniel's original had this actually. I think I added it
> > > > because I didn't want mmio to mess up my testing.
> > >
> > > It gets removed in a separate patch (2/5). So I assumed Daniel added it
> > > and you removed it.
> >
> > Hmm. That could indeed be the case.
> >
> > >
> > >
> > > > In general I'd really
> > > > prefer to mask everything so that we can be sure that the PSR exits
> > > > happen when we want/need them and not randomly due to other activity.
> > > >
> > > > If any random mmio can cause a PSR exit it's going to hard to prove that
> > > > we're doing the correct thing. Also seems pretty wasteful if eg. GT
> > > > interrupts can cause PSR exits for no good reason. I never tested to see
> > > > which registers cause the PSR exit. That would be good to know.
> > > >
> > >
> > > The same bit on SKL+ is called "Mask Display Reg Write", so it either
> > > means
> > > 1) HSW and BDW exit only on display register writes and the bspec
> > > description is ambiguous or
> >
> > The problem is that eg. some of the interrupt registers live in the
> > display block. Hence any interrupt could potentially cause the PSR exit.
> > As could any register read we do for debug purpose etc. So at least I
> > would feel safer if PSR can be made to work reliably with more or less
> > everything masked.
> >
> The idea is to move towards hardware triggered exits since that's what
> is necessary for PSR2 selective updates[1]. More importantly, there are
> places in the driver where we can't manually exit PSR without
> circumventing locking issues - from vblank enable for instance. Relying
> on hardware triggered exits in the modeset/commit path keeps it
> consistent IMO.
What's the difference between the hardware behaviour between "write any
random mmio to trigger an exit" vs. "write some specific register to
trigger an exit". I presume there is a way to trigger the exit
explicitly?
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw
2018-03-22 11:33 ` Ville Syrjälä
@ 2018-03-22 20:39 ` Rodrigo Vivi
2018-03-23 0:01 ` Pandiyan, Dhinakaran
1 sibling, 0 replies; 25+ messages in thread
From: Rodrigo Vivi @ 2018-03-22 20:39 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Vetter, Daniel, intel-gfx, Pandiyan, Dhinakaran, daniel.vetter
On Thu, Mar 22, 2018 at 01:33:10PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 22, 2018 at 01:19:19AM +0000, Pandiyan, Dhinakaran wrote:
> >
> >
> >
> > On Wed, 2018-03-21 at 21:29 +0200, Ville Syrjälä wrote:
> > > On Wed, Mar 21, 2018 at 07:19:53PM +0000, Pandiyan, Dhinakaran wrote:
> > > >
> > > >
> > > >
> > > > On Wed, 2018-03-21 at 20:59 +0200, Ville Syrjälä wrote:
> > > > > On Tue, Mar 20, 2018 at 03:41:47PM -0700, Dhinakaran Pandiyan wrote:
> > > > > > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > >
> > > > > > The definitions for the error register should be valid on bdw/skl too,
> > > > > > but there we haven't even enabled DE_MISC handling yet.
> > > > > >
> > > > > > Somewhat confusing the the moved register offset on bdw is only for
> > > > > > the _CTL/_AUX register, and that _IIR/IMR stayed where they have been
> > > > > > on bdw.
> > > > > >
> > > > > > v2: Fixes from Ville.
> > > > > >
> > > > > > v3: From DK
> > > > > > * Rebased on drm-tip
> > > > > > * Removed BDW IIR bit definition, looks like an unintentional change that
> > > > > > should be in the following patch.
> > > > > >
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++++++++
> > > > > > drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++
> > > > > > drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> > > > > > 3 files changed, 44 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > index 44eef355e12c..e94a835b7515 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > @@ -2392,6 +2392,26 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
> > > > > > ironlake_rps_change_irq_handler(dev_priv);
> > > > > > }
> > > > > >
> > > > > > +static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
> > > > > > +{
> > > > > > + u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
> > > > > > +
> > > > > > + if (edp_psr_iir & EDP_PSR_ERROR)
> > > > > > + DRM_DEBUG_KMS("PSR error\n");
> > > > > > +
> > > > > > + if (edp_psr_iir & EDP_PSR_PRE_ENTRY) {
> > > > > > + DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n");
> > > > >
> > > > > I doubt we want to have the entry/exit interrupts generate all this
> > > > > noise in dmesg all the time. We should probably only enable the
> > > > > interrupts for testing. The error interrupt I suppose we want always,
> > > > > might even want DRM_ERROR on it.
> > > >
> > > > I implement debugfs control in Patch 4/5, agreed on DRM_ERROR.
> > >
> > > Right. It's a bit hard to read this with stuff getting
> > > added/remove/moved more or less randomly.
> > >
> > > >
> > > > >
> > > > > > + I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY);
> > > > > > + }
> > > > > > +
> > > > > > + if (edp_psr_iir & EDP_PSR_POST_EXIT) {
> > > > > > + DRM_DEBUG_KMS("PSR exit completed\n");
> > > > > > + I915_WRITE(EDP_PSR_IMR, 0);
> > > > > > + }
> > > > > > +
> > > > > > + I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
> > > > > > +}
> > > > > > +
> > > > > > static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > > > > > u32 de_iir)
> > > > > > {
> > > > > > @@ -2404,6 +2424,9 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > > > > > if (de_iir & DE_ERR_INT_IVB)
> > > > > > ivb_err_int_handler(dev_priv);
> > > > > >
> > > > > > + if (de_iir & DE_EDP_PSR_INT_HSW)
> > > > > > + hsw_edp_psr_irq_handler(dev_priv);
> > > > > > +
> > > > > > if (de_iir & DE_AUX_CHANNEL_A_IVB)
> > > > > > dp_aux_irq_handler(dev_priv);
> > > > > >
> > > > > > @@ -3252,6 +3275,11 @@ static void ironlake_irq_reset(struct drm_device *dev)
> > > > > > if (IS_GEN7(dev_priv))
> > > > > > I915_WRITE(GEN7_ERR_INT, 0xffffffff);
> > > > > >
> > > > > > + if (IS_HASWELL(dev_priv)) {
> > > > > > + I915_WRITE(EDP_PSR_IMR, 0xffffffff);
> > > > > > + I915_WRITE(EDP_PSR_IIR, 0xffffffff);
> > > > > > + }
> > > > > > +
> > > > > > gen5_gt_irq_reset(dev_priv);
> > > > > >
> > > > > > ibx_irq_reset(dev_priv);
> > > > > > @@ -3663,6 +3691,12 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> > > > > > DE_DP_A_HOTPLUG);
> > > > > > }
> > > > > >
> > > > > > + if (IS_HASWELL(dev_priv)) {
> > > > > > + gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> > > > > > + I915_WRITE(EDP_PSR_IMR, 0);
> > > > > > + display_mask |= DE_EDP_PSR_INT_HSW;
> > > > > > + }
> > > > > > +
> > > > > > dev_priv->irq_mask = ~display_mask;
> > > > > >
> > > > > > ibx_irq_pre_postinstall(dev);
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index 1e000f3004cb..3447f03eac3d 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -3828,6 +3828,13 @@ 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 */
> > > > > > +#define EDP_PSR_IMR _MMIO(0x64834)
> > > > > > +#define EDP_PSR_IIR _MMIO(0x64838)
> > > > > > +#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_DATA(i) _MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
> > > > > >
> > > > > > @@ -6628,6 +6635,7 @@ enum {
> > > > > > #define DE_PCH_EVENT_IVB (1<<28)
> > > > > > #define DE_DP_A_HOTPLUG_IVB (1<<27)
> > > > > > #define DE_AUX_CHANNEL_A_IVB (1<<26)
> > > > > > +#define DE_EDP_PSR_INT_HSW (1<<19)
> > > > > > #define DE_SPRITEC_FLIP_DONE_IVB (1<<14)
> > > > > > #define DE_PLANEC_FLIP_DONE_IVB (1<<13)
> > > > > > #define DE_PIPEC_VBLANK_IVB (1<<10)
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > index 317cb4a12693..27dfd507a4f7 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > @@ -613,7 +613,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > > > > I915_WRITE(EDP_PSR_DEBUG,
> > > > > > EDP_PSR_DEBUG_MASK_MEMUP |
> > > > > > EDP_PSR_DEBUG_MASK_HPD |
> > > > > > - EDP_PSR_DEBUG_MASK_LPSP);
> > > > > > + EDP_PSR_DEBUG_MASK_LPSP |
> > > > > > + EDP_PSR_DEBUG_MASK_REG_WRITE);
> > > > >
> > > > > I don't think Daniel's original had this actually. I think I added it
> > > > > because I didn't want mmio to mess up my testing.
> > > >
> > > > It gets removed in a separate patch (2/5). So I assumed Daniel added it
> > > > and you removed it.
> > >
> > > Hmm. That could indeed be the case.
> > >
> > > >
> > > >
> > > > > In general I'd really
> > > > > prefer to mask everything so that we can be sure that the PSR exits
> > > > > happen when we want/need them and not randomly due to other activity.
> > > > >
> > > > > If any random mmio can cause a PSR exit it's going to hard to prove that
> > > > > we're doing the correct thing. Also seems pretty wasteful if eg. GT
> > > > > interrupts can cause PSR exits for no good reason. I never tested to see
> > > > > which registers cause the PSR exit. That would be good to know.
> > > > >
> > > >
> > > > The same bit on SKL+ is called "Mask Display Reg Write", so it either
> > > > means
> > > > 1) HSW and BDW exit only on display register writes and the bspec
> > > > description is ambiguous or
> > >
> > > The problem is that eg. some of the interrupt registers live in the
> > > display block. Hence any interrupt could potentially cause the PSR exit.
> > > As could any register read we do for debug purpose etc. So at least I
> > > would feel safer if PSR can be made to work reliably with more or less
> > > everything masked.
> > >
> > The idea is to move towards hardware triggered exits since that's what
> > is necessary for PSR2 selective updates[1]. More importantly, there are
> > places in the driver where we can't manually exit PSR without
> > circumventing locking issues - from vblank enable for instance. Relying
> > on hardware triggered exits in the modeset/commit path keeps it
> > consistent IMO.
>
> What's the difference between the hardware behaviour between "write any
> random mmio to trigger an exit" vs. "write some specific register to
> trigger an exit". I presume there is a way to trigger the exit
> explicitly?
There isn't. :(
This is the main reason why psr_exit was still defined for core platforms
as psr_disable.x
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts.
2018-03-22 9:21 ` Ville Syrjälä
@ 2018-03-22 20:59 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 25+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-22 20:59 UTC (permalink / raw)
To: ville.syrjala; +Cc: Vetter, Daniel, intel-gfx, Vivi, Rodrigo
On Thu, 2018-03-22 at 11:21 +0200, Ville Syrjälä wrote:
> On Thu, Mar 22, 2018 at 01:05:24AM +0000, Pandiyan, Dhinakaran wrote:
> > On Wed, 2018-03-21 at 21:48 +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 20, 2018 at 03:41:51PM -0700, Dhinakaran Pandiyan wrote:
> > > > Timestamps are useful for IGT tests that trigger PSR exit and/or wait for
> > > > PSR entry.
> > > >
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++++
> > > > drivers/gpu/drm/i915/i915_drv.h | 3 +++
> > > > drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++++++--
> > > > 3 files changed, 34 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index 669f3d56054a..d28dc4d8388e 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -2686,6 +2686,18 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> > > > }
> > > > mutex_unlock(&dev_priv->psr.lock);
> > > >
> > > > + if (READ_ONCE(dev_priv->psr.debug)) {
> > > > + unsigned int seq;
> > > > +
> > > > + do {
> > > > + seq = read_seqbegin(&dev_priv->psr.debug_lock);
> > > > + seq_printf(m, "Last attempted entry at: %lld\n",
> > > > + dev_priv->psr.last_entry_attempt);
> > > > + seq_printf(m, "Last exit at: %lld\n",
> > > > + dev_priv->psr.last_exit);
> > > > + } while (read_seqretry(&dev_priv->psr.debug_lock, seq));
> > >
> > > What does the seqlock buy us?
> >
> > Reading debugfs wouldn't block the update inside the irq handler,
> > compared to using a spinlock. We need to serialize the read and update
> > parts, don't we? Otherwise tests might end up reading partial updates.
>
> Hmm. ktime_t is 64 bits so I guess on 32bit systems it could be a slight
> issue. Not sure we should care that much about PSR debug on 32bit though.
> It's a rather unlikely configuration, and at least we don't do that in
> the ci.
>
Another thing is, if two IIR bits are set at the same time. EXIT and
PRE_ENTRY probably won't be though. How about if an earlier interrupt
was not handled? Won't multiple IIR bits be set? Implementing the lock
is slightly future proof and doesn't slow down irq handling.
> >
> > >
> > > > + }
> > > > +
> > > > intel_runtime_pm_put(dev_priv);
> > > > return 0;
> > > > }
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 136fa2267a66..b8170882e1ab 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -609,6 +609,9 @@ struct i915_psr {
> > > > bool alpm;
> > > > bool has_hw_tracking;
> > > > bool debug;
> > > > + ktime_t last_entry_attempt;
> > > > + ktime_t last_exit;
> > > > + seqlock_t debug_lock;
> > > >
> > > > void (*enable_source)(struct intel_dp *,
> > > > const struct intel_crtc_state *);
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 64ecea80438d..a83d95b1b587 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -125,28 +125,44 @@ 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();
> > > > + unsigned long flags = 0;
> > > >
> > > > if (INTEL_GEN(dev_priv) >= 8)
> > > > transcoders |= BIT(TRANSCODER_A) |
> > > > BIT(TRANSCODER_B) |
> > > > BIT(TRANSCODER_C);
> > > >
> > > > + write_seqlock_irqsave(&dev_priv->psr.debug_lock, flags);
> > > > for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> > > > + bool handled = false;
> > > > +
> > > > + /* PSR supported only on one transcoder currently */
> > > > + WARN_ON_ONCE(handled);
> > > > +
> > > > /* FIXME: Exit PSR when this happens. */
> > > > - if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > > > + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > > > + handled = true;
> > > > DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
> > > > transcoder_name(cpu_transcoder));
> > > >
> > > > + }
> > > > +
> > > > if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > > > - DRM_DEBUG_KMS("[transcoder %s] PSR entry in 2 vblanks\n",
> > > > + handled = true;
> > > > + 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)) {
> > > > + handled = true;
> > > > + dev_priv->psr.last_exit = time_ns;
> > > > DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> > > > transcoder_name(cpu_transcoder));
> > > > }
> > > > }
> > > > + write_sequnlock_irqrestore(&dev_priv->psr.debug_lock, flags);
> > > > }
> > > >
> > > > static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > > > @@ -1160,6 +1176,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
> > > >
> > > > INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
> > > > mutex_init(&dev_priv->psr.lock);
> > > > + seqlock_init(&dev_priv->psr.debug_lock);
> > > >
> > > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > > dev_priv->psr.enable_source = vlv_psr_enable_source;
> > > > --
> > > > 2.14.1
> > >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts.
2018-03-20 22:41 ` [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts Dhinakaran Pandiyan
2018-03-21 19:48 ` Ville Syrjälä
@ 2018-03-22 21:08 ` Chris Wilson
2018-03-22 23:55 ` Pandiyan, Dhinakaran
1 sibling, 1 reply; 25+ messages in thread
From: Chris Wilson @ 2018-03-22 21:08 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Dhinakaran Pandiyan, Rodrigo Vivi
Quoting Dhinakaran Pandiyan (2018-03-20 22:41:51)
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 64ecea80438d..a83d95b1b587 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -125,28 +125,44 @@ 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();
> + unsigned long flags = 0;
>
> if (INTEL_GEN(dev_priv) >= 8)
> transcoders |= BIT(TRANSCODER_A) |
> BIT(TRANSCODER_B) |
> BIT(TRANSCODER_C);
>
> + write_seqlock_irqsave(&dev_priv->psr.debug_lock, flags);
You are inside a hardirq handler. irqsave/irqrestore are not required.
Even a seqlock here looks overkill, but whatever. (I don't buy that you
need that precise level of coordination for a slow debugfs interface.)
> for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> + bool handled = false;
> +
> + /* PSR supported only on one transcoder currently */
> + WARN_ON_ONCE(handled);
Now this is just silly. At least get the check right.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts.
2018-03-22 21:08 ` Chris Wilson
@ 2018-03-22 23:55 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 25+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-22 23:55 UTC (permalink / raw)
To: chris; +Cc: Vetter, Daniel, intel-gfx, Vivi, Rodrigo
On Thu, 2018-03-22 at 21:08 +0000, Chris Wilson wrote:
> Quoting Dhinakaran Pandiyan (2018-03-20 22:41:51)
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 64ecea80438d..a83d95b1b587 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -125,28 +125,44 @@ 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();
> > + unsigned long flags = 0;
> >
> > if (INTEL_GEN(dev_priv) >= 8)
> > transcoders |= BIT(TRANSCODER_A) |
> > BIT(TRANSCODER_B) |
> > BIT(TRANSCODER_C);
> >
> > + write_seqlock_irqsave(&dev_priv->psr.debug_lock, flags);
>
> You are inside a hardirq handler. irqsave/irqrestore are not required.
>
> Even a seqlock here looks overkill, but whatever. (I don't buy that you
> need that precise level of coordination for a slow debugfs interface.)
>
Looks like I'll make two reviewers happy without the seqlock, will
remove it in the next version :)
> > for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> > + bool handled = false;
> > +
> > + /* PSR supported only on one transcoder currently */
> > + WARN_ON_ONCE(handled);
>
> Now this is just silly. At least get the check right.
Argh, I should have caught it. Thanks.
> -Chris
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw
2018-03-22 11:33 ` Ville Syrjälä
2018-03-22 20:39 ` Rodrigo Vivi
@ 2018-03-23 0:01 ` Pandiyan, Dhinakaran
1 sibling, 0 replies; 25+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-23 0:01 UTC (permalink / raw)
To: ville.syrjala; +Cc: Vetter, Daniel, daniel.vetter, intel-gfx, Vivi, Rodrigo
On Thu, 2018-03-22 at 13:33 +0200, Ville Syrjälä wrote:
> On Thu, Mar 22, 2018 at 01:19:19AM +0000, Pandiyan, Dhinakaran wrote:
> >
> >
> >
> > On Wed, 2018-03-21 at 21:29 +0200, Ville Syrjälä wrote:
> > > On Wed, Mar 21, 2018 at 07:19:53PM +0000, Pandiyan, Dhinakaran wrote:
> > > >
> > > >
> > > >
> > > > On Wed, 2018-03-21 at 20:59 +0200, Ville Syrjälä wrote:
> > > > > On Tue, Mar 20, 2018 at 03:41:47PM -0700, Dhinakaran Pandiyan wrote:
> > > > > > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > >
> > > > > > The definitions for the error register should be valid on bdw/skl too,
> > > > > > but there we haven't even enabled DE_MISC handling yet.
> > > > > >
> > > > > > Somewhat confusing the the moved register offset on bdw is only for
> > > > > > the _CTL/_AUX register, and that _IIR/IMR stayed where they have been
> > > > > > on bdw.
> > > > > >
> > > > > > v2: Fixes from Ville.
> > > > > >
> > > > > > v3: From DK
> > > > > > * Rebased on drm-tip
> > > > > > * Removed BDW IIR bit definition, looks like an unintentional change that
> > > > > > should be in the following patch.
> > > > > >
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++++++++++++++++++++++
> > > > > > drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++
> > > > > > drivers/gpu/drm/i915/intel_psr.c | 3 ++-
> > > > > > 3 files changed, 44 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > index 44eef355e12c..e94a835b7515 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > @@ -2392,6 +2392,26 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv,
> > > > > > ironlake_rps_change_irq_handler(dev_priv);
> > > > > > }
> > > > > >
> > > > > > +static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv)
> > > > > > +{
> > > > > > + u32 edp_psr_iir = I915_READ(EDP_PSR_IIR);
> > > > > > +
> > > > > > + if (edp_psr_iir & EDP_PSR_ERROR)
> > > > > > + DRM_DEBUG_KMS("PSR error\n");
> > > > > > +
> > > > > > + if (edp_psr_iir & EDP_PSR_PRE_ENTRY) {
> > > > > > + DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n");
> > > > >
> > > > > I doubt we want to have the entry/exit interrupts generate all this
> > > > > noise in dmesg all the time. We should probably only enable the
> > > > > interrupts for testing. The error interrupt I suppose we want always,
> > > > > might even want DRM_ERROR on it.
> > > >
> > > > I implement debugfs control in Patch 4/5, agreed on DRM_ERROR.
> > >
> > > Right. It's a bit hard to read this with stuff getting
> > > added/remove/moved more or less randomly.
> > >
> > > >
> > > > >
> > > > > > + I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY);
> > > > > > + }
> > > > > > +
> > > > > > + if (edp_psr_iir & EDP_PSR_POST_EXIT) {
> > > > > > + DRM_DEBUG_KMS("PSR exit completed\n");
> > > > > > + I915_WRITE(EDP_PSR_IMR, 0);
> > > > > > + }
> > > > > > +
> > > > > > + I915_WRITE(EDP_PSR_IIR, edp_psr_iir);
> > > > > > +}
> > > > > > +
> > > > > > static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > > > > > u32 de_iir)
> > > > > > {
> > > > > > @@ -2404,6 +2424,9 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv,
> > > > > > if (de_iir & DE_ERR_INT_IVB)
> > > > > > ivb_err_int_handler(dev_priv);
> > > > > >
> > > > > > + if (de_iir & DE_EDP_PSR_INT_HSW)
> > > > > > + hsw_edp_psr_irq_handler(dev_priv);
> > > > > > +
> > > > > > if (de_iir & DE_AUX_CHANNEL_A_IVB)
> > > > > > dp_aux_irq_handler(dev_priv);
> > > > > >
> > > > > > @@ -3252,6 +3275,11 @@ static void ironlake_irq_reset(struct drm_device *dev)
> > > > > > if (IS_GEN7(dev_priv))
> > > > > > I915_WRITE(GEN7_ERR_INT, 0xffffffff);
> > > > > >
> > > > > > + if (IS_HASWELL(dev_priv)) {
> > > > > > + I915_WRITE(EDP_PSR_IMR, 0xffffffff);
> > > > > > + I915_WRITE(EDP_PSR_IIR, 0xffffffff);
> > > > > > + }
> > > > > > +
> > > > > > gen5_gt_irq_reset(dev_priv);
> > > > > >
> > > > > > ibx_irq_reset(dev_priv);
> > > > > > @@ -3663,6 +3691,12 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> > > > > > DE_DP_A_HOTPLUG);
> > > > > > }
> > > > > >
> > > > > > + if (IS_HASWELL(dev_priv)) {
> > > > > > + gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR);
> > > > > > + I915_WRITE(EDP_PSR_IMR, 0);
> > > > > > + display_mask |= DE_EDP_PSR_INT_HSW;
> > > > > > + }
> > > > > > +
> > > > > > dev_priv->irq_mask = ~display_mask;
> > > > > >
> > > > > > ibx_irq_pre_postinstall(dev);
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index 1e000f3004cb..3447f03eac3d 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -3828,6 +3828,13 @@ 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 */
> > > > > > +#define EDP_PSR_IMR _MMIO(0x64834)
> > > > > > +#define EDP_PSR_IIR _MMIO(0x64838)
> > > > > > +#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_DATA(i) _MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
> > > > > >
> > > > > > @@ -6628,6 +6635,7 @@ enum {
> > > > > > #define DE_PCH_EVENT_IVB (1<<28)
> > > > > > #define DE_DP_A_HOTPLUG_IVB (1<<27)
> > > > > > #define DE_AUX_CHANNEL_A_IVB (1<<26)
> > > > > > +#define DE_EDP_PSR_INT_HSW (1<<19)
> > > > > > #define DE_SPRITEC_FLIP_DONE_IVB (1<<14)
> > > > > > #define DE_PLANEC_FLIP_DONE_IVB (1<<13)
> > > > > > #define DE_PIPEC_VBLANK_IVB (1<<10)
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > index 317cb4a12693..27dfd507a4f7 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > @@ -613,7 +613,8 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > > > > > I915_WRITE(EDP_PSR_DEBUG,
> > > > > > EDP_PSR_DEBUG_MASK_MEMUP |
> > > > > > EDP_PSR_DEBUG_MASK_HPD |
> > > > > > - EDP_PSR_DEBUG_MASK_LPSP);
> > > > > > + EDP_PSR_DEBUG_MASK_LPSP |
> > > > > > + EDP_PSR_DEBUG_MASK_REG_WRITE);
> > > > >
> > > > > I don't think Daniel's original had this actually. I think I added it
> > > > > because I didn't want mmio to mess up my testing.
> > > >
> > > > It gets removed in a separate patch (2/5). So I assumed Daniel added it
> > > > and you removed it.
> > >
> > > Hmm. That could indeed be the case.
> > >
> > > >
> > > >
> > > > > In general I'd really
> > > > > prefer to mask everything so that we can be sure that the PSR exits
> > > > > happen when we want/need them and not randomly due to other activity.
> > > > >
> > > > > If any random mmio can cause a PSR exit it's going to hard to prove that
> > > > > we're doing the correct thing. Also seems pretty wasteful if eg. GT
> > > > > interrupts can cause PSR exits for no good reason. I never tested to see
> > > > > which registers cause the PSR exit. That would be good to know.
> > > > >
> > > >
> > > > The same bit on SKL+ is called "Mask Display Reg Write", so it either
> > > > means
> > > > 1) HSW and BDW exit only on display register writes and the bspec
> > > > description is ambiguous or
> > >
> > > The problem is that eg. some of the interrupt registers live in the
> > > display block. Hence any interrupt could potentially cause the PSR exit.
> > > As could any register read we do for debug purpose etc. So at least I
> > > would feel safer if PSR can be made to work reliably with more or less
> > > everything masked.
> > >
> > The idea is to move towards hardware triggered exits since that's what
> > is necessary for PSR2 selective updates[1]. More importantly, there are
> > places in the driver where we can't manually exit PSR without
> > circumventing locking issues - from vblank enable for instance. Relying
> > on hardware triggered exits in the modeset/commit path keeps it
> > consistent IMO.
>
> What's the difference between the hardware behaviour between "write any
> random mmio to trigger an exit" vs. "write some specific register to
> trigger an exit". I presume there is a way to trigger the exit
> explicitly?
>
I suppose we could write to the CUR_SURF_LIVE_STATUS (or something like
that) and still mask "Display reg write". Can't tell without testing it
on all of them - HSW, BDW and SKL. There is no change in the mask due to
this series. Change in this patch gets undone in the next. Let's go
ahead with this series and I'll come back to the masks after that.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2018-03-23 0:01 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-20 22:41 [PATCH 0/5] PSR interrupts Dhinakaran Pandiyan
2018-03-20 22:41 ` [PATCH 1/5] drm/i915: Enable edp psr error interrupts on hsw Dhinakaran Pandiyan
2018-03-21 18:59 ` Ville Syrjälä
2018-03-21 19:19 ` Pandiyan, Dhinakaran
2018-03-21 19:29 ` Ville Syrjälä
2018-03-22 1:19 ` Pandiyan, Dhinakaran
2018-03-22 11:33 ` Ville Syrjälä
2018-03-22 20:39 ` Rodrigo Vivi
2018-03-23 0:01 ` Pandiyan, Dhinakaran
2018-03-20 22:41 ` [PATCH 2/5] drm/i915: Drop reg_write from the PSR mask Dhinakaran Pandiyan
2018-03-20 22:41 ` [PATCH 3/5] drm/i915: Enable edp psr error interrupts on bdw+ Dhinakaran Pandiyan
2018-03-20 22:41 ` [PATCH 4/5] drm/i915/psr: Control PSR interrupts via debugfs Dhinakaran Pandiyan
2018-03-21 19:45 ` Ville Syrjälä
2018-03-22 0:59 ` Pandiyan, Dhinakaran
2018-03-20 22:41 ` [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts Dhinakaran Pandiyan
2018-03-21 19:48 ` Ville Syrjälä
2018-03-22 1:05 ` Pandiyan, Dhinakaran
2018-03-22 9:21 ` Ville Syrjälä
2018-03-22 20:59 ` Pandiyan, Dhinakaran
2018-03-22 21:08 ` Chris Wilson
2018-03-22 23:55 ` Pandiyan, Dhinakaran
2018-03-21 8:02 ` ✗ Fi.CI.CHECKPATCH: warning for PSR interrupts Patchwork
2018-03-21 8:03 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-03-21 8:16 ` ✓ Fi.CI.BAT: success " Patchwork
2018-03-21 9:13 ` ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.