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

[Francisco Jerez]

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>

 - 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151019/f808b638/attachment.sig>