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

[Eric Anholt]

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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130521/9554c33d/attachment.pgp>