[Mesa-dev] [PATCH] i965/vec4: Fix TCS output reads with non-zero component qualifiers.

[Jason Ekstrand]

On Wed, Dec 14, 2016 at 12:45 AM, Kenneth Graunke <kenneth at whitecape.org>
wrote:

> We want to perform the URB read to a vec4 temporary, with no writemask,
> then issue a MOV to swizzle the data and store it to the actual
> destination, using the final writemask.
>
> We were doing this wrong.  For example, let's say we wanted to read
> a vec2 stored in components 2-3 of a vec4.  We would generate a URB
> read message of:
>
>    SEND <actual destination>.XY <header with mask set to XY>
>    MOV <actual destination>.XY <actual destination>.ZW
>
> This doesn't work, because the URB message reads the .XY components
> of the vec4, rather than the ZW.  It writes to the right place, but
> with the wrong data.  Then the MOV comes along and overwrites it
> with data that didn't even come from the URB at all.
>
> Instead we want to do:
>
>    SEND <temporary>.ZW <header with mask set to ZW>
>    MOV <actual destination>.XY <temporary>.ZW
>
> or more simply:
>
>    SEND <temporary>.XYZW <header with mask set to XYZW>
>    MOV <actual destination>.XY <temporary>.ZW
>
> Cc: mesa-stable at lists.freedesktop.org
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_vec4_tcs.cpp | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_tcs.cpp
> b/src/mesa/drivers/dri/i965/brw_vec4_tcs.cpp
> index c7278e4..9d090eb 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_tcs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_tcs.cpp
> @@ -219,9 +219,10 @@ vec4_tcs_visitor::emit_output_urb_read(const dst_reg
> &dst,
>     read->base_mrf = -1;
>
>     if (first_component) {
> -      src_reg src = src_reg(dst);
> -      src.swizzle = BRW_SWZ_COMP_INPUT(first_component);
> -      emit(MOV(dst, src));
> +      read->dst = retype(dst_reg(this, glsl_type::ivec4_type), dst.type);
> +      emit(MOV(dst, swizzle(src_reg(read->dst),
> +                            BRW_SWZ_COMP_INPUT(first_component))));
> +      inst->src[0] = brw_imm_ud(WRITEMASK_XYZW);
>

This works.  I'm not sure how I feel about this much modification of the
two instructions after the fact though.  Why not just change the
SET_OUTPUT_URB_OFFSETS to have brw_imm_ud(dst.writemask << first_component)
and then you would only be whacking the read instruction itself.

Also, there's a comment that says that the read ignores writemasks which it
clearly doesn't.  We should probably fix that.

In any case, this does what it says it does and my suggestion is purely
cosmetic.

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>


>     }
>  }
>
> --
> 2.10.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161214/2cdf5813/attachment.html>