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

[Erik Faye-Lund]

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");