diff mbox

drm/i915: vlv: reserve the GT power context only once during driver init

Message ID 1396267844-9235-1-git-send-email-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Imre Deak March 31, 2014, 12:10 p.m. UTC 
Atm we reserve/allocate and free the power context during GT power
enable/disable time. There is no need to do this, we can reserve/allocate
the buffer once during driver loading and free it during driver cleanup.
The re-reservation can also fail in case the driver previously manages to
allocate something on the given fixed address.

The buffer isn't exepected to move even if allocated by the BIOS, for
safety add an assert to check this assumption.

This also fixed a bug for Ville, where re-reserving the context failed
during a GPU reset (I assume because something else got allocated on its
fixed address).

Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  8 +++++++
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_pm.c      | 41 ++++++++++++++++++++++++++++++------
 3 files changed, 44 insertions(+), 7 deletions(-)

Comments

Ville Syrjälä April 1, 2014, 5:11 p.m. UTC | #1 
On Mon, Mar 31, 2014 at 03:10:44PM +0300, Imre Deak wrote:
> Atm we reserve/allocate and free the power context during GT power
> enable/disable time. There is no need to do this, we can reserve/allocate
> the buffer once during driver loading and free it during driver cleanup.
> The re-reservation can also fail in case the driver previously manages to
> allocate something on the given fixed address.
> 
> The buffer isn't exepected to move even if allocated by the BIOS, for
> safety add an assert to check this assumption.
> 
> This also fixed a bug for Ville, where re-reserving the context failed
> during a GPU reset (I assume because something else got allocated on its
> fixed address).

I'm assuming it was the already existing pctx allocation that caused the
warning. We just call intel_enable_gt_powersave() again during GPU reset
w/o having called intel_disable_gt_powersave() anywhere. So no danger of
clobbering the pctx AFAICS, but getting a WARN on every GPU reset is
rather annoying.

> 
> Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

The patch looks good to me.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

<snip>
Daniel Vetter April 1, 2014, 8:31 p.m. UTC | #2 
On Tue, Apr 01, 2014 at 08:11:22PM +0300, Ville Syrjälä wrote:
> On Mon, Mar 31, 2014 at 03:10:44PM +0300, Imre Deak wrote:
> > Atm we reserve/allocate and free the power context during GT power
> > enable/disable time. There is no need to do this, we can reserve/allocate
> > the buffer once during driver loading and free it during driver cleanup.
> > The re-reservation can also fail in case the driver previously manages to
> > allocate something on the given fixed address.
> > 
> > The buffer isn't exepected to move even if allocated by the BIOS, for
> > safety add an assert to check this assumption.
> > 
> > This also fixed a bug for Ville, where re-reserving the context failed
> > during a GPU reset (I assume because something else got allocated on its
> > fixed address).
> 
> I'm assuming it was the already existing pctx allocation that caused the
> warning. We just call intel_enable_gt_powersave() again during GPU reset
> w/o having called intel_disable_gt_powersave() anywhere. So no danger of
> clobbering the pctx AFAICS, but getting a WARN on every GPU reset is
> rather annoying.
> 
> > 
> > Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> The patch looks good to me.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ecc01f5..0684215 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11759,6 +11759,10 @@  void intel_modeset_gem_init(struct drm_device *dev)
 	struct drm_crtc *c;
 	struct intel_framebuffer *fb;
 
+	mutex_lock(&dev->struct_mutex);
+	intel_init_gt_powersave(dev);
+	mutex_unlock(&dev->struct_mutex);
+
 	intel_modeset_init_hw(dev);
 
 	intel_setup_overlay(dev);
@@ -11845,6 +11849,10 @@  void intel_modeset_cleanup(struct drm_device *dev)
 	drm_mode_config_cleanup(dev);
 
 	intel_cleanup_overlay(dev);
+
+	mutex_lock(&dev->struct_mutex);
+	intel_cleanup_gt_powersave(dev);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9002e77..7b21571 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -897,6 +897,8 @@  void intel_display_power_get(struct drm_i915_private *dev_priv,
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
 void intel_power_domains_init_hw(struct drm_i915_private *dev_priv);
+void intel_init_gt_powersave(struct drm_device *dev);
+void intel_cleanup_gt_powersave(struct drm_device *dev);
 void intel_enable_gt_powersave(struct drm_device *dev);
 void intel_disable_gt_powersave(struct drm_device *dev);
 void ironlake_teardown_rc6(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8482a5b..1095b3e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3201,11 +3201,6 @@  static void valleyview_disable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC_CONTROL, 0);
 
 	gen6_disable_rps_interrupts(dev);
-
-	if (dev_priv->vlv_pctx) {
-		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
-		dev_priv->vlv_pctx = NULL;
-	}
 }
 
 static void intel_print_rc6_info(struct drm_device *dev, u32 mode)
@@ -3549,6 +3544,15 @@  int valleyview_rps_min_freq(struct drm_i915_private *dev_priv)
 	return vlv_punit_read(dev_priv, PUNIT_REG_GPU_LFM) & 0xff;
 }
 
+/* Check that the pctx buffer wasn't move under us. */
+static void valleyview_check_pctx(struct drm_i915_private *dev_priv)
+{
+	unsigned long pctx_addr = I915_READ(VLV_PCBR) & ~4095;
+
+	WARN_ON(pctx_addr != dev_priv->mm.stolen_base +
+			     dev_priv->vlv_pctx->stolen->start);
+}
+
 static void valleyview_setup_pctx(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3593,6 +3597,17 @@  out:
 	dev_priv->vlv_pctx = pctx;
 }
 
+static void valleyview_cleanup_pctx(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (WARN_ON(!dev_priv->vlv_pctx))
+		return;
+
+	drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
+	dev_priv->vlv_pctx = NULL;
+}
+
 static void valleyview_enable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3602,6 +3617,8 @@  static void valleyview_enable_rps(struct drm_device *dev)
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
+	valleyview_check_pctx(dev_priv);
+
 	if ((gtfifodbg = I915_READ(GTFIFODBG))) {
 		DRM_DEBUG_DRIVER("GT fifo had a previous error %x\n",
 				 gtfifodbg);
@@ -4418,6 +4435,18 @@  static void intel_init_emon(struct drm_device *dev)
 	dev_priv->ips.corr = (lcfuse & LCFUSE_HIV_MASK);
 }
 
+void intel_init_gt_powersave(struct drm_device *dev)
+{
+	if (IS_VALLEYVIEW(dev))
+		valleyview_setup_pctx(dev);
+}
+
+void intel_cleanup_gt_powersave(struct drm_device *dev)
+{
+	if (IS_VALLEYVIEW(dev))
+		valleyview_cleanup_pctx(dev);
+}
+
 void intel_disable_gt_powersave(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4472,8 +4501,6 @@  void intel_enable_gt_powersave(struct drm_device *dev)
 		ironlake_enable_rc6(dev);
 		intel_init_emon(dev);
 	} else if (IS_GEN6(dev) || IS_GEN7(dev)) {
-		if (IS_VALLEYVIEW(dev))
-			valleyview_setup_pctx(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