Bug 3899 - minor src/glx/x11 cleanups
Summary: minor src/glx/x11 cleanups
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: GLX (show other bugs)
Version: unspecified
Hardware: x86 (IA32) Linux (All)
: high minor
Assignee: Ian Romanick
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-07-28 11:41 UTC by George -
Modified: 2006-07-21 16:19 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description George - 2005-07-28 11:41:14 UTC
In the spirit of cleaning libGL code, here are some minor suggestions:

*	src/mesa/drivers/dri/common/glcontextmodes.c

	__DRIinterfaceMethodsRec provides both createContextModes and 
	destroyContextModes. The dri drivers use createContextModes from 
	__DRIinterfaceMethodsRec and _gl_context_modes_destroy directly 
	from glcontextmodes.c (called within dri_util.c). Is this on purpose ? 
	Otherwise, the dri drivers could use destroyContextModes instead of 
	_gl_context_modes_destroy. In that case, it is possible to:

	o	move glcontextmodes.[hc] to src/glx/x11/
	o	eliminate IN_DRI_DRIVER from glcontextmodes.[hc]

	Moreover, 

	o	functions _gl_convert_to_x_visual_type and 
		_gl_copy_visual_to_context_mode appear to be dead code
	o	the #ifdef XFree86Server directive appears to be dead code 
		since the symlink-mesa.sh script from xorg does not symlink 
		glcontextmodes.c

*	src/glx/x11/glxclient.h

	remove structs marked as DEPRECATED

*	include/GL/internal/dri_interface.h

	DRI_NEW_INTERFACE_ONLY "survived" as "#if 0", better remove it

*	src/glx/x11/Makefile

	see patch below (tested with glxgears), under the assumptions that:

	o	glcontextmodes.[hc] was treated as above
	o	dispatch.c can be compiled similar to glapi.c, not symlinked
	o	the code in src/mesa/main/glheader.h within 
		the #ifndef __MINGW32__ directive is actually 
		moved somewhere in src/mesa/drivers/windows/

--------------------------------------------------------------------------------
--------------------------------------------------------------------------------

diff -Naur x11.orig/dri_glx.c x11/dri_glx.c
--- x11.orig/dri_glx.c	2005-07-28 04:00:50.419361728 +0300
+++ x11/dri_glx.c	2005-07-28 03:56:09.851014000 +0300
@@ -41,7 +41,7 @@
 #include "extutil.h"
 #include "glxclient.h"
 #include "xf86dri.h"
-#include "sarea.h"
+#include "GL/internal/sarea.h"
 #include <stdio.h>
 #include <dlfcn.h>
 #include "dri_glx.h"
diff -Naur x11.orig/glcontextmodes.c x11/glcontextmodes.c
--- x11.orig/glcontextmodes.c	2005-07-28 03:56:17.640830368 +0300
+++ x11/glcontextmodes.c	2005-07-28 03:44:37.297298000 +0300
@@ -35,7 +35,7 @@
 # include <stdlib.h>
 # include <string.h>
 # include <GL/gl.h>
-# include "dri_interface.h"
+# include "GL/internal/dri_interface.h"
 # include "imports.h"
 # define __glXMemset  memset
 #else
diff -Naur x11.orig/glxext.c x11/glxext.c
--- x11.orig/glxext.c	2005-07-28 02:32:47.655463912 +0300
+++ x11/glxext.c	2005-07-28 03:33:45.728352000 +0300
@@ -64,7 +64,7 @@
 #include <inttypes.h>
 #include <sys/mman.h>
 #include "xf86dri.h"
-#include "sarea.h"
+#include "GL/internal/sarea.h"
 #include "dri_glx.h"
 #endif
 
diff -Naur x11.orig/Makefile x11/Makefile
--- x11.orig/Makefile	2005-07-28 02:32:47.671461480 +0300
+++ x11/Makefile	2005-07-28 03:47:35.191254000 +0300
@@ -5,21 +5,21 @@
 # code will not build with DNIO defined.  When we finally drop old interface
 # support in libGL, we need to clean up both glxcmds.c and dri_interface.h.
 
-DEFINES += -DGLX_DIRECT_RENDERING -DGLXEXT -DXF86DRI -DGLX_USE_DLOPEN \
-	-DGLX_USE_MESA -DXF86VIDMODE -D_REENTRANT -UIN_DRI_DRIVER
+DEFINES += -DGLX_DIRECT_RENDERING -DXF86DRI -DXF86VIDMODE \
+	-D_REENTRANT -UIN_DRI_DRIVER
 
 C_SOURCES = \
 	  $(TOP)/src/mesa/glapi/glapi.c \
 	  $(TOP)/src/mesa/glapi/glthread.c \
-	  glcontextmodes.c \
+	  $(TOP)/src/mesa/main/dispatch.c \
 	  $(DRM_SOURCE_PATH)/libdrm/xf86drm.c \
 	  $(DRM_SOURCE_PATH)/libdrm/xf86drmHash.c \
 	  $(DRM_SOURCE_PATH)/libdrm/xf86drmRandom.c \
 	  $(DRM_SOURCE_PATH)/libdrm/xf86drmSL.c \
 	  clientattrib.c \
 	  compsize.c \
-	  dispatch.c \
 	  eval.c \
+	  glcontextmodes.c \
 	  glxcmds.c \
 	  glxext.c \
 	  glxextensions.c \
@@ -54,15 +54,8 @@
 
 INCLUDES = -I. \
 	-I$(TOP)/include \
-	-I$(TOP)/include/GL/internal \
-	-I$(TOP)/src/mesa \
 	-I$(TOP)/src/mesa/main \
 	-I$(TOP)/src/mesa/glapi \
-	-I$(TOP)/src/mesa/math \
-	-I$(TOP)/src/mesa/transform \
-	-I$(TOP)/src/mesa/swrast \
-	-I$(TOP)/src/mesa/swrast_setup \
-	-I$(TOP)/src/mesa/drivers/dri/common \
 	-I$(DRM_SOURCE_PATH)/libdrm \
 	-I$(DRM_SOURCE_PATH)/shared-core \
 	$(X11_INCLUDES)
@@ -80,12 +73,6 @@
 
 default: depend $(LIB_DIR)/$(GL_LIB_NAME)
 
-glcontextmodes.c:
-	ln -s $(TOP)/src/mesa/drivers/dri/common/glcontextmodes.c .
-
-dispatch.c:
-	ln -s $(TOP)/src/mesa/main/dispatch.c .
-
 # Make libGL
 $(LIB_DIR)/$(GL_LIB_NAME):  $(OBJECTS) Makefile
 	$(TOP)/bin/mklib -o $(GL_LIB) -linker '$(CC)' \
@@ -93,10 +80,6 @@
 		-install $(LIB_DIR) $(GL_LIB_DEPS) $(OBJECTS)
 
 
-drmtest: xf86drm.o drmtest.o
-	rm -f drmtest && $(CC) -o drmtest xf86drm.o drmtest.o
-
-
 depend: $(C_SOURCES) $(ASM_SOURCES) Makefile
 	touch depend
 	$(MKDEP) $(MKDEP_OPTIONS) $(INCLUDES) $(C_SOURCES) $(ASM_SOURCES)
Comment 1 Ian Romanick 2005-07-29 03:44:18 UTC
(In reply to comment #0)
> In the spirit of cleaning libGL code, here are some minor suggestions:
> 
> *	src/mesa/drivers/dri/common/glcontextmodes.c
> 
> 	__DRIinterfaceMethodsRec provides both createContextModes and 
> 	destroyContextModes. The dri drivers use createContextModes from 
> 	__DRIinterfaceMethodsRec and _gl_context_modes_destroy directly 
> 	from glcontextmodes.c (called within dri_util.c). Is this on purpose ? 
> 	Otherwise, the dri drivers could use destroyContextModes instead of 
> 	_gl_context_modes_destroy. In that case, it is possible to:
> 
> 	o	move glcontextmodes.[hc] to src/glx/x11/
> 	o	eliminate IN_DRI_DRIVER from glcontextmodes.[hc]

That should be doable.  Having glcontextmodes.c compiled into the DRI drivers is
an artifact of having to support versions of libGL that didn't supply it.  Since
we no longer need to support those versions, it can go.

> 	Moreover, 
> 
> 	o	functions _gl_convert_to_x_visual_type and 
> 		_gl_copy_visual_to_context_mode appear to be dead code
> 	o	the #ifdef XFree86Server directive appears to be dead code 
> 		since the symlink-mesa.sh script from xorg does not symlink 
> 		glcontextmodes.c

Where does it get glcontextmodes.c?  I set it up like that so that we wouldn't
have to keep two copies of the file in sync.

> *	src/glx/x11/glxclient.h
> 
> 	remove structs marked as DEPRECATED

Yes!  Those can finally go away!  They had to be left in place because changing,
moving, or removing them broke binary compatibility with older DRI drivers.  A
few of the changes in the "new" interface were designed specifically to fix
this.  We should be able to gut these at any point with no problems.

> *	include/GL/internal/dri_interface.h
> 
> 	DRI_NEW_INTERFACE_ONLY "survived" as "#if 0", better remove it

Oops.  I changed it to '#if 0' to make sure it really could be removed, but
never went back and removed it.

> *	src/glx/x11/Makefile
> 
> 	see patch below (tested with glxgears), under the assumptions that:
> 
> 	o	glcontextmodes.[hc] was treated as above
> 	o	dispatch.c can be compiled similar to glapi.c, not symlinked
> 	o	the code in src/mesa/main/glheader.h within 
> 		the #ifndef __MINGW32__ directive is actually 
> 		moved somewhere in src/mesa/drivers/windows/

I think that should be doable.  I'll have to look at it a bit more.

FYI, in-line patches are pure evil. :)
Comment 2 George - 2005-07-29 22:42:18 UTC
(In reply to comment #1)
> > 	Moreover, 
> > 
> > 	o	functions _gl_convert_to_x_visual_type and 
> > 		_gl_copy_visual_to_context_mode appear to be dead code
> > 	o	the #ifdef XFree86Server directive appears to be dead code 
> > 		since the symlink-mesa.sh script from xorg does not symlink 
> > 		glcontextmodes.c
> 
> Where does it get glcontextmodes.c?  I set it up like that so that we wouldn't
> have to keep two copies of the file in sync.
> 
there is a checked in copy at http://cvs.freedesktop.org/xorg/xserver/xorg/GL/glx/


> > *	src/glx/x11/Makefile
> > 
> > 	see patch below (tested with glxgears), under the assumptions that:
> > 
> > 	o	glcontextmodes.[hc] was treated as above
> > 	o	dispatch.c can be compiled similar to glapi.c, not symlinked
> > 	o	the code in src/mesa/main/glheader.h within 
> > 		the #ifndef __MINGW32__ directive is actually 
> > 		moved somewhere in src/mesa/drivers/windows/
> 
> I think that should be doable.  I'll have to look at it a bit more.
> 
that was before IN_DRI_DRIVER was used in dispatch.h
in the discussion for mesa-solo you said that the drivers don"t actually link
dispatch.c. Is that all drivers ?
then dispatch.c should be moved to mesa/glapi, 
this would fix the mess with building dispatch.c in three different ways with
specific order each time and make libGL code self-contained (only use mesa/glapi).

i will provide some more details and an attached patch if this the case.
Comment 3 George - 2005-08-10 05:34:14 UTC
Here is the suggestion in detail:

*	move glcontextmodes.[hc] to glx/x11
*	move dispatch.c to mesa/glapi
*	kill glx/mini/dispatch.c
*	apply the attached patch (mesa/sources, glx/*/Makefile)

glcontextmodes.c is no longer linked in DRI drivers

dispatch.c is compiled in the following ways:

*	non-dri   :	in-place from mesa/Makefile
*	linux-dri :	in-place from mesa/Makefile (unused)
			symlink  from glx/x11/Makefile
*	linux-solo:	in-place from glx/mini/Makefile
			in-place from mesa/Makefile (does nothing)
			in this case module compilation order is 
			important: glx/mini before mesa

This can be fixed by moving dispatch.c to mesa/glapi,
mesa/Makefile needs no changes: in the DRI case SOLO_SOURCES 
is used which does not contain GLAPI_SOURCES but DRI drivers 
don't mind and most importantly they *don't touch* dispatch.c; 
in the non-DRI case ALL_SOURCES is used which contains both 
MAIN_SOURCES and GLAPI_SOURCES, (except for beos which gets 
GLAPI_SOURCES in its own Makefile)
Comment 4 George - 2005-08-10 05:35:39 UTC
Created attachment 3311 [details] [review]
Patch to implement the changes described above.
Comment 5 Ian Romanick 2005-08-12 05:42:29 UTC
I think all of the suggested changes that do not require moving files have been
committed.  Moving glcontextmodes.[ch] and dispatch.c is a good idea, but I
don't want to create extra havoc importing updates of Mesa that move files until
after X.org 6.9 ships.  Once that ships, we'll never have to import Mesa into
the X.org tree again (yay!), so this won't be an issue.
Comment 6 George - 2006-01-04 06:30:50 UTC
(In reply to comment #5)
> I think all of the suggested changes that do not require moving files have been
> committed.  Moving glcontextmodes.[ch] and dispatch.c is a good idea, but I
> don't want to create extra havoc importing updates of Mesa that move files until
> after X.org 6.9 ships.  Once that ships, we'll never have to import Mesa into
> the X.org tree again (yay!), so this won't be an issue.

Isn't it time to close this ?

Also, what about the following aesthetic renames during the cvs surgery ?
glx_texture_compression.c	-> indirect_texture_compression.c
indirect_va_private.h		-> indirect_vertex_array_priv.h
Comment 7 Ian Romanick 2006-07-21 16:19:35 UTC
All of the cleanups that were going to be made have been made.


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.