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

Submitted by Francisco Jerez on Aug. 23, 2016, 6:24 p.m.

Details

Message ID 87k2f79we5.fsf@riseup.net
State New
Headers show
Series "Series without cover letter" ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

Francisco Jerez Aug. 23, 2016, 6:24 p.m.
Ilia Mirkin <imirkin@alum.mit.edu> writes:

> On Tue, Aug 23, 2016 at 12:18 AM, Francisco Jerez <currojerez@riseup.net> wrote:
>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>
>>> On Tue, Aug 23, 2016 at 12:05 AM, Francisco Jerez <currojerez@riseup.net> wrote:
>>>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>>>
>>>>> On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez <currojerez@riseup.net> wrote:
>>>>>> Ilia Mirkin <imirkin@alum.mit.edu> writes:
>>>>>>
>>>>>>> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez <currojerez@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.

>   -ilia

Patch hide | download patch | download mbox

From 4b6657e057a1e0af4dd7adec84ae31fe89e0b135 Mon Sep 17 00:00:00 2001
From: Francisco Jerez <currojerez@riseup.net>
Date: Tue, 23 Aug 2016 11:18:19 -0700
Subject: [PATCH 2/2] st/glsl_to_tgsi: Translate fragment shader secondary
 outputs to TGSI outputs.

---
 src/mesa/state_tracker/st_glsl_to_tgsi.cpp |  5 +++--
 src/mesa/state_tracker/st_program.c        | 16 ++++++++++------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
index 5a9cadc..7d8228c 100644
--- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
@@ -2419,7 +2419,8 @@  glsl_to_tgsi_visitor::visit(ir_dereference_variable *ir)
             entry = new(mem_ctx) variable_storage(var,
                                                   PROGRAM_OUTPUT,
                                                   var->data.location
-                                                  + var->data.index);
+                                                  + FRAG_RESULT_MAX *
+                                                    var->data.index);
          }
          this->variables.push_tail(entry);
          break;
@@ -5367,7 +5368,7 @@  dst_register(struct st_translate *t, gl_register_file file, unsigned index,
    case PROGRAM_OUTPUT:
       if (!array_id) {
          if (t->procType == PIPE_SHADER_FRAGMENT)
-            assert(index < FRAG_RESULT_MAX);
+            assert(index < 2 * FRAG_RESULT_MAX);
          else if (t->procType == PIPE_SHADER_TESS_CTRL ||
                   t->procType == PIPE_SHADER_TESS_EVAL)
             assert(index < VARYING_SLOT_TESS_MAX);
diff --git a/src/mesa/state_tracker/st_program.c b/src/mesa/state_tracker/st_program.c
index 03a685c..2a4edfa 100644
--- a/src/mesa/state_tracker/st_program.c
+++ b/src/mesa/state_tracker/st_program.c
@@ -586,7 +586,7 @@  bool
 st_translate_fragment_program(struct st_context *st,
                               struct st_fragment_program *stfp)
 {
-   GLuint outputMapping[FRAG_RESULT_MAX];
+   GLuint outputMapping[2 * FRAG_RESULT_MAX];
    GLuint inputMapping[VARYING_SLOT_MAX];
    GLuint inputSlotToAttr[VARYING_SLOT_MAX];
    GLuint interpMode[PIPE_MAX_SHADER_INPUTS];  /* XXX size? */
@@ -810,9 +810,13 @@  st_translate_fragment_program(struct st_context *st,
       }
 
       /* handle remaining outputs (color) */
-      for (attr = 0; attr < FRAG_RESULT_MAX; attr++) {
-         if (outputsWritten & BITFIELD64_BIT(attr)) {
-            switch (attr) {
+      for (attr = 0; attr < ARRAY_SIZE(outputMapping); attr++) {
+         const GLbitfield64 written = attr < FRAG_RESULT_MAX ? outputsWritten :
+            stfp->Base.Base.SecondaryOutputsWritten;
+         const unsigned loc = attr % FRAG_RESULT_MAX;
+
+         if (written & BITFIELD64_BIT(loc)) {
+            switch (loc) {
             case FRAG_RESULT_DEPTH:
             case FRAG_RESULT_STENCIL:
             case FRAG_RESULT_SAMPLE_MASK:
@@ -822,8 +826,8 @@  st_translate_fragment_program(struct st_context *st,
             case FRAG_RESULT_COLOR:
                write_all = GL_TRUE; /* fallthrough */
             default:
-               assert(attr == FRAG_RESULT_COLOR ||
-                      (FRAG_RESULT_DATA0 <= attr && attr < FRAG_RESULT_MAX));
+               assert(loc == FRAG_RESULT_COLOR ||
+                      (FRAG_RESULT_DATA0 <= loc && loc < FRAG_RESULT_MAX));
                fs_output_semantic_name[fs_num_outputs] = TGSI_SEMANTIC_COLOR;
                fs_output_semantic_index[fs_num_outputs] = numColors;
                outputMapping[attr] = fs_num_outputs;
-- 
2.9.0