[PATCH 0/4] drm/i915: Reorder planes vs. modeset on gmch platforms

[PATCH 0/4] drm/i915: Reorder planes vs. modeset on gmch platforms

[ville.syrjala]

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

This series unifies all the platforms to disable/enable planes as the
first/last during a modeset. The end result is pretty much similar to
a patch I posted a long time ago, but it just got applied in stages;
first hsw, then other pch platforms, and now finally gmch platforms.

I also kill off a bunch of the vblank waits that shouldn't really
be there, and may just time out depending on whether the pipe actually
starts to run immediately or only when the port gets enabled.

I gave this a whirl on 855,946,g45 and vlv, and everything looks to be
working as well as before.

Ville Syrjälä (4):
  drm/i915: Convert gmch platforms over to
    ilk_crtc_{enable,disable}_planes()
  drm/i915: Disable/enable planes as the first/last thing during modeset
    on gmch platforms
  drm/i915: Kill vblank waits after pipe enable on gmch platforms
  drm/i915: Move the drm_vblank_on() calls to intel_crtc_enable_planes()

 drivers/gpu/drm/i915/intel_display.c | 150 ++++++++++++++---------------------
 1 file changed, 59 insertions(+), 91 deletions(-)

-- 
1.8.3.2

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

[PATCH 1/4] drm/i915: Convert gmch platforms over to ilk_crtc_{enable, disable}_planes()

[ville.syrjala]

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

Use the same code for enabling/disabling planes on all platforms. Rename
the functions to reflect that they're no longer specific to any
platform.

For now we leave the plane enable/disable to ccur at the same old
position in the modeset sequence.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c65e7f7..3ee4995 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3634,7 +3634,49 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
 		hsw_enable_ips(intel_crtc);
 }
 
-static void ilk_crtc_enable_planes(struct drm_crtc *crtc)
+static void intel_crtc_dpms_overlay(struct intel_crtc *intel_crtc, bool enable)
+{
+	if (!enable && intel_crtc->overlay) {
+		struct drm_device *dev = intel_crtc->base.dev;
+		struct drm_i915_private *dev_priv = dev->dev_private;
+
+		mutex_lock(&dev->struct_mutex);
+		dev_priv->mm.interruptible = false;
+		(void) intel_overlay_switch_off(intel_crtc->overlay);
+		dev_priv->mm.interruptible = true;
+		mutex_unlock(&dev->struct_mutex);
+	}
+
+	/* Let userspace switch the overlay on again. In most cases userspace
+	 * has to recompute where to put it anyway.
+	 */
+}
+
+/**
+ * i9xx_fixup_plane - ugly workaround for G45 to fire up the hardware
+ * cursor plane briefly if not already running after enabling the display
+ * plane.
+ * This workaround avoids occasional blank screens when self refresh is
+ * enabled.
+ */
+static void
+g4x_fixup_plane(struct drm_i915_private *dev_priv, enum pipe pipe)
+{
+	u32 cntl = I915_READ(CURCNTR(pipe));
+
+	if ((cntl & CURSOR_MODE) == 0) {
+		u32 fw_bcl_self = I915_READ(FW_BLC_SELF);
+
+		I915_WRITE(FW_BLC_SELF, fw_bcl_self & ~FW_BLC_SELF_EN);
+		I915_WRITE(CURCNTR(pipe), CURSOR_MODE_64_ARGB_AX);
+		intel_wait_for_vblank(dev_priv->dev, pipe);
+		I915_WRITE(CURCNTR(pipe), cntl);
+		I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
+		I915_WRITE(FW_BLC_SELF, fw_bcl_self);
+	}
+}
+
+static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3644,7 +3686,11 @@ static void ilk_crtc_enable_planes(struct drm_crtc *crtc)
 
 	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
+	/* The fixup needs to happen before cursor is enabled */
+	if (IS_G4X(dev))
+		g4x_fixup_plane(dev_priv, pipe);
 	intel_crtc_update_cursor(crtc, true);
+	intel_crtc_dpms_overlay(intel_crtc, true);
 
 	hsw_enable_ips(intel_crtc);
 
@@ -3653,7 +3699,7 @@ static void ilk_crtc_enable_planes(struct drm_crtc *crtc)
 	mutex_unlock(&dev->struct_mutex);
 }
 
-static void ilk_crtc_disable_planes(struct drm_crtc *crtc)
+static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3669,6 +3715,7 @@ static void ilk_crtc_disable_planes(struct drm_crtc *crtc)
 
 	hsw_disable_ips(intel_crtc);
 
+	intel_crtc_dpms_overlay(intel_crtc, false);
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
 	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
@@ -3726,7 +3773,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	if (HAS_PCH_CPT(dev))
 		cpt_verify_modeset(dev, intel_crtc->pipe);
 
-	ilk_crtc_enable_planes(crtc);
+	intel_crtc_enable_planes(crtc);
 
 	/*
 	 * There seems to be a race in PCH platform hw (at least on some
@@ -3829,7 +3876,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	/* If we change the relative order between pipe/planes enabling, we need
 	 * to change the workaround. */
 	haswell_mode_set_planes_workaround(intel_crtc);
-	ilk_crtc_enable_planes(crtc);
+	intel_crtc_enable_planes(crtc);
 
 	drm_vblank_on(dev, pipe);
 }
@@ -3861,7 +3908,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	if (!intel_crtc->active)
 		return;
 
-	ilk_crtc_disable_planes(crtc);
+	intel_crtc_disable_planes(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
@@ -3924,7 +3971,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	if (!intel_crtc->active)
 		return;
 
-	ilk_crtc_disable_planes(crtc);
+	intel_crtc_disable_planes(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		intel_opregion_notify_encoder(encoder, false);
@@ -3970,48 +4017,6 @@ static void haswell_crtc_off(struct drm_crtc *crtc)
 	intel_ddi_put_crtc_pll(crtc);
 }
 
-static void intel_crtc_dpms_overlay(struct intel_crtc *intel_crtc, bool enable)
-{
-	if (!enable && intel_crtc->overlay) {
-		struct drm_device *dev = intel_crtc->base.dev;
-		struct drm_i915_private *dev_priv = dev->dev_private;
-
-		mutex_lock(&dev->struct_mutex);
-		dev_priv->mm.interruptible = false;
-		(void) intel_overlay_switch_off(intel_crtc->overlay);
-		dev_priv->mm.interruptible = true;
-		mutex_unlock(&dev->struct_mutex);
-	}
-
-	/* Let userspace switch the overlay on again. In most cases userspace
-	 * has to recompute where to put it anyway.
-	 */
-}
-
-/**
- * i9xx_fixup_plane - ugly workaround for G45 to fire up the hardware
- * cursor plane briefly if not already running after enabling the display
- * plane.
- * This workaround avoids occasional blank screens when self refresh is
- * enabled.
- */
-static void
-g4x_fixup_plane(struct drm_i915_private *dev_priv, enum pipe pipe)
-{
-	u32 cntl = I915_READ(CURCNTR(pipe));
-
-	if ((cntl & CURSOR_MODE) == 0) {
-		u32 fw_bcl_self = I915_READ(FW_BLC_SELF);
-
-		I915_WRITE(FW_BLC_SELF, fw_bcl_self & ~FW_BLC_SELF_EN);
-		I915_WRITE(CURCNTR(pipe), CURSOR_MODE_64_ARGB_AX);
-		intel_wait_for_vblank(dev_priv->dev, pipe);
-		I915_WRITE(CURCNTR(pipe), cntl);
-		I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
-		I915_WRITE(FW_BLC_SELF, fw_bcl_self);
-	}
-}
-
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -4315,7 +4320,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
-	int plane = intel_crtc->plane;
 	bool is_dsi;
 
 	WARN_ON(!crtc->enabled);
@@ -4347,11 +4351,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
 
-	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
-	intel_enable_planes(crtc);
-	intel_crtc_update_cursor(crtc, true);
-
-	intel_update_fbc(dev);
+	intel_crtc_enable_planes(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
@@ -4366,7 +4366,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
-	int plane = intel_crtc->plane;
 
 	WARN_ON(!crtc->enabled);
 
@@ -4390,17 +4389,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
 
-	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
-	intel_enable_planes(crtc);
-	/* The fixup needs to happen before cursor is enabled */
-	if (IS_G4X(dev))
-		g4x_fixup_plane(dev_priv, pipe);
-	intel_crtc_update_cursor(crtc, true);
-
-	/* Give the overlay scaler a chance to enable if it's on this pipe */
-	intel_crtc_dpms_overlay(intel_crtc, true);
-
-	intel_update_fbc(dev);
+	intel_crtc_enable_planes(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
@@ -4430,7 +4419,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
-	int plane = intel_crtc->plane;
 
 	if (!intel_crtc->active)
 		return;
@@ -4438,17 +4426,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
 
-	/* Give the overlay scaler a chance to disable if it's on this pipe */
-	intel_crtc_wait_for_pending_flips(crtc);
-	drm_vblank_off(dev, pipe);
-
-	if (dev_priv->fbc.plane == plane)
-		intel_disable_fbc(dev);
-
-	intel_crtc_dpms_overlay(intel_crtc, false);
-	intel_crtc_update_cursor(crtc, false);
-	intel_disable_planes(crtc);
-	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
+	intel_crtc_disable_planes(crtc);
 
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
 	intel_disable_pipe(dev_priv, pipe);
-- 
1.8.3.2

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

[PATCH 2/4] drm/i915: Disable/enable planes as the first/last thing during modeset on gmch platforms

[ville.syrjala]

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

We already moved the plane disable/enable to happen as the first/last
thing on every other platforms. Follow suit with gmch platforms.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3ee4995..fec618e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4351,11 +4351,11 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
 
-	intel_crtc_enable_planes(crtc);
-
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
 
+	intel_crtc_enable_planes(crtc);
+
 	drm_vblank_on(dev, pipe);
 }
 
@@ -4389,11 +4389,11 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
 
-	intel_crtc_enable_planes(crtc);
-
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
 
+	intel_crtc_enable_planes(crtc);
+
 	drm_vblank_on(dev, pipe);
 }
 
@@ -4423,11 +4423,11 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	if (!intel_crtc->active)
 		return;
 
+	intel_crtc_disable_planes(crtc);
+
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
 
-	intel_crtc_disable_planes(crtc);
-
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
 	intel_disable_pipe(dev_priv, pipe);
 
-- 
1.8.3.2

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

[PATCH 3/4] drm/i915: Kill vblank waits after pipe enable on gmch platforms

[ville.syrjala]

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

The pipe might not start to actually run until the port has been enabled
(depends on the platform and port type). So don't try to wait for vblank
after we enabled the pipe but haven't yet enabled the port.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fec618e..820c3a9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4316,7 +4316,6 @@ static void valleyview_modeset_global_resources(struct drm_device *dev)
 static void valleyview_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
@@ -4348,7 +4347,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(intel_crtc);
-	intel_wait_for_vblank(dev_priv->dev, pipe);
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
@@ -4362,7 +4360,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 static void i9xx_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
@@ -4386,7 +4383,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(intel_crtc);
-	intel_wait_for_vblank(dev_priv->dev, pipe);
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
-- 
1.8.3.2

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

[PATCH 4/4] drm/i915: Move the drm_vblank_on() calls to intel_crtc_enable_planes()

[ville.syrjala]

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

We always enable vblank interrupts after the planes so move the call
into intel_crtc_enable_planes(). Later we will reorder the plane vs.
vblank enable/disable to allow vblank interrupts while the planes are
getting configured.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 820c3a9..aa40c813 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3697,6 +3697,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
+
+	drm_vblank_on(dev, pipe);
 }
 
 static void intel_crtc_disable_planes(struct drm_crtc *crtc)
@@ -3784,8 +3786,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	 * happening.
 	 */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
-
-	drm_vblank_on(dev, pipe);
 }
 
 /* IPS only exists on ULT machines and is tied to pipe A. */
@@ -3877,8 +3877,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	 * to change the workaround. */
 	haswell_mode_set_planes_workaround(intel_crtc);
 	intel_crtc_enable_planes(crtc);
-
-	drm_vblank_on(dev, pipe);
 }
 
 static void ironlake_pfit_disable(struct intel_crtc *crtc)
@@ -4353,8 +4351,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 		encoder->enable(encoder);
 
 	intel_crtc_enable_planes(crtc);
-
-	drm_vblank_on(dev, pipe);
 }
 
 static void i9xx_crtc_enable(struct drm_crtc *crtc)
@@ -4389,8 +4385,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 		encoder->enable(encoder);
 
 	intel_crtc_enable_planes(crtc);
-
-	drm_vblank_on(dev, pipe);
 }
 
 static void i9xx_pfit_disable(struct intel_crtc *crtc)
-- 
1.8.3.2

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

Re: [PATCH 4/4] drm/i915: Move the drm_vblank_on() calls to intel_crtc_enable_planes()

[Chris Wilson]

On Thu, May 08, 2014 at 07:23:16PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We always enable vblank interrupts after the planes so move the call
> into intel_crtc_enable_planes(). Later we will reorder the plane vs.
> vblank enable/disable to allow vblank interrupts while the planes are
> getting configured.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/4] drm/i915: Convert gmch platforms over to ilk_crtc_{enable, disable}_planes()

[Chris Wilson]

On Thu, May 08, 2014 at 07:23:13PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Use the same code for enabling/disabling planes on all platforms. Rename
> the functions to reflect that they're no longer specific to any
> platform.
> 
> For now we leave the plane enable/disable to ccur at the same old
> position in the modeset sequence.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I think this looks ok, I didn't spot any functional change.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/4] drm/i915: Kill vblank waits after pipe enable on gmch platforms

[Chris Wilson]

On Thu, May 08, 2014 at 07:23:15PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The pipe might not start to actually run until the port has been enabled
> (depends on the platform and port type). So don't try to wait for vblank
> after we enabled the pipe but haven't yet enabled the port.

I am pretty sure those waits were in the docs. Pretty sure, not certain
though.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] drm/i915: Disable/enable planes as the first/last thing during modeset on gmch platforms

[Chris Wilson]

On Thu, May 08, 2014 at 07:23:14PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We already moved the plane disable/enable to happen as the first/last
> thing on every other platforms. Follow suit with gmch platforms.

There is merit in the argument here. Still nerve wracking. Will need to
check to see if there a counter-argument in the specs.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/4] drm/i915: Kill vblank waits after pipe enable on gmch platforms

[Ville Syrjälä]

On Fri, May 09, 2014 at 07:03:09AM +0100, Chris Wilson wrote:
> On Thu, May 08, 2014 at 07:23:15PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The pipe might not start to actually run until the port has been enabled
> > (depends on the platform and port type). So don't try to wait for vblank
> > after we enabled the pipe but haven't yet enabled the port.
> 
> I am pretty sure those waits were in the docs. Pretty sure, not certain
> though.

I didn't find them anywhere. The only vblank waits I see are the ones
for gen2 between disabling the planes and the pipe since the pipe
disable isn't double buffered on vblank. And that wait is still there
due to intel_disable_primary_hw_plane(). If my mmio vs. CS flip race
series ever gets reviewed that also gets killed, but I do add back an
explicit vblank wait for gen2 only into i9xx_crtc_disable().

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/4] drm/i915: Disable/enable planes as the first/last thing during modeset on gmch platforms

[Ville Syrjälä]

On Fri, May 09, 2014 at 07:22:16AM +0100, Chris Wilson wrote:
> On Thu, May 08, 2014 at 07:23:14PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We already moved the plane disable/enable to happen as the first/last
> > thing on every other platforms. Follow suit with gmch platforms.
> 
> There is merit in the argument here. Still nerve wracking. Will need to
> check to see if there a counter-argument in the specs.

I've gone through the specs and didn't spot anything explicit. Some
of the modeset sequences do list enabling planes before the ports
and disabling planes after the ports though. But eg. in gen2/3 specs
there are two different sequences: one for modeset, and one for
enable/disable. In the modeset sequence only planes and pipes are
toggled off and on and ports are left enabled for the whole time,
and in the enable/disable sequence ports are turned off first and
back on last. So I think in general you can do these in any order
you like as long as planes aren't enabled when the pipe isn't.

Also we can turn the planes on and off as we like while everything is
up and running so I don't see why the modeset sequence should really
care about planes.

And we should really be able to light up the display without any planes
being active, eg. if a screensaver just turns off all the planes to
show a black screen and then DPMS kicks in and turns everything off.
When it comes back from DPMS the user should still see the same black
screen until some plane gets enabled.

But still if you find anything alarming in the specs I may need to
rethink this.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 4/4] drm/i915: Move the drm_vblank_on() calls to intel_crtc_enable_planes()

[Daniel Vetter]

On Fri, May 09, 2014 at 06:53:24AM +0100, Chris Wilson wrote:
> On Thu, May 08, 2014 at 07:23:16PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We always enable vblank interrupts after the planes so move the call
> > into intel_crtc_enable_planes(). Later we will reorder the plane vs.
> > vblank enable/disable to allow vblank interrupts while the planes are
> > getting configured.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I've dug myself a conflict hell grave by stalling on the core vblank
rework stuff too long ... Working on that now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm/i915: Convert gmch platforms over to ilk_crtc_{enable, disable}_planes()

[Daniel Vetter]

On Fri, May 09, 2014 at 07:02:08AM +0100, Chris Wilson wrote:
> On Thu, May 08, 2014 at 07:23:13PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Use the same code for enabling/disabling planes on all platforms. Rename
> > the functions to reflect that they're no longer specific to any
> > platform.
> > 
> > For now we leave the plane enable/disable to ccur at the same old
> > position in the modeset sequence.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I think this looks ok, I didn't spot any functional change.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Merged, with the usual drm_vblank_on conflict frobbed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm/i915: Convert gmch platforms over to ilk_crtc_{enable, disable}_planes()

[Daniel Vetter]

On Thu, May 08, 2014 at 07:23:13PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Use the same code for enabling/disabling planes on all platforms. Rename
> the functions to reflect that they're no longer specific to any
> platform.
> 
> For now we leave the plane enable/disable to ccur at the same old
> position in the modeset sequence.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 134 +++++++++++++++--------------------
>  1 file changed, 56 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c65e7f7..3ee4995 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3634,7 +3634,49 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
>  		hsw_enable_ips(intel_crtc);
>  }
>  
> -static void ilk_crtc_enable_planes(struct drm_crtc *crtc)
> +static void intel_crtc_dpms_overlay(struct intel_crtc *intel_crtc, bool enable)
> +{
> +	if (!enable && intel_crtc->overlay) {
> +		struct drm_device *dev = intel_crtc->base.dev;
> +		struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +		mutex_lock(&dev->struct_mutex);
> +		dev_priv->mm.interruptible = false;
> +		(void) intel_overlay_switch_off(intel_crtc->overlay);
> +		dev_priv->mm.interruptible = true;
> +		mutex_unlock(&dev->struct_mutex);
> +	}
> +
> +	/* Let userspace switch the overlay on again. In most cases userspace
> +	 * has to recompute where to put it anyway.
> +	 */
> +}
> +
> +/**
> + * i9xx_fixup_plane - ugly workaround for G45 to fire up the hardware
> + * cursor plane briefly if not already running after enabling the display
> + * plane.
> + * This workaround avoids occasional blank screens when self refresh is
> + * enabled.
> + */
> +static void
> +g4x_fixup_plane(struct drm_i915_private *dev_priv, enum pipe pipe)
> +{
> +	u32 cntl = I915_READ(CURCNTR(pipe));
> +
> +	if ((cntl & CURSOR_MODE) == 0) {
> +		u32 fw_bcl_self = I915_READ(FW_BLC_SELF);
> +
> +		I915_WRITE(FW_BLC_SELF, fw_bcl_self & ~FW_BLC_SELF_EN);
> +		I915_WRITE(CURCNTR(pipe), CURSOR_MODE_64_ARGB_AX);
> +		intel_wait_for_vblank(dev_priv->dev, pipe);
> +		I915_WRITE(CURCNTR(pipe), cntl);
> +		I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
> +		I915_WRITE(FW_BLC_SELF, fw_bcl_self);
> +	}
> +}
> +
> +static void intel_crtc_enable_planes(struct drm_crtc *crtc)

i9xx_crtc_enable_planes might have been better, given Damien's pet
project. But otoh we might just want to switch to doing a generic atomic
pageflip across all planes before we call down into crtc_disable hooks
(and the reverse after crtc_enable), so meh ;-)
-Daniel

>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3644,7 +3686,11 @@ static void ilk_crtc_enable_planes(struct drm_crtc *crtc)
>  
>  	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
> +	/* The fixup needs to happen before cursor is enabled */
> +	if (IS_G4X(dev))
> +		g4x_fixup_plane(dev_priv, pipe);
>  	intel_crtc_update_cursor(crtc, true);
> +	intel_crtc_dpms_overlay(intel_crtc, true);
>  
>  	hsw_enable_ips(intel_crtc);
>  
> @@ -3653,7 +3699,7 @@ static void ilk_crtc_enable_planes(struct drm_crtc *crtc)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> -static void ilk_crtc_disable_planes(struct drm_crtc *crtc)
> +static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3669,6 +3715,7 @@ static void ilk_crtc_disable_planes(struct drm_crtc *crtc)
>  
>  	hsw_disable_ips(intel_crtc);
>  
> +	intel_crtc_dpms_overlay(intel_crtc, false);
>  	intel_crtc_update_cursor(crtc, false);
>  	intel_disable_planes(crtc);
>  	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
> @@ -3726,7 +3773,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	if (HAS_PCH_CPT(dev))
>  		cpt_verify_modeset(dev, intel_crtc->pipe);
>  
> -	ilk_crtc_enable_planes(crtc);
> +	intel_crtc_enable_planes(crtc);
>  
>  	/*
>  	 * There seems to be a race in PCH platform hw (at least on some
> @@ -3829,7 +3876,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	/* If we change the relative order between pipe/planes enabling, we need
>  	 * to change the workaround. */
>  	haswell_mode_set_planes_workaround(intel_crtc);
> -	ilk_crtc_enable_planes(crtc);
> +	intel_crtc_enable_planes(crtc);
>  
>  	drm_vblank_on(dev, pipe);
>  }
> @@ -3861,7 +3908,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	if (!intel_crtc->active)
>  		return;
>  
> -	ilk_crtc_disable_planes(crtc);
> +	intel_crtc_disable_planes(crtc);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->disable(encoder);
> @@ -3924,7 +3971,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	if (!intel_crtc->active)
>  		return;
>  
> -	ilk_crtc_disable_planes(crtc);
> +	intel_crtc_disable_planes(crtc);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		intel_opregion_notify_encoder(encoder, false);
> @@ -3970,48 +4017,6 @@ static void haswell_crtc_off(struct drm_crtc *crtc)
>  	intel_ddi_put_crtc_pll(crtc);
>  }
>  
> -static void intel_crtc_dpms_overlay(struct intel_crtc *intel_crtc, bool enable)
> -{
> -	if (!enable && intel_crtc->overlay) {
> -		struct drm_device *dev = intel_crtc->base.dev;
> -		struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -		mutex_lock(&dev->struct_mutex);
> -		dev_priv->mm.interruptible = false;
> -		(void) intel_overlay_switch_off(intel_crtc->overlay);
> -		dev_priv->mm.interruptible = true;
> -		mutex_unlock(&dev->struct_mutex);
> -	}
> -
> -	/* Let userspace switch the overlay on again. In most cases userspace
> -	 * has to recompute where to put it anyway.
> -	 */
> -}
> -
> -/**
> - * i9xx_fixup_plane - ugly workaround for G45 to fire up the hardware
> - * cursor plane briefly if not already running after enabling the display
> - * plane.
> - * This workaround avoids occasional blank screens when self refresh is
> - * enabled.
> - */
> -static void
> -g4x_fixup_plane(struct drm_i915_private *dev_priv, enum pipe pipe)
> -{
> -	u32 cntl = I915_READ(CURCNTR(pipe));
> -
> -	if ((cntl & CURSOR_MODE) == 0) {
> -		u32 fw_bcl_self = I915_READ(FW_BLC_SELF);
> -
> -		I915_WRITE(FW_BLC_SELF, fw_bcl_self & ~FW_BLC_SELF_EN);
> -		I915_WRITE(CURCNTR(pipe), CURSOR_MODE_64_ARGB_AX);
> -		intel_wait_for_vblank(dev_priv->dev, pipe);
> -		I915_WRITE(CURCNTR(pipe), cntl);
> -		I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
> -		I915_WRITE(FW_BLC_SELF, fw_bcl_self);
> -	}
> -}
> -
>  static void i9xx_pfit_enable(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> @@ -4315,7 +4320,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
> -	int plane = intel_crtc->plane;
>  	bool is_dsi;
>  
>  	WARN_ON(!crtc->enabled);
> @@ -4347,11 +4351,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	intel_wait_for_vblank(dev_priv->dev, pipe);
>  	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
>  
> -	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
> -	intel_enable_planes(crtc);
> -	intel_crtc_update_cursor(crtc, true);
> -
> -	intel_update_fbc(dev);
> +	intel_crtc_enable_planes(crtc);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> @@ -4366,7 +4366,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
> -	int plane = intel_crtc->plane;
>  
>  	WARN_ON(!crtc->enabled);
>  
> @@ -4390,17 +4389,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	intel_wait_for_vblank(dev_priv->dev, pipe);
>  	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
>  
> -	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
> -	intel_enable_planes(crtc);
> -	/* The fixup needs to happen before cursor is enabled */
> -	if (IS_G4X(dev))
> -		g4x_fixup_plane(dev_priv, pipe);
> -	intel_crtc_update_cursor(crtc, true);
> -
> -	/* Give the overlay scaler a chance to enable if it's on this pipe */
> -	intel_crtc_dpms_overlay(intel_crtc, true);
> -
> -	intel_update_fbc(dev);
> +	intel_crtc_enable_planes(crtc);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> @@ -4430,7 +4419,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
> -	int plane = intel_crtc->plane;
>  
>  	if (!intel_crtc->active)
>  		return;
> @@ -4438,17 +4426,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->disable(encoder);
>  
> -	/* Give the overlay scaler a chance to disable if it's on this pipe */
> -	intel_crtc_wait_for_pending_flips(crtc);
> -	drm_vblank_off(dev, pipe);
> -
> -	if (dev_priv->fbc.plane == plane)
> -		intel_disable_fbc(dev);
> -
> -	intel_crtc_dpms_overlay(intel_crtc, false);
> -	intel_crtc_update_cursor(crtc, false);
> -	intel_disable_planes(crtc);
> -	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
> +	intel_crtc_disable_planes(crtc);
>  
>  	intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
>  	intel_disable_pipe(dev_priv, pipe);
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/4] drm/i915: Disable/enable planes as the first/last thing during modeset on gmch platforms

[Chris Wilson]

On Fri, May 09, 2014 at 12:28:19PM +0300, Ville Syrjälä wrote:
> On Fri, May 09, 2014 at 07:22:16AM +0100, Chris Wilson wrote:
> > On Thu, May 08, 2014 at 07:23:14PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > We already moved the plane disable/enable to happen as the first/last
> > > thing on every other platforms. Follow suit with gmch platforms.
> > 
> > There is merit in the argument here. Still nerve wracking. Will need to
> > check to see if there a counter-argument in the specs.
> 
> I've gone through the specs and didn't spot anything explicit. Some
> of the modeset sequences do list enabling planes before the ports
> and disabling planes after the ports though. But eg. in gen2/3 specs
> there are two different sequences: one for modeset, and one for
> enable/disable. In the modeset sequence only planes and pipes are
> toggled off and on and ports are left enabled for the whole time,
> and in the enable/disable sequence ports are turned off first and
> back on last. So I think in general you can do these in any order
> you like as long as planes aren't enabled when the pipe isn't.

Right, the specs are nicely muddled with different sequences for
different encoders. But whenever it turns the pipe (or keeps it on for
the sequence) before the planes it has the caveat "only registers that
are double buffered should be updated while the display pipe is enabled
in order to avoid screen glitches".

So yes, so long as we adhere to that rule, it does look like the
pipe/plane ordering is arbitrary.
 
> Also we can turn the planes on and off as we like while everything is
> up and running so I don't see why the modeset sequence should really
> care about planes.
> 
> And we should really be able to light up the display without any planes
> being active, eg. if a screensaver just turns off all the planes to
> show a black screen and then DPMS kicks in and turns everything off.
> When it comes back from DPMS the user should still see the same black
> screen until some plane gets enabled.
> 
> But still if you find anything alarming in the specs I may need to
> rethink this.

Nope. Though we can harangue Daniel for not delivering the quick
modechange for integrated panels. ;

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/4] drm/i915: Kill vblank waits after pipe enable on gmch platforms

[Chris Wilson]

On Fri, May 09, 2014 at 12:09:46PM +0300, Ville Syrjälä wrote:
> On Fri, May 09, 2014 at 07:03:09AM +0100, Chris Wilson wrote:
> > On Thu, May 08, 2014 at 07:23:15PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The pipe might not start to actually run until the port has been enabled
> > > (depends on the platform and port type). So don't try to wait for vblank
> > > after we enabled the pipe but haven't yet enabled the port.
> > 
> > I am pretty sure those waits were in the docs. Pretty sure, not certain
> > though.
> 
> I didn't find them anywhere. The only vblank waits I see are the ones
> for gen2 between disabling the planes and the pipe since the pipe
> disable isn't double buffered on vblank. And that wait is still there
> due to intel_disable_primary_hw_plane(). If my mmio vs. CS flip race
> series ever gets reviewed that also gets killed, but I do add back an
> explicit vblank wait for gen2 only into i9xx_crtc_disable().

Right, there is only the explicit wait required for the panel scaler, as
that more or less requires a sequence of writes to the double-buffered
registers. Otherwise, the waits I remember are just warning that the
register updates are double-buffered. So removing the waits for the pipe
on seems a resonable thing.

In the event that we enable then disable the pipe all within a single
frame (i.e. before the enable register updates are written) this should
still work as the next register update will take the disable values for
the current request. I.e. I don't think we need to insert explicit waits
between enable/disabled.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/4] drm/i915: Kill vblank waits after pipe enable on gmch platforms

[Daniel Vetter]

On Tue, May 13, 2014 at 09:28:34AM +0100, Chris Wilson wrote:
> On Fri, May 09, 2014 at 12:09:46PM +0300, Ville Syrjälä wrote:
> > On Fri, May 09, 2014 at 07:03:09AM +0100, Chris Wilson wrote:
> > > On Thu, May 08, 2014 at 07:23:15PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > The pipe might not start to actually run until the port has been enabled
> > > > (depends on the platform and port type). So don't try to wait for vblank
> > > > after we enabled the pipe but haven't yet enabled the port.
> > > 
> > > I am pretty sure those waits were in the docs. Pretty sure, not certain
> > > though.
> > 
> > I didn't find them anywhere. The only vblank waits I see are the ones
> > for gen2 between disabling the planes and the pipe since the pipe
> > disable isn't double buffered on vblank. And that wait is still there
> > due to intel_disable_primary_hw_plane(). If my mmio vs. CS flip race
> > series ever gets reviewed that also gets killed, but I do add back an
> > explicit vblank wait for gen2 only into i9xx_crtc_disable().
> 
> Right, there is only the explicit wait required for the panel scaler, as
> that more or less requires a sequence of writes to the double-buffered
> registers. Otherwise, the waits I remember are just warning that the
> register updates are double-buffered. So removing the waits for the pipe
> on seems a resonable thing.
> 
> In the event that we enable then disable the pipe all within a single
> frame (i.e. before the enable register updates are written) this should
> still work as the next register update will take the disable values for
> the current request. I.e. I don't think we need to insert explicit waits
> between enable/disabled.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Merged patches 2&3, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch