[Mesa-dev] [PATCH 1/3] i965: Don't use message headers for typed/untyped reads

[Jordan Justen]

On 2015-10-19 12:23:27, Kristian Høgsberg wrote:
> On Mon, Oct 19, 2015 at 3:19 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> > Hey Kristian,
> >
> > Kristian Høgsberg Kristensen <krh at bitplanet.net> writes:
> >
> >> We always set the mask to 0xffff, which is what it defaults to when no
> >> header is present. Let's drop the header instead.
> >>
> >> Signed-off-by: Kristian Høgsberg Kristensen <krh at bitplanet.net>
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_eu_emit.c | 3 +--
> >>  src/mesa/drivers/dri/i965/brw_fs.cpp    | 4 ++--
> >>  2 files changed, 3 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> >> index bf2fee9..ebd811f 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> >> @@ -2906,11 +2906,10 @@ brw_untyped_surface_read(struct brw_codegen *p,
> >>     const unsigned sfid = (devinfo->gen >= 8 || devinfo->is_haswell ?
> >>                            HSW_SFID_DATAPORT_DATA_CACHE_1 :
> >>                            GEN7_SFID_DATAPORT_DATA_CACHE);
> >> -   const bool align1 = (brw_inst_access_mode(devinfo, p->current) == BRW_ALIGN_1);
> >>     struct brw_inst *insn = brw_send_indirect_surface_message(
> >>        p, sfid, dst, payload, surface, msg_length,
> >>        brw_surface_payload_size(p, num_channels, true, true),
> >> -      align1);
> >> +      false);
> >>
> >>     brw_set_dp_untyped_surface_read_message(
> >>        p, insn, num_channels);
> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> index a2fd441..457bf59 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> @@ -4032,7 +4032,7 @@ fs_visitor::lower_logical_sends()
> >>        case SHADER_OPCODE_UNTYPED_SURFACE_READ_LOGICAL:
> >>           lower_surface_logical_send(ibld, inst,
> >>                                      SHADER_OPCODE_UNTYPED_SURFACE_READ,
> >> -                                    fs_reg(0xffff));
> >> +                                    fs_reg());
> >>           break;
> >>
> >>        case SHADER_OPCODE_UNTYPED_SURFACE_WRITE_LOGICAL:
> >> @@ -4050,7 +4050,7 @@ fs_visitor::lower_logical_sends()
> >>        case SHADER_OPCODE_TYPED_SURFACE_READ_LOGICAL:
> >>           lower_surface_logical_send(ibld, inst,
> >>                                      SHADER_OPCODE_TYPED_SURFACE_READ,
> >> -                                    fs_reg(0xffff));
> >> +                                    fs_reg());
> >>           break;
> >>
> >
> > Whatever you do here must be in agreement with the generator and whether
> > it sets the header-present bit in the message descriptor or not,
> > otherwise you're likely to get a hang or misrendering (I guess Jenkins
> > would've caught this).
> >
> > However, according to my hardware docs "Typed messages (which go to the
> > render cache data port) must include the header.".  There's no mention
> > of which generation that restriction applies to, but I believe it
> > applies to IVB/VLV *only*, which is the only generation in which typed
> > surface messages are handled by the render cache instead of by the data
> > cache -- The parenthesized comment doesn't quite make sense on newer
> > gens.  A quick test shows no piglit regressions on HSW after removing
> > the header from typed surface read messages.
> >
> > I guess there are two things you could do, I'm okay with either:
> >
> >  - Just drop this hunk in order to stick to the letter of the BSpec and
> >    always pass a header together with typed surface read messages.  I
> >    have the strong suspicion though that the docs are just being
> >    inaccurate and the header is in fact unnecessary on HSW+.  No need to
> >    resend if you decide to go down this path, if you just drop the
> >    change for typed reads feel free to include my:
> >
> >    Reviewed-by: Francisco Jerez <currojerez at riseup.net>
> 
> You're right, that should not have been there. I did get some image
> load/store regressions from this (on BDW as well) that I only noticed
> this morning. Backing out this change fixes it, thanks.

With this change:

Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>

and, at least with CS,

Tested-by: Jordan Justen <jordan.l.justen at intel.com>

> >  - Pass 'gen == 7 ? fs_reg(0xffff) : fs_reg()' as sample_mask argument
> >    to lower_surface_logical_send() in the TYPED_SURFACE_READ case.  For
> >    this to work you'll also have to change the generator code to pass
> >    the fs_inst::header_size field down to brw_typed_surface_read() so it
> >    knows whether the header is present or not.
> >
> >>        case SHADER_OPCODE_TYPED_SURFACE_WRITE_LOGICAL:
> >> --
> >> 2.6.2
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev