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