public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS
@ 2014-11-20 21:01 Imre Deak
  2014-11-20 21:01 ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset Imre Deak
  2014-12-01 16:29 ` [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS Paulo Zanoni
  0 siblings, 2 replies; 13+ messages in thread
From: Imre Deak @ 2014-11-20 21:01 UTC (permalink / raw)
  To: intel-gfx

Atm, igt/gem_reset_stats can trigger the recently added WARN on
left-over PM_IIR bits in gen6_enable_rps_interrupts(). There are two
reasons for this:
1. we call intel_enable_gt_powersave() without a preceeding
   intel_disable_gt_powersave()
2. gen6_disable_rps_interrupts() doesn't mask interrupts in PM_IMR

1. means RPS interrupts will remain enabled and can be serviced during
the HW initialization after a GPU reset. 2. means even if we called
gen6_disable_rps_interrupts() any new RPS interrupt during RPS
initialization would still propagate to PM_IIR too early (though
wouldn't be serviced).

This patch solves the 2. issue by also masking interrupts in PM_IMR, the
following patch fixes 1. getting rid of the WARN. This also makes
intel_enable_gt_powersave() and intel_disable_gt_powersave() more
symmetric.

Since gen6_disable_rps_interrupts() is called during driver loading with
i915 interrupts disabled add a new version of gen6_disable_pm_irq() that
doesn't WARN for this.

Also while at it, get the irq_lock around the whole PM_IMR/IER/IIR
programming sequence and make sure that any queued PM_IIR bit is also
cleared.

The WARN was caught by PRTS after I sent my previous RPS sanitizing
patchset and I could easily reproduce it on HSW. To actually fix it we
also need the next patch.

Reported-by: He, Shuang <shuang.he@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8d169e1..598e9689 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -231,9 +231,6 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
-		return;
-
 	new_val = dev_priv->pm_irq_mask;
 	new_val &= ~interrupt_mask;
 	new_val |= (~enabled_irq_mask & interrupt_mask);
@@ -247,14 +244,26 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
 
 void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
 {
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
+		return;
+
 	snb_update_pm_irq(dev_priv, mask, mask);
 }
 
-void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+static void __gen6_disable_pm_irq(struct drm_i915_private *dev_priv,
+				  uint32_t mask)
 {
 	snb_update_pm_irq(dev_priv, mask, 0);
 }
 
+void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
+{
+	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
+		return;
+
+	__gen6_disable_pm_irq(dev_priv, mask);
+}
+
 void gen6_reset_rps_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -289,16 +298,20 @@ void gen6_disable_rps_interrupts(struct drm_device *dev)
 
 	cancel_work_sync(&dev_priv->rps.work);
 
+	spin_lock_irq(&dev_priv->irq_lock);
+
 	I915_WRITE(GEN6_PMINTRMSK, INTEL_INFO(dev_priv)->gen >= 8 ?
 		   ~GEN8_PMINTR_REDIRECT_TO_NON_DISP : ~0);
+
+	__gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events);
 	I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) &
 				~dev_priv->pm_rps_events);
+	I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
+	I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
 
-	spin_lock_irq(&dev_priv->irq_lock);
 	dev_priv->rps.pm_iir = 0;
+
 	spin_unlock_irq(&dev_priv->irq_lock);
-
-	I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
 }
 
 /**
-- 
1.8.4

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset
  2014-11-20 21:01 [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS Imre Deak
@ 2014-11-20 21:01 ` Imre Deak
  2014-11-22 23:17   ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU shuang.he
                     ` (2 more replies)
  2014-12-01 16:29 ` [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS Paulo Zanoni
  1 sibling, 3 replies; 13+ messages in thread
From: Imre Deak @ 2014-11-20 21:01 UTC (permalink / raw)
  To: intel-gfx

Atm, we don't disable RPS interrupts and related work items before
resetting the GPU. This may interfere with the following GPU
initialization and cause RPS interrupts to show up in PM_IIR too early
before calling gen6_enable_rps_interrupts() (triggering a WARN there).

Solve this by disabling RPS interrupts and flushing any related work
items before resetting the GPU.

Reported-by: He, Shuang <shuang.he@intel.com>
Testcase: igt/gem_reset_stats/ban-render
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |  5 ++++-
 drivers/gpu/drm/i915/intel_pm.c | 14 +++++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1df4079..93f3df8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -810,6 +810,9 @@ int i915_reset(struct drm_device *dev)
 	if (!i915.reset)
 		return 0;
 
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
+		intel_reset_gt_powersave(dev);
+
 	mutex_lock(&dev->struct_mutex);
 
 	i915_gem_reset(dev);
@@ -879,7 +882,7 @@ int i915_reset(struct drm_device *dev)
 		 * of re-init after reset.
 		 */
 		if (INTEL_INFO(dev)->gen > 5)
-			intel_reset_gt_powersave(dev);
+			intel_enable_gt_powersave(dev);
 	} else {
 		mutex_unlock(&dev->struct_mutex);
 	}
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e361014..9417f8f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6316,8 +6316,20 @@ void intel_reset_gt_powersave(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (INTEL_INFO(dev)->gen < 6)
+		return;
+
+	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
+
+	/*
+	 * TODO: disable RPS interrupts on GEN9+ too once RPS support
+	 * is added for it.
+	 */
+	if (INTEL_INFO(dev)->gen < 9)
+		gen6_disable_rps_interrupts(dev);
+
 	dev_priv->rps.enabled = false;
-	intel_enable_gt_powersave(dev);
+
 }
 
 static void ibx_init_clock_gating(struct drm_device *dev)
-- 
1.8.4

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU
  2014-11-20 21:01 ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset Imre Deak
@ 2014-11-22 23:17   ` shuang.he
  2014-11-27 11:08   ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset Imre Deak
  2014-12-01 17:35   ` Paulo Zanoni
  2 siblings, 0 replies; 13+ messages in thread
From: shuang.he @ 2014-11-22 23:17 UTC (permalink / raw)
  To: shuang.he, intel-gfx, imre.deak

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  367/367              367/367
ILK                 -4              373/375              369/375
SNB                                  450/450              450/450
IVB                 -1              502/503              501/503
BYT                                  289/289              289/289
HSW                 -3              567/567              564/567
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
ILK  igt_kms_flip_bcs-flip-vs-modeset-interruptible      PASS(2, M37M26)      DMESG_WARN(1, M26)
ILK  igt_kms_flip_flip-vs-dpms-interruptible      PASS(2, M37M26)      DMESG_WARN(1, M26)
ILK  igt_kms_flip_flip-vs-rmfb-interruptible      PASS(2, M37M26)      DMESG_WARN(1, M26)
ILK  igt_kms_flip_rcs-flip-vs-modeset      DMESG_WARN(1, M26)PASS(1, M37)      DMESG_WARN(1, M26)
IVB  igt_gem_bad_reloc_negative-reloc-lut      NSPT(3, M21M34M4)PASS(1, M21)      NSPT(2, M4)
HSW  igt_gem_bad_reloc_negative-reloc-lut      NSPT(9, M40M20)PASS(1, M20)      NSPT(1, M40)
HSW  igt_kms_rotation_crc_primary-rotation      PASS(10, M20M40)      DMESG_WARN(1, M40)
HSW  igt_pm_rc6_residency_rc6-accuracy      PASS(10, M20M40)      FAIL(1, M40)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset
  2014-11-20 21:01 ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset Imre Deak
  2014-11-22 23:17   ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU shuang.he
@ 2014-11-27 11:08   ` Imre Deak
  2014-11-27 19:07     ` Daniel Vetter
  2014-12-01 17:35   ` Paulo Zanoni
  2 siblings, 1 reply; 13+ messages in thread
From: Imre Deak @ 2014-11-27 11:08 UTC (permalink / raw)
  To: intel-gfx

On Thu, 2014-11-20 at 23:01 +0200, Imre Deak wrote:
> Atm, we don't disable RPS interrupts and related work items before
> resetting the GPU. This may interfere with the following GPU
> initialization and cause RPS interrupts to show up in PM_IIR too early
> before calling gen6_enable_rps_interrupts() (triggering a WARN there).
> 
> Solve this by disabling RPS interrupts and flushing any related work
> items before resetting the GPU.
> 
> Reported-by: He, Shuang <shuang.he@intel.com>
> Testcase: igt/gem_reset_stats/ban-render
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86644

> ---
>  drivers/gpu/drm/i915/i915_drv.c |  5 ++++-
>  drivers/gpu/drm/i915/intel_pm.c | 14 +++++++++++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1df4079..93f3df8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -810,6 +810,9 @@ int i915_reset(struct drm_device *dev)
>  	if (!i915.reset)
>  		return 0;
>  
> +	if (drm_core_check_feature(dev, DRIVER_MODESET))
> +		intel_reset_gt_powersave(dev);
> +
>  	mutex_lock(&dev->struct_mutex);
>  
>  	i915_gem_reset(dev);
> @@ -879,7 +882,7 @@ int i915_reset(struct drm_device *dev)
>  		 * of re-init after reset.
>  		 */
>  		if (INTEL_INFO(dev)->gen > 5)
> -			intel_reset_gt_powersave(dev);
> +			intel_enable_gt_powersave(dev);
>  	} else {
>  		mutex_unlock(&dev->struct_mutex);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e361014..9417f8f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6316,8 +6316,20 @@ void intel_reset_gt_powersave(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	if (INTEL_INFO(dev)->gen < 6)
> +		return;
> +
> +	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
> +	/*
> +	 * TODO: disable RPS interrupts on GEN9+ too once RPS support
> +	 * is added for it.
> +	 */
> +	if (INTEL_INFO(dev)->gen < 9)
> +		gen6_disable_rps_interrupts(dev);
> +
>  	dev_priv->rps.enabled = false;
> -	intel_enable_gt_powersave(dev);
> +
>  }
>  
>  static void ibx_init_clock_gating(struct drm_device *dev)


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset
  2014-11-27 11:08   ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset Imre Deak
@ 2014-11-27 19:07     ` Daniel Vetter
  2014-11-27 19:26       ` Imre Deak
  0 siblings, 1 reply; 13+ messages in thread
From: Daniel Vetter @ 2014-11-27 19:07 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

On Thu, Nov 27, 2014 at 01:08:58PM +0200, Imre Deak wrote:
> On Thu, 2014-11-20 at 23:01 +0200, Imre Deak wrote:
> > Atm, we don't disable RPS interrupts and related work items before
> > resetting the GPU. This may interfere with the following GPU
> > initialization and cause RPS interrupts to show up in PM_IIR too early
> > before calling gen6_enable_rps_interrupts() (triggering a WARN there).
> > 
> > Solve this by disabling RPS interrupts and flushing any related work
> > items before resetting the GPU.
> > 
> > Reported-by: He, Shuang <shuang.he@intel.com>
> > Testcase: igt/gem_reset_stats/ban-render
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86644

Also this fixes a regression introduced in

commit 3cc134e3ee09055d5a87193fc7eb0ecf4a59eaa1
Author: Imre Deak <imre.deak@intel.com>
Date:   Wed Nov 19 15:30:03 2014 +0200

    drm/i915: sanitize rps irq enabling

Paulo, can you please review Imre's patches?
-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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset
  2014-11-27 19:07     ` Daniel Vetter
@ 2014-11-27 19:26       ` Imre Deak
  0 siblings, 0 replies; 13+ messages in thread
From: Imre Deak @ 2014-11-27 19:26 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Thu, 2014-11-27 at 20:07 +0100, Daniel Vetter wrote:
> On Thu, Nov 27, 2014 at 01:08:58PM +0200, Imre Deak wrote:
> > On Thu, 2014-11-20 at 23:01 +0200, Imre Deak wrote:
> > > Atm, we don't disable RPS interrupts and related work items before
> > > resetting the GPU. This may interfere with the following GPU
> > > initialization and cause RPS interrupts to show up in PM_IIR too early
> > > before calling gen6_enable_rps_interrupts() (triggering a WARN there).
> > > 
> > > Solve this by disabling RPS interrupts and flushing any related work
> > > items before resetting the GPU.
> > > 
> > > Reported-by: He, Shuang <shuang.he@intel.com>
> > > Testcase: igt/gem_reset_stats/ban-render
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86644
> 
> Also this fixes a regression introduced in
> 
> commit 3cc134e3ee09055d5a87193fc7eb0ecf4a59eaa1
> Author: Imre Deak <imre.deak@intel.com>
> Date:   Wed Nov 19 15:30:03 2014 +0200
> 
>     drm/i915: sanitize rps irq enabling

This issue has been always around it was just less likely to trigger the
WARN. The above commit only made this happen all the time.

> 
> Paulo, can you please review Imre's patches?
> -Daniel


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS
  2014-11-20 21:01 [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS Imre Deak
  2014-11-20 21:01 ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset Imre Deak
@ 2014-12-01 16:29 ` Paulo Zanoni
  2014-12-01 16:34   ` Chris Wilson
  2014-12-01 16:42   ` Imre Deak
  1 sibling, 2 replies; 13+ messages in thread
From: Paulo Zanoni @ 2014-12-01 16:29 UTC (permalink / raw)
  To: Imre Deak, Chris Wilson; +Cc: Intel Graphics Development

2014-11-20 19:01 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> Atm, igt/gem_reset_stats can trigger the recently added WARN on
> left-over PM_IIR bits in gen6_enable_rps_interrupts(). There are two
> reasons for this:
> 1. we call intel_enable_gt_powersave() without a preceeding
>    intel_disable_gt_powersave()
> 2. gen6_disable_rps_interrupts() doesn't mask interrupts in PM_IMR

We don't do this, but we mask stuff through GEN6_PMINTRMSK. Shouldn't
this be enough to prevent IIR from changing?

Chris?

>
> 1. means RPS interrupts will remain enabled and can be serviced during
> the HW initialization after a GPU reset. 2. means even if we called
> gen6_disable_rps_interrupts() any new RPS interrupt during RPS
> initialization would still propagate to PM_IIR too early (though
> wouldn't be serviced).
>
> This patch solves the 2. issue by also masking interrupts in PM_IMR, the
> following patch fixes 1. getting rid of the WARN. This also makes
> intel_enable_gt_powersave() and intel_disable_gt_powersave() more
> symmetric.
>
> Since gen6_disable_rps_interrupts() is called during driver loading with
> i915 interrupts disabled add a new version of gen6_disable_pm_irq() that
> doesn't WARN for this.
>
> Also while at it, get the irq_lock around the whole PM_IMR/IER/IIR
> programming sequence and make sure that any queued PM_IIR bit is also
> cleared.
>
> The WARN was caught by PRTS after I sent my previous RPS sanitizing
> patchset and I could easily reproduce it on HSW. To actually fix it we
> also need the next patch.
>
> Reported-by: He, Shuang <shuang.he@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 8d169e1..598e9689 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -231,9 +231,6 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>
>         assert_spin_locked(&dev_priv->irq_lock);
>
> -       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> -               return;
> -
>         new_val = dev_priv->pm_irq_mask;
>         new_val &= ~interrupt_mask;
>         new_val |= (~enabled_irq_mask & interrupt_mask);
> @@ -247,14 +244,26 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>
>  void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
>  {
> +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> +               return;
> +
>         snb_update_pm_irq(dev_priv, mask, mask);
>  }
>
> -void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> +static void __gen6_disable_pm_irq(struct drm_i915_private *dev_priv,
> +                                 uint32_t mask)
>  {
>         snb_update_pm_irq(dev_priv, mask, 0);
>  }
>
> +void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> +{
> +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> +               return;
> +
> +       __gen6_disable_pm_irq(dev_priv, mask);
> +}
> +
>  void gen6_reset_rps_interrupts(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -289,16 +298,20 @@ void gen6_disable_rps_interrupts(struct drm_device *dev)
>
>         cancel_work_sync(&dev_priv->rps.work);
>
> +       spin_lock_irq(&dev_priv->irq_lock);
> +
>         I915_WRITE(GEN6_PMINTRMSK, INTEL_INFO(dev_priv)->gen >= 8 ?
>                    ~GEN8_PMINTR_REDIRECT_TO_NON_DISP : ~0);
> +
> +       __gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events);
>         I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) &
>                                 ~dev_priv->pm_rps_events);
> +       I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
> +       I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
>
> -       spin_lock_irq(&dev_priv->irq_lock);
>         dev_priv->rps.pm_iir = 0;
> +
>         spin_unlock_irq(&dev_priv->irq_lock);
> -
> -       I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
>  }
>
>  /**
> --
> 1.8.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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS
  2014-12-01 16:29 ` [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS Paulo Zanoni
@ 2014-12-01 16:34   ` Chris Wilson
  2014-12-01 16:47     ` Paulo Zanoni
  2014-12-01 16:42   ` Imre Deak
  1 sibling, 1 reply; 13+ messages in thread
From: Chris Wilson @ 2014-12-01 16:34 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: Intel Graphics Development

On Mon, Dec 01, 2014 at 02:29:11PM -0200, Paulo Zanoni wrote:
> 2014-11-20 19:01 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> > Atm, igt/gem_reset_stats can trigger the recently added WARN on
> > left-over PM_IIR bits in gen6_enable_rps_interrupts(). There are two
> > reasons for this:
> > 1. we call intel_enable_gt_powersave() without a preceeding
> >    intel_disable_gt_powersave()
> > 2. gen6_disable_rps_interrupts() doesn't mask interrupts in PM_IMR
> 
> We don't do this, but we mask stuff through GEN6_PMINTRMSK. Shouldn't
> this be enough to prevent IIR from changing?
> 
> Chris?

It should. We should be doing both really, use PM_IMR to treat
IMR/IIR/IER consistently with other interrupts, and use the special
PMINTRMASK as part of rps power tuning.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS
  2014-12-01 16:29 ` [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS Paulo Zanoni
  2014-12-01 16:34   ` Chris Wilson
@ 2014-12-01 16:42   ` Imre Deak
  1 sibling, 0 replies; 13+ messages in thread
From: Imre Deak @ 2014-12-01 16:42 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: Intel Graphics Development

On Mon, 2014-12-01 at 14:29 -0200, Paulo Zanoni wrote:
> 2014-11-20 19:01 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> > Atm, igt/gem_reset_stats can trigger the recently added WARN on
> > left-over PM_IIR bits in gen6_enable_rps_interrupts(). There are two
> > reasons for this:
> > 1. we call intel_enable_gt_powersave() without a preceeding
> >    intel_disable_gt_powersave()
> > 2. gen6_disable_rps_interrupts() doesn't mask interrupts in PM_IMR
> 
> We don't do this, but we mask stuff through GEN6_PMINTRMSK. Shouldn't
> this be enough to prevent IIR from changing?

Note also, that during a subsequent RPS enabling, interrupts will be
unmasked in GEN6_PMINTRMSK, before calling gen6_enable_rps_interrupts().
So not masking the interrupts in PM_IMR here would lead to triggering
RPS interrupts before we explicitly enable them.

> 
> Chris?
> 
> >
> > 1. means RPS interrupts will remain enabled and can be serviced during
> > the HW initialization after a GPU reset. 2. means even if we called
> > gen6_disable_rps_interrupts() any new RPS interrupt during RPS
> > initialization would still propagate to PM_IIR too early (though
> > wouldn't be serviced).
> >
> > This patch solves the 2. issue by also masking interrupts in PM_IMR, the
> > following patch fixes 1. getting rid of the WARN. This also makes
> > intel_enable_gt_powersave() and intel_disable_gt_powersave() more
> > symmetric.
> >
> > Since gen6_disable_rps_interrupts() is called during driver loading with
> > i915 interrupts disabled add a new version of gen6_disable_pm_irq() that
> > doesn't WARN for this.
> >
> > Also while at it, get the irq_lock around the whole PM_IMR/IER/IIR
> > programming sequence and make sure that any queued PM_IIR bit is also
> > cleared.
> >
> > The WARN was caught by PRTS after I sent my previous RPS sanitizing
> > patchset and I could easily reproduce it on HSW. To actually fix it we
> > also need the next patch.
> >
> > Reported-by: He, Shuang <shuang.he@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 8d169e1..598e9689 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -231,9 +231,6 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
> >
> >         assert_spin_locked(&dev_priv->irq_lock);
> >
> > -       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> > -               return;
> > -
> >         new_val = dev_priv->pm_irq_mask;
> >         new_val &= ~interrupt_mask;
> >         new_val |= (~enabled_irq_mask & interrupt_mask);
> > @@ -247,14 +244,26 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
> >
> >  void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> >  {
> > +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> > +               return;
> > +
> >         snb_update_pm_irq(dev_priv, mask, mask);
> >  }
> >
> > -void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > +static void __gen6_disable_pm_irq(struct drm_i915_private *dev_priv,
> > +                                 uint32_t mask)
> >  {
> >         snb_update_pm_irq(dev_priv, mask, 0);
> >  }
> >
> > +void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > +{
> > +       if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> > +               return;
> > +
> > +       __gen6_disable_pm_irq(dev_priv, mask);
> > +}
> > +
> >  void gen6_reset_rps_interrupts(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -289,16 +298,20 @@ void gen6_disable_rps_interrupts(struct drm_device *dev)
> >
> >         cancel_work_sync(&dev_priv->rps.work);
> >
> > +       spin_lock_irq(&dev_priv->irq_lock);
> > +
> >         I915_WRITE(GEN6_PMINTRMSK, INTEL_INFO(dev_priv)->gen >= 8 ?
> >                    ~GEN8_PMINTR_REDIRECT_TO_NON_DISP : ~0);
> > +
> > +       __gen6_disable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> >         I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) &
> >                                 ~dev_priv->pm_rps_events);
> > +       I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
> > +       I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
> >
> > -       spin_lock_irq(&dev_priv->irq_lock);
> >         dev_priv->rps.pm_iir = 0;
> > +
> >         spin_unlock_irq(&dev_priv->irq_lock);
> > -
> > -       I915_WRITE(gen6_pm_iir(dev_priv), dev_priv->pm_rps_events);
> >  }
> >
> >  /**
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS
  2014-12-01 16:34   ` Chris Wilson
@ 2014-12-01 16:47     ` Paulo Zanoni
  2014-12-01 17:13       ` Imre Deak
  2014-12-01 17:26       ` Daniel Vetter
  0 siblings, 2 replies; 13+ messages in thread
From: Paulo Zanoni @ 2014-12-01 16:47 UTC (permalink / raw)
  To: Chris Wilson, Paulo Zanoni, Imre Deak, Intel Graphics Development

2014-12-01 14:34 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Mon, Dec 01, 2014 at 02:29:11PM -0200, Paulo Zanoni wrote:
>> 2014-11-20 19:01 GMT-02:00 Imre Deak <imre.deak@intel.com>:
>> > Atm, igt/gem_reset_stats can trigger the recently added WARN on
>> > left-over PM_IIR bits in gen6_enable_rps_interrupts(). There are two
>> > reasons for this:
>> > 1. we call intel_enable_gt_powersave() without a preceeding
>> >    intel_disable_gt_powersave()
>> > 2. gen6_disable_rps_interrupts() doesn't mask interrupts in PM_IMR
>>
>> We don't do this, but we mask stuff through GEN6_PMINTRMSK. Shouldn't
>> this be enough to prevent IIR from changing?
>>
>> Chris?
>
> It should. We should be doing both really, use PM_IMR to treat
> IMR/IIR/IER consistently with other interrupts, and use the special
> PMINTRMASK as part of rps power tuning.

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

But one thing makes me wonder: we disable IER on
gen6_disable_rps_interrupts but never seem to enable it again (except
for the usual pre/post/uninstall functions)... I know it is not a
problem introduced by this patch, but shouldn't this be a problem too?

I also wonder if there's a way to greatly simplify all this RPS
interrupt handling...

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS
  2014-12-01 16:47     ` Paulo Zanoni
@ 2014-12-01 17:13       ` Imre Deak
  2014-12-01 17:26       ` Daniel Vetter
  1 sibling, 0 replies; 13+ messages in thread
From: Imre Deak @ 2014-12-01 17:13 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: Intel Graphics Development

On Mon, 2014-12-01 at 14:47 -0200, Paulo Zanoni wrote:
> 2014-12-01 14:34 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Mon, Dec 01, 2014 at 02:29:11PM -0200, Paulo Zanoni wrote:
> >> 2014-11-20 19:01 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> >> > Atm, igt/gem_reset_stats can trigger the recently added WARN on
> >> > left-over PM_IIR bits in gen6_enable_rps_interrupts(). There are two
> >> > reasons for this:
> >> > 1. we call intel_enable_gt_powersave() without a preceeding
> >> >    intel_disable_gt_powersave()
> >> > 2. gen6_disable_rps_interrupts() doesn't mask interrupts in PM_IMR
> >>
> >> We don't do this, but we mask stuff through GEN6_PMINTRMSK. Shouldn't
> >> this be enough to prevent IIR from changing?
> >>
> >> Chris?
> >
> > It should. We should be doing both really, use PM_IMR to treat
> > IMR/IIR/IER consistently with other interrupts, and use the special
> > PMINTRMASK as part of rps power tuning.
> 
> In that case: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> But one thing makes me wonder: we disable IER on
> gen6_disable_rps_interrupts but never seem to enable it again (except
> for the usual pre/post/uninstall functions)... I know it is not a
> problem introduced by this patch, but shouldn't this be a problem too?

Yes, it is a problem, I haven't noticed this..

It wasn't a problem for suspend/resume, since there during resume we
call pre/postinstall which will reenable all RPS interrupts in IER too.
But after patch 2/2, we'd disable RPS interrupts during reset clearing
IER, but wouldn't re-enable them in IER afterwards in
gen6_enable_rps_interrupts().

The solution imo is to unmask interrupts in IER in
gen6_enable_rps_interrupts(), and leave them disabled in
pre/postinstall. I'll look into this.

Thanks,
Imre

> 
> I also wonder if there's a way to greatly simplify all this RPS
> interrupt handling...
> 
> > -Chris
> >
> > --
> > Chris Wilson, Intel Open Source Technology Centre
> 
> 
> 


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS
  2014-12-01 16:47     ` Paulo Zanoni
  2014-12-01 17:13       ` Imre Deak
@ 2014-12-01 17:26       ` Daniel Vetter
  1 sibling, 0 replies; 13+ messages in thread
From: Daniel Vetter @ 2014-12-01 17:26 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: Intel Graphics Development

On Mon, Dec 01, 2014 at 02:47:06PM -0200, Paulo Zanoni wrote:
> 2014-12-01 14:34 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Mon, Dec 01, 2014 at 02:29:11PM -0200, Paulo Zanoni wrote:
> >> 2014-11-20 19:01 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> >> > Atm, igt/gem_reset_stats can trigger the recently added WARN on
> >> > left-over PM_IIR bits in gen6_enable_rps_interrupts(). There are two
> >> > reasons for this:
> >> > 1. we call intel_enable_gt_powersave() without a preceeding
> >> >    intel_disable_gt_powersave()
> >> > 2. gen6_disable_rps_interrupts() doesn't mask interrupts in PM_IMR
> >>
> >> We don't do this, but we mask stuff through GEN6_PMINTRMSK. Shouldn't
> >> this be enough to prevent IIR from changing?
> >>
> >> Chris?
> >
> > It should. We should be doing both really, use PM_IMR to treat
> > IMR/IIR/IER consistently with other interrupts, and use the special
> > PMINTRMASK as part of rps power tuning.
> 
> In that case: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> But one thing makes me wonder: we disable IER on
> gen6_disable_rps_interrupts but never seem to enable it again (except
> for the usual pre/post/uninstall functions)... I know it is not a
> problem introduced by this patch, but shouldn't this be a problem too?

We only need rps interrupts when we are not rpm suspended, so I think this
should be fine. Not fully convinced, so if anyone else has doubts a new
rpm subtest for the pm_rps testcase might be useful to prove this one way
or the other.
-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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset
  2014-11-20 21:01 ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset Imre Deak
  2014-11-22 23:17   ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU shuang.he
  2014-11-27 11:08   ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset Imre Deak
@ 2014-12-01 17:35   ` Paulo Zanoni
  2 siblings, 0 replies; 13+ messages in thread
From: Paulo Zanoni @ 2014-12-01 17:35 UTC (permalink / raw)
  To: Imre Deak; +Cc: Intel Graphics Development

2014-11-20 19:01 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> Atm, we don't disable RPS interrupts and related work items before
> resetting the GPU. This may interfere with the following GPU
> initialization and cause RPS interrupts to show up in PM_IIR too early
> before calling gen6_enable_rps_interrupts() (triggering a WARN there).
>
> Solve this by disabling RPS interrupts and flushing any related work
> items before resetting the GPU.
>
> Reported-by: He, Shuang <shuang.he@intel.com>
> Testcase: igt/gem_reset_stats/ban-render
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |  5 ++++-
>  drivers/gpu/drm/i915/intel_pm.c | 14 +++++++++++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1df4079..93f3df8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -810,6 +810,9 @@ int i915_reset(struct drm_device *dev)
>         if (!i915.reset)
>                 return 0;
>
> +       if (drm_core_check_feature(dev, DRIVER_MODESET))
> +               intel_reset_gt_powersave(dev);
> +
>         mutex_lock(&dev->struct_mutex);
>
>         i915_gem_reset(dev);
> @@ -879,7 +882,7 @@ int i915_reset(struct drm_device *dev)
>                  * of re-init after reset.
>                  */
>                 if (INTEL_INFO(dev)->gen > 5)
> -                       intel_reset_gt_powersave(dev);
> +                       intel_enable_gt_powersave(dev);
>         } else {
>                 mutex_unlock(&dev->struct_mutex);
>         }
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e361014..9417f8f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6316,8 +6316,20 @@ void intel_reset_gt_powersave(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
> +       if (INTEL_INFO(dev)->gen < 6)
> +               return;
> +
> +       flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> +
> +       /*
> +        * TODO: disable RPS interrupts on GEN9+ too once RPS support
> +        * is added for it.
> +        */
> +       if (INTEL_INFO(dev)->gen < 9)
> +               gen6_disable_rps_interrupts(dev);
> +
>         dev_priv->rps.enabled = false;
> -       intel_enable_gt_powersave(dev);

In the review of patch 1/2 you mentioned a problem on this patch, so I
guess you'll send a new version, but I just wanted to point that this
function is now very similar to intel_suspend_gt_powersave(), so maybe
there's some magic we could do to avoid the duplication. It's just an
idea, not a requirement.


> +
>  }
>
>  static void ibx_init_clock_gating(struct drm_device *dev)
> --
> 1.8.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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2014-12-01 17:35 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-20 21:01 [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS Imre Deak
2014-11-20 21:01 ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset Imre Deak
2014-11-22 23:17   ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU shuang.he
2014-11-27 11:08   ` [PATCH 2/2] drm/i915: sanitize RPS resetting during GPU reset Imre Deak
2014-11-27 19:07     ` Daniel Vetter
2014-11-27 19:26       ` Imre Deak
2014-12-01 17:35   ` Paulo Zanoni
2014-12-01 16:29 ` [PATCH 1/2] drm/i915: mask RPS IRQs properly when disabling RPS Paulo Zanoni
2014-12-01 16:34   ` Chris Wilson
2014-12-01 16:47     ` Paulo Zanoni
2014-12-01 17:13       ` Imre Deak
2014-12-01 17:26       ` Daniel Vetter
2014-12-01 16:42   ` Imre Deak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox