[Mesa-dev] [PATCH] dri: Choose a decent global driNConfigOptions.
Ian Romanick
idr at freedesktop.org
Thu Aug 8 11:58:58 PDT 2013
On 08/08/2013 03:59 AM, Eric Anholt wrote:
> 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().
This makes sense. I thought part of the reason for __driNConfigOptions
being exact was that it was used by external driconf GUI tools. Will
this change break those tools?
> ---
> 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;
>
>
More information about the mesa-dev
mailing list