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

[Daniel Vetter]

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.

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 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