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 at 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 at ubuntu.com> > >> Cc: Daniel Vetter <daniel.vetter at ffwll.ch> > >> Signed-off-by: Chris Wilson <chris at 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 at 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
