[Mesa-dev] [PATCH] i965/simd8vs: Fix SIMD8 atomics
Jason Ekstrand
jason at jlekstrand.net
Mon Feb 16 10:30:06 PST 2015
On Mon, Feb 16, 2015 at 10:28 AM, Ben Widawsky <ben at 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" <currojerez at riseup.net>
> wrote:
> > >
> > > 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.
> >
> > Good enough for me. With the control flow change below,
> >
> > Reviewed-by: Jason Ekstrand <jason.ekstrand at 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 <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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150216/96072e4c/attachment.html>
More information about the mesa-dev
mailing list