VT switchless suspend/resume

VT switchless suspend/resume

[Jesse Barnes]

Updated with the fix from Ville.

[PATCH 1/6] drm/i915: don't restore LVDS enable state blindly v2

[Jesse Barnes]

We still rely on a few LVDS bits, but restoring the enable bit can cause
trouble at this point, so don't.

v2: use the right mask to prevent restore (Daniel)
    conditionalize on KMS support (Denial)

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

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 2135f21..c1e02b0 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -255,6 +255,7 @@ static void i915_save_display(struct drm_device *dev)
 static void i915_restore_display(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 mask = 0xffffffff;
 
 	/* Display arbitration */
 	if (INTEL_INFO(dev)->gen <= 4)
@@ -267,10 +268,13 @@ static void i915_restore_display(struct drm_device *dev)
 	if (INTEL_INFO(dev)->gen >= 4 && !HAS_PCH_SPLIT(dev))
 		I915_WRITE(BLC_PWM_CTL2, dev_priv->regfile.saveBLC_PWM_CTL2);
 
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
+		mask = ~LVDS_PORT_EN;
+
 	if (HAS_PCH_SPLIT(dev)) {
-		I915_WRITE(PCH_LVDS, dev_priv->regfile.saveLVDS);
+		I915_WRITE(PCH_LVDS, dev_priv->regfile.saveLVDS & mask);
 	} else if (IS_MOBILE(dev) && !IS_I830(dev))
-		I915_WRITE(LVDS, dev_priv->regfile.saveLVDS);
+		I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
 
 	if (!IS_I830(dev) && !IS_845G(dev) && !HAS_PCH_SPLIT(dev))
 		I915_WRITE(PFIT_CONTROL, dev_priv->regfile.savePFIT_CONTROL);
-- 
1.7.9.5

[PATCH 2/6] drm/i915: add sprite restore function v3

[Jesse Barnes]

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 005a91f..65d3968 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 03cfd62..1862b8f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -438,6 +438,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;
 
@@ -644,6 +653,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

[PATCH 3/6] drm/i915: restore cursor and sprite state when forcing a config restore

[Jesse Barnes]

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 3e6dadf..2bf076e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8985,6 +8985,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;
@@ -9081,8 +9082,20 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 
 	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

[PATCH 4/6] drm/i915: enable VT switchless resume v2

[Jesse Barnes]

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

v2: disable backlight on suspend to prevent premature enablement on resume

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c5b8c81..e76b038 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -492,9 +492,10 @@ 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);
+
+		if (dev_priv->backlight)
+			intel_panel_disable_backlight(dev);
 	}
 
 	i915_save_state(dev);
@@ -569,9 +570,14 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		mutex_unlock(&dev->struct_mutex);
 
 		intel_modeset_init_hw(dev);
-		intel_modeset_setup_hw_state(dev, false);
+
 		drm_irq_install(dev);
 		intel_hpd_init(dev);
+
+		/* Resume the modeset for every activated CRTC */
+		drm_modeset_lock_all(dev);
+		intel_modeset_setup_hw_state(dev, true);
+		drm_modeset_unlock_all(dev);
 	}
 
 	intel_opregion_init(dev);
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 1c510da..987bc33 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -149,6 +149,9 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
 	}
 	info->screen_size = size;
 
+	/* This driver doesn't need a VT switch to restore the mode on resume */
+	info->skip_vt_switch = true;
+
 //	memset(info->screen_base, 0, size);
 
 	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
-- 
1.7.9.5

[PATCH 5/6] drm/i915: emit a hotplug event on resume

[Jesse Barnes]

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 e76b038..1b37eec 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -551,6 +551,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;
@@ -578,6 +596,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		drm_modeset_lock_all(dev);
 		intel_modeset_setup_hw_state(dev, true);
 		drm_modeset_unlock_all(dev);
+
+		intel_resume_hotplug(dev);
 	}
 
 	intel_opregion_init(dev);
-- 
1.7.9.5

[PATCH 6/6] drm/i915: remove disabled memset of framebuffer from intel_fb

[Jesse Barnes]

Commented out and unneeded.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_fb.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 987bc33..f4e0b88 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -152,8 +152,6 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
 	/* This driver doesn't need a VT switch to restore the mode on resume */
 	info->skip_vt_switch = true;
 
-//	memset(info->screen_base, 0, size);
-
 	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

Re: [PATCH 1/6] drm/i915: don't restore LVDS enable state blindly v2

[Paulo Zanoni]

Hi

2013/2/19 Jesse Barnes <jbarnes@virtuousgeek.org>:
> We still rely on a few LVDS bits, but restoring the enable bit can cause
> trouble at this point, so don't.
>
> v2: use the right mask to prevent restore (Daniel)
>     conditionalize on KMS support (Denial)
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Briefly tested on SNB, and it still works.

> ---
>  drivers/gpu/drm/i915/i915_suspend.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 2135f21..c1e02b0 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -255,6 +255,7 @@ static void i915_save_display(struct drm_device *dev)
>  static void i915_restore_display(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       u32 mask = 0xffffffff;
>
>         /* Display arbitration */
>         if (INTEL_INFO(dev)->gen <= 4)
> @@ -267,10 +268,13 @@ static void i915_restore_display(struct drm_device *dev)
>         if (INTEL_INFO(dev)->gen >= 4 && !HAS_PCH_SPLIT(dev))
>                 I915_WRITE(BLC_PWM_CTL2, dev_priv->regfile.saveBLC_PWM_CTL2);
>
> +       if (drm_core_check_feature(dev, DRIVER_MODESET))
> +               mask = ~LVDS_PORT_EN;
> +
>         if (HAS_PCH_SPLIT(dev)) {
> -               I915_WRITE(PCH_LVDS, dev_priv->regfile.saveLVDS);
> +               I915_WRITE(PCH_LVDS, dev_priv->regfile.saveLVDS & mask);
>         } else if (IS_MOBILE(dev) && !IS_I830(dev))
> -               I915_WRITE(LVDS, dev_priv->regfile.saveLVDS);
> +               I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
>
>         if (!IS_I830(dev) && !IS_845G(dev) && !HAS_PCH_SPLIT(dev))
>                 I915_WRITE(PFIT_CONTROL, dev_priv->regfile.savePFIT_CONTROL);
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 6/6] drm/i915: remove disabled memset of framebuffer from intel_fb

[Paulo Zanoni]

Hi

2013/2/19 Jesse Barnes <jbarnes@virtuousgeek.org>:
> Commented out and unneeded.

Oh, "DRM: i915: add mode setting support" added it, already commented.

>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_fb.c |    2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index 987bc33..f4e0b88 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -152,8 +152,6 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
>         /* This driver doesn't need a VT switch to restore the mode on resume */
>         info->skip_vt_switch = true;
>
> -//     memset(info->screen_base, 0, size);
> -
>         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



-- 
Paulo Zanoni

Re: VT switchless suspend/resume

[Paulo Zanoni]

2013/2/19 Jesse Barnes <jbarnes@virtuousgeek.org>:
> Updated with the fix from Ville.

Very briefly tested on SNB (LVDS) and HSW (eDP + DP). Suspend-to-ram +
resume still work.

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



-- 
Paulo Zanoni

Re: [PATCH 1/6] drm/i915: don't restore LVDS enable state blindly v2

[Daniel Vetter]

On Tue, Feb 19, 2013 at 05:39:49PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/2/19 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > We still rely on a few LVDS bits, but restoring the enable bit can cause
> > trouble at this point, so don't.
> >
> > v2: use the right mask to prevent restore (Daniel)
> >     conditionalize on KMS support (Denial)
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Briefly tested on SNB, and it still works.
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: VT switchless suspend/resume

[Paulo Zanoni]

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

Hi

2013/2/19 Paulo Zanoni <przanoni@gmail.com>:
> 2013/2/19 Jesse Barnes <jbarnes@virtuousgeek.org>:
>> Updated with the fix from Ville.
>
> Very briefly tested on SNB (LVDS) and HSW (eDP + DP). Suspend-to-ram +
> resume still work.

So, today I looked at the patches I didn't review last week and they
all looked correct, so I was ready to give a "Reviewed-by" stamp on
all of them. I decided to do one more testing and I noticed that with
this series, dmesg gives me some error messages that don't happen
without the series. On a SNB machine with LVDS+DP, after resume I get
error messages saying that DP link training failed (even though the DP
monitor is working).

The dmesg file is attached.

Thanks,
Paulo

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



-- 
Paulo Zanoni

[-- Attachment #2: dmesg.tar.gz --]
[-- Type: application/x-gzip, Size: 49472 bytes --]

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

Re: VT switchless suspend/resume

[Jesse Barnes]

On Mon, 25 Feb 2013 16:19:53 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> Hi
> 
> 2013/2/19 Paulo Zanoni <przanoni@gmail.com>:
> > 2013/2/19 Jesse Barnes <jbarnes@virtuousgeek.org>:
> >> Updated with the fix from Ville.
> >
> > Very briefly tested on SNB (LVDS) and HSW (eDP + DP). Suspend-to-ram +
> > resume still work.
> 
> So, today I looked at the patches I didn't review last week and they
> all looked correct, so I was ready to give a "Reviewed-by" stamp on
> all of them. I decided to do one more testing and I noticed that with
> this series, dmesg gives me some error messages that don't happen
> without the series. On a SNB machine with LVDS+DP, after resume I get
> error messages saying that DP link training failed (even though the DP
> monitor is working).
> 
> The dmesg file is attached.

Hm ugly... any ideas why?  This really shouldn't have affected
anything; we just do a mode set from the kernel first instead of
waiting on the VT switch...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 6/6] drm/i915: remove disabled memset of framebuffer from intel_fb

[Daniel Vetter]

On Tue, Feb 19, 2013 at 06:17:09PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/2/19 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > Commented out and unneeded.
> 
> Oh, "DRM: i915: add mode setting support" added it, already commented.
> 
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Queued for -next, thanks for the patch. I'll hold off on the other ones
until it's clear what's going on on Paulo's machine.
-Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_fb.c |    2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> > index 987bc33..f4e0b88 100644
> > --- a/drivers/gpu/drm/i915/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/intel_fb.c
> > @@ -152,8 +152,6 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
> >         /* This driver doesn't need a VT switch to restore the mode on resume */
> >         info->skip_vt_switch = true;
> >
> > -//     memset(info->screen_base, 0, size);
> > -
> >         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
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> 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: VT switchless suspend/resume

[Jesse Barnes]

On Mon, 25 Feb 2013 13:31:42 -0800
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Mon, 25 Feb 2013 16:19:53 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > Hi
> > 
> > 2013/2/19 Paulo Zanoni <przanoni@gmail.com>:
> > > 2013/2/19 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > >> Updated with the fix from Ville.
> > >
> > > Very briefly tested on SNB (LVDS) and HSW (eDP + DP). Suspend-to-ram +
> > > resume still work.
> > 
> > So, today I looked at the patches I didn't review last week and they
> > all looked correct, so I was ready to give a "Reviewed-by" stamp on
> > all of them. I decided to do one more testing and I noticed that with
> > this series, dmesg gives me some error messages that don't happen
> > without the series. On a SNB machine with LVDS+DP, after resume I get
> > error messages saying that DP link training failed (even though the DP
> > monitor is working).
> > 
> > The dmesg file is attached.
> 
> Hm ugly... any ideas why?  This really shouldn't have affected
> anything; we just do a mode set from the kernel first instead of
> waiting on the VT switch...

Looks like it's trying to restore a 2 pipe config?  Is that correct?
The BIOS re-enabled VGA across suspend/resume too, I guess that would
cause trouble...  but the check code says both CRTCs are disabled at
resume.

So first a DP train appears to fail, then we fail waiting for a pipe to
disable because we try another, single pipe mode set.

Which is the proper config?  One pipe or two?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/6] drm/i915: enable VT switchless resume v2

[Daniel Vetter]

On Tue, Feb 19, 2013 at 12:11:41PM -0800, 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
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |   12 +++++++++---
>  drivers/gpu/drm/i915/intel_fb.c |    3 +++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index c5b8c81..e76b038 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -492,9 +492,10 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
>  
> -		intel_modeset_disable(dev);

As discussed in person last week, simply dropping this will probably kill
S0i3 support.
-Daniel

> -
>  		drm_irq_uninstall(dev);
> +
> +		if (dev_priv->backlight)
> +			intel_panel_disable_backlight(dev);
>  	}
>  
>  	i915_save_state(dev);
> @@ -569,9 +570,14 @@ static int __i915_drm_thaw(struct drm_device *dev)
>  		mutex_unlock(&dev->struct_mutex);
>  
>  		intel_modeset_init_hw(dev);
> -		intel_modeset_setup_hw_state(dev, false);
> +
>  		drm_irq_install(dev);
>  		intel_hpd_init(dev);
> +
> +		/* Resume the modeset for every activated CRTC */
> +		drm_modeset_lock_all(dev);
> +		intel_modeset_setup_hw_state(dev, true);
> +		drm_modeset_unlock_all(dev);
>  	}
>  
>  	intel_opregion_init(dev);
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index 1c510da..987bc33 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -149,6 +149,9 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
>  	}
>  	info->screen_size = size;
>  
> +	/* This driver doesn't need a VT switch to restore the mode on resume */
> +	info->skip_vt_switch = true;
> +
>  //	memset(info->screen_base, 0, size);
>  
>  	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> -- 
> 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 4/6] drm/i915: enable VT switchless resume v2

[Jesse Barnes]

On Mon, 18 Mar 2013 08:49:07 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Feb 19, 2013 at 12:11:41PM -0800, 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
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c |   12 +++++++++---
> >  drivers/gpu/drm/i915/intel_fb.c |    3 +++
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index c5b8c81..e76b038 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -492,9 +492,10 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  
> >  		cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
> >  
> > -		intel_modeset_disable(dev);
> 
> As discussed in person last week, simply dropping this will probably kill
> S0i3 support.

Not really, since DPMS will be off in that case too generally, but it
does make testing harder.

I think it just needs to be a low level call to crtc disable on each
pipe, otherwise we'll zap the state we're trying to save.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/6] drm/i915: enable VT switchless resume v2

[Daniel Vetter]

On Mon, Mar 18, 2013 at 6:42 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Mon, 18 Mar 2013 08:49:07 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> On Tue, Feb 19, 2013 at 12:11:41PM -0800, 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
>> >
>> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.c |   12 +++++++++---
>> >  drivers/gpu/drm/i915/intel_fb.c |    3 +++
>> >  2 files changed, 12 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> > index c5b8c81..e76b038 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.c
>> > +++ b/drivers/gpu/drm/i915/i915_drv.c
>> > @@ -492,9 +492,10 @@ static int i915_drm_freeze(struct drm_device *dev)
>> >
>> >             cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
>> >
>> > -           intel_modeset_disable(dev);
>>
>> As discussed in person last week, simply dropping this will probably kill
>> S0i3 support.
>
> Not really, since DPMS will be off in that case too generally, but it
> does make testing harder.

Hm, where do we do that currently? Without fbcon I don't see it, and
we can't really rely on userspace to get this right I guess ...

> I think it just needs to be a low level call to crtc disable on each
> pipe, otherwise we'll zap the state we're trying to save.

That just reminded me that we also should restore the right dpms state
I think. At least I'm not too sure whether we'll currently do that
(and whether the modeset state tracker would catch it). Otoh dpms
standby/suspend died with gen4 ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/6] drm/i915: enable VT switchless resume v2

[Jesse Barnes]

On Mon, 18 Mar 2013 19:50:37 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Mar 18, 2013 at 6:42 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Mon, 18 Mar 2013 08:49:07 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> >> On Tue, Feb 19, 2013 at 12:11:41PM -0800, 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
> >> >
> >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_drv.c |   12 +++++++++---
> >> >  drivers/gpu/drm/i915/intel_fb.c |    3 +++
> >> >  2 files changed, 12 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> > index c5b8c81..e76b038 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.c
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> > @@ -492,9 +492,10 @@ static int i915_drm_freeze(struct drm_device *dev)
> >> >
> >> >             cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
> >> >
> >> > -           intel_modeset_disable(dev);
> >>
> >> As discussed in person last week, simply dropping this will probably kill
> >> S0i3 support.
> >
> > Not really, since DPMS will be off in that case too generally, but it
> > does make testing harder.
> 
> Hm, where do we do that currently? Without fbcon I don't see it, and
> we can't really rely on userspace to get this right I guess ...

I was assuming userspace would do it, but yes we shouldn't require that.

> > I think it just needs to be a low level call to crtc disable on each
> > pipe, otherwise we'll zap the state we're trying to save.
> 
> That just reminded me that we also should restore the right dpms state
> I think. At least I'm not too sure whether we'll currently do that
> (and whether the modeset state tracker would catch it). Otoh dpms
> standby/suspend died with gen4 ;-)

Hm yeah haven't tested that at all.  One typical kind of suspend will
happen after DPMS off when the machine has been idle for some period.
When it comes back up the user will probably want to see the display.
But we don't have to enforce that on the kernel side; we can leave it
to userspace.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/6] drm/i915: enable VT switchless resume v2

[Daniel Vetter]

On Tue, Mar 19, 2013 at 5:56 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> > I think it just needs to be a low level call to crtc disable on each
>> > pipe, otherwise we'll zap the state we're trying to save.
>>
>> That just reminded me that we also should restore the right dpms state
>> I think. At least I'm not too sure whether we'll currently do that
>> (and whether the modeset state tracker would catch it). Otoh dpms
>> standby/suspend died with gen4 ;-)
>
> Hm yeah haven't tested that at all.  One typical kind of suspend will
> happen after DPMS off when the machine has been idle for some period.
> When it comes back up the user will probably want to see the display.
> But we don't have to enforce that on the kernel side; we can leave it
> to userspace.

Note that this isn't about dpms state in general, we'll take care of
that. The problem is with intermediate dpms levels, which requires us
to keep the pipe partially running. If you force-restore such a thing
we'll end up at dpms ON. Which isn't quite what we want.

Otoh it's old hw, so I don't think we need to spill too many brain
cycles on this issue. But if we do fix it, I think we should implement
proper support to read out that hw state and cross-check it -
otherwise it's pretty much guaranteed to be broken.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/6] drm/i915: enable VT switchless resume v2

[Daniel Vetter]

On Tue, Mar 19, 2013 at 06:13:09PM +0100, Daniel Vetter wrote:
> On Tue, Mar 19, 2013 at 5:56 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> > I think it just needs to be a low level call to crtc disable on each
> >> > pipe, otherwise we'll zap the state we're trying to save.
> >>
> >> That just reminded me that we also should restore the right dpms state
> >> I think. At least I'm not too sure whether we'll currently do that
> >> (and whether the modeset state tracker would catch it). Otoh dpms
> >> standby/suspend died with gen4 ;-)
> >
> > Hm yeah haven't tested that at all.  One typical kind of suspend will
> > happen after DPMS off when the machine has been idle for some period.
> > When it comes back up the user will probably want to see the display.
> > But we don't have to enforce that on the kernel side; we can leave it
> > to userspace.
> 
> Note that this isn't about dpms state in general, we'll take care of
> that. The problem is with intermediate dpms levels, which requires us
> to keep the pipe partially running. If you force-restore such a thing
> we'll end up at dpms ON. Which isn't quite what we want.
> 
> Otoh it's old hw, so I don't think we need to spill too many brain
> cycles on this issue. But if we do fix it, I think we should implement
> proper support to read out that hw state and cross-check it -
> otherwise it's pretty much guaranteed to be broken.

Ajax just submitted a patch to fix restoring of intermediate dpms levels,
so we should be good here. Another idea for safe/restore around suspend
which should just work is loop over all connectors and adjust the dpms
state (and safe just that piece somewhere). That way we get nice implicit
coverag of our dpms code while doing suspends and suspend doesn't need
anything in addition infrastructure wise.

The only downside is that we need to make the power wells stuff work with
dpms, too. But that's a requirement, anyway.

Now the real reason for writing this mail: 3.10 feature merge is drawing
to a close (in about a month I think) and imo we should get switchless
resume in a few weeks ahead. That way we can give it a decent beating
before it reaches Linus' upstream git. And I like switchless resume a lot.

So what's your plan here?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/6] drm/i915: enable VT switchless resume v2

[Jesse Barnes]

On Tue, 26 Mar 2013 13:14:48 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Mar 19, 2013 at 06:13:09PM +0100, Daniel Vetter wrote:
> > On Tue, Mar 19, 2013 at 5:56 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > >> > I think it just needs to be a low level call to crtc disable on each
> > >> > pipe, otherwise we'll zap the state we're trying to save.
> > >>
> > >> That just reminded me that we also should restore the right dpms state
> > >> I think. At least I'm not too sure whether we'll currently do that
> > >> (and whether the modeset state tracker would catch it). Otoh dpms
> > >> standby/suspend died with gen4 ;-)
> > >
> > > Hm yeah haven't tested that at all.  One typical kind of suspend will
> > > happen after DPMS off when the machine has been idle for some period.
> > > When it comes back up the user will probably want to see the display.
> > > But we don't have to enforce that on the kernel side; we can leave it
> > > to userspace.
> > 
> > Note that this isn't about dpms state in general, we'll take care of
> > that. The problem is with intermediate dpms levels, which requires us
> > to keep the pipe partially running. If you force-restore such a thing
> > we'll end up at dpms ON. Which isn't quite what we want.
> > 
> > Otoh it's old hw, so I don't think we need to spill too many brain
> > cycles on this issue. But if we do fix it, I think we should implement
> > proper support to read out that hw state and cross-check it -
> > otherwise it's pretty much guaranteed to be broken.
> 
> Ajax just submitted a patch to fix restoring of intermediate dpms levels,
> so we should be good here. Another idea for safe/restore around suspend
> which should just work is loop over all connectors and adjust the dpms
> state (and safe just that piece somewhere). That way we get nice implicit
> coverag of our dpms code while doing suspends and suspend doesn't need
> anything in addition infrastructure wise.
> 
> The only downside is that we need to make the power wells stuff work with
> dpms, too. But that's a requirement, anyway.
> 
> Now the real reason for writing this mail: 3.10 feature merge is drawing
> to a close (in about a month I think) and imo we should get switchless
> resume in a few weeks ahead. That way we can give it a decent beating
> before it reaches Linus' upstream git. And I like switchless resume a lot.
> 
> So what's your plan here?

What's left?  Just dealing with the shut down at suspend time (so that
suspend testing and RTD3 works) and Paulo's issue?

I'm not sure what to do about the latter, but I can spin up a new patch
with the shutdown bits in place.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 4/6] drm/i915: enable VT switchless resume v2

[Jesse Barnes]

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

v2: disable backlight on suspend to prevent premature enablement on resume

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c5b8c81..e76b038 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -492,9 +492,10 @@ 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);
+
+		if (dev_priv->backlight)
+			intel_panel_disable_backlight(dev);
 	}
 
 	i915_save_state(dev);
@@ -569,9 +570,14 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		mutex_unlock(&dev->struct_mutex);
 
 		intel_modeset_init_hw(dev);
-		intel_modeset_setup_hw_state(dev, false);
+
 		drm_irq_install(dev);
 		intel_hpd_init(dev);
+
+		/* Resume the modeset for every activated CRTC */
+		drm_modeset_lock_all(dev);
+		intel_modeset_setup_hw_state(dev, true);
+		drm_modeset_unlock_all(dev);
 	}
 
 	intel_opregion_init(dev);
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 1c510da..987bc33 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -149,6 +149,9 @@ static int intelfb_create(struct intel_fbdev *ifbdev,
 	}
 	info->screen_size = size;
 
+	/* This driver doesn't need a VT switch to restore the mode on resume */
+	info->skip_vt_switch = true;
+
 //	memset(info->screen_base, 0, size);
 
 	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
-- 
1.7.9.5