[Intel-gfx] [PATCH] drm/i915: Move vblank enable earlier and disable later

[Daniel Vetter]

On Thu, Aug 14, 2014 at 10:04:37PM +0300, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> We changed to an interrupt based vblank wait (as opposed to polling)
> in:
>  commit 44bd93a3d367913d883be6abba9a6e51a53c4e90
>  Author: Daniel Vetter <daniel.vetter at ffwll.ch>
>  Date:   Fri Jul 25 23:36:44 2014 +0200
> 
>     drm/i915: Use generic vblank wait
> 
> However we already had vblank waits on the wrong side of
> drm_vblank_{on,off}() calls due to various workarounds, so now we get
> a warning more or less every time we do a modeset, and we fail to
> wait for the vblank like we should.
> 
> Move the drm_vblank_{on,off}() calls back out from
> intel_crtc_{enable,disable}_planes() so that all of these vblank waits
> return to proper operation. Also move the cxsr wait a bit earlier so
> that we can keep the encoder disable after we've turned off vblanks.
> Moving stuff out from the plane enable/disable functions seems
> preferrable to moving the workaround stuff in since the workarounds are
> required only on specific platforms.
> 
> While at it switch over to the drm_crtc_ variants of the vblank on/off
> functions.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82525
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82490
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

I've dropped the offending patch temporarily and picked up yours here into
my atomic branch - currently there's too many things unmerged to get this
in. Thanks anyway for the patch.
> ---
>  drivers/gpu/drm/i915/intel_display.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3813526..970c2ab 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3845,10 +3845,6 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_crtc->pipe;
>  
> -	assert_vblank_disabled(crtc);
> -
> -	drm_vblank_on(dev, pipe);
> -
>  	intel_enable_primary_hw_plane(crtc->primary, crtc);
>  	intel_enable_planes(crtc);
>  	intel_crtc_update_cursor(crtc, true);
> @@ -3894,10 +3890,6 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>  	 * consider this a flip to a NULL plane.
>  	 */
>  	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
> -
> -	drm_vblank_off(dev, pipe);
> -
> -	assert_vblank_disabled(crtc);
>  }
>  
>  static void ironlake_crtc_enable(struct drm_crtc *crtc)
> @@ -3967,6 +3959,9 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	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);
>  }
>  
> @@ -4074,6 +4069,9 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  		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);
> @@ -4109,6 +4107,9 @@ 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);
>  
> @@ -4175,6 +4176,9 @@ 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);
> @@ -4638,6 +4642,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
>  
> +	assert_vblank_disabled(crtc);
> +	drm_crtc_vblank_on(crtc);
> +
>  	intel_crtc_enable_planes(crtc);
>  
>  	/* Underruns don't raise interrupts, so check manually. */
> @@ -4695,6 +4702,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
>  
> +	assert_vblank_disabled(crtc);
> +	drm_crtc_vblank_on(crtc);
> +
>  	intel_crtc_enable_planes(crtc);
>  
>  	/*
> @@ -4758,9 +4768,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	intel_set_memory_cxsr(dev_priv, false);
>  	intel_crtc_disable_planes(crtc);
>  
> -	for_each_encoder_on_crtc(dev, crtc, encoder)
> -		encoder->disable(encoder);
> -
>  	/*
>  	 * On gen2 planes are double buffered but the pipe isn't, so we must
>  	 * wait for planes to fully turn off before disabling the pipe.
> @@ -4769,6 +4776,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);
> +
>  	intel_disable_pipe(dev_priv, pipe);
>  
>  	i9xx_pfit_disable(intel_crtc);
> -- 
> 1.8.5.5
> 

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