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

[Jason Ekstrand]

Thanks for figuring out how to do this properly.

Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>

On Fri, Feb 27, 2015 at 12:06 AM, Kenneth Graunke <kenneth at 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 <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
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150227/f5deb8fa/attachment-0001.html>