Re: [PATCH xserver 4/7] glx: Use vnd layer for dispatch (v2)

2018-02-01 Thread Kyle Brenneman 

On 02/01/2018 02:31 PM, Adam Jackson wrote:

On Wed, 2018-01-10 at 13:57 -0700, Kyle Brenneman wrote:


xorgGlxThunkRequest should be calling addXIDMap and removeXIDMap, not
the internal handlers.

I'm not sure that can be right. If it did, and adding the map failed,
there's no way to get the backend to clean up; at least, not the way
GlxAddXIDMap is currently written. Perhaps that should be changed to
call FreeResource on the XID if AddResource fails? (Like the one place
in the whole server where that'd be correct...)
It works if the stub calls addXIDMap first, before calling into the 
vendor. Then, if addXIDMap fails, then the stub can return BadAlloc 
before the vendor has a chance to do anything that would require cleanup.


If the vendor returns an error code, then the stub would call 
removeXIDMap to clean up before returning.


That's also why the LEGAL_NEW_RESOURCE call has to be moved to the 
dispatch stub: The addXIDMap call happens before calling into the 
vendor, so by the time the vendor gets called, there's a resource 
defined for that XID and LEGAL_NEW_RESOURCE would fail.





The default branch there may need some additional
attention if it would handle any requests that create or destroy resources.

It would. The only other vendorpriv extension I know of that does that
is the underspecified and probably unimplementable
GLX_SGIX_video_source though, so it's not really a big deal.

- ajax


___
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

Re: [PATCH xserver 4/7] glx: Use vnd layer for dispatch (v2)

2018-02-01 Thread Adam Jackson 
On Wed, 2018-01-10 at 13:57 -0700, Kyle Brenneman wrote:

> xorgGlxThunkRequest should be calling addXIDMap and removeXIDMap, not 
> the internal handlers.

I'm not sure that can be right. If it did, and adding the map failed,
there's no way to get the backend to clean up; at least, not the way
GlxAddXIDMap is currently written. Perhaps that should be changed to
call FreeResource on the XID if AddResource fails? (Like the one place
in the whole server where that'd be correct...)

> The default branch there may need some additional 
> attention if it would handle any requests that create or destroy resources.

It would. The only other vendorpriv extension I know of that does that
is the underspecified and probably unimplementable
GLX_SGIX_video_source though, so it's not really a big deal.

- ajax
___
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

Re: [PATCH xserver 4/7] glx: Use vnd layer for dispatch (v2)

2018-01-10 Thread Kyle Brenneman 


On 01/10/2018 01:57 PM, Kyle Brenneman wrote:

On 01/10/2018 11:05 AM, Adam Jackson wrote:

The big change here is MakeCurrent and context tag tracking. We now
delegate context tags entirely to the vnd layer, and simply store a
pointer to the context state as the tag data. If a context is deleted
while it's current, we allocate a fake ID for the context and move the
context state there, so the tag data still points to a real context. As
a result we can stop trying so hard to detach the client from contexts
at disconnect time and just let resource destruction handle it.

Since vnd handles all the MakeCurrent protocol now, our request handlers
for it can just be return BadImplementation.

We also remove a bunch of LEGAL_NEW_RESOURCE, because now by the time
we're called vnd has already allocated its tracking resource on that
XID. Note that we only do this for core GLX requests, for vendor private
requests we still need to call LEGAL_NEW_RESOURCE and in addition need
to call up to addXIDMap and friends.

v2: Update to match v2 of the vnd import, and remove more redundant work
like request length checks.

Signed-off-by: Adam Jackson 
---
  configure.ac   |   2 +-
  glx/createcontext.c|   2 -
  glx/glxcmds.c  | 275 
--

  glx/glxcmdsswap.c  |  98 +---
  glx/glxext.c   | 329 
-

  glx/glxext.h   |   4 +
  glx/glxscreens.h   |   1 +
  glx/glxserver.h|   5 -
  glx/xfont.c|   2 -
  hw/kdrive/ephyr/ephyr.c|   2 +-
  hw/kdrive/ephyr/meson.build|   1 +
  hw/kdrive/src/kdrive.c |   3 +
  hw/vfb/InitOutput.c|   2 +
  hw/vfb/meson.build |   3 +-
  hw/xfree86/Makefile.am |   5 +
  hw/xfree86/common/xf86Init.c   |   2 +-
  hw/xfree86/dixmods/glxmodule.c |   1 +
  hw/xfree86/meson.build |   1 +
  hw/xquartz/darwin.c|   4 +-
  hw/xwayland/Makefile.am|   1 +
  hw/xwayland/meson.build|   1 +
  hw/xwayland/xwayland.c |   2 +
  include/glx_extinit.h  |   5 +-
  23 files changed, 359 insertions(+), 392 deletions(-)




xorgGlxThunkRequest should be calling addXIDMap and removeXIDMap, not 
the internal handlers. The default branch there may need some 
additional attention if it would handle any requests that create or 
destroy resources.


And, in answer to the question next to xorgGlxThunkRequest, those 
could indeed be generated. The generate_dispatch_stubs.py script in 
the libglvnd repo only generates the core GLX requests, but it should 
be able to handle of those GLXVendorPrivate requests as well.


-Kyle



You've still got the xorgVendorInitClosure struct left over in glxext.c, 
too. I don't think anything uses it now.

___
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

Re: [PATCH xserver 4/7] glx: Use vnd layer for dispatch (v2)

2018-01-10 Thread Kyle Brenneman 

On 01/10/2018 11:05 AM, Adam Jackson wrote:

The big change here is MakeCurrent and context tag tracking. We now
delegate context tags entirely to the vnd layer, and simply store a
pointer to the context state as the tag data. If a context is deleted
while it's current, we allocate a fake ID for the context and move the
context state there, so the tag data still points to a real context. As
a result we can stop trying so hard to detach the client from contexts
at disconnect time and just let resource destruction handle it.

Since vnd handles all the MakeCurrent protocol now, our request handlers
for it can just be return BadImplementation.

We also remove a bunch of LEGAL_NEW_RESOURCE, because now by the time
we're called vnd has already allocated its tracking resource on that
XID. Note that we only do this for core GLX requests, for vendor private
requests we still need to call LEGAL_NEW_RESOURCE and in addition need
to call up to addXIDMap and friends.

v2: Update to match v2 of the vnd import, and remove more redundant work
like request length checks.

Signed-off-by: Adam Jackson 
---
  configure.ac   |   2 +-
  glx/createcontext.c|   2 -
  glx/glxcmds.c  | 275 --
  glx/glxcmdsswap.c  |  98 +---
  glx/glxext.c   | 329 -
  glx/glxext.h   |   4 +
  glx/glxscreens.h   |   1 +
  glx/glxserver.h|   5 -
  glx/xfont.c|   2 -
  hw/kdrive/ephyr/ephyr.c|   2 +-
  hw/kdrive/ephyr/meson.build|   1 +
  hw/kdrive/src/kdrive.c |   3 +
  hw/vfb/InitOutput.c|   2 +
  hw/vfb/meson.build |   3 +-
  hw/xfree86/Makefile.am |   5 +
  hw/xfree86/common/xf86Init.c   |   2 +-
  hw/xfree86/dixmods/glxmodule.c |   1 +
  hw/xfree86/meson.build |   1 +
  hw/xquartz/darwin.c|   4 +-
  hw/xwayland/Makefile.am|   1 +
  hw/xwayland/meson.build|   1 +
  hw/xwayland/xwayland.c |   2 +
  include/glx_extinit.h  |   5 +-
  23 files changed, 359 insertions(+), 392 deletions(-)




xorgGlxThunkRequest should be calling addXIDMap and removeXIDMap, not 
the internal handlers. The default branch there may need some additional 
attention if it would handle any requests that create or destroy resources.


And, in answer to the question next to xorgGlxThunkRequest, those could 
indeed be generated. The generate_dispatch_stubs.py script in the 
libglvnd repo only generates the core GLX requests, but it should be 
able to handle of those GLXVendorPrivate requests as well.


-Kyle

___
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

[PATCH xserver 4/7] glx: Use vnd layer for dispatch (v2)

2018-01-10 Thread Adam Jackson 
The big change here is MakeCurrent and context tag tracking. We now
delegate context tags entirely to the vnd layer, and simply store a
pointer to the context state as the tag data. If a context is deleted
while it's current, we allocate a fake ID for the context and move the
context state there, so the tag data still points to a real context. As
a result we can stop trying so hard to detach the client from contexts
at disconnect time and just let resource destruction handle it.

Since vnd handles all the MakeCurrent protocol now, our request handlers
for it can just be return BadImplementation.

We also remove a bunch of LEGAL_NEW_RESOURCE, because now by the time
we're called vnd has already allocated its tracking resource on that
XID. Note that we only do this for core GLX requests, for vendor private
requests we still need to call LEGAL_NEW_RESOURCE and in addition need
to call up to addXIDMap and friends.

v2: Update to match v2 of the vnd import, and remove more redundant work
like request length checks.

Signed-off-by: Adam Jackson 
---
 configure.ac   |   2 +-
 glx/createcontext.c|   2 -
 glx/glxcmds.c  | 275 --
 glx/glxcmdsswap.c  |  98 +---
 glx/glxext.c   | 329 -
 glx/glxext.h   |   4 +
 glx/glxscreens.h   |   1 +
 glx/glxserver.h|   5 -
 glx/xfont.c|   2 -
 hw/kdrive/ephyr/ephyr.c|   2 +-
 hw/kdrive/ephyr/meson.build|   1 +
 hw/kdrive/src/kdrive.c |   3 +
 hw/vfb/InitOutput.c|   2 +
 hw/vfb/meson.build |   3 +-
 hw/xfree86/Makefile.am |   5 +
 hw/xfree86/common/xf86Init.c   |   2 +-
 hw/xfree86/dixmods/glxmodule.c |   1 +
 hw/xfree86/meson.build |   1 +
 hw/xquartz/darwin.c|   4 +-
 hw/xwayland/Makefile.am|   1 +
 hw/xwayland/meson.build|   1 +
 hw/xwayland/xwayland.c |   2 +
 include/glx_extinit.h  |   5 +-
 23 files changed, 359 insertions(+), 392 deletions(-)

diff --git a/configure.ac b/configure.ac
index 30b4b383d..29ccd986d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1297,7 +1297,7 @@ if test "x$GLX" = xyes; then
PKG_CHECK_MODULES([GL], $GLPROTO $LIBGL)
AC_SUBST(XLIB_CFLAGS)
AC_DEFINE(GLXEXT, 1, [Build GLX extension])
-   GLX_LIBS='$(top_builddir)/glx/libglx.la'
+   GLX_LIBS='$(top_builddir)/glx/libglx.la 
$(top_builddir)/glx/libglxvnd.la'
GLX_SYS_LIBS="$GLX_SYS_LIBS $GL_LIBS"
 else
 GLX=no
diff --git a/glx/createcontext.c b/glx/createcontext.c
index 76316db67..00c23fcdd 100644
--- a/glx/createcontext.c
+++ b/glx/createcontext.c
@@ -123,8 +123,6 @@ __glXDisp_CreateContextAttribsARB(__GLXclientState * cl, 
GLbyte * pc)
 if (req->length != expected_size)
 return BadLength;
 
-LEGAL_NEW_RESOURCE(req->context, client);
-
 /* The GLX_ARB_create_context spec says:
  *
  * "* If  is not a valid GLXFBConfig, GLXBadFBConfig is
diff --git a/glx/glxcmds.c b/glx/glxcmds.c
index f4820d8e4..66018f6de 100644
--- a/glx/glxcmds.c
+++ b/glx/glxcmds.c
@@ -47,6 +47,7 @@
 #include "indirect_table.h"
 #include "indirect_util.h"
 #include "protocol-versions.h"
+#include "glxvndabi.h"
 
 static char GLXServerVendorName[] = "SGI";
 
@@ -135,6 +136,10 @@ _X_HIDDEN int
 validGlxContext(ClientPtr client, XID id, int access_mode,
 __GLXcontext ** context, int *err)
 {
+/* no ghost contexts */
+if (id & SERVER_BIT)
+return FALSE;
+
 *err = dixLookupResourceByType((void **) context, id,
__glXContextRes, client, access_mode);
 if (*err != Success || (*context)->idExists == GL_FALSE) {
@@ -240,8 +245,6 @@ DoCreateContext(__GLXclientState * cl, GLXContextID gcId,
 __GLXcontext *glxc, *shareglxc;
 int err;
 
-LEGAL_NEW_RESOURCE(gcId, client);
-
 /*
  ** Find the display list space that we want to share.
  **
@@ -356,14 +359,11 @@ DoCreateContext(__GLXclientState * cl, GLXContextID gcId,
 int
 __glXDisp_CreateContext(__GLXclientState * cl, GLbyte * pc)
 {
-ClientPtr client = cl->client;
 xGLXCreateContextReq *req = (xGLXCreateContextReq *) pc;
 __GLXconfig *config;
 __GLXscreen *pGlxScreen;
 int err;
 
-REQUEST_SIZE_MATCH(xGLXCreateContextReq);
-
 if (!validGlxScreen(cl->client, req->screen, , ))
 return err;
 if (!validGlxVisual(cl->client, pGlxScreen, req->visual, , ))
@@ -376,14 +376,11 @@ __glXDisp_CreateContext(__GLXclientState * cl, GLbyte * 
pc)
 int
 __glXDisp_CreateNewContext(__GLXclientState * cl, GLbyte * pc)
 {
-ClientPtr client = cl->client;
 xGLXCreateNewContextReq *req = (xGLXCreateNewContextReq *) pc;
 __GLXconfig *config;
 __GLXscreen *pGlxScreen;
 int err;
 
-REQUEST_SIZE_MATCH(xGLXCreateNewContextReq);
-
 if