On 2018-02-02 12:08 PM, Michel Dänzer wrote:
> On 2018-02-02 10:42 AM, Roman Gilg wrote:
> 
>> I also tried to not copy, but set the window pixmap to the flip
>> pixmap. The problem I encountered here, is that the window original
>> pixmap can be controlled by the client.
> 
> How?
> 
> 
>> And when unflipping and restoring the original pixmap, this already
>> might have been deleted by the client.
> 
> A pixmap is only destroyed when its refcnt member drops to 0, so that
> shouldn't be an issue.

Taking a step back, do we even need to keep around the original pixmap
and unflip to it at all? I had a chat on IRC about this with Keith a
while ago, see the attached log excerpt.

Keith's main concern is that the presenting client could be able to
read/write the window pixmap's contents behind the X server's back. I
understand this concern at least somewhat in the case of per-screen
flips, because the screen pixmap can be used for completely different
windows after flipping.

But this seems irrelevant for per-window flips. In this case, the window
pixmap isn't used for anything else after flipping, so having direct
access to the pixmap doesn't allow the client to do anything it couldn't
do anyway using the X11 protocol. (There might be exceptions to this if
the window wasn't created by the presenting client, and the Security
extension comes into play. But that would be a rather exotic scenario,
so I don't think we need to consider it here.)


>> There might be ways to circumvent this problem, I have a few ideas,
>> but I decided to first go for a simple implementation by just copying
>> the pixmap content in order to decrease the overall complexity of the
>> patch set.
> 
> I'm not sure it would significantly increase the complexity, but fair
> enough I guess.

Unfortunately, I've come to realize that this doesn't work correctly in
all cases. Consider this scenario:

A client does one or more presentations to a window, which can be
executed as a flip. At some point it stops doing presentations, and
instead uses other X11 requests to update the window contents.

With the implementation in this patch, the latter updates will only
affect the original window pixmap, not the flip pixmap, so they won't be
visible to the Wayland compositor and its output (until an unflip to the
original window pixmap is triggered for some reason).

To avoid this, the flip pixmap must be set as the window pixmap, as is
done for per-screen flips.

Assuming the original window pixmap doesn't need to be saved and
restored, this should result in even less complexity than in this patch.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
Aug 31 11:18:33 <MrCooper>	anholt, keithp: what are the reasons for Present unflipping instead of changing the screen pixmap?
Aug 31 12:03:16 <keithp>	MrCooper: the screen pixmap may be owned and writable by a client
Aug 31 12:03:30 <keithp>	or rather, the pixmap currently doing scanout
Aug 31 12:03:50 <keithp>	oh, why is 'unflip' different from 'flip to existing screen pixmap'
Aug 31 12:13:43 <MrCooper>	keithp: you mean a client could scribble over the screen pixmap when it's no longer flipping?
Aug 31 12:13:53 <MrCooper>	or something else?
Aug 31 12:19:11 <MrCooper>	keithp: FWIW, I meant not keeping the pre-flip screen pixmap around at all
Aug 31 12:38:27 <keithp>	MrCooper: ah, ok. Yeah, that means potentially not being able to recover when a client crashes
Aug 31 12:38:29 <keithp>	that would suck
Aug 31 12:38:48 <keithp>	but, I agree that having it hang around kinda sucks from memory usage
Aug 31 12:38:56 <MrCooper>	recover from what exactly?
Aug 31 12:39:11 <keithp>	when the client crashes, we need to find a pixmap to put up on the screen
Aug 31 12:39:24 <keithp>	and not having one already allocated means trusting that we'll be able to allocate one
Aug 31 12:39:57 <MrCooper>	just leave the one that was flipped to last?
Aug 31 12:40:12 <keithp>	that one is "owned" by a client
Aug 31 12:40:42 <keithp>	and we need to draw the desktop again somewhere
Aug 31 12:40:55 <keithp>	are you concerned that having a spare pixmap around wastes memory?
Aug 31 12:41:52 <MrCooper>	not really, I'm just wondering if restoring the original pixmap is really necessary, since not doing so would make romangg's life easier for Xwayland per-window flips
Aug 31 12:42:17 <keithp>	well, the original pixmap is what all of the other windows on the screen are being drawn to
Aug 31 12:42:32 <keithp>	but for per-window flips, the answer is probably different
Aug 31 12:42:40 <MrCooper>	so, what exactly is the problem with the pixmap being owned by a client? It should certainly be usable for drawing the desktop?
Aug 31 12:42:40 <keithp>	the only flips we have in regular X are full-screen ones
Aug 31 12:42:55 <keithp>	well, the client has DMA access to it and was presumably doing GL to it
Aug 31 12:43:08 <keithp>	and if the client crashed...
Aug 31 12:43:18 <keithp>	it could well be unusable in practice
Aug 31 12:45:25 <MrCooper>	If a flip pixmap can't be used for drawing, it would already break things like GetImage while flipping, wouldn't it?
Aug 31 12:46:17 <MrCooper>	or indeed the unflip operation itself, which involves copying the flip pixmap contents to the screen pixmap
Aug 31 12:46:19 <keithp>	no, I'm thinking that if a client crashes, the pixmap it is using for drawing might have various rendering operations pending, or the client might be going nuts elsewhere and drawing all over it randomly
Aug 31 12:46:47 <keithp>	the goal is to get back to a pixmap which only The X server can use directly
Aug 31 12:47:41 <MrCooper>	the client could also go nuts and draw to the root window with IncludeInferiors
Aug 31 12:47:49 <keithp>	you can kill the client
Aug 31 12:47:57 <keithp>	you can't kill the owner of the pixmap
Aug 31 12:48:56 <keithp>	now, if you want there to be a different answer for per-window flips, I can see how that might be a good idea
Aug 31 12:49:51 <MrCooper>	actually, I'd say your arguments apply equally in both cases
Aug 31 12:50:30 <MrCooper>	so it's not possible to "disassociate" the pixmap from the client when it dies?
Aug 31 12:50:47 <keithp>	well, it's a DRM buffer that has been turned into a pixmap
Aug 31 12:51:06 <keithp>	so even if the client disconnects from X, it's still got a kernel handle to the buffer
Aug 31 12:52:07 <keithp>	that was my concern -- an out-of-X connection to the screen pixmap seems like a bad plan
Aug 31 12:55:52 <MrCooper>	I see, I guess that's less of a concern with per-window flips, since the window will generally be owned by the same client anyway
Aug 31 12:59:13 <MrCooper>	thanks for your insight so far, I may be back with more questions later
Aug 31 13:02:17 <keithp>	yeah, let's figure out how to make it work
_______________________________________________
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

Reply via email to