[virglrenderer-devel] [PATCH] expand gl_SampleMask to ivec4 to match TGSI definition

[Erik Faye-Lund]

On 06. juli 2018 09:21, Erik Faye-Lund wrote:
> On 06. juli 2018 09:05, Dave Airlie wrote:
>> On 6 July 2018 at 17:00, Erik Faye-Lund 
>> <erik.faye-lund at collabora.com> wrote:
>>> On 06. juli 2018 02:12, Dave Airlie wrote:
>>>> On 5 July 2018 at 17:43, Erik Faye-Lund<erik.faye-lund at collabora.com>
>>>> wrote:
>>>>> TGSI defines TGSI_SEMANTIC_SAMPLEMASK to be a four-compoent integer
>>>>> vector, with the x component set to the same value as gl_SampleMask,
>>>>> and the y, z and w components set to 0.
>>>> I think the other patch was more correct, the value is SV[0].xxxx, 
>>>> which
>>>> means
>>>> it's swizzled X into all 4 components.
>>>>
>>> I don't think so. Here's what the documentation says:
>>>
>>> https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/gallium/docs/source/tgsi.rst#L3239 
>>>
>>>
>>> "If MSAA is not enabled, the value is (1, 0, 0, 0).
>>>
>>> For an output, the sample mask is used to disable further sample 
>>> processing.
>>>
>>> For both, the register type is uint[4] but only the X component is used
>>> (i.e. gl_SampleMask[0])."
>>>
>>> If we repeat the same value to all components, we don't fulfill the
>>> (1, 0, 0, 0)-bit.
>>>
>>> So I think this is a more accurate implementation than repeating, even
>>> though
>>> it *probably* doesn't matter much, as I think only hand-written TGSI 
>>> shaders
>>> can actually reach the other components. And I seriously doubt we 
>>> have any
>>> hand-written TGSI that cases about the distinction...
>> Nope you are still not getting it :-)
>>
>> SV[0].xyzw is a 4 component looking like "gl_SampleMask[0], 0,0,0"
>> SV[0].xxxx is what we see which is "gl_SampleMask[0],
>> gl_SampleMask[0], gl_SampleMask[0], gl_SampleMask[0]"
>>
>> The swizzling matters here.
>>
>> Now we actually are probably wrong in a fair few cases for system
>> values as I've tended towards what works
>> instead of doing it properly.
>>
>> We should probably construct a full explicit vec4 using the swizzles
>> to work out each value, instead of the
>> lazy (uvec4(gl_SampleMask[0])) we do now, though we coould also detect
>> a .xxxx swizzle and just do that,
>> and have a backup for the non .xxxx cases.
> OK, I think I get what you're saying now. There's a swizzle in
> the instruction here that usually is .xxxx and we're ignoring
> it, right?
>
> If so, perhaps something like this is correct?
>
> snprintf(srcs[i], 255, "uvec4(%s, %s, %s, %s)",
>          src->Register.SwizzleX == TGSI_SWIZZLE_X ? 
> ctx->system_values[j].glsl_name : "0",
>          src->Register.SwizzleY == TGSI_SWIZZLE_X ? 
> ctx->system_values[j].glsl_name : "0",
>          src->Register.SwizzleZ == TGSI_SWIZZLE_X ? 
> ctx->system_values[j].glsl_name : "0",
>          src->Register.SwizzleW == TGSI_SWIZZLE_X ? 
> ctx->system_values[j].glsl_name : "0");
Ugh, with ivec4 instead of uvec4, of course :P