Thanks for figuring out how to do this properly. Reviewed-by: Jason Ekstrand <jason.ekstr...@intel.com>
On Fri, Feb 27, 2015 at 12:06 AM, Kenneth Graunke <kenn...@whitecape.org> wrote: > 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://bugs.freedesktop.org/show_bug.cgi?id=86747 > Signed-off-by: Kenneth Graunke <kenn...@whitecape.org> > Cc: mesa-sta...@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
