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

[Kristian Høgsberg]

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.

Kristian

>  - 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