[PATCH 00/11] drm/i915: VGA vs. power well, round 2

[PATCH 00/11] drm/i915: VGA vs. power well, round 2

[ville.syrjala]

The first round ended without a knowckout, so here's round two.

Changes:
- Split the i915_request handling change to a separate patch
- Added intel_resume_power_well() to force the well on during resume
- Renamed the inc/dec funcs as suggested by Chris
- Moved power well init earlier during init and resume
- Move the redisable_vga thing to happen before modeset on resume
- Call intel_uncore_early_sanitize() during resume
- Killed some extra plane restore calls we no longer need

My HSW machine now boots and does a hibernate+resume cycle w/o unclaimed
register warnings, and it also tells me that it re-disables VGA during
resume. I guess the BIOS has left it on at that point. The BIOS doesn't
support S3 so I couldn't test that one.

[PATCH v2 01/11] drm/i915: Change i915_request power well handling

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reorganize the internal i915_request power well handling to use the
reference count just like everyone else. This way all we need to do is
check the reference count and we know whether the power well needs to be
enabled of disabled.

v2: Split he intel_display_power_{get,put} change to another patch.
    Add intel_resume_power_well() to make sure we enable the power
    well on resume

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 43 +++++++++++++++++++++++++++++++---------
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 774ebb6..8853f53 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -765,6 +765,7 @@ extern bool intel_display_power_enabled(struct drm_device *dev,
 					enum intel_display_power_domain domain);
 extern void intel_init_power_well(struct drm_device *dev);
 extern void intel_set_power_well(struct drm_device *dev, bool enable);
+extern void intel_resume_power_well(struct drm_device *dev);
 extern void intel_enable_gt_powersave(struct drm_device *dev);
 extern void intel_disable_gt_powersave(struct drm_device *dev);
 extern void ironlake_teardown_rc6(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8cffef4..310d2ed 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5342,8 +5342,7 @@ void i915_request_power_well(void)
 		return;
 
 	spin_lock_irq(&hsw_pwr->lock);
-	if (!hsw_pwr->count++ &&
-			!hsw_pwr->i915_request)
+	if (!hsw_pwr->count++)
 		__intel_set_power_well(hsw_pwr->device, true);
 	spin_unlock_irq(&hsw_pwr->lock);
 }
@@ -5357,8 +5356,7 @@ void i915_release_power_well(void)
 
 	spin_lock_irq(&hsw_pwr->lock);
 	WARN_ON(!hsw_pwr->count);
-	if (!--hsw_pwr->count &&
-		       !hsw_pwr->i915_request)
+	if (!--hsw_pwr->count)
 		__intel_set_power_well(hsw_pwr->device, false);
 	spin_unlock_irq(&hsw_pwr->lock);
 }
@@ -5394,15 +5392,41 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 		return;
 
 	spin_lock_irq(&power_well->lock);
+
+	/*
+	 * This function will only ever contribute one
+	 * to the power well reference count. i915_request
+	 * is what tracks whether we have or have not
+	 * added the one to the reference count.
+	 */
+	if (power_well->i915_request == enable)
+		goto out;
+
 	power_well->i915_request = enable;
 
-	/* only reject "disable" power well request */
-	if (power_well->count && !enable) {
-		spin_unlock_irq(&power_well->lock);
-		return;
+	if (enable) {
+		if (!power_well->count++)
+			__intel_set_power_well(dev, true);
+	} else {
+		WARN_ON(!power_well->count);
+		if (!--power_well->count)
+			__intel_set_power_well(dev, false);
 	}
 
-	__intel_set_power_well(dev, enable);
+ out:
+	spin_unlock_irq(&power_well->lock);
+}
+
+void intel_resume_power_well(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_well *power_well = &dev_priv->power_well;
+
+	if (!HAS_POWER_WELL(dev))
+		return;
+
+	spin_lock_irq(&power_well->lock);
+	__intel_set_power_well(dev, power_well->count > 0);
 	spin_unlock_irq(&power_well->lock);
 }
 
@@ -5421,6 +5445,7 @@ void intel_init_power_well(struct drm_device *dev)
 
 	/* For now, we need the power well to be always enabled. */
 	intel_set_power_well(dev, true);
+	intel_resume_power_well(dev);
 
 	/* We're taking over the BIOS, so clear any requests made by it since
 	 * the driver is in charge now. */
-- 
1.8.1.5

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

[PATCH v2 02/11] drm/i915: Add intel_display_power_{get, put} to request power for specific domains

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add APIs to get/put power well references for specific purposes.

v2: Split the i915_request change to another patch

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  4 +++
 drivers/gpu/drm/i915/intel_pm.c  | 63 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8853f53..7a23911 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -763,6 +763,10 @@ extern void i915_remove_power_well(struct drm_device *dev);
 
 extern bool intel_display_power_enabled(struct drm_device *dev,
 					enum intel_display_power_domain domain);
+extern void intel_display_power_get(struct drm_device *dev,
+				    enum intel_display_power_domain domain);
+extern void intel_display_power_put(struct drm_device *dev,
+				    enum intel_display_power_domain domain);
 extern void intel_init_power_well(struct drm_device *dev);
 extern void intel_set_power_well(struct drm_device *dev, bool enable);
 extern void intel_resume_power_well(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 310d2ed..de7b3b1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5333,6 +5333,69 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
 	}
 }
 
+void intel_display_power_get(struct drm_device *dev,
+			     enum intel_display_power_domain domain)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_well *power_well = &dev_priv->power_well;
+
+	if (!HAS_POWER_WELL(dev))
+		return;
+
+	switch (domain) {
+	case POWER_DOMAIN_PIPE_A:
+	case POWER_DOMAIN_TRANSCODER_EDP:
+		return;
+	case POWER_DOMAIN_PIPE_B:
+	case POWER_DOMAIN_PIPE_C:
+	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
+	case POWER_DOMAIN_TRANSCODER_A:
+	case POWER_DOMAIN_TRANSCODER_B:
+	case POWER_DOMAIN_TRANSCODER_C:
+		spin_lock_irq(&power_well->lock);
+		if (!power_well->count++)
+			__intel_set_power_well(power_well->device, true);
+		spin_unlock_irq(&power_well->lock);
+		return;
+	default:
+		BUG();
+	}
+}
+
+void intel_display_power_put(struct drm_device *dev,
+			     enum intel_display_power_domain domain)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_well *power_well = &dev_priv->power_well;
+
+	if (!HAS_POWER_WELL(dev))
+		return;
+
+	switch (domain) {
+	case POWER_DOMAIN_PIPE_A:
+	case POWER_DOMAIN_TRANSCODER_EDP:
+		return;
+	case POWER_DOMAIN_PIPE_B:
+	case POWER_DOMAIN_PIPE_C:
+	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
+	case POWER_DOMAIN_TRANSCODER_A:
+	case POWER_DOMAIN_TRANSCODER_B:
+	case POWER_DOMAIN_TRANSCODER_C:
+		spin_lock_irq(&power_well->lock);
+		WARN_ON(!power_well->count);
+		if (!--power_well->count)
+			__intel_set_power_well(power_well->device, false);
+		spin_unlock_irq(&power_well->lock);
+		return;
+	default:
+		BUG();
+	}
+}
+
 static struct i915_power_well *hsw_pwr;
 
 /* Display audio driver power well request */
-- 
1.8.1.5

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

[PATCH v2 03/11] drm/i915: Refactor power well refcount inc/dec operations

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We increase/decrease the power well refcount in several places now, and
all of those places need to do the same thing, so pull that code into
a few small helper functions.

v2: Rename the funcs to __intel_power_well_{get,put}

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index de7b3b1..cb65326 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5333,6 +5333,19 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
 	}
 }
 
+static void __intel_power_well_get(struct i915_power_well *power_well)
+{
+	if (!power_well->count++)
+		__intel_set_power_well(power_well->device, true);
+}
+
+static void __intel_power_well_put(struct i915_power_well *power_well)
+{
+	WARN_ON(!power_well->count);
+	if (!--power_well->count)
+		__intel_set_power_well(power_well->device, false);
+}
+
 void intel_display_power_get(struct drm_device *dev,
 			     enum intel_display_power_domain domain)
 {
@@ -5355,8 +5368,7 @@ void intel_display_power_get(struct drm_device *dev,
 	case POWER_DOMAIN_TRANSCODER_B:
 	case POWER_DOMAIN_TRANSCODER_C:
 		spin_lock_irq(&power_well->lock);
-		if (!power_well->count++)
-			__intel_set_power_well(power_well->device, true);
+		__intel_power_well_get(power_well);
 		spin_unlock_irq(&power_well->lock);
 		return;
 	default:
@@ -5386,9 +5398,7 @@ void intel_display_power_put(struct drm_device *dev,
 	case POWER_DOMAIN_TRANSCODER_B:
 	case POWER_DOMAIN_TRANSCODER_C:
 		spin_lock_irq(&power_well->lock);
-		WARN_ON(!power_well->count);
-		if (!--power_well->count)
-			__intel_set_power_well(power_well->device, false);
+		__intel_power_well_put(power_well);
 		spin_unlock_irq(&power_well->lock);
 		return;
 	default:
@@ -5405,8 +5415,7 @@ void i915_request_power_well(void)
 		return;
 
 	spin_lock_irq(&hsw_pwr->lock);
-	if (!hsw_pwr->count++)
-		__intel_set_power_well(hsw_pwr->device, true);
+	__intel_power_well_get(hsw_pwr);
 	spin_unlock_irq(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_request_power_well);
@@ -5418,9 +5427,7 @@ void i915_release_power_well(void)
 		return;
 
 	spin_lock_irq(&hsw_pwr->lock);
-	WARN_ON(!hsw_pwr->count);
-	if (!--hsw_pwr->count)
-		__intel_set_power_well(hsw_pwr->device, false);
+	__intel_power_well_put(hsw_pwr);
 	spin_unlock_irq(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
@@ -5467,14 +5474,10 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 
 	power_well->i915_request = enable;
 
-	if (enable) {
-		if (!power_well->count++)
-			__intel_set_power_well(dev, true);
-	} else {
-		WARN_ON(!power_well->count);
-		if (!--power_well->count)
-			__intel_set_power_well(dev, false);
-	}
+	if (enable)
+		__intel_power_well_get(power_well);
+	else
+		__intel_power_well_put(power_well);
 
  out:
 	spin_unlock_irq(&power_well->lock);
-- 
1.8.1.5

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

[PATCH 04/11] drm/i915: Add POWER_DOMAIN_VGA

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

VGA registers/memory live inside the the display power well. Add a power
domain for VGA.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 1 +
 drivers/gpu/drm/i915/intel_pm.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 152291c..54b1966 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -99,6 +99,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_TRANSCODER_B,
 	POWER_DOMAIN_TRANSCODER_C,
 	POWER_DOMAIN_TRANSCODER_EDP = POWER_DOMAIN_TRANSCODER_A + 0xF,
+	POWER_DOMAIN_VGA,
 };
 
 #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index cb65326..5c4922f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5271,6 +5271,7 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	case POWER_DOMAIN_PIPE_A:
 	case POWER_DOMAIN_TRANSCODER_EDP:
 		return true;
+	case POWER_DOMAIN_VGA:
 	case POWER_DOMAIN_PIPE_B:
 	case POWER_DOMAIN_PIPE_C:
 	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
@@ -5359,6 +5360,7 @@ void intel_display_power_get(struct drm_device *dev,
 	case POWER_DOMAIN_PIPE_A:
 	case POWER_DOMAIN_TRANSCODER_EDP:
 		return;
+	case POWER_DOMAIN_VGA:
 	case POWER_DOMAIN_PIPE_B:
 	case POWER_DOMAIN_PIPE_C:
 	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
@@ -5389,6 +5391,7 @@ void intel_display_power_put(struct drm_device *dev,
 	case POWER_DOMAIN_PIPE_A:
 	case POWER_DOMAIN_TRANSCODER_EDP:
 		return;
+	case POWER_DOMAIN_VGA:
 	case POWER_DOMAIN_PIPE_B:
 	case POWER_DOMAIN_PIPE_C:
 	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
-- 
1.8.1.5

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

[PATCH 05/11] drm/i915: Pull intel_init_power_well() out of intel_modeset_init_hw()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The init and resume codepaths want to handel the power well in slightly
different ways, so pull the power well init out from
intel_modeset_init_hw() which gets called in both cases.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      | 2 ++
 drivers/gpu/drm/i915/i915_drv.c      | 2 ++
 drivers/gpu/drm/i915/intel_display.c | 2 --
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 9b265a4..e5c7b10 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1324,6 +1324,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
 
+	intel_init_power_well(dev);
+
 	intel_modeset_gem_init(dev);
 
 	/* Always safe in the mode setting case. */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ec690ca..cd5a66d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -596,6 +596,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		/* We need working interrupts for modeset enabling ... */
 		drm_irq_install(dev);
 
+		intel_init_power_well(dev);
+
 		intel_modeset_init_hw(dev);
 
 		drm_modeset_lock_all(dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 60284fc..8162cd1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10275,8 +10275,6 @@ void i915_disable_vga_mem(struct drm_device *dev)
 
 void intel_modeset_init_hw(struct drm_device *dev)
 {
-	intel_init_power_well(dev);
-
 	intel_prepare_ddi(dev);
 
 	intel_init_clock_gating(dev);
-- 
1.8.1.5

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

[PATCH 06/11] drm/i915: Fix unclaimed register access due to delayed VGA memroy disable

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

VGA registers live inside the power well on HSW, so in order to write
the VGA MSR register we need the power well to be on.

We really must write to the register to properly clear the
VGA_MSR_MEM_EN enable bit, even if all VGA registers get zeroed when
the power well is down. It seems that the implicit zeroing done by
the power well is not enough to propagate the VGA_MSR_MEM_EN bit to
whomever is actually responsible for the memory decode ranges.

If we leave VGA memory decode enabled, and then turn off the power well,
all VGA memory reads will return zeroes. But if we first disable VGA
memory deocde and then turn off the power well, VGA memory reads
return all ones, indicating that the access wasn't claimed by anyone.
For the vga arbiter to function correctly the IGD must not claim the
VGA memory accesses.

Previously we were doing the VGA_MSR register access while the power well
was excplicitly powered up during driver init. But ever since

 commit 6e1b4fdad5157bb9e88777d525704aba24389bee
 Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
 Date:   Thu Sep 5 20:40:52 2013 +0300

    drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff is done

we delay the VGA memory disable until fbcon has initialized, and so
there's a possibility that the power well got turned off during the
fbcon modeset. Also vgacon_save_screen() will need the power well to be
on to be able to read the VGA memory.

So immediately after enabling the power well during init grab a refence
for VGA purposes, and after all the VGA handling is done, release it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e5c7b10..23f965d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1326,6 +1326,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_init_power_well(dev);
 
+	/* Keep VGA alive until i915_disable_vga_mem() */
+	intel_display_power_get(dev, POWER_DOMAIN_VGA);
+
 	intel_modeset_gem_init(dev);
 
 	/* Always safe in the mode setting case. */
@@ -1358,6 +1361,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	 * vgacon_save_screen() works during the handover.
 	 */
 	i915_disable_vga_mem(dev);
+	intel_display_power_put(dev, POWER_DOMAIN_VGA);
 
 	/* Only enable hotplug handling once the fbdev is fully set up. */
 	dev_priv->enable_hotplug_processing = true;
-- 
1.8.1.5

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

[PATCH 07/11] drm/i915: Redisable VGA before the modeset on resume

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The VGA plane needs to be disabled before we start doing any
modeset operations on resume.

This should also guarantee that the power well will be enabled
when we call i915_redisable_vga() since it gets explicitly powered on
during resume, and will get powered back off during the modeset
operation if no longer needed.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8162cd1..c9093bb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10689,6 +10689,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	}
 
 	if (force_restore) {
+		i915_redisable_vga(dev);
+
 		/*
 		 * We need to use raw interfaces for restoring state to avoid
 		 * checking (bogus) intermediate states.
@@ -10702,8 +10704,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		}
 		list_for_each_entry(plane, &dev->mode_config.plane_list, head)
 			intel_plane_restore(plane);
-
-		i915_redisable_vga(dev);
 	} else {
 		intel_modeset_update_staged_output_state(dev);
 	}
-- 
1.8.1.5

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

[PATCH 08/11] drm/i915: Move power well init earlier during driver load

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

intel_modeset_init() will already attempt to disable VGA. In order to do
that, it needs the power well to be on. So move the power well init
to happen before intel_modeset_init() during driver load.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 23f965d..8a92eb8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1314,21 +1314,21 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_gem_stolen;
 
+	intel_init_power_well(dev);
+
+	/* Keep VGA alive until i915_disable_vga_mem() */
+	intel_display_power_get(dev, POWER_DOMAIN_VGA);
+
 	/* Important: The output setup functions called by modeset_init need
 	 * working irqs for e.g. gmbus and dp aux transfers. */
 	intel_modeset_init(dev);
 
 	ret = i915_gem_init(dev);
 	if (ret)
-		goto cleanup_irq;
+		goto cleanup_power;
 
 	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
 
-	intel_init_power_well(dev);
-
-	/* Keep VGA alive until i915_disable_vga_mem() */
-	intel_display_power_get(dev, POWER_DOMAIN_VGA);
-
 	intel_modeset_gem_init(dev);
 
 	/* Always safe in the mode setting case. */
@@ -1377,7 +1377,8 @@ cleanup_gem:
 	mutex_unlock(&dev->struct_mutex);
 	i915_gem_cleanup_aliasing_ppgtt(dev);
 	drm_mm_takedown(&dev_priv->gtt.base.mm);
-cleanup_irq:
+cleanup_power:
+	intel_display_power_put(dev, POWER_DOMAIN_VGA);
 	drm_irq_uninstall(dev);
 cleanup_gem_stolen:
 	i915_gem_cleanup_stolen(dev);
-- 
1.8.1.5

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

[PATCH 09/11] drm/i915: Move power well resume earlier

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

i915_restore_state() -> i915_restore_display() will attempt to
re-disable VGA during resume. So the power well needs to be powered on
before that.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index cd5a66d..fdf3eef 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -581,6 +581,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int error = 0;
 
+	intel_init_power_well(dev);
+
 	i915_restore_state(dev);
 	intel_opregion_setup(dev);
 
@@ -596,8 +598,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		/* We need working interrupts for modeset enabling ... */
 		drm_irq_install(dev);
 
-		intel_init_power_well(dev);
-
 		intel_modeset_init_hw(dev);
 
 		drm_modeset_lock_all(dev);
-- 
1.8.1.5

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

[PATCH 10/11] drm/i915: Call intel_uncore_early_sanitize() during resume

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Call intel_uncore_early_sanitize() first thing during resume to prevent
stale BIOS leftovers from being reported as unclaimed register access.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index fdf3eef..3d25731 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -670,6 +670,8 @@ int i915_resume(struct drm_device *dev)
 
 	pci_set_master(dev->pdev);
 
+	intel_uncore_early_sanitize(dev);
+
 	intel_uncore_sanitize(dev);
 
 	/*
-- 
1.8.1.5

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

[PATCH 11/11] drm/i915: Drop explicit plane restoration during resume

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We already restore planes during the modeset operation, so no need to do
another loop over the planes and try to restore them again.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c9093bb..37a470f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10641,7 +10641,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe;
-	struct drm_plane *plane;
 	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
 	int i;
@@ -10702,8 +10701,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 			__intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
 					 crtc->fb);
 		}
-		list_for_each_entry(plane, &dev->mode_config.plane_list, head)
-			intel_plane_restore(plane);
 	} else {
 		intel_modeset_update_staged_output_state(dev);
 	}
-- 
1.8.1.5

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

Re: [PATCH v2 01/11] drm/i915: Change i915_request power well handling

[Paulo Zanoni]

2013/9/16  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Reorganize the internal i915_request power well handling to use the
> reference count just like everyone else. This way all we need to do is
> check the reference count and we know whether the power well needs to be
> enabled of disabled.
>
> v2: Split he intel_display_power_{get,put} change to another patch.
>     Add intel_resume_power_well() to make sure we enable the power
>     well on resume
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 43 +++++++++++++++++++++++++++++++---------
>  2 files changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 774ebb6..8853f53 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -765,6 +765,7 @@ extern bool intel_display_power_enabled(struct drm_device *dev,
>                                         enum intel_display_power_domain domain);
>  extern void intel_init_power_well(struct drm_device *dev);
>  extern void intel_set_power_well(struct drm_device *dev, bool enable);
> +extern void intel_resume_power_well(struct drm_device *dev);
>  extern void intel_enable_gt_powersave(struct drm_device *dev);
>  extern void intel_disable_gt_powersave(struct drm_device *dev);
>  extern void ironlake_teardown_rc6(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8cffef4..310d2ed 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5342,8 +5342,7 @@ void i915_request_power_well(void)
>                 return;
>
>         spin_lock_irq(&hsw_pwr->lock);
> -       if (!hsw_pwr->count++ &&
> -                       !hsw_pwr->i915_request)
> +       if (!hsw_pwr->count++)
>                 __intel_set_power_well(hsw_pwr->device, true);
>         spin_unlock_irq(&hsw_pwr->lock);
>  }
> @@ -5357,8 +5356,7 @@ void i915_release_power_well(void)
>
>         spin_lock_irq(&hsw_pwr->lock);
>         WARN_ON(!hsw_pwr->count);
> -       if (!--hsw_pwr->count &&
> -                      !hsw_pwr->i915_request)
> +       if (!--hsw_pwr->count)
>                 __intel_set_power_well(hsw_pwr->device, false);
>         spin_unlock_irq(&hsw_pwr->lock);
>  }
> @@ -5394,15 +5392,41 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>                 return;
>
>         spin_lock_irq(&power_well->lock);
> +
> +       /*
> +        * This function will only ever contribute one
> +        * to the power well reference count. i915_request
> +        * is what tracks whether we have or have not
> +        * added the one to the reference count.
> +        */
> +       if (power_well->i915_request == enable)
> +               goto out;
> +
>         power_well->i915_request = enable;
>
> -       /* only reject "disable" power well request */
> -       if (power_well->count && !enable) {
> -               spin_unlock_irq(&power_well->lock);
> -               return;
> +       if (enable) {
> +               if (!power_well->count++)
> +                       __intel_set_power_well(dev, true);
> +       } else {
> +               WARN_ON(!power_well->count);
> +               if (!--power_well->count)
> +                       __intel_set_power_well(dev, false);
>         }
>
> -       __intel_set_power_well(dev, enable);
> + out:
> +       spin_unlock_irq(&power_well->lock);
> +}
> +
> +void intel_resume_power_well(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_power_well *power_well = &dev_priv->power_well;
> +
> +       if (!HAS_POWER_WELL(dev))
> +               return;
> +
> +       spin_lock_irq(&power_well->lock);
> +       __intel_set_power_well(dev, power_well->count > 0);
>         spin_unlock_irq(&power_well->lock);
>  }
>
> @@ -5421,6 +5445,7 @@ void intel_init_power_well(struct drm_device *dev)
>
>         /* For now, we need the power well to be always enabled. */
>         intel_set_power_well(dev, true);
> +       intel_resume_power_well(dev);

I find this a little bit confusing because we basically have 2
functions that maybe call __intel_set_power_well, and in the init code
we end calling it twice. It would be nicer if we had only 1 codepath
leading to __intel_set_power_well.


>
>         /* We're taking over the BIOS, so clear any requests made by it since
>          * the driver is in charge now. */
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v2 01/11] drm/i915: Change i915_request power well handling

[Ville Syrjälä]

On Mon, Sep 16, 2013 at 02:33:06PM -0300, Paulo Zanoni wrote:
> 2013/9/16  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Reorganize the internal i915_request power well handling to use the
> > reference count just like everyone else. This way all we need to do is
> > check the reference count and we know whether the power well needs to be
> > enabled of disabled.
> >
> > v2: Split he intel_display_power_{get,put} change to another patch.
> >     Add intel_resume_power_well() to make sure we enable the power
> >     well on resume
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c  | 43 +++++++++++++++++++++++++++++++---------
> >  2 files changed, 35 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 774ebb6..8853f53 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -765,6 +765,7 @@ extern bool intel_display_power_enabled(struct drm_device *dev,
> >                                         enum intel_display_power_domain domain);
> >  extern void intel_init_power_well(struct drm_device *dev);
> >  extern void intel_set_power_well(struct drm_device *dev, bool enable);
> > +extern void intel_resume_power_well(struct drm_device *dev);
> >  extern void intel_enable_gt_powersave(struct drm_device *dev);
> >  extern void intel_disable_gt_powersave(struct drm_device *dev);
> >  extern void ironlake_teardown_rc6(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8cffef4..310d2ed 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5342,8 +5342,7 @@ void i915_request_power_well(void)
> >                 return;
> >
> >         spin_lock_irq(&hsw_pwr->lock);
> > -       if (!hsw_pwr->count++ &&
> > -                       !hsw_pwr->i915_request)
> > +       if (!hsw_pwr->count++)
> >                 __intel_set_power_well(hsw_pwr->device, true);
> >         spin_unlock_irq(&hsw_pwr->lock);
> >  }
> > @@ -5357,8 +5356,7 @@ void i915_release_power_well(void)
> >
> >         spin_lock_irq(&hsw_pwr->lock);
> >         WARN_ON(!hsw_pwr->count);
> > -       if (!--hsw_pwr->count &&
> > -                      !hsw_pwr->i915_request)
> > +       if (!--hsw_pwr->count)
> >                 __intel_set_power_well(hsw_pwr->device, false);
> >         spin_unlock_irq(&hsw_pwr->lock);
> >  }
> > @@ -5394,15 +5392,41 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
> >                 return;
> >
> >         spin_lock_irq(&power_well->lock);
> > +
> > +       /*
> > +        * This function will only ever contribute one
> > +        * to the power well reference count. i915_request
> > +        * is what tracks whether we have or have not
> > +        * added the one to the reference count.
> > +        */
> > +       if (power_well->i915_request == enable)
> > +               goto out;
> > +
> >         power_well->i915_request = enable;
> >
> > -       /* only reject "disable" power well request */
> > -       if (power_well->count && !enable) {
> > -               spin_unlock_irq(&power_well->lock);
> > -               return;
> > +       if (enable) {
> > +               if (!power_well->count++)
> > +                       __intel_set_power_well(dev, true);
> > +       } else {
> > +               WARN_ON(!power_well->count);
> > +               if (!--power_well->count)
> > +                       __intel_set_power_well(dev, false);
> >         }
> >
> > -       __intel_set_power_well(dev, enable);
> > + out:
> > +       spin_unlock_irq(&power_well->lock);
> > +}
> > +
> > +void intel_resume_power_well(struct drm_device *dev)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct i915_power_well *power_well = &dev_priv->power_well;
> > +
> > +       if (!HAS_POWER_WELL(dev))
> > +               return;
> > +
> > +       spin_lock_irq(&power_well->lock);
> > +       __intel_set_power_well(dev, power_well->count > 0);
> >         spin_unlock_irq(&power_well->lock);
> >  }
> >
> > @@ -5421,6 +5445,7 @@ void intel_init_power_well(struct drm_device *dev)
> >
> >         /* For now, we need the power well to be always enabled. */
> >         intel_set_power_well(dev, true);
> > +       intel_resume_power_well(dev);
> 
> I find this a little bit confusing because we basically have 2
> functions that maybe call __intel_set_power_well, and in the init code
> we end calling it twice. It would be nicer if we had only 1 codepath
> leading to __intel_set_power_well.

I think the split is now very clear. intel_resume_power_well() makes
sure the hardware state matches the software state, and
intel_set_power_well() requests the power well to be activated just
like before.

> 
> 
> >
> >         /* We're taking over the BIOS, so clear any requests made by it since
> >          * the driver is in charge now. */
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 06/11] drm/i915: Fix unclaimed register access due to delayed VGA memroy disable

[Paulo Zanoni]

2013/9/16  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> VGA registers live inside the power well on HSW, so in order to write
> the VGA MSR register we need the power well to be on.
>
> We really must write to the register to properly clear the
> VGA_MSR_MEM_EN enable bit, even if all VGA registers get zeroed when
> the power well is down. It seems that the implicit zeroing done by
> the power well is not enough to propagate the VGA_MSR_MEM_EN bit to
> whomever is actually responsible for the memory decode ranges.
>
> If we leave VGA memory decode enabled, and then turn off the power well,
> all VGA memory reads will return zeroes. But if we first disable VGA
> memory deocde and then turn off the power well, VGA memory reads
> return all ones, indicating that the access wasn't claimed by anyone.
> For the vga arbiter to function correctly the IGD must not claim the
> VGA memory accesses.
>
> Previously we were doing the VGA_MSR register access while the power well
> was excplicitly powered up during driver init. But ever since
>
>  commit 6e1b4fdad5157bb9e88777d525704aba24389bee
>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>  Date:   Thu Sep 5 20:40:52 2013 +0300
>
>     drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff is done
>
> we delay the VGA memory disable until fbcon has initialized, and so
> there's a possibility that the power well got turned off during the
> fbcon modeset. Also vgacon_save_screen() will need the power well to be
> on to be able to read the VGA memory.
>
> So immediately after enabling the power well during init grab a refence
> for VGA purposes, and after all the VGA handling is done, release it.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index e5c7b10..23f965d 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1326,6 +1326,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
>
>         intel_init_power_well(dev);
>
> +       /* Keep VGA alive until i915_disable_vga_mem() */
> +       intel_display_power_get(dev, POWER_DOMAIN_VGA);
> +
>         intel_modeset_gem_init(dev);
>
>         /* Always safe in the mode setting case. */
> @@ -1358,6 +1361,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>          * vgacon_save_screen() works during the handover.
>          */
>         i915_disable_vga_mem(dev);
> +       intel_display_power_put(dev, POWER_DOMAIN_VGA);

There's a "return 0" between the get and the put. (I was already at
patch 8 when I realized that!)

While reviewing your series I started thinking: shouldn't we just
get/put POWER_DOMAIN_VGA directly inside the functions that touch the
VGA code? This shouldn't really be an expensive thing, and it will
make our code simpler and harder to break. While your series seems
correct, there is code that depends on state left by other code, which
IMHO increases the complexity of the already-too-complex init/resume
paths. This is just a suggestion, not a requirement :)

>
>         /* Only enable hotplug handling once the fbdev is fully set up. */
>         dev_priv->enable_hotplug_processing = true;
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 05/11] drm/i915: Pull intel_init_power_well() out of intel_modeset_init_hw()

[Paulo Zanoni]

2013/9/16  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The init and resume codepaths want to handel the power well in slightly

s/handel/handle/

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


> different ways, so pull the power well init out from
> intel_modeset_init_hw() which gets called in both cases.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      | 2 ++
>  drivers/gpu/drm/i915/i915_drv.c      | 2 ++
>  drivers/gpu/drm/i915/intel_display.c | 2 --
>  3 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 9b265a4..e5c7b10 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1324,6 +1324,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
>
>         INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
>
> +       intel_init_power_well(dev);
> +
>         intel_modeset_gem_init(dev);
>
>         /* Always safe in the mode setting case. */
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ec690ca..cd5a66d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -596,6 +596,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
>                 /* We need working interrupts for modeset enabling ... */
>                 drm_irq_install(dev);
>
> +               intel_init_power_well(dev);
> +
>                 intel_modeset_init_hw(dev);
>
>                 drm_modeset_lock_all(dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 60284fc..8162cd1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10275,8 +10275,6 @@ void i915_disable_vga_mem(struct drm_device *dev)
>
>  void intel_modeset_init_hw(struct drm_device *dev)
>  {
> -       intel_init_power_well(dev);
> -
>         intel_prepare_ddi(dev);
>
>         intel_init_clock_gating(dev);
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 07/11] drm/i915: Redisable VGA before the modeset on resume

[Paulo Zanoni]

2013/9/16  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The VGA plane needs to be disabled before we start doing any
> modeset operations on resume.
>
> This should also guarantee that the power well will be enabled
> when we call i915_redisable_vga() since it gets explicitly powered on
> during resume, and will get powered back off during the modeset
> operation if no longer needed.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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


> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8162cd1..c9093bb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10689,6 +10689,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>         }
>
>         if (force_restore) {
> +               i915_redisable_vga(dev);
> +
>                 /*
>                  * We need to use raw interfaces for restoring state to avoid
>                  * checking (bogus) intermediate states.
> @@ -10702,8 +10704,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>                 }
>                 list_for_each_entry(plane, &dev->mode_config.plane_list, head)
>                         intel_plane_restore(plane);
> -
> -               i915_redisable_vga(dev);
>         } else {
>                 intel_modeset_update_staged_output_state(dev);
>         }
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 09/11] drm/i915: Move power well resume earlier

[Paulo Zanoni]

2013/9/16  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> i915_restore_state() -> i915_restore_display() will attempt to
> re-disable VGA during resume. So the power well needs to be powered on
> before that.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Needs a rebase, but looks correct.

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index cd5a66d..fdf3eef 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -581,6 +581,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         int error = 0;
>
> +       intel_init_power_well(dev);
> +
>         i915_restore_state(dev);
>         intel_opregion_setup(dev);
>
> @@ -596,8 +598,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
>                 /* We need working interrupts for modeset enabling ... */
>                 drm_irq_install(dev);
>
> -               intel_init_power_well(dev);
> -
>                 intel_modeset_init_hw(dev);
>
>                 drm_modeset_lock_all(dev);
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 10/11] drm/i915: Call intel_uncore_early_sanitize() during resume

[Paulo Zanoni]

2013/9/16  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Call intel_uncore_early_sanitize() first thing during resume to prevent
> stale BIOS leftovers from being reported as unclaimed register access.

Just out of curiosity: do you actually need this patch to avoid error
messages? The change makes sense, but since we didn't need it so far,
I wonder if maybe it's something we're doing wrong that's causing the
need to sanitize.


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index fdf3eef..3d25731 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -670,6 +670,8 @@ int i915_resume(struct drm_device *dev)
>
>         pci_set_master(dev->pdev);
>
> +       intel_uncore_early_sanitize(dev);
> +
>         intel_uncore_sanitize(dev);
>
>         /*
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 11/11] drm/i915: Drop explicit plane restoration during resume

[Paulo Zanoni]

2013/9/16  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We already restore planes during the modeset operation, so no need to do
> another loop over the planes and try to restore them again.

What about the call from intel_lid_notify()? It helps if you explain
on the commit message why/how we already restore things.


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c9093bb..37a470f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10641,7 +10641,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         enum pipe pipe;
> -       struct drm_plane *plane;
>         struct intel_crtc *crtc;
>         struct intel_encoder *encoder;
>         int i;
> @@ -10702,8 +10701,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>                         __intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
>                                          crtc->fb);
>                 }
> -               list_for_each_entry(plane, &dev->mode_config.plane_list, head)
> -                       intel_plane_restore(plane);
>         } else {
>                 intel_modeset_update_staged_output_state(dev);
>         }
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 06/11] drm/i915: Fix unclaimed register access due to delayed VGA memroy disable

[Ville Syrjälä]

On Thu, Sep 19, 2013 at 07:05:14PM -0300, Paulo Zanoni wrote:
> 2013/9/16  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > VGA registers live inside the power well on HSW, so in order to write
> > the VGA MSR register we need the power well to be on.
> >
> > We really must write to the register to properly clear the
> > VGA_MSR_MEM_EN enable bit, even if all VGA registers get zeroed when
> > the power well is down. It seems that the implicit zeroing done by
> > the power well is not enough to propagate the VGA_MSR_MEM_EN bit to
> > whomever is actually responsible for the memory decode ranges.
> >
> > If we leave VGA memory decode enabled, and then turn off the power well,
> > all VGA memory reads will return zeroes. But if we first disable VGA
> > memory deocde and then turn off the power well, VGA memory reads
> > return all ones, indicating that the access wasn't claimed by anyone.
> > For the vga arbiter to function correctly the IGD must not claim the
> > VGA memory accesses.
> >
> > Previously we were doing the VGA_MSR register access while the power well
> > was excplicitly powered up during driver init. But ever since
> >
> >  commit 6e1b4fdad5157bb9e88777d525704aba24389bee
> >  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >  Date:   Thu Sep 5 20:40:52 2013 +0300
> >
> >     drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff is done
> >
> > we delay the VGA memory disable until fbcon has initialized, and so
> > there's a possibility that the power well got turned off during the
> > fbcon modeset. Also vgacon_save_screen() will need the power well to be
> > on to be able to read the VGA memory.
> >
> > So immediately after enabling the power well during init grab a refence
> > for VGA purposes, and after all the VGA handling is done, release it.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index e5c7b10..23f965d 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1326,6 +1326,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >
> >         intel_init_power_well(dev);
> >
> > +       /* Keep VGA alive until i915_disable_vga_mem() */
> > +       intel_display_power_get(dev, POWER_DOMAIN_VGA);
> > +
> >         intel_modeset_gem_init(dev);
> >
> >         /* Always safe in the mode setting case. */
> > @@ -1358,6 +1361,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >          * vgacon_save_screen() works during the handover.
> >          */
> >         i915_disable_vga_mem(dev);
> > +       intel_display_power_put(dev, POWER_DOMAIN_VGA);
> 
> There's a "return 0" between the get and the put. (I was already at
> patch 8 when I realized that!)

Ugh. That whole 0 pipes thing seems rather wonky. Does that thing even
have many of the display resources we touch before the return 0 there?
I guess it's not a real problem for HSW at this point since there aren't
HSWs w/o the display block yet. But I'll add a put call there just to
keep the resemblence of sanity for that case.

> 
> While reviewing your series I started thinking: shouldn't we just
> get/put POWER_DOMAIN_VGA directly inside the functions that touch the
> VGA code? This shouldn't really be an expensive thing, and it will
> make our code simpler and harder to break. While your series seems
> correct, there is code that depends on state left by other code, which
> IMHO increases the complexity of the already-too-complex init/resume
> paths. This is just a suggestion, not a requirement :)

We need VGA to stay alive (and hence the power well) the whole time until
i915_disable_vga_mem() is called. I didn't want to put the get/put calls
inside some random functions we call during init since then it becomes
much harder to see the relationship between the get/put calls.

> 
> >
> >         /* Only enable hotplug handling once the fbdev is fully set up. */
> >         dev_priv->enable_hotplug_processing = true;
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC

[PATCH v2 06/11] drm/i915: Fix unclaimed register access due to delayed VGA memroy disable

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

VGA registers live inside the power well on HSW, so in order to write
the VGA MSR register we need the power well to be on.

We really must write to the register to properly clear the
VGA_MSR_MEM_EN enable bit, even if all VGA registers get zeroed when
the power well is down. It seems that the implicit zeroing done by
the power well is not enough to propagate the VGA_MSR_MEM_EN bit to
whomever is actually responsible for the memory decode ranges.

If we leave VGA memory decode enabled, and then turn off the power well,
all VGA memory reads will return zeroes. But if we first disable VGA
memory deocde and then turn off the power well, VGA memory reads
return all ones, indicating that the access wasn't claimed by anyone.
For the vga arbiter to function correctly the IGD must not claim the
VGA memory accesses.

Previously we were doing the VGA_MSR register access while the power well
was excplicitly powered up during driver init. But ever since

 commit 6e1b4fdad5157bb9e88777d525704aba24389bee
 Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
 Date:   Thu Sep 5 20:40:52 2013 +0300

    drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff is done

we delay the VGA memory disable until fbcon has initialized, and so
there's a possibility that the power well got turned off during the
fbcon modeset. Also vgacon_save_screen() will need the power well to be
on to be able to read the VGA memory.

So immediately after enabling the power well during init grab a refence
for VGA purposes, and after all the VGA handling is done, release it.

v2: Add intel_display_power_put() for the num_pipes==0 case

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e5c7b10..956d143 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1326,13 +1326,18 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_init_power_well(dev);
 
+	/* Keep VGA alive until i915_disable_vga_mem() */
+	intel_display_power_get(dev, POWER_DOMAIN_VGA);
+
 	intel_modeset_gem_init(dev);
 
 	/* Always safe in the mode setting case. */
 	/* FIXME: do pre/post-mode set stuff in core KMS code */
 	dev->vblank_disable_allowed = 1;
-	if (INTEL_INFO(dev)->num_pipes == 0)
+	if (INTEL_INFO(dev)->num_pipes == 0) {
+		intel_display_power_put(dev, POWER_DOMAIN_VGA);
 		return 0;
+	}
 
 	ret = intel_fbdev_init(dev);
 	if (ret)
@@ -1358,6 +1363,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	 * vgacon_save_screen() works during the handover.
 	 */
 	i915_disable_vga_mem(dev);
+	intel_display_power_put(dev, POWER_DOMAIN_VGA);
 
 	/* Only enable hotplug handling once the fbdev is fully set up. */
 	dev_priv->enable_hotplug_processing = true;
-- 
1.8.1.5

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

Re: [PATCH 11/11] drm/i915: Drop explicit plane restoration during resume

[Ville Syrjälä]

On Thu, Sep 19, 2013 at 07:24:19PM -0300, Paulo Zanoni wrote:
> 2013/9/16  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We already restore planes during the modeset operation, so no need to do
> > another loop over the planes and try to restore them again.
> 
> What about the call from intel_lid_notify()? It helps if you explain
> on the commit message why/how we already restore things.

Sorry, I figured it's more or less obvious:

for each crtc
 -> __intel_set_mode
   -> .crtc_enable 
     -> intel_enable_planes
       -> for each plane on crtc
         -> intel_plane_restore

> 
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index c9093bb..37a470f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10641,7 +10641,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         enum pipe pipe;
> > -       struct drm_plane *plane;
> >         struct intel_crtc *crtc;
> >         struct intel_encoder *encoder;
> >         int i;
> > @@ -10702,8 +10701,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> >                         __intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
> >                                          crtc->fb);
> >                 }
> > -               list_for_each_entry(plane, &dev->mode_config.plane_list, head)
> > -                       intel_plane_restore(plane);
> >         } else {
> >                 intel_modeset_update_staged_output_state(dev);
> >         }
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 10/11] drm/i915: Call intel_uncore_early_sanitize() during resume

[Ville Syrjälä]

On Thu, Sep 19, 2013 at 07:18:54PM -0300, Paulo Zanoni wrote:
> 2013/9/16  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Call intel_uncore_early_sanitize() first thing during resume to prevent
> > stale BIOS leftovers from being reported as unclaimed register access.
> 
> Just out of curiosity: do you actually need this patch to avoid error
> messages? The change makes sense, but since we didn't need it so far,
> I wonder if maybe it's something we're doing wrong that's causing the
> need to sanitize.

It does get rid of an error message on my HSW machine during hibernate.

I just noticed that I didn't have CONFIG_SND_HDA_I915 enabled, but I
just tried to enable it and I still got the error. The latest BIOS
didn't fix it either. And I also tried to clear the error as the last
thing before we suspend the device, and that didn't help either. So it
does look like the BIOS does something silly.


> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index fdf3eef..3d25731 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -670,6 +670,8 @@ int i915_resume(struct drm_device *dev)
> >
> >         pci_set_master(dev->pdev);
> >
> > +       intel_uncore_early_sanitize(dev);
> > +
> >         intel_uncore_sanitize(dev);
> >
> >         /*
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 05/11] drm/i915: Pull intel_init_power_well() out of intel_modeset_init_hw()

[Daniel Vetter]

On Thu, Sep 19, 2013 at 07:07:04PM -0300, Paulo Zanoni wrote:
> 2013/9/16  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > The init and resume codepaths want to handel the power well in slightly
> 
> s/handel/handle/
> 
> Patches 1-5: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Up to this all merged, thanks for patches&review.
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 11/11] drm/i915: Drop explicit plane restoration during resume

[Paulo Zanoni]

2013/9/20 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Thu, Sep 19, 2013 at 07:24:19PM -0300, Paulo Zanoni wrote:
>> 2013/9/16  <ville.syrjala@linux.intel.com>:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > We already restore planes during the modeset operation, so no need to do
>> > another loop over the planes and try to restore them again.
>>
>> What about the call from intel_lid_notify()? It helps if you explain
>> on the commit message why/how we already restore things.
>
> Sorry, I figured it's more or less obvious:
>
> for each crtc
>  -> __intel_set_mode
>    -> .crtc_enable
>      -> intel_enable_planes
>        -> for each plane on crtc
>          -> intel_plane_restore

But that still doesn't explain the case where intel_lid_notify calls
setup_hw_state directly.
Perhaps maybe I'm just confused.

>
>>
>>
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 3 ---
>> >  1 file changed, 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index c9093bb..37a470f 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -10641,7 +10641,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>> >  {
>> >         struct drm_i915_private *dev_priv = dev->dev_private;
>> >         enum pipe pipe;
>> > -       struct drm_plane *plane;
>> >         struct intel_crtc *crtc;
>> >         struct intel_encoder *encoder;
>> >         int i;
>> > @@ -10702,8 +10701,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>> >                         __intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
>> >                                          crtc->fb);
>> >                 }
>> > -               list_for_each_entry(plane, &dev->mode_config.plane_list, head)
>> > -                       intel_plane_restore(plane);
>> >         } else {
>> >                 intel_modeset_update_staged_output_state(dev);
>> >         }
>> > --
>> > 1.8.1.5
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Paulo Zanoni
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni

Re: [PATCH 11/11] drm/i915: Drop explicit plane restoration during resume

[Paulo Zanoni]

2013/9/20 Paulo Zanoni <przanoni@gmail.com>:
> 2013/9/20 Ville Syrjälä <ville.syrjala@linux.intel.com>:
>> On Thu, Sep 19, 2013 at 07:24:19PM -0300, Paulo Zanoni wrote:
>>> 2013/9/16  <ville.syrjala@linux.intel.com>:
>>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> >
>>> > We already restore planes during the modeset operation, so no need to do
>>> > another loop over the planes and try to restore them again.
>>>
>>> What about the call from intel_lid_notify()? It helps if you explain
>>> on the commit message why/how we already restore things.
>>
>> Sorry, I figured it's more or less obvious:
>>
>> for each crtc
>>  -> __intel_set_mode
>>    -> .crtc_enable
>>      -> intel_enable_planes
>>        -> for each plane on crtc
>>          -> intel_plane_restore
>
> But that still doesn't explain the case where intel_lid_notify calls
> setup_hw_state directly.
> Perhaps maybe I'm just confused.

Yeah, I was totally confused.

Now that you resent patch 6, for patches 6-11: Reviewed-by: Paulo
Zanoni <paulo.r.zanoni@intel.com>
(Daniel will have to rebase some things)

>
>>
>>>
>>>
>>> >
>>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> > ---
>>> >  drivers/gpu/drm/i915/intel_display.c | 3 ---
>>> >  1 file changed, 3 deletions(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> > index c9093bb..37a470f 100644
>>> > --- a/drivers/gpu/drm/i915/intel_display.c
>>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>>> > @@ -10641,7 +10641,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>>> >  {
>>> >         struct drm_i915_private *dev_priv = dev->dev_private;
>>> >         enum pipe pipe;
>>> > -       struct drm_plane *plane;
>>> >         struct intel_crtc *crtc;
>>> >         struct intel_encoder *encoder;
>>> >         int i;
>>> > @@ -10702,8 +10701,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>>> >                         __intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
>>> >                                          crtc->fb);
>>> >                 }
>>> > -               list_for_each_entry(plane, &dev->mode_config.plane_list, head)
>>> > -                       intel_plane_restore(plane);
>>> >         } else {
>>> >                 intel_modeset_update_staged_output_state(dev);
>>> >         }
>>> > --
>>> > 1.8.1.5
>>> >
>>> > _______________________________________________
>>> > Intel-gfx mailing list
>>> > Intel-gfx@lists.freedesktop.org
>>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>>
>>>
>>> --
>>> Paulo Zanoni
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>
>
>
> --
> Paulo Zanoni



-- 
Paulo Zanoni

Re: [PATCH 11/11] drm/i915: Drop explicit plane restoration during resume

[Daniel Vetter]

On Fri, Sep 20, 2013 at 03:21:03PM -0300, Paulo Zanoni wrote:
> 2013/9/20 Paulo Zanoni <przanoni@gmail.com>:
> > 2013/9/20 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> >> On Thu, Sep 19, 2013 at 07:24:19PM -0300, Paulo Zanoni wrote:
> >>> 2013/9/16  <ville.syrjala@linux.intel.com>:
> >>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> >
> >>> > We already restore planes during the modeset operation, so no need to do
> >>> > another loop over the planes and try to restore them again.
> >>>
> >>> What about the call from intel_lid_notify()? It helps if you explain
> >>> on the commit message why/how we already restore things.
> >>
> >> Sorry, I figured it's more or less obvious:
> >>
> >> for each crtc
> >>  -> __intel_set_mode
> >>    -> .crtc_enable
> >>      -> intel_enable_planes
> >>        -> for each plane on crtc
> >>          -> intel_plane_restore
> >
> > But that still doesn't explain the case where intel_lid_notify calls
> > setup_hw_state directly.
> > Perhaps maybe I'm just confused.
> 
> Yeah, I was totally confused.
> 
> Now that you resent patch 6, for patches 6-11: Reviewed-by: Paulo
> Zanoni <paulo.r.zanoni@intel.com>

Merged them all, thanks for patches&review.

> (Daniel will have to rebase some things)

wiggle took care of everything ;-)

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

Re: [PATCH v2 06/11] drm/i915: Fix unclaimed register access due to delayed VGA memroy disable

[Paulo Zanoni]

2013/9/20  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> VGA registers live inside the power well on HSW, so in order to write
> the VGA MSR register we need the power well to be on.
>
> We really must write to the register to properly clear the
> VGA_MSR_MEM_EN enable bit, even if all VGA registers get zeroed when
> the power well is down. It seems that the implicit zeroing done by
> the power well is not enough to propagate the VGA_MSR_MEM_EN bit to
> whomever is actually responsible for the memory decode ranges.
>
> If we leave VGA memory decode enabled, and then turn off the power well,
> all VGA memory reads will return zeroes. But if we first disable VGA
> memory deocde and then turn off the power well, VGA memory reads
> return all ones, indicating that the access wasn't claimed by anyone.
> For the vga arbiter to function correctly the IGD must not claim the
> VGA memory accesses.
>
> Previously we were doing the VGA_MSR register access while the power well
> was excplicitly powered up during driver init. But ever since
>
>  commit 6e1b4fdad5157bb9e88777d525704aba24389bee
>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>  Date:   Thu Sep 5 20:40:52 2013 +0300
>
>     drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff is done
>
> we delay the VGA memory disable until fbcon has initialized, and so
> there's a possibility that the power well got turned off during the
> fbcon modeset. Also vgacon_save_screen() will need the power well to be
> on to be able to read the VGA memory.
>
> So immediately after enabling the power well during init grab a refence
> for VGA purposes, and after all the VGA handling is done, release it.
>
> v2: Add intel_display_power_put() for the num_pipes==0 case

Even though the patch was applied, the issue didn't go away. It looks
like the conflict-merging went wrong. Look:
http://cgit.freedesktop.org/~danvet/drm-intel/commit/?h=drm-intel-next-queued&id=971b0597b3a13576e1af1e0a90379aa308f498e5

The new _put function is not after i915_disable_vga_mem().


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index e5c7b10..956d143 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1326,13 +1326,18 @@ static int i915_load_modeset_init(struct drm_device *dev)
>
>         intel_init_power_well(dev);
>
> +       /* Keep VGA alive until i915_disable_vga_mem() */
> +       intel_display_power_get(dev, POWER_DOMAIN_VGA);
> +
>         intel_modeset_gem_init(dev);
>
>         /* Always safe in the mode setting case. */
>         /* FIXME: do pre/post-mode set stuff in core KMS code */
>         dev->vblank_disable_allowed = 1;
> -       if (INTEL_INFO(dev)->num_pipes == 0)
> +       if (INTEL_INFO(dev)->num_pipes == 0) {
> +               intel_display_power_put(dev, POWER_DOMAIN_VGA);
>                 return 0;
> +       }
>
>         ret = intel_fbdev_init(dev);
>         if (ret)
> @@ -1358,6 +1363,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>          * vgacon_save_screen() works during the handover.
>          */
>         i915_disable_vga_mem(dev);
> +       intel_display_power_put(dev, POWER_DOMAIN_VGA);
>
>         /* Only enable hotplug handling once the fbdev is fully set up. */
>         dev_priv->enable_hotplug_processing = true;
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v2 06/11] drm/i915: Fix unclaimed register access due to delayed VGA memroy disable

[Daniel Vetter]

On Mon, Sep 23, 2013 at 01:48:42PM -0300, Paulo Zanoni wrote:
> 2013/9/20  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > VGA registers live inside the power well on HSW, so in order to write
> > the VGA MSR register we need the power well to be on.
> >
> > We really must write to the register to properly clear the
> > VGA_MSR_MEM_EN enable bit, even if all VGA registers get zeroed when
> > the power well is down. It seems that the implicit zeroing done by
> > the power well is not enough to propagate the VGA_MSR_MEM_EN bit to
> > whomever is actually responsible for the memory decode ranges.
> >
> > If we leave VGA memory decode enabled, and then turn off the power well,
> > all VGA memory reads will return zeroes. But if we first disable VGA
> > memory deocde and then turn off the power well, VGA memory reads
> > return all ones, indicating that the access wasn't claimed by anyone.
> > For the vga arbiter to function correctly the IGD must not claim the
> > VGA memory accesses.
> >
> > Previously we were doing the VGA_MSR register access while the power well
> > was excplicitly powered up during driver init. But ever since
> >
> >  commit 6e1b4fdad5157bb9e88777d525704aba24389bee
> >  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >  Date:   Thu Sep 5 20:40:52 2013 +0300
> >
> >     drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff is done
> >
> > we delay the VGA memory disable until fbcon has initialized, and so
> > there's a possibility that the power well got turned off during the
> > fbcon modeset. Also vgacon_save_screen() will need the power well to be
> > on to be able to read the VGA memory.
> >
> > So immediately after enabling the power well during init grab a refence
> > for VGA purposes, and after all the VGA handling is done, release it.
> >
> > v2: Add intel_display_power_put() for the num_pipes==0 case
> 
> Even though the patch was applied, the issue didn't go away. It looks
> like the conflict-merging went wrong. Look:
> http://cgit.freedesktop.org/~danvet/drm-intel/commit/?h=drm-intel-next-queued&id=971b0597b3a13576e1af1e0a90379aa308f498e5
> 
> The new _put function is not after i915_disable_vga_mem().


Oops, so much for trusting the tools too much ;-) Should be fixed now,
thanks for catching this.
-Daniel

> 
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index e5c7b10..956d143 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1326,13 +1326,18 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >
> >         intel_init_power_well(dev);
> >
> > +       /* Keep VGA alive until i915_disable_vga_mem() */
> > +       intel_display_power_get(dev, POWER_DOMAIN_VGA);
> > +
> >         intel_modeset_gem_init(dev);
> >
> >         /* Always safe in the mode setting case. */
> >         /* FIXME: do pre/post-mode set stuff in core KMS code */
> >         dev->vblank_disable_allowed = 1;
> > -       if (INTEL_INFO(dev)->num_pipes == 0)
> > +       if (INTEL_INFO(dev)->num_pipes == 0) {
> > +               intel_display_power_put(dev, POWER_DOMAIN_VGA);
> >                 return 0;
> > +       }
> >
> >         ret = intel_fbdev_init(dev);
> >         if (ret)
> > @@ -1358,6 +1363,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >          * vgacon_save_screen() works during the handover.
> >          */
> >         i915_disable_vga_mem(dev);
> > +       intel_display_power_put(dev, POWER_DOMAIN_VGA);
> >
> >         /* Only enable hotplug handling once the fbdev is fully set up. */
> >         dev_priv->enable_hotplug_processing = true;
> > --
> > 1.8.1.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