[PATCH 1/2] Revert "drm/radeon: remove drm_vblank_get|put from pflip handling"

[Dieter Nützel]

Am 24.06.2014 12:05, schrieb Michel Dänzer:
> On 24.06.2014 05:32, Dieter Nützel wrote:
>> Am 23.06.2014 21:46, schrieb Dieter Nützel:
>>> Am 23.06.2014 11:34, schrieb Michel Dänzer:
>>>> On 18.06.2014 18:14, Christian König wrote:
>>>>> Am 18.06.2014 07:53, schrieb Michel Dänzer:
>>>>>> 
>>>>>>   (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip 
>>>>>> completion
>>>>>> event has impossible msc [x-1] < target_msc [x]
>>>>>> 
>>>>>> messages in the X log file which have been popping up in bug 
>>>>>> reports
>>>>>> lately.
>>>>>> This also results in 0s being returned to the client for the MSC 
>>>>>> and
>>>>>> timestamp of the swap completion, which could cause all kinds of 
>>>>>> bad
>>>>>> behaviour.
>>>>> First of all thanks for looking into it. Are you getting this on
>>>>> 3.16 or
>>>>> 3.15?
>>>> 
>>>> I haven't actually run into this myself yet. I thought I'd seen it 
>>>> in
>>>> several bug reports, but right now I can only find
>>>> https://bugs.freedesktop.org/show_bug.cgi?id=80029#c17 , which seems 
>>>> to
>>>> include the page flipping changes from 3.16.
>>> 
>>> With 3.16-rc2 I get it now on my RV730 AGP as in the above bug 
>>> report.
>>> But only the lines in Xorg.0.log.
>>> NO signs of any damage/error in use.
>>> 
>>> Since 3.15 and 3.16 (rc2 only) my system is rock solid.
>>> 
>>> I've tried 3.15-rc7 + Christian's pflip rework (did some little
>>> handwork), too.
>>> It was solid but I saw the reported flip/black distortion in the 
>>> below
>>> part during Kwin 4.13 cube screen effect (rotation). Your fix for
>>> 3.16-rc1 fixed that.
> 
> That's good to hear.
> 
> 
>> I can reliable generate such lines in Xorg.0.log with KWin cube 
>> desktop
>> effect.
>> 
>> Rotate screens with mouse wheel or screen switcher => new entry in
>> Xorg.0.log. If it happens I notice ('see') flip delay.
> 
> I was only able to reproduce it a couple of times even with that, but 
> not
> at all yet with the patch below. Does it help for you as well?

You have my Tested-by: for it.
Can't reproduce it any longer with your patch below.
Even that it didn't apply ontop of 3.16-rc2, but
most of the time I know what I'm doing...;-)

Now some little Fußball watching!

Cheers,
Dieter

> diff --git a/drivers/gpu/drm/radeon/radeon_display.c
> b/drivers/gpu/drm/radeon/radeon_display.c
> index 8b575a4..8350f8c 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -336,14 +336,19 @@ void radeon_crtc_handle_flip(struct
> radeon_device *rdev, int crtc_id)
>         struct radeon_crtc *radeon_crtc = 
> rdev->mode_info.crtcs[crtc_id];
>         struct radeon_flip_work *work;
>         unsigned long flags;
> +       struct timeval vblank_time;
> +       u32 vblank_seq;
> 
>         /* this can happen at init */
>         if (radeon_crtc == NULL)
>                 return;
> 
> +       vblank_seq = drm_vblank_count_and_time(rdev->ddev, crtc_id,
> &vblank_time);
> +
>         spin_lock_irqsave(&rdev->ddev->event_lock, flags);
>         work = radeon_crtc->flip_work;
> -       if (work == NULL) {
> +       if (work == NULL ||
> +           (vblank_seq - work->event->event.sequence) > (1<<23)) {
>                 spin_unlock_irqrestore(&rdev->ddev->event_lock, flags);
>                 return;
>         }
> @@ -379,6 +384,7 @@ static void radeon_flip_work_func(struct
> work_struct *__work)
> 
>         struct drm_crtc *crtc = &radeon_crtc->base;
>         struct drm_framebuffer *fb = work->fb;
> +       struct timeval vblank_time;
> 
>         uint32_t tiling_flags, pitch_pixels;
>         uint64_t base;
> @@ -466,6 +472,10 @@ static void radeon_flip_work_func(struct
> work_struct *__work)
>                 goto pflip_cleanup;
>         }
> 
> +       work->event->event.sequence =
> +               drm_vblank_count_and_time(crtc->dev, 
> radeon_crtc->crtc_id,
> +                                         &vblank_time) + 1;
> +
>         /* We borrow the event spin lock for protecting flip_work */
>         spin_lock_irqsave(&crtc->dev->event_lock, flags);