Created attachment 66133 [details] glxgears.c demonstrating the described problem Calling glXMakeCurrent(dpy, win, ctx) where win is a Window (not a GLXWindow) and then calling glXQueryDrawable(dpy, glXGetCurrentDrawable(), GLX_WIDTH, &tmp) fails with GLXBadDrawable. It works with Nvidia libGL or with LIBGL_ALWAYS_INDIRECT. Attaching a glxgears.c with glXQueryDrawable added after glXMakeCurrent.
I confirm this. Moreover, with LIBGL_ALWAYS_INDIRECT set glXQueryDrawable(GLX_WIDTH or GLX_HEIGHT) doesn't fail, but returns zero size. Calling e.g. glXSwapBuffers() with the same drawable works fine. So there is at least some inconsistency in glx behavior.
Adam, you seem to have looked at this a couple of times. Will it be fixed with your pending GLX patches?
(In reply to comment #2) > Adam, you seem to have looked at this a couple of times. Will it be fixed > with your pending GLX patches? It's a Mesa bug, it's not sending MakeCurrent protocol for direct contexts, which means the X server never knows it needs to promote the Window into a glx window.
Would the same behaviour/bug be exposed by calling XGetGeometry on a drawable created with glXCreatePixmap ?
(In reply to comment #4) > Would the same behaviour/bug be exposed by calling XGetGeometry on a > drawable created with glXCreatePixmap ? No, but you'd have a different problem there, which is that XGetGeometry on a GLXPixmap will throw BadDrawable, since GetGeometry only operates on core protocol drawables (ie, on the Pixmap you pass into glXCreatePixmap, not on the GLXPixmap it returns).
Created attachment 107229 [details] [review] call SendMakeCurrent. With the 0001-SendMakeCurrent, the error goes away, but causes an Xorg segfault when glxgears exits. (gdb) bt #0 in ?? () #1 in glxClientCallback (list=<optimized out>, closure=<optimized out>, data=<optimized out>) at xserver/glx/glxext.c:291 #2 in _CallCallbacks (pcbl=pcbl@entry=0x5555559a5d60 <ClientStateCallback>, call_data=call_data@entry=0x7fffffffd9f0) at xserver/dix/dixutils.c:718 #3 in CallCallbacks (call_data=0x7fffffffd9f0, pcbl=0x5555559a5d60 <ClientStateCallback>) at xserver/include/callback.h:83 #4 CloseDownClient (client=client@entry=0x5555560d5f10) at xserver/dix/dispatch.c:3381 #5 in Dispatch () at xserver/dix/dispatch.c:444 #6 in dix_main (argc=4, argv=0x7fffffffdbe8, envp=<optimized out>) at xserver/dix/main.c:296 #7 in __libc_start_main (main=0x5555555994e0 <main>, argc=4, argv=0x7fffffffdbe8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffdbd8) at libc-start.c:287 #8 in _start () xserver/glx/glxext.c:291 looks like this 286 case ClientStateGone: 287 /* detach from all current contexts */ 288 for (c = glxAllContexts; c; c = next) { 289 next = c->next; 290 if (c->currentClient == pClient) { >291 c->loseCurrent(c); 292 lastGLContext = NULL; 293 c->currentClient = NULL; 294 __glXFreeContext(c); 295 } 296 } but c->loseCurrent is NULL, as most of the other fields of `c', except a few, like destroy and isDirect. For dri2, loseCurrent should be __glXDRIcontextLoseCurrent, set by __glXDRIscreenCreateContext (by looking at xserver/glx/glxdri2.c) But GLXscreen::createContext is called only when !isDirect by DoCreateContext in xserver/glx/glxcmds.c by __glXDisp_CreateContextAttribsARB in ./glx/createcontext.c
The server doesn't make the direct-rendering client current, so it gets angry when it tries to make it un-current. This is the reason we weren't sending it the MakeCurrent in the first place. There's probably some work that would need to be done in the server to make this work.
FYI the latest Oculus Rift SDK release hits this bug now. This means under Linux, the Rift can only be used with alternatives like the nVidia closed-source driver. https://developer.oculusvr.com/forums/viewtopic.php?f=34&t=16664
Makes tools hard.
Created attachment 109771 [details] [review] glx: Allow passing a Window to GLXGetDrawableAttributes Just adding another perspective here, since this was one of the notable issues I hit when getting Vogl running with Mesa earlier this year and I was also made aware of another tool today that is hitting this (not public / open source, sorry). Although the Nvidia driver apparently allows passing a vanilla XID to glXQueryDrawable; I hadn't previously considered it a bug that Mesa doesn't allow this. The GLX spec explicitly states that a Window is only considered a GLXDrawable for backwards compatibility with GLX 1.2, but GLX 1.2 didn't support glXQueryDrawable. It seems somewhat reasonable that GLX implementations shouldn't need to support Windows as GLXDrawables for apis introduced since GLX 1.2. That said though, this has clearly caused quite a few incompatibility issues due to Nvidia's driver allowing this so maybe it's worthwhile making an exception here. Looking into this a bit more, I'm not sure the problem only lies with how we handle MakeCurrent considering that even though this cited example uses MakeCurrent, it doesn't look like it's a requirement to have a current context when calling glXQueryDrawable. Digging into the X server code it looks like we can update the __glXGetDrawable() helper function (that's already used in various places to create a __GLXdrawable as necessary when a vanilla Window XID has been given) so that it doesn't require a current context. With that it seems we can then take advantage of the helper in DoGetDrawableAttributes() which should in turn allow applications to pass a vanilla Window to glXQueryDrawable. I've attached a patch that does this, though I'm afraid I haven't been able to test it yet. (I just figured I'd publish what I have in the interim) I was trying to test last night with the oculus sdk, but just hit a bunch of fiddly issues with my x server build (unrelated to the patch itself) so if anyone else wants to take a stab at testing this too that would be really appreciated.
ok I can confirm I have at least smoke tested my patch and it seems to work to query the width, height and fbconfig id using a Window xid now. Sadly testing with the oculus sdk things fall over literally on the next line of code after successfully calling glXQueryDrawable because the sdk currently assumes that the XID of an fbconfig can be cast directly as a GLXFBConfig, which is presumably the case with Nvidia's driver. This patch seemed to get me past that issue... --- a/LibOVR/Src/Displays/OVR_Linux_SDKWindow.cpp +++ b/LibOVR/Src/Displays/OVR_Linux_SDKWindow.cpp @@ -245,10 +245,25 @@ bool SDKWindow::getVisualFromDrawable(GLXDrawable drawable, XVisualInfo* vinfoOu { _XDisplay* display = glXGetCurrentDisplay(); - unsigned int value; - glXQueryDrawable(display, drawable, GLX_FBCONFIG_ID, &value); - XVisualInfo* chosen = glXGetVisualFromFBConfig(display, reinterpret_cast<GLXFBConfig>(value)); + unsigned int config_id; + + glXQueryDrawable(display, drawable, GLX_FBCONFIG_ID, &config_id); + + int attrib_list[3] = { + GLX_FBCONFIG_ID, config_id, + None, + }; + GLXFBConfig *configs; + int n_configs; + + configs = glXChooseFBConfig(display, DefaultScreen(display), attrib_list, &n_configs); + if (n_configs <= 0) + return false; + + XVisualInfo* chosen = glXGetVisualFromFBConfig(display, configs[0]); *vinfoOut = *chosen; + + XFree(configs); return true; }
I just realised I minced my words a bit in my earlier comment... Where I noted "it doesn't look like it's a requirement to have a current context when calling glXQueryDrawable." I realise it's not a question of the context being current at the time of the query. Rather the ambiguity in the spec seems to be that there's no explicit requirement that glXMakeCurrent was previously called as a way to bless a Window XID as being a GLXDrawable. Similarly where I said "it looks like we can update the __glXGetDrawable() helper function (that's already used in various places to create a __GLXdrawable as necessary when a vanilla Window XID has been given) so that it doesn't require a current context." again it's not really a question of there being a current context, but rather that __glXGetDrawable() required a context pointer to be able to fall back to creating a __GLXdrawable for a vanilla Window and since the GLXGetDrawableAttributes request doesn't include a context id the implementation needed changing to allow DoGetDrawableAttributes to be able to use __glXGetDrawable().
I've added a separate bug for the issue where the X server crashes if you make a direct context current because it seems like it's a separate issue which we don't need to fix for this bug. https://bugs.freedesktop.org/show_bug.cgi?id=86531
(In reply to Robert Bragg from comment #10) > Digging into the X server code it looks like we can update the > __glXGetDrawable() helper function (that's already used in various places to > create a __GLXdrawable as necessary when a vanilla Window XID has been > given) so that it doesn't require a current context. With that it seems we > can then take advantage of the helper in DoGetDrawableAttributes() which > should in turn allow applications to pass a vanilla Window to > glXQueryDrawable. This seems like it makes it legal to call SwapBuffers on arbitrary windows without a context, which would at best result in clobbering the window content with garbage. Does nvidia's driver let you get away with that? Slapping an fbconfig onto the window without a context seems like it could be a problem. Imagine app A does QueryDrawable on B's window before B gets a chance to call MakeCurrent, and that B's context is against a different fbconfig than we picked, now you'd throw BadMatch. Seems like it'd be better to leave the config slot in the drawable NULL, and amend DoMakeCurrent to fix that up and GetDrawableAttributes to either make up or omit the response for GLX_FBCONFIG_ID. I'll work on an updated patch.
Just to note for posterity: The NVIDIA driver appears to handle this by simply not returning a value for GLX_FBCONFIG_ID if the window is a naked window that hasn't yet been made current (and thus not had an fbconfig associated). That requires a little sleight of hand to implement in DRI since you have to kinda half-vivify the glx drawable private, but it's doable.
Posted: http://lists.x.org/archives/xorg-devel/2014-December/044701.html http://lists.x.org/archives/xorg-devel/2014-December/044702.html I gave up on half-vivifying the glx drawable private and just special-cased bare windows.
(In reply to ajax at nwnk dot net from comment #14) > (In reply to Robert Bragg from comment #10) > > > Digging into the X server code it looks like we can update the > > __glXGetDrawable() helper function (that's already used in various places to > > create a __GLXdrawable as necessary when a vanilla Window XID has been > > given) so that it doesn't require a current context. With that it seems we > > can then take advantage of the helper in DoGetDrawableAttributes() which > > should in turn allow applications to pass a vanilla Window to > > glXQueryDrawable. > > This seems like it makes it legal to call SwapBuffers on arbitrary windows > without a context, which would at best result in clobbering the window > content with garbage. Does nvidia's driver let you get away with that? > > Slapping an fbconfig onto the window without a context seems like it could > be a problem. Imagine app A does QueryDrawable on B's window before B gets > a chance to call MakeCurrent, and that B's context is against a different > fbconfig than we picked, now you'd throw BadMatch. Seems like it'd be > better to leave the config slot in the drawable NULL, and amend > DoMakeCurrent to fix that up and GetDrawableAttributes to either make up or > omit the response for GLX_FBCONFIG_ID. > > I'll work on an updated patch. Yeah, it's a pretty horrible scenario, and I hope nothing does that. Retrospectively maybe it would have been better for the glx spec to have drawn a cleaner line under using plain Windows/Pixmaps and explicitly disallow use with new api, and explicitly state that there is no way to associate such a drawable with an fbconfig, ah well. This approach you took is very similar to what I did to start with, in that I just special cased querying the width/height to behave the same as XGetGeometry. (I didn't even return GLX_Y_INVERTED_EXT to try and really minimize the possibility of applications depending on this working at all) The reason I didn't stick with that in the end was that it didn't fix the Oculus Rift SDK case that expects to query the fbconfig id. The current problem with this approach is that to fix the Oculus Rift SDK it relies on us sending a MakeCurrent requests in the dri case so we get the opportunity to associate an fbconfig at that point. But assuming we change that too, then this looks good to me.
(In reply to Robert Bragg from comment #17) > (In reply to ajax at nwnk dot net from comment #14) > > (In reply to Robert Bragg from comment #10) > > > > > Digging into the X server code it looks like we can update the > > > __glXGetDrawable() helper function (that's already used in various places to > > > create a __GLXdrawable as necessary when a vanilla Window XID has been > > > given) so that it doesn't require a current context. With that it seems we > > > can then take advantage of the helper in DoGetDrawableAttributes() which > > > should in turn allow applications to pass a vanilla Window to > > > glXQueryDrawable. > > > > This seems like it makes it legal to call SwapBuffers on arbitrary windows > > without a context, which would at best result in clobbering the window > > content with garbage. Does nvidia's driver let you get away with that? > > Just as an aside here; I believe it is indeed legal to call SwapBuffers on a window without a context, and the spec language explicitly considers this case, clarifying that it only implies a glFlush when currently associated with a context. I believe there is software in the wild that will even take advantage of this to defer SwapBuffers to a separate thread as a way to avoid blocking. A long time ago we looked at doing this in Clutter, before pursuing the GLX swap event extension instead to avoid blocking the applications mainloop, and I vaguely recall Qt was once looking at this too. When it comes to "arbitrary windows" though; yeah my patch really embraced the glx spec's wording that says that an X Window is a GLXDrawable, and it's arguably just an implementation detail how that's made to magically Just Work™. I think the side effect of my patch was that if you called SwapBuffers on a vanilla X drawable that had never been associated with a context (quite likely in the dri case I suppose as we don't send MakeCurrent requests a.t.m) then with my patch I'd be upgrading the drawable and associating it with an fbconfig where previously we'd have returned an error. I wouldn't guess that would be a problem, but maybe there's some case where the fbconfig derived from the Visual wouldn't later be compatible with the fbconfig of some context.
commit cadd70c809232c9a6601fb8baab665a7ab10045d Author: Adam Jackson <ajax@redhat.com> Date: Tue Dec 2 14:52:35 2014 -0500 glx: Add hack for GLX-1.2-style naked windows to GetDrawableAttributes Some people like to call this on bare Window XIDs and expect reasonable results. I sure wish they wouldn't, but since they do, if we're given a window without any glx decoration just fill in as much as we can. This means you won't actually get an answer for GLX_FBCONFIG_ID and friends, but there's not much to be done about that, and it matches what NVIDIA's driver seems to do. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54080 Reviewed-by: Eric Anholt <eric@anholt.net> Signed-off-by: Adam Jackson <ajax@redhat.com>
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.