[Mesa-dev] [PATCH 10/12] mesa: Use _mesa_lock_context_textures in _mesa_GetTexParameterfv()

[Kristian Høgsberg]

On Wed, Aug 13, 2014 at 01:30:52PM -0700, Kenneth Graunke wrote:
> On Monday, August 11, 2014 05:29:40 PM Kristian Høgsberg wrote:
> > GetTexParamterfv() doesnt change texture state, so instead of
> > _mesa_lock_texture() we can use _mesa_lock_context_textures(),
> > which doesn't increase the texture stamp.  With this change,
> > _mesa_update_state_locked() is now only called from under
> > _mesa_lock_context_textures(), which is right thing to do.  Right now
> > it's the same mutex, but if we made texture locking more fine grained
> > locking one day, just locking one texture here would be wrong.
> > 
> > This all ignores the fact that texture locking seem a bit
> > flaky and broken, but we're trying to not blatantly make it worse.
> > 
> > This change allows us to reliably unlock the context textures in the
> > dd::UpdateState callback as is necessary for meta color resolves.
> > 
> > Signed-off-by: Kristian Høgsberg <krh at bitplanet.net>
> > ---
> >  src/mesa/main/texparam.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/mesa/main/texparam.c b/src/mesa/main/texparam.c
> > index 30dd0b9..e40fb24 100644
> > --- a/src/mesa/main/texparam.c
> > +++ b/src/mesa/main/texparam.c
> > @@ -1384,7 +1384,7 @@ _mesa_GetTexParameterfv( GLenum target, GLenum pname, GLfloat *params )
> >     if (!obj)
> >        return;
> >  
> > -   _mesa_lock_texture(ctx, obj);
> > +   _mesa_lock_context_textures(ctx);
> >     switch (pname) {
> >        case GL_TEXTURE_MAG_FILTER:
> >  	 *params = ENUM_TO_FLOAT(obj->Sampler.MagFilter);
> > @@ -1591,11 +1591,11 @@ _mesa_GetTexParameterfv( GLenum target, GLenum pname, GLfloat *params )
> >     }
> >  
> >     /* no error if we get here */
> > -   _mesa_unlock_texture(ctx, obj);
> > +   _mesa_unlock_context_textures(ctx);
> >     return;
> >  
> >  invalid_pname:
> > -   _mesa_unlock_texture(ctx, obj);
> > +   _mesa_unlock_context_textures(ctx);
> >     _mesa_error(ctx, GL_INVALID_ENUM, "glGetTexParameterfv(pname=0x%x)", pname);
> >  }
> >  
> > 
> 

> Yeah...the old code incremented the texture timestamp, and I don't
> see any point in that.  It doesn't alter the state at all.  This
> patch retains taking the mutex.
>
> But, it really seems like you need to do the analogous change in
> _mesa_GetTexParameteriv().  I think you'll hit the same issue, and
> are just getting lucky...

It doesn't trigger for _mesa_GetTexParameteriv because it doesn't call
_mesa_update_state_locked().  That's where we eventually may do a meta
resolve, and it's only called from _mesa_GetTexParameterfv.  Locking
all context textures is only required when calling
_mesa_update_state_locked(), so it looks like only locking the texture
in question with _mesa_lock_texture() is cheaper.  But that bumps the
texture stamp, which in turn dirties _NEW_TEXTURE, even though we
don't modify the texture.

So I'm not sure... we could change it to avoid unecessarily bumping
the stamp, but that seems like an orthogonal change at that point.

Kristian

> With that fixed,
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>