[Mesa-dev] [PATCH 3/3] mesa: glGet: fix parameter lookup for apps using multiple APIs

[Imre Deak]

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.deak at 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 <brianp at vmware.com>
> 
> -Brian