[PATCH 0/5] Improve Present support in Xwayland with per window flips

[Alex Deucher]

On Wed, Aug 30, 2017 at 5:15 AM, Roman Gilg <subdiff at gmail.com> wrote:
> On Wed, Aug 30, 2017 at 4:42 AM, Michel Dänzer <michel at daenzer.net> wrote:
>>
>>
>> Hi Roman,
>>
>>
>> On 30/08/17 12:24 AM, Roman Gilg wrote:
>> >
>> > Originating from the bug report
>> >
>> > https://bugs.freedesktop.org/show_bug.cgi?id=99702
>> >
>> > and my own observations with Xwayland misbehaving when outscanning on
>> > overlay planes, this patch series aims at improving Present support in
>> > Xwayland.
>> >
>> > For that it introduces an internal flip mode API to Present, with that
>> > it's possible to try other Pixmap flips than just for a whole screen like
>> > before. For Xwayland we add a flip mode per window, but for example in the
>> > future we could also try to add a mode for flips per CRTC. Anyway the idea
>> > is to clearly separate different flip modes with their own code paths.
>> >
>> > In Xwayland we flip per window, and also with the last patch in the
>> > series use sub-surfaces for that in order to flip on child windows. In my
>> > tests this was still somewhat fragile.
>>
>> I made some high level comments based on commit logs. I haven't reviewed
>> the patches in detail yet, because it seems difficult unless at least
>> some of them are split up:
>>
>> * Moving code without any functional changes should be in its own patch,
>>   not intermixed with functional changes.
>
>
> What do you mean exactly? I put the code moving of present.c to
> present_execute.c and present_vblank.c in the separate patch 2. Or should it
> not be part of this patch set at all? Or do you mean the moving of code in
> patch 1 to present_scrmode.c?
>
>> * Only one logical change per patch.
>
>
> Can you give an example? What do you mean by a logical change. I partitioned
> my changes such that they form functional units and such that every patch is
> self-contained, i.e. such that the Xserver can work even if the later ones
> won't get pushed.

I haven't looked closely at the patches, but if you are moving code
around with no functional change (e.g., breaking some common code out
into a new shared function or moving code to avoid an extra function
declaration or moving code to a new file), those should be separate
from functional changes (e.g. changing the code to do A rather than
B).  Each one is a separate logical change.  Mixing them together
makes it harder to review each change.

Alex

>
>> > * Mutter: Neverball framerate capped to 120 (probably Mutter doesn't
>> > release buffers faster)
>>
>> Was that with PresentOptionAsync (e.g. via "V-Sync" disabled in the game
>> settings, or e.g. vblank_mode=0)? Without PresentOptionAsync, the
>> intention is for the presentation frame rate to match the scanout
>> refresh rate.
>
>
> PresentOptionAsync is always on in Xwayland. And I disabled V-Sync in game
> (set vblank_mode=0). Although that in game setting makes with my current
> patches no difference, since V-Sync regressed for now with this patch series
> and only works again when later some mechanism for queuing vblanks, for
> example by using the Presentation Time protocol, is added.
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel