[PATCH 0/6] drm/i915: Unfify some modeset sequences

[PATCH 0/6] drm/i915: Unfify some modeset sequences

[ville.syrjala]

We have some random variation in the ordering of the plane related stuff
during modeset. This series tries to unify that a little bit.

We could take this a bit further even and enable the planes as the very
last step of modeset. But for now I more of less kept the general sequence
unchanged.

The other bigger change here is that now we actually disable/enable sprites
around modeset.

[PATCH 1/6] drm/i915: Always load the display palette before enabling the pipe

[ville.syrjala]

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

Loading the palette after the planes are enabled can risk showing
incorrect colors. ILK+ already load the palette before even the pipe
is enabled. Just follow the same order for gen2-4 and VLV.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b0e29e2..40e014af 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3683,10 +3683,11 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	/* Enable panel fitting for eDP */
 	i9xx_pfit_enable(intel_crtc);
 
+	intel_crtc_load_lut(crtc);
+
 	intel_enable_pipe(dev_priv, pipe, false);
 	intel_enable_plane(dev_priv, plane, pipe);
 
-	intel_crtc_load_lut(crtc);
 	intel_update_fbc(dev);
 
 	/* Give the overlay scaler a chance to enable if it's on this pipe */
@@ -3722,12 +3723,13 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	/* Enable panel fitting for LVDS */
 	i9xx_pfit_enable(intel_crtc);
 
+	intel_crtc_load_lut(crtc);
+
 	intel_enable_pipe(dev_priv, pipe, false);
 	intel_enable_plane(dev_priv, plane, pipe);
 	if (IS_G4X(dev))
 		g4x_fixup_plane(dev_priv, pipe);
 
-	intel_crtc_load_lut(crtc);
 	intel_update_fbc(dev);
 
 	/* Give the overlay scaler a chance to enable if it's on this pipe */
-- 
1.8.1.5

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

[PATCH 2/6] drm/i915: Always enable the cursor right after the primary plane

[ville.syrjala]

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

Follow the same sequence when enabling the cursor plane during
modeset. No point in doing this stuff in different order on different
generations.

This should also avoid a needless wait for vblank for the g4x cursor
workaround when the cursor gets enabled anyway.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 40e014af..e2037d5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3319,6 +3319,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_enable_pipe(dev_priv, pipe,
 			  intel_crtc->config.has_pch_encoder);
 	intel_enable_plane(dev_priv, plane, pipe);
+	intel_crtc_update_cursor(crtc, true);
 
 	if (intel_crtc->config.has_pch_encoder)
 		ironlake_pch_enable(crtc);
@@ -3327,8 +3328,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_crtc_update_cursor(crtc, true);
-
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
 
@@ -3392,6 +3391,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_enable_pipe(dev_priv, pipe,
 			  intel_crtc->config.has_pch_encoder);
 	intel_enable_plane(dev_priv, plane, pipe);
+	intel_crtc_update_cursor(crtc, true);
 
 	if (intel_crtc->config.has_pch_encoder)
 		lpt_pch_enable(crtc);
@@ -3400,8 +3400,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_crtc_update_cursor(crtc, true);
-
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
 
@@ -3687,12 +3685,12 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	intel_enable_pipe(dev_priv, pipe, false);
 	intel_enable_plane(dev_priv, plane, pipe);
+	intel_crtc_update_cursor(crtc, true);
 
 	intel_update_fbc(dev);
 
 	/* Give the overlay scaler a chance to enable if it's on this pipe */
 	intel_crtc_dpms_overlay(intel_crtc, true);
-	intel_crtc_update_cursor(crtc, true);
 
 	mutex_unlock(&dev_priv->dpio_lock);
 }
@@ -3727,6 +3725,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	intel_enable_pipe(dev_priv, pipe, false);
 	intel_enable_plane(dev_priv, plane, pipe);
+	intel_crtc_update_cursor(crtc, true);
 	if (IS_G4X(dev))
 		g4x_fixup_plane(dev_priv, pipe);
 
@@ -3734,7 +3733,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	/* Give the overlay scaler a chance to enable if it's on this pipe */
 	intel_crtc_dpms_overlay(intel_crtc, true);
-	intel_crtc_update_cursor(crtc, true);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
-- 
1.8.1.5

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

[PATCH 3/6] drm/i915: Enable the overlay right after primary and cursor planes

[ville.syrjala]

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

Again follow the same sequence for all generations, because doing
otherwise just doesn't make sense.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e2037d5..ee656f8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3687,11 +3687,11 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_enable_plane(dev_priv, plane, pipe);
 	intel_crtc_update_cursor(crtc, true);
 
-	intel_update_fbc(dev);
-
 	/* 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);
+
 	mutex_unlock(&dev_priv->dpio_lock);
 }
 
@@ -3729,11 +3729,11 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	if (IS_G4X(dev))
 		g4x_fixup_plane(dev_priv, pipe);
 
-	intel_update_fbc(dev);
-
 	/* 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);
+
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
 }
-- 
1.8.1.5

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

[PATCH 4/6] drm/i915: Follow the same sequence when disabling planes

[ville.syrjala]

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

First disable FBC, then disable all planes, and finally disable the
pipe.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ee656f8..d87d929 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3433,13 +3433,13 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc_wait_for_pending_flips(crtc);
 	drm_vblank_off(dev, pipe);
-	intel_crtc_update_cursor(crtc, false);
-
-	intel_disable_plane(dev_priv, plane, pipe);
 
 	if (dev_priv->cfb_plane == plane)
 		intel_disable_fbc(dev);
 
+	intel_crtc_update_cursor(crtc, false);
+	intel_disable_plane(dev_priv, plane, pipe);
+
 	intel_set_pch_fifo_underrun_reporting(dev, pipe, false);
 	intel_disable_pipe(dev_priv, pipe);
 
@@ -3514,13 +3514,13 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc_wait_for_pending_flips(crtc);
 	drm_vblank_off(dev, pipe);
-	intel_crtc_update_cursor(crtc, false);
-
-	intel_disable_plane(dev_priv, plane, pipe);
 
 	if (dev_priv->cfb_plane == plane)
 		intel_disable_fbc(dev);
 
+	intel_crtc_update_cursor(crtc, false);
+	intel_disable_plane(dev_priv, plane, pipe);
+
 	if (intel_crtc->config.has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, false);
 	intel_disable_pipe(dev_priv, pipe);
@@ -3776,13 +3776,14 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	/* 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);
-	intel_crtc_dpms_overlay(intel_crtc, false);
-	intel_crtc_update_cursor(crtc, false);
 
 	if (dev_priv->cfb_plane == plane)
 		intel_disable_fbc(dev);
 
+	intel_crtc_dpms_overlay(intel_crtc, false);
+	intel_crtc_update_cursor(crtc, false);
 	intel_disable_plane(dev_priv, plane, pipe);
+
 	intel_disable_pipe(dev_priv, pipe);
 
 	i9xx_pfit_disable(intel_crtc);
-- 
1.8.1.5

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

[PATCH 5/6] drm/i915: Drop overlay DPMS call from valleyview_crtc_enable

[ville.syrjala]

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

VLV doesn't have the old video overlay.

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 d87d929..ac21889 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3687,9 +3687,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_enable_plane(dev_priv, plane, 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);
 
 	mutex_unlock(&dev_priv->dpio_lock);
-- 
1.8.1.5

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

[PATCH 6/6] drm/i915: Disable/restore all sprite planes around modeset

[ville.syrjala]

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

Disable/restore sprite planes around mode-set just like we do for the
primary and cursor planes. Now that we have working sprite clipping,
this actually works quite decently.

Previosuly we didn't even bother to disable sprites when changing mode,
which could lead to a corrupted sprite appearing on the screen after a
modeset (at least on my IVB). Not sure if all hardware generations would
be so forgiving when enabled sprites end up outside the pipe dimensons.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_sprite.c  |  8 ++++++++
 3 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ac21889..94d6604 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3264,6 +3264,28 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
 	}
 }
 
+static void intel_enable_planes(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	enum pipe pipe = to_intel_crtc(crtc)->pipe;
+	struct intel_plane *intel_plane;
+
+	list_for_each_entry(intel_plane, &dev->mode_config.plane_list, base.head)
+		if (intel_plane->pipe == pipe)
+			intel_plane_restore(&intel_plane->base);
+}
+
+static void intel_disable_planes(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	enum pipe pipe = to_intel_crtc(crtc)->pipe;
+	struct intel_plane *intel_plane;
+
+	list_for_each_entry(intel_plane, &dev->mode_config.plane_list, base.head)
+		if (intel_plane->pipe == pipe)
+			intel_plane_disable(&intel_plane->base);
+}
+
 static void ironlake_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -3319,6 +3341,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_enable_pipe(dev_priv, pipe,
 			  intel_crtc->config.has_pch_encoder);
 	intel_enable_plane(dev_priv, plane, pipe);
+	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
 
 	if (intel_crtc->config.has_pch_encoder)
@@ -3391,6 +3414,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_enable_pipe(dev_priv, pipe,
 			  intel_crtc->config.has_pch_encoder);
 	intel_enable_plane(dev_priv, plane, pipe);
+	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
 
 	if (intel_crtc->config.has_pch_encoder)
@@ -3438,6 +3462,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 		intel_disable_fbc(dev);
 
 	intel_crtc_update_cursor(crtc, false);
+	intel_enable_planes(crtc);
 	intel_disable_plane(dev_priv, plane, pipe);
 
 	intel_set_pch_fifo_underrun_reporting(dev, pipe, false);
@@ -3519,6 +3544,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 		intel_disable_fbc(dev);
 
 	intel_crtc_update_cursor(crtc, false);
+	intel_disable_planes(crtc);
 	intel_disable_plane(dev_priv, plane, pipe);
 
 	if (intel_crtc->config.has_pch_encoder)
@@ -3685,6 +3711,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	intel_enable_pipe(dev_priv, pipe, false);
 	intel_enable_plane(dev_priv, plane, pipe);
+	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
 
 	intel_update_fbc(dev);
@@ -3722,6 +3749,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	intel_enable_pipe(dev_priv, pipe, false);
 	intel_enable_plane(dev_priv, plane, pipe);
+	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
 	if (IS_G4X(dev))
 		g4x_fixup_plane(dev_priv, pipe);
@@ -3779,6 +3807,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc_dpms_overlay(intel_crtc, false);
 	intel_crtc_update_cursor(crtc, false);
+	intel_disable_planes(crtc);
 	intel_disable_plane(dev_priv, plane, pipe);
 
 	intel_disable_pipe(dev_priv, pipe);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6e01393..cd1297e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -614,6 +614,7 @@ extern void intel_connector_dpms(struct drm_connector *, int mode);
 extern bool intel_connector_get_hw_state(struct intel_connector *connector);
 extern void intel_modeset_check_state(struct drm_device *dev);
 extern void intel_plane_restore(struct drm_plane *plane);
+extern void intel_plane_disable(struct drm_plane *plane);
 
 
 static inline struct intel_encoder *intel_attached_encoder(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 19b9cb9..91eb97d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -955,6 +955,14 @@ void intel_plane_restore(struct drm_plane *plane)
 			   intel_plane->src_w, intel_plane->src_h);
 }
 
+void intel_plane_disable(struct drm_plane *plane)
+{
+	if (!plane->crtc || !plane->fb)
+		return;
+
+	intel_disable_plane(plane);
+}
+
 static const struct drm_plane_funcs intel_plane_funcs = {
 	.update_plane = intel_update_plane,
 	.disable_plane = intel_disable_plane,
-- 
1.8.1.5

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

Re: [PATCH 1/6] drm/i915: Always load the display palette before enabling the pipe

[Jesse Barnes]

On Wed,  8 May 2013 12:50:23 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Loading the palette after the planes are enabled can risk showing
> incorrect colors. ILK+ already load the palette before even the pipe
> is enabled. Just follow the same order for gen2-4 and VLV.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b0e29e2..40e014af 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3683,10 +3683,11 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	/* Enable panel fitting for eDP */
>  	i9xx_pfit_enable(intel_crtc);
>  
> +	intel_crtc_load_lut(crtc);
> +
>  	intel_enable_pipe(dev_priv, pipe, false);
>  	intel_enable_plane(dev_priv, plane, pipe);
>  
> -	intel_crtc_load_lut(crtc);
>  	intel_update_fbc(dev);
>  
>  	/* Give the overlay scaler a chance to enable if it's on this pipe */
> @@ -3722,12 +3723,13 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	/* Enable panel fitting for LVDS */
>  	i9xx_pfit_enable(intel_crtc);
>  
> +	intel_crtc_load_lut(crtc);
> +
>  	intel_enable_pipe(dev_priv, pipe, false);
>  	intel_enable_plane(dev_priv, plane, pipe);
>  	if (IS_G4X(dev))
>  		g4x_fixup_plane(dev_priv, pipe);
>  
> -	intel_crtc_load_lut(crtc);
>  	intel_update_fbc(dev);
>  
>  	/* Give the overlay scaler a chance to enable if it's on this pipe */

So it's just the pll that we need to have running in this case, not the
pipe?  I guess we'd find out quickly enough, the failure mode is a hard
system hang...
-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915: Always enable the cursor right after the primary plane

[Egbert Eich]

On Wed, May 08, 2013 at 12:50:24PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Follow the same sequence when enabling the cursor plane during
> modeset. No point in doing this stuff in different order on different
> generations.
> 
> This should also avoid a needless wait for vblank for the g4x cursor
> workaround when the cursor gets enabled anyway.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> @@ -3727,6 +3725,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  
>  	intel_enable_pipe(dev_priv, pipe, false);
>  	intel_enable_plane(dev_priv, plane, pipe);
> +	intel_crtc_update_cursor(crtc, true);
>  	if (IS_G4X(dev))
>  		g4x_fixup_plane(dev_priv, pipe);
>  

As discussed on IRC this may interfere with 

commit 61bc95c1fbbb6a08b55bbe161fdf1ea5493fc595
Author: Egbert Eich <eich@suse.com>
Date:   Mon Mar 4 09:24:38 2013 -0500

    DRM/i915: On G45 enable cursor plane briefly after enabling the display plane.

described in https://bugs.freedesktop.org/show_bug.cgi?id=61457

I cannot test on the affected hardware as it's not available to
me at the moment. If Ville's G4x doesn't show the issue I will 
test this next time I have access to the HW in question.
Therefore:

Acked-by: Egbert Eich <eich@suse.com>

Cheers,
	Egbert.

Re: [PATCH 2/6] drm/i915: Always enable the cursor right after the primary plane

[Ville Syrjälä]

On Tue, May 21, 2013 at 05:17:27PM +0200, Egbert Eich wrote:
> On Wed, May 08, 2013 at 12:50:24PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Follow the same sequence when enabling the cursor plane during
> > modeset. No point in doing this stuff in different order on different
> > generations.
> > 
> > This should also avoid a needless wait for vblank for the g4x cursor
> > workaround when the cursor gets enabled anyway.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > @@ -3727,6 +3725,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	intel_enable_pipe(dev_priv, pipe, false);
> >  	intel_enable_plane(dev_priv, plane, pipe);
> > +	intel_crtc_update_cursor(crtc, true);
> >  	if (IS_G4X(dev))
> >  		g4x_fixup_plane(dev_priv, pipe);
> >  
> 
> As discussed on IRC this may interfere with 
> 
> commit 61bc95c1fbbb6a08b55bbe161fdf1ea5493fc595
> Author: Egbert Eich <eich@suse.com>
> Date:   Mon Mar 4 09:24:38 2013 -0500
> 
>     DRM/i915: On G45 enable cursor plane briefly after enabling the display plane.
> 
> described in https://bugs.freedesktop.org/show_bug.cgi?id=61457
> 
> I cannot test on the affected hardware as it's not available to
> me at the moment. If Ville's G4x doesn't show the issue I will 
> test this next time I have access to the HW in question.

I couldn't reproduce the original problem on my g4x :(

> Therefore:
> 
> Acked-by: Egbert Eich <eich@suse.com>
> 
> Cheers,
> 	Egbert.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/6] drm/i915: Always enable the cursor right after the primary plane

[Egbert Eich]

On Wed, May 22, 2013 at 02:15:28PM +0300, Ville Syrjälä wrote:
> 
> I couldn't reproduce the original problem on my g4x :(

Ville, just go ahead with your patch. I will test this as soon
as I have access to a system where I know I can reproduce this.
I'll get back to you then.

> 
> > Therefore:
> > 
> > Acked-by: Egbert Eich <eich@suse.com>
> > 

Cheers,
	Egbert.

[PATCH v2] drm/i915: Always load the display palette before enabling the pipe

[ville.syrjala]

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

Loading the palette after the planes are enabled can risk showing
incorrect colors. ILK+ already load the palette before even the pipe
is enabled. Just follow the same order for gen2-4 and VLV.

According to BSpec the requirements for palette access are
display core clock and display PLL running. In certain platforms
just the core clock may be enough. But we definitely should have both
running when this gets called during the modeset.

v2: Amend the commit message with some display PLL/core clock info

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ffbd66a..a3ceae6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3579,10 +3579,11 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	/* Enable panel fitting for eDP */
 	i9xx_pfit_enable(intel_crtc);
 
+	intel_crtc_load_lut(crtc);
+
 	intel_enable_pipe(dev_priv, pipe, false);
 	intel_enable_plane(dev_priv, plane, pipe);
 
-	intel_crtc_load_lut(crtc);
 	intel_update_fbc(dev);
 
 	/* Give the overlay scaler a chance to enable if it's on this pipe */
@@ -3618,12 +3619,13 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	/* Enable panel fitting for LVDS */
 	i9xx_pfit_enable(intel_crtc);
 
+	intel_crtc_load_lut(crtc);
+
 	intel_enable_pipe(dev_priv, pipe, false);
 	intel_enable_plane(dev_priv, plane, pipe);
 	if (IS_G4X(dev))
 		g4x_fixup_plane(dev_priv, pipe);
 
-	intel_crtc_load_lut(crtc);
 	intel_update_fbc(dev);
 
 	/* Give the overlay scaler a chance to enable if it's on this pipe */
-- 
1.8.1.5

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

Re: [PATCH v2] drm/i915: Always load the display palette before enabling the pipe

[Daniel Vetter]

On Fri, May 31, 2013 at 08:09:47PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Loading the palette after the planes are enabled can risk showing
> incorrect colors. ILK+ already load the palette before even the pipe
> is enabled. Just follow the same order for gen2-4 and VLV.
> 
> According to BSpec the requirements for palette access are
> display core clock and display PLL running. In certain platforms
> just the core clock may be enough. But we definitely should have both
> running when this gets called during the modeset.
> 
> v2: Amend the commit message with some display PLL/core clock info
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I'd prefer if we add asserts to check that such ordering constraints are
fulfilled. With all the modeset rework the ordering sequence is much more
stable, but due to all that refactoring it's now also way too easy to
accidentally reorder something to the wrong spot. Just now I'm suffering
through a bit of this while trying to get pch pll state moved into the
pipe config.

For this case I think adding an assert_(correct pll)_enabled should help.
Maybe in a second patch since it's quite tricky with all the different
platforms ...
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ffbd66a..a3ceae6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3579,10 +3579,11 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	/* Enable panel fitting for eDP */
>  	i9xx_pfit_enable(intel_crtc);
>  
> +	intel_crtc_load_lut(crtc);
> +
>  	intel_enable_pipe(dev_priv, pipe, false);
>  	intel_enable_plane(dev_priv, plane, pipe);
>  
> -	intel_crtc_load_lut(crtc);
>  	intel_update_fbc(dev);
>  
>  	/* Give the overlay scaler a chance to enable if it's on this pipe */
> @@ -3618,12 +3619,13 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	/* Enable panel fitting for LVDS */
>  	i9xx_pfit_enable(intel_crtc);
>  
> +	intel_crtc_load_lut(crtc);
> +
>  	intel_enable_pipe(dev_priv, pipe, false);
>  	intel_enable_plane(dev_priv, plane, pipe);
>  	if (IS_G4X(dev))
>  		g4x_fixup_plane(dev_priv, pipe);
>  
> -	intel_crtc_load_lut(crtc);
>  	intel_update_fbc(dev);
>  
>  	/* Give the overlay scaler a chance to enable if it's on this pipe */
> -- 
> 1.8.1.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/6] drm/i915: Always enable the cursor right after the primary plane

[Egbert Eich]

On Tue, May 21, 2013 at 05:17:27PM +0200, Egbert Eich wrote:
> On Wed, May 08, 2013 at 12:50:24PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Follow the same sequence when enabling the cursor plane during
> > modeset. No point in doing this stuff in different order on different
> > generations.
> > 
> > This should also avoid a needless wait for vblank for the g4x cursor
> > workaround when the cursor gets enabled anyway.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > @@ -3727,6 +3725,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	intel_enable_pipe(dev_priv, pipe, false);
> >  	intel_enable_plane(dev_priv, plane, pipe);
> > +	intel_crtc_update_cursor(crtc, true);
> >  	if (IS_G4X(dev))
> >  		g4x_fixup_plane(dev_priv, pipe);
> >  
> 
> As discussed on IRC this may interfere with 
> 
> commit 61bc95c1fbbb6a08b55bbe161fdf1ea5493fc595
> Author: Egbert Eich <eich@suse.com>
> Date:   Mon Mar 4 09:24:38 2013 -0500
> 
>     DRM/i915: On G45 enable cursor plane briefly after enabling the display plane.
> 
> described in https://bugs.freedesktop.org/show_bug.cgi?id=61457
> 

Today I had the chance to test this. First I tried
if I can still reproduce the blank with this patch
added when I disable my voodoo g4x_fixup_plane():
It turned out it still happens however very rarely
(like 1 out of 20 tries). When I reenabled my voodoo
the issue still occurred.
I had to switch two lines around, ie:

 	intel_enable_plane(dev_priv, plane, pipe);
 	if (IS_G4X(dev))
 		g4x_fixup_plane(dev_priv, pipe);
+	intel_crtc_update_cursor(crtc, true);

to avoid the blank screen issue - which is it didn't
happen in ~75 tries.

With this change:

Acked-by: Egbert Eich <eich@suse.com>


Cheers,
	Egbert.

Re: [PATCH 2/6] drm/i915: Always enable the cursor right after the primary plane

[Ville Syrjälä]

On Thu, Jun 06, 2013 at 06:30:52PM +0200, Egbert Eich wrote:
> On Tue, May 21, 2013 at 05:17:27PM +0200, Egbert Eich wrote:
> > On Wed, May 08, 2013 at 12:50:24PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Follow the same sequence when enabling the cursor plane during
> > > modeset. No point in doing this stuff in different order on different
> > > generations.
> > > 
> > > This should also avoid a needless wait for vblank for the g4x cursor
> > > workaround when the cursor gets enabled anyway.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > > 
> > > @@ -3727,6 +3725,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> > >  
> > >  	intel_enable_pipe(dev_priv, pipe, false);
> > >  	intel_enable_plane(dev_priv, plane, pipe);
> > > +	intel_crtc_update_cursor(crtc, true);
> > >  	if (IS_G4X(dev))
> > >  		g4x_fixup_plane(dev_priv, pipe);
> > >  
> > 
> > As discussed on IRC this may interfere with 
> > 
> > commit 61bc95c1fbbb6a08b55bbe161fdf1ea5493fc595
> > Author: Egbert Eich <eich@suse.com>
> > Date:   Mon Mar 4 09:24:38 2013 -0500
> > 
> >     DRM/i915: On G45 enable cursor plane briefly after enabling the display plane.
> > 
> > described in https://bugs.freedesktop.org/show_bug.cgi?id=61457
> > 
> 
> Today I had the chance to test this. First I tried
> if I can still reproduce the blank with this patch
> added when I disable my voodoo g4x_fixup_plane():
> It turned out it still happens however very rarely
> (like 1 out of 20 tries). When I reenabled my voodoo
> the issue still occurred.
> I had to switch two lines around, ie:
> 
>  	intel_enable_plane(dev_priv, plane, pipe);
>  	if (IS_G4X(dev))
>  		g4x_fixup_plane(dev_priv, pipe);
> +	intel_crtc_update_cursor(crtc, true);
> 
> to avoid the blank screen issue - which is it didn't
> happen in ~75 tries.
> 
> With this change:
> 
> Acked-by: Egbert Eich <eich@suse.com>

Hmm. That means the cursor itself isn't perhaps really relevant, and
it's maybe more about the self refresh.

BTW did you ever try w/ just the self refresh disable but w/o the cursor trick?

We seem to enable self refresh already before we enable any planes (in
intel_update_watermarks()). So maybe something like this would work:

i9xx_crtc_enable()
 ...
 intel_update_watermarks()
 ...
 <SR off>
 intel_enable_pipe()
 intel_enable_plane()
 <enable other planes>
 [ maybe intel_wait_for_vblank(), but we seem to have too many of those already... ]
 <SR back on>

BTW the spec lists an SR workaround, which we don't follow apparently.
It's only relevant for switching between 1 and 2 pipes though. But maybe
there's a more general issue enabling even 1 pipe w/ SR enabled...

I suspect I need to dig more into the g4x watermark/SR stuff at some
point, so this is good practice for me :) I just wish I was able to
reproduce the bug. Maybe I need to find a ctg instead of the elk I
have...

-- 
Ville Syrjälä
Intel OTC