[Mesa-dev] [PATCH] i965/simd8vs: Fix SIMD8 atomics

[Francisco Jerez]

Jason Ekstrand <jason at jlekstrand.net> writes:

> On Feb 15, 2015 11:55 PM, "Ben Widawsky" <benjamin.widawsky at intel.com>
> wrote:
>>
>> The short version: we need to set bits in R0.7 which provide a mask to be
> used
>> for PS kill samples/pixels. Since the VS has no such concept, we just
> need to
>> set all 1.
>>
>> The longer version...
>> Execution for SIMD8 atomics is defined as follows:
>> SIMD8: The low 8 bits of the execution mask are ANDed with 8 bits of the
>> Pixel/Sample Mask from the message header. For the typed messages, the
> Slot
>> Group in the message descriptor selects either the low or high 8 bits.
> For the
>> untyped messages, the low 8 bits are always selected. The resulting mask
> is used
>> to determine which slots are read into the destination GRF register (for
> read),
>> or which slots are written to the surface (for write). If the header is
> not
>> present, only the low 8 bits of the execution mask are used.
>>
>> The message header for untyped messages is defined in R0.7 "This field
> contains
>> the 16-bit pixel/sample mask to be used for SIMD16 and SIMD8 messages.
> All 16
>> bits are used for SIMD16 messages.  For typed SIMD8 messages, Slot Group
> selects
>> which 8 bits of this field are used. For untyped SIMD8 messages, the low
> 8 bits
>> of this field are used." Furthermore, "The message header for the untyped
>> messages only needs to be delivered for pixel shader threads, where the
>> execution mask may indicate pixels/samples that are enabled only due to
>> derivative (LOD) calculations, but the corresponding slot on the surface
> must
>> not be accessed." We're not using a pixel shader here, but AFAICT, this
> mask is
>> used for all stages.
>>
>> This leaves two options, Remove the header, or make the VS code emit the
> correct
>> thing for the header. I believe one of the goals of using SIMD8 VS was to
> get as
>> much code reuse as possible, and so I chose the latter. Since the VS has
> no such
>> thing as kill instructions, the mask is derived simple as all 1's.
>
> Hm. This seems a little fishy. Don't we still have an execution mask for
> vertex shaders?  The second half of the if copies that into the bit field.
>

That's fine.  The execution mask is part of the side-band information
sent together with the message payload to the data port. It's implicitly
ANDed with the sample mask sent in the header to compute the actual set
of channels that execute the atomic operation.  What the "second half of
the if" copies is not the execution mask but the sample mask used to
identify helper invocations of the fragment shader executed to
approximate derivatives which are required not to have side effects.

>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87258
>> Cc: Kristian Høgsberg <krh at bitplanet.net>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> index 2a36d94..c20289f 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> @@ -2994,6 +2994,9 @@ fs_visitor::emit_untyped_atomic(unsigned atomic_op,
> unsigned surf_index,
>>     if (uses_kill) {
>>        emit(MOV(component(sources[0], 7), brw_flag_reg(0, 1)))
>>           ->force_writemask_all = true;
>> +   } else if (stage == MESA_SHADER_VERTEX) {
>> +      emit(MOV(component(sources[0], 7),
>> +               brw_imm_ud(0xff)))->force_writemask_all = true;
>>     } else {
>>        emit(MOV(component(sources[0], 7),
>>                 retype(brw_vec1_grf(1, 7), BRW_REGISTER_TYPE_UD));
>
> Also would you mind rearranging the logic a bit here.  Something like this
> would make it more clear that the header stuff is only for vertex shaders:
>
> If (fragment) {
>    If (uses_kill) {
>    } else {
>    }
> } else {
>    // vertex stuff
> }
>

Agreed, I had the same thought when I reviewed this but didn't care
enough to complain. :)

>> --
>> 2.3.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150216/d8f02265/attachment.sig>