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. > -ilia
From ccd4f17436018688c77a23ea0728316a7f5141b3 Mon Sep 17 00:00:00 2001
From: Francisco Jerez <curroje...@riseup.net>
Date: Sat, 20 Aug 2016 14:55:19 -0700
Subject: [PATCH] glsl: Fix gl_program::OutputsWritten computation for
dual-source blending.
---
src/compiler/glsl/ir_set_program_inouts.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/compiler/glsl/ir_set_program_inouts.cpp b/src/compiler/glsl/ir_set_program_inouts.cpp
index fcfbcd4..0292b67 100644
--- a/src/compiler/glsl/ir_set_program_inouts.cpp
+++ b/src/compiler/glsl/ir_set_program_inouts.cpp
@@ -96,7 +96,7 @@ mark(struct gl_program *prog, ir_variable *var, int offset, int len,
for (int i = 0; i < len; i++) {
assert(var->data.location != -1);
- int idx = var->data.location + var->data.index + offset + i;
+ int idx = var->data.location + offset + i;
bool is_patch_generic = var->data.patch &&
idx != VARYING_SLOT_TESS_LEVEL_INNER &&
idx != VARYING_SLOT_TESS_LEVEL_OUTER;
--
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
