[Mesa-dev] [PATCH 2/2] mesa: fix GLSL program objects with more than 16 samplers combined

[Marek Olšák]

1) The tabs are used in the entire file. They might have got there
while I was moving some of the bits of code around. I don't really use
tabs for core Mesa work. I can change the tabs to spaces unless you
mean I should change everything to tabs (you didn't make that clear).

2) _mesa_get_sampler_uniform_value is really called at link time,
specifically at the conversion to TGSI. The only case where it might
be called after link time is when a new shader variant is being
generated, though i'm not really sure about this. Anyway, the function
already calls linker_error above my code to report an error. I decided
to do the same. _mesa_problem sounds better though.

Marek

On Tue, May 21, 2013 at 11:54 PM, Eric Anholt <eric at anholt.net> wrote:
> Marek Olšák <maraeo at gmail.com> writes:
>
>> The problem is the sampler units are allocated from the same pool for all
>> shader stages, so if a vertex shader uses 12 samplers (0..11), the fragment
>> shader samplers start at index 12, leaving only 4 sampler units
>> for the fragment shader. The main cause is probably the fact that samplers
>> (texture unit -> sampler unit mapping, etc.) are tracked globally
>> for an entire program object.
>>
>> This commit adapts the GLSL linker and core Mesa such that the sampler units
>> are assigned to sampler uniforms for each shader stage separately
>> (if a sampler uniform is used in all shader stages, it may occupy a different
>> sampler unit in each, and vice versa, an i-th sampler unit may refer to
>> a different sampler uniform in each shader stage), and the sampler-specific
>> variables are moved from gl_shader_program to gl_shader.
>>
>> This doesn't require any driver changes, and it fixes piglit/max-samplers
>> for gallium and classic swrast. It also works with any number of shader
>> stages.
>
> I was initially uncomfortable with gl_uniform_storage getting bigger,
> but it's *huge* already, and the increased overhead I thought I saw in
> uniform updates isn't really there.  So, just nitpicks:
>
>> @@ -335,8 +339,38 @@ public:
>>     int ubo_block_index;
>>     int ubo_byte_offset;
>>     bool ubo_row_major;
>> +   gl_shader_type shader_type;
>>
>>  private:
>> +   void handle_samplers(const glsl_type *base_type,
>> +                        struct gl_uniform_storage *uniform)
>> +   {
>> +
>> +      if (base_type->is_sampler()) {
>> +         uniform->sampler[shader_type].index = this->next_sampler;
>> +         uniform->sampler[shader_type].active = true;
>> +
>> +      /* Increment the sampler by 1 for non-arrays and by the number of
>> +       * array elements for arrays.
>> +       */
>
> bad tab indentation
>
>> +         this->next_sampler +=
>> +               MAX2(1, uniform->array_elements);
>> +
>> +      const gl_texture_index target = base_type->sampler_index();
>> +      const unsigned shadow = base_type->sampler_shadow;
>
> tabs
>
>> +         for (unsigned i = uniform->sampler[shader_type].index;
>> +              i < MIN2(this->next_sampler, MAX_SAMPLERS);
>> +              i++) {
>> +         this->targets[i] = target;
>> +         this->shader_samplers_used |= 1U << i;
>> +         this->shader_shadow_samplers |= shadow << i;
>> +      }
>
> tabs
>
>> @@ -388,26 +397,16 @@ private:
>>        base_type = type;
>>        }
>>
>> -      if (base_type->is_sampler()) {
>> -      this->uniforms[id].sampler = this->next_sampler;
>> +      /* This assigns sampler uniforms to sampler units. */
>> +      handle_samplers(base_type, &this->uniforms[id]);
>>
>> -      /* Increment the sampler by 1 for non-arrays and by the number of
>> -       * array elements for arrays.
>> -       */
>> -      this->next_sampler += MAX2(1, this->uniforms[id].array_elements);
>> -
>> -      const gl_texture_index target = base_type->sampler_index();
>> -      const unsigned shadow = base_type->sampler_shadow;
>> -      for (unsigned i = this->uniforms[id].sampler
>> -              ; i < MIN2(this->next_sampler, MAX_SAMPLERS)
>> -              ; i++) {
>> -         this->targets[i] = target;
>> -         this->shader_samplers_used |= 1U << i;
>> -         this->shader_shadow_samplers |= shadow << i;
>> -      }
>> -
>> -      } else {
>> -      this->uniforms[id].sampler = ~0;
>> +      /* If there is already storage associated with this uniform, it means
>> +       * that it was set while processing an earlier shader stage.  For
>> +       * example, we may be processing the uniform in the fragment shader, but
>> +       * the uniform was already processed in the vertex shader.
>> +       */
>> +      if (this->uniforms[id].storage != NULL) {
>> +      return;
>
> tabs
>
>> @@ -119,6 +122,14 @@ _mesa_get_sampler_uniform_value(class ir_dereference *sampler,
>>        return 0;
>>     }
>>
>> -   return shader_program->UniformStorage[location].sampler + getname.offset;
>> -}
>> +   if (!shader_program->UniformStorage[location].sampler[shader].active) {
>> +      linker_error(shader_program,
>> +                "cannot return a sampler named %s, because it is not "
>> +                   "used in this shader stage. This is a driver bug.\n",
>> +                   getname.name);
>> +      return 0;
>> +   }
>
> Maybe _mesa_problem()?  This isn't really called at link time, and you
> want something really visible like an assert.
>
> Other than the style bits,
>
> Reviewed-by: Eric Anholt <eric at anholt.net>