[Mesa-dev] [PATCH 26/38] i965/fs: Migrate FS framebuffer writes to the IR builder.
Francisco Jerez
currojerez at riseup.net
Fri Jun 5 16:47:41 PDT 2015
Matt Turner <mattst88 at gmail.com> writes:
> On Thu, Jun 4, 2015 at 9:05 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> ---
>> src/mesa/drivers/dri/i965/brw_fs.cpp | 9 ++---
>> src/mesa/drivers/dri/i965/brw_fs.h | 3 +-
>> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 57 ++++++++++++++--------------
>> 3 files changed, 35 insertions(+), 34 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index e55998c..4c1aa60 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -2918,13 +2918,12 @@ fs_visitor::emit_repclear_shader()
>> int base_mrf = 1;
>> int color_mrf = base_mrf + 2;
>>
>> - fs_inst *mov = emit(MOV(vec4(brw_message_reg(color_mrf)),
>> - fs_reg(UNIFORM, 0, BRW_REGISTER_TYPE_F)));
>> - mov->force_writemask_all = true;
>> + fs_inst *mov = bld.exec_all().MOV(vec4(brw_message_reg(color_mrf)),
>> + fs_reg(UNIFORM, 0, BRW_REGISTER_TYPE_F));
>>
>> fs_inst *write;
>> if (key->nr_color_regions == 1) {
>> - write = emit(FS_OPCODE_REP_FB_WRITE);
>> + write = bld.emit(FS_OPCODE_REP_FB_WRITE);
>> write->saturate = key->clamp_fragment_color;
>> write->base_mrf = color_mrf;
>> write->target = 0;
>> @@ -2933,7 +2932,7 @@ fs_visitor::emit_repclear_shader()
>> } else {
>> assume(key->nr_color_regions > 0);
>> for (int i = 0; i < key->nr_color_regions; ++i) {
>> - write = emit(FS_OPCODE_REP_FB_WRITE);
>> + write = bld.emit(FS_OPCODE_REP_FB_WRITE);
>> write->saturate = key->clamp_fragment_color;
>> write->base_mrf = base_mrf;
>> write->target = i;
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
>> index de37298..571d411 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>> @@ -330,7 +330,8 @@ public:
>> void setup_color_payload(fs_reg *dst, fs_reg color, unsigned components,
>> unsigned exec_size, bool use_2nd_half);
>> void emit_alpha_test();
>> - fs_inst *emit_single_fb_write(fs_reg color1, fs_reg color2,
>> + fs_inst *emit_single_fb_write(const brw::fs_builder &bld,
>> + fs_reg color1, fs_reg color2,
>> fs_reg src0_alpha, unsigned components,
>> unsigned exec_size, bool use_2nd_half = false);
>> void emit_fb_writes();
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> index 5d6d6d6..98429fa 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> @@ -1296,12 +1296,12 @@ fs_visitor::emit_dummy_fs()
>> /* Everyone's favorite color. */
>> const float color[4] = { 1.0, 0.0, 1.0, 0.0 };
>> for (int i = 0; i < 4; i++) {
>> - emit(MOV(fs_reg(MRF, 2 + i * reg_width, BRW_REGISTER_TYPE_F,
>> - dispatch_width), fs_reg(color[i])));
>> + bld.MOV(fs_reg(MRF, 2 + i * reg_width, BRW_REGISTER_TYPE_F,
>> + dispatch_width), fs_reg(color[i]));
>> }
>>
>> fs_inst *write;
>> - write = emit(FS_OPCODE_FB_WRITE);
>> + write = bld.emit(FS_OPCODE_FB_WRITE);
>> write->eot = true;
>> if (devinfo->gen >= 6) {
>> write->base_mrf = 2;
>> @@ -1479,7 +1479,7 @@ fs_visitor::setup_color_payload(fs_reg *dst, fs_reg color, unsigned components,
>> fs_reg tmp = vgrf(glsl_type::vec4_type);
>> assert(color.type == BRW_REGISTER_TYPE_F);
>> for (unsigned i = 0; i < components; i++) {
>> - inst = emit(MOV(offset(tmp, i), offset(color, i)));
>> + inst = bld.MOV(offset(tmp, i), offset(color, i));
>> inst->saturate = true;
>> }
>> color = tmp;
>> @@ -1550,15 +1550,14 @@ fs_visitor::emit_alpha_test()
>> }
>>
>> fs_inst *
>> -fs_visitor::emit_single_fb_write(fs_reg color0, fs_reg color1,
>> +fs_visitor::emit_single_fb_write(const fs_builder &bld,
>> + fs_reg color0, fs_reg color1,
>> fs_reg src0_alpha, unsigned components,
>> unsigned exec_size, bool use_2nd_half)
>> {
>> assert(stage == MESA_SHADER_FRAGMENT);
>> brw_wm_prog_data *prog_data = (brw_wm_prog_data*) this->prog_data;
>> brw_wm_prog_key *key = (brw_wm_prog_key*) this->key;
>> -
>> - this->current_annotation = "FB write header";
>> int header_size = 2, payload_header_size;
>>
>> /* We can potentially have a message length of up to 15, so we have to set
>> @@ -1589,22 +1588,23 @@ fs_visitor::emit_single_fb_write(fs_reg color0, fs_reg color1,
>>
>> if (payload.aa_dest_stencil_reg) {
>> sources[length] = fs_reg(GRF, alloc.allocate(1));
>> - emit(MOV(sources[length],
>> - fs_reg(brw_vec8_grf(payload.aa_dest_stencil_reg, 0))));
>> + bld.exec_all().annotate("FB write stencil/AA alpha")
>> + .MOV(sources[length],
>> + fs_reg(brw_vec8_grf(payload.aa_dest_stencil_reg, 0)));
>> length++;
>> }
>>
>> prog_data->uses_omask =
>> prog->OutputsWritten & BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK);
>> if (prog_data->uses_omask) {
>> - this->current_annotation = "FB write oMask";
>> assert(this->sample_mask.file != BAD_FILE);
>> /* Hand over gl_SampleMask. Only lower 16 bits are relevant. Since
>> * it's unsinged single words, one vgrf is always 16-wide.
>> */
>> sources[length] = fs_reg(GRF, alloc.allocate(1),
>> BRW_REGISTER_TYPE_UW, 16);
>> - emit(FS_OPCODE_SET_OMASK, sources[length], this->sample_mask);
>> + bld.exec_all().annotate("FB write oMask")
>> + .emit(FS_OPCODE_SET_OMASK, sources[length], this->sample_mask);
>> length++;
>> }
>>
>> @@ -1665,20 +1665,21 @@ fs_visitor::emit_single_fb_write(fs_reg color0, fs_reg color1,
>> if (payload.dest_depth_reg)
>> sources[length++] = fs_reg(brw_vec8_grf(payload.dest_depth_reg, 0));
>>
>> + const fs_builder ubld = bld.group(exec_size, use_2nd_half);
>> fs_inst *load;
>> fs_inst *write;
>> if (devinfo->gen >= 7) {
>> /* Send from the GRF */
>> fs_reg payload = fs_reg(GRF, -1, BRW_REGISTER_TYPE_F, exec_size);
>> - load = emit(LOAD_PAYLOAD(payload, sources, length, payload_header_size));
>> + load = ubld.LOAD_PAYLOAD(payload, sources, length, payload_header_size);
>
> I don't see that the LOAD_PAYLOAD had force_sechalf set before. Same
> comment elsewhere in this patch.
>
> What's going on? If this is a functional change... you need to write a
> commit message!
It's neither an accident nor a functional change. As I mentioned in the
commit message of PATCH 14, the builder requires you to be explicit
about the subset of channel enables you want to affect your instructions
(or call exec_all() if you don't want any at all) if you are planning to
emit any instruction of non-native SIMD width. Otherwise the assertion
in fs_builder::emit() fires. This is not a functional change because we
don't emit the FB writes under non-uniform control flow (we can't
because of EOT), and because the kill-pixel mask is always sent as a
header rather than via predication on HSW and up for dual-source writes,
so always using the first quarter was probably okay, by pure luck.
I can repeat basically the same justification as in PATCH 14 in the
commit message of this patch, if you'd like it to be more clear.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150606/51e14966/attachment.sig>
More information about the mesa-dev
mailing list