| Message ID | 1360779408-21705-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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. 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 ;-) -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
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
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);
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. 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(-)