[Mesa-dev] [PATCH 2/3] glsl: Fix incorrect hard-coded location of the gl_SecondaryFragColorEXT built-in.

[Ilia Mirkin]

On Tue, Aug 23, 2016 at 12:18 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>
>> On Tue, Aug 23, 2016 at 12:05 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>>>
>>>> On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>>>>> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>>>>>
>>>>>> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>>>>>>> gl_SecondaryFragColorEXT should have the same location as gl_FragColor
>>>>>>> for the secondary fragment color to be replicated to all fragment
>>>>>>> outputs.  The incorrect location of gl_SecondaryFragColorEXT would
>>>>>>> cause the linker to mark both FRAG_RESULT_COLOR and FRAG_RESULT_DATA0
>>>>>>> as being written to, which isn't allowed by the spec and would
>>>>>>> ultimately lead to an assertion failure in
>>>>>>> fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.
>>>>>>
>>>>>> My recollection was that it didn't work with COLOR for "stupid"
>>>>>> reasons. Can you confirm that
>>>>>> bin/arb_blend_func_extended-fbo-extended-blend-pattern_gles2 -auto
>>>>>> passes with this patch?
>>>>>>
>>>>> Yes, it does, in fact
>>>>> arb_blend_func_extended-fbo-extended-blend-pattern_gles2 hits the i965
>>>>> assertion failure I mentioned above unless this patch is applied.
>>>>
>>>> This causes the test in question to fail on nouveau... the TGSI shader
>>>> generated starts with
>>>>
>>>> FRAG
>>>> PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1
>>>> DCL IN[0], POSITION, LINEAR
>>>> DCL OUT[0], SAMPLEMASK
>>>
>>> Heh, this smells a lot like the bug fixed in PATCH 1, but somewhere in
>>> the mesa state tracker.  st_glsl_to_tgsi.cpp:2422 does:
>>>
>>> | entry = new(mem_ctx) variable_storage(var,
>>> |                                       PROGRAM_OUTPUT,
>>> |                                       var->data.location
>>> |                                       + var->data.index);
>>>
>>> which is obviously bogus, e.g. for var->data.location ==
>>> FRAG_RESULT_COLOR and var->data.index == 1 you get
>>> FRAG_RESULT_SAMPLE_MASK which explains the sample mask declaration
>>> above.
>>
>> Right, because having FRAG_RESULT_COLOR and index != 0 was never
>> possible prior to this. That might be why Ryan stuck it into
>> FRAG_RESULT_DATA0 [I may have been the one to suggest that].
>
> Heh, so I guess that's the "stupid" reason you were referring to,
> working around this mesa state tracker bug in the GLSL front-end.

Right. Or another way of looking at it, FRAG_RESULT_COLOR + index != 0
is illegal (as it would, among other things, imply multi-rt support
for dual-source blending), and the former code was making the GLSL fe
not emit the illegal combination. Whichever way you look at it,
breaking st/mesa isn't a great option.

  -ilia