Re: [Mesa-dev] [PATCH] glx: block attempt to swapbuffer on pixmap. (v2)
On Wed, Dec 7, 2011 at 6:32 PM, Eric Anholt e...@anholt.net wrote: On Wed, 7 Dec 2011 10:24:09 +, Dave Airlie airl...@gmail.com wrote: From: Dave Airlie airl...@redhat.com This keeps track of the creation process and stores a drawable type, it then blocks DRI2 from getting called if the drawable is a pixmap. v2: check if we have a GLX drawable, which means we aren't a pbuffer, avoid doing flush at all since its meant to be a no-op. I still think this is the wrong way to go. As ajax pointed out, there's all sorts of races available from trying to guess client-side, and there's no way anybody's relying on the current (print an error message in the xorg log) behavior of the DRI2 protocol for single-buffered, so we might as well resolve that DRI2 protocol should do what we want for GLX. I've been thinking about the races, and I'm not sure they really apply, like we would have all those races now with the current code, Look at the patch, when the glx_drawable struct is created we record the type of the pixmap, we then call GetGLXDrawable, which looks the XID up in the hash, so if the XID has been reused or raced, then we would have to expect the hash to be incorrect, or else avoid using it in a lot of other places. Dave. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glx: block attempt to swapbuffer on pixmap. (v2)
On Mon, 2011-12-19 at 09:40 +, Dave Airlie wrote: On Wed, Dec 7, 2011 at 6:32 PM, Eric Anholt e...@anholt.net wrote: On Wed, 7 Dec 2011 10:24:09 +, Dave Airlie airl...@gmail.com wrote: From: Dave Airlie airl...@redhat.com This keeps track of the creation process and stores a drawable type, it then blocks DRI2 from getting called if the drawable is a pixmap. v2: check if we have a GLX drawable, which means we aren't a pbuffer, avoid doing flush at all since its meant to be a no-op. I still think this is the wrong way to go. As ajax pointed out, there's all sorts of races available from trying to guess client-side, and there's no way anybody's relying on the current (print an error message in the xorg log) behavior of the DRI2 protocol for single-buffered, so we might as well resolve that DRI2 protocol should do what we want for GLX. I've been thinking about the races, and I'm not sure they really apply, like we would have all those races now with the current code, Look at the patch, when the glx_drawable struct is created we record the type of the pixmap, we then call GetGLXDrawable, which looks the XID up in the hash, so if the XID has been reused or raced, then we would have to expect the hash to be incorrect, or else avoid using it in a lot of other places. True, but we could fix that by moving the UnlockDisplay() to after the DRI2 stanza (and fixing the called code to not try to take the lock itself). There'd still be a race between the GLX and DRI2 requests, in that someone else could Destroy that which you just created, but that's a race against an actively malicious client; it would have to be spamming the server with Destroy requests, since there's no way it can know what XIDs you're going to pick. In contrast, trying to track pixmapness like you're doing here races against at least two app programming techniques that GLX allows and apps actually use: multiple threads, and XID passing between applications. In the latter case you'll not have fixed anything: glx_draw will be NULL because we've not tracked this pixmap since its creation, so we'll carry on with the DRI request. The server is the only thing that knows how to handle this scenario, and it _can_ get this request, so you have to fix it there anyway. All you can do in the client is optimize out sending some requests you know will be no-ops. But thanks for prompting me to re-read that code, it's less thread-safe than I remembered. - ajax signature.asc Description: This is a digitally signed message part ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glx: block attempt to swapbuffer on pixmap. (v2)
On Mon, 2011-12-19 at 10:14 -0500, Adam Jackson wrote: On Mon, 2011-12-19 at 09:40 +, Dave Airlie wrote: On Wed, Dec 7, 2011 at 6:32 PM, Eric Anholt e...@anholt.net wrote: On Wed, 7 Dec 2011 10:24:09 +, Dave Airlie airl...@gmail.com wrote: From: Dave Airlie airl...@redhat.com This keeps track of the creation process and stores a drawable type, it then blocks DRI2 from getting called if the drawable is a pixmap. v2: check if we have a GLX drawable, which means we aren't a pbuffer, avoid doing flush at all since its meant to be a no-op. I still think this is the wrong way to go. As ajax pointed out, there's all sorts of races available from trying to guess client-side, and there's no way anybody's relying on the current (print an error message in the xorg log) behavior of the DRI2 protocol for single-buffered, so we might as well resolve that DRI2 protocol should do what we want for GLX. I've been thinking about the races, and I'm not sure they really apply, like we would have all those races now with the current code, Look at the patch, when the glx_drawable struct is created we record the type of the pixmap, we then call GetGLXDrawable, which looks the XID up in the hash, so if the XID has been reused or raced, then we would have to expect the hash to be incorrect, or else avoid using it in a lot of other places. True, but we could fix that by moving the UnlockDisplay() to after the DRI2 stanza (and fixing the called code to not try to take the lock itself). There'd still be a race between the GLX and DRI2 requests, in that someone else could Destroy that which you just created, but that's a race against an actively malicious client; it would have to be spamming the server with Destroy requests, since there's no way it can know what XIDs you're going to pick. In contrast, trying to track pixmapness like you're doing here races against at least two app programming techniques that GLX allows and apps actually use: multiple threads, and XID passing between applications. In the latter case you'll not have fixed anything: glx_draw will be NULL because we've not tracked this pixmap since its creation, so we'll carry on with the DRI request. The server is the only thing that knows how to handle this scenario, and it _can_ get this request, so you have to fix it there anyway. Actually no, the DRI2 code in the server can't know, as this is a perfectly legitimate request for a GLXPbuffer. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Debian, X and DRI developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glx: block attempt to swapbuffer on pixmap. (v2)
On Mon, 2011-12-19 at 16:45 +0100, Michel Dänzer wrote: On Mon, 2011-12-19 at 10:14 -0500, Adam Jackson wrote: The server is the only thing that knows how to handle this scenario, and it _can_ get this request, so you have to fix it there anyway. Actually no, the DRI2 code in the server can't know, as this is a perfectly legitimate request for a GLXPbuffer. Only as an artifact of how we implement pbuffers (which is, stylistically, a disaster). And even then I don't think that's true, just that the implementation would be unpleasant. xserver gets DRI2SwapBuffers on an XID for an X pixmap. It can either be backing a GLXPixmap or a GLXPbuffer, but not both*. Therefore DRI2 needs to wrap the glx drawable create path in the server, and stash a note on the PixmapRec private for the GLXPixmap and GLXPbuffer cases. That way SwapBuffers has type information without needing to walk all GLXPixmaps looking for an XID match with the backing pixmap. This works because Pixmaps have to be created before GLXPixmaps, but the backing pixmap for a pbuffer is created after the pbuffer. So technically that * above is a lie. We create the backing pixmap for the pbuffer inside xlib, and we don't leak that XID outside libGL, but the client could guess. The failure cases would be: a) create pbuffer, guess Pixmap XID, bind a GLXPixmap to it b) create Pixmap, create GLXPixmap, fabricate your own protocol for CreatePbuffer and DRI2CreateDrawable to bind to the Pixmap Both of which are detectable, because they involve calling DRI2CreateDrawable a second time after you've already inferred type. Or, better yet, if DRI2 insists on keeping the current weird 'alias' semantics for DRI2CreateDrawable, it could alias based on the GLX resource ID. This would mean moving the creation of the backing pixmap for pbuffers inside the server, but honestly that's a lot cleaner anyway. Among other things, there'd be no possibility of leaking an XID to the client for the backing resource. - ajax signature.asc Description: This is a digitally signed message part ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glx: block attempt to swapbuffer on pixmap. (v2)
From: Dave Airlie airl...@redhat.com
This keeps track of the creation process and stores a drawable type,
it then blocks DRI2 from getting called if the drawable is a pixmap.
v2: check if we have a GLX drawable, which means we aren't a pbuffer,
avoid doing flush at all since its meant to be a no-op.
Suggested by Michel Dänzer mic...@daenzer.net
Signed-off-by: Dave Airlie airl...@redhat.com
---
src/glx/glx_pbuffer.c |3 ++-
src/glx/glxclient.h |3 ++-
src/glx/glxcmds.c | 10 --
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/glx/glx_pbuffer.c b/src/glx/glx_pbuffer.c
index 6738252..4bc3292 100644
--- a/src/glx/glx_pbuffer.c
+++ b/src/glx/glx_pbuffer.c
@@ -434,7 +434,8 @@ CreateDrawable(Display *dpy, struct glx_config *config,
UnlockDisplay(dpy);
SyncHandle();
- if (InitGLXDrawable(dpy, glxDraw, drawable, xid)) {
+ if (InitGLXDrawable(dpy, glxDraw, drawable, xid,
+ (glxCode == X_GLXCreatePixmap) ? GLX_PIXMAP_BIT :
GLX_WINDOW_BIT)) {
free(glxDraw);
return None;
}
diff --git a/src/glx/glxclient.h b/src/glx/glxclient.h
index f915426..a47a2f7 100644
--- a/src/glx/glxclient.h
+++ b/src/glx/glxclient.h
@@ -585,6 +585,7 @@ struct glx_drawable {
XID xDrawable;
XID drawable;
+ uint32_t drawableType;
uint32_t lastEventSbc;
int64_t eventSbcWrap;
};
@@ -797,7 +798,7 @@ applegl_create_display(struct glx_display *display);
extern struct glx_drawable *GetGLXDrawable(Display *dpy, GLXDrawable drawable);
extern int InitGLXDrawable(Display *dpy, struct glx_drawable *glxDraw,
- XID xDrawable, GLXDrawable drawable);
+ XID xDrawable, GLXDrawable drawable, uint32_t
drawableType);
extern void DestroyGLXDrawable(Display *dpy, GLXDrawable drawable);
extern struct glx_context dummyContext;
diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
index c8ec9c2..634f0c5 100644
--- a/src/glx/glxcmds.c
+++ b/src/glx/glxcmds.c
@@ -107,7 +107,7 @@ GetGLXDrawable(Display *dpy, GLXDrawable drawable)
_X_HIDDEN int
InitGLXDrawable(Display *dpy, struct glx_drawable *glxDraw, XID xDrawable,
- GLXDrawable drawable)
+GLXDrawable drawable, uint32_t drawableType)
{
struct glx_display *priv = __glXInitialize(dpy);
@@ -118,6 +118,7 @@ InitGLXDrawable(Display *dpy, struct glx_drawable *glxDraw,
XID xDrawable,
glxDraw-drawable = drawable;
glxDraw-lastEventSbc = 0;
glxDraw-eventSbcWrap = 0;
+ glxDraw-drawableType = drawableType;
return __glxHashInsert(priv-glXDrawHash, drawable, glxDraw);
}
@@ -678,7 +679,7 @@ glXCreateGLXPixmap(Display * dpy, XVisualInfo * vis, Pixmap
pixmap)
UnlockDisplay(dpy);
SyncHandle();
- if (InitGLXDrawable(dpy, glxDraw, pixmap, req-glxpixmap)) {
+ if (InitGLXDrawable(dpy, glxDraw, pixmap, req-glxpixmap, GLX_PIXMAP_BIT)) {
free(glxDraw);
return None;
}
@@ -796,6 +797,11 @@ glXSwapBuffers(Display * dpy, GLXDrawable drawable)
#if defined(GLX_DIRECT_RENDERING) !defined(GLX_USE_APPLEGL)
{
__GLXDRIdrawable *pdraw = GetGLXDRIDrawable(dpy, drawable);
+ struct glx_drawable *glx_draw = GetGLXDrawable(dpy, drawable);
+
+ /* GLX specifies a swapbuffer on a Pixmap to be a no-op. */
+ if (glx_draw glx_draw-drawableType == GLX_PIXMAP_BIT)
+ return;
if (pdraw != NULL) {
if (gc drawable == gc-currentDrawable) {
--
1.7.6.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glx: block attempt to swapbuffer on pixmap. (v2)
On Mit, 2011-12-07 at 10:24 +, Dave Airlie wrote:
From: Dave Airlie airl...@redhat.com
This keeps track of the creation process and stores a drawable type,
it then blocks DRI2 from getting called if the drawable is a pixmap.
v2: check if we have a GLX drawable, which means we aren't a pbuffer,
avoid doing flush at all since its meant to be a no-op.
Suggested by Michel Dänzer mic...@daenzer.net
Signed-off-by: Dave Airlie airl...@redhat.com
[...]
diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
index c8ec9c2..634f0c5 100644
--- a/src/glx/glxcmds.c
+++ b/src/glx/glxcmds.c
@@ -796,6 +797,11 @@ glXSwapBuffers(Display * dpy, GLXDrawable drawable)
#if defined(GLX_DIRECT_RENDERING) !defined(GLX_USE_APPLEGL)
{
__GLXDRIdrawable *pdraw = GetGLXDRIDrawable(dpy, drawable);
+ struct glx_drawable *glx_draw = GetGLXDrawable(dpy, drawable);
+
+ /* GLX specifies a swapbuffer on a Pixmap to be a no-op. */
+ if (glx_draw glx_draw-drawableType == GLX_PIXMAP_BIT)
+ return;
if (pdraw != NULL) {
if (gc drawable == gc-currentDrawable) {
I think __glXSwapBuffersMscOML needs similar treatment:
glXSwapBuffersMscOML is a no-op and will always return 0 if the
specified drawable was created with a non-double-buffered
GLXFBConfig or if the specified drawable is a GLXPixmap.
Looks good otherwise, thanks!
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Debian, X and DRI developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glx: block attempt to swapbuffer on pixmap. (v2)
On Wed, 7 Dec 2011 10:24:09 +, Dave Airlie airl...@gmail.com wrote: From: Dave Airlie airl...@redhat.com This keeps track of the creation process and stores a drawable type, it then blocks DRI2 from getting called if the drawable is a pixmap. v2: check if we have a GLX drawable, which means we aren't a pbuffer, avoid doing flush at all since its meant to be a no-op. I still think this is the wrong way to go. As ajax pointed out, there's all sorts of races available from trying to guess client-side, and there's no way anybody's relying on the current (print an error message in the xorg log) behavior of the DRI2 protocol for single-buffered, so we might as well resolve that DRI2 protocol should do what we want for GLX. pgpN0DeVKIF3G.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
