[RFC/RFT] mode setting assertion functions

[RFC/RFT] mode setting assertion functions

[Jesse Barnes]

Here's a small (but still growing) set of patches to assert some of the
various conditions required to enable pipes, planes, and PLLs.  It also
has the effect of consolidating much of our pipe/plane handling code,
which should make it easier to add 3 pipe support (an IVB feature).

Please test & comment.  Ironlake+ adds transcoders and FDI; I'm still
working on functions for those (they'll need more assertions added).

Overall this approach seems to be a win in terms of code readability,
and should make it easier to find & fix mode setting related bugs,
especially during bringup.

[PATCH 1/4] 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 |  180 ++++++++++++++++++++++++++--------
 1 files changed, 137 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6313076..559e342 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1058,6 +1058,136 @@ 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");
+}
+
+/* Only for pre-ILK configs */
+static void assert_pll(struct drm_i915_private *dev_priv, enum pipe pipe,
+		       enum state state)
+{
+	int reg;
+	u32 val;
+	bool cur_state;
+
+	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;
+
+	/*
+	 * A pipe without a PLL won't actually be able to drive bits from
+	 * a plane.  On ILK+ the pipe PLLs are integrated, so we don't
+	 * need the check.
+	 */
+	if (!HAS_PCH_SPLIT(dev_priv->dev))
+		assert_pll_enabled(dev_priv, 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 planes won't keep trying to pump pixels to us,
+	 * or we might hang the display.
+	 */
+	assert_planes_disabled(dev_priv, pipe);
+
+	/* Don't disable pipe A or pipe A PLLs if needed */
+	if (pipe == PIPE_A && (dev_priv->quirks & QUIRK_PIPEA_FORCE))
+		return;
+
+	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 +2192,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 +2320,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 +2518,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 +2575,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 +4317,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 +4527,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);
 
@@ -5579,16 +5682,7 @@ static void intel_sanitize_modesetting(struct drm_device *dev,
 	if (IS_GEN2(dev))
 		intel_wait_for_vblank(dev, pipe);
 
-	if (pipe == 0 && (dev_priv->quirks & QUIRK_PIPEA_FORCE))
-		return;
-
-	/* Switch off the pipe. */
-	reg = PIPECONF(pipe);
-	val = I915_READ(reg);
-	if (val & PIPECONF_ENABLE) {
-		I915_WRITE(reg, val & ~PIPECONF_ENABLE);
-		intel_wait_for_pipe_off(dev, pipe);
-	}
+	intel_disable_pipe(dev_priv, pipe);
 }
 
 static void intel_crtc_init(struct drm_device *dev, int pipe)
-- 
1.7.0.4

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

[Jesse Barnes]

Add plane enable/disable functions to prevent duplicated code and allow
us to easily check for plane enable/disable requirements (such as pipe
enable).

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_reg.h      |    5 +-
 drivers/gpu/drm/i915/intel_display.c |  123 +++++++++++++++++++--------------
 2 files changed, 74 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2b6cafb..796dee5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2527,9 +2527,10 @@
 #define   DISPPLANE_32BPP_30BIT_NO_ALPHA	(0xa<<26)
 #define   DISPPLANE_STEREO_ENABLE		(1<<25)
 #define   DISPPLANE_STEREO_DISABLE		0
-#define   DISPPLANE_SEL_PIPE_MASK		(1<<24)
+#define   DISPPLANE_SEL_PIPE_SHIFT		24
+#define   DISPPLANE_SEL_PIPE_MASK		(3<<DISPPLANE_SEL_PIPE_SHIFT)
 #define   DISPPLANE_SEL_PIPE_A			0
-#define   DISPPLANE_SEL_PIPE_B			(1<<24)
+#define   DISPPLANE_SEL_PIPE_B			(1<<DISPPLANE_SEL_PIPE_SHIFT)
 #define   DISPPLANE_SRC_KEY_ENABLE		(1<<22)
 #define   DISPPLANE_SRC_KEY_DISABLE		0
 #define   DISPPLANE_LINE_DOUBLE			(1<<20)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 559e342..800514d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1086,6 +1086,19 @@ static void assert_pll(struct drm_i915_private *dev_priv, enum pipe pipe,
 #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_pipe_enabled(struct drm_i915_private *dev_priv,
+				enum pipe pipe)
+{
+	int reg;
+	u32 val;
+
+	reg = PIPECONF(pipe);
+	val = I915_READ(reg);
+	WARN(!(val & PIPECONF_ENABLE),
+	     "pipe %c assertion failure, should be active but is disabled\n",
+	     pipe ? 'B' : 'A');
+}
+
 static void assert_plane_enabled(struct drm_i915_private *dev_priv,
 				 enum plane plane)
 {
@@ -1188,6 +1201,55 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
 	intel_wait_for_pipe_off(dev_priv->dev, pipe);
 }
 
+/**
+ * intel_enable_plane - enable a display plane on a given pipe
+ * @dev_priv: i915 private structure
+ * @plane: plane to enable
+ * @pipe: pipe being fed
+ *
+ * Enable @plane on @pipe, making sure that @pipe is running first.
+ */
+static void intel_enable_plane(struct drm_i915_private *dev_priv,
+			       enum plane plane, enum pipe pipe)
+{
+	int reg;
+	u32 val;
+
+	/* If the pipe isn't enabled, we can't pump pixels and may hang */
+	assert_pipe_enabled(dev_priv, pipe);
+
+	reg = DSPCNTR(plane);
+	val = I915_READ(reg);
+	val |= DISPLAY_PLANE_ENABLE;
+	I915_WRITE(reg, val);
+	POSTING_READ(reg);
+	intel_wait_for_vblank(dev_priv->dev, pipe);
+}
+
+/**
+ * intel_disable_plane - disable a display plane
+ * @dev_priv: i915 private structure
+ * @plane: plane to disable
+ * @pipe: pipe consuming the data
+ *
+ * Disable @plane; should be an independent operation.
+ */
+static void intel_disable_plane(struct drm_i915_private *dev_priv,
+				enum plane plane, enum pipe pipe)
+{
+	int reg;
+	u32 val;
+
+	reg = DSPCNTR(plane);
+	val = I915_READ(reg);
+	val &= ~DISPLAY_PLANE_ENABLE;
+	I915_WRITE(reg, val);
+	POSTING_READ(reg);
+	reg = DSPADDR(plane);
+	I915_WRITE(reg, I915_READ(reg)); /* trigger an update */
+	intel_wait_for_vblank(dev_priv->dev, pipe);
+}
+
 static void i8xx_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 {
 	struct drm_device *dev = crtc->dev;
@@ -2112,14 +2174,6 @@ static void ironlake_fdi_enable(struct drm_crtc *crtc)
 	}
 }
 
-static void intel_flush_display_plane(struct drm_device *dev,
-				      int plane)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 reg = DSPADDR(plane);
-	I915_WRITE(reg, I915_READ(reg));
-}
-
 /*
  * When we disable a pipe, we need to clear any pending scanline wait events
  * to avoid hanging the ring, which we assume we are waiting on.
@@ -2193,14 +2247,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	}
 
 	intel_enable_pipe(dev_priv, pipe);
-
-	/* configure and enable CPU plane */
-	reg = DSPCNTR(plane);
-	temp = I915_READ(reg);
-	if ((temp & DISPLAY_PLANE_ENABLE) == 0) {
-		I915_WRITE(reg, temp | DISPLAY_PLANE_ENABLE);
-		intel_flush_display_plane(dev, plane);
-	}
+	intel_enable_plane(dev_priv, plane, pipe);
 
 	/* For PCH output, training FDI link */
 	if (IS_GEN6(dev))
@@ -2308,13 +2355,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	drm_vblank_off(dev, pipe);
 	intel_crtc_update_cursor(crtc, false);
 
-	/* Disable display plane */
-	reg = DSPCNTR(plane);
-	temp = I915_READ(reg);
-	if (temp & DISPLAY_PLANE_ENABLE) {
-		I915_WRITE(reg, temp & ~DISPLAY_PLANE_ENABLE);
-		intel_flush_display_plane(dev, plane);
-	}
+	intel_disable_plane(dev_priv, plane, pipe);
 
 	if (dev_priv->cfb_plane == plane &&
 	    dev_priv->display.disable_fbc)
@@ -2519,14 +2560,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	}
 
 	intel_enable_pipe(dev_priv, pipe);
-
-	/* Enable the plane */
-	reg = DSPCNTR(plane);
-	temp = I915_READ(reg);
-	if ((temp & DISPLAY_PLANE_ENABLE) == 0) {
-		I915_WRITE(reg, temp | DISPLAY_PLANE_ENABLE);
-		intel_flush_display_plane(dev, plane);
-	}
+	intel_enable_plane(dev_priv, plane, pipe);
 
 	intel_crtc_load_lut(crtc);
 	intel_update_fbc(dev);
@@ -2558,18 +2592,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	    dev_priv->display.disable_fbc)
 		dev_priv->display.disable_fbc(dev);
 
-	/* Disable display plane */
-	reg = DSPCNTR(plane);
-	temp = I915_READ(reg);
-	if (temp & DISPLAY_PLANE_ENABLE) {
-		I915_WRITE(reg, temp & ~DISPLAY_PLANE_ENABLE);
-		/* Flush the plane changes */
-		intel_flush_display_plane(dev, plane);
-
-		/* Wait for vblank for the disable to take effect */
-		if (IS_GEN2(dev))
-			intel_wait_for_vblank(dev, pipe);
-	}
+	intel_disable_plane(dev_priv, plane, pipe);
 
 	/* Don't disable pipe A or pipe A PLLs if needed */
 	if (pipe == 0 && (dev_priv->quirks & QUIRK_PIPEA_FORCE))
@@ -4315,10 +4338,8 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 			pipeconf &= ~PIPECONF_DOUBLE_WIDE;
 	}
 
-	if (!HAS_PCH_SPLIT(dev)) {
-		dspcntr |= DISPLAY_PLANE_ENABLE;
+	if (!HAS_PCH_SPLIT(dev))
 		dpll |= DPLL_VCO_ENABLE;
-	}
 
 	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe == 0 ? 'A' : 'B');
 	drm_mode_debug_printmodeline(mode);
@@ -4539,6 +4560,9 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 	}
 
 	I915_WRITE(DSPCNTR(plane), dspcntr);
+	POSTING_READ(DSPCNTR(plane));
+	if (!HAS_PCH_SPLIT(dev))
+		intel_enable_plane(dev_priv, plane, pipe);
 
 	ret = intel_pipe_set_base(crtc, x, y, old_fb);
 
@@ -5676,12 +5700,7 @@ static void intel_sanitize_modesetting(struct drm_device *dev,
 	pipe = !pipe;
 
 	/* Disable the plane and wait for it to stop reading from the pipe. */
-	I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
-	intel_flush_display_plane(dev, plane);
-
-	if (IS_GEN2(dev))
-		intel_wait_for_vblank(dev, pipe);
-
+	intel_disable_plane(dev_priv, plane, pipe);
 	intel_disable_pipe(dev_priv, pipe);
 }
 
-- 
1.7.0.4

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

[Chris Wilson]

Trivial comment, on passing...

On Thu, 30 Dec 2010 13:16:30 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Add plane enable/disable functions to prevent duplicated code and allow
> us to easily check for plane enable/disable requirements (such as pipe
> enable).

> +static void intel_disable_plane(struct drm_i915_private *dev_priv,
> +				enum plane plane, enum pipe pipe)
> +{
> +	int reg;
> +	u32 val;
> +
> +	reg = DSPCNTR(plane);
> +	val = I915_READ(reg);
> +	val &= ~DISPLAY_PLANE_ENABLE;
> +	I915_WRITE(reg, val);
> +	POSTING_READ(reg);

> +	reg = DSPADDR(plane);
> +	I915_WRITE(reg, I915_READ(reg)); /* trigger an update */

Use intel_flush_display_plane(dev_priv, plane); though maybe that function
becomes redundant? Unlikely...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jesse Barnes]

On Thu, 30 Dec 2010 21:34:46 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Trivial comment, on passing...
> 
> On Thu, 30 Dec 2010 13:16:30 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Add plane enable/disable functions to prevent duplicated code and allow
> > us to easily check for plane enable/disable requirements (such as pipe
> > enable).
> 
> > +static void intel_disable_plane(struct drm_i915_private *dev_priv,
> > +				enum plane plane, enum pipe pipe)
> > +{
> > +	int reg;
> > +	u32 val;
> > +
> > +	reg = DSPCNTR(plane);
> > +	val = I915_READ(reg);
> > +	val &= ~DISPLAY_PLANE_ENABLE;
> > +	I915_WRITE(reg, val);
> > +	POSTING_READ(reg);
> 
> > +	reg = DSPADDR(plane);
> > +	I915_WRITE(reg, I915_READ(reg)); /* trigger an update */
> 
> Use intel_flush_display_plane(dev_priv, plane); though maybe that function
> becomes redundant? Unlikely...

It was only used in one place after I added this, so I just stuffed the
update trigger into this function and deleted flush_display_plane.

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Chris Wilson]

On Thu, 30 Dec 2010 13:40:30 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Thu, 30 Dec 2010 21:34:46 +0000
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Trivial comment, on passing...
> > 
> > On Thu, 30 Dec 2010 13:16:30 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > Add plane enable/disable functions to prevent duplicated code and allow
> > > us to easily check for plane enable/disable requirements (such as pipe
> > > enable).
> > 
> > > +static void intel_disable_plane(struct drm_i915_private *dev_priv,
> > > +				enum plane plane, enum pipe pipe)
> > > +{
> > > +	int reg;
> > > +	u32 val;
> > > +
> > > +	reg = DSPCNTR(plane);
> > > +	val = I915_READ(reg);
> > > +	val &= ~DISPLAY_PLANE_ENABLE;
> > > +	I915_WRITE(reg, val);
> > > +	POSTING_READ(reg);
> > 
> > > +	reg = DSPADDR(plane);
> > > +	I915_WRITE(reg, I915_READ(reg)); /* trigger an update */
> > 
> > Use intel_flush_display_plane(dev_priv, plane); though maybe that function
> > becomes redundant? Unlikely...
> 
> It was only used in one place after I added this, so I just stuffed the
> update trigger into this function and deleted flush_display_plane.

That was in the next function I read. ;-) I'd argue that the
intel_flush_display_plane(), with the requisite whitespacing, remains
clearer than the block of I915_READ/I915_WRITE + comment.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jesse Barnes]

On Thu, 30 Dec 2010 21:45:22 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 30 Dec 2010 13:40:30 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Thu, 30 Dec 2010 21:34:46 +0000
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > Trivial comment, on passing...
> > > 
> > > On Thu, 30 Dec 2010 13:16:30 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > > Add plane enable/disable functions to prevent duplicated code and allow
> > > > us to easily check for plane enable/disable requirements (such as pipe
> > > > enable).
> > > 
> > > > +static void intel_disable_plane(struct drm_i915_private *dev_priv,
> > > > +				enum plane plane, enum pipe pipe)
> > > > +{
> > > > +	int reg;
> > > > +	u32 val;
> > > > +
> > > > +	reg = DSPCNTR(plane);
> > > > +	val = I915_READ(reg);
> > > > +	val &= ~DISPLAY_PLANE_ENABLE;
> > > > +	I915_WRITE(reg, val);
> > > > +	POSTING_READ(reg);
> > > 
> > > > +	reg = DSPADDR(plane);
> > > > +	I915_WRITE(reg, I915_READ(reg)); /* trigger an update */
> > > 
> > > Use intel_flush_display_plane(dev_priv, plane); though maybe that function
> > > becomes redundant? Unlikely...
> > 
> > It was only used in one place after I added this, so I just stuffed the
> > update trigger into this function and deleted flush_display_plane.
> 
> That was in the next function I read. ;-) I'd argue that the
> intel_flush_display_plane(), with the requisite whitespacing, remains
> clearer than the block of I915_READ/I915_WRITE + comment.

Sure, I can put it back.  Self-documenting function names ftw.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 3/4] drm/i915: add panel lock assertion function

[Jesse Barnes]

When PLLs or timing regs are changed, we need to make sure the panel
lock will allow it.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 800514d..159cf7b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1086,6 +1086,35 @@ static void assert_pll(struct drm_i915_private *dev_priv, enum pipe pipe,
 #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_panel_unlocked(struct drm_i915_private *dev_priv,
+				  enum pipe pipe)
+{
+	int pp_reg, lvds_reg;
+	u32 val;
+	enum pipe panel_pipe = PIPE_A;
+	bool locked = locked;
+
+	if (HAS_PCH_SPLIT(dev_priv->dev)) {
+		pp_reg = PCH_PP_CONTROL;
+		lvds_reg = PCH_LVDS;
+	} else {
+		pp_reg = PP_CONTROL;
+		lvds_reg = LVDS;
+	}
+
+	val = I915_READ(pp_reg);
+	if (!(val & PANEL_POWER_ON) ||
+	    ((val & PANEL_UNLOCK_REGS) == PANEL_UNLOCK_REGS))
+		locked = false;
+
+	if (I915_READ(lvds_reg) & LVDS_PIPEB_SELECT)
+		panel_pipe = PIPE_B;
+
+	WARN(panel_pipe == pipe && locked,
+	     "panel assertion failure, pipe %c regs locked\n",
+	     pipe ? 'B' : 'A');
+}
+
 static void assert_pipe_enabled(struct drm_i915_private *dev_priv,
 				enum pipe pipe)
 {
-- 
1.7.0.4

[PATCH 4/4] drm/i915: add PLL enable/disable functions

[Jesse Barnes]

For pre-ILK only.  Saves some code in the CRTC enable/disable functions
and allows us to check for pipe and panel status at enable/disable time.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 159cf7b..43fe58b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1115,18 +1115,22 @@ static void assert_panel_unlocked(struct drm_i915_private *dev_priv,
 	     pipe ? 'B' : 'A');
 }
 
-static void assert_pipe_enabled(struct drm_i915_private *dev_priv,
-				enum pipe pipe)
+static void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
+			enum state state)
 {
 	int reg;
 	u32 val;
+	bool cur_state;
 
 	reg = PIPECONF(pipe);
 	val = I915_READ(reg);
-	WARN(!(val & PIPECONF_ENABLE),
-	     "pipe %c assertion failure, should be active but is disabled\n",
-	     pipe ? 'B' : 'A');
+	cur_state = !!(val & PIPECONF_ENABLE);
+	WARN(cur_state != state,
+	     "pipe %c assertion failure (expected %s, current %s)\n",
+	     pipe ? 'B' : 'A', state_string(state), state_string(cur_state));
 }
+#define assert_pipe_enabled(d, p) assert_pipe(d, p, on)
+#define assert_pipe_disabled(d, p) assert_pipe(d, p, off)
 
 static void assert_plane_enabled(struct drm_i915_private *dev_priv,
 				 enum plane plane)
@@ -1161,6 +1165,70 @@ static void assert_planes_disabled(struct drm_i915_private *dev_priv,
 }
 
 /**
+ * intel_enable_pll - enable a PLL
+ * @dev_priv: i915 private structure
+ * @pipe: pipe PLL to enable
+ *
+ * Enable @pipe's PLL so we can start pumping pixels from a plane.  Check to
+ * make sure the PLL reg is writable first though, since the panel write
+ * protect mechanism may be enabled.
+ *
+ * Note!  This is for pre-ILK only.
+ */
+static void intel_enable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
+{
+	int reg;
+	u32 val;
+
+	/* PLL is protected by panel, make sure we can write it */
+	if (IS_MOBILE(dev_priv->dev) && !IS_I830(dev_priv->dev))
+		assert_panel_unlocked(dev_priv, pipe);
+
+	/* Don't disable pipe A or pipe A PLLs if needed */
+	if (pipe == PIPE_A && (dev_priv->quirks & QUIRK_PIPEA_FORCE))
+		return;
+
+	reg = DPLL(pipe);
+	val = I915_READ(reg);
+	val |= DPLL_VCO_ENABLE;
+
+	/* We do this three times for luck */
+	I915_WRITE(reg, val);
+	POSTING_READ(reg);
+	udelay(150); /* wait for warmup */
+	I915_WRITE(reg, val);
+	POSTING_READ(reg);
+	udelay(150); /* wait for warmup */
+	I915_WRITE(reg, val);
+	POSTING_READ(reg);
+	udelay(150); /* wait for warmup */
+}
+
+/**
+ * intel_disable_pll - disable a PLL
+ * @dev_priv: i915 private structure
+ * @pipe: pipe PLL to disable
+ *
+ * Disable the PLL for @pipe, making sure the pipe is off first.
+ *
+ * Note!  This is for pre-ILK only.
+ */
+static void intel_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
+{
+	int reg;
+	u32 val;
+
+	/* Make sure the pipe isn't still relying on us */
+	assert_pipe_disabled(dev_priv, pipe);
+
+	reg = DPLL(pipe);
+	val = I915_READ(reg);
+	val &= ~DPLL_VCO_ENABLE;
+	I915_WRITE(reg, val);
+	POSTING_READ(reg);
+}
+
+/**
  * intel_enable_pipe - enable a pipe, assertiing requirements
  * @dev_priv: i915 private structure
  * @pipe: pipe to enable
@@ -2557,7 +2625,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
-	u32 reg, temp;
 
 	if (intel_crtc->active)
 		return;
@@ -2565,29 +2632,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc->active = true;
 	intel_update_watermarks(dev);
 
-	/* Enable the DPLL */
-	reg = DPLL(pipe);
-	temp = I915_READ(reg);
-	if ((temp & DPLL_VCO_ENABLE) == 0) {
-		I915_WRITE(reg, temp);
-
-		/* Wait for the clocks to stabilize. */
-		POSTING_READ(reg);
-		udelay(150);
-
-		I915_WRITE(reg, temp | DPLL_VCO_ENABLE);
-
-		/* Wait for the clocks to stabilize. */
-		POSTING_READ(reg);
-		udelay(150);
-
-		I915_WRITE(reg, temp | DPLL_VCO_ENABLE);
-
-		/* Wait for the clocks to stabilize. */
-		POSTING_READ(reg);
-		udelay(150);
-	}
-
+	intel_enable_pll(dev_priv, pipe);
 	intel_enable_pipe(dev_priv, pipe);
 	intel_enable_plane(dev_priv, plane, pipe);
 
@@ -2606,7 +2651,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
-	u32 reg, temp;
 
 	if (!intel_crtc->active)
 		return;
@@ -2622,24 +2666,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 		dev_priv->display.disable_fbc(dev);
 
 	intel_disable_plane(dev_priv, plane, pipe);
-
-	/* Don't disable pipe A or pipe A PLLs if needed */
-	if (pipe == 0 && (dev_priv->quirks & QUIRK_PIPEA_FORCE))
-		goto done;
-
 	intel_disable_pipe(dev_priv, pipe);
+	intel_disable_pll(dev_priv, pipe);
 
-	reg = DPLL(pipe);
-	temp = I915_READ(reg);
-	if (temp & DPLL_VCO_ENABLE) {
-		I915_WRITE(reg, temp & ~DPLL_VCO_ENABLE);
-
-		/* Wait for the clocks to turn off. */
-		POSTING_READ(reg);
-		udelay(150);
-	}
-
-done:
 	intel_crtc->active = false;
 	intel_update_fbc(dev);
 	intel_update_watermarks(dev);
-- 
1.7.0.4

Re: [PATCH 4/4] drm/i915: add PLL enable/disable functions

[Chris Wilson]

On Thu, 30 Dec 2010 13:16:32 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>  /**
> + * intel_enable_pll - enable a PLL
> + * @dev_priv: i915 private structure
> + * @pipe: pipe PLL to enable
> + *
> + * Enable @pipe's PLL so we can start pumping pixels from a plane.  Check to
> + * make sure the PLL reg is writable first though, since the panel write
> + * protect mechanism may be enabled.
> + *
> + * Note!  This is for pre-ILK only.
> + */
> +static void intel_enable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> +{
> +	int reg;
> +	u32 val;

Reinforce the comments with a BUG_ON(dev_priv->info->gen >= 5);
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: add PLL enable/disable functions

[Jesse Barnes]

On Thu, 30 Dec 2010 21:41:18 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 30 Dec 2010 13:16:32 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >  /**
> > + * intel_enable_pll - enable a PLL
> > + * @dev_priv: i915 private structure
> > + * @pipe: pipe PLL to enable
> > + *
> > + * Enable @pipe's PLL so we can start pumping pixels from a plane.  Check to
> > + * make sure the PLL reg is writable first though, since the panel write
> > + * protect mechanism may be enabled.
> > + *
> > + * Note!  This is for pre-ILK only.
> > + */
> > +static void intel_enable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> > +{
> > +	int reg;
> > +	u32 val;
> 
> Reinforce the comments with a BUG_ON(dev_priv->info->gen >= 5);

Ah good call.  Fixed.  Also moved the "don't disable" quirks check into
the proper function (the disable one!).

-- 
Jesse Barnes, Intel Open Source Technology Center