[PATCH 1/2] drm/i915: avoid reading non-existent PLL reg on Ironlake+

[PATCH 1/2] drm/i915: avoid reading non-existent PLL reg on Ironlake+

[Jesse Barnes]

These functions need to be reworked for Ironlake and above, but until
then at least avoid reading non-existent registers.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e7e7b8a..6313076 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5042,11 +5042,12 @@ static void intel_increase_pllclock(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
 	int dpll_reg = (pipe == 0) ? DPLL_A : DPLL_B;
-	int dpll = I915_READ(dpll_reg);
+	int dpll;
 
 	if (HAS_PCH_SPLIT(dev))
 		return;
 
+	dpll = I915_READ(dpll_reg);
 	if (!dev_priv->lvds_downclock_avail)
 		return;
 
-- 
1.7.0.4

[PATCH 2/2] drm/i915: add pipe enable/disable functions

[Jesse Barnes]

These allow us to check for pipe enable/disable requirements at
enable/disable time (e.g. plane status, pll status, etc.).

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |  161 +++++++++++++++++++++++++++-------
 1 files changed, 128 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6313076..b1c78b5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1058,6 +1058,128 @@ void intel_wait_for_pipe_off(struct drm_device *dev, int pipe)
 	}
 }
 
+enum state {
+	off = 0,
+	on = 1,
+};
+
+static char *state_string(enum state enabled)
+{
+	return (enabled ? "on" : "off");
+}
+
+static void assert_pll(struct drm_i915_private *dev_priv, enum pipe pipe,
+		       enum state state)
+{
+	int reg;
+	u32 val;
+	bool cur_state;
+
+	if (HAS_PCH_SPLIT(dev_priv->dev))
+		reg = PCH_DPLL(pipe);
+	else
+		reg = DPLL(pipe);
+
+	val = I915_READ(reg);
+	cur_state = !!(val & DPLL_VCO_ENABLE);
+	WARN(cur_state != state,
+	     "PLL state assertion failure (expected %s, current %s)\n",
+	     state_string(state), state_string(cur_state));
+}
+#define assert_pll_enabled(d, p) assert_pll(d, p, on)
+#define assert_pll_disabled(d, p) assert_pll(d, p, off)
+
+static void assert_plane_enabled(struct drm_i915_private *dev_priv,
+				 enum plane plane)
+{
+	int reg;
+	u32 val;
+
+	reg = DSPCNTR(plane);
+	val = I915_READ(reg);
+	WARN(!(val & DISPLAY_PLANE_ENABLE),
+	     "plane %c assertion failure, should be active but is disabled\n",
+	     plane ? 'B' : 'A');
+}
+
+static void assert_planes_disabled(struct drm_i915_private *dev_priv,
+				   enum pipe pipe)
+{
+	int reg, i;
+	u32 val;
+	int cur_pipe;
+
+	/* Need to check both planes against the pipe */
+	for (i = 0; i < 2; i++) {
+		reg = DSPCNTR(i);
+		val = I915_READ(reg);
+		cur_pipe = (val & DISPPLANE_SEL_PIPE_MASK) >>
+			DISPPLANE_SEL_PIPE_SHIFT;
+		WARN((val & DISPLAY_PLANE_ENABLE) && pipe == cur_pipe,
+		     "plane %d assertion failure, should be off on pipe %c but is still active\n",
+		     i, pipe ? 'B' : 'A');
+	}
+}
+
+/**
+ * intel_enable_pipe - enable a pipe, assertiing requirements
+ * @dev_priv: i915 private structure
+ * @pipe: pipe to enable
+ *
+ * Enable @pipe, making sure that various hardware specific requirements
+ * are met, if applicable, e.g. PLL enabled, LVDS pairs enabled, etc.
+ *
+ * @pipe should be %PIPE_A or %PIPE_B.
+ *
+ * Will wait until the pipe is actually running (i.e. first vblank) before
+ * returning.
+ */
+static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
+{
+	int reg;
+	u32 val;
+
+	/* Make sure a PLL is enabled first */
+	assert_pll_enabled(dev_priv, pipe);
+
+	/* Enable CPU pipe */
+	reg = PIPECONF(pipe);
+	val = I915_READ(reg);
+	val |= PIPECONF_ENABLE;
+	I915_WRITE(reg, val);
+	POSTING_READ(reg);
+	intel_wait_for_vblank(dev_priv->dev, pipe);
+}
+
+/**
+ * intel_disable_pipe - disable a pipe, assertiing requirements
+ * @dev_priv: i915 private structure
+ * @pipe: pipe to disable
+ *
+ * Disable @pipe, making sure that various hardware specific requirements
+ * are met, if applicable, e.g. plane disabled, panel fitter off, etc.
+ *
+ * @pipe should be %PIPE_A or %PIPE_B.
+ *
+ * Will wait until the pipe has shut down before returning.
+ */
+static void intel_disable_pipe(struct drm_i915_private *dev_priv,
+			       enum pipe pipe)
+{
+	int reg;
+	u32 val;
+
+	/* Make sure plane is disabled */
+	assert_planes_disabled(dev_priv, pipe);
+
+	reg = PIPECONF(pipe);
+	val = I915_READ(reg);
+	val &= ~PIPECONF_ENABLE;
+	I915_WRITE(reg, val);
+	POSTING_READ(reg);
+	intel_wait_for_pipe_off(dev_priv->dev, pipe);
+}
+
 static void i8xx_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 {
 	struct drm_device *dev = crtc->dev;
@@ -2062,14 +2184,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 			   dev_priv->pch_pf_size);
 	}
 
-	/* Enable CPU pipe */
-	reg = PIPECONF(pipe);
-	temp = I915_READ(reg);
-	if ((temp & PIPECONF_ENABLE) == 0) {
-		I915_WRITE(reg, temp | PIPECONF_ENABLE);
-		POSTING_READ(reg);
-		intel_wait_for_vblank(dev, intel_crtc->pipe);
-	}
+	intel_enable_pipe(dev_priv, pipe);
 
 	/* configure and enable CPU plane */
 	reg = DSPCNTR(plane);
@@ -2197,15 +2312,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	    dev_priv->display.disable_fbc)
 		dev_priv->display.disable_fbc(dev);
 
-	/* disable cpu pipe, disable after all planes disabled */
-	reg = PIPECONF(pipe);
-	temp = I915_READ(reg);
-	if (temp & PIPECONF_ENABLE) {
-		I915_WRITE(reg, temp & ~PIPECONF_ENABLE);
-		POSTING_READ(reg);
-		/* wait for cpu pipe off, pipe state */
-		intel_wait_for_pipe_off(dev, intel_crtc->pipe);
-	}
+	intel_disable_pipe(dev_priv, pipe);
 
 	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) || HAS_eDP)
 		I915_WRITE(PCH_DP_D, I915_READ(PCH_DP_D) & ~DP_PORT_EN);
@@ -2403,11 +2510,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 		udelay(150);
 	}
 
-	/* Enable the pipe */
-	reg = PIPECONF(pipe);
-	temp = I915_READ(reg);
-	if ((temp & PIPECONF_ENABLE) == 0)
-		I915_WRITE(reg, temp | PIPECONF_ENABLE);
+	intel_enable_pipe(dev_priv, pipe);
 
 	/* Enable the plane */
 	reg = DSPCNTR(plane);
@@ -2464,16 +2567,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	if (pipe == 0 && (dev_priv->quirks & QUIRK_PIPEA_FORCE))
 		goto done;
 
-	/* Next, disable display pipes */
-	reg = PIPECONF(pipe);
-	temp = I915_READ(reg);
-	if (temp & PIPECONF_ENABLE) {
-		I915_WRITE(reg, temp & ~PIPECONF_ENABLE);
-
-		/* Wait for the pipe to turn off */
-		POSTING_READ(reg);
-		intel_wait_for_pipe_off(dev, pipe);
-	}
+	intel_disable_pipe(dev_priv, pipe);
 
 	reg = DPLL(pipe);
 	temp = I915_READ(reg);
@@ -4215,7 +4309,6 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 
 	if (!HAS_PCH_SPLIT(dev)) {
 		dspcntr |= DISPLAY_PLANE_ENABLE;
-		pipeconf |= PIPECONF_ENABLE;
 		dpll |= DPLL_VCO_ENABLE;
 	}
 
@@ -4426,6 +4519,8 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 
 	I915_WRITE(PIPECONF(pipe), pipeconf);
 	POSTING_READ(PIPECONF(pipe));
+	if (!HAS_PCH_SPLIT(dev))
+		intel_enable_pipe(dev_priv, pipe);
 
 	intel_wait_for_vblank(dev, pipe);
 
-- 
1.7.0.4

Re: [PATCH 2/2] drm/i915: add pipe enable/disable functions

[Jesse Barnes]

On Thu, 30 Dec 2010 09:36:40 -0800
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> These allow us to check for pipe enable/disable requirements at
> enable/disable time (e.g. plane status, pll status, etc.).
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  161 +++++++++++++++++++++++++++-------
>  1 files changed, 128 insertions(+), 33 deletions(-)
> 

Don't apply this one yet, I'm still adding checks for the various
conditions on gen3, gen4 and ILK+ display configurations.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915: avoid reading non-existent PLL reg on Ironlake+

[Chris Wilson]

On Thu, 30 Dec 2010 09:36:39 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> These functions need to be reworked for Ironlake and above, but until
> then at least avoid reading non-existent registers.

This looks good by itself and provided an opportunity for another tiny
cleanup... Pushed to -next. Though it does raise the question if we are
arming the upclock timer even when there is nothing to be done and so
causing excess wakeups?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: avoid reading non-existent PLL reg on Ironlake+

[Jesse Barnes]

On Thu, 30 Dec 2010 19:50:17 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 30 Dec 2010 09:36:39 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > These functions need to be reworked for Ironlake and above, but until
> > then at least avoid reading non-existent registers.
> 
> This looks good by itself and provided an opportunity for another tiny
> cleanup... Pushed to -next. Though it does raise the question if we are
> arming the upclock timer even when there is nothing to be done and so
> causing excess wakeups?

Yeah, we shouldn't be doing that.  I thought we had it disabled but I
haven't checked.

-- 
Jesse Barnes, Intel Open Source Technology Center