[Mesa-dev] [PATCH v3 06/11] glsl: Add arb_cull_distance support

[Ian Romanick]

On 05/09/2016 12:24 PM, Tobias Klausmann wrote:
> 
> On 09.05.2016 20:41, Ian Romanick wrote:
> 
>> snip
>>> +      /* From the ARB_cull_distance spec:
>>> +       *
>>> +       * It is a compile-time or link-time error for the set of
>>> shaders forming
>>> +       * a program to have the sum of the sizes of the
>>> gl_ClipDistance and
>>> +       * gl_CullDistance arrays to be larger than
>>> +       * gl_MaxCombinedClipAndCullDistances.
>>> +       */
>>> +      if ((*clip_distance_array_size + *cull_distance_array_size) >
>>> +          ctx->Const.MaxClipPlanes) {
>>> +          linker_error(prog, "%s shader: the combined size of "
>>> +                       "'gl_ClipDistance' and 'gl_CullDistance' size
>>> cannot "
>>> +                       "be larger than "
>>> +                       "gl_MaxCombinedClipAndCullDistances (%u)",
>>> +                       _mesa_shader_stage_to_string(shader->Stage),
>>> +                       ctx->Const.MaxClipPlanes);
>>> +      }
>> I think this is the wrong check for this limitation in the spec.  This
>> will only catch cases where the usages within a stage exceed the limit.
>>   I *believe* (but it's not 100% clear from the quotation) that a vertex
>> shader that writes  MaxClipPlanes elements to gl_ClipDisatnce and a
>> geometry shader that also writes MaxClipPlances to gl_CullDistance
>> should fail to link.  If that is the case, then we need checks elsewhere
>> that use LastClipDistanceArraySize and LastCullDistanceArray size to
>> enforce the limits.
> 
> I think you are right, but where would be the best place (stage) to
> check this? (You know the codebase way better)

Right after LastClipDistanceArraySize is set in link_shaders seems like
a good place.  Maybe pull that code, the code added by this patch to set
LastCullDistanceArraySize, and the new validation code to a new
function.  link_shaders is pretty long. :(

> Thanks,
> Tobias