* [PATCH] drm/i915: Wait for pending flips to complete before tearing down the encoders
@ 2013-02-13 18:16 Chris Wilson
2013-02-14 14:18 ` Ville Syrjälä
0 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2013-02-13 18:16 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
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(-)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Wait for pending flips to complete before tearing down the encoders
2013-02-13 18:16 [PATCH] drm/i915: Wait for pending flips to complete before tearing down the encoders Chris Wilson
@ 2013-02-14 14:18 ` Ville Syrjälä
2013-02-14 15:53 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Ville Syrjälä @ 2013-02-14 14:18 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Wait for pending flips to complete before tearing down the encoders
2013-02-14 14:18 ` Ville Syrjälä
@ 2013-02-14 15:53 ` Daniel Vetter
2013-02-14 18:15 ` Ville Syrjälä
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2013-02-14 15:53 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Wait for pending flips to complete before tearing down the encoders
2013-02-14 15:53 ` Daniel Vetter
@ 2013-02-14 18:15 ` Ville Syrjälä
0 siblings, 0 replies; 5+ messages in thread
From: Ville Syrjälä @ 2013-02-14 18:15 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] drm/i915: Wait for pending flips to complete before tearing down the encoders
@ 2013-02-13 18:12 Chris Wilson
0 siblings, 0 replies; 5+ messages in thread
From: Chris Wilson @ 2013-02-13 18:12 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
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.
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 | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index da1ad9c..dbd8895 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3486,14 +3486,14 @@ 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);
+
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);
@@ -3570,12 +3570,13 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
if (!intel_crtc->active)
return;
+ intel_crtc_wait_for_pending_flips(crtc);
+
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);
@@ -3687,15 +3688,15 @@ 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);
+
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-02-14 18:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-13 18:16 [PATCH] drm/i915: Wait for pending flips to complete before tearing down the encoders Chris Wilson
2013-02-14 14:18 ` Ville Syrjälä
2013-02-14 15:53 ` Daniel Vetter
2013-02-14 18:15 ` Ville Syrjälä
-- strict thread matches above, loose matches on Subject: below --
2013-02-13 18:12 Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.