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

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

2015-10-20 Thread Jordan Justen

On 2015-10-19 12:23:27, Kristian Høgsberg wrote:
> On Mon, Oct 19, 2015 at 3:19 AM, Francisco Jerez  
> wrote:
> > Hey Kristian,
> >
> > Kristian Høgsberg Kristensen  writes:
> >
> >> We always set the mask to 0x, which is what it defaults to when no
> >> header is present. Let's drop the header instead.
> >>
> >> Signed-off-by: Kristian Høgsberg Kristensen 
> >> ---
> >>  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(0x));
> >> +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(0x));
> >> +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 
> 
> 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 

and, at least with CS,

Tested-by: Jordan Justen 

> >  - Pass 'gen == 7 ? fs_reg(0x) : 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-10-19 Thread Iago Toral

On Sun, 2015-10-18 at 21:31 -0700, Kristian Høgsberg Kristensen wrote:
> We always set the mask to 0x, which is what it defaults to when no
> header is present. Let's drop the header instead.

Right, according to the PRM (3.9.9 Typed/Untyped Surface Read/Write and
Typed/Untyped Atomic Operation):

"SIMD16: The 16 bits of the execution mask are ANDed with the 16 bits of
the Pixel/Sample Mask from the message header and 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 execution mask is used."

Similar text exists for SIMD8 and SIMD4x2 is not affected by Sample
mask.

Reviewed-by: Iago Toral Quiroga 

> Signed-off-by: Kristian Høgsberg Kristensen 
> ---
>  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(0x));
> +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(0x));
> +fs_reg());
>   break;
>  
>case SHADER_OPCODE_TYPED_SURFACE_WRITE_LOGICAL:


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-10-19 Thread Francisco Jerez

Hey Kristian,

Kristian Høgsberg Kristensen  writes:

> We always set the mask to 0x, which is what it defaults to when no
> header is present. Let's drop the header instead.
>
> Signed-off-by: Kristian Høgsberg Kristensen 
> ---
>  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(0x));
> +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(0x));
> +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 

 - Pass 'gen == 7 ? fs_reg(0x) : 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


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-10-19 Thread Kristian Høgsberg

On Mon, Oct 19, 2015 at 3:19 AM, Francisco Jerez  wrote:
> Hey Kristian,
>
> Kristian Høgsberg Kristensen  writes:
>
>> We always set the mask to 0x, which is what it defaults to when no
>> header is present. Let's drop the header instead.
>>
>> Signed-off-by: Kristian Høgsberg Kristensen 
>> ---
>>  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(0x));
>> +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(0x));
>> +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 

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(0x) : 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2015-10-18 Thread Kristian Høgsberg Kristensen

We always set the mask to 0x, which is what it defaults to when no
header is present. Let's drop the header instead.

Signed-off-by: Kristian Høgsberg Kristensen 
---
 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(0x));
+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(0x));
+fs_reg());
  break;
 
   case SHADER_OPCODE_TYPED_SURFACE_WRITE_LOGICAL:
-- 
2.6.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

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

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

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

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

5 matches

Site Navigation

Mail list logo

Footer information