On Feb 16, 2015 7:46 AM, "Francisco Jerez" <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> wrote: > > Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes: > > > On Feb 15, 2015 11:55 PM, "Ben Widawsky" <<a href="mailto:benjamin.widawsky@intel.com">benjamin.widawsky@intel.com</a>> > > 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. Good enough for me. With the control flow change below, Reviewed-by: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>> > >> Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=87258">https://bugs.freedesktop.org/show_bug.cgi?id=87258</a> > >> Cc: Kristian Høgsberg <<a href="mailto:krh@bitplanet.net">krh@bitplanet.net</a>> > >> Signed-off-by: Ben Widawsky <<a href="mailto:ben@bwidawsk.net">ben@bwidawsk.net</a>> > >> --- > >> 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 > >> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > >> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > _______________________________________________ > > mesa-dev mailing list > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>