On Thu, 2012-08-23 at 09:13 -0600, Brian Paul wrote:
> On 08/23/2012 08:55 AM, Imre Deak wrote:
> > The glGet hash was initialized only once for a single GL API, even if
> > the application later created a context for a different API. This
> > resulted in glGet failing for otherwise valid parameters in a context
> > if that parameter was invalid in another context created earlier.
> >
> > Amend this by also adding to the hash the parameters of a new API if the
> > application starts to use one.
> >
> > With the change the hash collision statistics reported by GET_DEBUG for
> > the combined API lookup tables aren't significantly different from the
> > single API case. The worst case is still 6 collisions for a single
> > parameter.
> >
> > Signed-off-by: Imre Deak<imre.d...@intel.com>
> > ---
> > src/mesa/main/context.c | 5 ++---
> > src/mesa/main/get.c | 41 ++++++++++++++++++++++++++++++++---------
> > 2 files changed, 34 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> > index b78bcee..9831a5f 100644
> > --- a/src/mesa/main/context.c
> > +++ b/src/mesa/main/context.c
> > @@ -404,9 +404,6 @@ one_time_init( struct gl_context *ctx )
> >
> > _mesa_get_cpu_features();
> >
> > - /* context dependence is never a one-time thing... */
> > - _mesa_init_get_hash(ctx);
> > -
> > for (i = 0; i< 256; i++) {
> > _mesa_ubyte_to_float_color_tab[i] = (float) i / 255.0F;
> > }
> > @@ -425,6 +422,8 @@ one_time_init( struct gl_context *ctx )
> >
> > /* per-API one-time init */
> > if (!(api_init_mask& (1<< ctx->API))) {
> > + _mesa_init_get_hash(ctx);
> > +
> > /*
> > * This is fine as ES does not use the remap table, but it may not
> > be
> > * future-proof. We cannot always initialize the remap table
> > because
> > diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
> > index 38d6cc3..e4818ba 100644
> > --- a/src/mesa/main/get.c
> > +++ b/src/mesa/main/get.c
> > @@ -1380,7 +1380,14 @@ static const struct value_desc values[] = {
> > * collisions for any enum (typical numbers). And the code is very
> > * simple, even though it feels a little magic. */
> >
> > -static unsigned short table[1024];
> > +#define TABLE_IDX_BITS 10
> > +
> > +#define TABLE_IDX(v) ((v)& ((1<< TABLE_IDX_BITS) - 1))
> > +#define TABLE_API(v) ((v)>> TABLE_IDX_BITS)
> > +
> > +#define TABLE_VALUE(api, idx) (((api)<< TABLE_IDX_BITS) | (idx))
> > +
> > +static unsigned short table[1<< TABLE_IDX_BITS];
> > static const int prime_factor = 89, prime_step = 281;
> >
> > #ifdef GET_DEBUG
> > @@ -1398,14 +1405,14 @@ print_table_stats(void)
> > if (!table[i])
> > continue;
> > count++;
> > - d =&values[table[i]];
> > + d =&values[TABLE_IDX(table[i])];
> > hash = (d->pname * prime_factor);
> > j = 0;
> > while (1) {
> > - if (values[table[hash& mask]].pname == d->pname)
> > - break;
> > - hash += prime_step;
> > - j++;
> > + if (values[TABLE_IDX(table[hash& mask])].pname == d->pname)
> > + break;
> > + hash += prime_step;
> > + j++;
> > }
> >
> > if (j< 10)
> > @@ -1435,6 +1442,8 @@ void _mesa_init_get_hash(struct gl_context *ctx)
> > int i, hash, index, mask;
> > int api_mask = 0, api_bit;
> >
> > + assert(Elements(values)<= 1<< TABLE_IDX_BITS);
> > +
> > mask = Elements(table) - 1;
> > api_bit = 1<< ctx->API;
> >
> > @@ -1450,9 +1459,13 @@ void _mesa_init_get_hash(struct gl_context *ctx)
> > while (1) {
> > index = hash& mask;
> > if (!table[index]) {
> > - table[index] = i;
> > + table[index] = TABLE_VALUE(api_mask, i);
> > break;
> > }
> > + /* Did some other API install the entry already? */
> > + if (TABLE_IDX(table[index]) == i&&
> > + TABLE_API(table[index]) == api_mask)
> > + break;
> > hash += prime_step;
> > }
> > }
> > @@ -1981,11 +1994,16 @@ find_value(const char *func, GLenum pname, void
> > **p, union value *v)
> > struct gl_texture_unit *unit;
> > int mask, hash;
> > const struct value_desc *d;
> > + int api_bit;
> > +
> > + api_bit = 1<< ctx->API;
> >
> > mask = Elements(table) - 1;
> > hash = (pname * prime_factor);
> > while (1) {
> > - d =&values[table[hash& mask]];
> > + unsigned table_val = table[hash& mask];
> > +
> > + d =&values[TABLE_IDX(table_val)];
> >
> > /* If the enum isn't valid, the hash walk ends with index 0,
> > * which is the API mask entry at the beginning of values[]. */
> > @@ -1995,7 +2013,12 @@ find_value(const char *func, GLenum pname, void **p,
> > union value *v)
> > return&error_value;
> > }
> >
> > - if (likely(d->pname == pname))
> > + if (likely(d->pname == pname&& (TABLE_API(table_val)& api_bit)))
> > + /*
> > + * Don't bail out here if the API doesn't match, since the same
> > pname
> > + * can be present in the table for other APIs (with different
> > + * semantics).
> > + */
> > break;
> >
> > hash += prime_step;
>
> I don't fully grok the hashing changes but this looks OK to me
> assuming you've run piglit, etc.
Khm. Didn't run he piglit test :/ I only ran my own test so far (being
new to mesa development). So I'll run the piglit tests now, perhaps
adding a new test case for the bug this fixes and report back.
> Is this a candidate for the 8.0 branch too?
Yes, though it only applies with a trivial conflict.
--Imre
>
> For the series:
> Reviewed-by: Brian Paul <bri...@vmware.com>
>
> -Brian
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
