diff mbox

[3/7] drm/i915/vlv: Not reallocating VLV PCTX upon every suspend/resume

Message ID 1389245442-10947-1-git-send-email-akash.goel@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

akash.goel@intel.com Jan. 9, 2014, 5:30 a.m. UTC
From: Akash Goel <akash.goel@intel.com>

VLV PCTX will come from stolen memory. Upon every suspend/resume cycle,
this is being deallocated/reallocated. Given that PCTX has to be there
at a constant stolen memory offset, doing this is not required. Also
there is a chance of it getting corrupted, if this range gets used for
some other allocation(on resume), which can result in GPU hangs.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 5 +++++
 drivers/gpu/drm/i915/intel_pm.c        | 9 ++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

Comments

Daniel Vetter Jan. 9, 2014, 7:03 a.m. UTC | #1
On Thu, Jan 09, 2014 at 11:00:42AM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> VLV PCTX will come from stolen memory. Upon every suspend/resume cycle,
> this is being deallocated/reallocated. Given that PCTX has to be there
> at a constant stolen memory offset, doing this is not required. Also
> there is a chance of it getting corrupted, if this range gets used for
> some other allocation(on resume), which can result in GPU hangs.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 5 +++++
>  drivers/gpu/drm/i915/intel_pm.c        | 9 ++++-----
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 29b3693..5cf97d6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -213,6 +213,11 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
>  	if (!drm_mm_initialized(&dev_priv->mm.stolen))
>  		return;
>  
> +	if (dev_priv->vlv_pctx) {
> +		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> +		dev_priv->vlv_pctx = NULL;
> +	}

This should be extracted into a intel_pm_teardown to mirror
intel_pm_setup. I know that we already cleanup the fbc allocations here,
but for sane driver teardown code we should move all those deallocations
out eventually and just have a WARN_ON(drm_mm_not_empty) in here before
tearing down the stolen mem subsystem.
-Daniel

> +
>  	i915_gem_stolen_cleanup_compression(dev);
>  	drm_mm_takedown(&dev_priv->mm.stolen);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 469170c..369cc73 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3138,11 +3138,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)
> @@ -3508,6 +3503,10 @@ static void valleyview_setup_pctx(struct drm_device *dev)
>  	u32 pcbr;
>  	int pctx_size = 24*1024;
>  
> +	/* If PC Context is already there, then bail out*/
> +	if (dev_priv->vlv_pctx)
> +		return;
> +
>  	pcbr = I915_READ(VLV_PCBR);
>  	if (pcbr) {
>  		/* BIOS set it up already, grab the pre-alloc'd space */
> -- 
> 1.8.5.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
akash.goel@intel.com Jan. 12, 2014, 8:47 a.m. UTC | #2
> @@ -213,6 +213,11 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
>  	if (!drm_mm_initialized(&dev_priv->mm.stolen))
>  		return;
>  
> +	if (dev_priv->vlv_pctx) {
> +		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> +		dev_priv->vlv_pctx = NULL;
> +	}

>> This should be extracted into a intel_pm_teardown to mirror intel_pm_setup. I know that we already cleanup the fbc allocations here, but for sane driver teardown code we should move all those deallocations out eventually and just
>> have a WARN_ON(drm_mm_not_empty) in here before tearing down the stolen mem subsystem.

Actually as of now there isn't any such function intel_pm_teardown defined to mirror intel_pm_setup. 
Although there is a intel_enable_gt_powersave & intel_disable_gt_powersave pair, but we couldn't use the intel_disable_gt_powersave for this, as we wanted to clean up the PCTX only at the time of driver unload.
So the most appropriate place for us to do so was i915_gem_cleanup_stolen function. 

Best Regards
Akash

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Thursday, January 09, 2014 12:34 PM
To: Goel, Akash
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 3/7] drm/i915/vlv: Not reallocating VLV PCTX upon every suspend/resume

On Thu, Jan 09, 2014 at 11:00:42AM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> VLV PCTX will come from stolen memory. Upon every suspend/resume 
> cycle, this is being deallocated/reallocated. Given that PCTX has to 
> be there at a constant stolen memory offset, doing this is not 
> required. Also there is a chance of it getting corrupted, if this 
> range gets used for some other allocation(on resume), which can result in GPU hangs.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 5 +++++
>  drivers/gpu/drm/i915/intel_pm.c        | 9 ++++-----
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c 
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 29b3693..5cf97d6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -213,6 +213,11 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
>  	if (!drm_mm_initialized(&dev_priv->mm.stolen))
>  		return;
>  
> +	if (dev_priv->vlv_pctx) {
> +		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> +		dev_priv->vlv_pctx = NULL;
> +	}

This should be extracted into a intel_pm_teardown to mirror intel_pm_setup. I know that we already cleanup the fbc allocations here, but for sane driver teardown code we should move all those deallocations out eventually and just have a WARN_ON(drm_mm_not_empty) in here before tearing down the stolen mem subsystem.
-Daniel

> +
>  	i915_gem_stolen_cleanup_compression(dev);
>  	drm_mm_takedown(&dev_priv->mm.stolen);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
> b/drivers/gpu/drm/i915/intel_pm.c index 469170c..369cc73 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3138,11 +3138,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) @@ 
> -3508,6 +3503,10 @@ static void valleyview_setup_pctx(struct drm_device *dev)
>  	u32 pcbr;
>  	int pctx_size = 24*1024;
>  
> +	/* If PC Context is already there, then bail out*/
> +	if (dev_priv->vlv_pctx)
> +		return;
> +
>  	pcbr = I915_READ(VLV_PCBR);
>  	if (pcbr) {
>  		/* BIOS set it up already, grab the pre-alloc'd space */
> --
> 1.8.5.2
> 
> _______________________________________________
> 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
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 29b3693..5cf97d6 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -213,6 +213,11 @@  void i915_gem_cleanup_stolen(struct drm_device *dev)
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return;
 
+	if (dev_priv->vlv_pctx) {
+		drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
+		dev_priv->vlv_pctx = NULL;
+	}
+
 	i915_gem_stolen_cleanup_compression(dev);
 	drm_mm_takedown(&dev_priv->mm.stolen);
 }
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 469170c..369cc73 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3138,11 +3138,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)
@@ -3508,6 +3503,10 @@  static void valleyview_setup_pctx(struct drm_device *dev)
 	u32 pcbr;
 	int pctx_size = 24*1024;
 
+	/* If PC Context is already there, then bail out*/
+	if (dev_priv->vlv_pctx)
+		return;
+
 	pcbr = I915_READ(VLV_PCBR);
 	if (pcbr) {
 		/* BIOS set it up already, grab the pre-alloc'd space */