public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH 1/4] drm/i915: add sprite restore function v3
@ 2013-03-26 16:25 Jesse Barnes
  2013-03-26 16:25 ` [PATCH 2/4] drm/i915: restore cursor and sprite state when forcing a config restore Jesse Barnes
                   ` (3 more replies)
  0 siblings, 4 replies; 28+ messages in thread
From: Jesse Barnes @ 2013-03-26 16:25 UTC (permalink / raw)
  To: intel-gfx

To be used to restore sprite state on resume.

v2: move sprite tracking bits up so we don't track modified sprite state
v3: use src_x/y in sprite suspend/resume code (Ville)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_drv.h    |    5 +++++
 drivers/gpu/drm/i915/intel_sprite.c |   23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 599e978..276f665 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -247,6 +247,10 @@ struct intel_plane {
 	bool can_scale;
 	int max_downscale;
 	u32 lut_r[1024], lut_g[1024], lut_b[1024];
+	int crtc_x, crtc_y;
+	unsigned int crtc_w, crtc_h;
+	uint32_t src_x, src_y;
+	uint32_t src_w, src_h;
 	void (*update_plane)(struct drm_plane *plane,
 			     struct drm_framebuffer *fb,
 			     struct drm_i915_gem_object *obj,
@@ -532,6 +536,7 @@ extern bool intel_encoder_check_is_cloned(struct intel_encoder *encoder);
 extern void intel_connector_dpms(struct drm_connector *, int mode);
 extern bool intel_connector_get_hw_state(struct intel_connector *connector);
 extern void intel_modeset_check_state(struct drm_device *dev);
+extern void intel_plane_restore(struct drm_plane *plane);
 
 
 static inline struct intel_encoder *intel_attached_encoder(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 1b6eb76..6fdd427 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -441,6 +441,15 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	old_obj = intel_plane->obj;
 
+	intel_plane->crtc_x = crtc_x;
+	intel_plane->crtc_y = crtc_y;
+	intel_plane->crtc_w = crtc_w;
+	intel_plane->crtc_h = crtc_h;
+	intel_plane->src_x = src_x;
+	intel_plane->src_y = src_y;
+	intel_plane->src_w = src_w;
+	intel_plane->src_h = src_h;
+
 	src_w = src_w >> 16;
 	src_h = src_h >> 16;
 
@@ -647,6 +656,20 @@ out_unlock:
 	return ret;
 }
 
+void intel_plane_restore(struct drm_plane *plane)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+
+	if (!plane->crtc || !plane->fb)
+		return;
+
+	intel_update_plane(plane, plane->crtc, plane->fb,
+			   intel_plane->crtc_x, intel_plane->crtc_y,
+			   intel_plane->crtc_w, intel_plane->crtc_h,
+			   intel_plane->src_x, intel_plane->src_y,
+			   intel_plane->src_w, intel_plane->src_h);
+}
+
 static const struct drm_plane_funcs intel_plane_funcs = {
 	.update_plane = intel_update_plane,
 	.disable_plane = intel_disable_plane,
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 2/4] drm/i915: restore cursor and sprite state when forcing a config restore
  2013-03-26 16:25 [PATCH 1/4] drm/i915: add sprite restore function v3 Jesse Barnes
@ 2013-03-26 16:25 ` Jesse Barnes
  2013-03-26 16:40   ` Rodrigo Vivi
                     ` (3 more replies)
  2013-03-26 16:25 ` [PATCH 3/4] drm/i915: enable VT switchless resume v3 Jesse Barnes
                   ` (2 subsequent siblings)
  3 siblings, 4 replies; 28+ messages in thread
From: Jesse Barnes @ 2013-03-26 16:25 UTC (permalink / raw)
  To: intel-gfx

Needed for VT switchless resume.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7307974..093006b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9106,6 +9106,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe;
 	u32 tmp;
+	struct drm_plane *plane;
 	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
@@ -9210,8 +9211,20 @@ setup_pipes:
 
 	if (force_restore) {
 		for_each_pipe(pipe) {
-			intel_crtc_restore_mode(dev_priv->pipe_to_crtc_mapping[pipe]);
+			struct drm_crtc *crtc =
+				dev_priv->pipe_to_crtc_mapping[pipe];
+			struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+			intel_crtc_restore_mode(crtc);
+			if (intel_crtc->cursor_visible) {
+				/* Force update for previously enabled cursor */
+				intel_crtc->cursor_visible = false;
+				intel_crtc_update_cursor(&intel_crtc->base,
+							 true);
+			}
 		}
+		list_for_each_entry(plane, &dev->mode_config.plane_list, head)
+			intel_plane_restore(plane);
 
 		i915_redisable_vga(dev);
 	} else {
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2013-03-26 16:25 [PATCH 1/4] drm/i915: add sprite restore function v3 Jesse Barnes
  2013-03-26 16:25 ` [PATCH 2/4] drm/i915: restore cursor and sprite state when forcing a config restore Jesse Barnes
@ 2013-03-26 16:25 ` Jesse Barnes
  2013-03-26 16:42   ` Rodrigo Vivi
                     ` (2 more replies)
  2013-03-26 16:25 ` [PATCH 4/4] drm/i915: emit a hotplug event on resume Jesse Barnes
  2013-03-26 16:38 ` [PATCH 1/4] drm/i915: add sprite restore function v3 Rodrigo Vivi
  3 siblings, 3 replies; 28+ messages in thread
From: Jesse Barnes @ 2013-03-26 16:25 UTC (permalink / raw)
  To: intel-gfx

With the other bits in place, we can do this safely.

v2: disable backlight on suspend to prevent premature enablement on resume
v3: disable CRTCs on suspend to allow RTD3 (Kristen)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c |   14 +++++++++++---
 drivers/gpu/drm/i915/intel_fb.c |    3 +++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c4b13c..bf57e1c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -458,6 +458,7 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
 
 	/* ignore lid events during suspend */
 	mutex_lock(&dev_priv->modeset_restore_lock);
@@ -481,10 +482,14 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
 
-		intel_modeset_disable(dev);
-
 		drm_irq_uninstall(dev);
 		dev_priv->enable_hotplug_processing = false;
+		/*
+		 * Disable CRTCs directly since we want to preserve sw state
+		 * for _thaw.
+		 */
+		list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+			dev_priv->display.crtc_disable(crtc);
 	}
 
 	i915_save_state(dev);
@@ -562,7 +567,10 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		drm_irq_install(dev);
 
 		intel_modeset_init_hw(dev);
-		intel_modeset_setup_hw_state(dev, false);
+
+		drm_modeset_lock_all(dev);
+		intel_modeset_setup_hw_state(dev, true);
+		drm_modeset_unlock_all(dev);
 
 		/*
 		 * ... but also need to make sure that hotplug processing
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index f203418..8d81c929 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -150,6 +150,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	}
 	info->screen_size = size;
 
+	/* This driver doesn't need a VT switch to restore the mode on resume */
+	info->skip_vt_switch = true;
+
 	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
 	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 4/4] drm/i915: emit a hotplug event on resume
  2013-03-26 16:25 [PATCH 1/4] drm/i915: add sprite restore function v3 Jesse Barnes
  2013-03-26 16:25 ` [PATCH 2/4] drm/i915: restore cursor and sprite state when forcing a config restore Jesse Barnes
  2013-03-26 16:25 ` [PATCH 3/4] drm/i915: enable VT switchless resume v3 Jesse Barnes
@ 2013-03-26 16:25 ` Jesse Barnes
  2013-03-26 16:43   ` Rodrigo Vivi
  2013-03-26 16:38 ` [PATCH 1/4] drm/i915: add sprite restore function v3 Rodrigo Vivi
  3 siblings, 1 reply; 28+ messages in thread
From: Jesse Barnes @ 2013-03-26 16:25 UTC (permalink / raw)
  To: intel-gfx

This will poke userspace into probing for configuration changes that may
have occurred across suspend/resume.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index bf57e1c..0cfc778 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -545,6 +545,24 @@ void intel_console_resume(struct work_struct *work)
 	console_unlock();
 }
 
+static void intel_resume_hotplug(struct drm_device *dev)
+{
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct intel_encoder *encoder;
+
+	mutex_lock(&mode_config->mutex);
+	DRM_DEBUG_KMS("running encoder hotplug functions\n");
+
+	list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
+		if (encoder->hot_plug)
+			encoder->hot_plug(encoder);
+
+	mutex_unlock(&mode_config->mutex);
+
+	/* Just fire off a uevent and let userspace tell us what to do */
+	drm_helper_hpd_irq_event(dev);
+}
+
 static int __i915_drm_thaw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -580,6 +598,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		 * */
 		intel_hpd_init(dev);
 		dev_priv->enable_hotplug_processing = true;
+		/* Config may have changed between suspend and resume */
+		intel_resume_hotplug(dev);
 	}
 
 	intel_opregion_init(dev);
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH 1/4] drm/i915: add sprite restore function v3
  2013-03-26 16:25 [PATCH 1/4] drm/i915: add sprite restore function v3 Jesse Barnes
                   ` (2 preceding siblings ...)
  2013-03-26 16:25 ` [PATCH 4/4] drm/i915: emit a hotplug event on resume Jesse Barnes
@ 2013-03-26 16:38 ` Rodrigo Vivi
  3 siblings, 0 replies; 28+ messages in thread
From: Rodrigo Vivi @ 2013-03-26 16:38 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx


[-- Attachment #1.1: Type: text/plain, Size: 3415 bytes --]

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


On Tue, Mar 26, 2013 at 1:25 PM, Jesse Barnes <jbarnes@virtuousgeek.org>wrote:

> To be used to restore sprite state on resume.
>
> v2: move sprite tracking bits up so we don't track modified sprite state
> v3: use src_x/y in sprite suspend/resume code (Ville)
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_drv.h    |    5 +++++
>  drivers/gpu/drm/i915/intel_sprite.c |   23 +++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 599e978..276f665 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -247,6 +247,10 @@ struct intel_plane {
>         bool can_scale;
>         int max_downscale;
>         u32 lut_r[1024], lut_g[1024], lut_b[1024];
> +       int crtc_x, crtc_y;
> +       unsigned int crtc_w, crtc_h;
> +       uint32_t src_x, src_y;
> +       uint32_t src_w, src_h;
>         void (*update_plane)(struct drm_plane *plane,
>                              struct drm_framebuffer *fb,
>                              struct drm_i915_gem_object *obj,
> @@ -532,6 +536,7 @@ extern bool intel_encoder_check_is_cloned(struct
> intel_encoder *encoder);
>  extern void intel_connector_dpms(struct drm_connector *, int mode);
>  extern bool intel_connector_get_hw_state(struct intel_connector
> *connector);
>  extern void intel_modeset_check_state(struct drm_device *dev);
> +extern void intel_plane_restore(struct drm_plane *plane);
>
>
>  static inline struct intel_encoder *intel_attached_encoder(struct
> drm_connector *connector)
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 1b6eb76..6fdd427 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -441,6 +441,15 @@ intel_update_plane(struct drm_plane *plane, struct
> drm_crtc *crtc,
>
>         old_obj = intel_plane->obj;
>
> +       intel_plane->crtc_x = crtc_x;
> +       intel_plane->crtc_y = crtc_y;
> +       intel_plane->crtc_w = crtc_w;
> +       intel_plane->crtc_h = crtc_h;
> +       intel_plane->src_x = src_x;
> +       intel_plane->src_y = src_y;
> +       intel_plane->src_w = src_w;
> +       intel_plane->src_h = src_h;
> +
>         src_w = src_w >> 16;
>         src_h = src_h >> 16;
>
> @@ -647,6 +656,20 @@ out_unlock:
>         return ret;
>  }
>
> +void intel_plane_restore(struct drm_plane *plane)
> +{
> +       struct intel_plane *intel_plane = to_intel_plane(plane);
> +
> +       if (!plane->crtc || !plane->fb)
> +               return;
> +
> +       intel_update_plane(plane, plane->crtc, plane->fb,
> +                          intel_plane->crtc_x, intel_plane->crtc_y,
> +                          intel_plane->crtc_w, intel_plane->crtc_h,
> +                          intel_plane->src_x, intel_plane->src_y,
> +                          intel_plane->src_w, intel_plane->src_h);
> +}
> +
>  static const struct drm_plane_funcs intel_plane_funcs = {
>         .update_plane = intel_update_plane,
>         .disable_plane = intel_disable_plane,
> --
> 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

[-- Attachment #1.2: Type: text/html, Size: 4462 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/4] drm/i915: restore cursor and sprite state when forcing a config restore
  2013-03-26 16:25 ` [PATCH 2/4] drm/i915: restore cursor and sprite state when forcing a config restore Jesse Barnes
@ 2013-03-26 16:40   ` Rodrigo Vivi
  2013-03-26 16:52   ` Daniel Vetter
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 28+ messages in thread
From: Rodrigo Vivi @ 2013-03-26 16:40 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx


[-- Attachment #1.1: Type: text/plain, Size: 2201 bytes --]

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


On Tue, Mar 26, 2013 at 1:25 PM, Jesse Barnes <jbarnes@virtuousgeek.org>wrote:

> Needed for VT switchless resume.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 7307974..093006b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9106,6 +9106,7 @@ void intel_modeset_setup_hw_state(struct drm_device
> *dev,
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         enum pipe pipe;
>         u32 tmp;
> +       struct drm_plane *plane;
>         struct intel_crtc *crtc;
>         struct intel_encoder *encoder;
>         struct intel_connector *connector;
> @@ -9210,8 +9211,20 @@ setup_pipes:
>
>         if (force_restore) {
>                 for_each_pipe(pipe) {
> -
> intel_crtc_restore_mode(dev_priv->pipe_to_crtc_mapping[pipe]);
> +                       struct drm_crtc *crtc =
> +                               dev_priv->pipe_to_crtc_mapping[pipe];
> +                       struct intel_crtc *intel_crtc =
> to_intel_crtc(crtc);
> +
> +                       intel_crtc_restore_mode(crtc);
> +                       if (intel_crtc->cursor_visible) {
> +                               /* Force update for previously enabled
> cursor */
> +                               intel_crtc->cursor_visible = false;
> +                               intel_crtc_update_cursor(&intel_crtc->base,
> +                                                        true);
> +                       }
>                 }
> +               list_for_each_entry(plane, &dev->mode_config.plane_list,
> head)
> +                       intel_plane_restore(plane);
>
>                 i915_redisable_vga(dev);
>         } else {
> --
> 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

[-- Attachment #1.2: Type: text/html, Size: 3113 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2013-03-26 16:25 ` [PATCH 3/4] drm/i915: enable VT switchless resume v3 Jesse Barnes
@ 2013-03-26 16:42   ` Rodrigo Vivi
  2013-04-03  9:15   ` Daniel Vetter
  2014-04-21 16:37   ` Knut Petersen
  2 siblings, 0 replies; 28+ messages in thread
From: Rodrigo Vivi @ 2013-03-26 16:42 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx


[-- Attachment #1.1: Type: text/plain, Size: 3053 bytes --]

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


On Tue, Mar 26, 2013 at 1:25 PM, Jesse Barnes <jbarnes@virtuousgeek.org>wrote:

> With the other bits in place, we can do this safely.
>
> v2: disable backlight on suspend to prevent premature enablement on resume
> v3: disable CRTCs on suspend to allow RTD3 (Kristen)
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |   14 +++++++++++---
>  drivers/gpu/drm/i915/intel_fb.c |    3 +++
>  2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 6c4b13c..bf57e1c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -458,6 +458,7 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
>  static int i915_drm_freeze(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct drm_crtc *crtc;
>
>         /* ignore lid events during suspend */
>         mutex_lock(&dev_priv->modeset_restore_lock);
> @@ -481,10 +482,14 @@ static int i915_drm_freeze(struct drm_device *dev)
>
>
> cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
>
> -               intel_modeset_disable(dev);
> -
>                 drm_irq_uninstall(dev);
>                 dev_priv->enable_hotplug_processing = false;
> +               /*
> +                * Disable CRTCs directly since we want to preserve sw
> state
> +                * for _thaw.
> +                */
> +               list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> head)
> +                       dev_priv->display.crtc_disable(crtc);
>         }
>
>         i915_save_state(dev);
> @@ -562,7 +567,10 @@ static int __i915_drm_thaw(struct drm_device *dev)
>                 drm_irq_install(dev);
>
>                 intel_modeset_init_hw(dev);
> -               intel_modeset_setup_hw_state(dev, false);
> +
> +               drm_modeset_lock_all(dev);
> +               intel_modeset_setup_hw_state(dev, true);
> +               drm_modeset_unlock_all(dev);
>
>                 /*
>                  * ... but also need to make sure that hotplug processing
> diff --git a/drivers/gpu/drm/i915/intel_fb.c
> b/drivers/gpu/drm/i915/intel_fb.c
> index f203418..8d81c929 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -150,6 +150,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>         }
>         info->screen_size = size;
>
> +       /* This driver doesn't need a VT switch to restore the mode on
> resume */
> +       info->skip_vt_switch = true;
> +
>         drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
>         drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width,
> sizes->fb_height);
>
> --
> 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

[-- Attachment #1.2: Type: text/html, Size: 4069 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 4/4] drm/i915: emit a hotplug event on resume
  2013-03-26 16:25 ` [PATCH 4/4] drm/i915: emit a hotplug event on resume Jesse Barnes
@ 2013-03-26 16:43   ` Rodrigo Vivi
  0 siblings, 0 replies; 28+ messages in thread
From: Rodrigo Vivi @ 2013-03-26 16:43 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx


[-- Attachment #1.1: Type: text/plain, Size: 2085 bytes --]

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


On Tue, Mar 26, 2013 at 1:25 PM, Jesse Barnes <jbarnes@virtuousgeek.org>wrote:

> This will poke userspace into probing for configuration changes that may
> have occurred across suspend/resume.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index bf57e1c..0cfc778 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -545,6 +545,24 @@ void intel_console_resume(struct work_struct *work)
>         console_unlock();
>  }
>
> +static void intel_resume_hotplug(struct drm_device *dev)
> +{
> +       struct drm_mode_config *mode_config = &dev->mode_config;
> +       struct intel_encoder *encoder;
> +
> +       mutex_lock(&mode_config->mutex);
> +       DRM_DEBUG_KMS("running encoder hotplug functions\n");
> +
> +       list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
> +               if (encoder->hot_plug)
> +                       encoder->hot_plug(encoder);
> +
> +       mutex_unlock(&mode_config->mutex);
> +
> +       /* Just fire off a uevent and let userspace tell us what to do */
> +       drm_helper_hpd_irq_event(dev);
> +}
> +
>  static int __i915_drm_thaw(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -580,6 +598,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
>                  * */
>                 intel_hpd_init(dev);
>                 dev_priv->enable_hotplug_processing = true;
> +               /* Config may have changed between suspend and resume */
> +               intel_resume_hotplug(dev);
>         }
>
>         intel_opregion_init(dev);
> --
> 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

[-- Attachment #1.2: Type: text/html, Size: 3021 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 2/4] drm/i915: restore cursor and sprite state when forcing a config restore
  2013-03-26 16:25 ` [PATCH 2/4] drm/i915: restore cursor and sprite state when forcing a config restore Jesse Barnes
  2013-03-26 16:40   ` Rodrigo Vivi
@ 2013-03-26 16:52   ` Daniel Vetter
  2013-03-26 17:07   ` [PATCH 1/2] drm/i915: enable VT switchless resume v3 Jesse Barnes
  2013-03-26 20:25   ` drm/i915: restore cursor and sprite state when forcing a config restore v2 Jesse Barnes
  3 siblings, 0 replies; 28+ messages in thread
From: Daniel Vetter @ 2013-03-26 16:52 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx

On Tue, Mar 26, 2013 at 09:25:44AM -0700, Jesse Barnes wrote:
> Needed for VT switchless resume.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7307974..093006b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9106,6 +9106,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum pipe pipe;
>  	u32 tmp;
> +	struct drm_plane *plane;
>  	struct intel_crtc *crtc;
>  	struct intel_encoder *encoder;
>  	struct intel_connector *connector;
> @@ -9210,8 +9211,20 @@ setup_pipes:
>  
>  	if (force_restore) {
>  		for_each_pipe(pipe) {
> -			intel_crtc_restore_mode(dev_priv->pipe_to_crtc_mapping[pipe]);
> +			struct drm_crtc *crtc =
> +				dev_priv->pipe_to_crtc_mapping[pipe];
> +			struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +			intel_crtc_restore_mode(crtc);

I've just noticed that crtc_restore_mode is a bit buggy, since it
unconditionally enables the pipe. Which is the wrong thing to do if the
pipe has a mode set, but is disabled with dpms off.

> +			if (intel_crtc->cursor_visible) {
> +				/* Force update for previously enabled cursor */
> +				intel_crtc->cursor_visible = false;
> +				intel_crtc_update_cursor(&intel_crtc->base,
> +							 true);
> +			}

We already have an unconditional cursor enable call in the various
platform_crtc_enable functions. Do we really neeed this? Or should we
actually do this ->cursor_visible dance in there instead of here?

>  		}
> +		list_for_each_entry(plane, &dev->mode_config.plane_list, head)
> +			intel_plane_restore(plane);

Thinking more about this we have the same plane restore issue for dpms.
Thanks to my utterly lazy legacy overlay code userspace has to take care
of things already, but what about we move that fix here to the right
place? Would also be useful for the atomic modeset support, where we want
to light up a cursor/plane config right away ...

With those things fixed I think the only thing we need to do here is walk
all the connectors and do a dpms update. Presuming that the crtc->active
tracking is reflecting reality (i.e. all false) it should do the right
thing and restore stuff (at least if your crtc disabling doesn't update
the dpms state - disabled crtc means implicitly dpms off).

Like I've said this has the added benefit that the dpms code is shared
with the resume code.

Cheers,
Your royal pita maintainer ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 1/2] drm/i915: enable VT switchless resume v3
  2013-03-26 16:25 ` [PATCH 2/4] drm/i915: restore cursor and sprite state when forcing a config restore Jesse Barnes
  2013-03-26 16:40   ` Rodrigo Vivi
  2013-03-26 16:52   ` Daniel Vetter
@ 2013-03-26 17:07   ` Jesse Barnes
  2013-03-26 17:07     ` [PATCH 2/2] drm/i915: emit a hotplug event on resume Jesse Barnes
  2013-03-26 20:25   ` drm/i915: restore cursor and sprite state when forcing a config restore v2 Jesse Barnes
  3 siblings, 1 reply; 28+ messages in thread
From: Jesse Barnes @ 2013-03-26 17:07 UTC (permalink / raw)
  To: intel-gfx

With the other bits in place, we can do this safely.

v2: disable backlight on suspend to prevent premature enablement on resume
v3: disable CRTCs on suspend to allow RTD3 (Kristen)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c |   14 +++++++++++---
 drivers/gpu/drm/i915/intel_fb.c |    3 +++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c4b13c..bf57e1c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -458,6 +458,7 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
 
 	/* ignore lid events during suspend */
 	mutex_lock(&dev_priv->modeset_restore_lock);
@@ -481,10 +482,14 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
 
-		intel_modeset_disable(dev);
-
 		drm_irq_uninstall(dev);
 		dev_priv->enable_hotplug_processing = false;
+		/*
+		 * Disable CRTCs directly since we want to preserve sw state
+		 * for _thaw.
+		 */
+		list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+			dev_priv->display.crtc_disable(crtc);
 	}
 
 	i915_save_state(dev);
@@ -562,7 +567,10 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		drm_irq_install(dev);
 
 		intel_modeset_init_hw(dev);
-		intel_modeset_setup_hw_state(dev, false);
+
+		drm_modeset_lock_all(dev);
+		intel_modeset_setup_hw_state(dev, true);
+		drm_modeset_unlock_all(dev);
 
 		/*
 		 * ... but also need to make sure that hotplug processing
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index f203418..8d81c929 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -150,6 +150,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	}
 	info->screen_size = size;
 
+	/* This driver doesn't need a VT switch to restore the mode on resume */
+	info->skip_vt_switch = true;
+
 	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
 	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 2/2] drm/i915: emit a hotplug event on resume
  2013-03-26 17:07   ` [PATCH 1/2] drm/i915: enable VT switchless resume v3 Jesse Barnes
@ 2013-03-26 17:07     ` Jesse Barnes
  0 siblings, 0 replies; 28+ messages in thread
From: Jesse Barnes @ 2013-03-26 17:07 UTC (permalink / raw)
  To: intel-gfx

This will poke userspace into probing for configuration changes that may
have occurred across suspend/resume.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index bf57e1c..0cfc778 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -545,6 +545,24 @@ void intel_console_resume(struct work_struct *work)
 	console_unlock();
 }
 
+static void intel_resume_hotplug(struct drm_device *dev)
+{
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct intel_encoder *encoder;
+
+	mutex_lock(&mode_config->mutex);
+	DRM_DEBUG_KMS("running encoder hotplug functions\n");
+
+	list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
+		if (encoder->hot_plug)
+			encoder->hot_plug(encoder);
+
+	mutex_unlock(&mode_config->mutex);
+
+	/* Just fire off a uevent and let userspace tell us what to do */
+	drm_helper_hpd_irq_event(dev);
+}
+
 static int __i915_drm_thaw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -580,6 +598,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		 * */
 		intel_hpd_init(dev);
 		dev_priv->enable_hotplug_processing = true;
+		/* Config may have changed between suspend and resume */
+		intel_resume_hotplug(dev);
 	}
 
 	intel_opregion_init(dev);
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* drm/i915: restore cursor and sprite state when forcing a config restore v2
  2013-03-26 16:25 ` [PATCH 2/4] drm/i915: restore cursor and sprite state when forcing a config restore Jesse Barnes
                     ` (2 preceding siblings ...)
  2013-03-26 17:07   ` [PATCH 1/2] drm/i915: enable VT switchless resume v3 Jesse Barnes
@ 2013-03-26 20:25   ` Jesse Barnes
  2013-03-26 20:46     ` Daniel Vetter
  3 siblings, 1 reply; 28+ messages in thread
From: Jesse Barnes @ 2013-03-26 20:25 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx

commit 79dd6a5bd6603b9d76b992d59819c3aec50e6c33
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Fri Feb 15 12:37:40 2013 -0800

    drm/i915: restore sprite state when forcing a config restore v2
    
    Needed for VT switchless resume.
    
    v2: cursor state is now handled correctly in crtc_enable (Daniel)
    
    Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7307974..8f0db8c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9106,6 +9106,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe;
 	u32 tmp;
+	struct drm_plane *plane;
 	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
@@ -9210,8 +9211,12 @@ setup_pipes:
 
 	if (force_restore) {
 		for_each_pipe(pipe) {
-			intel_crtc_restore_mode(dev_priv->pipe_to_crtc_mapping[pipe]);
+			struct drm_crtc *crtc =
+				dev_priv->pipe_to_crtc_mapping[pipe];
+			intel_crtc_restore_mode(crtc);
 		}
+		list_for_each_entry(plane, &dev->mode_config.plane_list, head)
+			intel_plane_restore(plane);
 
 		i915_redisable_vga(dev);
 	} else {

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: drm/i915: restore cursor and sprite state when forcing a config restore v2
  2013-03-26 20:25   ` drm/i915: restore cursor and sprite state when forcing a config restore v2 Jesse Barnes
@ 2013-03-26 20:46     ` Daniel Vetter
  2013-03-26 20:57       ` Jesse Barnes
  0 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2013-03-26 20:46 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx

On Tue, Mar 26, 2013 at 01:25:27PM -0700, Jesse Barnes wrote:
> commit 79dd6a5bd6603b9d76b992d59819c3aec50e6c33
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Fri Feb 15 12:37:40 2013 -0800
> 
>     drm/i915: restore sprite state when forcing a config restore v2
>     
>     Needed for VT switchless resume.
>     
>     v2: cursor state is now handled correctly in crtc_enable (Daniel)
>     
>     Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Hand-frobbing patches ... tsk. Applied all for patches of this series,
with a mental note that we have some debt to fix (from way yonder) in this
area around plane/cursor restoring and dpms handling. Next who wanders
beyond, expect to get random-volunteered to improve things ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: drm/i915: restore cursor and sprite state when forcing a config restore v2
  2013-03-26 20:46     ` Daniel Vetter
@ 2013-03-26 20:57       ` Jesse Barnes
  0 siblings, 0 replies; 28+ messages in thread
From: Jesse Barnes @ 2013-03-26 20:57 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Tue, 26 Mar 2013 21:46:03 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Mar 26, 2013 at 01:25:27PM -0700, Jesse Barnes wrote:
> > commit 79dd6a5bd6603b9d76b992d59819c3aec50e6c33
> > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Date:   Fri Feb 15 12:37:40 2013 -0800
> > 
> >     drm/i915: restore sprite state when forcing a config restore v2
> >     
> >     Needed for VT switchless resume.
> >     
> >     v2: cursor state is now handled correctly in crtc_enable (Daniel)
> >     
> >     Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Hand-frobbing patches ... tsk. Applied all for patches of this series,
> with a mental note that we have some debt to fix (from way yonder) in this
> area around plane/cursor restoring and dpms handling. Next who wanders
> beyond, expect to get random-volunteered to improve things ;-)

