[PATCH xserver 2/3] present: Factor code for restoring screen pixmap out of present_unflip

[Michel Dänzer]

On 19.02.2016 14:31, Keith Packard wrote:
> Michel Dänzer <michel at daenzer.net> writes:
> 
> Here's another review, now that I've more carefully looked at the other
> two patches.
> 
>> +    if (!flip_window || !flip_pixmap)
>> +        return;
> 
> In the original code, this didn't abort the operation entirely...
> 
> This operation would have been skipped:
> 
>> +    present_set_tree_pixmap(flip_window, flip_pixmap, screen_pixmap);
> 
> But this operation would not:
> 
>> +    present_set_tree_pixmap(screen->root, NULL, screen_pixmap);

BTW, it might be safer to make this

    present_set_tree_pixmap(screen->root, flip_pixmap, screen_pixmap);

as well, so the root window pixmap is only changed to the screen pixmap
if it's currently the flip pixmap? Anyway, this can be done in a
followup, since the current call with NULL doesn't seem to cause any
problem in practice.


> Reading the rest of the code, I can't find a case which could call this
> with a null flip_window or flip_pixmap;

While I couldn't either just looking at present.c, I can get it called
with flip_window == NULL when pressing Escape while glxgears is flipping
in fullscreen, presumably due to present_clear_window_flip.

I'm not sure the present_copy_region or
present_set_tree_pixmap(screen->root, ...) calls will ever have any
actual effect in this case, but for now let's preserve the logic of the
refactored code wrt this.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer