Francisco Jerez <curroje...@riseup.net> writes: > Ilia Mirkin <imir...@alum.mit.edu> writes: > >> On Tue, Aug 23, 2016 at 12:18 AM, Francisco Jerez <curroje...@riseup.net> >> wrote: >>> Ilia Mirkin <imir...@alum.mit.edu> writes: >>> >>>> On Tue, Aug 23, 2016 at 12:05 AM, Francisco Jerez <curroje...@riseup.net> >>>> wrote: >>>>> Ilia Mirkin <imir...@alum.mit.edu> writes: >>>>> >>>>>> On Mon, Aug 22, 2016 at 10:55 PM, Francisco Jerez >>>>>> <curroje...@riseup.net> wrote: >>>>>>> Ilia Mirkin <imir...@alum.mit.edu> writes: >>>>>>> >>>>>>>> On Mon, Aug 22, 2016 at 9:59 PM, Francisco Jerez >>>>>>>> <curroje...@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 >
From 7503e974d08636ae83d7248af8cc5991bd0034e5 Mon Sep 17 00:00:00 2001
From: Francisco Jerez <curroje...@riseup.net>
Date: Tue, 23 Aug 2016 11:15:57 -0700
Subject: [PATCH 1/2] glsl: Calculate bitset of secondary outputs written in
ir_set_program_inouts.
---
src/compiler/glsl/ir_set_program_inouts.cpp | 8 ++++++--
src/mesa/main/mtypes.h | 1 +
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/ir_set_program_inouts.cpp b/src/compiler/glsl/ir_set_program_inouts.cpp
index 0292b67..44c20e5 100644
--- a/src/compiler/glsl/ir_set_program_inouts.cpp
+++ b/src/compiler/glsl/ir_set_program_inouts.cpp
@@ -135,10 +135,14 @@ mark(struct gl_program *prog, ir_variable *var, int offset, int len,
prog->SystemValuesRead |= bitfield;
} else {
assert(var->data.mode == ir_var_shader_out);
- if (is_patch_generic)
+ if (is_patch_generic) {
prog->PatchOutputsWritten |= bitfield;
- else if (!var->data.read_only)
+ } else if (!var->data.read_only) {
prog->OutputsWritten |= bitfield;
+ if (var->data.index > 0)
+ prog->SecondaryOutputsWritten |= bitfield;
+ }
+
if (var->data.fb_fetch_output)
prog->OutputsRead |= bitfield;
}
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index caf93ee..c12d981 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -1907,6 +1907,7 @@ struct gl_program
GLbitfield64 InputsRead; /**< Bitmask of which input regs are read */
GLbitfield64 DoubleInputsRead; /**< Bitmask of which input regs are read and are doubles */
GLbitfield64 OutputsWritten; /**< Bitmask of which output regs are written */
+ GLbitfield64 SecondaryOutputsWritten; /**< Subset of OutputsWritten outputs written with non-zero index. */
GLbitfield64 OutputsRead; /**< Bitmask of which output regs are read */
GLbitfield PatchInputsRead; /**< VAR[0..31] usage for patch inputs (user-defined only) */
GLbitfield PatchOutputsWritten; /**< VAR[0..31] usage for patch outputs (user-defined only) */
--
2.9.0
From 4b6657e057a1e0af4dd7adec84ae31fe89e0b135 Mon Sep 17 00:00:00 2001
From: Francisco Jerez <curroje...@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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
