<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 16, 2016 at 5:59 PM, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes:<br>
<br>
> On Sep 16, 2016 3:04 PM, "Francisco Jerez" <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> wrote:<br>
>><br>
>> Not intended for upstream. Should cause a GPU hang if some thread is<br>
>> executed with a non-contiguous dispatch mask breaking assumptions of<br>
>> brw_stage_has_packed_dispatch(<wbr>). Doesn't cause any CTS, DEQP or<br>
>> Piglit regressions, while replacing brw_stage_has_packed_dispatch(<wbr>)<br>
>> with a dummy implementation that unconditionally returns true on top<br>
>> of this patch causes multiple GPU hangs.<br>
>> ---<br>
>> src/mesa/drivers/dri/i965/brw_<wbr>fs_nir.cpp | 17 +++++++++++++++++<br>
>> src/mesa/drivers/dri/i965/brw_<wbr>vec4_nir.cpp | 21 +++++++++++++++++++++<br>
>> 2 files changed, 38 insertions(+)<br>
>><br>
>> diff --git a/src/mesa/drivers/dri/i965/<wbr>brw_fs_nir.cpp<br>
> b/src/mesa/drivers/dri/i965/<wbr>brw_fs_nir.cpp<br>
>> index 042203d..b3eec49 100644<br>
>> --- a/src/mesa/drivers/dri/i965/<wbr>brw_fs_nir.cpp<br>
>> +++ b/src/mesa/drivers/dri/i965/<wbr>brw_fs_nir.cpp<br>
>> @@ -33,6 +33,23 @@ using namespace brw::surface_access;<br>
>> void<br>
>> fs_visitor::emit_nir_code()<br>
>> {<br>
>> + if (brw_stage_has_packed_<wbr>dispatch(stage, prog_data)) {<br>
><br>
> Mind adding "0 &&" and merging this patch so we remain aware of the issue,<br>
> keep it building, and can easily test future hardware.<br>
><br>
</span>I guess that would work -- An alternative that would keep the<br>
NIR-to-i965 pass tidier would be to assert(devinfo->gen <= 9) in<br>
brw_stage_has_packed_dispatch(<wbr>) to make sure we don't forget to do a<br>
full Piglit/DEQP/CTS run with this patch applied when a new generation<br>
is powered on. I can also add a comment with a link to this patch.<br><div><div class="h5"></div></div></blockquote><div><br></div>Can we do both? Maybe something like this instead of an assert:<br><br></div><div class="gmail_quote">if (devinfo->gen > 9) {<br></div><div class="gmail_quote"> static bool warned = false;<br></div><div class="gmail_quote"> if (!warned) {<br></div><div class="gmail_quote"> fprintf(stderr, "WARNING: VMask/DMask power-of-two assumptions need to be verified. Once verified using emit_fs_mask_pow2_check(), this warning may be disabled for gen%d", devinfo->gen);<br></div><div class="gmail_quote"> warned = true;<br> }<br></div><div class="gmail_quote">}<br><br></div><div class="gmail_quote">where emit_fs_mask_pow2_check() is a helper that we put the check code into. That way it's not an assert-failure that will get instantly removed but something that will constantly bug the bring-up person until they have verified the assumption.<br><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
>> + const fs_builder ubld = bld.exec_all().group(1, 0);<br>
>> + const fs_reg tmp = component(bld.vgrf(BRW_<wbr>REGISTER_TYPE_UD), 0);<br>
>> + const fs_reg mask = (stage == MESA_SHADER_FRAGMENT ?<br>
> brw_vmask_reg() :<br>
>> + brw_dmask_reg());<br>
>> +<br>
>> + ubld.ADD(tmp, mask, brw_imm_ud(1));<br>
>> + ubld.AND(tmp, mask, tmp);<br>
>> +<br>
>> + /* This will loop forever if the dispatch mask doesn't have the<br>
> expected<br>
>> + * form '2^n-1', in which case tmp will be non-zero.<br>
>> + */<br>
>> + bld.emit(BRW_OPCODE_DO);<br>
>> + bld.CMP(bld.null_reg_ud(), tmp, brw_imm_ud(0), BRW_CONDITIONAL_NZ);<br>
>> + set_predicate(BRW_PREDICATE_<wbr>NORMAL, bld.emit(BRW_OPCODE_WHILE));<br>
>> + }<br>
>> +<br>
>> /* emit the arrays used for inputs and outputs - load/store<br>
> intrinsics will<br>
>> * be converted to reads/writes of these arrays<br>
>> */<br>
>> diff --git a/src/mesa/drivers/dri/i965/<wbr>brw_vec4_nir.cpp<br>
> b/src/mesa/drivers/dri/i965/<wbr>brw_vec4_nir.cpp<br>
>> index ba3bbdf..9f7a1f0 100644<br>
>> --- a/src/mesa/drivers/dri/i965/<wbr>brw_vec4_nir.cpp<br>
>> +++ b/src/mesa/drivers/dri/i965/<wbr>brw_vec4_nir.cpp<br>
>> @@ -35,6 +35,27 @@ namespace brw {<br>
>> void<br>
>> vec4_visitor::emit_nir_code()<br>
>> {<br>
>> + if (brw_stage_has_packed_<wbr>dispatch(stage, &prog_data->base)) {<br>
>> + const dst_reg tmp = writemask(dst_reg(this, glsl_type::uint_type),<br>
>> + WRITEMASK_X);<br>
>> + const src_reg mask =<br>
>> +<br>
> brw_swizzle(retype(stride(brw_<wbr>vec4_reg(BRW_ARCHITECTURE_<wbr>REGISTER_FILE,<br>
> BRW_ARF_STATE, 0),<br>
>> + 0, 4, 1),<br>
>> + BRW_REGISTER_TYPE_UD),<br>
>> + BRW_SWIZZLE_ZZZZ);<br>
><br>
> Can we just do vec4_reg(brw_vmask_reg)?<br>
><br>
</div></div>That didn't seem to work, because both brw_dmask_reg() and<br>
brw_vmask_reg() return a register region that is equivalent to<br>
sr0.0.xxxx in Align16 addressing mode (maybe a bug of PATCH 1? It's<br>
unlikely to matter a lot in practice though), so the Z component where<br>
the DMask is stored in is not even part of the returned Align16 region.<br></blockquote><div><br></div><div>Right. As I said in another patch, I'm not terribly concerned about vec4. We've shown it works and we won't be bringing up more vec4. I'm fine with just merging the fs version.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
>> +<br>
>> + emit(ADD(tmp, mask, brw_imm_ud(1)));<br>
>> + emit(AND(tmp, mask, src_reg(tmp)));<br>
>> +<br>
>> + /* This will loop forever if the dispatch mask doesn't have the<br>
> expected<br>
>> + * form '2^n-1', in which case tmp will be non-zero.<br>
>> + */<br>
>> + emit(BRW_OPCODE_DO);<br>
>> + emit(CMP(dst_null_ud(), src_reg(tmp), brw_imm_ud(0),<br>
>> + BRW_CONDITIONAL_NZ));<br>
>> + emit(BRW_OPCODE_WHILE)-><wbr>predicate = BRW_PREDICATE_NORMAL;<br>
>> + }<br>
>> +<br>
>> if (nir->num_uniforms > 0)<br>
>> nir_setup_uniforms();<br>
>><br>
>> --<br>
>> 2.9.0<br>
>><br>
</div></div></blockquote></div><br></div></div>