[PATCH 1/2] drm/i915: Don't vblank wait on ilk-ivb after pipe enable

[PATCH 1/2] drm/i915: Don't vblank wait on ilk-ivb after pipe enable

[Daniel Vetter]

Like on hsw/bdw the pipe isn't actually running yet at this point.
This holds for both pch ports and the cpu edp port according to my
testing on ilk, snb and ivb.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77297
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1aae7361b7a5..e0310e3018ee 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1827,16 +1827,6 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
 
 	I915_WRITE(reg, val | PIPECONF_ENABLE);
 	POSTING_READ(reg);
-
-	/*
-	 * There's no guarantee the pipe will really start running now. It
-	 * depends on the Gen, the output type and the relative order between
-	 * pipe and plane enabling. Avoid waiting on HSW+ since it's not
-	 * necessary.
-	 * TODO: audit the previous gens.
-	 */
-	if (INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev))
-		intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
 /**
@@ -4461,7 +4451,9 @@ 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);
+
 	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
@@ -4546,7 +4538,9 @@ 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);
+
 	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	/* The fixup needs to happen before cursor is enabled */
-- 
1.8.4.rc3

[PATCH 2/2] drm/i915: Move plane enabling to the end of ilk_crtc_enable

[Daniel Vetter]

Like on hsw/bdw the pipe only starts running once the port/pch
transcoder combo is all enabled. Before that the vblank wait in the
primary plane enable function simply times out.

This is also really nice prep work for atomic modesets since now all
the plane enabling is at the very end and all tightly grouped
together, like on hsw+. Which means we can enable it all atomically
with a nuclear pageflip.

vlv is still different and the watermark code is also still somewhere
else, so it's not yet quite perfect. But we're getting there.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77297
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e0310e3018ee..e6555c0dedca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3714,9 +3714,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(intel_crtc);
-	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
-	intel_enable_planes(crtc);
-	intel_crtc_update_cursor(crtc, true);
 
 	if (intel_crtc->config.has_pch_encoder)
 		ironlake_pch_enable(crtc);
@@ -3742,6 +3739,10 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	 */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
+	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
+	intel_enable_planes(crtc);
+	intel_crtc_update_cursor(crtc, true);
+
 	drm_vblank_on(dev, pipe);
 }
 
-- 
1.8.4.rc3

Re: [PATCH 2/2] drm/i915: Move plane enabling to the end of ilk_crtc_enable

[Ville Syrjälä]

On Tue, Apr 15, 2014 at 06:41:23PM +0200, Daniel Vetter wrote:
> Like on hsw/bdw the pipe only starts running once the port/pch
> transcoder combo is all enabled. Before that the vblank wait in the
> primary plane enable function simply times out.
> 
> This is also really nice prep work for atomic modesets since now all
> the plane enabling is at the very end and all tightly grouped
> together, like on hsw+. Which means we can enable it all atomically
> with a nuclear pageflip.
> 
> vlv is still different and the watermark code is also still somewhere
> else, so it's not yet quite perfect. But we're getting there.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77297
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e0310e3018ee..e6555c0dedca 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3714,9 +3714,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  
>  	intel_update_watermarks(crtc);
>  	intel_enable_pipe(intel_crtc);
> -	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
> -	intel_enable_planes(crtc);
> -	intel_crtc_update_cursor(crtc, true);
>  
>  	if (intel_crtc->config.has_pch_encoder)
>  		ironlake_pch_enable(crtc);
> @@ -3742,6 +3739,10 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	 */
>  	intel_wait_for_vblank(dev, intel_crtc->pipe);
>  
> +	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
> +	intel_enable_planes(crtc);
> +	intel_crtc_update_cursor(crtc, true);

This more or less duplicates what's in my watermark series already. Except
you have a few more bugs here. The intel_wait_for_vblank() should be after
the plane enabling since it's a hack to avoid the flip done interrupts
getting mixed up. Also intel_update_fbc() must happen after enabling the
planes. I also made the enable and disable symmetric whereas you didn't.

Dunno maybe you just want to grab the patch from my series?

> +
>  	drm_vblank_on(dev, pipe);
>  }
>  
> -- 
> 1.8.4.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/2] drm/i915: Don't vblank wait on ilk-ivb after pipe enable

[Ville Syrjälä]

On Tue, Apr 15, 2014 at 06:41:22PM +0200, Daniel Vetter wrote:
> Like on hsw/bdw the pipe isn't actually running yet at this point.
> This holds for both pch ports and the cpu edp port according to my
> testing on ilk, snb and ivb.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77297
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Yeah I hate these weird waits we have sprinkled all over the place w/o
clear justification.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1aae7361b7a5..e0310e3018ee 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1827,16 +1827,6 @@ static void intel_enable_pipe(struct intel_crtc *crtc)
>  
>  	I915_WRITE(reg, val | PIPECONF_ENABLE);
>  	POSTING_READ(reg);
> -
> -	/*
> -	 * There's no guarantee the pipe will really start running now. It
> -	 * depends on the Gen, the output type and the relative order between
> -	 * pipe and plane enabling. Avoid waiting on HSW+ since it's not
> -	 * necessary.
> -	 * TODO: audit the previous gens.
> -	 */
> -	if (INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev))
> -		intel_wait_for_vblank(dev_priv->dev, pipe);
>  }
>  
>  /**
> @@ -4461,7 +4451,9 @@ 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);
> +
>  	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	intel_crtc_update_cursor(crtc, true);
> @@ -4546,7 +4538,9 @@ 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);
> +
>  	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	/* The fixup needs to happen before cursor is enabled */
> -- 
> 1.8.4.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: Move plane enabling to the end of ilk_crtc_enable

[Daniel Vetter]

On Tue, Apr 15, 2014 at 10:05 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> This more or less duplicates what's in my watermark series already. Except
> you have a few more bugs here. The intel_wait_for_vblank() should be after
> the plane enabling since it's a hack to avoid the flip done interrupts
> getting mixed up. Also intel_update_fbc() must happen after enabling the
> planes. I also made the enable and disable symmetric whereas you didn't.
>
> Dunno maybe you just want to grab the patch from my series?

Which one would that be? I just frobbed things sufficiently until the
warning from enable_primary_hw_plane disappeared ;-) But yeah fbc
should be enabled afterwards for sure.

I'm really looking forward to the brave new world where we bring up
the pipe in a fashionable black and then do a nuclear flip.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx