[PATCH 1/5] drm/i915: Bail out early on gen6_signal if no semaphores

[PATCH 1/5] drm/i915: Bail out early on gen6_signal if no semaphores

[Mika Kuoppala]

If we dont have semaphores enabled, we allocate 4
dwords for signalling. But end up emitting more regardless.

Fix this by bailing out early if semaphores are not enabled.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78274
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78283
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 3974e82..93b4062 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -684,6 +684,8 @@ static int gen6_signal(struct intel_ring_buffer *signaller,
 #define MBOX_UPDATE_DWORDS 4
 	if (i915_semaphore_is_enabled(dev))
 		num_dwords += ((I915_NUM_RINGS-1) * MBOX_UPDATE_DWORDS);
+	else
+		return intel_ring_begin(signaller, num_dwords);
 
 	ret = intel_ring_begin(signaller, num_dwords);
 	if (ret)
-- 
1.7.9.5

[PATCH 2/5] drm/i915/bdw: Implement a basic PM interrupt handler

[Mika Kuoppala]

From: Ben Widawsky <benjamin.widawsky@intel.com>

Almost all of it is reusable from the existing code. The primary
difference is we need to do even less in the interrupt handler, since
interrupts are not shared in the same way.

The patch is mostly a copy-paste of the existing snb+ code, with updates
to the relevant parts requiring changes to the interrupt handling. As
such it /should/ be relatively trivial. It's highly likely that I missed
some places where I need a gen8 version of the PM interrupts, but it has
become invisible to me by now.

This patch could probably be split into adding the new functions,
followed by actually handling the interrupts. Since the code is
currently disabled (and broken) I think the patch stands better by
itself.

v2: Move the commit about not touching the ringbuffer interrupt to the
snb_* function where it belongs (Rodrigo)

v3: Rebased on Paulo's runtime PM changes

v4: Not well validated, but rebase on
commit 730488b2eddded4497f63f70867b1256cd9e117c
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Fri Mar 7 20:12:32 2014 -0300

    drm/i915: kill dev_priv->pm.regsave

v5: Rebased on latest code base. (Deepak)

v6: Remove conflict markers, Unnecessary empty line and use right
IIR interrupt (Ville)

v7: mask modified without rmw (Ville Syrjälä)

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Deepak S <deepak.s@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c  |   74 ++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_reg.h  |    1 +
 drivers/gpu/drm/i915/intel_drv.h |    2 ++
 drivers/gpu/drm/i915/intel_pm.c  |   38 ++++++++++++++++++--
 4 files changed, 111 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b10fbde..4a88fde 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -248,6 +248,46 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
 	return true;
 }
 
+/**
+  * bdw_update_pm_irq - update GT interrupt 2
+  * @dev_priv: driver private
+  * @interrupt_mask: mask of interrupt bits to update
+  * @enabled_irq_mask: mask of interrupt bits to enable
+  *
+  * Copied from the snb function, updated with relevant register offsets
+  */
+static void bdw_update_pm_irq(struct drm_i915_private *dev_priv,
+			      uint32_t interrupt_mask,
+			      uint32_t enabled_irq_mask)
+{
+	uint32_t new_val;
+
+	assert_spin_locked(&dev_priv->irq_lock);
+
+	if (WARN_ON(dev_priv->pm.irqs_disabled))
+		return;
+
+	new_val = dev_priv->pm_irq_mask;
+	new_val &= ~interrupt_mask;
+	new_val |= (~enabled_irq_mask & interrupt_mask);
+
+	if (new_val != dev_priv->pm_irq_mask) {
+		dev_priv->pm_irq_mask = new_val;
+		I915_WRITE(GEN8_GT_IMR(2), dev_priv->pm_irq_mask);
+		POSTING_READ(GEN8_GT_IMR(2));
+	}
+}
+
+void bdw_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+{
+	bdw_update_pm_irq(dev_priv, mask, mask);
+}
+
+void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+{
+	bdw_update_pm_irq(dev_priv, mask, 0);
+}
+
 static bool cpt_can_enable_serr_int(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1098,8 +1138,12 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	spin_lock_irq(&dev_priv->irq_lock);
 	pm_iir = dev_priv->rps.pm_iir;
 	dev_priv->rps.pm_iir = 0;
-	/* Make sure not to corrupt PMIMR state used by ringbuffer code */
-	snb_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
+	if (IS_BROADWELL(dev_priv->dev))
+		bdw_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
+	else {
+		/* Make sure not to corrupt PMIMR state used by ringbuffer */
+		snb_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
+	}
 	spin_unlock_irq(&dev_priv->irq_lock);
 
 	/* Make sure we didn't queue anything we're not going to process. */
@@ -1296,6 +1340,19 @@ static void snb_gt_irq_handler(struct drm_device *dev,
 		ivybridge_parity_error_irq_handler(dev, gt_iir);
 }
 
+static void gen8_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
+{
+	if ((pm_iir & dev_priv->pm_rps_events) == 0)
+		return;
+
+	spin_lock(&dev_priv->irq_lock);
+	dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events;
+	bdw_disable_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
+	spin_unlock(&dev_priv->irq_lock);
+
+	queue_work(dev_priv->wq, &dev_priv->rps.work);
+}
+
 static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
 				       struct drm_i915_private *dev_priv,
 				       u32 master_ctl)
@@ -1334,6 +1391,17 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
 			DRM_ERROR("The master control interrupt lied (GT1)!\n");
 	}
 
+	if (master_ctl & GEN8_GT_PM_IRQ) {
+		tmp = I915_READ(GEN8_GT_IIR(2));
+		if (tmp & dev_priv->pm_rps_events) {
+			ret = IRQ_HANDLED;
+			gen8_rps_irq_handler(dev_priv, tmp);
+			I915_WRITE(GEN8_GT_IIR(2),
+				   tmp & dev_priv->pm_rps_events);
+		} else
+			DRM_ERROR("The master control interrupt lied (PM)!\n");
+	}
+
 	if (master_ctl & GEN8_GT_VECS_IRQ) {
 		tmp = I915_READ(GEN8_GT_IIR(3));
 		if (tmp) {
@@ -3372,6 +3440,8 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
 
 	for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++)
 		GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]);
+
+	dev_priv->pm_irq_mask = 0xffffffff;
 }
 
 static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 122ed3f..76fdfc2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4378,6 +4378,7 @@ enum punit_power_well {
 #define  GEN8_DE_PIPE_A_IRQ		(1<<16)
 #define  GEN8_DE_PIPE_IRQ(pipe)		(1<<(16+pipe))
 #define  GEN8_GT_VECS_IRQ		(1<<6)
+#define  GEN8_GT_PM_IRQ			(1<<4)
 #define  GEN8_GT_VCS2_IRQ		(1<<3)
 #define  GEN8_GT_VCS1_IRQ		(1<<2)
 #define  GEN8_GT_BCS_IRQ		(1<<1)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 32a74e1..03223dc 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -668,6 +668,8 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
+void bdw_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
+void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
 void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
 int intel_get_crtc_scanline(struct intel_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fb1f806..b804036 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3286,6 +3286,26 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv, val));
 }
 
+static void gen8_disable_rps_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
+	I915_WRITE(GEN8_GT_IER(2), I915_READ(GEN8_GT_IER(2)) &
+				   ~dev_priv->pm_rps_events);
+	/* Complete PM interrupt masking here doesn't race with the rps work
+	 * item again unmasking PM interrupts because that is using a different
+	 * register (GEN8_GT_IMR(2)) to mask PM interrupts. The only risk is in
+	 * leaving stale bits in GEN8_GT_IIR(2) and GEN8_GT_IMR(2) which
+	 * gen8_enable_rps will clean up. */
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	dev_priv->rps.pm_iir = 0;
+	spin_unlock_irq(&dev_priv->irq_lock);
+
+	I915_WRITE(GEN8_GT_IIR(2), dev_priv->pm_rps_events);
+}
+
 static void gen6_disable_rps_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3312,7 +3332,10 @@ static void gen6_disable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC_CONTROL, 0);
 	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
 
-	gen6_disable_rps_interrupts(dev);
+	if (IS_BROADWELL(dev))
+		gen8_disable_rps_interrupts(dev);
+	else
+		gen6_disable_rps_interrupts(dev);
 }
 
 static void valleyview_disable_rps(struct drm_device *dev)
@@ -3384,6 +3407,17 @@ int intel_enable_rc6(const struct drm_device *dev)
 	return i915.enable_rc6;
 }
 
+static void gen8_enable_rps_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	WARN_ON(dev_priv->rps.pm_iir);
+	bdw_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
+	I915_WRITE(GEN8_GT_IIR(2), dev_priv->pm_rps_events);
+	spin_unlock_irq(&dev_priv->irq_lock);
+}
+
 static void gen6_enable_rps_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3486,7 +3520,7 @@ static void gen8_enable_rps(struct drm_device *dev)
 
 	gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) & 0xff00) >> 8);
 
-	gen6_enable_rps_interrupts(dev);
+	gen8_enable_rps_interrupts(dev);
 
 	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
 }
-- 
1.7.9.5

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

[PATCH 3/5] drm/i915: Enable PM Interrupts target via Display Interface.

[Mika Kuoppala]

From: Deepak S <deepak.s@linux.intel.com>

In BDW, Apart from unmasking up/down threshold interrupts. we need
to umask bit 32 of PM_INTRMASK to route interrupts to target via Display
Interface.

v2: Add (1<<31) mask (Ville)

v3: Add Gen check for the mask (ville)

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |    1 +
 drivers/gpu/drm/i915/intel_pm.c |    3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 76fdfc2..ac90786 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5311,6 +5311,7 @@ enum punit_power_well {
 #define VLV_RCEDATA				0xA0BC
 #define GEN6_RC6pp_THRESHOLD			0xA0C0
 #define GEN6_PMINTRMSK				0xA168
+#define GEN8_PMINTR_REDIRECT_TO_NON_DISP	(1<<31)
 #define VLV_PWRDWNUPCTL				0xA294
 
 #define GEN6_PMISR				0x44020
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b804036..36c6473 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3154,6 +3154,9 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
 	if (INTEL_INFO(dev_priv->dev)->gen <= 7 && !IS_HASWELL(dev_priv->dev))
 		mask |= GEN6_PM_RP_UP_EI_EXPIRED;
 
+	if (IS_GEN8(dev_priv->dev))
+		mask |= GEN8_PMINTR_REDIRECT_TO_NON_DISP;
+
 	return ~mask;
 }
 
-- 
1.7.9.5

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

[PATCH 4/5] drm/i915: Fix rc6 options debug info

[Mika Kuoppala]

by correctly displaying result and requested.

Suggested-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 36c6473..36eb1ea 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3390,7 +3390,7 @@ static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
 
 		if ((enable_rc6 & mask) != enable_rc6)
 			DRM_INFO("Adjusting RC6 mask to %d (requested %d, valid %d)\n",
-				 enable_rc6, enable_rc6 & mask, mask);
+				 enable_rc6 & mask, enable_rc6, mask);
 
 		return enable_rc6 & mask;
 	}
-- 
1.7.9.5

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

[PATCH 5/5] drm/i915: Enable rc6 with bdw

[Mika Kuoppala]

Everything should be in place so enable rc6/rps for bdw.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 36eb1ea..34b0766 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3374,10 +3374,6 @@ static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
 	if (INTEL_INFO(dev)->gen == 5 && !IS_IRONLAKE_M(dev))
 		return 0;
 
-	/* Disable RC6 on Broadwell for now */
-	if (IS_BROADWELL(dev))
-		return 0;
-
 	/* Respect the kernel parameter if it is set */
 	if (enable_rc6 >= 0) {
 		int mask;
@@ -4685,7 +4681,7 @@ void intel_disable_gt_powersave(struct drm_device *dev)
 	if (IS_IRONLAKE_M(dev)) {
 		ironlake_disable_drps(dev);
 		ironlake_disable_rc6(dev);
-	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
+	} else if (IS_GEN6(dev) || IS_GEN7(dev) || IS_BROADWELL(dev)) {
 		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
 		cancel_work_sync(&dev_priv->rps.work);
 		mutex_lock(&dev_priv->rps.hw_lock);
@@ -4732,7 +4728,7 @@ void intel_enable_gt_powersave(struct drm_device *dev)
 		ironlake_enable_rc6(dev);
 		intel_init_emon(dev);
 		mutex_unlock(&dev->struct_mutex);
-	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
+	} else if (IS_GEN6(dev) || IS_GEN7(dev) || IS_BROADWELL(dev)) {
 		/*
 		 * PCU communication is slow and this doesn't need to be
 		 * done at any specific time, so do this out of our fast path
-- 
1.7.9.5

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

Re: [PATCH 1/5] drm/i915: Bail out early on gen6_signal if no semaphores

[Ville Syrjälä]

On Thu, May 15, 2014 at 08:58:07PM +0300, Mika Kuoppala wrote:
> If we dont have semaphores enabled, we allocate 4
> dwords for signalling. But end up emitting more regardless.
> 
> Fix this by bailing out early if semaphores are not enabled.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78274
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78283
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 3974e82..93b4062 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -684,6 +684,8 @@ static int gen6_signal(struct intel_ring_buffer *signaller,
>  #define MBOX_UPDATE_DWORDS 4
>  	if (i915_semaphore_is_enabled(dev))
>  		num_dwords += ((I915_NUM_RINGS-1) * MBOX_UPDATE_DWORDS);
> +	else
> +		return intel_ring_begin(signaller, num_dwords);

I guess that's the minimal band aid we can do. Or just add another
i915_semaphore_is_enabled() check with return after the
intel_ring_begin() we already have.

Ben's gen8 semaphore series makes the .signal function pointer optional,
so the check will be in the caller. But extracting just that minimal
part from that patch would be more work.

I don't see any real problem in going with this minimal fix for now, so:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	ret = intel_ring_begin(signaller, num_dwords);
>  	if (ret)
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 5/5] drm/i915: Enable rc6 with bdw

[Daniel Vetter]

On Thu, May 15, 2014 at 08:58:11PM +0300, Mika Kuoppala wrote:
> Everything should be in place so enable rc6/rps for bdw.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Merged all 5 patches, thanks. Now that we have the prerequisites it's time
to give runtime pm on bdw another shot. Can you please test a revert of

commit f033579f7759bfb34c082aacbd19a830f1e587cc
Author: Imre Deak <imre.deak@intel.com>
Date:   Mon Apr 28 12:03:59 2014 +0300

    drm/i915: bdw: fix RC6 enabled status reporting and disable runtime PM

and if it checks out please submit it?

Thanks, Daniel
> ---
>  drivers/gpu/drm/i915/intel_pm.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 36eb1ea..34b0766 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3374,10 +3374,6 @@ static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
>  	if (INTEL_INFO(dev)->gen == 5 && !IS_IRONLAKE_M(dev))
>  		return 0;
>  
> -	/* Disable RC6 on Broadwell for now */
> -	if (IS_BROADWELL(dev))
> -		return 0;
> -
>  	/* Respect the kernel parameter if it is set */
>  	if (enable_rc6 >= 0) {
>  		int mask;
> @@ -4685,7 +4681,7 @@ void intel_disable_gt_powersave(struct drm_device *dev)
>  	if (IS_IRONLAKE_M(dev)) {
>  		ironlake_disable_drps(dev);
>  		ironlake_disable_rc6(dev);
> -	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
> +	} else if (IS_GEN6(dev) || IS_GEN7(dev) || IS_BROADWELL(dev)) {
>  		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
>  		cancel_work_sync(&dev_priv->rps.work);
>  		mutex_lock(&dev_priv->rps.hw_lock);
> @@ -4732,7 +4728,7 @@ void intel_enable_gt_powersave(struct drm_device *dev)
>  		ironlake_enable_rc6(dev);
>  		intel_init_emon(dev);
>  		mutex_unlock(&dev->struct_mutex);
> -	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
> +	} else if (IS_GEN6(dev) || IS_GEN7(dev) || IS_BROADWELL(dev)) {
>  		/*
>  		 * PCU communication is slow and this doesn't need to be
>  		 * done at any specific time, so do this out of our fast path
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915/bdw: Implement a basic PM interrupt handler

[O'Rourke, Tom]

>+static void gen8_disable_rps_interrupts(struct drm_device *dev) {
>+	struct drm_i915_private *dev_priv = dev->dev_private;
>+
>+	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);

[TOR:] Please note that for Broadwell, bit 31 in GEN6_PMINTRMSK is not an interrupt disable bit.
In "drm/i915: Enable PM Interrupts target via	Display Interface." this bit is defined as:
+#define GEN8_PMINTR_REDIRECT_TO_NON_DISP	(1<<31)

Writing this bit here could have unintended consequences.

Thanks,
Tom

>+	I915_WRITE(GEN8_GT_IER(2), I915_READ(GEN8_GT_IER(2)) &
>+				   ~dev_priv->pm_rps_events);
>+	/* Complete PM interrupt masking here doesn't race with the rps work
>+	 * item again unmasking PM interrupts because that is using a different
>+	 * register (GEN8_GT_IMR(2)) to mask PM interrupts. The only risk is in
>+	 * leaving stale bits in GEN8_GT_IIR(2) and GEN8_GT_IMR(2) which
>+	 * gen8_enable_rps will clean up. */
>+
>+	spin_lock_irq(&dev_priv->irq_lock);
>+	dev_priv->rps.pm_iir = 0;
>+	spin_unlock_irq(&dev_priv->irq_lock);
>+
>+	I915_WRITE(GEN8_GT_IIR(2), dev_priv->pm_rps_events); }
>+

Re: [PATCH 2/5] drm/i915/bdw: Implement a basic PM interrupt handler

[Daniel Vetter]

On Fri, May 16, 2014 at 01:38:18AM +0000, O'Rourke, Tom wrote:
> >+static void gen8_disable_rps_interrupts(struct drm_device *dev) {
> >+	struct drm_i915_private *dev_priv = dev->dev_private;
> >+
> >+	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> 
> [TOR:] Please note that for Broadwell, bit 31 in GEN6_PMINTRMSK is not an interrupt disable bit.
> In "drm/i915: Enable PM Interrupts target via	Display Interface." this bit is defined as:
> +#define GEN8_PMINTR_REDIRECT_TO_NON_DISP	(1<<31)
> 
> Writing this bit here could have unintended consequences.

Hm, we seem to unconditionally set this bit on gen8 anyway. Still harmful?
Mika, Ville?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915/bdw: Implement a basic PM interrupt handler

[Ville Syrjälä]

On Fri, May 16, 2014 at 11:09:35AM +0200, Daniel Vetter wrote:
> On Fri, May 16, 2014 at 01:38:18AM +0000, O'Rourke, Tom wrote:
> > >+static void gen8_disable_rps_interrupts(struct drm_device *dev) {
> > >+	struct drm_i915_private *dev_priv = dev->dev_private;
> > >+
> > >+	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> > 
> > [TOR:] Please note that for Broadwell, bit 31 in GEN6_PMINTRMSK is not an interrupt disable bit.
> > In "drm/i915: Enable PM Interrupts target via	Display Interface." this bit is defined as:
> > +#define GEN8_PMINTR_REDIRECT_TO_NON_DISP	(1<<31)
> > 
> > Writing this bit here could have unintended consequences.
> 
> Hm, we seem to unconditionally set this bit on gen8 anyway. Still harmful?
> Mika, Ville?

I was thinking that since we mask all the interrupts anyway it
doesn't really matter which way we set the bit here. But I'm
not actually sure what happens if there's an already pending
interrupt when we flip the bit. That is I have no idea if it
could raise an interrupt on the non-disp side or not.

So leaving the bit as zero here might be the safer option, and
at least it would be more consistent with the rest of the gen8
pm irq code. So if someone wants to make that change, my r-b
will still stand.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/5] drm/i915/bdw: Implement a basic PM interrupt handler

[Daniel Vetter]

On Fri, May 16, 2014 at 12:46:00PM +0300, Ville Syrjälä wrote:
> On Fri, May 16, 2014 at 11:09:35AM +0200, Daniel Vetter wrote:
> > On Fri, May 16, 2014 at 01:38:18AM +0000, O'Rourke, Tom wrote:
> > > >+static void gen8_disable_rps_interrupts(struct drm_device *dev) {
> > > >+	struct drm_i915_private *dev_priv = dev->dev_private;
> > > >+
> > > >+	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> > > 
> > > [TOR:] Please note that for Broadwell, bit 31 in GEN6_PMINTRMSK is not an interrupt disable bit.
> > > In "drm/i915: Enable PM Interrupts target via	Display Interface." this bit is defined as:
> > > +#define GEN8_PMINTR_REDIRECT_TO_NON_DISP	(1<<31)
> > > 
> > > Writing this bit here could have unintended consequences.
> > 
> > Hm, we seem to unconditionally set this bit on gen8 anyway. Still harmful?
> > Mika, Ville?
> 
> I was thinking that since we mask all the interrupts anyway it
> doesn't really matter which way we set the bit here. But I'm
> not actually sure what happens if there's an already pending
> interrupt when we flip the bit. That is I have no idea if it
> could raise an interrupt on the non-disp side or not.
> 
> So leaving the bit as zero here might be the safer option, and
> at least it would be more consistent with the rest of the gen8
> pm irq code. So if someone wants to make that change, my r-b
> will still stand.

Imo better as a follow-up patch with the Bspec quote above in the commit
message.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Be careful with non-disp bit in PMINTRMSK

[Mika Kuoppala]

Bit 31 in GEN6_PMINTRMSK is not an interrupt disable bit with gen8.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 34b0766..b59e8ab 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3293,7 +3293,7 @@ static void gen8_disable_rps_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
+	I915_WRITE(GEN6_PMINTRMSK, ~GEN8_PMINTR_REDIRECT_TO_NON_DISP);
 	I915_WRITE(GEN8_GT_IER(2), I915_READ(GEN8_GT_IER(2)) &
 				   ~dev_priv->pm_rps_events);
 	/* Complete PM interrupt masking here doesn't race with the rps work
-- 
1.7.9.5

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

Re: [PATCH] drm/i915: Be careful with non-disp bit in PMINTRMSK

[Daniel Vetter]

On Fri, May 16, 2014 at 01:44:12PM +0300, Mika Kuoppala wrote:
> Bit 31 in GEN6_PMINTRMSK is not an interrupt disable bit with gen8.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch