[Mesa-dev] [HACK] i965/fs: Fix ordering of src0 alpha and oMask in the framebuffer write payload.

[Jason Ekstrand]

On Jul 9, 2015 7:57 AM, "Francisco Jerez" <currojerez at riseup.net> wrote:
>
> We were passing src0 alpha and oMask in reverse order.  There seems to
> be no good way to pass them in the correct order to the new-style
> LOAD_PAYLOAD (how surprising) because src0 alpha is per-channel while
> oMask is not.  Just split src0 alpha in fixed-width registers and pass
> them to LOAD_PAYLOAD as if they were part of the header as work-around
> for now.

Bah... I came across this when I did the LOAD_PAYLOAD rework but thought it
was only theoretical.  I wasn't very familiar with what omask actually did
and, since piglit didn't hit it, I wasn't sure if it was a real problem or
not.  I probably should have done more digging and written a piglit test at
the time. My bad.

One solution that I proposed at the time was to turn header_size into
header_mask in the obvious way. We can still use 8 bits because we should
never have a header source higher than 8.

Thoughts?
--Jason

> I've written a piglit test that demonstrates the problem by using
> gl_SampleMask from a fragment shader with multiple color outputs [1].
>
> [1] http://lists.freedesktop.org/archives/piglit/2015-July/016499.html
> ---
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 26
+++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index 94d6a58..304ae74 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -1535,6 +1535,19 @@ fs_visitor::emit_single_fb_write(const fs_builder
&bld,
>        length++;
>     }
>
> +   if (src0_alpha.file != BAD_FILE && color0.file != BAD_FILE) {
> +      /* Neat, we need to chop the src0 alpha component and pass it as
part of
> +       * the header even though it has per-channel semantics, because
the next
> +       * optional field is header-like and LOAD_PAYLOAD requires all such
> +       * fields to form a contiguous segment at the beginning of the
message.
> +       */
> +      for (unsigned i = 0; i < exec_size / 8; i++) {
> +         setup_color_payload(&sources[length], src0_alpha, 1, 8,
> +                             use_2nd_half || i == 1);
> +         length++;
> +      }
> +   }
> +
>     prog_data->uses_omask =
>        prog->OutputsWritten & BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK);
>     if (prog_data->uses_omask) {
> @@ -1561,19 +1574,14 @@ fs_visitor::emit_single_fb_write(const fs_builder
&bld,
>                               offset(this->outputs[0], bld, 3),
>                               1, exec_size, false);
>        length += 4;
> -   } else if (color1.file == BAD_FILE) {
> -      if (src0_alpha.file != BAD_FILE) {
> -         setup_color_payload(&sources[length], src0_alpha, 1, exec_size,
false);
> -         length++;
> -      }
> -
> -      setup_color_payload(&sources[length], color0, components,
> -                          exec_size, use_2nd_half);
> -      length += 4;
>     } else {
>        setup_color_payload(&sources[length], color0, components,
>                            exec_size, use_2nd_half);
>        length += 4;
> +
> +   }
> +
> +   if (color1.file != BAD_FILE) {
>        setup_color_payload(&sources[length], color1, components,
>                            exec_size, use_2nd_half);
>        length += 4;
> --
> 2.4.3
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150709/33aacb7b/attachment.html>