[Mesa-dev] [PATCH] dri: Choose a decent global driNConfigOptions.

Eric Anholt eric at anholt.net
Thu Aug 8 03:59:17 PDT 2013


Previously, we were asserting that each driver specified an NConfigOptions
exactly equal to the number of options they supplied, leading to frequent
bugs when people would forget to adjust the value when adjusting driver
options.  Instead, just overallocate the table by a bit and leave sanity
checking to the assert in findOption().
---
 src/gallium/state_trackers/dri/common/dri_screen.c |  5 +--
 src/mesa/drivers/dri/common/dri_util.c             |  4 +-
 src/mesa/drivers/dri/common/xmlconfig.c            | 44 ++++------------------
 src/mesa/drivers/dri/common/xmlconfig.h            |  4 +-
 src/mesa/drivers/dri/i915/intel_screen.c           |  5 +--
 src/mesa/drivers/dri/i965/intel_screen.c           |  5 +--
 src/mesa/drivers/dri/radeon/radeon_screen.c        |  5 +--
 7 files changed, 14 insertions(+), 58 deletions(-)

diff --git a/src/gallium/state_trackers/dri/common/dri_screen.c b/src/gallium/state_trackers/dri/common/dri_screen.c
index 3b42b5a..779741e 100644
--- a/src/gallium/state_trackers/dri/common/dri_screen.c
+++ b/src/gallium/state_trackers/dri/common/dri_screen.c
@@ -74,8 +74,6 @@ PUBLIC const char __driConfigOptions[] =
 
 #define false 0
 
-static const uint __driNConfigOptions = 13;
-
 static const __DRIconfig **
 dri_fill_in_modes(struct dri_screen *screen)
 {
@@ -417,8 +415,7 @@ dri_init_screen_helper(struct dri_screen *screen,
    else
       screen->target = PIPE_TEXTURE_RECT;
 
-   driParseOptionInfo(&screen->optionCacheDefaults,
-                      __driConfigOptions, __driNConfigOptions);
+   driParseOptionInfo(&screen->optionCacheDefaults, __driConfigOptions);
 
    driParseConfigFiles(&screen->optionCache,
 		       &screen->optionCacheDefaults,
diff --git a/src/mesa/drivers/dri/common/dri_util.c b/src/mesa/drivers/dri/common/dri_util.c
index 9ed9df4..fa520ea 100644
--- a/src/mesa/drivers/dri/common/dri_util.c
+++ b/src/mesa/drivers/dri/common/dri_util.c
@@ -52,8 +52,6 @@ PUBLIC const char __dri2ConfigOptions[] =
       DRI_CONF_SECTION_END
    DRI_CONF_END;
 
-static const uint __dri2NConfigOptions = 1;
-
 /*****************************************************************/
 /** \name Screen handling functions                              */
 /*****************************************************************/
@@ -112,7 +110,7 @@ dri2CreateNewScreen(int scrn, int fd,
 	return NULL;
     }
 
-    driParseOptionInfo(&psp->optionInfo, __dri2ConfigOptions, __dri2NConfigOptions);
+    driParseOptionInfo(&psp->optionInfo, __dri2ConfigOptions);
     driParseConfigFiles(&psp->optionCache, &psp->optionInfo, psp->myNum, "dri2");
 
     return psp;
diff --git a/src/mesa/drivers/dri/common/xmlconfig.c b/src/mesa/drivers/dri/common/xmlconfig.c
index 5c97c20..b95e452 100644
--- a/src/mesa/drivers/dri/common/xmlconfig.c
+++ b/src/mesa/drivers/dri/common/xmlconfig.c
@@ -132,16 +132,6 @@ static GLuint findOption (const driOptionCache *cache, const char *name) {
     return hash;
 }
 
-/** \brief Count the real number of options in an option cache */
-static GLuint countOptions (const driOptionCache *cache) {
-    GLuint size = 1 << cache->tableSize;
-    GLuint i, count = 0;
-    for (i = 0; i < size; ++i)
-	if (cache->info[i].name)
-	    count++;
-    return count;
-}
-
 /** \brief Like strdup but using malloc and with error checking. */
 #define XSTRDUP(dest,source) do { \
     GLuint len = strlen (source); \
@@ -685,25 +675,18 @@ static void optInfoEndElem (void *userData, const XML_Char *name) {
     }
 }
 
-void driParseOptionInfo (driOptionCache *info,
-			 const char *configOptions, GLuint nConfigOptions) {
+void driParseOptionInfo (driOptionCache *info, const char *configOptions) {
     XML_Parser p;
     int status;
     struct OptInfoData userData;
     struct OptInfoData *data = &userData;
-    GLuint realNoptions;
-
-  /* determine hash table size and allocate memory:
-   * 3/2 of the number of options, rounded up, so there remains always
-   * at least one free entry. This is needed for detecting undefined
-   * options in configuration files without getting a hash table overflow.
-   * Round this up to a power of two. */
-    GLuint minSize = (nConfigOptions*3 + 1) / 2;
-    GLuint size, log2size;
-    for (size = 1, log2size = 0; size < minSize; size <<= 1, ++log2size);
-    info->tableSize = log2size;
-    info->info = calloc(size, sizeof (driOptionInfo));
-    info->values = calloc(size, sizeof (driOptionValue));
+
+    /* Make the hash table big enough to fit more than the maximum number of
+     * config options we've ever seen in a driver.
+     */
+    info->tableSize = 6;
+    info->info = calloc(1 << info->tableSize, sizeof (driOptionInfo));
+    info->values = calloc(1 << info->tableSize, sizeof (driOptionValue));
     if (info->info == NULL || info->values == NULL) {
 	fprintf (stderr, "%s: %d: out of memory.\n", __FILE__, __LINE__);
 	abort();
@@ -728,17 +711,6 @@ void driParseOptionInfo (driOptionCache *info,
 	XML_FATAL ("%s.", XML_ErrorString(XML_GetErrorCode(p)));
 
     XML_ParserFree (p);
-
-  /* Check if the actual number of options matches nConfigOptions.
-   * A mismatch is not fatal (a hash table overflow would be) but we
-   * want the driver developer's attention anyway. */
-    realNoptions = countOptions (info);
-    if (realNoptions != nConfigOptions) {
-	fprintf (stderr,
-		 "Error: nConfigOptions (%u) does not match the actual number of options in\n"
-		 "       __driConfigOptions (%u).\n",
-		 nConfigOptions, realNoptions);
-    }
 }
 
 /** \brief Parser context for configuration files. */
diff --git a/src/mesa/drivers/dri/common/xmlconfig.h b/src/mesa/drivers/dri/common/xmlconfig.h
index c363af7..d0ad42c 100644
--- a/src/mesa/drivers/dri/common/xmlconfig.h
+++ b/src/mesa/drivers/dri/common/xmlconfig.h
@@ -76,7 +76,6 @@ typedef struct driOptionCache {
     GLuint tableSize;
   /**< \brief Size of the arrays
    *
-   * Depending on the hash function this may differ from __driNConfigOptions.
    * In the current implementation it's not actually a size but log2(size).
    * The value is the same in the screen and all contexts. */
 } driOptionCache;
@@ -87,14 +86,13 @@ typedef struct driOptionCache {
  *
  * \param info    pointer to a driOptionCache that will store the option info
  * \param configOptions   XML document describing available configuration opts
- * \param nConfigOptions  number of options, used to choose a hash table size
  *
  * For the option information to be available to external configuration tools
  * it must be a public symbol __driConfigOptions. It is also passed as a
  * parameter to driParseOptionInfo in order to avoid driver-independent code
  * depending on symbols in driver-specific code. */
 void driParseOptionInfo (driOptionCache *info,
-			 const char *configOptions, GLuint nConfigOptions);
+			 const char *configOptions);
 /** \brief Initialize option cache from info and parse configuration files
  *
  * To be called in <driver>CreateContext. screenNum and driverName select
diff --git a/src/mesa/drivers/dri/i915/intel_screen.c b/src/mesa/drivers/dri/i915/intel_screen.c
index 30a867e..f8b95f4 100644
--- a/src/mesa/drivers/dri/i915/intel_screen.c
+++ b/src/mesa/drivers/dri/i915/intel_screen.c
@@ -77,8 +77,6 @@ PUBLIC const char __driConfigOptions[] =
    DRI_CONF_SECTION_END
 DRI_CONF_END;
 
-const GLuint __driNConfigOptions = 12;
-
 #include "intel_batchbuffer.h"
 #include "intel_buffers.h"
 #include "intel_bufmgr.h"
@@ -1124,8 +1122,7 @@ __DRIconfig **intelInitScreen2(__DRIscreen *psp)
       return false;
    }
    /* parse information in __driConfigOptions */
-   driParseOptionInfo(&intelScreen->optionCache,
-                      __driConfigOptions, __driNConfigOptions);
+   driParseOptionInfo(&intelScreen->optionCache, __driConfigOptions);
 
    intelScreen->driScrnPriv = psp;
    psp->driverPrivate = (void *) intelScreen;
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
index 4ee8602..9b3c31a 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -77,8 +77,6 @@ PUBLIC const char __driConfigOptions[] =
    DRI_CONF_SECTION_END
 DRI_CONF_END;
 
-const GLuint __driNConfigOptions = 12;
-
 #include "intel_batchbuffer.h"
 #include "intel_buffers.h"
 #include "intel_bufmgr.h"
@@ -1259,8 +1257,7 @@ __DRIconfig **intelInitScreen2(__DRIscreen *psp)
       return false;
    }
    /* parse information in __driConfigOptions */
-   driParseOptionInfo(&intelScreen->optionCache,
-                      __driConfigOptions, __driNConfigOptions);
+   driParseOptionInfo(&intelScreen->optionCache, __driConfigOptions);
 
    intelScreen->driScrnPriv = psp;
    psp->driverPrivate = (void *) intelScreen;
diff --git a/src/mesa/drivers/dri/radeon/radeon_screen.c b/src/mesa/drivers/dri/radeon/radeon_screen.c
index e7a27cf..dc44d4a 100644
--- a/src/mesa/drivers/dri/radeon/radeon_screen.c
+++ b/src/mesa/drivers/dri/radeon/radeon_screen.c
@@ -95,7 +95,6 @@ DRI_CONF_BEGIN
         DRI_CONF_NO_RAST("false")
     DRI_CONF_SECTION_END
 DRI_CONF_END;
-static const GLuint __driNConfigOptions = 14;
 
 #elif defined(RADEON_R200)
 
@@ -123,7 +122,6 @@ DRI_CONF_BEGIN
         DRI_CONF_NO_RAST("false")
     DRI_CONF_SECTION_END
 DRI_CONF_END;
-static const GLuint __driNConfigOptions = 15;
 
 #endif
 
@@ -492,8 +490,7 @@ radeonCreateScreen2(__DRIscreen *sPriv)
    radeon_init_debug();
 
    /* parse information in __driConfigOptions */
-   driParseOptionInfo (&screen->optionCache,
-		       __driConfigOptions, __driNConfigOptions);
+   driParseOptionInfo (&screen->optionCache, __driConfigOptions);
 
    screen->chip_flags = 0;
 
-- 
1.8.4.rc1



More information about the mesa-dev mailing list