Re: [PATCH] drm/i915: Wait for pending flips to complete before tearing down the encoders

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Feb 13, 2013 at 06:16:48PM +0000, Chris Wilson wrote:
> If we start disabling the encoders, there is a potential for a pending
> flip to never occur - and so we will end up waiting indefinitely.

To me that would indicate that the encoder disable hooks either kill
the vblank signal or the entire pixel clock, so that the flip never
completes.

But if that is the case, then shouldn't we also disable all planes and
indeed the whole pipe before calling the encoder disabled hooks?

> 
> v2: Also pre-emptively perform the drm_vblank_off() before switching off
> the encoders.
> 
> References: https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-video-intel/+bug/1097315
> Cc: Timo Aaltonen <tjaalton@ubuntu.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  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 da1ad9c..15cc838 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3486,15 +3486,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	int plane = intel_crtc->plane;
>  	u32 reg, temp;
>  
> -
>  	if (!intel_crtc->active)
>  		return;
>  
> +	intel_crtc_wait_for_pending_flips(crtc);
> +	drm_vblank_off(dev, pipe);
> +
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->disable(encoder);
>  
> -	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);
> @@ -3570,13 +3570,14 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	if (!intel_crtc->active)
>  		return;
>  
> +	intel_crtc_wait_for_pending_flips(crtc);
> +	drm_vblank_off(dev, pipe);
> +
>  	is_pch_port = haswell_crtc_driving_pch(crtc);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->disable(encoder);
>  
> -	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);
> @@ -3687,16 +3688,16 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	int plane = intel_crtc->plane;
>  
> -
>  	if (!intel_crtc->active)
>  		return;
>  
> +	intel_crtc_wait_for_pending_flips(crtc);
> +	drm_vblank_off(dev, pipe);
> +
>  	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);
>  	intel_crtc_dpms_overlay(intel_crtc, false);
>  	intel_crtc_update_cursor(crtc, false);
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC