[PATCH 00/14] drm/i915: IRQ work for chv mostly

[PATCH 00/14] drm/i915: IRQ work for chv mostly

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

After enabling the pipe-a power well on CHV I noticed that hpd and interrupts
didn't work too well anymore. The reason is the same as on VLV; the power well
kills that stuff. So we need to get CHV to use the vlv display irq management
code. Thise series does that, and there's at least one patch just for VLV and
another one to apply a bit of ocd to the gen8 code.

After this series the CHV interupt code is starting to look somewhat decent, 
mostly just calling a few VLV or gen8 helpers. And stuff actually works even
after the power well has gone off and back on. Obviously we have the same
limitation as VLV in that hpd and whatnot doesn't work while the power well
is off, but I think we've decided not to care about that for now.

Ville Syrjälä (14):
  drm/i915: Apply some ocd for IMR vs. IER order during irq enable
  drm/i915: Use DPINVGTT_STATUS_MASK
  drm/i915: Use gen8_gt_irq_reset() in cherryview_irq_uninstall()
  drm/i915: Drop the extra GEN8_PCU_IIR posting read from
    cherryview_irq_preinstall()
  drm/i915: Use a consistent order between IIR,IER,IMR writes on vlv/chv
  drm/i915: Use GEN5_IRQ_RESET() on vlv/chv
  drm/i915: Call gen5_gt_irq_reset() from valleyview_irq_uninstall()
  drm/i915: Make valleyview_display_irqs_(un)install() work for chv
  drm/i915: Refactor vlv_display_irq_reset()
  drm/i915: Refactor vlv_display_irq_uninstall()
  drm/i914: Refactor vlv_display_irq_postinstall()
  drm/i915: Drop useless VLV_IIR writes from
    vlv_display_irq_postinstall()
  drm/i915: Use vlv display irq setup code for chv
  drm/i915: Reinit display irqs and hpd from chv pipe-a power well

 drivers/gpu/drm/i915/i915_irq.c         | 199 ++++++++++++--------------------
 drivers/gpu/drm/i915/intel_runtime_pm.c |  23 ++++
 2 files changed, 94 insertions(+), 128 deletions(-)

-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/14] drm/i915: Apply some ocd for IMR vs. IER order during irq enable

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When disabling interrupts we do the writes in this order:
IMR,IER,IIR,IIR. But when enabling interrupts we don't do use the
mirrored order, and instead do IIR,IIR,IMR,IER.

I like consistency unless there's a good reason against it, which I
can't think of here, so change the enable order to IIR,IIR,IER,IMR.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a2b013d..98a8d65 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -126,16 +126,16 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 
 #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \
 	GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \
-	I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \
 	I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \
-	POSTING_READ(GEN8_##type##_IER(which)); \
+	I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \
+	POSTING_READ(GEN8_##type##_IMR(which)); \
 } while (0)
 
 #define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \
 	GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \
-	I915_WRITE(type##IMR, (imr_val)); \
 	I915_WRITE(type##IER, (ier_val)); \
-	POSTING_READ(type##IER); \
+	I915_WRITE(type##IMR, (imr_val)); \
+	POSTING_READ(type##IMR); \
 } while (0)
 
 /* For display hotplug interrupt */
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/14] drm/i915: Use DPINVGTT_STATUS_MASK

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Some has given a name for the DPINVGTT status bitmask, so let's use it
instead of the magic number. Looks more like the chv code now.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 98a8d65..e41272d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3122,7 +3122,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 
 	gen5_gt_irq_reset(dev);
 
-	I915_WRITE(DPINVGTT, 0xff);
+	I915_WRITE(DPINVGTT, DPINVGTT_STATUS_MASK);
 
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/14] drm/i915: Use gen8_gt_irq_reset() in cherryview_irq_uninstall()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the hand rolled macros with gen8_gt_irq_reset() and
GEN5_IRQ_RESET() in cherryview_irq_uninstall().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e41272d..1ec4ebb 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3625,33 +3625,9 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(GEN8_MASTER_IRQ, 0);
 	POSTING_READ(GEN8_MASTER_IRQ);
 
-#define GEN8_IRQ_FINI_NDX(type, which)				\
-do {								\
-	I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff);	\
-	I915_WRITE(GEN8_##type##_IER(which), 0);		\
-	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff);	\
-	POSTING_READ(GEN8_##type##_IIR(which));			\
-	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff);	\
-} while (0)
-
-#define GEN8_IRQ_FINI(type)				\
-do {							\
-	I915_WRITE(GEN8_##type##_IMR, 0xffffffff);	\
-	I915_WRITE(GEN8_##type##_IER, 0);		\
-	I915_WRITE(GEN8_##type##_IIR, 0xffffffff);	\
-	POSTING_READ(GEN8_##type##_IIR);		\
-	I915_WRITE(GEN8_##type##_IIR, 0xffffffff);	\
-} while (0)
-
-	GEN8_IRQ_FINI_NDX(GT, 0);
-	GEN8_IRQ_FINI_NDX(GT, 1);
-	GEN8_IRQ_FINI_NDX(GT, 2);
-	GEN8_IRQ_FINI_NDX(GT, 3);
-
-	GEN8_IRQ_FINI(PCU);
+	gen8_gt_irq_reset(dev_priv);
 
-#undef GEN8_IRQ_FINI
-#undef GEN8_IRQ_FINI_NDX
+	GEN5_IRQ_RESET(GEN8_PCU_);
 
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/14] drm/i915: Drop the extra GEN8_PCU_IIR posting read from cherryview_irq_preinstall()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Looks like a leftover POSTING_READ(GEN8_PCU_IIR) in
cherryview_irq_preinstall() from some earlier age. GEN5_IRQ_RESET()
already does the posting read so this changes nothing, so kill it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1ec4ebb..1e4062d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3188,8 +3188,6 @@ static void cherryview_irq_preinstall(struct drm_device *dev)
 
 	GEN5_IRQ_RESET(GEN8_PCU_);
 
-	POSTING_READ(GEN8_PCU_IIR);
-
 	I915_WRITE(DPINVGTT, DPINVGTT_STATUS_MASK_CHV);
 
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/14] drm/i915: Use a consistent order between IIR, IER, IMR writes on vlv/chv

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Follow the same ordering rules for the IIR,IER,IMR writes on vlv/chv
that we do on other gen5+ platforms.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1e4062d..589ae51 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3128,10 +3128,11 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	for_each_pipe(dev_priv, pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
 	I915_WRITE(VLV_IMR, 0xffffffff);
 	I915_WRITE(VLV_IER, 0x0);
-	POSTING_READ(VLV_IER);
+	I915_WRITE(VLV_IIR, 0xffffffff);
+	I915_WRITE(VLV_IIR, 0xffffffff);
+	POSTING_READ(VLV_IIR);
 }
 
 static void gen8_gt_irq_reset(struct drm_i915_private *dev_priv)
@@ -3199,6 +3200,7 @@ static void cherryview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(VLV_IMR, 0xffffffff);
 	I915_WRITE(VLV_IER, 0x0);
 	I915_WRITE(VLV_IIR, 0xffffffff);
+	I915_WRITE(VLV_IIR, 0xffffffff);
 	POSTING_READ(VLV_IIR);
 }
 
@@ -3362,9 +3364,9 @@ static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)
 
 	I915_WRITE(VLV_IIR, iir_mask);
 	I915_WRITE(VLV_IIR, iir_mask);
-	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
 	I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
-	POSTING_READ(VLV_IER);
+	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
+	POSTING_READ(VLV_IMR);
 }
 
 static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
@@ -3377,8 +3379,8 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
 		   I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
 
 	dev_priv->irq_mask |= iir_mask;
-	I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
 	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
+	I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
 	I915_WRITE(VLV_IIR, iir_mask);
 	I915_WRITE(VLV_IIR, iir_mask);
 	POSTING_READ(VLV_IIR);
@@ -3432,10 +3434,11 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	POSTING_READ(PORT_HOTPLUG_EN);
 
-	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
-	I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
 	I915_WRITE(VLV_IIR, 0xffffffff);
-	POSTING_READ(VLV_IER);
+	I915_WRITE(VLV_IIR, 0xffffffff);
+	I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
+	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
+	POSTING_READ(VLV_IMR);
 
 	/* Interrupt setup is already guaranteed to be single-threaded, this is
 	 * just to make the assert_spin_locked check happy. */
@@ -3559,8 +3562,10 @@ static int cherryview_irq_postinstall(struct drm_device *dev)
 	spin_unlock_irq(&dev_priv->irq_lock);
 
 	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
+	I915_WRITE(VLV_IIR, 0xffffffff);
 	I915_WRITE(VLV_IER, enable_mask);
+	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
+	POSTING_READ(VLV_IMR);
 
 	gen8_gt_irq_postinstall(dev_priv);
 
@@ -3606,10 +3611,11 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 
 	dev_priv->irq_mask = 0;
 
-	I915_WRITE(VLV_IIR, 0xffffffff);
 	I915_WRITE(VLV_IMR, 0xffffffff);
 	I915_WRITE(VLV_IER, 0x0);
-	POSTING_READ(VLV_IER);
+	I915_WRITE(VLV_IIR, 0xffffffff);
+	I915_WRITE(VLV_IIR, 0xffffffff);
+	POSTING_READ(VLV_IIR);
 }
 
 static void cherryview_irq_uninstall(struct drm_device *dev)
@@ -3636,6 +3642,7 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(VLV_IMR, 0xffffffff);
 	I915_WRITE(VLV_IER, 0x0);
 	I915_WRITE(VLV_IIR, 0xffffffff);
+	I915_WRITE(VLV_IIR, 0xffffffff);
 	POSTING_READ(VLV_IIR);
 }
 
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/14] drm/i915: Use GEN5_IRQ_RESET() on vlv/chv

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the hand rolled IIR,IER,IMR disable sequences with
GEN5_IRQ_RESET().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 589ae51..c106bba 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3128,11 +3128,8 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 	for_each_pipe(dev_priv, pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
-	I915_WRITE(VLV_IMR, 0xffffffff);
-	I915_WRITE(VLV_IER, 0x0);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	POSTING_READ(VLV_IIR);
+
+	GEN5_IRQ_RESET(VLV_);
 }
 
 static void gen8_gt_irq_reset(struct drm_i915_private *dev_priv)
@@ -3197,11 +3194,7 @@ static void cherryview_irq_preinstall(struct drm_device *dev)
 	for_each_pipe(dev_priv, pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
 
-	I915_WRITE(VLV_IMR, 0xffffffff);
-	I915_WRITE(VLV_IER, 0x0);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	POSTING_READ(VLV_IIR);
+	GEN5_IRQ_RESET(VLV_);
 }
 
 static void ibx_hpd_irq_setup(struct drm_device *dev)
@@ -3611,11 +3604,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 
 	dev_priv->irq_mask = 0;
 
-	I915_WRITE(VLV_IMR, 0xffffffff);
-	I915_WRITE(VLV_IER, 0x0);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	POSTING_READ(VLV_IIR);
+	GEN5_IRQ_RESET(VLV_);
 }
 
 static void cherryview_irq_uninstall(struct drm_device *dev)
@@ -3639,11 +3628,7 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
 	for_each_pipe(dev_priv, pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
 
-	I915_WRITE(VLV_IMR, 0xffffffff);
-	I915_WRITE(VLV_IER, 0x0);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	POSTING_READ(VLV_IIR);
+	GEN5_IRQ_RESET(VLV_);
 }
 
 static void ironlake_irq_uninstall(struct drm_device *dev)
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/14] drm/i915: Call gen5_gt_irq_reset() from valleyview_irq_uninstall()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Looks like we forgot to call gen5_gt_irq_reset() for vlv in the
uninstall phase. Do so.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c106bba..67c046b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3588,6 +3588,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(VLV_MASTER_IER, 0);
 
+	gen5_gt_irq_reset(dev);
+
 	for_each_pipe(dev_priv, pipe)
 		I915_WRITE(PIPESTAT(pipe), 0xffff);
 
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/14] drm/i915: Make valleyview_display_irqs_(un)install() work for chv

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Genralize valleyview_display_irqs_install() and
valleyview_display_irqs_uninstall() enough so that they work on chv.
The only difference to vlv here being the third pipe that chv brings.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 67c046b..50431f6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3335,24 +3335,27 @@ static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)
 {
 	u32 pipestat_mask;
 	u32 iir_mask;
+	enum pipe pipe;
 
 	pipestat_mask = PIPESTAT_INT_STATUS_MASK |
 			PIPE_FIFO_UNDERRUN_STATUS;
 
-	I915_WRITE(PIPESTAT(PIPE_A), pipestat_mask);
-	I915_WRITE(PIPESTAT(PIPE_B), pipestat_mask);
+	for_each_pipe(dev_priv, pipe)
+		I915_WRITE(PIPESTAT(pipe), pipestat_mask);
 	POSTING_READ(PIPESTAT(PIPE_A));
 
 	pipestat_mask = PLANE_FLIP_DONE_INT_STATUS_VLV |
 			PIPE_CRC_DONE_INTERRUPT_STATUS;
 
-	i915_enable_pipestat(dev_priv, PIPE_A, pipestat_mask |
-					       PIPE_GMBUS_INTERRUPT_STATUS);
-	i915_enable_pipestat(dev_priv, PIPE_B, pipestat_mask);
+	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
+	for_each_pipe(dev_priv, pipe)
+		      i915_enable_pipestat(dev_priv, pipe, pipestat_mask);
 
 	iir_mask = I915_DISPLAY_PORT_INTERRUPT |
 		   I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
 		   I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
+	if (IS_CHERRYVIEW(dev_priv))
+		iir_mask |= I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
 	dev_priv->irq_mask &= ~iir_mask;
 
 	I915_WRITE(VLV_IIR, iir_mask);
@@ -3366,10 +3369,13 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
 {
 	u32 pipestat_mask;
 	u32 iir_mask;
+	enum pipe pipe;
 
 	iir_mask = I915_DISPLAY_PORT_INTERRUPT |
 		   I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
 		   I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
+	if (IS_CHERRYVIEW(dev_priv))
+		iir_mask |= I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
 
 	dev_priv->irq_mask |= iir_mask;
 	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
@@ -3381,14 +3387,15 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
 	pipestat_mask = PLANE_FLIP_DONE_INT_STATUS_VLV |
 			PIPE_CRC_DONE_INTERRUPT_STATUS;
 
-	i915_disable_pipestat(dev_priv, PIPE_A, pipestat_mask |
-					        PIPE_GMBUS_INTERRUPT_STATUS);
-	i915_disable_pipestat(dev_priv, PIPE_B, pipestat_mask);
+	i915_disable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
+	for_each_pipe(dev_priv, pipe)
+		i915_disable_pipestat(dev_priv, pipe, pipestat_mask);
 
 	pipestat_mask = PIPESTAT_INT_STATUS_MASK |
 			PIPE_FIFO_UNDERRUN_STATUS;
-	I915_WRITE(PIPESTAT(PIPE_A), pipestat_mask);
-	I915_WRITE(PIPESTAT(PIPE_B), pipestat_mask);
+
+	for_each_pipe(dev_priv, pipe)
+		I915_WRITE(PIPESTAT(pipe), pipestat_mask);
 	POSTING_READ(PIPESTAT(PIPE_A));
 }
 
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/14] drm/i915: Refactor vlv_display_irq_reset()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pull the vlv display irq reset code to a new functions. The aim is to
share the code with chv.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 50431f6..38e57dd 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3105,10 +3105,22 @@ static void ironlake_irq_reset(struct drm_device *dev)
 	ibx_irq_reset(dev);
 }
 
+static void vlv_display_irq_reset(struct drm_i915_private *dev_priv)
+{
+	enum pipe pipe;
+
+	I915_WRITE(PORT_HOTPLUG_EN, 0);
+	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
+
+	for_each_pipe(dev_priv, pipe)
+		I915_WRITE(PIPESTAT(pipe), 0xffff);
+
+	GEN5_IRQ_RESET(VLV_);
+}
+
 static void valleyview_irq_preinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int pipe;
 
 	/* VLV magic */
 	I915_WRITE(VLV_IMR, 0);
@@ -3124,12 +3136,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 
 	I915_WRITE(DPINVGTT, DPINVGTT_STATUS_MASK);
 
-	I915_WRITE(PORT_HOTPLUG_EN, 0);
-	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
-	for_each_pipe(dev_priv, pipe)
-		I915_WRITE(PIPESTAT(pipe), 0xffff);
-
-	GEN5_IRQ_RESET(VLV_);
+	vlv_display_irq_reset(dev_priv);
 }
 
 static void gen8_gt_irq_reset(struct drm_i915_private *dev_priv)
@@ -3177,7 +3184,6 @@ void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv)
 static void cherryview_irq_preinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int pipe;
 
 	I915_WRITE(GEN8_MASTER_IRQ, 0);
 	POSTING_READ(GEN8_MASTER_IRQ);
@@ -3188,13 +3194,7 @@ static void cherryview_irq_preinstall(struct drm_device *dev)
 
 	I915_WRITE(DPINVGTT, DPINVGTT_STATUS_MASK_CHV);
 
-	I915_WRITE(PORT_HOTPLUG_EN, 0);
-	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
-
-	for_each_pipe(dev_priv, pipe)
-		I915_WRITE(PIPESTAT(pipe), 0xffff);
-
-	GEN5_IRQ_RESET(VLV_);
+	vlv_display_irq_reset(dev_priv);
 }
 
 static void ibx_hpd_irq_setup(struct drm_device *dev)
@@ -3588,7 +3588,6 @@ static void gen8_irq_uninstall(struct drm_device *dev)
 static void valleyview_irq_uninstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int pipe;
 
 	if (!dev_priv)
 		return;
@@ -3597,12 +3596,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 
 	gen5_gt_irq_reset(dev);
 
-	for_each_pipe(dev_priv, pipe)
-		I915_WRITE(PIPESTAT(pipe), 0xffff);
-
 	I915_WRITE(HWSTAM, 0xffffffff);
-	I915_WRITE(PORT_HOTPLUG_EN, 0);
-	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
 	/* Interrupt setup is already guaranteed to be single-threaded, this is
 	 * just to make the assert_spin_locked check happy. */
@@ -3611,9 +3605,9 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 		valleyview_display_irqs_uninstall(dev_priv);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	dev_priv->irq_mask = 0;
+	vlv_display_irq_reset(dev_priv);
 
-	GEN5_IRQ_RESET(VLV_);
+	dev_priv->irq_mask = 0;
 }
 
 static void cherryview_irq_uninstall(struct drm_device *dev)
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/14] drm/i915: Refactor vlv_display_irq_uninstall()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pull the vlv display irq uninstall code into a separate function, for
eventual sharing with chv.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 38e57dd..b05dee5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3585,6 +3585,20 @@ static void gen8_irq_uninstall(struct drm_device *dev)
 	gen8_irq_reset(dev);
 }
 
+static void vlv_display_irq_uninstall(struct drm_i915_private *dev_priv)
+{
+	/* Interrupt setup is already guaranteed to be single-threaded, this is
+	 * just to make the assert_spin_locked check happy. */
+	spin_lock_irq(&dev_priv->irq_lock);
+	if (dev_priv->display_irqs_enabled)
+		valleyview_display_irqs_uninstall(dev_priv);
+	spin_unlock_irq(&dev_priv->irq_lock);
+
+	vlv_display_irq_reset(dev_priv);
+
+	dev_priv->irq_mask = 0;
+}
+
 static void valleyview_irq_uninstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3598,16 +3612,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xffffffff);
 
-	/* Interrupt setup is already guaranteed to be single-threaded, this is
-	 * just to make the assert_spin_locked check happy. */
-	spin_lock_irq(&dev_priv->irq_lock);
-	if (dev_priv->display_irqs_enabled)
-		valleyview_display_irqs_uninstall(dev_priv);
-	spin_unlock_irq(&dev_priv->irq_lock);
-
-	vlv_display_irq_reset(dev_priv);
-
-	dev_priv->irq_mask = 0;
+	vlv_display_irq_uninstall(dev_priv);
 }
 
 static void cherryview_irq_uninstall(struct drm_device *dev)
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/14] drm/i914: Refactor vlv_display_irq_postinstall()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Split the vlv display irq postinstall code to a separate function so
that we can share it with chv.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b05dee5..6a00e6e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3425,10 +3425,8 @@ void valleyview_disable_display_irqs(struct drm_i915_private *dev_priv)
 		valleyview_display_irqs_uninstall(dev_priv);
 }
 
-static int valleyview_irq_postinstall(struct drm_device *dev)
+static void vlv_display_irq_postinstall(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
 	dev_priv->irq_mask = ~0;
 
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
@@ -3449,6 +3447,13 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 
 	I915_WRITE(VLV_IIR, 0xffffffff);
 	I915_WRITE(VLV_IIR, 0xffffffff);
+}
+
+static int valleyview_irq_postinstall(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	vlv_display_irq_postinstall(dev_priv);
 
 	gen5_gt_irq_postinstall(dev);
 
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/14] drm/i915: Drop useless VLV_IIR writes from vlv_display_irq_postinstall()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The extra VLV_IIR writes at the end of vlv_display_irq_postinstall()
serve no purpose. Remove them.

The VLV_IMR/IER/IIR setup at the start of the function also seems a bit
pointless since it doesn't unmask/enable anything. But leave it be for
now.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6a00e6e..628a129 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3444,9 +3444,6 @@ static void vlv_display_irq_postinstall(struct drm_i915_private *dev_priv)
 	if (dev_priv->display_irqs_enabled)
 		valleyview_display_irqs_install(dev_priv);
 	spin_unlock_irq(&dev_priv->irq_lock);
-
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
 }
 
 static int valleyview_irq_postinstall(struct drm_device *dev)
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 13/14] drm/i915: Use vlv display irq setup code for chv

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Throw away the hand rolled display irq setup code on chv, and instead
just call vlv_display_irq_postinstall() and vlv_display_irq_uninstall().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 37 ++-----------------------------------
 1 file changed, 2 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 628a129..722f73c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3540,34 +3540,8 @@ static int gen8_irq_postinstall(struct drm_device *dev)
 static int cherryview_irq_postinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 enable_mask = I915_DISPLAY_PORT_INTERRUPT |
-		I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
-		I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
-		I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
-	u32 pipestat_enable = PLANE_FLIP_DONE_INT_STATUS_VLV |
-		PIPE_CRC_DONE_INTERRUPT_STATUS;
-	int pipe;
-
-	/*
-	 * Leave vblank interrupts masked initially.  enable/disable will
-	 * toggle them based on usage.
-	 */
-	dev_priv->irq_mask = ~enable_mask;
-
-	for_each_pipe(dev_priv, pipe)
-		I915_WRITE(PIPESTAT(pipe), 0xffff);
-
-	spin_lock_irq(&dev_priv->irq_lock);
-	i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
-	for_each_pipe(dev_priv, pipe)
-		i915_enable_pipestat(dev_priv, pipe, pipestat_enable);
-	spin_unlock_irq(&dev_priv->irq_lock);
 
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IIR, 0xffffffff);
-	I915_WRITE(VLV_IER, enable_mask);
-	I915_WRITE(VLV_IMR, dev_priv->irq_mask);
-	POSTING_READ(VLV_IMR);
+	vlv_display_irq_postinstall(dev_priv);
 
 	gen8_gt_irq_postinstall(dev_priv);
 
@@ -3620,7 +3594,6 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 static void cherryview_irq_uninstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int pipe;
 
 	if (!dev_priv)
 		return;
@@ -3632,13 +3605,7 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
 
 	GEN5_IRQ_RESET(GEN8_PCU_);
 
-	I915_WRITE(PORT_HOTPLUG_EN, 0);
-	I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
-
-	for_each_pipe(dev_priv, pipe)
-		I915_WRITE(PIPESTAT(pipe), 0xffff);
-
-	GEN5_IRQ_RESET(VLV_);
+	vlv_display_irq_uninstall(dev_priv);
 }
 
 static void ironlake_irq_uninstall(struct drm_device *dev)
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 14/14] drm/i915: Reinit display irqs and hpd from chv pipe-a power well

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On chv the pipe-a power well is the new disp2d well, and it kills pretty
much everything in the display block. So we need to do the the same
dance that vlv does wrt. display irqs and hpd when the power well goes
up or down.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index dcbecff..f5a78d5 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -577,6 +577,23 @@ static void chv_pipe_power_well_enable(struct drm_i915_private *dev_priv,
 		     power_well->data != PIPE_C);
 
 	chv_set_pipe_power_well(dev_priv, power_well, true);
+
+	if (power_well->data == PIPE_A) {
+		spin_lock_irq(&dev_priv->irq_lock);
+		valleyview_enable_display_irqs(dev_priv);
+		spin_unlock_irq(&dev_priv->irq_lock);
+
+		/*
+		 * During driver initialization/resume we can avoid restoring the
+		 * part of the HW/SW state that will be inited anyway explicitly.
+		 */
+		if (dev_priv->power_domains.initializing)
+			return;
+
+		intel_hpd_init(dev_priv);
+
+		i915_redisable_vga_power_on(dev_priv->dev);
+	}
 }
 
 static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
@@ -586,6 +603,12 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
 		     power_well->data != PIPE_B &&
 		     power_well->data != PIPE_C);
 
+	if (power_well->data == PIPE_A) {
+		spin_lock_irq(&dev_priv->irq_lock);
+		valleyview_disable_display_irqs(dev_priv);
+		spin_unlock_irq(&dev_priv->irq_lock);
+	}
+
 	chv_set_pipe_power_well(dev_priv, power_well, false);
 
 	if (power_well->data == PIPE_A)
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/14] drm/i915: Apply some ocd for IMR vs. IER order during irq enable

[Paulo Zanoni]

2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When disabling interrupts we do the writes in this order:
> IMR,IER,IIR,IIR. But when enabling interrupts we don't do use the
> mirrored order, and instead do IIR,IIR,IMR,IER.
>
> I like consistency unless there's a good reason against it, which I
> can't think of here, so change the enable order to IIR,IIR,IER,IMR.

I can't think of a reason either, so: Reviewed-by: Paulo Zanoni
<paulo.r.zanoni@intel.com> .

Writing IMR after IER will also eliminate the super-tiny chance that
we'll get an interrupt after writing IMR but not IER, which means we
won't really get the interrupt itself, but still flip IIR :)

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a2b013d..98a8d65 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -126,16 +126,16 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>
>  #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \
>         GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \
> -       I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \
>         I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \
> -       POSTING_READ(GEN8_##type##_IER(which)); \
> +       I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \
> +       POSTING_READ(GEN8_##type##_IMR(which)); \
>  } while (0)
>
>  #define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \
>         GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \
> -       I915_WRITE(type##IMR, (imr_val)); \
>         I915_WRITE(type##IER, (ier_val)); \
> -       POSTING_READ(type##IER); \
> +       I915_WRITE(type##IMR, (imr_val)); \
> +       POSTING_READ(type##IMR); \
>  } while (0)
>
>  /* For display hotplug interrupt */
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/14] drm/i915: Use DPINVGTT_STATUS_MASK

[Paulo Zanoni]

2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Some has given a name for the DPINVGTT status bitmask, so let's use it
> instead of the magic number. Looks more like the chv code now.

Notice that valleyview_irq_postinstall() contains a write using the
correct name, but it's under an "#if 0" with a FIXME comment. You
might want to audit that.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 98a8d65..e41272d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3122,7 +3122,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
>
>         gen5_gt_irq_reset(dev);
>
> -       I915_WRITE(DPINVGTT, 0xff);
> +       I915_WRITE(DPINVGTT, DPINVGTT_STATUS_MASK);
>
>         I915_WRITE(PORT_HOTPLUG_EN, 0);
>         I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/14] drm/i915: Use gen8_gt_irq_reset() in cherryview_irq_uninstall()

[Paulo Zanoni]

2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Replace the hand rolled macros with gen8_gt_irq_reset() and
> GEN5_IRQ_RESET() in cherryview_irq_uninstall().
>

I guess that was the result of a rebase?

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 28 ++--------------------------
>  1 file changed, 2 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index e41272d..1ec4ebb 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3625,33 +3625,9 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
>         I915_WRITE(GEN8_MASTER_IRQ, 0);
>         POSTING_READ(GEN8_MASTER_IRQ);
>
> -#define GEN8_IRQ_FINI_NDX(type, which)                         \
> -do {                                                           \
> -       I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff);       \
> -       I915_WRITE(GEN8_##type##_IER(which), 0);                \
> -       I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff);       \
> -       POSTING_READ(GEN8_##type##_IIR(which));                 \
> -       I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff);       \
> -} while (0)
> -
> -#define GEN8_IRQ_FINI(type)                            \
> -do {                                                   \
> -       I915_WRITE(GEN8_##type##_IMR, 0xffffffff);      \
> -       I915_WRITE(GEN8_##type##_IER, 0);               \
> -       I915_WRITE(GEN8_##type##_IIR, 0xffffffff);      \
> -       POSTING_READ(GEN8_##type##_IIR);                \
> -       I915_WRITE(GEN8_##type##_IIR, 0xffffffff);      \
> -} while (0)
> -
> -       GEN8_IRQ_FINI_NDX(GT, 0);
> -       GEN8_IRQ_FINI_NDX(GT, 1);
> -       GEN8_IRQ_FINI_NDX(GT, 2);
> -       GEN8_IRQ_FINI_NDX(GT, 3);
> -
> -       GEN8_IRQ_FINI(PCU);
> +       gen8_gt_irq_reset(dev_priv);
>
> -#undef GEN8_IRQ_FINI
> -#undef GEN8_IRQ_FINI_NDX
> +       GEN5_IRQ_RESET(GEN8_PCU_);
>
>         I915_WRITE(PORT_HOTPLUG_EN, 0);
>         I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/14] drm/i915: Drop the extra GEN8_PCU_IIR posting read from cherryview_irq_preinstall()

[Paulo Zanoni]

2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Looks like a leftover POSTING_READ(GEN8_PCU_IIR) in
> cherryview_irq_preinstall() from some earlier age. GEN5_IRQ_RESET()
> already does the posting read so this changes nothing, so kill it.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1ec4ebb..1e4062d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3188,8 +3188,6 @@ static void cherryview_irq_preinstall(struct drm_device *dev)
>
>         GEN5_IRQ_RESET(GEN8_PCU_);
>
> -       POSTING_READ(GEN8_PCU_IIR);
> -
>         I915_WRITE(DPINVGTT, DPINVGTT_STATUS_MASK_CHV);
>
>         I915_WRITE(PORT_HOTPLUG_EN, 0);
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/14] drm/i915: Use DPINVGTT_STATUS_MASK

[Ville Syrjälä]

On Thu, Oct 30, 2014 at 04:41:36PM -0200, Paulo Zanoni wrote:
> 2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Some has given a name for the DPINVGTT status bitmask, so let's use it
> > instead of the magic number. Looks more like the chv code now.
> 
> Notice that valleyview_irq_postinstall() contains a write using the
> correct name, but it's under an "#if 0" with a FIXME comment. You
> might want to audit that.

Yeah, I did consider just killing that stuff since it doesn't look like
anyone is ever going to do what's required there. But then I decided to
leave it in for now.

> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 98a8d65..e41272d 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3122,7 +3122,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
> >
> >         gen5_gt_irq_reset(dev);
> >
> > -       I915_WRITE(DPINVGTT, 0xff);
> > +       I915_WRITE(DPINVGTT, DPINVGTT_STATUS_MASK);
> >
> >         I915_WRITE(PORT_HOTPLUG_EN, 0);
> >         I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> > --
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/14] drm/i915: Use gen8_gt_irq_reset() in cherryview_irq_uninstall()

[Ville Syrjälä]

On Thu, Oct 30, 2014 at 04:49:21PM -0200, Paulo Zanoni wrote:
> 2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Replace the hand rolled macros with gen8_gt_irq_reset() and
> > GEN5_IRQ_RESET() in cherryview_irq_uninstall().
> >
> 
> I guess that was the result of a rebase?

I originally mostly copy pasted these from the bdw code, and then while
chv was sitting in an internal tree the big irq cleanup(s) happened.
But the chv code kept working even if upstream changed so there was no
major urgency to change it.

> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 28 ++--------------------------
> >  1 file changed, 2 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index e41272d..1ec4ebb 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3625,33 +3625,9 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
> >         I915_WRITE(GEN8_MASTER_IRQ, 0);
> >         POSTING_READ(GEN8_MASTER_IRQ);
> >
> > -#define GEN8_IRQ_FINI_NDX(type, which)                         \
> > -do {                                                           \
> > -       I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff);       \
> > -       I915_WRITE(GEN8_##type##_IER(which), 0);                \
> > -       I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff);       \
> > -       POSTING_READ(GEN8_##type##_IIR(which));                 \
> > -       I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff);       \
> > -} while (0)
> > -
> > -#define GEN8_IRQ_FINI(type)                            \
> > -do {                                                   \
> > -       I915_WRITE(GEN8_##type##_IMR, 0xffffffff);      \
> > -       I915_WRITE(GEN8_##type##_IER, 0);               \
> > -       I915_WRITE(GEN8_##type##_IIR, 0xffffffff);      \
> > -       POSTING_READ(GEN8_##type##_IIR);                \
> > -       I915_WRITE(GEN8_##type##_IIR, 0xffffffff);      \
> > -} while (0)
> > -
> > -       GEN8_IRQ_FINI_NDX(GT, 0);
> > -       GEN8_IRQ_FINI_NDX(GT, 1);
> > -       GEN8_IRQ_FINI_NDX(GT, 2);
> > -       GEN8_IRQ_FINI_NDX(GT, 3);
> > -
> > -       GEN8_IRQ_FINI(PCU);
> > +       gen8_gt_irq_reset(dev_priv);
> >
> > -#undef GEN8_IRQ_FINI
> > -#undef GEN8_IRQ_FINI_NDX
> > +       GEN5_IRQ_RESET(GEN8_PCU_);
> >
> >         I915_WRITE(PORT_HOTPLUG_EN, 0);
> >         I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> > --
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/14] drm/i915: Use a consistent order between IIR, IER, IMR writes on vlv/chv

[Paulo Zanoni]

2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Follow the same ordering rules for the IIR,IER,IMR writes on vlv/chv
> that we do on other gen5+ platforms.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1e4062d..589ae51 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3128,10 +3128,11 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
>         I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>         for_each_pipe(dev_priv, pipe)
>                 I915_WRITE(PIPESTAT(pipe), 0xffff);
> -       I915_WRITE(VLV_IIR, 0xffffffff);
>         I915_WRITE(VLV_IMR, 0xffffffff);
>         I915_WRITE(VLV_IER, 0x0);
> -       POSTING_READ(VLV_IER);
> +       I915_WRITE(VLV_IIR, 0xffffffff);
> +       I915_WRITE(VLV_IIR, 0xffffffff);
> +       POSTING_READ(VLV_IIR);

This is also a "fix" since clearing IIR before IMR doesn't guarantee
us anything. The same applies in many chunks below.


>  }
>
>  static void gen8_gt_irq_reset(struct drm_i915_private *dev_priv)
> @@ -3199,6 +3200,7 @@ static void cherryview_irq_preinstall(struct drm_device *dev)
>         I915_WRITE(VLV_IMR, 0xffffffff);
>         I915_WRITE(VLV_IER, 0x0);
>         I915_WRITE(VLV_IIR, 0xffffffff);
> +       I915_WRITE(VLV_IIR, 0xffffffff);
>         POSTING_READ(VLV_IIR);
>  }
>
> @@ -3362,9 +3364,9 @@ static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)
>
>         I915_WRITE(VLV_IIR, iir_mask);
>         I915_WRITE(VLV_IIR, iir_mask);
> -       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
>         I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> -       POSTING_READ(VLV_IER);
> +       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> +       POSTING_READ(VLV_IMR);

At this point you should probably just be asserting that IIR should
still be zero, since this seems to run at the postinstall stage, and
we already cleared IIR/IMR/IER at preinstall, so in theory it should
be impossible for IIR to be non-zero. But I see there's also a call
from intel_runtime_pm.c, so I don't know...

The "only check IIR at poinstinstall since we already disabled it at
preinstall" argument is also valid in some chunks below.

Anyway, this commit is already an improvement so if you don't plan to
change anything for now:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>  }
>
>  static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
> @@ -3377,8 +3379,8 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
>                    I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
>
>         dev_priv->irq_mask |= iir_mask;
> -       I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
>         I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> +       I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
>         I915_WRITE(VLV_IIR, iir_mask);
>         I915_WRITE(VLV_IIR, iir_mask);
>         POSTING_READ(VLV_IIR);
> @@ -3432,10 +3434,11 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
>         I915_WRITE(PORT_HOTPLUG_EN, 0);
>         POSTING_READ(PORT_HOTPLUG_EN);
>
> -       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> -       I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
>         I915_WRITE(VLV_IIR, 0xffffffff);
> -       POSTING_READ(VLV_IER);
> +       I915_WRITE(VLV_IIR, 0xffffffff);
> +       I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> +       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> +       POSTING_READ(VLV_IMR);
>
>         /* Interrupt setup is already guaranteed to be single-threaded, this is
>          * just to make the assert_spin_locked check happy. */
> @@ -3559,8 +3562,10 @@ static int cherryview_irq_postinstall(struct drm_device *dev)
>         spin_unlock_irq(&dev_priv->irq_lock);
>
>         I915_WRITE(VLV_IIR, 0xffffffff);
> -       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> +       I915_WRITE(VLV_IIR, 0xffffffff);
>         I915_WRITE(VLV_IER, enable_mask);
> +       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> +       POSTING_READ(VLV_IMR);
>
>         gen8_gt_irq_postinstall(dev_priv);
>
> @@ -3606,10 +3611,11 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>
>         dev_priv->irq_mask = 0;
>
> -       I915_WRITE(VLV_IIR, 0xffffffff);
>         I915_WRITE(VLV_IMR, 0xffffffff);
>         I915_WRITE(VLV_IER, 0x0);
> -       POSTING_READ(VLV_IER);
> +       I915_WRITE(VLV_IIR, 0xffffffff);
> +       I915_WRITE(VLV_IIR, 0xffffffff);
> +       POSTING_READ(VLV_IIR);
>  }
>
>  static void cherryview_irq_uninstall(struct drm_device *dev)
> @@ -3636,6 +3642,7 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
>         I915_WRITE(VLV_IMR, 0xffffffff);
>         I915_WRITE(VLV_IER, 0x0);
>         I915_WRITE(VLV_IIR, 0xffffffff);
> +       I915_WRITE(VLV_IIR, 0xffffffff);
>         POSTING_READ(VLV_IIR);
>  }
>
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/14] drm/i915: Use GEN5_IRQ_RESET() on vlv/chv

[Paulo Zanoni]

2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Replace the hand rolled IIR,IER,IMR disable sequences with
> GEN5_IRQ_RESET().

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 25 +++++--------------------
>  1 file changed, 5 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 589ae51..c106bba 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3128,11 +3128,8 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
>         I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>         for_each_pipe(dev_priv, pipe)
>                 I915_WRITE(PIPESTAT(pipe), 0xffff);
> -       I915_WRITE(VLV_IMR, 0xffffffff);
> -       I915_WRITE(VLV_IER, 0x0);
> -       I915_WRITE(VLV_IIR, 0xffffffff);
> -       I915_WRITE(VLV_IIR, 0xffffffff);
> -       POSTING_READ(VLV_IIR);
> +
> +       GEN5_IRQ_RESET(VLV_);
>  }
>
>  static void gen8_gt_irq_reset(struct drm_i915_private *dev_priv)
> @@ -3197,11 +3194,7 @@ static void cherryview_irq_preinstall(struct drm_device *dev)
>         for_each_pipe(dev_priv, pipe)
>                 I915_WRITE(PIPESTAT(pipe), 0xffff);
>
> -       I915_WRITE(VLV_IMR, 0xffffffff);
> -       I915_WRITE(VLV_IER, 0x0);
> -       I915_WRITE(VLV_IIR, 0xffffffff);
> -       I915_WRITE(VLV_IIR, 0xffffffff);
> -       POSTING_READ(VLV_IIR);
> +       GEN5_IRQ_RESET(VLV_);
>  }
>
>  static void ibx_hpd_irq_setup(struct drm_device *dev)
> @@ -3611,11 +3604,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>
>         dev_priv->irq_mask = 0;
>
> -       I915_WRITE(VLV_IMR, 0xffffffff);
> -       I915_WRITE(VLV_IER, 0x0);
> -       I915_WRITE(VLV_IIR, 0xffffffff);
> -       I915_WRITE(VLV_IIR, 0xffffffff);
> -       POSTING_READ(VLV_IIR);
> +       GEN5_IRQ_RESET(VLV_);
>  }
>
>  static void cherryview_irq_uninstall(struct drm_device *dev)
> @@ -3639,11 +3628,7 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
>         for_each_pipe(dev_priv, pipe)
>                 I915_WRITE(PIPESTAT(pipe), 0xffff);
>
> -       I915_WRITE(VLV_IMR, 0xffffffff);
> -       I915_WRITE(VLV_IER, 0x0);
> -       I915_WRITE(VLV_IIR, 0xffffffff);
> -       I915_WRITE(VLV_IIR, 0xffffffff);
> -       POSTING_READ(VLV_IIR);
> +       GEN5_IRQ_RESET(VLV_);
>  }
>
>  static void ironlake_irq_uninstall(struct drm_device *dev)
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/14] drm/i915: Use a consistent order between IIR, IER, IMR writes on vlv/chv

[Ville Syrjälä]

On Thu, Oct 30, 2014 at 05:24:05PM -0200, Paulo Zanoni wrote:
> 2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Follow the same ordering rules for the IIR,IER,IMR writes on vlv/chv
> > that we do on other gen5+ platforms.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 1e4062d..589ae51 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3128,10 +3128,11 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
> >         I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> >         for_each_pipe(dev_priv, pipe)
> >                 I915_WRITE(PIPESTAT(pipe), 0xffff);
> > -       I915_WRITE(VLV_IIR, 0xffffffff);
> >         I915_WRITE(VLV_IMR, 0xffffffff);
> >         I915_WRITE(VLV_IER, 0x0);
> > -       POSTING_READ(VLV_IER);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> > +       POSTING_READ(VLV_IIR);
> 
> This is also a "fix" since clearing IIR before IMR doesn't guarantee
> us anything. The same applies in many chunks below.
> 
> 
> >  }
> >
> >  static void gen8_gt_irq_reset(struct drm_i915_private *dev_priv)
> > @@ -3199,6 +3200,7 @@ static void cherryview_irq_preinstall(struct drm_device *dev)
> >         I915_WRITE(VLV_IMR, 0xffffffff);
> >         I915_WRITE(VLV_IER, 0x0);
> >         I915_WRITE(VLV_IIR, 0xffffffff);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> >         POSTING_READ(VLV_IIR);
> >  }
> >
> > @@ -3362,9 +3364,9 @@ static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)
> >
> >         I915_WRITE(VLV_IIR, iir_mask);
> >         I915_WRITE(VLV_IIR, iir_mask);
> > -       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> >         I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> > -       POSTING_READ(VLV_IER);
> > +       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > +       POSTING_READ(VLV_IMR);
> 
> At this point you should probably just be asserting that IIR should
> still be zero, since this seems to run at the postinstall stage, and
> we already cleared IIR/IMR/IER at preinstall, so in theory it should
> be impossible for IIR to be non-zero. But I see there's also a call
> from intel_runtime_pm.c, so I don't know...
> 
> The "only check IIR at poinstinstall since we already disabled it at
> preinstall" argument is also valid in some chunks below.
> 
> Anyway, this commit is already an improvement so if you don't plan to
> change anything for now:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Yeah I did eye the INIT() macros lustfully for a while, but decided that
switching over would probably require a bit of extra diligence on my
part. So definitely something I want to try doing at some point, but I
figured I should avoid piling on too many patches in this one series.

> 
> >  }
> >
> >  static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
> > @@ -3377,8 +3379,8 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
> >                    I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
> >
> >         dev_priv->irq_mask |= iir_mask;
> > -       I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> >         I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > +       I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> >         I915_WRITE(VLV_IIR, iir_mask);
> >         I915_WRITE(VLV_IIR, iir_mask);
> >         POSTING_READ(VLV_IIR);
> > @@ -3432,10 +3434,11 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
> >         I915_WRITE(PORT_HOTPLUG_EN, 0);
> >         POSTING_READ(PORT_HOTPLUG_EN);
> >
> > -       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > -       I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> >         I915_WRITE(VLV_IIR, 0xffffffff);
> > -       POSTING_READ(VLV_IER);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> > +       I915_WRITE(VLV_IER, ~dev_priv->irq_mask);
> > +       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > +       POSTING_READ(VLV_IMR);
> >
> >         /* Interrupt setup is already guaranteed to be single-threaded, this is
> >          * just to make the assert_spin_locked check happy. */
> > @@ -3559,8 +3562,10 @@ static int cherryview_irq_postinstall(struct drm_device *dev)
> >         spin_unlock_irq(&dev_priv->irq_lock);
> >
> >         I915_WRITE(VLV_IIR, 0xffffffff);
> > -       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> >         I915_WRITE(VLV_IER, enable_mask);
> > +       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > +       POSTING_READ(VLV_IMR);
> >
> >         gen8_gt_irq_postinstall(dev_priv);
> >
> > @@ -3606,10 +3611,11 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
> >
> >         dev_priv->irq_mask = 0;
> >
> > -       I915_WRITE(VLV_IIR, 0xffffffff);
> >         I915_WRITE(VLV_IMR, 0xffffffff);
> >         I915_WRITE(VLV_IER, 0x0);
> > -       POSTING_READ(VLV_IER);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> > +       POSTING_READ(VLV_IIR);
> >  }
> >
> >  static void cherryview_irq_uninstall(struct drm_device *dev)
> > @@ -3636,6 +3642,7 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
> >         I915_WRITE(VLV_IMR, 0xffffffff);
> >         I915_WRITE(VLV_IER, 0x0);
> >         I915_WRITE(VLV_IIR, 0xffffffff);
> > +       I915_WRITE(VLV_IIR, 0xffffffff);
> >         POSTING_READ(VLV_IIR);
> >  }
> >
> > --
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/14] drm/i915: Call gen5_gt_irq_reset() from valleyview_irq_uninstall()

[Paulo Zanoni]

2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Looks like we forgot to call gen5_gt_irq_reset() for vlv in the
> uninstall phase. Do so.

I also see that valleyview_irq_preinstall() contains 2 writes to GTIIR
just before calling gen5_gt_irq_reset(), which should already clear
GTIIR, and at the right order. On a quick look, none of your later
patches seem to do that, so you could write patch 15/14 for that...

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index c106bba..67c046b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3588,6 +3588,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>
>         I915_WRITE(VLV_MASTER_IER, 0);
>
> +       gen5_gt_irq_reset(dev);
> +
>         for_each_pipe(dev_priv, pipe)
>                 I915_WRITE(PIPESTAT(pipe), 0xffff);
>
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/14] drm/i915: Make valleyview_display_irqs_(un)install() work for chv

[Paulo Zanoni]

2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Genralize valleyview_display_irqs_install() and
> valleyview_display_irqs_uninstall() enough so that they work on chv.
> The only difference to vlv here being the third pipe that chv brings.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 67c046b..50431f6 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3335,24 +3335,27 @@ static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)
>  {
>         u32 pipestat_mask;
>         u32 iir_mask;
> +       enum pipe pipe;
>
>         pipestat_mask = PIPESTAT_INT_STATUS_MASK |
>                         PIPE_FIFO_UNDERRUN_STATUS;
>
> -       I915_WRITE(PIPESTAT(PIPE_A), pipestat_mask);
> -       I915_WRITE(PIPESTAT(PIPE_B), pipestat_mask);
> +       for_each_pipe(dev_priv, pipe)
> +               I915_WRITE(PIPESTAT(pipe), pipestat_mask);
>         POSTING_READ(PIPESTAT(PIPE_A));
>
>         pipestat_mask = PLANE_FLIP_DONE_INT_STATUS_VLV |
>                         PIPE_CRC_DONE_INTERRUPT_STATUS;
>
> -       i915_enable_pipestat(dev_priv, PIPE_A, pipestat_mask |
> -                                              PIPE_GMBUS_INTERRUPT_STATUS);
> -       i915_enable_pipestat(dev_priv, PIPE_B, pipestat_mask);
> +       i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
> +       for_each_pipe(dev_priv, pipe)
> +                     i915_enable_pipestat(dev_priv, pipe, pipestat_mask);

While trying to check for the correctness of the lines above, I
noticed that in __i915_enable_pipestat(), if the enable mask is
already what we want, we won't clear/update the status bits. Is that
correct? Why? Anyway, any problems in that function should be fixed by
a separate patch.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
>         iir_mask = I915_DISPLAY_PORT_INTERRUPT |
>                    I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
>                    I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
> +       if (IS_CHERRYVIEW(dev_priv))
> +               iir_mask |= I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
>         dev_priv->irq_mask &= ~iir_mask;
>
>         I915_WRITE(VLV_IIR, iir_mask);
> @@ -3366,10 +3369,13 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
>  {
>         u32 pipestat_mask;
>         u32 iir_mask;
> +       enum pipe pipe;
>
>         iir_mask = I915_DISPLAY_PORT_INTERRUPT |
>                    I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
>                    I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
> +       if (IS_CHERRYVIEW(dev_priv))
> +               iir_mask |= I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
>
>         dev_priv->irq_mask |= iir_mask;
>         I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> @@ -3381,14 +3387,15 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
>         pipestat_mask = PLANE_FLIP_DONE_INT_STATUS_VLV |
>                         PIPE_CRC_DONE_INTERRUPT_STATUS;
>
> -       i915_disable_pipestat(dev_priv, PIPE_A, pipestat_mask |
> -                                               PIPE_GMBUS_INTERRUPT_STATUS);
> -       i915_disable_pipestat(dev_priv, PIPE_B, pipestat_mask);
> +       i915_disable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
> +       for_each_pipe(dev_priv, pipe)
> +               i915_disable_pipestat(dev_priv, pipe, pipestat_mask);
>
>         pipestat_mask = PIPESTAT_INT_STATUS_MASK |
>                         PIPE_FIFO_UNDERRUN_STATUS;
> -       I915_WRITE(PIPESTAT(PIPE_A), pipestat_mask);
> -       I915_WRITE(PIPESTAT(PIPE_B), pipestat_mask);
> +
> +       for_each_pipe(dev_priv, pipe)
> +               I915_WRITE(PIPESTAT(pipe), pipestat_mask);
>         POSTING_READ(PIPESTAT(PIPE_A));
>  }
>
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/14] drm/i915: Refactor vlv_display_irq_reset()

[Paulo Zanoni]

2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Pull the vlv display irq reset code to a new functions. The aim is to
> share the code with chv.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 40 +++++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 50431f6..38e57dd 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3105,10 +3105,22 @@ static void ironlake_irq_reset(struct drm_device *dev)
>         ibx_irq_reset(dev);
>  }
>
> +static void vlv_display_irq_reset(struct drm_i915_private *dev_priv)
> +{
> +       enum pipe pipe;
> +
> +       I915_WRITE(PORT_HOTPLUG_EN, 0);
> +       I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> +
> +       for_each_pipe(dev_priv, pipe)
> +               I915_WRITE(PIPESTAT(pipe), 0xffff);
> +
> +       GEN5_IRQ_RESET(VLV_);
> +}
> +
>  static void valleyview_irq_preinstall(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       int pipe;
>
>         /* VLV magic */
>         I915_WRITE(VLV_IMR, 0);
> @@ -3124,12 +3136,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
>
>         I915_WRITE(DPINVGTT, DPINVGTT_STATUS_MASK);
>
> -       I915_WRITE(PORT_HOTPLUG_EN, 0);
> -       I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> -       for_each_pipe(dev_priv, pipe)
> -               I915_WRITE(PIPESTAT(pipe), 0xffff);
> -
> -       GEN5_IRQ_RESET(VLV_);
> +       vlv_display_irq_reset(dev_priv);
>  }
>
>  static void gen8_gt_irq_reset(struct drm_i915_private *dev_priv)
> @@ -3177,7 +3184,6 @@ void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv)
>  static void cherryview_irq_preinstall(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       int pipe;
>
>         I915_WRITE(GEN8_MASTER_IRQ, 0);
>         POSTING_READ(GEN8_MASTER_IRQ);
> @@ -3188,13 +3194,7 @@ static void cherryview_irq_preinstall(struct drm_device *dev)
>
>         I915_WRITE(DPINVGTT, DPINVGTT_STATUS_MASK_CHV);
>
> -       I915_WRITE(PORT_HOTPLUG_EN, 0);
> -       I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> -
> -       for_each_pipe(dev_priv, pipe)
> -               I915_WRITE(PIPESTAT(pipe), 0xffff);
> -
> -       GEN5_IRQ_RESET(VLV_);
> +       vlv_display_irq_reset(dev_priv);
>  }
>
>  static void ibx_hpd_irq_setup(struct drm_device *dev)
> @@ -3588,7 +3588,6 @@ static void gen8_irq_uninstall(struct drm_device *dev)
>  static void valleyview_irq_uninstall(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       int pipe;
>
>         if (!dev_priv)
>                 return;
> @@ -3597,12 +3596,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>
>         gen5_gt_irq_reset(dev);
>
> -       for_each_pipe(dev_priv, pipe)
> -               I915_WRITE(PIPESTAT(pipe), 0xffff);
> -
>         I915_WRITE(HWSTAM, 0xffffffff);
> -       I915_WRITE(PORT_HOTPLUG_EN, 0);
> -       I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>
>         /* Interrupt setup is already guaranteed to be single-threaded, this is
>          * just to make the assert_spin_locked check happy. */
> @@ -3611,9 +3605,9 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>                 valleyview_display_irqs_uninstall(dev_priv);
>         spin_unlock_irq(&dev_priv->irq_lock);
>
> -       dev_priv->irq_mask = 0;
> +       vlv_display_irq_reset(dev_priv);
>
> -       GEN5_IRQ_RESET(VLV_);
> +       dev_priv->irq_mask = 0;
>  }
>
>  static void cherryview_irq_uninstall(struct drm_device *dev)
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/14] drm/i915: Refactor vlv_display_irq_uninstall()

[Paulo Zanoni]

2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Pull the vlv display irq uninstall code into a separate function, for
> eventual sharing with chv.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 38e57dd..b05dee5 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3585,6 +3585,20 @@ static void gen8_irq_uninstall(struct drm_device *dev)
>         gen8_irq_reset(dev);
>  }
>
> +static void vlv_display_irq_uninstall(struct drm_i915_private *dev_priv)
> +{
> +       /* Interrupt setup is already guaranteed to be single-threaded, this is
> +        * just to make the assert_spin_locked check happy. */
> +       spin_lock_irq(&dev_priv->irq_lock);
> +       if (dev_priv->display_irqs_enabled)
> +               valleyview_display_irqs_uninstall(dev_priv);
> +       spin_unlock_irq(&dev_priv->irq_lock);

I wonder how much I'll be able to review without needing to understand
why we have the chunk above...

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> +
> +       vlv_display_irq_reset(dev_priv);
> +
> +       dev_priv->irq_mask = 0;
> +}
> +
>  static void valleyview_irq_uninstall(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3598,16 +3612,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>
>         I915_WRITE(HWSTAM, 0xffffffff);
>
> -       /* Interrupt setup is already guaranteed to be single-threaded, this is
> -        * just to make the assert_spin_locked check happy. */
> -       spin_lock_irq(&dev_priv->irq_lock);
> -       if (dev_priv->display_irqs_enabled)
> -               valleyview_display_irqs_uninstall(dev_priv);
> -       spin_unlock_irq(&dev_priv->irq_lock);
> -
> -       vlv_display_irq_reset(dev_priv);
> -
> -       dev_priv->irq_mask = 0;
> +       vlv_display_irq_uninstall(dev_priv);
>  }
>
>  static void cherryview_irq_uninstall(struct drm_device *dev)
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/14] drm/i914: Refactor vlv_display_irq_postinstall()

[Paulo Zanoni]

2014-10-30 15:43 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Split the vlv display irq postinstall code to a separate function so
> that we can share it with chv.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b05dee5..6a00e6e 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3425,10 +3425,8 @@ void valleyview_disable_display_irqs(struct drm_i915_private *dev_priv)
>                 valleyview_display_irqs_uninstall(dev_priv);
>  }
>
> -static int valleyview_irq_postinstall(struct drm_device *dev)
> +static void vlv_display_irq_postinstall(struct drm_i915_private *dev_priv)
>  {
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> -
>         dev_priv->irq_mask = ~0;
>
>         I915_WRITE(PORT_HOTPLUG_EN, 0);
> @@ -3449,6 +3447,13 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
>
>         I915_WRITE(VLV_IIR, 0xffffffff);
>         I915_WRITE(VLV_IIR, 0xffffffff);
> +}
> +
> +static int valleyview_irq_postinstall(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       vlv_display_irq_postinstall(dev_priv);
>
>         gen5_gt_irq_postinstall(dev);
>
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/14] drm/i915: Drop useless VLV_IIR writes from vlv_display_irq_postinstall()

[Paulo Zanoni]

2014-10-30 15:43 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The extra VLV_IIR writes at the end of vlv_display_irq_postinstall()
> serve no purpose. Remove them.
>
> The VLV_IMR/IER/IIR setup at the start of the function also seems a bit
> pointless since it doesn't unmask/enable anything. But leave it be for
> now.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6a00e6e..628a129 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3444,9 +3444,6 @@ static void vlv_display_irq_postinstall(struct drm_i915_private *dev_priv)
>         if (dev_priv->display_irqs_enabled)
>                 valleyview_display_irqs_install(dev_priv);
>         spin_unlock_irq(&dev_priv->irq_lock);
> -
> -       I915_WRITE(VLV_IIR, 0xffffffff);
> -       I915_WRITE(VLV_IIR, 0xffffffff);
>  }
>
>  static int valleyview_irq_postinstall(struct drm_device *dev)
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/14] drm/i915: Refactor vlv_display_irq_uninstall()

[Paulo Zanoni]

2014-10-30 18:22 GMT-02:00 Paulo Zanoni <przanoni@gmail.com>:
> 2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Pull the vlv display irq uninstall code into a separate function, for
>> eventual sharing with chv.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 38e57dd..b05dee5 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -3585,6 +3585,20 @@ static void gen8_irq_uninstall(struct drm_device *dev)
>>         gen8_irq_reset(dev);
>>  }
>>
>> +static void vlv_display_irq_uninstall(struct drm_i915_private *dev_priv)

Actually, now we have vlv_display_irq_uninstall() and
valleyview_display_irqs_uninstall(), which is very confusing. Please
rename something :)

>> +{
>> +       /* Interrupt setup is already guaranteed to be single-threaded, this is
>> +        * just to make the assert_spin_locked check happy. */
>> +       spin_lock_irq(&dev_priv->irq_lock);
>> +       if (dev_priv->display_irqs_enabled)
>> +               valleyview_display_irqs_uninstall(dev_priv);
>> +       spin_unlock_irq(&dev_priv->irq_lock);
>
> I wonder how much I'll be able to review without needing to understand
> why we have the chunk above...
>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
>> +
>> +       vlv_display_irq_reset(dev_priv);
>> +
>> +       dev_priv->irq_mask = 0;
>> +}
>> +
>>  static void valleyview_irq_uninstall(struct drm_device *dev)
>>  {
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -3598,16 +3612,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>>
>>         I915_WRITE(HWSTAM, 0xffffffff);
>>
>> -       /* Interrupt setup is already guaranteed to be single-threaded, this is
>> -        * just to make the assert_spin_locked check happy. */
>> -       spin_lock_irq(&dev_priv->irq_lock);
>> -       if (dev_priv->display_irqs_enabled)
>> -               valleyview_display_irqs_uninstall(dev_priv);
>> -       spin_unlock_irq(&dev_priv->irq_lock);
>> -
>> -       vlv_display_irq_reset(dev_priv);
>> -
>> -       dev_priv->irq_mask = 0;
>> +       vlv_display_irq_uninstall(dev_priv);
>>  }
>>
>>  static void cherryview_irq_uninstall(struct drm_device *dev)
>> --
>> 2.0.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/14] drm/i915: Use vlv display irq setup code for chv

[Paulo Zanoni]

2014-10-30 15:43 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Throw away the hand rolled display irq setup code on chv, and instead
> just call vlv_display_irq_postinstall() and vlv_display_irq_uninstall().
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 37 ++-----------------------------------
>  1 file changed, 2 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 628a129..722f73c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3540,34 +3540,8 @@ static int gen8_irq_postinstall(struct drm_device *dev)
>  static int cherryview_irq_postinstall(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       u32 enable_mask = I915_DISPLAY_PORT_INTERRUPT |
> -               I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> -               I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
> -               I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
> -       u32 pipestat_enable = PLANE_FLIP_DONE_INT_STATUS_VLV |
> -               PIPE_CRC_DONE_INTERRUPT_STATUS;
> -       int pipe;
> -
> -       /*
> -        * Leave vblank interrupts masked initially.  enable/disable will
> -        * toggle them based on usage.
> -        */
> -       dev_priv->irq_mask = ~enable_mask;
> -
> -       for_each_pipe(dev_priv, pipe)
> -               I915_WRITE(PIPESTAT(pipe), 0xffff);
> -
> -       spin_lock_irq(&dev_priv->irq_lock);
> -       i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
> -       for_each_pipe(dev_priv, pipe)
> -               i915_enable_pipestat(dev_priv, pipe, pipestat_enable);
> -       spin_unlock_irq(&dev_priv->irq_lock);
>
> -       I915_WRITE(VLV_IIR, 0xffffffff);
> -       I915_WRITE(VLV_IIR, 0xffffffff);
> -       I915_WRITE(VLV_IER, enable_mask);
> -       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> -       POSTING_READ(VLV_IMR);
> +       vlv_display_irq_postinstall(dev_priv);

This chunk changes the order of stuff a little bit, but seems mostly
equivalent. I'll consider it's ok.

>
>         gen8_gt_irq_postinstall(dev_priv);
>
> @@ -3620,7 +3594,6 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>  static void cherryview_irq_uninstall(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       int pipe;
>
>         if (!dev_priv)
>                 return;
> @@ -3632,13 +3605,7 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
>
>         GEN5_IRQ_RESET(GEN8_PCU_);
>
> -       I915_WRITE(PORT_HOTPLUG_EN, 0);
> -       I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> -
> -       for_each_pipe(dev_priv, pipe)
> -               I915_WRITE(PIPESTAT(pipe), 0xffff);
> -
> -       GEN5_IRQ_RESET(VLV_);
> +       vlv_display_irq_uninstall(dev_priv);

The perfect match for the code you removed seems to be
vlv_display_irq_reset(). Why use vlv_display_irq_uninstall() instead?

>  }
>
>  static void ironlake_irq_uninstall(struct drm_device *dev)
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/14] drm/i915: Call gen5_gt_irq_reset() from valleyview_irq_uninstall()

[Ville Syrjälä]

On Thu, Oct 30, 2014 at 05:51:49PM -0200, Paulo Zanoni wrote:
> 2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Looks like we forgot to call gen5_gt_irq_reset() for vlv in the
> > uninstall phase. Do so.
> 
> I also see that valleyview_irq_preinstall() contains 2 writes to GTIIR
> just before calling gen5_gt_irq_reset(), which should already clear
> GTIIR, and at the right order. On a quick look, none of your later
> patches seem to do that, so you could write patch 15/14 for that...

Yeah, I thought those were part of the "VLV magic" stuff, but apparently
they were just leftovers basically from these three commits:

 commit d18ea1b58a5003eb6fca03aff03c4c01321e6cb1
 Author: Daniel Vetter <daniel.vetter@ffwll.ch>
 Date:   Fri Jul 12 22:43:25 2013 +0200

    drm/i915: unify PM interrupt preinstall sequence

 commit 35079899e78315355d882658ae29bb94a2b6609b
 Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
 Date:   Tue Apr 1 15:37:15 2014 -0300

    drm/i915: add GEN5_IRQ_INIT

 commit 337ba0175f49b2d3a0bcc893f97f539bda831007
 Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
 Date:   Tue Apr 1 15:37:16 2014 -0300

    drm/i915: check if IIR is still zero at postinstall on Gen5+

So I'll whip up another patch to kill them.

I also want kill the "VLV magic" stuff as well, but I think I want to
test that kind of stuff a bit more since there's no explanation
whatsoever for the magic.

> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index c106bba..67c046b 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3588,6 +3588,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
> >
> >         I915_WRITE(VLV_MASTER_IER, 0);
> >
> > +       gen5_gt_irq_reset(dev);
> > +
> >         for_each_pipe(dev_priv, pipe)
> >                 I915_WRITE(PIPESTAT(pipe), 0xffff);
> >
> > --
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/14] drm/i915: Make valleyview_display_irqs_(un)install() work for chv

