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

Tue Aug 23 18:31:06 UTC 2016

Francisco Jerez <currojerez at riseup.net> writes:

> Ilia Mirkin <imirkin at alum.mit.edu> writes:
>
>> 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,
>
> That would be an acceptable limitation if it were taken into account
> consistently (which would imply among other things binding gl_FragColor
> to FRAG_RESULT_DATA0 if gl_SecondaryFragColor is written).  Otherwise
> you will be telling the linker and the back-end that both
> FRAG_RESULT_COLOR and FRAG_RESULT_DATA0 are being written, which is
> illegal.  The i965 has been misbehaving since forever because of this,
> but we just didn't notice until I added that assertion because the only
> symptom is increased shader recompiles.
>
>> (as it would, among other things, imply multi-rt support for
>> dual-source blending)
>
> FRAG_RESULT_COLOR + index != 0 doesn't imply multi-rt support, it's a
> requirement for multi-rt support, which the GLSL spec makes room for and
> some drivers in this tree could potentially support if the GLSL IR
> representation of dual-source blending wasn't deliberately inconsistent.
>
>> and the former code was making the GLSL fe not emit the illegal
>> combination.
>>
> I started digging into this and found out that that's not the only way
> the GLSL-to-TGSI pass relies on a GLSL bug: The output semantic array
> calculation in st_translate_fragment_program() relies on there being
> multiple locations marked as written in the shader's OutputsWritten set
> in the dual-source blending case (IOW you're relying on the bug fixed by
> the previous patch too).  GLSL is not TGSI, in GLSL the secondary and
> primary colors of a dual-source-blended output have the same location,
> so you cannot expect there will be multiple elements present in a bitset
> of locations.
>
>> Whichever way you look at it, breaking st/mesa isn't a great option.
>
> Then you may want to take a look at the attached patches in addition.
> They are untested and I have close to zero experience with the
> GLSL-to-TGSI pass, so they may break st/mesa after all.
>

Oops, I attached the wrong 0001-glsl.*.patch file, these are the right
patches.

>>   -ilia
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-glsl-Calculate-bitset-of-secondary-outputs-written-i.patch
Type: text/x-diff
Size: 2160 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160823/0502349f/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-st-glsl_to_tgsi-Translate-fragment-shader-secondary-.patch
Type: text/x-diff
Size: 3656 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160823/0502349f/attachment-0003.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160823/0502349f/attachment-0001.sig>