On 2 December 2013 11:39, Francisco Jerez <curroje...@riseup.net> wrote:
> ---
> src/mesa/drivers/dri/i965/brw_defines.h | 4 +
> src/mesa/drivers/dri/i965/brw_eu.h | 25 ++++
> src/mesa/drivers/dri/i965/brw_eu_emit.c | 166
> +++++++++++++++++++++
> src/mesa/drivers/dri/i965/brw_fs.cpp | 3 +
> src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 18 +++
> .../drivers/dri/i965/brw_schedule_instructions.cpp | 3 +
> src/mesa/drivers/dri/i965/brw_shader.cpp | 2 +
> src/mesa/drivers/dri/i965/brw_vec4.cpp | 3 +
> src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 18 +++
> 9 files changed, 242 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
> b/src/mesa/drivers/dri/i965/brw_defines.h
> index 988b07e..631473a 100644
> --- a/src/mesa/drivers/dri/i965/brw_defines.h
> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> @@ -780,6 +780,10 @@ enum opcode {
> SHADER_OPCODE_UNTYPED_SURFACE_READ,
> SHADER_OPCODE_UNTYPED_SURFACE_WRITE,
>
> + SHADER_OPCODE_TYPED_ATOMIC,
> + SHADER_OPCODE_TYPED_SURFACE_READ,
> + SHADER_OPCODE_TYPED_SURFACE_WRITE,
> +
> SHADER_OPCODE_GEN4_SCRATCH_READ,
> SHADER_OPCODE_GEN4_SCRATCH_WRITE,
> SHADER_OPCODE_GEN7_SCRATCH_READ,
> diff --git a/src/mesa/drivers/dri/i965/brw_eu.h
> b/src/mesa/drivers/dri/i965/brw_eu.h
> index e17dc49..17822ce 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu.h
> +++ b/src/mesa/drivers/dri/i965/brw_eu.h
> @@ -383,6 +383,31 @@ brw_untyped_surface_write(struct brw_compile *p,
> unsigned msg_length,
> unsigned num_channels);
>
> +void
> +brw_typed_atomic(struct brw_compile *p,
> + struct brw_reg dst,
> + struct brw_reg mrf,
> + struct brw_reg surface,
> + unsigned atomic_op,
> + unsigned msg_length,
> + bool response_expected);
> +
> +void
> +brw_typed_surface_read(struct brw_compile *p,
> + struct brw_reg dst,
> + struct brw_reg mrf,
> + struct brw_reg surface,
> + unsigned msg_length,
> + unsigned num_channels);
> +
> +void
> +brw_typed_surface_write(struct brw_compile *p,
> + struct brw_reg dst,
> + struct brw_reg mrf,
> + struct brw_reg surface,
> + unsigned msg_length,
> + unsigned num_channels);
> +
> /***********************************************************************
> * brw_eu_util.c:
> */
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> index 13dd59a..772be7a 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> @@ -2753,6 +2753,172 @@ brw_untyped_surface_write(struct brw_compile *p,
> brw_send_indirect_message(p, sfid, dst, mrf, desc);
> }
>
> +static void
> +brw_set_dp_typed_atomic_message(struct brw_compile *p,
> + struct brw_instruction *insn,
> + unsigned atomic_op,
> + bool response_expected)
> +{
> + const unsigned access_mode = p->current->header.access_mode;
> + const unsigned compression_control =
> p->current->header.compression_control;
> +
> + if (p->brw->is_haswell) {
> + if (access_mode == BRW_ALIGN_1) {
> + if (compression_control == GEN6_COMPRESSION_2Q)
> + insn->bits3.ud |= 1 << 12; /* Use high 8 slots of the sample
> mask */
> +
> + insn->bits3.gen7_dp.msg_type =
> + HSW_DATAPORT_DC_PORT1_TYPED_ATOMIC_OP;
> + } else {
> + insn->bits3.gen7_dp.msg_type =
> + HSW_DATAPORT_DC_PORT1_TYPED_ATOMIC_OP_SIMD4X2;
> + }
> +
> + } else {
> + insn->bits3.gen7_dp.msg_type = GEN7_DATAPORT_RC_TYPED_ATOMIC_OP;
> +
> + if (compression_control == GEN6_COMPRESSION_2Q)
> + insn->bits3.ud |= 1 << 12; /* Use high 8 slots of the sample
> mask */
>
As in the previous patch, it would be nice to have a brief reminder of why
it is safe to use a SIMD8 operation in a SIMD4x2 shader. A similar comment
applies to brw_set_dp_typed_surface_{read,write}_message().
> + }
> +
> + if (response_expected)
> + insn->bits3.ud |= 1 << 13; /* Return data expected */
> +
> + insn->bits3.ud |= atomic_op << 8;
> +}
> +
> +void
> +brw_typed_atomic(struct brw_compile *p,
> + struct brw_reg dst,
> + struct brw_reg mrf,
> + struct brw_reg surface,
> + unsigned atomic_op,
> + unsigned msg_length,
> + bool response_expected) {
> + const unsigned sfid = (p->brw->is_haswell ?
> HSW_SFID_DATAPORT_DATA_CACHE_1 :
> + GEN6_SFID_DATAPORT_RENDER_CACHE);
> + struct brw_reg desc = retype(brw_address_reg(0), BRW_REGISTER_TYPE_UD);
> + struct brw_instruction *insn;
> +
> + insn = brw_load_indirect_message_descriptor(
> + p, desc, surface, msg_length,
> + brw_surface_payload_size(p, response_expected, p->brw->is_haswell,
> false),
> + true);
>
My concern from patch 9 about passing surface directly to
brw_load_indirect_message_descriptor() applies here as well. A similar
comment applies to brw_typed_surface_{read,write}().
> +
> + brw_set_dp_typed_atomic_message(
> + p, insn, atomic_op, response_expected);
> +
> + brw_send_indirect_message(p, sfid, dst, mrf, desc);
> +}
> +
> +static void
> +brw_set_dp_typed_surface_read_message(struct brw_compile *p,
> + struct brw_instruction *insn,
> + unsigned num_channels)
> +{
> + const unsigned access_mode = p->current->header.access_mode;
> + const unsigned compression_control =
> p->current->header.compression_control;
> +
> + if (p->brw->is_haswell) {
> + insn->bits3.gen7_dp.msg_type =
> HSW_DATAPORT_DC_PORT1_TYPED_SURFACE_READ;
> +
> + if (access_mode == BRW_ALIGN_1) {
> + if (compression_control == GEN6_COMPRESSION_2Q)
> + insn->bits3.ud |= 2 << 12; /* Use high 8 slots of the sample
> mask */
> + else
> + insn->bits3.ud |= 1 << 12; /* Use low 8 slots of the sample
> mask */
> + }
> +
> + } else {
> + insn->bits3.gen7_dp.msg_type = GEN7_DATAPORT_RC_TYPED_SURFACE_READ;
> +
> + if (access_mode == BRW_ALIGN_1) {
> + if (compression_control == GEN6_COMPRESSION_2Q)
> + insn->bits3.ud |= 1 << 13; /* Use high 8 slots of the sample
> mask */
> + }
> + }
> +
> + /* Set mask of unused channels. */
> + insn->bits3.ud |= (0xf & (0xf << num_channels)) << 8;
> +}
> +
> +void
> +brw_typed_surface_read(struct brw_compile *p,
> + struct brw_reg dst,
> + struct brw_reg mrf,
> + struct brw_reg surface,
> + unsigned msg_length,
> + unsigned num_channels)
> +{
> + const unsigned sfid = (p->brw->is_haswell ?
> HSW_SFID_DATAPORT_DATA_CACHE_1 :
> + GEN6_SFID_DATAPORT_RENDER_CACHE);
> + struct brw_reg desc = retype(brw_address_reg(0), BRW_REGISTER_TYPE_UD);
> + struct brw_instruction *insn;
> +
> + insn = brw_load_indirect_message_descriptor(
> + p, desc, surface, msg_length,
> + brw_surface_payload_size(p, num_channels, p->brw->is_haswell,
> false),
> + true);
> +
> + brw_set_dp_typed_surface_read_message(
> + p, insn, num_channels);
> +
> + brw_send_indirect_message(p, sfid, dst, mrf, desc);
> +}
> +
> +static void
> +brw_set_dp_typed_surface_write_message(struct brw_compile *p,
> + struct brw_instruction *insn,
> + unsigned num_channels)
> +{
> + const unsigned access_mode = p->current->header.access_mode;
> + const unsigned compression_control =
> p->current->header.compression_control;
> +
> + if (p->brw->is_haswell) {
> + insn->bits3.gen7_dp.msg_type =
> HSW_DATAPORT_DC_PORT1_TYPED_SURFACE_WRITE;
> +
> + if (access_mode == BRW_ALIGN_1) {
> + if (compression_control == GEN6_COMPRESSION_2Q)
> + insn->bits3.ud |= 2 << 12; /* Use high 8 slots of the sample
> mask */
> + else
> + insn->bits3.ud |= 1 << 12; /* Use low 8 slots of the sample
> mask */
> + }
> +
> + } else {
> + insn->bits3.gen7_dp.msg_type = GEN7_DATAPORT_RC_TYPED_SURFACE_WRITE;
> +
> + if (access_mode == BRW_ALIGN_1) {
> + if (compression_control == GEN6_COMPRESSION_2Q)
> + insn->bits3.ud |= 1 << 13; /* Use high 8 slots of the sample
> mask */
> + }
> + }
> +
> + /* Set mask of unused channels. */
> + insn->bits3.ud |= (0xf & (0xf << num_channels)) << 8;
> +}
> +
> +void
> +brw_typed_surface_write(struct brw_compile *p,
> + struct brw_reg dst,
> + struct brw_reg mrf,
> + struct brw_reg surface,
> + unsigned msg_length,
> + unsigned num_channels)
> +{
> + const unsigned sfid = (p->brw->is_haswell ?
> HSW_SFID_DATAPORT_DATA_CACHE_1 :
> + GEN6_SFID_DATAPORT_RENDER_CACHE);
> + struct brw_reg desc = retype(brw_address_reg(0), BRW_REGISTER_TYPE_UD);
> + struct brw_instruction *insn;
> +
> + insn = brw_load_indirect_message_descriptor(
> + p, desc, surface, msg_length, 0, true);
> +
> + brw_set_dp_typed_surface_write_message(
> + p, insn, num_channels);
> +
> + brw_send_indirect_message(p, sfid, dst, mrf, desc);
> +}
> +
> /**
> * This instruction is generated as a single-channel align1 instruction by
> * both the VS and FS stages when using INTEL_DEBUG=shader_time.
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 721162f..20cb4b9 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -782,6 +782,9 @@ fs_visitor::implied_mrf_writes(fs_inst *inst)
> case SHADER_OPCODE_UNTYPED_ATOMIC:
> case SHADER_OPCODE_UNTYPED_SURFACE_READ:
> case SHADER_OPCODE_UNTYPED_SURFACE_WRITE:
> + case SHADER_OPCODE_TYPED_ATOMIC:
> + case SHADER_OPCODE_TYPED_SURFACE_READ:
> + case SHADER_OPCODE_TYPED_SURFACE_WRITE:
> return 0;
> default:
> assert(!"not reached");
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> index 2ebb90a..9601183 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> @@ -1696,6 +1696,24 @@ fs_generator::generate_code(exec_list *instructions)
> src[0], inst->mlen, src[1].dw1.ud);
> break;
>
> + case SHADER_OPCODE_TYPED_ATOMIC:
> + assert(src[1].file == BRW_IMMEDIATE_VALUE);
> + brw_typed_atomic(p, dst, brw_message_reg(inst->base_mrf),
> + src[0], src[1].dw1.ud, inst->mlen, true);
> + break;
> +
> + case SHADER_OPCODE_TYPED_SURFACE_READ:
> + assert(src[1].file == BRW_IMMEDIATE_VALUE);
> + brw_typed_surface_read(p, dst, brw_message_reg(inst->base_mrf),
> + src[0], inst->mlen, src[1].dw1.ud);
> + break;
> +
> + case SHADER_OPCODE_TYPED_SURFACE_WRITE:
> + assert(src[1].file == BRW_IMMEDIATE_VALUE);
> + brw_typed_surface_write(p, dst, brw_message_reg(inst->base_mrf),
> + src[0], inst->mlen, src[1].dw1.ud);
> + break;
> +
>
As in the previous patch, I'm concerned that brw_mark_surface_used() isn't
being called. Same comment applies to brw_vec4_generator.cpp.
> case FS_OPCODE_SET_SIMD4X2_OFFSET:
> generate_set_simd4x2_offset(inst, dst, src[0]);
> break;
> diff --git a/src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp
> b/src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp
> index 39b63bc..0e4d8f1 100644
> --- a/src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp
> @@ -338,6 +338,7 @@ schedule_node::set_latency_gen7(bool is_haswell)
> break;
>
> case SHADER_OPCODE_UNTYPED_ATOMIC:
> + case SHADER_OPCODE_TYPED_ATOMIC:
> /* Test code:
> * mov(8) g112<1>ud 0x00000000ud { align1 WE_all
> 1Q };
> * mov(1) g112.7<1>ud g1.7<0,1,0>ud { align1 WE_all };
> @@ -357,6 +358,8 @@ schedule_node::set_latency_gen7(bool is_haswell)
>
> case SHADER_OPCODE_UNTYPED_SURFACE_READ:
> case SHADER_OPCODE_UNTYPED_SURFACE_WRITE:
> + case SHADER_OPCODE_TYPED_SURFACE_READ:
> + case SHADER_OPCODE_TYPED_SURFACE_WRITE:
> /* Test code:
> * mov(8) g112<1>UD 0x00000000UD { align1 WE_all
> 1Q };
> * mov(1) g112.7<1>UD g1.7<0,1,0>UD { align1 WE_all };
>
As in the previous patch, I don't think writes have the same latency as
reads.
With those issues addressed, the patch is:
Reviewed-by: Paul Berry <stereotype...@gmail.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
