[PATCH 1/3] drm/i915: don't block resume on fb console resume

[PATCH 1/3] drm/i915: don't block resume on fb console resume

[Jesse Barnes]

The console lock can be contended, so rather than prevent other drivers
after us from being held up, queue the console suspend into the global
work queue that can happen anytime.  I've measured this to take around
200ms on my T420.  Combined with the ring freq/turbo change, we should
save almost 1/2 a second on resume.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_dma.c |    3 +++
 drivers/gpu/drm/i915/i915_drv.c |   17 ++++++++++++++---
 drivers/gpu/drm/i915/i915_drv.h |    7 +++++++
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 491394f..f88bfa6 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1327,6 +1327,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_modeset_gem_init(dev);
 
+	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
+
 	ret = drm_irq_install(dev);
 	if (ret)
 		goto cleanup_gem;
@@ -1721,6 +1723,7 @@ int i915_driver_unload(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		intel_fbdev_fini(dev);
 		intel_modeset_cleanup(dev);
+		cancel_work_sync(&dev_priv->console_resume_work);
 
 		/*
 		 * free the memory space allocated for the child device
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a7837e5..824e3c8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -520,6 +520,18 @@ int i915_suspend(struct drm_device *dev, pm_message_t state)
 	return 0;
 }
 
+void intel_console_resume(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private,
+			     console_resume_work);
+	struct drm_device *dev = dev_priv->dev;
+
+	console_lock();
+	intel_fbdev_set_suspend(dev, 0);
+	console_unlock();
+}
+
 static int i915_drm_thaw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -555,9 +567,8 @@ static int i915_drm_thaw(struct drm_device *dev)
 
 	dev_priv->modeset_on_lid = 0;
 
-	console_lock();
-	intel_fbdev_set_suspend(dev, 0);
-	console_unlock();
+	schedule_work(&dev_priv->console_resume_work);
+
 	return error;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9e446b6..e22b9e3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -453,6 +453,12 @@ typedef struct drm_i915_private {
 	u32 hotplug_supported_mask;
 	struct work_struct hotplug_work;
 
+	/*
+	 * The console may be contended at resume, but we don't
+	 * want it to block on it.
+	 */
+	struct work_struct console_resume_work;
+
 	int num_pipe;
 	int num_pch_pll;
 
@@ -1257,6 +1263,7 @@ extern unsigned long i915_mch_val(struct drm_i915_private *dev_priv);
 extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
 extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
 
+extern void intel_console_resume(struct work_struct *work);
 
 /* i915_irq.c */
 void i915_hangcheck_elapsed(unsigned long data);
-- 
1.7.9.5

[PATCH 2/3] drm/i915: put ring frequency and turbo setup into a work queue

[Jesse Barnes]

Communicating via the mailbox registers with the PCU can take quite
awhile.  And updating the ring frequency or enabling turbo is not
something that needs to happen synchronously, so take it out of our init
and resume paths to speed things up (~200ms on my T420).

Signed-of-by: Jesse Barnes <jbarnes@virtuougseek.org>
---
 drivers/gpu/drm/i915/i915_dma.c |    1 +
 drivers/gpu/drm/i915/i915_drv.h |    3 +++
 drivers/gpu/drm/i915/intel_pm.c |   25 +++++++++++++++++++++++--
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f88bfa6..9780845 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1723,6 +1723,7 @@ int i915_driver_unload(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		intel_fbdev_fini(dev);
 		intel_modeset_cleanup(dev);
+		intel_gt_cleanup(dev);
 		cancel_work_sync(&dev_priv->console_resume_work);
 
 		/*
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e22b9e3..8418345 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -875,6 +875,8 @@ typedef struct drm_i915_private {
 		int r_t;
 	} ips;
 
+	struct work_struct gen6_power_work;
+
 	enum no_fbc_reason no_fbc_reason;
 
 	struct drm_mm_node *compressed_fb;
@@ -1271,6 +1273,7 @@ void i915_handle_error(struct drm_device *dev, bool wedged);
 
 extern void intel_irq_init(struct drm_device *dev);
 extern void intel_gt_init(struct drm_device *dev);
+extern void intel_gt_cleanup(struct drm_device *dev);
 
 void i915_error_state_free(struct kref *error_ref);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 07da990..fbf10b8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3286,15 +3286,28 @@ void intel_disable_gt_powersave(struct drm_device *dev)
 	}
 }
 
+static void intel_gen6_powersave_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, struct drm_i915_private, gen6_power_work);
+	struct drm_device *dev = dev_priv->dev;
+
+	mutex_lock(&dev->struct_mutex);
+	gen6_enable_rps(dev);
+	gen6_update_ring_freq(dev);
+	mutex_unlock(&dev->struct_mutex);
+}
+
 void intel_enable_gt_powersave(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
 	if (IS_IRONLAKE_M(dev)) {
 		ironlake_enable_drps(dev);
 		ironlake_enable_rc6(dev);
 		intel_init_emon(dev);
 	} else if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
-		gen6_enable_rps(dev);
-		gen6_update_ring_freq(dev);
+		schedule_work(&dev_priv->gen6_power_work);
 	}
 }
 
@@ -4149,6 +4162,14 @@ void intel_gt_init(struct drm_device *dev)
 					__gen6_gt_force_wake_mt_put;
 			}
 		}
+		INIT_WORK(&dev_priv->gen6_power_work,
+			  intel_gen6_powersave_work);
 	}
 }
 
+void intel_gt_cleanup(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	cancel_work_sync(&dev_priv->gen6_power_work);
+}
-- 
1.7.9.5

[PATCH 3/3] drm/i915: don't rewrite the GTT on resume

[Jesse Barnes]

The BIOS shouldn't be touching this memory across suspend/resume, so
just leave it alone.  This saves us ~50ms on resume on my T420.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 824e3c8..95e2b8b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -119,6 +119,10 @@ module_param_named(i915_enable_ppgtt, i915_enable_ppgtt, int, 0600);
 MODULE_PARM_DESC(i915_enable_ppgtt,
 		"Enable PPGTT (default: true)");
 
+int i915_needs_gtt_restore __read_mostly = 0;
+module_param_named(gtt_restore, i915_needs_gtt_restore, int, 0600);
+MODULE_PARM_DESC(gtt_restore, "Rewrite GTT on resume (default: false)");
+
 static struct drm_driver driver;
 extern int intel_agp_enabled;
 
@@ -537,7 +541,8 @@ static int i915_drm_thaw(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int error = 0;
 
-	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
+	    i915_needs_gtt_restore) {
 		mutex_lock(&dev->struct_mutex);
 		i915_gem_restore_gtt_mappings(dev);
 		mutex_unlock(&dev->struct_mutex);
-- 
1.7.9.5

Re: [PATCH 1/3] drm/i915: don't block resume on fb console resume

[Dave Airlie]

> The console lock can be contended, so rather than prevent other drivers
> after us from being held up, queue the console suspend into the global
> work queue that can happen anytime.  I've measured this to take around
> 200ms on my T420.  Combined with the ring freq/turbo change, we should
> save almost 1/2 a second on resume.

Did you measure booting with quiet? I can't think what else could be
contending on this lock other than printk, now maybe we should reduce
the printk noise!

This will make debugging things even worse than they are, since you
don't see any printks on the screen until after we do this.

Dave.

Re: [PATCH 1/3] drm/i915: don't block resume on fb console resume

[Jesse Barnes]

On Mon, 15 Oct 2012 12:32:05 +1000
Dave Airlie <airlied@gmail.com> wrote:

> > The console lock can be contended, so rather than prevent other drivers
> > after us from being held up, queue the console suspend into the global
> > work queue that can happen anytime.  I've measured this to take around
> > 200ms on my T420.  Combined with the ring freq/turbo change, we should
> > save almost 1/2 a second on resume.
> 
> Did you measure booting with quiet? I can't think what else could be
> contending on this lock other than printk, now maybe we should reduce
> the printk noise!
> 
> This will make debugging things even worse than they are, since you
> don't see any printks on the screen until after we do this.

I thought I measured with quiet, but I'll double check.

Not sure about debugging; it's pretty bad to begin with.  I generally
use netconsole or the "stare really hard at the code" method when it
comes to suspend/resume or init time anyway.

But yeah, some investigation into the console lock contention is
probably a good idea at any rate.  A typical distro config probably
also has some other fbcon driver and probably vgacon built-in too, and
I haven't checked what they do.

Jesse

Re: [PATCH 1/3] drm/i915: don't block resume on fb console resume

[Dave Airlie]

On Mon, Oct 15, 2012 at 2:49 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Mon, 15 Oct 2012 12:32:05 +1000
> Dave Airlie <airlied@gmail.com> wrote:
>
>> > The console lock can be contended, so rather than prevent other drivers
>> > after us from being held up, queue the console suspend into the global
>> > work queue that can happen anytime.  I've measured this to take around
>> > 200ms on my T420.  Combined with the ring freq/turbo change, we should
>> > save almost 1/2 a second on resume.
>>
>> Did you measure booting with quiet? I can't think what else could be
>> contending on this lock other than printk, now maybe we should reduce
>> the printk noise!
>>
>> This will make debugging things even worse than they are, since you
>> don't see any printks on the screen until after we do this.
>
> I thought I measured with quiet, but I'll double check.
>
> Not sure about debugging; it's pretty bad to begin with.  I generally
> use netconsole or the "stare really hard at the code" method when it
> comes to suspend/resume or init time anyway.
>
> But yeah, some investigation into the console lock contention is
> probably a good idea at any rate.  A typical distro config probably
> also has some other fbcon driver and probably vgacon built-in too, and
> I haven't checked what they do.
>

If they are still there at s/r time you now have two problems.

Dave.

Re: [PATCH 3/3] drm/i915: don't rewrite the GTT on resume

[Daniel Vetter]

On Sun, Oct 14, 2012 at 07:10:38PM -0700, Jesse Barnes wrote:
> The BIOS shouldn't be touching this memory across suspend/resume, so
> just leave it alone.  This saves us ~50ms on resume on my T420.

Is that 50ms still accurate with wc gtt ptes?
-Daniel

> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 824e3c8..95e2b8b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -119,6 +119,10 @@ module_param_named(i915_enable_ppgtt, i915_enable_ppgtt, int, 0600);
>  MODULE_PARM_DESC(i915_enable_ppgtt,
>  		"Enable PPGTT (default: true)");
>  
> +int i915_needs_gtt_restore __read_mostly = 0;
> +module_param_named(gtt_restore, i915_needs_gtt_restore, int, 0600);
> +MODULE_PARM_DESC(gtt_restore, "Rewrite GTT on resume (default: false)");
> +
>  static struct drm_driver driver;
>  extern int intel_agp_enabled;
>  
> @@ -537,7 +541,8 @@ static int i915_drm_thaw(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int error = 0;
>  
> -	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
> +	    i915_needs_gtt_restore) {
>  		mutex_lock(&dev->struct_mutex);
>  		i915_gem_restore_gtt_mappings(dev);
>  		mutex_unlock(&dev->struct_mutex);
> -- 
> 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/3] drm/i915: put ring frequency and turbo setup into a work queue

[Daniel Vetter]

On Sun, Oct 14, 2012 at 07:10:37PM -0700, Jesse Barnes wrote:
> Communicating via the mailbox registers with the PCU can take quite
> awhile.  And updating the ring frequency or enabling turbo is not
> something that needs to happen synchronously, so take it out of our init
> and resume paths to speed things up (~200ms on my T420).
> 
> Signed-of-by: Jesse Barnes <jbarnes@virtuougseek.org>

A few minor things:
- Please add a short comment why we schedule_work instead of calling these
  things directly.
- imo calling intel_gt_cleanup in the suspend function can't hurt.
- schedule_delayed_work sounds even better, since as long as the cpu is
  fully busy we don't need to set up anything for power saving, really.
  Also, a short delay of e.g. 1s could paper over a reported regression,
  see:

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

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/i915_dma.c |    1 +
>  drivers/gpu/drm/i915/i915_drv.h |    3 +++
>  drivers/gpu/drm/i915/intel_pm.c |   25 +++++++++++++++++++++++--
>  3 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index f88bfa6..9780845 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1723,6 +1723,7 @@ int i915_driver_unload(struct drm_device *dev)
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		intel_fbdev_fini(dev);
>  		intel_modeset_cleanup(dev);
> +		intel_gt_cleanup(dev);
>  		cancel_work_sync(&dev_priv->console_resume_work);
>  
>  		/*
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e22b9e3..8418345 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -875,6 +875,8 @@ typedef struct drm_i915_private {
>  		int r_t;
>  	} ips;
>  
> +	struct work_struct gen6_power_work;
> +
>  	enum no_fbc_reason no_fbc_reason;
>  
>  	struct drm_mm_node *compressed_fb;
> @@ -1271,6 +1273,7 @@ void i915_handle_error(struct drm_device *dev, bool wedged);
>  
>  extern void intel_irq_init(struct drm_device *dev);
>  extern void intel_gt_init(struct drm_device *dev);
> +extern void intel_gt_cleanup(struct drm_device *dev);
>  
>  void i915_error_state_free(struct kref *error_ref);
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 07da990..fbf10b8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3286,15 +3286,28 @@ void intel_disable_gt_powersave(struct drm_device *dev)
>  	}
>  }
>  
> +static void intel_gen6_powersave_work(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, struct drm_i915_private, gen6_power_work);
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	gen6_enable_rps(dev);
> +	gen6_update_ring_freq(dev);
> +	mutex_unlock(&dev->struct_mutex);
> +}
> +
>  void intel_enable_gt_powersave(struct drm_device *dev)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
>  	if (IS_IRONLAKE_M(dev)) {
>  		ironlake_enable_drps(dev);
>  		ironlake_enable_rc6(dev);
>  		intel_init_emon(dev);
>  	} else if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
> -		gen6_enable_rps(dev);
> -		gen6_update_ring_freq(dev);
> +		schedule_work(&dev_priv->gen6_power_work);
>  	}
>  }
>  
> @@ -4149,6 +4162,14 @@ void intel_gt_init(struct drm_device *dev)
>  					__gen6_gt_force_wake_mt_put;
>  			}
>  		}
> +		INIT_WORK(&dev_priv->gen6_power_work,
> +			  intel_gen6_powersave_work);
>  	}
>  }
>  
> +void intel_gt_cleanup(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	cancel_work_sync(&dev_priv->gen6_power_work);
> +}
> -- 
> 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 3/3] drm/i915: don't rewrite the GTT on resume

[Chris Wilson]

On Sun, 14 Oct 2012 19:10:38 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> The BIOS shouldn't be touching this memory across suspend/resume, so
> just leave it alone.  This saves us ~50ms on resume on my T420.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 824e3c8..95e2b8b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -119,6 +119,10 @@ module_param_named(i915_enable_ppgtt, i915_enable_ppgtt, int, 0600);
>  MODULE_PARM_DESC(i915_enable_ppgtt,
>  		"Enable PPGTT (default: true)");
>  
> +int i915_needs_gtt_restore __read_mostly = 0;
> +module_param_named(gtt_restore, i915_needs_gtt_restore, int, 0600);
> +MODULE_PARM_DESC(gtt_restore, "Rewrite GTT on resume (default: false)");

Wrong way around. This code exists purely because KMS resume was broken
on a number of machines without reinitialising the GTT, ergo this is a
regression.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: don't block resume on fb console resume

[Chris Wilson]

On Sun, 14 Oct 2012 19:10:36 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> The console lock can be contended, so rather than prevent other drivers
> after us from being held up, queue the console suspend into the global
> work queue that can happen anytime.  I've measured this to take around
> 200ms on my T420.  Combined with the ring freq/turbo change, we should
> save almost 1/2 a second on resume.

In gneral it looks like the first couple of patches are reflections of
the async-domains work, and would probably be better if we looked more
closely to integrating into that async init/resume infrastructure. The
first patches floating around were to offload attaching inteldrmfb to a
separate thread.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: don't rewrite the GTT on resume

[Chris Wilson]

On Mon, 15 Oct 2012 09:37:00 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sun, 14 Oct 2012 19:10:38 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > The BIOS shouldn't be touching this memory across suspend/resume, so
> > just leave it alone.  This saves us ~50ms on resume on my T420.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 824e3c8..95e2b8b 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -119,6 +119,10 @@ module_param_named(i915_enable_ppgtt, i915_enable_ppgtt, int, 0600);
> >  MODULE_PARM_DESC(i915_enable_ppgtt,
> >  		"Enable PPGTT (default: true)");
> >  
> > +int i915_needs_gtt_restore __read_mostly = 0;
> > +module_param_named(gtt_restore, i915_needs_gtt_restore, int, 0600);
> > +MODULE_PARM_DESC(gtt_restore, "Rewrite GTT on resume (default: false)");
> 
> Wrong way around. This code exists purely because KMS resume was broken
> on a number of machines without reinitialising the GTT, ergo this is a
> regression.

Looking into the history, at the time we wrote the GTT reinit, I believe
we did not have a GFX_FLUSH_CNTL in the resume path. Now we do, so I'd
be more inclined to believe that we have fixed a bug in the meantime. If
so, we only need to be extra careful for gen2.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: don't rewrite the GTT on resume

[Jesse Barnes]

On Mon, 15 Oct 2012 09:41:33 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sun, Oct 14, 2012 at 07:10:38PM -0700, Jesse Barnes wrote:
> > The BIOS shouldn't be touching this memory across suspend/resume, so
> > just leave it alone.  This saves us ~50ms on resume on my T420.
> 
> Is that 50ms still accurate with wc gtt ptes?

No, I haven't done timings since we switched.  If we determine that the
tlb invalidate is sufficient to fix the problems we saw earlier that
made us do the rewrite, then maybe we can just remove it in favor of a
debug option to CRC the GTT PTEs per my previous patch, and remove this
code altogether.

Jesse

Re: [PATCH 3/3] drm/i915: don't rewrite the GTT on resume

[Jesse Barnes]

On Mon, 15 Oct 2012 10:42:03 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon, 15 Oct 2012 09:37:00 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Sun, 14 Oct 2012 19:10:38 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > The BIOS shouldn't be touching this memory across suspend/resume, so
> > > just leave it alone.  This saves us ~50ms on resume on my T420.
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c |    7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 824e3c8..95e2b8b 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -119,6 +119,10 @@ module_param_named(i915_enable_ppgtt, i915_enable_ppgtt, int, 0600);
> > >  MODULE_PARM_DESC(i915_enable_ppgtt,
> > >  		"Enable PPGTT (default: true)");
> > >  
> > > +int i915_needs_gtt_restore __read_mostly = 0;
> > > +module_param_named(gtt_restore, i915_needs_gtt_restore, int, 0600);
> > > +MODULE_PARM_DESC(gtt_restore, "Rewrite GTT on resume (default: false)");
> > 
> > Wrong way around. This code exists purely because KMS resume was broken
> > on a number of machines without reinitialising the GTT, ergo this is a
> > regression.
> 
> Looking into the history, at the time we wrote the GTT reinit, I believe
> we did not have a GFX_FLUSH_CNTL in the resume path. Now we do, so I'd
> be more inclined to believe that we have fixed a bug in the meantime. If
> so, we only need to be extra careful for gen2.

Yeah good point, do we have victims who can check this for us?

Jesse