[Mesa-dev] [PATCH 5/6] st/mesa: optimize pipe_sampler_view validation

[Brian Paul]

On 10/06/2016 01:45 AM, Nicolai Hähnle wrote:
> On 06.10.2016 02:42, Brian Paul wrote:
>> Before, st_get_texture_sampler_view_from_stobj() did a lot of work to
>> check if the texture parameters matched the sampler view (format,
>> swizzle, min/max lod, first/last layer, etc).  We did this every time
>> we validated the texture state.
>>
>> Now, we use a ctx->Driver.TexParameter() callback and a couple other
>> checks to proactively release texture views when we know that
>> view-related parameters have changed.  Then, the validation step is
>> simplified:
>> - Search the texture's list of sampler views (just match the context).
>> - If found, we're done.
>> - Else, create a new sampler view.
>>
>> There will never be old, out-of-date sampler views attached to texture
>> objects that we have to test.
>>
>> Most apps create textures and set the texture parameters once.  This
>> make sampler view validation much cheaper for that case.
>>
>> Note that the old texture/sampler comparison code has been converted
>> into a set of assertions to verify that the sampler view is in fact
>> consistent with the texture parameters.  This should help to spot any
>> potential regressions.
>>
>> Reviewed-by: Edward O'Callaghan <funfunctor at folklore1984.net>
>> ---
>>  src/mesa/state_tracker/st_atom_texture.c | 58
>> ++++++++++++++++++++------------
>>  src/mesa/state_tracker/st_cb_texture.c   | 52
>> ++++++++++++++++++++++++++++
>>  src/mesa/state_tracker/st_texture.c      | 16 ++++-----
>>  src/mesa/state_tracker/st_texture.h      |  5 +++
>>  4 files changed, 101 insertions(+), 30 deletions(-)
>>
>> diff --git a/src/mesa/state_tracker/st_atom_texture.c
>> b/src/mesa/state_tracker/st_atom_texture.c
>> index bfa16dc..45f1f6b 100644
>> --- a/src/mesa/state_tracker/st_atom_texture.c
>> +++ b/src/mesa/state_tracker/st_atom_texture.c
>> @@ -370,7 +370,7 @@ st_create_texture_sampler_view_from_stobj(struct
>> st_context *st,
>>  static struct pipe_sampler_view *
>>  st_get_texture_sampler_view_from_stobj(struct st_context *st,
>>                                         struct st_texture_object *stObj,
>> -                       enum pipe_format format,
>> +                                       const struct gl_sampler_object
>> *samp,
>>                                         unsigned glsl_version)
>>  {
>>     struct pipe_sampler_view **sv;
>> @@ -381,34 +381,42 @@ st_get_texture_sampler_view_from_stobj(struct
>> st_context *st,
>>
>>     sv = st_texture_get_sampler_view(st, stObj);
>>
>> -   /* if sampler view has changed dereference it */
>>     if (*sv) {
>> -      if (check_sampler_swizzle(st, stObj, *sv, glsl_version) ||
>> -      (format != (*sv)->format) ||
>> -          gl_target_to_pipe(stObj->base.Target) != (*sv)->target ||
>> -          stObj->base.MinLevel + stObj->base.BaseLevel !=
>> (*sv)->u.tex.first_level ||
>> -          last_level(stObj) != (*sv)->u.tex.last_level ||
>> -          stObj->base.MinLayer != (*sv)->u.tex.first_layer ||
>> -          last_layer(stObj) != (*sv)->u.tex.last_layer) {
>> -     pipe_sampler_view_reference(sv, NULL);
>> +      /* Debug check: make sure that the sampler view's parameters are
>> +       * what they're supposed to be.
>> +       */
>> +      struct pipe_sampler_view *view = *sv;
>> +      assert(!check_sampler_swizzle(st, stObj, view, glsl_version));
>> +      assert(get_sampler_view_format(st, stObj, samp) == view->format);
>> +      assert(gl_target_to_pipe(stObj->base.Target) == view->target);
>> +      if (stObj->base.Target == GL_TEXTURE_BUFFER) {
>> +         unsigned base = stObj->base.BufferOffset;
>> +         unsigned size = MIN2(stObj->pt->width0 - base,
>> +                              (unsigned) stObj->base.BufferSize);
>> +         assert(view->u.buf.offset == base);
>> +         assert(view->u.buf.size == size);
>> +      }
>> +      else {
>
> Huh, so is this else-style intentional, or a spill-over from another
> code base? I wasn't aware of this style in st/mesa.

I've been doing it that way for about 25 years.  The } else { style is 
relatively new to Mesa and I guess I haven't adopted that habit yet.

'git grep else' shows a mix of styles.


> Also, the whole sampler lookup code gives me the race condition chills,
> but it looks like it's always been that way, so... patches 4 & 5:
>
> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> Patch 6 is Acked-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

Thanks for reviewing.

-Brian