[Mesa-stable] [Mesa-dev] [PATCH 4/4] i965/fs: Don't issue FB writes for bound but unwritten color targets.

[Roland Scheidegger]

I was just wondering, this actually clearly looks like a workaround for a broken app.
GLSL and OGL have language such as "The variable gl_FragData is an array. Writing to gl_FragData[n] specifies the fragment data that will be used by the subsequent fixed functionality pipeline for data n. If subsequent fixed functionality consumes fragment data and an execution of a fragment shader executable does not write a value to it, then the fragment data consumed is undefined."
Granted the wording isn't that obvious, because the "an execution" could mean it is only undefined if the output is statically assigned but not always written, however there isn't anything else more closely specifying this (unless I'm missing something), thus imho the only valid conclusion is this also applies to shaders not statically assigning such outputs.
Hence writing garbage is perfectly fine (we do the same in llvmpipe unless I'm mistaken).

Not that I'm against this patch, but it would probably make sense to notify the app developer so they can fix it.

________________________________________
Von: mesa-dev <mesa-dev-bounces at lists.freedesktop.org> im Auftrag von Kenneth Graunke <kenneth at whitecape.org>
Gesendet: Freitag, 27. Februar 2015 09:06
An: mesa-dev at lists.freedesktop.org
Cc: mesa-stable at lists.freedesktop.org
Betreff: [Mesa-dev] [PATCH 4/4] i965/fs: Don't issue FB writes for bound but    unwritten color targets.

We used to loop over all color attachments, and emit FB writes for each
one, even if the shader didn't write to a corresponding output variable.
Those color attachments would be filled with garbage (undefined values).

Football Manager binds a framebuffer with 4 color attachments, but draws
to it using a shader that only writes to gl_FragData[0..2].  This meant
that color attachment 3 would be filled with garbage, resulting in
rendering artifacts.  Now we skip writing to it, fixing rendering.

Writes to gl_FragColor initialize outputs[0..nr_color_regions-1] to
GRFs, while writes to gl_FragData[i] initialize outputs[i].

Bugzilla: https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.freedesktop.org_show-5Fbug.cgi-3Fid-3D86747&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Vjtt0vs_iqoI31UfJxBl7yv9I2FeiaeAYgMTLKRBc_I&m=ctnWRGAk7zRL2j0O05I8WjATVzAEtib9kuQ2GekVlyc&s=qf_P1myV3GNoVXrmcQJFMOCVxZNPs8dzD58EcbsV3mk&e=
Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
Cc: mesa-stable at lists.freedesktop.org
---
 src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

While this should be much more correct, I wonder if more fixes are needed.

It seems like the loop condition should be:

   for (int target = 0; target < BRW_MAX_DRAW_BUFFERS; target++)

Even if we have only 1 color region, it could be attached to
GL_COLOR_ATTACHMENT3.  (Ilia mentioned this should be legal.)
It sure looks like that would break here (and probably always has).
Unfortunately, fixing that means binding more NULL render targets,
and that's complex enough I think it's best left to a follow-up series.

diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
index 77e1103..dc5c2fe 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
@@ -3646,7 +3646,7 @@ fs_visitor::emit_fb_writes()
          do_dual_src = false;
    }

-   fs_inst *inst;
+   fs_inst *inst = NULL;
    if (do_dual_src) {
       this->current_annotation = ralloc_asprintf(this->mem_ctx,
                                                 "FB dual-source write");
@@ -3654,8 +3654,12 @@ fs_visitor::emit_fb_writes()
                                   reg_undef, 4);
       inst->target = 0;
       prog_data->dual_src_blend = true;
-   } else if (key->nr_color_regions > 0) {
+   } else {
       for (int target = 0; target < key->nr_color_regions; target++) {
+         /* Skip over outputs that weren't written. */
+         if (this->outputs[target].file == BAD_FILE)
+            continue;
+
          this->current_annotation = ralloc_asprintf(this->mem_ctx,
                                                     "FB write target %d",
                                                     target);
@@ -3668,7 +3672,9 @@ fs_visitor::emit_fb_writes()
                                      this->output_components[target]);
          inst->target = target;
       }
-   } else {
+   }
+
+   if (inst == NULL) {
       /* Even if there's no color buffers enabled, we still need to send
        * alpha out the pipeline to our null renderbuffer to support
        * alpha-testing, alpha-to-coverage, and so on.
--
2.2.2

_______________________________________________
mesa-dev mailing list
mesa-dev at lists.freedesktop.org
https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Vjtt0vs_iqoI31UfJxBl7yv9I2FeiaeAYgMTLKRBc_I&m=ctnWRGAk7zRL2j0O05I8WjATVzAEtib9kuQ2GekVlyc&s=-pel-EQQHsBhZuMJnIcN2YN4-h02tivD3TXg20tOnSU&e=