[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 d04facb..14526dc 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1329,6 +1329,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;
@@ -1723,6 +1725,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 59dc481..6d1fb51 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -519,6 +519,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 2fcf284..a5b0456 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;
 
@@ -1253,6 +1259,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 v2

[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).

v2: add comment about why we use a work queue (Daniel)
    make sure work queue is idle on suspend (Daniel)
    use a delayed work queue since there's no hurry (Daniel)

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

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

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 14526dc..b5977b4 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1725,6 +1725,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.c b/drivers/gpu/drm/i915/i915_drv.c
index 6d1fb51..4d858a9 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -470,6 +470,8 @@ static int i915_drm_freeze(struct drm_device *dev)
 			return error;
 		}
 
+		intel_gt_cleanup(dev);
+
 		intel_modeset_disable(dev);
 
 		drm_irq_uninstall(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a5b0456..1e84a59 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -871,6 +871,8 @@ typedef struct drm_i915_private {
 		int r_t;
 	} ips;
 
+	struct delayed_work gen6_power_work;
+
 	enum no_fbc_reason no_fbc_reason;
 
 	struct drm_mm_node *compressed_fb;
@@ -1268,6 +1270,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_reset(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 59068be..025abbf 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3312,15 +3312,34 @@ 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.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);
+		/*
+		 * PCU communication is slow and this doesn't need to be
+		 * done at any specific time, so do this out of our fast path
+		 * to make resume and init faster.
+		 */
+		schedule_delayed_work(&dev_priv->gen6_power_work, HZ);
 	}
 }
 
@@ -4175,6 +4194,15 @@ void intel_gt_init(struct drm_device *dev)
 		dev_priv->gt.force_wake_get = __gen6_gt_force_wake_get;
 		dev_priv->gt.force_wake_put = __gen6_gt_force_wake_put;
 	}
+	INIT_DELAYED_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_delayed_work_sync(&dev_priv->gen6_power_work);
 }
 
 int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
-- 
1.7.9.5

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

[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.

v2: change gtt restore default on pre-gen4 (Chris)
    move needs_gtt_restore flag into dev_priv

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_dma.c |    4 ++++
 drivers/gpu/drm/i915/i915_drv.c |    3 ++-
 drivers/gpu/drm/i915/i915_drv.h |    2 ++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index b5977b4..c027266 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1339,6 +1339,10 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	/* FIXME: do pre/post-mode set stuff in core KMS code */
 	dev->vblank_disable_allowed = 1;
 
+	/* Gen4+ should have saner BIOSes (we hope) */
+	if (INTEL_INFO(dev)->gen < 4)
+		dev_priv->needs_gtt_restore = true;
+
 	ret = intel_fbdev_init(dev);
 	if (ret)
 		goto cleanup_irq;
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4d858a9..be9f47d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -540,7 +540,8 @@ static int i915_drm_thaw(struct drm_device *dev)
 
 	intel_gt_reset(dev);
 
-	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
+	    dev_priv->needs_gtt_restore) {
 		mutex_lock(&dev->struct_mutex);
 		i915_gem_restore_gtt_mappings(dev);
 		mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1e84a59..a38eba8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -873,6 +873,8 @@ typedef struct drm_i915_private {
 
 	struct delayed_work gen6_power_work;
 
+	bool needs_gtt_restore;
+
 	enum no_fbc_reason no_fbc_reason;
 
 	struct drm_mm_node *compressed_fb;
-- 
1.7.9.5

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

[Chris Wilson]

On Fri, 26 Oct 2012 10:08: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.
> 
> v2: change gtt restore default on pre-gen4 (Chris)
>     move needs_gtt_restore flag into dev_priv

I'm confident that it will work on gen3 now as well, just gen2 remains
doubtful. Hmm, I still have the pnv box that required the GTT rewrite
after resume on my desk...  Perhaps a more explicit test for a "sane" BIOS
would be one that provides OpRegion.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Rodrigo Vivi]

Reviewed and tested on My SNB fixing bug:
https://bugs.freedesktop.org/show_bug.cgi?id=54089

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Tested-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Fri, Oct 26, 2012 at 3:08 PM, Jesse Barnes <jbarnes@virtuousgeek.org> 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).
>
> v2: add comment about why we use a work queue (Daniel)
>     make sure work queue is idle on suspend (Daniel)
>     use a delayed work queue since there's no hurry (Daniel)
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
>
> Signed-of-by: Jesse Barnes <jbarnes@virtuougseek.org>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |    1 +
>  drivers/gpu/drm/i915/i915_drv.c |    2 ++
>  drivers/gpu/drm/i915/i915_drv.h |    3 +++
>  drivers/gpu/drm/i915/intel_pm.c |   32 ++++++++++++++++++++++++++++++--
>  4 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 14526dc..b5977b4 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1725,6 +1725,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.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6d1fb51..4d858a9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -470,6 +470,8 @@ static int i915_drm_freeze(struct drm_device *dev)
>                         return error;
>                 }
>
> +               intel_gt_cleanup(dev);
> +
>                 intel_modeset_disable(dev);
>
>                 drm_irq_uninstall(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a5b0456..1e84a59 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -871,6 +871,8 @@ typedef struct drm_i915_private {
>                 int r_t;
>         } ips;
>
> +       struct delayed_work gen6_power_work;
> +
>         enum no_fbc_reason no_fbc_reason;
>
>         struct drm_mm_node *compressed_fb;
> @@ -1268,6 +1270,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_reset(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 59068be..025abbf 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3312,15 +3312,34 @@ 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.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);
> +               /*
> +                * PCU communication is slow and this doesn't need to be
> +                * done at any specific time, so do this out of our fast path
> +                * to make resume and init faster.
> +                */
> +               schedule_delayed_work(&dev_priv->gen6_power_work, HZ);
>         }
>  }
>
> @@ -4175,6 +4194,15 @@ void intel_gt_init(struct drm_device *dev)
>                 dev_priv->gt.force_wake_get = __gen6_gt_force_wake_get;
>                 dev_priv->gt.force_wake_put = __gen6_gt_force_wake_put;
>         }
> +       INIT_DELAYED_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_delayed_work_sync(&dev_priv->gen6_power_work);
>  }
>
>  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

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

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Fri, Oct 26, 2012 at 3:08 PM, 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.
>
> 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 d04facb..14526dc 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1329,6 +1329,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;
> @@ -1723,6 +1725,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 59dc481..6d1fb51 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -519,6 +519,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 2fcf284..a5b0456 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;
>
> @@ -1253,6 +1259,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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

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

[Jesse Barnes]

On Tue, 30 Oct 2012 14:53:15 -0200
Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:

> Reviewed and tested on My SNB fixing bug:
> https://bugs.freedesktop.org/show_bug.cgi?id=54089
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Tested-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

Cool, thanks for testing.  It really fixed that bug?  I had my
doubts. :)

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Rodrigo Vivi]

I was here trying to cheking init values and change forcewake set
during init, etc and nothing was getting RC6 running after resume
besides that msleep(50) workaround after setting CACHE_MODE_0...

Now with your patch my RC6 is back to life after resume: RC6 35.8% :D

Well, but when looking at BSPec I noticed that CACHE_MODE_0 is 0x02120
only for SNB. For IVB+ is 0x7004 what is defined as CACHE_MODE_1 in
our code.
I'm afraid we are doing something wrong here.... or am I missing something?

On Tue, Oct 30, 2012 at 2:58 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Tue, 30 Oct 2012 14:53:15 -0200
> Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>
>> Reviewed and tested on My SNB fixing bug:
>> https://bugs.freedesktop.org/show_bug.cgi?id=54089
>>
>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> Tested-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>
> Cool, thanks for testing.  It really fixed that bug?  I had my
> doubts. :)
>
> --
> Jesse Barnes, Intel Open Source Technology Center



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

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

[Daniel Vetter]

On Tue, Oct 30, 2012 at 6:03 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> I was here trying to cheking init values and change forcewake set
> during init, etc and nothing was getting RC6 running after resume
> besides that msleep(50) workaround after setting CACHE_MODE_0...
>
> Now with your patch my RC6 is back to life after resume: RC6 35.8% :D
>
> Well, but when looking at BSPec I noticed that CACHE_MODE_0 is 0x02120
> only for SNB. For IVB+ is 0x7004 what is defined as CACHE_MODE_1 in
> our code.
> I'm afraid we are doing something wrong here.... or am I missing something?

This is the legacy register save/restore code for resume, which should
be burnt. On a big pyre. With gasoline. Really.

Cheers, Daniel

PS: Unfortunately it's a giant mess to review all the code paths and
make sure that we can disable a given register restore block for kms.
I've started, but it's slow-going (see the "dont save/restore * regs
for kms" patch series which is already merged). Patches _highly_
welcome.
-- 
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 v2

[Daniel Vetter]

On Fri, Oct 26, 2012 at 7:08 PM, Jesse Barnes <jbarnes@virtuousgeek.org> 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).
>
> v2: add comment about why we use a work queue (Daniel)
>     make sure work queue is idle on suspend (Daniel)
>     use a delayed work queue since there's no hurry (Daniel)
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
>
> Signed-of-by: Jesse Barnes <jbarnes@virtuougseek.org>

Unfortunately I have to tear this patch apart ;-)

- The cleanup sequence is inconsistent afaict: Check the ordering of
intel_gt_cleanup vs. intel_disable_gt_powersave

- cancel_*_sync is on a level with *_lock in it's ability to hang
machines in a deadlock. Hiding it in an innocent sounding functions
like gt_cleanup (which _only_ cancels the work item) is not a good
idea. Also, I'd really prefer more symmetric in our setup/teardown
code, so if that cancel_work can't be in in disable_gt_powersave, we
need a good reason for it. I know, lock inversion, but my next point
will solve that ;-)

- you still grab the dev->struct_mutex lock, which means you've only
moved that 200ms delay to someplace you don't measure it. Most likely
it'll be right when userspace wants to do a nice 15 frames fade-in for
the unlock screen, which we'll completely drop on the floor due to
hogging the execbuf lock for an eternity.

Iow we need to add a new dev_priv->rps.lock mutex in a first patch,
then move things around like you do here. That should also take care
of any deadlock problems with the work item itself, so you can move
the cancel_work into disable_gt_powersave right before we shut down
rps/rc6.

Cheers, Daniel
-- 
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 v2

[Daniel Vetter]

On Fri, Oct 26, 2012 at 10:08:38AM -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.
> 
> v2: change gtt restore default on pre-gen4 (Chris)
>     move needs_gtt_restore flag into dev_priv
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

I've just realized: GGTT PTEs are stored in stolen mem, and hence not
restored accross S4.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_dma.c |    4 ++++
>  drivers/gpu/drm/i915/i915_drv.c |    3 ++-
>  drivers/gpu/drm/i915/i915_drv.h |    2 ++
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index b5977b4..c027266 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1339,6 +1339,10 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	/* FIXME: do pre/post-mode set stuff in core KMS code */
>  	dev->vblank_disable_allowed = 1;
>  
> +	/* Gen4+ should have saner BIOSes (we hope) */
> +	if (INTEL_INFO(dev)->gen < 4)
> +		dev_priv->needs_gtt_restore = true;
> +
>  	ret = intel_fbdev_init(dev);
>  	if (ret)
>  		goto cleanup_irq;
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 4d858a9..be9f47d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -540,7 +540,8 @@ static int i915_drm_thaw(struct drm_device *dev)
>  
>  	intel_gt_reset(dev);
>  
> -	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
> +	    dev_priv->needs_gtt_restore) {
>  		mutex_lock(&dev->struct_mutex);
>  		i915_gem_restore_gtt_mappings(dev);
>  		mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1e84a59..a38eba8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -873,6 +873,8 @@ typedef struct drm_i915_private {
>  
>  	struct delayed_work gen6_power_work;
>  
> +	bool needs_gtt_restore;
> +
>  	enum no_fbc_reason no_fbc_reason;
>  
>  	struct drm_mm_node *compressed_fb;
> -- 
> 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 v2

[Chris Wilson]

On Tue, 30 Oct 2012 18:59:31 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Oct 26, 2012 at 10:08:38AM -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.
> > 
> > v2: change gtt restore default on pre-gen4 (Chris)
> >     move needs_gtt_restore flag into dev_priv
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> I've just realized: GGTT PTEs are stored in stolen mem, and hence not
> restored accross S4.

How to ruin the day. So we may as just evict everything upon suspend and
rebuild as needed?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Daniel Vetter]

On Tue, Oct 30, 2012 at 10:32 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, 30 Oct 2012 18:59:31 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Fri, Oct 26, 2012 at 10:08:38AM -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.
>> >
>> > v2: change gtt restore default on pre-gen4 (Chris)
>> >     move needs_gtt_restore flag into dev_priv
>> >
>> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>>
>> I've just realized: GGTT PTEs are stored in stolen mem, and hence not
>> restored accross S4.
>
> How to ruin the day. So we may as just evict everything upon suspend and
> rebuild as needed?

I think we already do that for pretty much all objects (I might have
been confused a bit). The problem is to correctly sprinkle the entire
gtt with scratch page entries ... I don't see an easy way to avoid
that.
-Daniel
-- 
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 v2

[Jesse Barnes]

On Tue, 30 Oct 2012 21:32:17 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Tue, 30 Oct 2012 18:59:31 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Oct 26, 2012 at 10:08:38AM -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.
> > > 
> > > v2: change gtt restore default on pre-gen4 (Chris)
> > >     move needs_gtt_restore flag into dev_priv
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> > I've just realized: GGTT PTEs are stored in stolen mem, and hence not
> > restored accross S4.
> 
> How to ruin the day. So we may as just evict everything upon suspend and
> rebuild as needed?

Yeah Daniel is a party pooper.

So I need to measure this again anyway now that we write combine
things, it may not be worth it anymore (though 2M of writes is still a
fairly large amount).  If it's still a long delay, I'll just apply it
to the S3 paths instead of S4.

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Jesse Barnes]

On Tue, 30 Oct 2012 18:17:58 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Oct 26, 2012 at 7:08 PM, Jesse Barnes <jbarnes@virtuousgeek.org> 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).
> >
> > v2: add comment about why we use a work queue (Daniel)
> >     make sure work queue is idle on suspend (Daniel)
> >     use a delayed work queue since there's no hurry (Daniel)
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
> >
> > Signed-of-by: Jesse Barnes <jbarnes@virtuougseek.org>
> 
> Unfortunately I have to tear this patch apart ;-)
> 
> - The cleanup sequence is inconsistent afaict: Check the ordering of
> intel_gt_cleanup vs. intel_disable_gt_powersave

Ok will fix.

> - cancel_*_sync is on a level with *_lock in it's ability to hang
> machines in a deadlock. Hiding it in an innocent sounding functions
> like gt_cleanup (which _only_ cancels the work item) is not a good
> idea. Also, I'd really prefer more symmetric in our setup/teardown
> code, so if that cancel_work can't be in in disable_gt_powersave, we
> need a good reason for it. I know, lock inversion, but my next point
> will solve that ;-)

Note we have timeouts and such in the cancellation path, so I don't
think there's any way it could hang?

> - you still grab the dev->struct_mutex lock, which means you've only
> moved that 200ms delay to someplace you don't measure it. Most likely
> it'll be right when userspace wants to do a nice 15 frames fade-in for
> the unlock screen, which we'll completely drop on the floor due to
> hogging the execbuf lock for an eternity.

Oh it'll probably happen around VT switch time while we do the silly
dance (that's another thing I'd like to remove).

> Iow we need to add a new dev_priv->rps.lock mutex in a first patch,
> then move things around like you do here. That should also take care
> of any deadlock problems with the work item itself, so you can move
> the cancel_work into disable_gt_powersave right before we shut down
> rps/rc6.

Yeah that would be nicer.  There's really no need to block anyone on
the ring freq and RPS stuff; they can happen totally asynchronously (at
least I'm pretty sure of this, we do have bugs that indicate RPS vs RC6
may be an issue we need to handle, but that's neither here nor there).

-- 
Jesse Barnes, Intel Open Source Technology Center

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 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 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]

> 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.

[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