[PATCH] present: Fix Async swap logic

[Chris Wilson]

On Wed, Nov 04, 2015 at 10:48:40AM +0100, Axel Davy wrote:
> On 04/11/2015 10:40, Chris Wilson wrote:
> >On Tue, Nov 03, 2015 at 09:14:51AM +0100, Axel Davy wrote:
> >>+    if (pixmap != NULL &&
> >>+        !(options & PresentOptionCopy) &&
> >>+        screen_priv->info) {
> >>+        if (target_msc > crtc_msc &&
> >>+            present_check_flip (target_crtc, window, pixmap, TRUE, valid, x_off, y_off))
> >>+        {
> >>+          vblank->flip = TRUE;
> >>+          vblank->sync_flip = TRUE;
> >>+          target_msc--;
> >>+        } else if (target_msc == crtc_msc &&
> >>+            (options & PresentOptionAsync) &&
> >>+            (screen_priv->info->capabilities & PresentCapabilityAsync) &&
> >>+            present_check_flip (target_crtc, window, pixmap, FALSE, valid, x_off, y_off))
> >>+        {
> >>+          vblank->flip = TRUE;
> >>+        }
> >>      }
> >For reference, this is how I fixed the async flip + swap elision:
> >
> >t a/present/present.c b/present/present.c
> >index e9ccfb8..32522af 100644
> >--- a/present/present.c
> >+++ b/present/present.c
> >@@ -861,19 +861,19 @@ present_pixmap(WindowPtr window,
> >      vblank->notifies = notifies;
> >      vblank->num_notifies = num_notifies;
> >-    if (!(options & PresentOptionAsync))
> >-        vblank->sync_flip = TRUE;
> >-
> >-    if (!(options & PresentOptionCopy) &&
> >-        !((options & PresentOptionAsync) &&
> >-          (!screen_priv->info ||
> >-           !(screen_priv->info->capabilities & PresentCapabilityAsync))) &&
> >-        pixmap != NULL &&
> >-        present_check_flip (target_crtc, window, pixmap, vblank->sync_flip, valid, x_off, y_off))
> >-    {
> >-        vblank->flip = TRUE;
> >-        if (vblank->sync_flip)
> >+    if (!(options & PresentOptionCopy) && pixmap != NULL) {
> >+        if (options & PresentOptionAsync &&
> >+            (screen_priv->info &&
> >+             screen_priv->info->capabilities & PresentCapabilityAsync) &&
> >+            present_check_flip (target_crtc, window, pixmap, FALSE, valid, x_off, y_off)) {
> >+            vblank->flip = TRUE;
> >+        }
> >+        else if (present_check_flip (target_crtc, window, pixmap, TRUE, valid, x_off, y_off)) {
> >+            vblank->flip = TRUE;
> >+            vblank->sync_flip = TRUE;
> >              target_msc--;
> >+        } else if (options & PresentOptionAsync)
> >+            target_msc++;
> >      }
> >      if (wait_fence) {
> >
> >
> Hi,
> 
> Could you explain:
> . Why you increase target_msc when the Async option is requested ?

It's the fallthrough path where the client requested an async swap but
we cannot perform one and so must use a synchronous wait for the vblank
(in which case we actually wait for the vblank before target_msc, hence
the increment here to compenstae).

> . Why you check for Async flips first (isn't sync flips better when
> possible) ?

The choice is between using native async flips or emulating them through
sync flips. Native wins.
 
> To me the Async option isn't related particularly to Async flips.
> Async flips is just an optimization to handle it without a copy in
> the case you would need one.

It is described as being "perform the flip as soon as possible (if
target < current msc) without regard to synchronisation to vrefresh".
That says use tearing native async flips to me.

> http://cgit.freedesktop.org/xorg/proto/presentproto/tree/presentproto.txt#n212
> Given the spec, I believe when target_msc > crtc_msc, the behaviour
> should be the same with or without the flag, and thus you shouldn't
> increase target_msc.

Exactly, hence why we need to fudge it in the code to maintain the
requested msc (as the code fudges it again later on).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre