[PATCH 2/3] present: Query the Window's CRTC every time

[Chris Wilson]

On Thu, May 28, 2015 at 04:59:14PM +0900, Michel Dänzer wrote:
> On 27.05.2015 15:51, Chris Wilson wrote:
> > On Tue, May 26, 2015 at 02:30:32PM -0700, Keith Packard wrote:
> >> Michel Dänzer <michel at daenzer.net> writes:
> >>
> >>> The old code also called present_get_crtc() unless pixmap == NULL, so
> >>> the problem couldn't affect flips but only MSC waits.
> >>
> >> The original code was trying to let the client control which CRTC to
> >> track for each window. For PresentPixmap requests, there's an explicit
> >> CRTC argument, which allows the client to select the right one. For
> >> PresentNotifyMSC, there's no such argument.
> >>
> >> So, the code was trying to respect the client's wishes by using
> >> whichever CRTC was last associated with the window -- either implicitly
> >> through the last call to present_get_crtc or explicitly from the last
> >> crtc passed to PresentPixmap for the same window.
> >>
> >> Probably the right thing to do is to add an explicit CRTC parameter to
> >> PresentNotifyMSC, and then have both requests call present_get_crtc when
> >> an explicit CRTC is not provided.
> >>
> >> That doesn't solve the problem when an explicitly requested CRTC is
> >> disabled though, so this fix doesn't address the root cause, only one of
> >> the symptoms.
> >>
> >>> The problem I was hitting was that this code was running for an MSC wait
> >>> when the CRTC referenced by window_priv->crtc was already disabled for
> >>> DPMS off. This caused hangs at the GNOME lock screen. This patch seems
> >>> to fix that problem.
> >>
> >> Why isn't your queue_vblank function bailing when asked to queue a
> >> request for a CRTC which is disabled? If it simply fails,
> >> present_execute will get called immediately and the client would
> >> continue happily along.
> > 
> > Oh, but it does fail gracefully. The problem is not that but that it
> > sends a garbage msc to a valid CRTC.
> 
> The patch below is an alternative fix for the problem I'm seeing, while
> preserving the window CRTC for MSC waits when possible. Your
> description sounds like it might work for you as well, Chris. Can you
> try it?
> 
> 
> diff --git a/present/present.c b/present/present.c
> index a634601..dc58f25 100644
> --- a/present/present.c
> +++ b/present/present.c
> @@ -713,6 +713,7 @@ present_pixmap(WindowPtr window,
>      uint64_t                    ust;
>      uint64_t                    target_msc;
>      uint64_t                    crtc_msc;
> +    RRCrtcPtr                   new_crtc;
>      int                         ret;
>      present_vblank_ptr          vblank, tmp;
>      ScreenPtr                   screen = window->drawable.pScreen;
> @@ -734,7 +735,21 @@ present_pixmap(WindowPtr window,
>              target_crtc = present_get_crtc(window);
>      }
> 
> -    present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc);
> +    if (present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc) != Success) {
> +        /* Maybe we need to try a different CRTC?
> +         */
> +        new_crtc = present_get_crtc(window);
> +
> +        if (new_crtc != target_crtc &&
> +            present_get_ust_msc(screen, new_crtc, &ust, &crtc_msc) == Success)
> +            target_crtc = new_crtc;
> +        else {
> +            /* Fall back to fake MSC handling
> +             */
> +            target_crtc = NULL;
> +            present_fake_get_ust_msc(screen, &ust, &crtc_msc);
> +        }
> +    }

It survived for one more CRTC change, but still feeds passed msc into
the wait_vblank.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre