Second Feedback request for my GSoC project to improve Present support in Xwayland

[Roman Gilg]

On Wed, Aug 2, 2017 at 11:36 AM, Michel Dänzer <michel at daenzer.net> wrote:

> On 02/08/17 03:53 AM, Roman Gilg wrote:
> >
> > Overview of differences in Rootless mode:
> >
> > * Maintain a list of all windows, that are currently flipping pixmaps.
> > * Instead of saving the current flip_pixmap and so on in
> > present_screen_priv save it in present_window_priv.
> > * Don't restore the screen pixmap, instead save and refcount the pixmap
> > used by the window before Present took over and restore the window with
> > this pixmap.
> > * Introduce an additional DDX function hook flip_executed, that is
> > called directly before the flip is finished being executed in Present to
> > then fully commit the flipped window pixmap with the associated damage
> > and directly signal the flip as being complete.
>
> I don't understand what the flip_executed hook is good for.
>

On flip the Present extension has not yet calculated the damage for the
pixmap. This is not important on full screen flips in hardware, where we
used Present before only. But when we talk to the Wayland compositor, we
can tell it what parts of the surface are damaged.

Also there is the following issue, which I encountered: The Present
extension assumes, that on flip the DDX is not able to flip immediately,
but signals present_even_notify for the flip a little bit later (this
doesn't mean that's not async, just that the flip is pending for a little
while). But in the Xwayland  case (or maybe other DDX), we can and want
flip immediately. With flip_executed a DDX is able to send
present_event_notify back at the right time and knowing that nothing in
Present's current fip_executed is using the flip_pending values.


> > ad (3):
> >
> > In the Weston session I noticed extreme graphical artifacts in Neverball
> > (for a description of this app see below in Testing section). For sure
> > this was because of the reuse of pixmaps by the application after
> > Xwayland signaled that the pixmap is free to use again, but before the
> > compositor has released it. There were no artifacts in KWin, so I assume
> > KWin just quickly enough copies the pixel content and doesn't touch it
> > again afterwards. In the Weston case though the following happened:
> >
> > * Present gives pixmap A to Xwayland.
> > * A gets immediately committed and the flip is signaled as being
> > complete to Present (if we would wait for the frame callback, we would
> > have an implicit Vblank frame rate limit).
>
> Ideally, this should be triggered by and use the information from the
> Wayland Presentation-time extension events?


As far as I've understood the Presentation time extension, it informs the
client about the time the front buffer has flipped and probably the next
one will flip (which can happen for some compositors according to the
refresh rate, which might be on a 60 Hz display 60 times a second). But we
can't use this in our case, because it would limit the client's frame rate
to the one of the compositor, but we want to allow the client to render at
its own speed. For reference this is how Present works in general taking
xfree86 as an example:

xfree86 modesetting uses sync kernel modesetting. That means:

* Flip sent to DDX on Vsync time (flip is now pending)
* Flip sent to kernel by DDX DRM interface
* Time moves on while driver is flipping...
* Flip being completed by driver
* Kernel driver callback via DRM interface to DDX, that flip completed
* DDX  signals present_event_notify to Present
* Present sends complete and idle notices to client
* Client presents next pixmap, Present queues it until next VSync time
* If before VSync time an updated new pixmap is presented by the client,
Present throws away the queued pixmap and queues instead the new one
* On Vsync time step 1 happens again.

Because of the queuing the client is able to render at an arbitrary frame
rate. Would the DDX support the DRM async kernel modesetting it would look
like this:

* Flip sent to DDX at any time (flip is now pending)
* Flip sent to kernel by DDX DRM interface
* Time moves on while driver is flipping...
* Flip being completed by driver
* Kernel driver callback via DRM interface to DDX, that flip completed
* DDX  signals present_event_notify to Present
* Present sends complete and idle notices to client
* Client presents next pixmap
* Step 1 again

In other word if the complete notice would be sent on VSync time instead
after kernel callback we would limit the client rendering to the refresh
rate of the screen in async mode. In Xwayland the screen is the Wayland
compositor's backend, which renders and flips at a certain refresh rate
(for example in KWin it's currently the slowest refresh rate of all
connected displays because of some missing code pieces in the DRM
backend...). But we don't want to limit our clients to this refresh rate.


> > Another more serious problem would be, if an application only uses a
> > fixed number of pixmap and never uses one which hasn't yet received
> > the PresentIdleNotify event. In this case the app execution could halt.
>
> Not sure that's an issue. Every PresentCompleteNotify event should be
> followed by the corresponding PresentCompleteIdle event within finite time.
>

Yes, you're right in probably all relevant cases. It's just a principal
concern, because the Wayland specification mentions explicitly that a
compositor is free to send the buffer release event at any time. If a
compositor now would for whatever reason hoard these buffers and send the
release event back in batches after few seconds the client could run out of
pixmaps in these few seconds and stop refreshing its content. But yea, it's
probably not a real world problem.


> > ad (4):
> >
> > With the above I'm able to reliable flip pixmaps, when the flipping
> > window region equals the region of the toplevel window. This is
> > according to my experiments the case when not using any Composite
> > clipping - see VLC in Testing below for a good example. But tearing also
> > happens in composite windows, especially if a Wayland server wants to
> > outscan a Xwayland buffer to an overlay plane.
> >
> > For that I use a sub-surface for the flipping window. The flipping
> > window's pixmap buffer is flipped exclusively on the sub-surface, while
> > the rest of the window is committed to the main surface.
>
> This would require wrapping all Screen/GC rendering hooks to handle the
> sub-surfaces correctly.
>

Can you give an example why the wrapping would be necessary? I don't quite
see it. With the current code only the client's pixmap pixel data is
depicted on the Wayland sub-surface. Since the pixmap is rendered by the
client directly and independently to the rendering on the rest of the
Screen, everything else stays the same.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170803/2651475f/attachment.html>