On Wed, Oct 16, 2013 at 3:49 PM, Ian Romanick <i...@freedesktop.org> wrote:
> On 10/16/2013 11:29 AM, Kenneth Graunke wrote:
>> On 10/16/2013 10:40 AM, Ian Romanick wrote:
>>> On 10/15/2013 07:50 PM, Kenneth Graunke wrote:
>> [snip]
>>>> Right, but gl_SampleMask is not an unsized array. It's statically sized
>>>> based on your implementation's maximum supported number of color samples
>>>> (ceil(samples/32)), and this is totally known at compile time...
>>>
>>> That doesn't match what the spec says. At least the GLSL 4.00 spec:
>>>
>>> "In the fragment language, built-in variables are intrinsically
>>> declared as:
>>>
>>> in vec4 gl_FragCoord;
>>> in bool gl_FrontFacing;
>>> in float gl_ClipDistance[];
>>> in vec2 gl_PointCoord;
>>> in int gl_PrimitiveID;
>>> in int gl_SampleID;
>>> in vec2 gl_SamplePosition;
>>> out vec4 gl_FragColor;
>>> out vec4 gl_FragData[gl_MaxDrawBuffers];
>>> out float gl_FragDepth;
>>> out int gl_SampleMask[];"
>>>
>>> Note that both gl_ClipDistance and gl_SampleMask are unsized, and both
>>> have a maximum size based on implementation limits. Later the GLSL 4.00
>>> spec says:
>>>
>>> "This array must be sized in the fragment shader either implicitly
>>> or explicitly to be the same size as the implementation-dependent
>>> maximum sample-mask (as an array of 32bit elements), determined by
>>> the maximum number of samples."
>>>
>>> The maximum size of the array is ceil(max_samples/32), but initially it
>>> is unsized.
>>
>> You've got to be kidding me. This is utter and complete garbage.
>> "We know exactly how big the array is, at compile time, but we're going
>> to make you, the technical artist, tell us anyway. By the way, you have
>> to guess the exact number we're thinking."
>>
>> So, about that "explicitly the same size" text...what if my shader sets
>> it to some /other/ size? What happens?
>>
>> * Larger than ceil(float(gl_NumSamples)/32.0):
>>
>> A compile error? Truncated to the right size? Probably the same
>> behavior as ClipDistance. That doesn't seem well specified either.
>>
>> * Smaller than ceil(float(gl_NumSamples)/32.0):
>>
>> From the GLSL 4.40 specification,
>> "If the fragment shader statically assigns a value to gl_SampleMask,
>> the sample mask will be undefined for any array elements of any
>> fragment shader invocations that fail to assign a value."
>>
>> The hardware is expecting a certain number of samples, so ultimately,
>> the fact that you don't have them means they're undefined. Just like
>> the above case of forgetting to assign them. Do we pretend the array is
>> the right size, ignoring their request and treating any unwritten
>> elements as unsized? Or do we reject it as a compile error?
>>
>> ...
>>
>> Also, considering that the Radeon 8970 (arguably AMD's best card today)
>> only does 24x MSAA, and it looks like nVidia's GTX Titan doesn't do more
>> than 32x (though it's harder to tell), this array will always be exactly
>> size 1 for every implementation in existence.
>
> Looking that revision history, it was a fairly late change to make
> gl_SampleMask an array at all. It was just a scalar int.
>
>> <sarcasm>
>> I'm sure applications are fully on board to explicitly declare this and
>> cope with the consequences of it not being a single integer.
>> </sarcasm>
>
> Maybe. I doubt there are very many applications that use gl_SampleMask.
> Of the ones that do, they probably also use gl_SampleMaskIn and do
> something like:
>
> gl_SampleMask = gl_SampleMaskIn; // of course this fails. duh
>
> or
>
> for (...)
> gl_SampleMask[i] = gl_SampleMaskIn[i];
>
> Note that the spec says "implicitly or explicitly". The latter form
> implicitly sizes gl_SampleMask to be the same size as gl_SampleMaskIn.
> In ARB_gpu_shader5, gl_SampleMaskIn has somewhat more sensible language:
>
> in int gl_SampleMaskIn[];
>
> The variable gl_SampleMaskIn is an array of integers, each holding a
> bitfield indicating the set of samples covered by the primitive
> generating the fragment during multisample rasterization. The
> array has ceil(<s>/32) elements, where <s> is the maximum number of
> color samples supported by the implementation. Bit <n> or word <w>
> in the bitfield is set if and only if the sample numbered
> <w>*32+<n> is considered covered for this fragment shader
> invocation.
>
>> That said, at this point, it's about 4 years too late to change the
>> idiocy of the spec, so we're kind of stuck. We probably ought to
>> investigate what other vendors do.
>
> You are completely correct here. We should check what other vendors do.
> I think 5 tests will tell us everything we need to know (then we'll
> probably submit some spec bugs). I don't have any non-Intel hardware
> that supports either extension, so I can't actually try any of these.
>
I'll try these tests on AMD and NVIDIA hadware.
> 1. Does this compile, link, and run:
>
> #version 130
> #extension GL_ARB_sample_shading: require
>
> out vec4 color;
>
> void main() {
> color = vec4(gl_SampleMask.length());
> gl_SampleMask[0] = ~0;
> }
>
> 2. Does this compile and link:
>
> #version 130
> #extension GL_ARB_sample_shading: require
>
> out vec4 color;
>
> void main() {
> color = vec4(1);
> gl_SampleMask[0] = ~0;
> gl_SampleMask[1] = ~0;
> }
>
> 3. Does this compile and link:
>
> #version 130
> #extension GL_ARB_sample_shading: require
> #extension GL_ARB_gpu_shader5: require
>
> out vec4 color;
>
> void main() {
> color = vec4(1);
> gl_SampleMask = gl_SampleMaskIn;
> }
>
> 4. Does this compile and link:
>
> #version 130
> #extension GL_ARB_sample_shading: require
>
> out vec4 color;
> in int gl_SampleMask[1];
>
> void main() {
> color = vec4(1);
> gl_SampleMask[0] = ~0;
> }
>
> 5. Does this compile and link:
>
> #version 130
> #extension GL_ARB_sample_shading: require
>
> out vec4 color;
> in int gl_SampleMask[2];
>
> void main() {
> color = vec4(1);
> gl_SampleMask[0] = ~0;
> gl_SampleMask[1] = ~0;
> }
>
>> --Ken
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
