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

[Michel Dänzer]

On 30/08/17 06:15 PM, Roman Gilg wrote:
> On Wed, Aug 30, 2017 at 4:42 AM, Michel Dänzer  > wrote:
> 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?

Patch 2 is fine, it's an example of how this should be done.

> Or do you mean the moving of code in patch 1 to present_scrmode.c?

Yes, that's what I mean.


> * Only one logical change per patch.
> 
> Can you give an example? What do you mean by a logical change.

E.g. in patch 4:

"There is a small change to the window mode in Present as well, [...]"

that's a separate logical change and should be in a separate patch.


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

That's important as well.


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

That's unfortunate. Is there a way to at least keep it working about as
well as it has been without too much effort, e.g. by synchronizing to
the compositor's refresh cycle? In practice, the vast majority of
clients will always present on the next MSC, I don't think we need to
worry about supporting later MSCs at this point.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [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  wrote:
> On Wed, Aug 30, 2017 at 4:42 AM, Michel Dänzer  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@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Roman Gilg]

On Wed, Aug 30, 2017 at 4:42 AM, Michel Dänzer  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.

> * 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@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Michel Dänzer]

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.
* Only one logical change per patch.


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


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Roman Gilg]

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 tested these patches with several applications and environments. All patches 
should work on their own without their successors.

Test systems:
* RX470 with AMDGPU
* Intel graphics

Test environments:
* KDE Neon Dev Unstable (KWin) with Padoka PPA
* Fedora 26 (Mutter)

Test applications:
* Neverball
* VLC
* Google Chrome (with forced hardware acceleration)
* Steam
* Tomb Raider, Middle-Earth: Shadow of Mordor

Problems without sub-surfaces:
Steam login window is not showing buffer content. Might be a wrong 
implementation of direct rendering in the application.

Problems with sub-surfaces:
* The above problem with the Steam login window
* Resizing windows shows artifacts in the not yet updated sub-surface areas at 
the expanded edges
* KWin on AMDGPU: Won't quit completely when a sub-surface was used for Pixmap 
flipping at some point on run time. The error might be KWin internal, since it 
works with Mutter.
* KWin on AMDGPU: Surface of Chrome sometimes hangs on resizing operation. 
Might be again error in KWin, since it works with Mutter
* Mutter: VLC sub-surface misaligned (probably Mutter needs fix, since I needed 
to do something similar for KWin)
* Mutter: Sub-surface not correctly updating in Chrome (for example on tab 
switch, or when switching a Youtube video to full screen)
* Mutter: Neverball framerate capped to 120 (probably Mutter doesn't release 
buffers faster)

You can pull the patches also from this branch of my GitHub repo: 
https://github.com/subdiff/xserver/tree/gsoc
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel