[PATCH v2 0/2] drm/i915: sanitize RPS resetting during GPU reset

[PATCH v2 0/2] drm/i915: sanitize RPS resetting during GPU reset

[Imre Deak]

This is v2 of patchset [1], with one issue fixed that was noticed by
Paulo. Patch 1/1 of the original patchset is already merged.

[1]
http://lists.freedesktop.org/archives/intel-gfx/2014-November/055969.html

Imre Deak (2):
  drm/i915: move RPS PM_IER enabling to gen6_enable_rps_interrupts
  drm/i915: sanitize RPS resetting during GPU reset

 drivers/gpu/drm/i915/i915_drv.c |  5 ++++-
 drivers/gpu/drm/i915/i915_irq.c | 16 +++++++++++++---
 drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++++++++++++---------
 3 files changed, 36 insertions(+), 13 deletions(-)

-- 
1.8.4

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

[PATCH v2 1/2] drm/i915: move RPS PM_IER enabling to gen6_enable_rps_interrupts

[Imre Deak]

Paulo noticed that we don't enable RPS interrupts via PM_IER in
gen6_enable_rps_interrupts(). This wasn't a problem so far, since the
only place we disabled RPS interrupts was during system/runtime suspend
and after that we reenable all interrupts in the IRQ pre/postinstall
hooks.

In the next patch we'll disable/reenable RPS interrupts during GPU reset
too, but not call IRQ uninstall, pre/postinstall hooks, so there the
above wouldn't work. The logical place for programming PM_IER is
gen6_enable_rps_interrupts() and this also makes the function more
symmetric with gen6_disable_rps_interrupts(), so move the programming
there from the postinstall hooks.

Note that these changes don't affect the ILK RPS interrupt code, which
could be sanitized in a similar way. But that can be done as a
follow-up.

Credits-to: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7913a72..7ce443e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -281,10 +281,14 @@ void gen6_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);
 	WARN_ON(I915_READ(gen6_pm_iir(dev_priv)) & dev_priv->pm_rps_events);
 	dev_priv->rps.interrupts_enabled = true;
+	I915_WRITE(gen6_pm_ier(dev_priv), I915_READ(gen6_pm_ier(dev_priv)) |
+				dev_priv->pm_rps_events);
 	gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
+
 	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
@@ -3307,8 +3311,10 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 	GEN5_IRQ_INIT(GT, dev_priv->gt_irq_mask, gt_irqs);
 
 	if (INTEL_INFO(dev)->gen >= 6) {
-		pm_irqs |= dev_priv->pm_rps_events;
-
+		/*
+		 * RPS interrupts will get enabled/disabled on demand when RPS
+		 * itself is enabled/disabled.
+		 */
 		if (HAS_VEBOX(dev))
 			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
 
@@ -3520,7 +3526,11 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
 	dev_priv->pm_irq_mask = 0xffffffff;
 	GEN8_IRQ_INIT_NDX(GT, 0, ~gt_interrupts[0], gt_interrupts[0]);
 	GEN8_IRQ_INIT_NDX(GT, 1, ~gt_interrupts[1], gt_interrupts[1]);
-	GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_irq_mask, dev_priv->pm_rps_events);
+	/*
+	 * RPS interrupts will get enabled/disabled on demand when RPS itself
+	 * is enabled/disabled.
+	 */
+	GEN8_IRQ_INIT_NDX(GT, 2, dev_priv->pm_irq_mask, 0);
 	GEN8_IRQ_INIT_NDX(GT, 3, ~gt_interrupts[3], gt_interrupts[3]);
 }
 
-- 
1.8.4

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

[PATCH v2 2/2] drm/i915: sanitize RPS resetting during GPU reset

[Imre Deak]

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.

v2:
- split out the common parts of the gt suspend and the new gt reset
  functions (Paulo)

Reported-by: He, Shuang <shuang.he@intel.com>
Testcase: igt/gem_reset_stats/ban-render
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86644
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |  5 ++++-
 drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++++++++++++---------
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 71be3c9..8377249 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);
@@ -881,7 +884,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 78911e2..45c786f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6072,6 +6072,20 @@ void intel_cleanup_gt_powersave(struct drm_device *dev)
 		valleyview_cleanup_gt_powersave(dev);
 }
 
+static void gen6_suspend_rps(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	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);
+}
+
 /**
  * intel_suspend_gt_powersave - suspend PM work and helper threads
  * @dev: drm device
@@ -6087,14 +6101,7 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
 	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);
+	gen6_suspend_rps(dev);
 
 	/* Force GPU to min freq during suspend */
 	gen6_rps_idle(dev_priv);
@@ -6197,8 +6204,11 @@ 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;
+
+	gen6_suspend_rps(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

Re: [PATCH v2 2/2] drm/i915: sanitize RPS resetting during GPU reset

[Daniel Vetter]

On Thu, Dec 04, 2014 at 02:59:32PM +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.
> 
> v2:
> - split out the common parts of the gt suspend and the new gt reset
>   functions (Paulo)
> 
> Reported-by: He, Shuang <shuang.he@intel.com>
> Testcase: igt/gem_reset_stats/ban-render
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86644
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |  5 ++++-
>  drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++++++++++++---------
>  2 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 71be3c9..8377249 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);

UMS support is dead, so you can leave this hunk out.
-Daniel

> +
>  	mutex_lock(&dev->struct_mutex);
>  
>  	i915_gem_reset(dev);
> @@ -881,7 +884,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 78911e2..45c786f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6072,6 +6072,20 @@ void intel_cleanup_gt_powersave(struct drm_device *dev)
>  		valleyview_cleanup_gt_powersave(dev);
>  }
>  
> +static void gen6_suspend_rps(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	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);
> +}
> +
>  /**
>   * intel_suspend_gt_powersave - suspend PM work and helper threads
>   * @dev: drm device
> @@ -6087,14 +6101,7 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
>  	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);
> +	gen6_suspend_rps(dev);
>  
>  	/* Force GPU to min freq during suspend */
>  	gen6_rps_idle(dev_priv);
> @@ -6197,8 +6204,11 @@ 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;
> +
> +	gen6_suspend_rps(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

-- 
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 v2 2/2] drm/i915: sanitize RPS resetting during GPU reset

[Imre Deak]

On Thu, 2014-12-04 at 14:58 +0100, Daniel Vetter wrote:
> On Thu, Dec 04, 2014 at 02:59:32PM +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.
> > 
> > v2:
> > - split out the common parts of the gt suspend and the new gt reset
> >   functions (Paulo)
> > 
> > Reported-by: He, Shuang <shuang.he@intel.com>
> > Testcase: igt/gem_reset_stats/ban-render
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86644
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c |  5 ++++-
> >  drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++++++++++++---------
> >  2 files changed, 23 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 71be3c9..8377249 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);
> 
> UMS support is dead, so you can leave this hunk out.

Ok, we can call here intel_reset_gt_powersave() unconditionally.

> -Daniel
> 
> > +
> >  	mutex_lock(&dev->struct_mutex);
> >  
> >  	i915_gem_reset(dev);
> > @@ -881,7 +884,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 78911e2..45c786f 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -6072,6 +6072,20 @@ void intel_cleanup_gt_powersave(struct drm_device *dev)
> >  		valleyview_cleanup_gt_powersave(dev);
> >  }
> >  
> > +static void gen6_suspend_rps(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	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);
> > +}
> > +
> >  /**
> >   * intel_suspend_gt_powersave - suspend PM work and helper threads
> >   * @dev: drm device
> > @@ -6087,14 +6101,7 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
> >  	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);
> > +	gen6_suspend_rps(dev);
> >  
> >  	/* Force GPU to min freq during suspend */
> >  	gen6_rps_idle(dev_priv);
> > @@ -6197,8 +6204,11 @@ 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;
> > +
> > +	gen6_suspend_rps(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
> 


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

Re: [PATCH v2 2/2] drm/i915: sanitize RPS resetting during GPU reset

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK                 -1              366/366              365/366
SNB                                  450/450              450/450
IVB              +17                 481/498              498/498
BYT                                  289/289              289/289
HSW                 -1              564/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_kms_flip_bcs-flip-vs-modeset-interruptible      PASS(3, M37M26)      DMESG_WARN(1, M26)
 IVB  igt_kms_3d      DMESG_WARN(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_cursor_crc_cursor-128x128-onscreen      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_cursor_crc_cursor-128x128-random      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_cursor_crc_cursor-128x128-sliding      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_cursor_crc_cursor-256x256-offscreen      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_cursor_crc_cursor-256x256-onscreen      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_cursor_crc_cursor-256x256-sliding      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_cursor_crc_cursor-64x64-offscreen      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_cursor_crc_cursor-64x64-onscreen      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_cursor_crc_cursor-64x64-random      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_cursor_crc_cursor-64x64-sliding      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_cursor_crc_cursor-size-change      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_fence_pin_leak      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_mmio_vs_cs_flip_setcrtc_vs_cs_flip      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_rotation_crc_primary-rotation      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
 IVB  igt_kms_rotation_crc_sprite-rotation      NSPT(1, M34)PASS(7, M4M34M21)      PASS(1, M4)
*HSW  igt_kms_plane_plane-panning-top-left-pipe-C-plane-1      PASS(2, M40)      DMESG_WARN(1, M40)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915: sanitize RPS resetting during GPU reset

[Paulo Zanoni]

2014-12-04 12:07 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> On Thu, 2014-12-04 at 14:58 +0100, Daniel Vetter wrote:
>> On Thu, Dec 04, 2014 at 02:59:32PM +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.
>> >
>> > v2:
>> > - split out the common parts of the gt suspend and the new gt reset
>> >   functions (Paulo)
>> >
>> > Reported-by: He, Shuang <shuang.he@intel.com>
>> > Testcase: igt/gem_reset_stats/ban-render
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86644
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.c |  5 ++++-
>> >  drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++++++++++++---------
>> >  2 files changed, 23 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> > index 71be3c9..8377249 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);
>>
>> UMS support is dead, so you can leave this hunk out.
>
> Ok, we can call here intel_reset_gt_powersave() unconditionally.

Well, it still looks correct, so for both patches:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
>> -Daniel
>>
>> > +
>> >     mutex_lock(&dev->struct_mutex);
>> >
>> >     i915_gem_reset(dev);
>> > @@ -881,7 +884,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 78911e2..45c786f 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -6072,6 +6072,20 @@ void intel_cleanup_gt_powersave(struct drm_device *dev)
>> >             valleyview_cleanup_gt_powersave(dev);
>> >  }
>> >
>> > +static void gen6_suspend_rps(struct drm_device *dev)
>> > +{
>> > +   struct drm_i915_private *dev_priv = dev->dev_private;
>> > +
>> > +   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);
>> > +}
>> > +
>> >  /**
>> >   * intel_suspend_gt_powersave - suspend PM work and helper threads
>> >   * @dev: drm device
>> > @@ -6087,14 +6101,7 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
>> >     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);
>> > +   gen6_suspend_rps(dev);
>> >
>> >     /* Force GPU to min freq during suspend */
>> >     gen6_rps_idle(dev_priv);
>> > @@ -6197,8 +6204,11 @@ 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;
>> > +
>> > +   gen6_suspend_rps(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
>>
>
>
> _______________________________________________
> 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 v2 2/2] drm/i915: sanitize RPS resetting during GPU reset

[Jani Nikula]

On Thu, 04 Dec 2014, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, 2014-12-04 at 14:58 +0100, Daniel Vetter wrote:
>> On Thu, Dec 04, 2014 at 02:59:32PM +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.
>> > 
>> > v2:
>> > - split out the common parts of the gt suspend and the new gt reset
>> >   functions (Paulo)
>> > 
>> > Reported-by: He, Shuang <shuang.he@intel.com>
>> > Testcase: igt/gem_reset_stats/ban-render
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86644
>> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.c |  5 ++++-
>> >  drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++++++++++++---------
>> >  2 files changed, 23 insertions(+), 10 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> > index 71be3c9..8377249 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);
>> 
>> UMS support is dead, so you can leave this hunk out.
>
> Ok, we can call here intel_reset_gt_powersave() unconditionally.

Imre, are these headed for 3.19 since the original 1/2 is there?

commit 9939fba226649c62630a74d36ee45c5d5402b460
Author: Imre Deak <imre.deak@intel.com>
Date:   Thu Nov 20 23:01:47 2014 +0200

    drm/i915: mask RPS IRQs properly when disabling RPS

Please post the version addressing Daniel's comment.

Thanks,
Jani.



>
>> -Daniel
>> 
>> > +
>> >  	mutex_lock(&dev->struct_mutex);
>> >  
>> >  	i915_gem_reset(dev);
>> > @@ -881,7 +884,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 78911e2..45c786f 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -6072,6 +6072,20 @@ void intel_cleanup_gt_powersave(struct drm_device *dev)
>> >  		valleyview_cleanup_gt_powersave(dev);
>> >  }
>> >  
>> > +static void gen6_suspend_rps(struct drm_device *dev)
>> > +{
>> > +	struct drm_i915_private *dev_priv = dev->dev_private;
>> > +
>> > +	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);
>> > +}
>> > +
>> >  /**
>> >   * intel_suspend_gt_powersave - suspend PM work and helper threads
>> >   * @dev: drm device
>> > @@ -6087,14 +6101,7 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
>> >  	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);
>> > +	gen6_suspend_rps(dev);
>> >  
>> >  	/* Force GPU to min freq during suspend */
>> >  	gen6_rps_idle(dev_priv);
>> > @@ -6197,8 +6204,11 @@ 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;
>> > +
>> > +	gen6_suspend_rps(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
>> 
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx