[Mesa-dev] [PATCH] mesa: add const flags to skip MaxVarying and MaxUniform linker checks

[Marek Olšák]

On Sat, Dec 10, 2011 at 8:19 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 12/09/2011 07:14 PM, Marek Olšák wrote:
>>
>> This is only temporary until a better solution is available.
>> ---
>>
>> I haven't given up yet because reporting incorrect driver limits
>> may cause more troubles than this. I made it pretty small this
>> time.
>>
>> It doesn't change any driver besides Gallium. Please review,
>> especially the part in st_extensions.c whether it's acceptable
>> or whether some other conditions should be used instead.
>> (e.g. strcmp(get_vendor(), "X.Org R300 Project") is possible too)
>>
>> I plan to implement a better solution, so that this code can go
>> away.
>>
>> I see only two ways out of this.
>
>
> To be honest, I we want to do both.
>
>
>> The first one is:
>> - Let Gallium drivers report an error when they can't run
>>   a certain shader.
>
>
> I was thinking this would look like:
>
> - Have the linker calculate the most conservative and most optimistic
> uniform and varying usage.
>
> - Have the linker call into the driver to calculate the actual usage.
> Driver's that don't optimize can just report back the data supplied by the
> linker.
>
> - If the driver's usage calculation is less than the conservative
> calculation, generate a link warning as a portability aid.
>
> - If the driver's usage calculation is less than the optimistic calculation,
> generate an assertion failure.  Clearly, some piece of software is broken,
> and some sort of other failure is likely imminent.
>
> - If the driver's usage calculation is greater than the driver's advertised
> limits, generate a link error.

Sounds good.

>
>
>> The second one is:
>> - Remove the Mesa IR.
>
>
> It won't be much longer until i915 and r200 are the only users of Mesa IR.
>  It's a pretty good fit for that hardware, so it won't disappear. However,
> it should disappear from the execution paths of every other driver.
>
>
>> - Replace TGSI by the GLSL IR completely.
>
>
> I think that would be useful, but it doesn't seem strictly necessary.
>
>
>> - Move the dead-constant elimination pass from the R300 compiler
>>   to the GLSL IR.
>
>
> I've added this to my todo list.  The new uniform tracking mechanism may
> make this a little bit more complex.
>
>
>> - Implement a GLSL optimization pass which breaks varying arrays
>>   into separate elements when possible.
>
>
> This may not be necessary in the short term.  I think r300 is
> under-reporting it's capabilities.  See
> https://bugs.freedesktop.org/show_bug.cgi?id=34201#c9

The problem is st/mesa is under-reporting its capabilities, because
when I was writing that code, I thought color varyings just don't
belong to the max varying limit. The color varyings shouldn't be
considered a generic varying resource (at least internally in Mesa),
because there may have different precision. They don't have to be
float when clamped, and they don't have to be exactly 32-bit float
when not clamped (r300 is an example of such a behavior).

I see this solution:

We don't need exact GL limits in gl_constants, only the limits which
make sense for drivers, like Gallium has. So we'd have something like
MaxGenericVaryings and glGet*(GL_MAX_VARYING_FLOATS) would return
MaxGenericVaryings*4+8. Then the linker wouldn't count the color
varyings in the number of used varying components. How does it sound?

>
>
>> Thanks.
>>
>>  src/glsl/linker.cpp                    |    9 ++++++---
>>  src/mesa/main/mtypes.h                 |    9 +++++++++
>>  src/mesa/state_tracker/st_extensions.c |    8 ++++++++
>>  3 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index 3527088..fe35ed3 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -1814,7 +1814,8 @@ assign_varying_locations(struct gl_context *ctx,
>>     }
>>
>>     if (ctx->API == API_OPENGLES2 || prog->Version == 100) {
>> -      if (varying_vectors>  ctx->Const.MaxVarying) {
>> +      if (varying_vectors>  ctx->Const.MaxVarying&&
>> +          !ctx->Const.GLSLSkipStrictMaxVaryingLimitCheck) {
>>         linker_error(prog, "shader uses too many varying vectors "
>>                      "(%u>  %u)\n",
>>                      varying_vectors, ctx->Const.MaxVarying);
>
>
> Even when we have compaction in the front-end, it seems useful, as a
> portability aid, to keep a warning in this case.

Will do.

>
>
>> @@ -1822,7 +1823,8 @@ assign_varying_locations(struct gl_context *ctx,
>>        }
>>     } else {
>>        const unsigned float_components = varying_vectors * 4;
>> -      if (float_components>  ctx->Const.MaxVarying * 4) {
>> +      if (float_components>  ctx->Const.MaxVarying * 4&&
>> +          !ctx->Const.GLSLSkipStrictMaxVaryingLimitCheck) {
>>         linker_error(prog, "shader uses too many varying components "
>>                      "(%u>  %u)\n",
>>                      float_components, ctx->Const.MaxVarying * 4);
>> @@ -1959,7 +1961,8 @@ check_resources(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>                      shader_names[i]);
>>        }
>>
>> -      if (sh->num_uniform_components>  max_uniform_components[i]) {
>> +      if (sh->num_uniform_components>  max_uniform_components[i]&&
>> +          !ctx->Const.GLSLSKipStrictMaxUniformLimitCheck) {
>>           linker_error(prog, "Too many %s shader uniform components",
>>                      shader_names[i]);
>>        }
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index fc494f7..7e9f6ca 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -2829,6 +2829,15 @@ struct gl_constants
>>      * Texture borders are deprecated in GL 3.0.
>>      **/
>>     GLboolean StripTextureBorder;
>> +
>> +   /**
>> +    * For drivers which can do a better job at eliminating unused
>> varyings
>> +    * and uniforms than the GLSL compiler.
>> +    *
>> +    * XXX Remove these as soon as a better solution is available.
>> +    */
>> +   GLboolean GLSLSkipStrictMaxVaryingLimitCheck;
>> +   GLboolean GLSLSKipStrictMaxUniformLimitCheck;
>>  };
>>
>>
>> diff --git a/src/mesa/state_tracker/st_extensions.c
>> b/src/mesa/state_tracker/st_extensions.c
>> index 37fb3e7..fdaffa8 100644
>> --- a/src/mesa/state_tracker/st_extensions.c
>> +++ b/src/mesa/state_tracker/st_extensions.c
>> @@ -224,6 +224,14 @@ void st_init_limits(struct st_context *st)
>>     c->UniformBooleanTrue = ~0;
>>
>>     c->StripTextureBorder = GL_TRUE;
>> +
>> +   c->GLSLSKipStrictMaxUniformLimitCheck =
>> +      screen->get_shader_param(screen, PIPE_SHADER_FRAGMENT,
>> +                               PIPE_SHADER_CAP_MAX_CONSTS)<= 256;
>> +
>> +   c->GLSLSkipStrictMaxVaryingLimitCheck =
>> +      screen->get_shader_param(screen, PIPE_SHADER_FRAGMENT,
>> +                               PIPE_SHADER_CAP_MAX_INPUTS)<= 10;
>
>
> This will break NVfx (Geforce5) drivers.  On NVfx, the limits are 32 and 10,
> respectively, and the the nouveaux maintainers have said that they don't
> have (and won't implement) the array compacting that r300 has.
>
> Maybe adding PIPE_SHADER_CAP_CAN_COMPACT_INPUT_ARRAYS and
> PIPE_SHADER_CAP_CAN_COMPACT_CONST_ARRAYS would be better?

Sounds good, will do.

Marek