<div dir="ltr"><div>Thanks for figuring out how to do this properly. </div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>> </div><div class="gmail_extra"> <div class="gmail_quote">On Fri, Feb 27, 2015 at 12:06 AM, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=86747" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=86747</a> Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> Cc: <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a> --- 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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div>