Oh I didn't edit it inline in the email. :)   I just did a format-patch
and sent it manually since I had trouble getting git send-email to
cooperate (I've since learned the proper way).

-- 
Jesse Barnes, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2013-03-26 16:25 ` [PATCH 3/4] drm/i915: enable VT switchless resume v3 Jesse Barnes
  2013-03-26 16:42   ` Rodrigo Vivi
@ 2013-04-03  9:15   ` Daniel Vetter
  2013-04-03 10:54     ` Chris Wilson
  2014-04-21 16:37   ` Knut Petersen
  2 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2013-04-03  9:15 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx

On Tue, Mar 26, 2013 at 09:25:45AM -0700, Jesse Barnes wrote:
> With the other bits in place, we can do this safely.
> 
> v2: disable backlight on suspend to prevent premature enablement on resume
> v3: disable CRTCs on suspend to allow RTD3 (Kristen)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Something seems to be race with this, occasionally when resuming my vt is
on the X session (at least nothing happens when I try to switch to it),
the cursor is enabled, but I see the kernel console window.

Doing a vt switch to a non-X console and back to X fixes things. I haven't
looked exactly where we could race ...
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c |   14 +++++++++++---
>  drivers/gpu/drm/i915/intel_fb.c |    3 +++
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6c4b13c..bf57e1c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -458,6 +458,7 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
>  static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
>  
>  	/* ignore lid events during suspend */
>  	mutex_lock(&dev_priv->modeset_restore_lock);
> @@ -481,10 +482,14 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
>  
> -		intel_modeset_disable(dev);
> -
>  		drm_irq_uninstall(dev);
>  		dev_priv->enable_hotplug_processing = false;
> +		/*
> +		 * Disable CRTCs directly since we want to preserve sw state
> +		 * for _thaw.
> +		 */
> +		list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> +			dev_priv->display.crtc_disable(crtc);
>  	}
>  
>  	i915_save_state(dev);
> @@ -562,7 +567,10 @@ static int __i915_drm_thaw(struct drm_device *dev)
>  		drm_irq_install(dev);
>  
>  		intel_modeset_init_hw(dev);
> -		intel_modeset_setup_hw_state(dev, false);
> +
> +		drm_modeset_lock_all(dev);
> +		intel_modeset_setup_hw_state(dev, true);
> +		drm_modeset_unlock_all(dev);
>  
>  		/*
>  		 * ... but also need to make sure that hotplug processing
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index f203418..8d81c929 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -150,6 +150,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	}
>  	info->screen_size = size;
>  
> +	/* This driver doesn't need a VT switch to restore the mode on resume */
> +	info->skip_vt_switch = true;
> +
>  	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
>  	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
>  
> -- 
> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2013-04-03  9:15   ` Daniel Vetter
@ 2013-04-03 10:54     ` Chris Wilson
  2013-04-03 15:13       ` Jesse Barnes
  0 siblings, 1 reply; 28+ messages in thread
From: Chris Wilson @ 2013-04-03 10:54 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Wed, Apr 03, 2013 at 11:15:40AM +0200, Daniel Vetter wrote:
> On Tue, Mar 26, 2013 at 09:25:45AM -0700, Jesse Barnes wrote:
> > With the other bits in place, we can do this safely.
> > 
> > v2: disable backlight on suspend to prevent premature enablement on resume
> > v3: disable CRTCs on suspend to allow RTD3 (Kristen)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Something seems to be race with this, occasionally when resuming my vt is
> on the X session (at least nothing happens when I try to switch to it),
> the cursor is enabled, but I see the kernel console window.
> 
> Doing a vt switch to a non-X console and back to X fixes things. I haven't
> looked exactly where we could race ...

This might be a DRM_MASTER race, if X fails to gain DRM_MASTER its
takeover is postponed indefinitely. Any clues in Xorg.0.log?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2013-04-03 10:54     ` Chris Wilson
@ 2013-04-03 15:13       ` Jesse Barnes
  0 siblings, 0 replies; 28+ messages in thread
From: Jesse Barnes @ 2013-04-03 15:13 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Wed, 3 Apr 2013 11:54:23 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Wed, Apr 03, 2013 at 11:15:40AM +0200, Daniel Vetter wrote:
> > On Tue, Mar 26, 2013 at 09:25:45AM -0700, Jesse Barnes wrote:
> > > With the other bits in place, we can do this safely.
> > > 
> > > v2: disable backlight on suspend to prevent premature enablement on resume
> > > v3: disable CRTCs on suspend to allow RTD3 (Kristen)
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> > Something seems to be race with this, occasionally when resuming my vt is
> > on the X session (at least nothing happens when I try to switch to it),
> > the cursor is enabled, but I see the kernel console window.
> > 
> > Doing a vt switch to a non-X console and back to X fixes things. I haven't
> > looked exactly where we could race ...
> 
> This might be a DRM_MASTER race, if X fails to gain DRM_MASTER its
> takeover is postponed indefinitely. Any clues in Xorg.0.log?

Yeah, David said he saw this even w/o the VT switch patches.  I've seen
this bug too, maybe the lack of a VT switch makes the race a little
more likely?

-- 
Jesse Barnes, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2013-03-26 16:25 ` [PATCH 3/4] drm/i915: enable VT switchless resume v3 Jesse Barnes
  2013-03-26 16:42   ` Rodrigo Vivi
  2013-04-03  9:15   ` Daniel Vetter
@ 2014-04-21 16:37   ` Knut Petersen
  2014-05-16 22:20     ` Jesse Barnes
  2 siblings, 1 reply; 28+ messages in thread
From: Knut Petersen @ 2014-04-21 16:37 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx, Daniel Vetter

[-- Attachment #1: Type: text/plain, Size: 4540 bytes --]

On 26.03.2013 17:25, Jesse Barnes wrote:
> With the other bits in place, we can do this safely.
>
> v2: disable backlight on suspend to prevent premature enablement on resume
> v3: disable CRTCs on suspend to allow RTD3 (Kristen)
>
> Signed-off-by: Jesse Barnes<jbarnes@virtuousgeek.org>

I might be a bit late as your patch has long be accepted, but it introduced
a regression. Steps to reproduce the problem:

boot linux,
startx,
suspend (s3),
resume,
terminate xserver,
startx,
mplayer -fs foo.video

Prior to your patch, everything worked fine. With your patch, the xserver starts much slowler,
the monitor is switched off and on again, the kde splash screen is displayed in the upper
left corner (1024x768) on my 1280x1024 monitor. mplayer -fs plays a video correctly, but
the (normally) black bars above and below the video contain garbage for x > ~1024 / y > ~768

Reproducibility: 100% on my system.

Hardware: AOpen i915GMm-hfs, Pentium-M Dothan, Eizo Flexscan L557 monitor

00:02.0 VGA compatible controller: Intel Corporation Mobile 915GM/GMS/910GML Express Graphics Controller (rev 04) (prog-if 00 [VGA controller])
         Subsystem: AOPEN Inc. Device 0554
         Flags: bus master, fast devsel, latency 0, IRQ 16
         Memory at d5300000 (32-bit, non-prefetchable) [size=512K]
         I/O ports at e400 [size=8]
         Memory at c0000000 (32-bit, prefetchable) [size=256M]
         Memory at d5400000 (32-bit, non-prefetchable) [size=256K]
         Expansion ROM at <unassigned> [disabled]
         Capabilities: [d0] Power Management version 2
         Kernel driver in use: i915

00:02.1 Display controller: Intel Corporation Mobile 915GM/GMS/910GML Express Graphics Controller (rev 04)
         Subsystem: AOPEN Inc. Device 0554
         Flags: bus master, fast devsel, latency 0
         Memory at d5380000 (32-bit, non-prefetchable) [size=512K]
         Capabilities: [d0] Power Management version 2

Software: openSuSE 13.1, current git xorg.

last good mainline linux kernel: 3.9.y
kernels affected: 3.10.y, 3.11.y, 3.12.y, 3.13.y, 3.14.y, 3.15-rc2

Attached is a patch against 3.14.1. Directly reverting 24576d2 is impossible as a helper
function was eliminated, a file was renamed, and other patches touched the code, but
more or less my patch is simply a functional revert of yours. At least here it cures the problem ...

cu,
  Knut


> ---
>   drivers/gpu/drm/i915/i915_drv.c |   14 +++++++++++---
>   drivers/gpu/drm/i915/intel_fb.c |    3 +++
>   2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6c4b13c..bf57e1c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -458,6 +458,7 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
>   static int i915_drm_freeze(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
>   
>   	/* ignore lid events during suspend */
>   	mutex_lock(&dev_priv->modeset_restore_lock);
> @@ -481,10 +482,14 @@ static int i915_drm_freeze(struct drm_device *dev)
>   
>   		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
>   
> -		intel_modeset_disable(dev);
> -
>   		drm_irq_uninstall(dev);
>   		dev_priv->enable_hotplug_processing = false;
> +		/*
> +		 * Disable CRTCs directly since we want to preserve sw state
> +		 * for _thaw.
> +		 */
> +		list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> +			dev_priv->display.crtc_disable(crtc);
>   	}
>   
>   	i915_save_state(dev);
> @@ -562,7 +567,10 @@ static int __i915_drm_thaw(struct drm_device *dev)
>   		drm_irq_install(dev);
>   
>   		intel_modeset_init_hw(dev);
> -		intel_modeset_setup_hw_state(dev, false);
> +
> +		drm_modeset_lock_all(dev);
> +		intel_modeset_setup_hw_state(dev, true);
> +		drm_modeset_unlock_all(dev);
>   
>   		/*
>   		 * ... but also need to make sure that hotplug processing
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index f203418..8d81c929 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -150,6 +150,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   	}
>   	info->screen_size = size;
>   
> +	/* This driver doesn't need a VT switch to restore the mode on resume */
> +	info->skip_vt_switch = true;
> +
>   	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
>   	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
>   


[-- Attachment #2: 0001-i915-Fix-S3-resume-regression-introduced-in-24576d2.patch --]
[-- Type: text/x-patch, Size: 3541 bytes --]

>From fc936457f7f7f178184b5decf538ec3280adb7df Mon Sep 17 00:00:00 2001
From: Knut Petersen <Knut_Petersen@t-online.de>
Date: Mon, 21 Apr 2014 17:37:10 +0200
Subject: [PATCH] [i915] Fix S3 resume regression introduced in 24576d2

Signed-off-by: Knut Petersen <Knut_Petersen@t-online.de>
---
 drivers/gpu/drm/i915/i915_drv.c      | 19 ++++++-------------
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_fbdev.c   |  3 ---
 4 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ec7bb0f..fb1354a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -527,16 +527,13 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
 
+		list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+			if (crtc->enabled)
+				intel_crtc_disable(crtc);
+		}
+
 		drm_irq_uninstall(dev);
 		dev_priv->enable_hotplug_processing = false;
-		/*
-		 * Disable CRTCs directly since we want to preserve sw state
-		 * for _thaw.
-		 */
-		mutex_lock(&dev->mode_config.mutex);
-		list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
-			dev_priv->display.crtc_disable(crtc);
-		mutex_unlock(&dev->mode_config.mutex);
 
 		intel_modeset_suspend_hw(dev);
 	}
@@ -648,11 +645,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 		drm_irq_install(dev);
 
 		intel_modeset_init_hw(dev);
-
-		drm_modeset_lock_all(dev);
-		drm_mode_config_reset(dev);
-		intel_modeset_setup_hw_state(dev, true);
-		drm_modeset_unlock_all(dev);
+		intel_modeset_setup_hw_state(dev, false);
 
 		/*
 		 * ... but also need to make sure that hotplug processing
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b8a7c7..825e888 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4346,7 +4346,7 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
 	intel_crtc_update_sarea(crtc, enable);
 }
 
-static void intel_crtc_disable(struct drm_crtc *crtc)
+void intel_crtc_disable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_connector *connector;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fbfaaba..1cc01fa 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -634,6 +634,7 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
+void intel_crtc_disable(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 void intel_connector_dpms(struct drm_connector *, int mode);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 39eac99..10d70b8 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -184,9 +184,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	}
 	info->screen_size = size;
 
-	/* This driver doesn't need a VT switch to restore the mode on resume */
-	info->skip_vt_switch = true;
-
 	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
 	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
 
-- 
1.8.4.5


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2014-04-21 16:37   ` Knut Petersen
@ 2014-05-16 22:20     ` Jesse Barnes
  2014-05-16 22:28       ` Daniel Vetter
  2014-05-16 22:38       ` Chris Wilson
  0 siblings, 2 replies; 28+ messages in thread
From: Jesse Barnes @ 2014-05-16 22:20 UTC (permalink / raw)
  To: Knut Petersen; +Cc: intel-gfx, Daniel Vetter

On Mon, 21 Apr 2014 18:37:31 +0200
Knut Petersen <Knut_Petersen@t-online.de> wrote:
> > +	/* This driver doesn't need a VT switch to restore the mode on resume */
> > +	info->skip_vt_switch = true;
> > +
> >   	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> >   	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);

Is it sufficient to just revert this part?  Or are the other bits
needed too?

Sorry for the delay on this, I've been traveling a lot the past month
and buried in other stuff so out of touch with much of my email.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2014-05-16 22:20     ` Jesse Barnes
@ 2014-05-16 22:28       ` Daniel Vetter
  2014-05-16 22:38       ` Chris Wilson
  1 sibling, 0 replies; 28+ messages in thread
From: Daniel Vetter @ 2014-05-16 22:28 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx

On Sat, May 17, 2014 at 12:20 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Mon, 21 Apr 2014 18:37:31 +0200
> Knut Petersen <Knut_Petersen@t-online.de> wrote:
>> > +   /* This driver doesn't need a VT switch to restore the mode on resume */
>> > +   info->skip_vt_switch = true;
>> > +
>> >     drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
>> >     drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
>
> Is it sufficient to just revert this part?  Or are the other bits
> needed too?
>
> Sorry for the delay on this, I've been traveling a lot the past month
> and buried in other stuff so out of touch with much of my email.

Aren't we tracking this in a bugzilla now? At least I have memories of
another bug where the output state is flip-flopping somehow ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2014-05-16 22:20     ` Jesse Barnes
  2014-05-16 22:28       ` Daniel Vetter
@ 2014-05-16 22:38       ` Chris Wilson
  2014-05-16 22:42         ` Chris Wilson
  1 sibling, 1 reply; 28+ messages in thread
From: Chris Wilson @ 2014-05-16 22:38 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx, Daniel Vetter

On Fri, May 16, 2014 at 03:20:47PM -0700, Jesse Barnes wrote:
> On Mon, 21 Apr 2014 18:37:31 +0200
> Knut Petersen <Knut_Petersen@t-online.de> wrote:
> > > +	/* This driver doesn't need a VT switch to restore the mode on resume */
> > > +	info->skip_vt_switch = true;
> > > +
> > >   	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> > >   	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
> 
> Is it sufficient to just revert this part?  Or are the other bits
> needed too?
> 
> Sorry for the delay on this, I've been traveling a lot the past month
> and buried in other stuff so out of touch with much of my email.

The key step here is that X is restarted after resume. The slow down was
due to X not finding any connected outputs and so disabling them all,
setting up a dummy 1024x768 fb which really confused KDE. (KDE queries
the config causing a forced reprobe of all outputs, setups the display
for the native 1280x1024, but screws up KDE's own bookkeeping.)

The impact has been fixed by handling the connector->status more
robusting during initial output probing in X. What remains is the
question whether connector->status can be set appropriately upon resume?
It requires a detection cycle to be sure that the outputs are still
there, which is arguably better deferred to userspace. To be consistent
the BIOS take over code should mark connector->status as unknown for the
CRTCs it takes over without doing a detection cycle (where we just
presume that the CRTC/output being enabled means something is on the
other end of the pipe and is still valid).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2014-05-16 22:38       ` Chris Wilson
@ 2014-05-16 22:42         ` Chris Wilson
  2014-05-20 19:53           ` Jesse Barnes
  0 siblings, 1 reply; 28+ messages in thread
From: Chris Wilson @ 2014-05-16 22:42 UTC (permalink / raw)
  To: Jesse Barnes, Knut Petersen, intel-gfx, Rodrigo Vivi,
	Daniel Vetter

On Fri, May 16, 2014 at 11:38:07PM +0100, Chris Wilson wrote:
> On Fri, May 16, 2014 at 03:20:47PM -0700, Jesse Barnes wrote:
> > On Mon, 21 Apr 2014 18:37:31 +0200
> > Knut Petersen <Knut_Petersen@t-online.de> wrote:
> > > > +	/* This driver doesn't need a VT switch to restore the mode on resume */
> > > > +	info->skip_vt_switch = true;
> > > > +
> > > >   	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> > > >   	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
> > 
> > Is it sufficient to just revert this part?  Or are the other bits
> > needed too?
> > 
> > Sorry for the delay on this, I've been traveling a lot the past month
> > and buried in other stuff so out of touch with much of my email.
> 
> The key step here is that X is restarted after resume. The slow down was
> due to X not finding any connected outputs and so disabling them all,
> setting up a dummy 1024x768 fb which really confused KDE. (KDE queries
> the config causing a forced reprobe of all outputs, setups the display
> for the native 1280x1024, but screws up KDE's own bookkeeping.)
> 
> The impact has been fixed by handling the connector->status more
> robusting during initial output probing in X. What remains is the
> question whether connector->status can be set appropriately upon resume?
> It requires a detection cycle to be sure that the outputs are still
> there, which is arguably better deferred to userspace. To be consistent
> the BIOS take over code should mark connector->status as unknown for the
> CRTCs it takes over without doing a detection cycle (where we just
> presume that the CRTC/output being enabled means something is on the
> other end of the pipe and is still valid).

Hmm. Why didn't fbcon respond to the hotplug event on resume and perform
a detection cycle before Knut was able to type startx on the console?
That I think is the bug.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2014-05-16 22:42         ` Chris Wilson
@ 2014-05-20 19:53           ` Jesse Barnes
  2014-05-20 19:58             ` Jesse Barnes
  0 siblings, 1 reply; 28+ messages in thread
From: Jesse Barnes @ 2014-05-20 19:53 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, Daniel Vetter

On Fri, 16 May 2014 23:42:23 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, May 16, 2014 at 11:38:07PM +0100, Chris Wilson wrote:
> > On Fri, May 16, 2014 at 03:20:47PM -0700, Jesse Barnes wrote:
> > > On Mon, 21 Apr 2014 18:37:31 +0200
> > > Knut Petersen <Knut_Petersen@t-online.de> wrote:
> > > > > +	/* This driver doesn't need a VT switch to restore the mode on resume */
> > > > > +	info->skip_vt_switch = true;
> > > > > +
> > > > >   	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> > > > >   	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
> > > 
> > > Is it sufficient to just revert this part?  Or are the other bits
> > > needed too?
> > > 
> > > Sorry for the delay on this, I've been traveling a lot the past month
> > > and buried in other stuff so out of touch with much of my email.
> > 
> > The key step here is that X is restarted after resume. The slow down was
> > due to X not finding any connected outputs and so disabling them all,
> > setting up a dummy 1024x768 fb which really confused KDE. (KDE queries
> > the config causing a forced reprobe of all outputs, setups the display
> > for the native 1280x1024, but screws up KDE's own bookkeeping.)
> > 
> > The impact has been fixed by handling the connector->status more
> > robusting during initial output probing in X. What remains is the
> > question whether connector->status can be set appropriately upon resume?
> > It requires a detection cycle to be sure that the outputs are still
> > there, which is arguably better deferred to userspace. To be consistent
> > the BIOS take over code should mark connector->status as unknown for the
> > CRTCs it takes over without doing a detection cycle (where we just
> > presume that the CRTC/output being enabled means something is on the
> > other end of the pipe and is still valid).
> 
> Hmm. Why didn't fbcon respond to the hotplug event on resume and perform
> a detection cycle before Knut was able to type startx on the console?
> That I think is the bug.

Well, fbcon resume is delayed, but we do perform an fb re-probe via
intel_resume_hotplug() that should have done this.  Not sure why that's
sufficient... but I agree userspace should probably re-probe on
resume.  We're supposed to generate a uevent on resume, but the fb
layer may suppress that if it detects that no change has occurred.

Maybe X is racing with our resume_hotplug somehow?  It doesn't look
like that should happen...

-- 
Jesse Barnes, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2014-05-20 19:53           ` Jesse Barnes
@ 2014-05-20 19:58             ` Jesse Barnes
  2014-05-20 20:15               ` Daniel Vetter
  0 siblings, 1 reply; 28+ messages in thread
From: Jesse Barnes @ 2014-05-20 19:58 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: Daniel Vetter, intel-gfx

On Tue, 20 May 2014 12:53:29 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Fri, 16 May 2014 23:42:23 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Fri, May 16, 2014 at 11:38:07PM +0100, Chris Wilson wrote:
> > > On Fri, May 16, 2014 at 03:20:47PM -0700, Jesse Barnes wrote:
> > > > On Mon, 21 Apr 2014 18:37:31 +0200
> > > > Knut Petersen <Knut_Petersen@t-online.de> wrote:
> > > > > > +	/* This driver doesn't need a VT switch to restore the mode on resume */
> > > > > > +	info->skip_vt_switch = true;
> > > > > > +
> > > > > >   	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> > > > > >   	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
> > > > 
> > > > Is it sufficient to just revert this part?  Or are the other bits
> > > > needed too?
> > > > 
> > > > Sorry for the delay on this, I've been traveling a lot the past month
> > > > and buried in other stuff so out of touch with much of my email.
> > > 
> > > The key step here is that X is restarted after resume. The slow down was
> > > due to X not finding any connected outputs and so disabling them all,
> > > setting up a dummy 1024x768 fb which really confused KDE. (KDE queries
> > > the config causing a forced reprobe of all outputs, setups the display
> > > for the native 1280x1024, but screws up KDE's own bookkeeping.)
> > > 
> > > The impact has been fixed by handling the connector->status more
> > > robusting during initial output probing in X. What remains is the
> > > question whether connector->status can be set appropriately upon resume?
> > > It requires a detection cycle to be sure that the outputs are still
> > > there, which is arguably better deferred to userspace. To be consistent
> > > the BIOS take over code should mark connector->status as unknown for the
> > > CRTCs it takes over without doing a detection cycle (where we just
> > > presume that the CRTC/output being enabled means something is on the
> > > other end of the pipe and is still valid).
> > 
> > Hmm. Why didn't fbcon respond to the hotplug event on resume and perform
> > a detection cycle before Knut was able to type startx on the console?
> > That I think is the bug.
> 
> Well, fbcon resume is delayed, but we do perform an fb re-probe via
> intel_resume_hotplug() that should have done this.  Not sure why that's
> sufficient... but I agree userspace should probably re-probe on
> resume.  We're supposed to generate a uevent on resume, but the fb
> layer may suppress that if it detects that no change has occurred.
> 
> Maybe X is racing with our resume_hotplug somehow?  It doesn't look
> like that should happen...

Ah, poll_enable is false until after _thaw finishes, so
our hotplug_resume call of hpd_irq_event does nothing.  So aside from
the encoder hot_plug callbacks (which really just check dp link status,
which ought to be a no-op) our resume_hotplug function doesn't do
anything right now.  May as well kill it and just send an unconditional
uevent?

-- 
Jesse Barnes, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2014-05-20 19:58             ` Jesse Barnes
@ 2014-05-20 20:15               ` Daniel Vetter
  2014-05-20 21:10                 ` Jesse Barnes
  0 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2014-05-20 20:15 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx

On Tue, May 20, 2014 at 9:58 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Ah, poll_enable is false until after _thaw finishes, so
> our hotplug_resume call of hpd_irq_event does nothing.  So aside from
> the encoder hot_plug callbacks (which really just check dp link status,
> which ought to be a no-op) our resume_hotplug function doesn't do
> anything right now.  May as well kill it and just send an unconditional
> uevent?

That's expensive since the reprobe will likely result in a few dropped
frames (if the edid reading takes a long time). Better to do that in
the kernel and avoid sending the uevent if nothing changed. Which iirc
hpd_irq_event does for us.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2014-05-20 20:15               ` Daniel Vetter
@ 2014-05-20 21:10                 ` Jesse Barnes
  2014-05-20 21:18                   ` Jesse Barnes
  0 siblings, 1 reply; 28+ messages in thread
From: Jesse Barnes @ 2014-05-20 21:10 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Tue, 20 May 2014 22:15:45 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> On Tue, May 20, 2014 at 9:58 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Ah, poll_enable is false until after _thaw finishes, so
> > our hotplug_resume call of hpd_irq_event does nothing.  So aside from
> > the encoder hot_plug callbacks (which really just check dp link status,
> > which ought to be a no-op) our resume_hotplug function doesn't do
> > anything right now.  May as well kill it and just send an unconditional
> > uevent?
> 
> That's expensive since the reprobe will likely result in a few dropped
> frames (if the edid reading takes a long time). Better to do that in
> the kernel and avoid sending the uevent if nothing changed. Which iirc
> hpd_irq_event does for us.

Well delaying the resume by a long time isn't much better, but I guess
we need to fix this somehow.

Knut, does this patch also address your issue?  It should reset the
connector status fields correctly so the X driver doesn't have to work
around things...

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b948c71..e725235 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -623,8 +623,6 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 		 * */
 		intel_hpd_init(dev);
 		dev_priv->enable_hotplug_processing = true;
-		/* Config may have changed between suspend and resume */
-		intel_resume_hotplug(dev);
 	}
 
 	intel_opregion_init(dev);
@@ -694,6 +692,11 @@ int i915_resume(struct drm_device *dev)
 		return ret;
 
 	drm_kms_helper_poll_enable(dev);
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Config may have changed between suspend and resume */
+		intel_resume_hotplug(dev);
+	}
+
 	return 0;
 }

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2014-05-20 21:10                 ` Jesse Barnes
@ 2014-05-20 21:18                   ` Jesse Barnes
  2014-05-20 21:27                     ` Jesse Barnes
  0 siblings, 1 reply; 28+ messages in thread
From: Jesse Barnes @ 2014-05-20 21:18 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: Daniel Vetter, intel-gfx

On Tue, 20 May 2014 14:10:16 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Tue, 20 May 2014 22:15:45 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > On Tue, May 20, 2014 at 9:58 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > Ah, poll_enable is false until after _thaw finishes, so
> > > our hotplug_resume call of hpd_irq_event does nothing.  So aside from
> > > the encoder hot_plug callbacks (which really just check dp link status,
> > > which ought to be a no-op) our resume_hotplug function doesn't do
> > > anything right now.  May as well kill it and just send an unconditional
> > > uevent?
> > 
> > That's expensive since the reprobe will likely result in a few dropped
> > frames (if the edid reading takes a long time). Better to do that in
> > the kernel and avoid sending the uevent if nothing changed. Which iirc
> > hpd_irq_event does for us.
> 
> Well delaying the resume by a long time isn't much better, but I guess
> we need to fix this somehow.
> 
> Knut, does this patch also address your issue?  It should reset the
> connector status fields correctly so the X driver doesn't have to work
> around things...
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b948c71..e725235 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -623,8 +623,6 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>  		 * */
>  		intel_hpd_init(dev);
>  		dev_priv->enable_hotplug_processing = true;
> -		/* Config may have changed between suspend and resume */
> -		intel_resume_hotplug(dev);
>  	}
>  
>  	intel_opregion_init(dev);
> @@ -694,6 +692,11 @@ int i915_resume(struct drm_device *dev)
>  		return ret;
>  
>  	drm_kms_helper_poll_enable(dev);
> +	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +		/* Config may have changed between suspend and resume */
> +		intel_resume_hotplug(dev);
> +	}
> +
>  	return 0;
>  }
>  

Bah nevermind, this won't do anything.  I was thinking that poll_enable
controlled the poll_enabled field, but it doesn't, it just schedules or
cancels the work for it.

So we're still left wondering why the connector status fields are
incorrect on resume in this case...

-- 
Jesse Barnes, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/4] drm/i915: enable VT switchless resume v3
  2014-05-20 21:18                   ` Jesse Barnes
@ 2014-05-20 21:27                     ` Jesse Barnes
  0 siblings, 0 replies; 28+ messages in thread
From: Jesse Barnes @ 2014-05-20 21:27 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: Daniel Vetter, intel-gfx

On Tue, 20 May 2014 14:18:07 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Tue, 20 May 2014 14:10:16 -0700
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > On Tue, 20 May 2014 22:15:45 +0200
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > 
> > > On Tue, May 20, 2014 at 9:58 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > > Ah, poll_enable is false until after _thaw finishes, so
> > > > our hotplug_resume call of hpd_irq_event does nothing.  So aside from
> > > > the encoder hot_plug callbacks (which really just check dp link status,
> > > > which ought to be a no-op) our resume_hotplug function doesn't do
> > > > anything right now.  May as well kill it and just send an unconditional
> > > > uevent?
> > > 
> > > That's expensive since the reprobe will likely result in a few dropped
> > > frames (if the edid reading takes a long time). Better to do that in
> > > the kernel and avoid sending the uevent if nothing changed. Which iirc
> > > hpd_irq_event does for us.
> > 
> > Well delaying the resume by a long time isn't much better, but I guess
> > we need to fix this somehow.
> > 
> > Knut, does this patch also address your issue?  It should reset the
> > connector status fields correctly so the X driver doesn't have to work
> > around things...
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index b948c71..e725235 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -623,8 +623,6 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> >  		 * */
> >  		intel_hpd_init(dev);
> >  		dev_priv->enable_hotplug_processing = true;
> > -		/* Config may have changed between suspend and resume */
> > -		intel_resume_hotplug(dev);
> >  	}
> >  
> >  	intel_opregion_init(dev);
> > @@ -694,6 +692,11 @@ int i915_resume(struct drm_device *dev)
> >  		return ret;
> >  
> >  	drm_kms_helper_poll_enable(dev);
> > +	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > +		/* Config may have changed between suspend and resume */
> > +		intel_resume_hotplug(dev);
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> 
> Bah nevermind, this won't do anything.  I was thinking that poll_enable
> controlled the poll_enabled field, but it doesn't, it just schedules or
> cancels the work for it.
> 
> So we're still left wondering why the connector status fields are
> incorrect on resume in this case...

And on top of that I can't reproduce your issue here on my BYT which
shares many of these code paths.  It could be something i915 specific
where we're not doing teardown correctly...

Knut, can you run testdisplay -i from intel-gpu-tools after you kill
the X server following the resume?  It shows connected for me both on
eDP and HDMI as expected.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2014-05-20 21:27 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-26 16:25 [PATCH 1/4] drm/i915: add sprite restore function v3 Jesse Barnes
2013-03-26 16:25 ` [PATCH 2/4] drm/i915: restore cursor and sprite state when forcing a config restore Jesse Barnes
2013-03-26 16:40   ` Rodrigo Vivi
2013-03-26 16:52   ` Daniel Vetter
2013-03-26 17:07   ` [PATCH 1/2] drm/i915: enable VT switchless resume v3 Jesse Barnes
2013-03-26 17:07     ` [PATCH 2/2] drm/i915: emit a hotplug event on resume Jesse Barnes
2013-03-26 20:25   ` drm/i915: restore cursor and sprite state when forcing a config restore v2 Jesse Barnes
2013-03-26 20:46     ` Daniel Vetter
2013-03-26 20:57       ` Jesse Barnes
2013-03-26 16:25 ` [PATCH 3/4] drm/i915: enable VT switchless resume v3 Jesse Barnes
2013-03-26 16:42   ` Rodrigo Vivi
2013-04-03  9:15   ` Daniel Vetter
2013-04-03 10:54     ` Chris Wilson
2013-04-03 15:13       ` Jesse Barnes
2014-04-21 16:37   ` Knut Petersen
2014-05-16 22:20     ` Jesse Barnes
2014-05-16 22:28       ` Daniel Vetter
2014-05-16 22:38       ` Chris Wilson
2014-05-16 22:42         ` Chris Wilson
2014-05-20 19:53           ` Jesse Barnes
2014-05-20 19:58             ` Jesse Barnes
2014-05-20 20:15               ` Daniel Vetter
2014-05-20 21:10                 ` Jesse Barnes
2014-05-20 21:18                   ` Jesse Barnes
2014-05-20 21:27                     ` Jesse Barnes
2013-03-26 16:25 ` [PATCH 4/4] drm/i915: emit a hotplug event on resume Jesse Barnes
2013-03-26 16:43   ` Rodrigo Vivi
2013-03-26 16:38 ` [PATCH 1/4] drm/i915: add sprite restore function v3 Rodrigo Vivi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox