[Mesa-dev] [PATCH] i965: Fix last slot calculations

[Timothy Arceri]

On Wed, 2017-01-04 at 21:33 -0800, Kenneth Graunke wrote:
> If the VUE map has slots at the end which the shader does not write,
> then we'd "flush" (constructing an URB write) on the last output it
> actually wrote.  Then, we'd construct another SEND with EOT, but with
> no actual payload data.  That's not legal.
> 
> For example, SSO programs have clip distance slots allocated no
> matter
> what, but the shader may not write them.  If it doesn't write any
> user
> defined varyings, then the clip distance slots will be the last ones.
> 
> Found while debugging
> dEQP-VK.tessellation.shader_input_output.gl_position_vs_to_tcs_to_tes
> 
> Cc: mesa-stable at lists.freedesktop.org
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index 14415bd5c7a..08ac7bc276a 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -672,6 +672,14 @@ fs_visitor::emit_urb_writes(const fs_reg
> &gs_vertex_count)
>     length = 0;
>     urb_offset = starting_urb_offset;
>     flush = false;
> +

It might be nice to add a small comment about why we have slots that
are not written to here.

/* SSO shaders can have slots that are never actually written to so
 * ignore them when looking for the last slot.
 */

Or I'm sure you can come up with something better.

Otherwise:
 
Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>

> +   int last_slot = vue_map->num_slots - 1;
> +   while (last_slot > 0 &&
> +          (vue_map->slot_to_varying[last_slot] ==
> BRW_VARYING_SLOT_PAD ||
> +           outputs[vue_map->slot_to_varying[last_slot]].file ==
> BAD_FILE)) {
> +      last_slot--;
> +   }
> +
>     for (slot = 0; slot < vue_map->num_slots; slot++) {
>        int varying = vue_map->slot_to_varying[slot];
>        switch (varying) {
> @@ -757,8 +765,7 @@ fs_visitor::emit_urb_writes(const fs_reg
> &gs_vertex_count)
>         * the last slot or if we need to flush (see BAD_FILE varying
> case
>         * above), emit a URB write send now to flush out the data.
>         */
> -      int last = slot == vue_map->num_slots - 1;
> -      if (length == 8 || last)
> +      if (length == 8 || slot == last_slot)
>           flush = true;
>        if (flush) {
>           fs_reg *payload_sources =
> @@ -777,7 +784,7 @@ fs_visitor::emit_urb_writes(const fs_reg
> &gs_vertex_count)
>                             header_size);
>  
>           fs_inst *inst = abld.emit(opcode, reg_undef, payload);
> -         inst->eot = last && stage != MESA_SHADER_GEOMETRY;
> +         inst->eot = slot == last_slot && stage !=
> MESA_SHADER_GEOMETRY;
>           inst->mlen = length + header_size;
>           inst->offset = urb_offset;
>           urb_offset = starting_urb_offset + slot + 1;