[Ville Syrjälä]

On Thu, Oct 30, 2014 at 06:12:51PM -0200, Paulo Zanoni wrote:
> 2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Genralize valleyview_display_irqs_install() and
> > valleyview_display_irqs_uninstall() enough so that they work on chv.
> > The only difference to vlv here being the third pipe that chv brings.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++----------
> >  1 file changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 67c046b..50431f6 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3335,24 +3335,27 @@ static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)
> >  {
> >         u32 pipestat_mask;
> >         u32 iir_mask;
> > +       enum pipe pipe;
> >
> >         pipestat_mask = PIPESTAT_INT_STATUS_MASK |
> >                         PIPE_FIFO_UNDERRUN_STATUS;
> >
> > -       I915_WRITE(PIPESTAT(PIPE_A), pipestat_mask);
> > -       I915_WRITE(PIPESTAT(PIPE_B), pipestat_mask);
> > +       for_each_pipe(dev_priv, pipe)
> > +               I915_WRITE(PIPESTAT(pipe), pipestat_mask);
> >         POSTING_READ(PIPESTAT(PIPE_A));
> >
> >         pipestat_mask = PLANE_FLIP_DONE_INT_STATUS_VLV |
> >                         PIPE_CRC_DONE_INTERRUPT_STATUS;
> >
> > -       i915_enable_pipestat(dev_priv, PIPE_A, pipestat_mask |
> > -                                              PIPE_GMBUS_INTERRUPT_STATUS);
> > -       i915_enable_pipestat(dev_priv, PIPE_B, pipestat_mask);
> > +       i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
> > +       for_each_pipe(dev_priv, pipe)
> > +                     i915_enable_pipestat(dev_priv, pipe, pipestat_mask);
> 
> While trying to check for the correctness of the lines above, I
> noticed that in __i915_enable_pipestat(), if the enable mask is
> already what we want, we won't clear/update the status bits. Is that
> correct? Why? Anyway, any problems in that function should be fixed by
> a separate patch.

I think the current behaviour is correct. We don't want to lose
interrupts in case someone enables the same pipestat bit twice. But
obviously then disabling twice doesn't work because we don't refcount
the individual bits. So perhaps we want to print a warning if some of
the bits we're trying to set are already set?

> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> >
> >         iir_mask = I915_DISPLAY_PORT_INTERRUPT |
> >                    I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> >                    I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
> > +       if (IS_CHERRYVIEW(dev_priv))
> > +               iir_mask |= I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
> >         dev_priv->irq_mask &= ~iir_mask;
> >
> >         I915_WRITE(VLV_IIR, iir_mask);
> > @@ -3366,10 +3369,13 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
> >  {
> >         u32 pipestat_mask;
> >         u32 iir_mask;
> > +       enum pipe pipe;
> >
> >         iir_mask = I915_DISPLAY_PORT_INTERRUPT |
> >                    I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> >                    I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
> > +       if (IS_CHERRYVIEW(dev_priv))
> > +               iir_mask |= I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
> >
> >         dev_priv->irq_mask |= iir_mask;
> >         I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > @@ -3381,14 +3387,15 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)
> >         pipestat_mask = PLANE_FLIP_DONE_INT_STATUS_VLV |
> >                         PIPE_CRC_DONE_INTERRUPT_STATUS;
> >
> > -       i915_disable_pipestat(dev_priv, PIPE_A, pipestat_mask |
> > -                                               PIPE_GMBUS_INTERRUPT_STATUS);
> > -       i915_disable_pipestat(dev_priv, PIPE_B, pipestat_mask);
> > +       i915_disable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
> > +       for_each_pipe(dev_priv, pipe)
> > +               i915_disable_pipestat(dev_priv, pipe, pipestat_mask);
> >
> >         pipestat_mask = PIPESTAT_INT_STATUS_MASK |
> >                         PIPE_FIFO_UNDERRUN_STATUS;
> > -       I915_WRITE(PIPESTAT(PIPE_A), pipestat_mask);
> > -       I915_WRITE(PIPESTAT(PIPE_B), pipestat_mask);
> > +
> > +       for_each_pipe(dev_priv, pipe)
> > +               I915_WRITE(PIPESTAT(pipe), pipestat_mask);
> >         POSTING_READ(PIPESTAT(PIPE_A));
> >  }
> >
> > --
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/14] drm/i915: Refactor vlv_display_irq_uninstall()

[Ville Syrjälä]

On Thu, Oct 30, 2014 at 06:37:46PM -0200, Paulo Zanoni wrote:
> 2014-10-30 18:22 GMT-02:00 Paulo Zanoni <przanoni@gmail.com>:
> > 2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Pull the vlv display irq uninstall code into a separate function, for
> >> eventual sharing with chv.
> >>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c | 25 +++++++++++++++----------
> >>  1 file changed, 15 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index 38e57dd..b05dee5 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -3585,6 +3585,20 @@ static void gen8_irq_uninstall(struct drm_device *dev)
> >>         gen8_irq_reset(dev);
> >>  }
> >>
> >> +static void vlv_display_irq_uninstall(struct drm_i915_private *dev_priv)
> 
> Actually, now we have vlv_display_irq_uninstall() and
> valleyview_display_irqs_uninstall(), which is very confusing. Please
> rename something :)

Yeah that did bug me as well. I think I need to come up with another
name for the valleyview_display_irqs_* functions because the other ones
already get their names from the drm interrupt setup split. But I'll
need to give it some thought so I don't make things even worse with a
bad name :)

> 
> >> +{
> >> +       /* Interrupt setup is already guaranteed to be single-threaded, this is
> >> +        * just to make the assert_spin_locked check happy. */
> >> +       spin_lock_irq(&dev_priv->irq_lock);
> >> +       if (dev_priv->display_irqs_enabled)
> >> +               valleyview_display_irqs_uninstall(dev_priv);
> >> +       spin_unlock_irq(&dev_priv->irq_lock);
> >
> > I wonder how much I'll be able to review without needing to understand
> > why we have the chunk above...
> >
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> >> +
> >> +       vlv_display_irq_reset(dev_priv);
> >> +
> >> +       dev_priv->irq_mask = 0;
> >> +}
> >> +
> >>  static void valleyview_irq_uninstall(struct drm_device *dev)
> >>  {
> >>         struct drm_i915_private *dev_priv = dev->dev_private;
> >> @@ -3598,16 +3612,7 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
> >>
> >>         I915_WRITE(HWSTAM, 0xffffffff);
> >>
> >> -       /* Interrupt setup is already guaranteed to be single-threaded, this is
> >> -        * just to make the assert_spin_locked check happy. */
> >> -       spin_lock_irq(&dev_priv->irq_lock);
> >> -       if (dev_priv->display_irqs_enabled)
> >> -               valleyview_display_irqs_uninstall(dev_priv);
> >> -       spin_unlock_irq(&dev_priv->irq_lock);
> >> -
> >> -       vlv_display_irq_reset(dev_priv);
> >> -
> >> -       dev_priv->irq_mask = 0;
> >> +       vlv_display_irq_uninstall(dev_priv);
> >>  }
> >>
> >>  static void cherryview_irq_uninstall(struct drm_device *dev)
> >> --
> >> 2.0.4
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> > --
> > Paulo Zanoni
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/14] drm/i915: Call gen5_gt_irq_reset() from valleyview_irq_uninstall()

[Ville Syrjälä]

On Fri, Oct 31, 2014 at 11:35:52AM +0200, Ville Syrjälä wrote:
> On Thu, Oct 30, 2014 at 05:51:49PM -0200, Paulo Zanoni wrote:
> > 2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Looks like we forgot to call gen5_gt_irq_reset() for vlv in the
> > > uninstall phase. Do so.
> > 
> > I also see that valleyview_irq_preinstall() contains 2 writes to GTIIR
> > just before calling gen5_gt_irq_reset(), which should already clear
> > GTIIR, and at the right order. On a quick look, none of your later
> > patches seem to do that, so you could write patch 15/14 for that...
> 
> Yeah, I thought those were part of the "VLV magic" stuff, but apparently
> they were just leftovers basically from these three commits:
> 
>  commit d18ea1b58a5003eb6fca03aff03c4c01321e6cb1
>  Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>  Date:   Fri Jul 12 22:43:25 2013 +0200
> 
>     drm/i915: unify PM interrupt preinstall sequence
> 
>  commit 35079899e78315355d882658ae29bb94a2b6609b
>  Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
>  Date:   Tue Apr 1 15:37:15 2014 -0300
> 
>     drm/i915: add GEN5_IRQ_INIT
> 
>  commit 337ba0175f49b2d3a0bcc893f97f539bda831007
>  Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
>  Date:   Tue Apr 1 15:37:16 2014 -0300
> 
>     drm/i915: check if IIR is still zero at postinstall on Gen5+

Oh, actually it's this one I think (+Daniel's original commit)

 commit f86f3fb005d0c907285fa8685badcb24ec31ee59
 Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
 Date:   Tue Apr 1 15:37:14 2014 -0300

     drm/i915: properly clear IIR at irq_uninstall on Gen5+


> 
> So I'll whip up another patch to kill them.
> 
> I also want kill the "VLV magic" stuff as well, but I think I want to
> test that kind of stuff a bit more since there's no explanation
> whatsoever for the magic.
> 
> > 
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index c106bba..67c046b 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -3588,6 +3588,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
> > >
> > >         I915_WRITE(VLV_MASTER_IER, 0);
> > >
> > > +       gen5_gt_irq_reset(dev);
> > > +
> > >         for_each_pipe(dev_priv, pipe)
> > >                 I915_WRITE(PIPESTAT(pipe), 0xffff);
> > >
> > > --
> > > 2.0.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> > 
> > -- 
> > Paulo Zanoni
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 15/14] drm/i915: Kill leftover GTIIR writes from valleyview_irq_preinstall()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There are two leftover GTIIR writes in valleyview_irq_preinstall().
Looks like the were originally left behind by:

 commit d18ea1b58a5003eb6fca03aff03c4c01321e6cb1
 Author: Daniel Vetter <daniel.vetter@ffwll.ch>
 Date:   Fri Jul 12 22:43:25 2013 +0200

    drm/i915: unify PM interrupt preinstall sequence

and then the GTIIR reset was added back here:

 commit f86f3fb005d0c907285fa8685badcb24ec31ee59
 Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
 Date:   Tue Apr 1 15:37:14 2014 -0300

    drm/i915: properly clear IIR at irq_uninstall on Gen5+

so we can kill the leftovers from the vlv code.

Cc: Paulo Zanoni <przanoni@gmail.com>
Suggested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 722f73c..3125c3f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3128,10 +3128,6 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(RING_IMR(GEN6_BSD_RING_BASE), 0);
 	I915_WRITE(RING_IMR(BLT_RING_BASE), 0);
 
-	/* and GT */
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
-
 	gen5_gt_irq_reset(dev);
 
 	I915_WRITE(DPINVGTT, DPINVGTT_STATUS_MASK);
-- 
2.0.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/14] drm/i915: Use vlv display irq setup code for chv

[Ville Syrjälä]

On Thu, Oct 30, 2014 at 06:41:11PM -0200, Paulo Zanoni wrote:
> 2014-10-30 15:43 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Throw away the hand rolled display irq setup code on chv, and instead
> > just call vlv_display_irq_postinstall() and vlv_display_irq_uninstall().
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 37 ++-----------------------------------
> >  1 file changed, 2 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 628a129..722f73c 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3540,34 +3540,8 @@ static int gen8_irq_postinstall(struct drm_device *dev)
> >  static int cherryview_irq_postinstall(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -       u32 enable_mask = I915_DISPLAY_PORT_INTERRUPT |
> > -               I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> > -               I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
> > -               I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
> > -       u32 pipestat_enable = PLANE_FLIP_DONE_INT_STATUS_VLV |
> > -               PIPE_CRC_DONE_INTERRUPT_STATUS;
> > -       int pipe;
> > -
> > -       /*
> > -        * Leave vblank interrupts masked initially.  enable/disable will
> > -        * toggle them based on usage.
> > -        */
> > -       dev_priv->irq_mask = ~enable_mask;
> > -
> > -       for_each_pipe(dev_priv, pipe)
> > -               I915_WRITE(PIPESTAT(pipe), 0xffff);
> > -
> > -       spin_lock_irq(&dev_priv->irq_lock);
> > -       i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
> > -       for_each_pipe(dev_priv, pipe)
> > -               i915_enable_pipestat(dev_priv, pipe, pipestat_enable);
> > -       spin_unlock_irq(&dev_priv->irq_lock);
> >
> > -       I915_WRITE(VLV_IIR, 0xffffffff);
> > -       I915_WRITE(VLV_IIR, 0xffffffff);
> > -       I915_WRITE(VLV_IER, enable_mask);
> > -       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
> > -       POSTING_READ(VLV_IMR);
> > +       vlv_display_irq_postinstall(dev_priv);
> 
> This chunk changes the order of stuff a little bit, but seems mostly
> equivalent. I'll consider it's ok.
> 
> >
> >         gen8_gt_irq_postinstall(dev_priv);
> >
> > @@ -3620,7 +3594,6 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
> >  static void cherryview_irq_uninstall(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -       int pipe;
> >
> >         if (!dev_priv)
> >                 return;
> > @@ -3632,13 +3605,7 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
> >
> >         GEN5_IRQ_RESET(GEN8_PCU_);
> >
> > -       I915_WRITE(PORT_HOTPLUG_EN, 0);
> > -       I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> > -
> > -       for_each_pipe(dev_priv, pipe)
> > -               I915_WRITE(PIPESTAT(pipe), 0xffff);
> > -
> > -       GEN5_IRQ_RESET(VLV_);
> > +       vlv_display_irq_uninstall(dev_priv);
> 
> The perfect match for the code you removed seems to be
> vlv_display_irq_reset(). Why use vlv_display_irq_uninstall() instead?

Becasue vlv uses it too :) I suppose the idea is to disable the
interrupts the same way they got enabled for symmetry. So if the display
interrupts got enabled by valleyview_display_irqs_install() we should
uninstall them using valleyview_display_irqs_uninstall(). And also
irq_mask gets zeroed there to accurately reflect the state of the
VLV_IER/IMR registers. But yeah we could just call
vlv_display_irq_reset() and get the same hardware effect.

I'm actually thinking we should drop the .irq_preinstall() call from
intel_runtime_pm_enable_interrupts() since IIRC we don't even
unregister the irq handler itself there, and then add a bit of
irq_mask and whatnot sanity checking to .irq_postinstall().

> 
> >  }
> >
> >  static void ironlake_irq_uninstall(struct drm_device *dev)
> > --
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/14] drm/i915: Call gen5_gt_irq_reset() from valleyview_irq_uninstall()

[Daniel Vetter]

On Fri, Oct 31, 2014 at 11:48:01AM +0200, Ville Syrjälä wrote:
> On Fri, Oct 31, 2014 at 11:35:52AM +0200, Ville Syrjälä wrote:
> > On Thu, Oct 30, 2014 at 05:51:49PM -0200, Paulo Zanoni wrote:
> > > 2014-10-30 15:42 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > Looks like we forgot to call gen5_gt_irq_reset() for vlv in the
> > > > uninstall phase. Do so.
> > > 
> > > I also see that valleyview_irq_preinstall() contains 2 writes to GTIIR
> > > just before calling gen5_gt_irq_reset(), which should already clear
> > > GTIIR, and at the right order. On a quick look, none of your later
> > > patches seem to do that, so you could write patch 15/14 for that...
> > 
> > Yeah, I thought those were part of the "VLV magic" stuff, but apparently
> > they were just leftovers basically from these three commits:
> > 
> >  commit d18ea1b58a5003eb6fca03aff03c4c01321e6cb1
> >  Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> >  Date:   Fri Jul 12 22:43:25 2013 +0200
> > 
> >     drm/i915: unify PM interrupt preinstall sequence
> > 
> >  commit 35079899e78315355d882658ae29bb94a2b6609b
> >  Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >  Date:   Tue Apr 1 15:37:15 2014 -0300
> > 
> >     drm/i915: add GEN5_IRQ_INIT
> > 
> >  commit 337ba0175f49b2d3a0bcc893f97f539bda831007
> >  Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >  Date:   Tue Apr 1 15:37:16 2014 -0300
> > 
> >     drm/i915: check if IIR is still zero at postinstall on Gen5+
> 
> Oh, actually it's this one I think (+Daniel's original commit)
> 
>  commit f86f3fb005d0c907285fa8685badcb24ec31ee59
>  Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
>  Date:   Tue Apr 1 15:37:14 2014 -0300
> 
>      drm/i915: properly clear IIR at irq_uninstall on Gen5+

Well I didn't touch them since they indeed looked like random vlv magic.
But I think we can risk a removal.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/14] drm/i915: Make valleyview_display_irqs_(un)install() work for chv

[Daniel Vetter]

On Fri, Oct 31, 2014 at 11:40:36AM +0200, Ville Syrjälä wrote:
> On Thu, Oct 30, 2014 at 06:12:51PM -0200, Paulo Zanoni wrote:
> > While trying to check for the correctness of the lines above, I
> > noticed that in __i915_enable_pipestat(), if the enable mask is
> > already what we want, we won't clear/update the status bits. Is that
> > correct? Why? Anyway, any problems in that function should be fixed by
> > a separate patch.
> 
> I think the current behaviour is correct. We don't want to lose
> interrupts in case someone enables the same pipestat bit twice. But
> obviously then disabling twice doesn't work because we don't refcount
> the individual bits. So perhaps we want to print a warning if some of
> the bits we're trying to set are already set?

Yeah a warning in case something is disabled/enabled already might be
useful. That's valid for all the various irq mask handling helpers we have
in general though (i.e. gt, display ...).

And I'm pretty sure it'll lead to a massive WARN backtrace fest ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/14] drm/i915: Use vlv display irq setup code for chv

[Daniel Vetter]

On Thu, Oct 30, 2014 at 06:41:11PM -0200, Paulo Zanoni wrote:
> 2014-10-30 15:43 GMT-02:00  <ville.syrjala@linux.intel.com>:
> >
> >         gen8_gt_irq_postinstall(dev_priv);
> >
> > @@ -3620,7 +3594,6 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
> >  static void cherryview_irq_uninstall(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -       int pipe;
> >
> >         if (!dev_priv)
> >                 return;
> > @@ -3632,13 +3605,7 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
> >
> >         GEN5_IRQ_RESET(GEN8_PCU_);
> >
> > -       I915_WRITE(PORT_HOTPLUG_EN, 0);
> > -       I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
> > -
> > -       for_each_pipe(dev_priv, pipe)
> > -               I915_WRITE(PIPESTAT(pipe), 0xffff);
> > -
> > -       GEN5_IRQ_RESET(VLV_);
> > +       vlv_display_irq_uninstall(dev_priv);
> 
> The perfect match for the code you removed seems to be
> vlv_display_irq_reset(). Why use vlv_display_irq_uninstall() instead?

Yeah, if we use _reset we don't have the confusion with shared _uninstall
hooks and can just forgoe that other patch.

Also the general naming scheme is to have split-out functions called
_irq_reset and use them both in _preinstall and _uninstall.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/14] drm/i915: IRQ work for chv mostly

[Daniel Vetter]

On Thu, Oct 30, 2014 at 07:42:49PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> After enabling the pipe-a power well on CHV I noticed that hpd and interrupts
> didn't work too well anymore. The reason is the same as on VLV; the power well
> kills that stuff. So we need to get CHV to use the vlv display irq management
> code. Thise series does that, and there's at least one patch just for VLV and
> another one to apply a bit of ocd to the gen8 code.
> 
> After this series the CHV interupt code is starting to look somewhat decent, 
> mostly just calling a few VLV or gen8 helpers. And stuff actually works even
> after the power well has gone off and back on. Obviously we have the same
> limitation as VLV in that hpd and whatnot doesn't work while the power well
> is off, but I think we've decided not to care about that for now.

Ok, pulled in most of the patches from this series, thanks a lot.

> Ville Syrjälä (14):
>   drm/i915: Apply some ocd for IMR vs. IER order during irq enable
>   drm/i915: Use DPINVGTT_STATUS_MASK
>   drm/i915: Use gen8_gt_irq_reset() in cherryview_irq_uninstall()
>   drm/i915: Drop the extra GEN8_PCU_IIR posting read from
>     cherryview_irq_preinstall()
>   drm/i915: Use a consistent order between IIR,IER,IMR writes on vlv/chv
>   drm/i915: Use GEN5_IRQ_RESET() on vlv/chv
>   drm/i915: Call gen5_gt_irq_reset() from valleyview_irq_uninstall()
>   drm/i915: Make valleyview_display_irqs_(un)install() work for chv
>   drm/i915: Refactor vlv_display_irq_reset()
>   drm/i915: Refactor vlv_display_irq_uninstall()

Except this, I think consens is that we don't need it?

>   drm/i914: Refactor vlv_display_irq_postinstall()
>   drm/i915: Drop useless VLV_IIR writes from
>     vlv_display_irq_postinstall()
>   drm/i915: Use vlv display irq setup code for chv
>   drm/i915: Reinit display irqs and hpd from chv pipe-a power well

And the above two since not yet reviewed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/14] drm/i915: IRQ work for chv mostly

[Ville Syrjälä]

On Mon, Nov 03, 2014 at 05:38:59PM +0100, Daniel Vetter wrote:
> On Thu, Oct 30, 2014 at 07:42:49PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > After enabling the pipe-a power well on CHV I noticed that hpd and interrupts
> > didn't work too well anymore. The reason is the same as on VLV; the power well
> > kills that stuff. So we need to get CHV to use the vlv display irq management
> > code. Thise series does that, and there's at least one patch just for VLV and
> > another one to apply a bit of ocd to the gen8 code.
> > 
> > After this series the CHV interupt code is starting to look somewhat decent, 
> > mostly just calling a few VLV or gen8 helpers. And stuff actually works even
> > after the power well has gone off and back on. Obviously we have the same
> > limitation as VLV in that hpd and whatnot doesn't work while the power well
> > is off, but I think we've decided not to care about that for now.
> 
> Ok, pulled in most of the patches from this series, thanks a lot.
> 
> > Ville Syrjälä (14):
> >   drm/i915: Apply some ocd for IMR vs. IER order during irq enable
> >   drm/i915: Use DPINVGTT_STATUS_MASK
> >   drm/i915: Use gen8_gt_irq_reset() in cherryview_irq_uninstall()
> >   drm/i915: Drop the extra GEN8_PCU_IIR posting read from
> >     cherryview_irq_preinstall()
> >   drm/i915: Use a consistent order between IIR,IER,IMR writes on vlv/chv
> >   drm/i915: Use GEN5_IRQ_RESET() on vlv/chv
> >   drm/i915: Call gen5_gt_irq_reset() from valleyview_irq_uninstall()
> >   drm/i915: Make valleyview_display_irqs_(un)install() work for chv
> >   drm/i915: Refactor vlv_display_irq_reset()
> >   drm/i915: Refactor vlv_display_irq_uninstall()
> 
> Except this, I think consens is that we don't need it?

No, it's needed by the later patches. Trying to replace the vlv/chv
uninstall() hooks with reset() is best left for another series IMO.
And doing that involves more than just reviewing the the display irq
install/uninstall paths. Eg. currently VLV_MASTER_IER handling is
very inconsistent.

> 
> >   drm/i914: Refactor vlv_display_irq_postinstall()
> >   drm/i915: Drop useless VLV_IIR writes from
> >     vlv_display_irq_postinstall()
> >   drm/i915: Use vlv display irq setup code for chv
> >   drm/i915: Reinit display irqs and hpd from chv pipe-a power well
> 
> And the above two since not yet reviewed.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/14] drm/i915: IRQ work for chv mostly

[Daniel Vetter]

On Tue, Nov 4, 2014 at 1:21 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> >   drm/i915: Refactor vlv_display_irq_uninstall()
>>
>> Except this, I think consens is that we don't need it?
>
> No, it's needed by the later patches. Trying to replace the vlv/chv
> uninstall() hooks with reset() is best left for another series IMO.
> And doing that involves more than just reviewing the the display irq
> install/uninstall paths. Eg. currently VLV_MASTER_IER handling is
> very inconsistent.

Well I think we should just open-code it for chv and not do this
extraction. All the complexity here is due to vlv runtime pm, and
somehow I still think that this should work more magically. At least
it should be possible if we start to handle power domains in the
system s/r code instead of just force-enabling them all.

So my idea was to drop this one and rebase the remaining few patches
on top. That way we have a nice reminder that there's still work to
do.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/14] drm/i915: IRQ work for chv mostly

[Ville Syrjälä]

On Tue, Nov 04, 2014 at 01:40:05PM +0100, Daniel Vetter wrote:
> On Tue, Nov 4, 2014 at 1:21 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >> >   drm/i915: Refactor vlv_display_irq_uninstall()
> >>
> >> Except this, I think consens is that we don't need it?
> >
> > No, it's needed by the later patches. Trying to replace the vlv/chv
> > uninstall() hooks with reset() is best left for another series IMO.
> > And doing that involves more than just reviewing the the display irq
> > install/uninstall paths. Eg. currently VLV_MASTER_IER handling is
> > very inconsistent.
> 
> Well I think we should just open-code it for chv and not do this
> extraction.

I don't see what the extraction hurts. If someone manages to unify it
more that's fine, but in the meantime it could at least prevent random
bugs from cropping up in one of the relevant platforms. Wouldn't be the
first time someone forgets about chv.

> All the complexity here is due to vlv runtime pm, and
> somehow I still think that this should work more magically. At least
> it should be possible if we start to handle power domains in the
> system s/r code instead of just force-enabling them all.

Well the same complexity is already there during driver init. So it just
comes from the power wells. Trying to avoid the force enable all thing
is a bit difficult because we can't really reconstruct the required
refcounts until we've read out the entire modeset state.

I guess one option would be to read out the current power well states
and convert that into temporary refcounts that get dropped after the
init is done. That would at least guarantee we don't prematurely turn
off anything that the BIOS already lit up. And as Imre pointed out to me,
we should then definitely add the unclaimed register check for vlv/chv
so that we might catch more easily any missing power well references
in the init path.

> So my idea was to drop this one and rebase the remaining few patches
> on top. That way we have a nice reminder that there's still work to
> do.

Such rebasing sounds like work without benefit to me.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/14] drm/i915: IRQ work for chv mostly

[Daniel Vetter]

On Tue, Nov 04, 2014 at 06:42:16PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 04, 2014 at 01:40:05PM +0100, Daniel Vetter wrote:
> > On Tue, Nov 4, 2014 at 1:21 PM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >> >   drm/i915: Refactor vlv_display_irq_uninstall()
> > >>
> > >> Except this, I think consens is that we don't need it?
> > >
> > > No, it's needed by the later patches. Trying to replace the vlv/chv
> > > uninstall() hooks with reset() is best left for another series IMO.
> > > And doing that involves more than just reviewing the the display irq
> > > install/uninstall paths. Eg. currently VLV_MASTER_IER handling is
> > > very inconsistent.
> > 
> > Well I think we should just open-code it for chv and not do this
> > extraction.
> 
> I don't see what the extraction hurts. If someone manages to unify it
> more that's fine, but in the meantime it could at least prevent random
> bugs from cropping up in one of the relevant platforms. Wouldn't be the
> first time someone forgets about chv.

Well my understanding was that chv doesn't yet have runtime pm and that
runtime pm on vlv is still broken. Hence my reluctance to copypaste code
around which might be part of the problem and not the solution.

> > All the complexity here is due to vlv runtime pm, and
> > somehow I still think that this should work more magically. At least
> > it should be possible if we start to handle power domains in the
> > system s/r code instead of just force-enabling them all.
> 
> Well the same complexity is already there during driver init. So it just
> comes from the power wells. Trying to avoid the force enable all thing
> is a bit difficult because we can't really reconstruct the required
> refcounts until we've read out the entire modeset state.
> 
> I guess one option would be to read out the current power well states
> and convert that into temporary refcounts that get dropped after the
> init is done. That would at least guarantee we don't prematurely turn
> off anything that the BIOS already lit up. And as Imre pointed out to me,
> we should then definitely add the unclaimed register check for vlv/chv
> so that we might catch more easily any missing power well references
> in the init path.

Hm yeah that might work well, both for driver load and init. We already
have the ->is_enabled hook, so we could reconstruct the current mask of
power well refcounts from the bios. At least over s/r we shouldn't have
any lingering power domain refcounts, so then we could just drop the bios
refcounts. Since usually on resume the bios hasn't turned up anything that
would get us exactly what we want, while the code can still be the same
for moduel load.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/14] drm/i915: Use vlv display irq setup code for chv

[Paulo Zanoni]

2014-10-31 8:04 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Thu, Oct 30, 2014 at 06:41:11PM -0200, Paulo Zanoni wrote:
>> 2014-10-30 15:43 GMT-02:00  <ville.syrjala@linux.intel.com>:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Throw away the hand rolled display irq setup code on chv, and instead
>> > just call vlv_display_irq_postinstall() and vlv_display_irq_uninstall().
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_irq.c | 37 ++-----------------------------------
>> >  1 file changed, 2 insertions(+), 35 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> > index 628a129..722f73c 100644
>> > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > @@ -3540,34 +3540,8 @@ static int gen8_irq_postinstall(struct drm_device *dev)
>> >  static int cherryview_irq_postinstall(struct drm_device *dev)
>> >  {
>> >         struct drm_i915_private *dev_priv = dev->dev_private;
>> > -       u32 enable_mask = I915_DISPLAY_PORT_INTERRUPT |
>> > -               I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
>> > -               I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
>> > -               I915_DISPLAY_PIPE_C_EVENT_INTERRUPT;
>> > -       u32 pipestat_enable = PLANE_FLIP_DONE_INT_STATUS_VLV |
>> > -               PIPE_CRC_DONE_INTERRUPT_STATUS;
>> > -       int pipe;
>> > -
>> > -       /*
>> > -        * Leave vblank interrupts masked initially.  enable/disable will
>> > -        * toggle them based on usage.
>> > -        */
>> > -       dev_priv->irq_mask = ~enable_mask;
>> > -
>> > -       for_each_pipe(dev_priv, pipe)
>> > -               I915_WRITE(PIPESTAT(pipe), 0xffff);
>> > -
>> > -       spin_lock_irq(&dev_priv->irq_lock);
>> > -       i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS);
>> > -       for_each_pipe(dev_priv, pipe)
>> > -               i915_enable_pipestat(dev_priv, pipe, pipestat_enable);
>> > -       spin_unlock_irq(&dev_priv->irq_lock);
>> >
>> > -       I915_WRITE(VLV_IIR, 0xffffffff);
>> > -       I915_WRITE(VLV_IIR, 0xffffffff);
>> > -       I915_WRITE(VLV_IER, enable_mask);
>> > -       I915_WRITE(VLV_IMR, dev_priv->irq_mask);
>> > -       POSTING_READ(VLV_IMR);
>> > +       vlv_display_irq_postinstall(dev_priv);
>>
>> This chunk changes the order of stuff a little bit, but seems mostly
>> equivalent. I'll consider it's ok.
>>
>> >
>> >         gen8_gt_irq_postinstall(dev_priv);
>> >
>> > @@ -3620,7 +3594,6 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
>> >  static void cherryview_irq_uninstall(struct drm_device *dev)
>> >  {
>> >         struct drm_i915_private *dev_priv = dev->dev_private;
>> > -       int pipe;
>> >
>> >         if (!dev_priv)
>> >                 return;
>> > @@ -3632,13 +3605,7 @@ static void cherryview_irq_uninstall(struct drm_device *dev)
>> >
>> >         GEN5_IRQ_RESET(GEN8_PCU_);
>> >
>> > -       I915_WRITE(PORT_HOTPLUG_EN, 0);
>> > -       I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>> > -
>> > -       for_each_pipe(dev_priv, pipe)
>> > -               I915_WRITE(PIPESTAT(pipe), 0xffff);
>> > -
>> > -       GEN5_IRQ_RESET(VLV_);
>> > +       vlv_display_irq_uninstall(dev_priv);
>>
>> The perfect match for the code you removed seems to be
>> vlv_display_irq_reset(). Why use vlv_display_irq_uninstall() instead?
>
> Becasue vlv uses it too :) I suppose the idea is to disable the
> interrupts the same way they got enabled for symmetry. So if the display
> interrupts got enabled by valleyview_display_irqs_install() we should
> uninstall them using valleyview_display_irqs_uninstall(). And also
> irq_mask gets zeroed there to accurately reflect the state of the
> VLV_IER/IMR registers. But yeah we could just call
> vlv_display_irq_reset() and get the same hardware effect.
>
> I'm actually thinking we should drop the .irq_preinstall() call from
> intel_runtime_pm_enable_interrupts() since IIRC we don't even
> unregister the irq handler itself there, and then add a bit of
> irq_mask and whatnot sanity checking to .irq_postinstall().

This whole VLV/CHV IRQ enabling is really different form the other
Gens and confusing... Since we're unifying things with the patch, I
guess fixing the confusion will be easier with this one applied...

Needs patch 10 applied. With that: Reviewed-by: Paulo Zanoni
<paulo.r.zanoni@intel.com>

>
>>
>> >  }
>> >
>> >  static void ironlake_irq_uninstall(struct drm_device *dev)
>> > --
>> > 2.0.4
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Paulo Zanoni
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 14/14] drm/i915: Reinit display irqs and hpd from chv pipe-a power well

[Paulo Zanoni]

2014-10-30 15:43 GMT-02:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On chv the pipe-a power well is the new disp2d well, and it kills pretty
> much everything in the display block. So we need to do the the same
> dance that vlv does wrt. display irqs and hpd when the power well goes
> up or down.

I don't have the docs for this, so I have to ask: does it kill *all*
the interrupt bits (like VLV), or does it kill only the pipe A
interrupt bits (like BDW on pipes B/C)?

If it kills everything: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index dcbecff..f5a78d5 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -577,6 +577,23 @@ static void chv_pipe_power_well_enable(struct drm_i915_private *dev_priv,
>                      power_well->data != PIPE_C);
>
>         chv_set_pipe_power_well(dev_priv, power_well, true);
> +
> +       if (power_well->data == PIPE_A) {
> +               spin_lock_irq(&dev_priv->irq_lock);
> +               valleyview_enable_display_irqs(dev_priv);
> +               spin_unlock_irq(&dev_priv->irq_lock);
> +
> +               /*
> +                * During driver initialization/resume we can avoid restoring the
> +                * part of the HW/SW state that will be inited anyway explicitly.
> +                */
> +               if (dev_priv->power_domains.initializing)
> +                       return;
> +
> +               intel_hpd_init(dev_priv);
> +
> +               i915_redisable_vga_power_on(dev_priv->dev);
> +       }
>  }
>
>  static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
> @@ -586,6 +603,12 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
>                      power_well->data != PIPE_B &&
>                      power_well->data != PIPE_C);
>
> +       if (power_well->data == PIPE_A) {
> +               spin_lock_irq(&dev_priv->irq_lock);
> +               valleyview_disable_display_irqs(dev_priv);
> +               spin_unlock_irq(&dev_priv->irq_lock);
> +       }
> +
>         chv_set_pipe_power_well(dev_priv, power_well, false);
>
>         if (power_well->data == PIPE_A)
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 14/14] drm/i915: Reinit display irqs and hpd from chv pipe-a power well

[Ville Syrjälä]

On Fri, Nov 14, 2014 at 03:49:25PM -0200, Paulo Zanoni wrote:
> 2014-10-30 15:43 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > On chv the pipe-a power well is the new disp2d well, and it kills pretty
> > much everything in the display block. So we need to do the the same
> > dance that vlv does wrt. display irqs and hpd when the power well goes
> > up or down.
> 
> I don't have the docs for this, so I have to ask: does it kill *all*
> the interrupt bits (like VLV), or does it kill only the pipe A
> interrupt bits (like BDW on pipes B/C)?
> 
> If it kills everything: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Yeah, everything goes even though it's called the "pipe A" power well.
I think the plan was to have per-pipe wells, but then someone decided that
it's not worth it. For some reason they still hooked it up to the pipe A
power well bits instead of doing the more sensible thing and reusing the
same bits that VLV used for this stuff.

> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index dcbecff..f5a78d5 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -577,6 +577,23 @@ static void chv_pipe_power_well_enable(struct drm_i915_private *dev_priv,
> >                      power_well->data != PIPE_C);
> >
> >         chv_set_pipe_power_well(dev_priv, power_well, true);
> > +
> > +       if (power_well->data == PIPE_A) {
> > +               spin_lock_irq(&dev_priv->irq_lock);
> > +               valleyview_enable_display_irqs(dev_priv);
> > +               spin_unlock_irq(&dev_priv->irq_lock);
> > +
> > +               /*
> > +                * During driver initialization/resume we can avoid restoring the
> > +                * part of the HW/SW state that will be inited anyway explicitly.
> > +                */
> > +               if (dev_priv->power_domains.initializing)
> > +                       return;
> > +
> > +               intel_hpd_init(dev_priv);
> > +
> > +               i915_redisable_vga_power_on(dev_priv->dev);
> > +       }
> >  }
> >
> >  static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
> > @@ -586,6 +603,12 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
> >                      power_well->data != PIPE_B &&
> >                      power_well->data != PIPE_C);
> >
> > +       if (power_well->data == PIPE_A) {
> > +               spin_lock_irq(&dev_priv->irq_lock);
> > +               valleyview_disable_display_irqs(dev_priv);
> > +               spin_unlock_irq(&dev_priv->irq_lock);
> > +       }
> > +
> >         chv_set_pipe_power_well(dev_priv, power_well, false);
> >
> >         if (power_well->data == PIPE_A)
> > --
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 14/14] drm/i915: Reinit display irqs and hpd from chv pipe-a power well

[Daniel Vetter]

On Fri, Nov 14, 2014 at 08:45:05PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 14, 2014 at 03:49:25PM -0200, Paulo Zanoni wrote:
> > 2014-10-30 15:43 GMT-02:00  <ville.syrjala@linux.intel.com>:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > On chv the pipe-a power well is the new disp2d well, and it kills pretty
> > > much everything in the display block. So we need to do the the same
> > > dance that vlv does wrt. display irqs and hpd when the power well goes
> > > up or down.
> > 
> > I don't have the docs for this, so I have to ask: does it kill *all*
> > the interrupt bits (like VLV), or does it kill only the pipe A
> > interrupt bits (like BDW on pipes B/C)?
> > 
> > If it kills everything: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Yeah, everything goes even though it's called the "pipe A" power well.
> I think the plan was to have per-pipe wells, but then someone decided that
> it's not worth it. For some reason they still hooked it up to the pipe A
> power well bits instead of doing the more sensible thing and reusing the
> same bits that VLV used for this stuff.

Ok, I've merged the remaining patches from this series, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx