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

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

On Thu, Feb 14, 2013 at 04:53:57PM +0100, Daniel Vetter wrote:
> On Thu, Feb 14, 2013 at 3:18 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > 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?
> 
> I think the current code is actually correct, since for all the output
> ports where disabling the port kills the frame start signal (and so
> the vblank counter afaik) we only disable the port in the post_disable
> hook. That's hsw ddi and cpu eDP.

Hmm. But then how do you explain the bug this is supposed to fix?

> But I think it's generally a nicer control flow when we block out &
> sync with vblank/pipe users before we start to turn things off. Note
> that concurrent calls to the wait vblank ioctl can still race since
> that doesn't grab the crtc lock and so could sneak in between the
> vblank_off call and us actually disabling the hw pipe. But since X is
> single-threaded I don't care one iota about this race for now ;-)

Sure we can do the change just for being nicer, but I'd like to
understand why it apparently fixes a real issue w/ flips not
completing...

Didn't we also have some bug about pipe disable timing out? That
also smells like the same problem.

> -Daniel
> 
> >
> >>
> >> 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
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC