[PATCH] drm/i915: Push vblank enable/disable past encoder->enable/disable

* [PATCH] drm/i915: Push vblank enable/disable past encoder->enable/disable
@ 2015-01-07 13:38 Daniel Vetter
  2015-01-07 14:19 ` Jani Nikula
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Daniel Vetter @ 2015-01-07 13:38 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter, Daniel Vetter

It is platform/output depenedent when exactly the pipe will start
running. Sometimes we just need the (cpu) pipe enabled, in other cases
the pch transcoder is enough and in yet other cases the (DP) port is
sending the frame start signal.

In a perfect world we'd put the drm_crtc_vblank_on call exactly where
the pipe starts running, but due to cloning and similar things this
will get messy. And the current approach of picking the most
conservative place for all combinations also doesn't work since that
results in legit vblank waits (in encoder->enable hooks, e.g. the 2
vblank waits for sdvo) failing.

Completely going back to the old world before

commit 51e31d49c89055299e34b8f44d13f70e19aaaad1
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Mon Sep 15 12:36:02 2014 +0200

    drm/i915: Use generic vblank wait# Please enter the commit message for your changes. Lines starting

isn't great either since screaming when the vblank wait work because
the pipe is off is kinda nice.

Pick a compromise and move the drm_crtc_vblank_on right before the
encoder->enable call. This is a lie on some outputs/platforms, but
after the ->enable callback the pipe is guaranteed to run everywhere.
So not that bad really. Suggested by Ville.

v2: Same treatment for drm_crtc_vblank_off and encoder->disable: I've
missed the ibx pipe B select w/a, which also has a vblank wait in the
disable function (while the pipe is obviously still running).

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 42 ++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a1dbe747a372..e224820ea5a4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4301,15 +4301,15 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	if (intel_crtc->config.has_pch_encoder)
 		ironlake_pch_enable(crtc);
 
+	assert_vblank_disabled(crtc);
+	drm_crtc_vblank_on(crtc);
+
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
 
 	if (HAS_PCH_CPT(dev))
 		cpt_verify_modeset(dev, intel_crtc->pipe);
 
-	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
-
 	intel_crtc_enable_planes(crtc);
 }
 
@@ -4421,14 +4421,14 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	if (intel_crtc->config.dp_encoder_is_mst)
 		intel_ddi_set_vc_payload_alloc(crtc, true);
 
+	assert_vblank_disabled(crtc);
+	drm_crtc_vblank_on(crtc);
+
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		encoder->enable(encoder);
 		intel_opregion_notify_encoder(encoder, true);
 	}
 
-	assert_vblank_disabled(crtc);
-	drm_crtc_vblank_on(crtc);
-
 	/* If we change the relative order between pipe/planes enabling, we need
 	 * to change the workaround. */
 	haswell_mode_set_planes_workaround(intel_crtc);
@@ -4479,12 +4479,12 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc_disable_planes(crtc);
 
-	drm_crtc_vblank_off(crtc);
-	assert_vblank_disabled(crtc);
-
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
 
+	drm_crtc_vblank_off(crtc);
+	assert_vblank_disabled(crtc);
+
 	if (intel_crtc->config.has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
 
@@ -4544,14 +4544,14 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc_disable_planes(crtc);
 
-	drm_crtc_vblank_off(crtc);
-	assert_vblank_disabled(crtc);
-
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		intel_opregion_notify_encoder(encoder, false);
 		encoder->disable(encoder);
 	}
 
+	drm_crtc_vblank_off(crtc);
+	assert_vblank_disabled(crtc);
+
 	if (intel_crtc->config.has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
 						      false);
@@ -5021,12 +5021,12 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(intel_crtc);
 
-	for_each_encoder_on_crtc(dev, crtc, encoder)
-		encoder->enable(encoder);
-
 	assert_vblank_disabled(crtc);
 	drm_crtc_vblank_on(crtc);
 
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		encoder->enable(encoder);
+
 	intel_crtc_enable_planes(crtc);
 
 	/* Underruns don't raise interrupts, so check manually. */
@@ -5082,12 +5082,12 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(intel_crtc);
 
-	for_each_encoder_on_crtc(dev, crtc, encoder)
-		encoder->enable(encoder);
-
 	assert_vblank_disabled(crtc);
 	drm_crtc_vblank_on(crtc);
 
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		encoder->enable(encoder);
+
 	intel_crtc_enable_planes(crtc);
 
 	/*
@@ -5159,12 +5159,12 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	 */
 	intel_wait_for_vblank(dev, pipe);
 
-	drm_crtc_vblank_off(crtc);
-	assert_vblank_disabled(crtc);
-
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
 
+	drm_crtc_vblank_off(crtc);
+	assert_vblank_disabled(crtc);
+
 	intel_disable_pipe(intel_crtc);
 
 	i9xx_pfit_disable(intel_crtc);
-- 
2.1.3

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread