On 2015-10-19 12:23:27, Kristian Høgsberg wrote: > On Mon, Oct 19, 2015 at 3:19 AM, Francisco Jerez <curroje...@riseup.net> > wrote: > > Hey Kristian, > > > > Kristian Høgsberg Kristensen <k...@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 <k...@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 <curroje...@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.jus...@intel.com> and, at least with CS, Tested-by: Jordan Justen <jordan.l.jus...@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev