[xserver,1/3] present: Only update screen pixmap from flip pixmap once per unflip

Submitted by Michel Dänzer on Feb. 19, 2016, 2:39 a.m.

Details

Message ID 1455849552-30757-1-git-send-email-michel@daenzer.net
State Accepted
Commit 72328e5eb98a3f27e1f0a0e17beae6db447bd87c
Headers show
Series "Series without cover letter" ( rev: 3 2 1 ) in X.org (DEPRECATED - USE GITLAB)

Not browsing as part of any series.

Commit Message

Michel Dänzer Feb. 19, 2016, 2:39 a.m. 
From: Michel Dänzer <michel.daenzer@amd.com>

present_unflip may be called several times from present_check_flip_window
during the same unflip. We can only copy to the screen pixmap the first
time, otherwise we may scribble over other windows. The flip pixmap
contents don't get updated after the first time anyway.

Fixes at least the following problems, which were introduced by commit
806470b9 ("present: Copy unflip contents back to the Screen Pixmap"):

On xfwm4 without compositing, run glxgears and put its window into
fullscreen mode to start flipping. While in fullscreen, open the xfwm4
window menu by pressing Alt-Space. The window menu was invisible most
of the time because it was getting scribbled over by a repeated unflip
copy.

When switching a flipping window out of fullscreen, a repeated unflip
copy could leave artifacts of the flip pixmap on the desktop.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 present/present.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox 

diff --git a/present/present.c b/present/present.c
index 8cf3b6f..d6ec895 100644
--- a/present/present.c
+++ b/present/present.c
@@ -421,6 +421,15 @@  present_unflip(ScreenPtr screen)
     assert (!screen_priv->unflip_event_id);
     assert (!screen_priv->flip_pending);
 
+    /* Update the screen pixmap with the current flip pixmap contents
+     * Only do this the first time for a particular unflip operation, or
+     * we'll probably scribble over other windows
+     */
+    if (screen->GetWindowPixmap(screen->root) == screen_priv->flip_pixmap) {
+        present_copy_region(&pixmap->drawable, screen_priv->flip_pixmap,
+                            NULL, 0, 0);
+    }
+
     if (screen_priv->flip_pixmap && screen_priv->flip_window)
         present_set_tree_pixmap(screen_priv->flip_window,
                                 screen_priv->flip_pixmap,
@@ -428,13 +437,6 @@  present_unflip(ScreenPtr screen)
 
     present_set_tree_pixmap(screen->root, NULL, pixmap);
 
-    /* Update the screen pixmap with the current flip pixmap contents
-     */
-    if (screen_priv->flip_pixmap && screen_priv->flip_window) {
-        present_copy_region(&pixmap->drawable,
-                            screen_priv->flip_pixmap,
-                            NULL, 0, 0);
-    }
     screen_priv->unflip_event_id = ++present_event_id;
     DebugPresent(("u %lld\n", screen_priv->unflip_event_id));
     (*screen_priv->info->unflip) (screen, screen_priv->unflip_event_id);

Comments

Keith Packard Feb. 19, 2016, 5:22 a.m. 
Michel Dänzer <michel@daenzer.net> writes:

> From: Michel Dänzer <michel.daenzer@amd.com>
>
> present_unflip may be called several times from present_check_flip_window
> during the same unflip. We can only copy to the screen pixmap the first
> time, otherwise we may scribble over other windows. The flip pixmap
> contents don't get updated after the first time anyway.
>
> Fixes at least the following problems, which were introduced by commit
> 806470b9 ("present: Copy unflip contents back to the Screen Pixmap"):
>
> On xfwm4 without compositing, run glxgears and put its window into
> fullscreen mode to start flipping. While in fullscreen, open the xfwm4
> window menu by pressing Alt-Space. The window menu was invisible most
> of the time because it was getting scribbled over by a repeated unflip
> copy.
>
> When switching a flipping window out of fullscreen, a repeated unflip
> copy could leave artifacts of the flip pixmap on the desktop.
>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>

Reviewed-by: Keith Packard <keithp@keithp.com>

> +    /* Update the screen pixmap with the current flip pixmap contents
> +     * Only do this the first time for a particular unflip operation, or
> +     * we'll probably scribble over other windows
> +     */
> +    if (screen->GetWindowPixmap(screen->root) == screen_priv->flip_pixmap) {
> +        present_copy_region(&pixmap->drawable, screen_priv->flip_pixmap,
> +                            NULL, 0, 0);
> +    }
> +

This removes the check for flip_pixmap && flip_window, but the
flip_pixmap check is not needed as you're checking it against the root
pixmap which is never NULL, and the check for flip_window was redundant
before as it was always set when flip_pixmap was set.
Chris Wilson Feb. 19, 2016, 12:05 p.m. 
On Fri, Feb 19, 2016 at 11:39:10AM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> present_unflip may be called several times from present_check_flip_window
> during the same unflip. We can only copy to the screen pixmap the first
> time, otherwise we may scribble over other windows. The flip pixmap
> contents don't get updated after the first time anyway.
> 
> Fixes at least the following problems, which were introduced by commit
> 806470b9 ("present: Copy unflip contents back to the Screen Pixmap"):
> 
> On xfwm4 without compositing, run glxgears and put its window into
> fullscreen mode to start flipping. While in fullscreen, open the xfwm4
> window menu by pressing Alt-Space. The window menu was invisible most
> of the time because it was getting scribbled over by a repeated unflip
> copy.
> 
> When switching a flipping window out of fullscreen, a repeated unflip
> copy could leave artifacts of the flip pixmap on the desktop.
> 
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>

And the ordering is better: we should do the restoration of the contents
before we restore the window->pixmap linkage.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris