On Mon, Feb 16, 2015 at 10:28 AM, Ben Widawsky <b...@bwidawsk.net> wrote:
> On Mon, Feb 16, 2015 at 09:24:34AM -0800, Jason Ekstrand wrote: > > On Feb 16, 2015 7:46 AM, "Francisco Jerez" <curroje...@riseup.net> > wrote: > > > > > > Jason Ekstrand <ja...@jlekstrand.net> writes: > > > > > > > On Feb 15, 2015 11:55 PM, "Ben Widawsky" < > benjamin.widaw...@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. > > > > Good enough for me. With the control flow change below, > > > > Reviewed-by: Jason Ekstrand <jason.ekstr...@intel.com> > > > > Hi. Thanks for asking this, it reminded me that I should throw a comment > in the > code as well - it took me a few hours to track down all the relevant parts > of > the docs, and this commit message is a bit dense (and that was after > several > hours to roughly figure out what was going on). > Yeah, I like comments in code better than in commit messages. That way you see it when you read it rather than having to do git archeology. > Thanks to the both of you. I will make the requested change. > > > > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87258 > > > >> Cc: Kristian Høgsberg <k...@bitplanet.net> > > > >> Signed-off-by: Ben Widawsky <b...@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@lists.freedesktop.org > > > >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > _______________________________________________ > > > > mesa-dev mailing list > > > > mesa-dev@lists.freedesktop.org > > > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
