<div dir="ltr">On 14 October 2013 10:12, Anuj Phogat <span dir="ltr"><<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Implement the FS backend for new builtins added by the extension:<br>
in vec2 gl_SamplePosition<br>
in int gl_SampleID<br>
in int gl_NumSamples<br>
out int gl_SampleMask[]<br></blockquote><div><br></div><div>There is a lot going on in this one patch, and it's getting hard to follow all the patch review that's going on. If you wind up re-spinning this patch series, can you please break it into four separate patches, one to add support for each builtin?<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Signed-off-by: Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</a>><br>
---<br>
src/mesa/drivers/dri/i965/brw_fs.cpp | 109 +++++++++++++++++++++++++++<br>
src/mesa/drivers/dri/i965/brw_fs.h | 4 +<br>
src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 23 ++++++<br>
src/mesa/drivers/dri/i965/brw_wm.h | 1 +<br>
4 files changed, 137 insertions(+)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp<br>
index e5d6e4b..e4f7745 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_fs.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp<br>
@@ -1115,6 +1115,109 @@ fs_visitor::emit_frontfacing_interpolation(ir_variable *ir)<br>
return reg;<br>
}<br>
<br>
+void<br>
+fs_visitor::compute_sample_position(fs_reg dst, fs_reg int_sample_pos)<br>
+{<br>
+ int num_samples = ctx->DrawBuffer->Visual.samples;<br></blockquote><div><br></div><div>This isn't safe. When compilation depends on a piece of GL state, you need to include the state in brw_wm_prog_key. Otherwise the program won't get recompiled when the value changes.<br>
<br>To avoid unnecessary recompiles, I'd recommend adding a boolean to brw_wm_prog_key which is:<br></div><div>- true if ctx->Multisample.Enabled && num_samples != 0 && (shader accesses gl_SamplePosition)<br>
</div><div>- false otherwise<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ assert(num_samples >= 0 && num_samples <= 8);<br>
+<br>
+ /* From arb_sample_shading specification:<br>
+ * "When rendering to a non-multisample buffer, or if multisample<br>
+ * rasterization is disabled, gl_SamplePosition will always be<br>
+ * (0.5, 0.5).<br>
+ */<br>
+ if (!ctx->Multisample.Enabled || num_samples == 0) {<br>
+ emit(BRW_OPCODE_MOV, dst, fs_reg(0.5f));<br></blockquote><div><br></div><div>It looks like you're using the old, more verbose style of emitting instructions. Can we convert this (and the other instructions in this patch) to the more compact style:<br>
<br></div><div>emit(MOV(dst, fs_reg(0.5f)));<br><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+ else {<br>
+ /* For num_samples = {4, 8} */<br>
+ emit(BRW_OPCODE_MOV, dst, int_sample_pos);<br>
+ emit(BRW_OPCODE_MUL, dst, dst, fs_reg(1 / 16.0f));<br></blockquote><div><br></div><div>Like Ken, I was confused as to why we needed a separate MOV before the MUL. The assertion Ken recommended would have been a useful clue, but I'd prefer to just have explicit comments explaining why we need the MOV. How about this:<br>
<br></div><div>/* Convert int_sample_pos to floating point */<br></div><div>emit(BRW_OPCODE_MOV, dst, int_sample_pos);<br></div><div>/* Scale to the range [0, 1] */<br></div><div>emit(BRW_OPCODE_MUL, dst, dst, fs_reg(1 / 16.0f));<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+}<br>
+<br>
+fs_reg *<br>
+fs_visitor::emit_samplepos_interpolation(ir_variable *ir)<br>
+{<br>
+ assert(brw->gen >= 6);<br>
+<br>
+ this->current_annotation = "compute sample position";<br>
+ fs_reg *reg = new(this->mem_ctx) fs_reg(this, ir->type);<br></blockquote><div><br></div><div>Since this code assigns to two consecutive registers, it relies on the fact that ir->type is vec2. Just to make that explicit, can we add:<br>
<br></div><div>assert(ir->type == glsl_type::vec2_type);<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ fs_reg pos = *reg;<br>
+ fs_reg int_sample_x = fs_reg(this, glsl_type::int_type);<br>
+ fs_reg int_sample_y = fs_reg(this, glsl_type::int_type);<br>
+<br>
+ /* WM will be run in MSDISPMODE_PERSAMPLE. So, only SIMD8 mode will be<br>
+ * enabled. The X, Y sample positions come in as bytes in thread payload.<br>
+ * Sample IDs and sample positions remain same for all four slots in a<br>
+ * subspan. So, read the positions using vstride=2, width=4, hstride=0.<br></blockquote><div><br></div><div>I have similar concerns to Ken about why MSDISPMODE_PERSAMPLE implies that only SIMD8 mode will be enabled. I'm assuming the two of you have adequately resolved that.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ */<br>
+ emit(BRW_OPCODE_AND, int_sample_x,<br>
+ fs_reg(stride(retype(brw_vec1_grf(c->sample_pos_reg, 0),<br>
+ BRW_REGISTER_TYPE_D), 2, 4, 0)),<br>
+ fs_reg(brw_imm_d(0xff)));<br></blockquote><div><br></div><div>If I understand correctly, this is creating the instruction<br><br></div><div>AND(8) int_sample_x<1>:d sample_pos_reg<2;4,0>:d 0x000000ff:d<br>
<br></div><div>I think this works, but it would be more straightforward to do this, which just selects the X coordinate bytes from the sample_pos_reg register:<br><br></div><div>MOV(8) int_sample_x<1>:d sample_pos_reg<16;8,2>:b<br>
<br></div><div>That would have the advantage that it doesn't rely on the fact that sample IDs and sample positions are the same for all four slots in a subspan.<br><br></div><div>(Note: there are some weird restrictions on byte operations, and I can't remember all the details. So you might need to try this in the simulator and tweak it to get it to work.) <br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ /* Compute gl_SamplePosition.x */<br>
+ compute_sample_position(pos, int_sample_x);<br>
+ pos.reg_offset++;<br></blockquote><div><br></div><div>I think if we change this to:<br><br>pos.reg_offset += dispatch_width / 8;<br><br></div><div>then the code will work correctly for both SIMD8 and SIMD16 execution modes.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ emit(BRW_OPCODE_SHR, int_sample_y,<br>
+ fs_reg(stride(retype(brw_vec1_grf(c->sample_pos_reg, 0),<br>
+ BRW_REGISTER_TYPE_D), 2, 4, 0)),<br>
+ fs_reg(8));<br>
+ emit(BRW_OPCODE_AND, int_sample_y, int_sample_y, fs_reg(brw_imm_d(0xff)));<br></blockquote><div><br></div><div>Similarly, these two instructions could be replaced with simply:<br><br></div><div>MOV(8) int_sample_y<1>:d sample_pos_reg.1<16;8,2>:b<br>
<br></div><div>Which just selects the Y coordintae bytes from the sample_pos_register.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ /* Compute gl_SamplePosition.y */<br>
+ compute_sample_position(pos, int_sample_y);<br>
+ return reg;<br>
+}<br>
+<br>
+fs_reg *<br>
+fs_visitor::emit_sampleid_interpolation(ir_variable *ir)<br>
+{<br>
+ assert(brw->gen >= 6);<br>
+ bool multisampled_fbo = ctx->DrawBuffer->Visual.samples > 1;<br></blockquote><div><br></div><div>As I mentioned in my comments on compute_sample_position(), it's not safe to consult the context like this. This information needs to go in the program key.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ this->current_annotation = "compute sample id";<br>
+ fs_reg *reg = new(this->mem_ctx) fs_reg(this, ir->type);<br>
+<br>
+ if (multisampled_fbo && ctx->Multisample.Enabled) {<br></blockquote><div><br></div><div>This too.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ fs_reg t1 = fs_reg(this, glsl_type::int_type);<br>
+ fs_reg t2 = fs_reg(this, glsl_type::int_type);<br>
+ t2.type = BRW_REGISTER_TYPE_UW;<br>
+<br>
+ /* The WM will be run in MSDISPMODE_PERSAMPLE with num_samples = 8.<br>
+ * Therefore, subspan 0 will represent sample N (where N is 0, 2, 4<br>
+ * or 6), subspan 1 will represent sample 1, 3, 5 or 7. We can find<br>
+ * the value of N by looking at R0.0 bits 7:6 ("Starting Sample Pair<br>
+ * Index (SSPI)") and multiplying by two (since samples are always<br>
+ * delivered in pairs). That is, we compute 2*((R0.0 & 0xc0) >> 6)<br>
+ * == (R0.0 & 0xc0) >> 5.<br>
+ *<br>
+ * Then we need to add N to the sequence (0, 0, 0, 0, 1, 1, 1, 1),<br>
+ * which we compute by populating a temporary variable with the<br>
+ * sequence (0, 1), and then reading from it using vstride=1,<br>
+ * width=4, hstride=0.<br>
+ * Same holds true for num_samples = 4.<br>
+ */<br>
+ emit(BRW_OPCODE_AND, t1,<br>
+ fs_reg(retype(brw_vec1_grf(0, 0), BRW_REGISTER_TYPE_D)),<br>
+ fs_reg(brw_imm_d(0xc0)));<br>
+ emit(BRW_OPCODE_SHR, t1, t1, fs_reg(5));<br>
+ emit(BRW_OPCODE_MOV, t2, fs_reg(brw_imm_v(0x11110000))); <br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ emit(BRW_OPCODE_ADD, *reg, t1, t2);<br></blockquote><div><br></div><div>I believe this works, but it's not what the comment says you're doing. It looks like you borrowed the comment from blorp, but I think that what you've actually done is actually a lot simpler (and better) than what blorp does. Rather than populate a register with (0, 1) and then reading from it using vstride=1, width=4, hstride=0, you're just populating the register with (0, 0, 0, 0, 1, 1, 1, 1) directly. Pleas update the comment accordingly.<br>
<br></div>For SIMD16 mode, we'll need to do something slightly more complicated, since we'll need t2 (which will be a pair of registers in SIMD16 mode) to contain (0, 0, 0, 0, 1, 1, 1, 1) in the first register and (2, 2, 2, 2, 3, 3, 3, 3) in the second register. I *think* you can do this with something like:<br>
<br>push_force_uncompressed();<br>emit(MOV(t2, fs_reg(brw_imm_v(0x11110000))));<br>push_force_sechalf();<br>t2.reg_offset++;<br>emit(MOV(t2, fs_reg(brw_imm_v(0x33332222))));<br>pop_force_sechalf();<br>pop_force_uncompressed();<br>
<br>But please double check in the simulator to make sure it does the right thing :) <br><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+ else {<br>
+ /* As per GL_ARB_sample_shading specification:<br>
+ * "When rendering to a non-multisample buffer, or if multisample<br>
+ * rasterization is disabled, gl_SampleID will always be zero."<br>
+ */<br>
+ emit(BRW_OPCODE_MOV, *reg, fs_reg(0));<br>
+ }<br>
+<br>
+ return reg;<br>
+}<br>
+<br>
fs_reg<br>
fs_visitor::fix_math_operand(fs_reg src)<br>
{<br>
@@ -2966,7 +3069,13 @@ fs_visitor::setup_payload_gen6()<br>
c->nr_payload_regs++;<br>
}<br>
}<br>
+<br>
/* R31: MSAA position offsets. */<br>
+ if (fp->Base.InputsRead & VARYING_BIT_SAMPLE_POS) {<br>
+ c->sample_pos_reg = c->nr_payload_regs;<br>
+ c->nr_payload_regs++;<br>
+ }<br>
+<br>
/* R32-: bary for 32-pixel. */<br>
/* R58-59: interp W for 32-pixel. */<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h<br>
index c78f9ae..812e9c7 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_fs.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_fs.h<br>
@@ -332,9 +332,12 @@ public:<br>
glsl_interp_qualifier interpolation_mode,<br>
bool is_centroid);<br>
fs_reg *emit_frontfacing_interpolation(ir_variable *ir);<br>
+ fs_reg *emit_samplepos_interpolation(ir_variable *ir);<br>
+ fs_reg *emit_sampleid_interpolation(ir_variable *ir);<br>
fs_reg *emit_general_interpolation(ir_variable *ir);<br>
void emit_interpolation_setup_gen4();<br>
void emit_interpolation_setup_gen6();<br>
+ void compute_sample_position(fs_reg dst, fs_reg int_sample_pos);<br>
fs_reg rescale_texcoord(ir_texture *ir, fs_reg coordinate,<br>
bool is_rect, int sampler, int texunit);<br>
fs_inst *emit_texture_gen4(ir_texture *ir, fs_reg dst, fs_reg coordinate,<br>
@@ -432,6 +435,7 @@ public:<br>
<br>
struct hash_table *variable_ht;<br>
fs_reg frag_depth;<br>
+ fs_reg sample_mask;<br>
fs_reg outputs[BRW_MAX_DRAW_BUFFERS];<br>
unsigned output_components[BRW_MAX_DRAW_BUFFERS];<br>
fs_reg dual_src_output;<br>
diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp<br>
index e659203..75ced1d 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp<br>
@@ -61,9 +61,14 @@ fs_visitor::visit(ir_variable *ir)<br>
reg = emit_fragcoord_interpolation(ir);<br>
} else if (!strcmp(ir->name, "gl_FrontFacing")) {<br>
reg = emit_frontfacing_interpolation(ir);<br>
+ } else if (!strcmp(ir->name, "gl_SamplePosition")) {<br>
+ reg = emit_samplepos_interpolation(ir);<br>
+ } else if (!strcmp(ir->name, "gl_SampleID")) {<br>
+ reg = emit_sampleid_interpolation(ir);<br>
} else {<br>
reg = emit_general_interpolation(ir);<br>
}<br>
+<br>
assert(reg);<br>
hash_table_insert(this->variable_ht, reg, ir);<br>
return;<br>
@@ -82,6 +87,8 @@ fs_visitor::visit(ir_variable *ir)<br>
}<br>
} else if (ir->location == FRAG_RESULT_DEPTH) {<br>
this->frag_depth = *reg;<br>
+ } else if (ir->location == FRAG_RESULT_SAMPLE_MASK) {<br>
+ this->sample_mask = *reg;<br>
} else {<br>
/* gl_FragData or a user-defined FS output */<br>
assert(ir->location >= FRAG_RESULT_DATA0 &&<br>
@@ -2502,6 +2509,22 @@ fs_visitor::emit_fb_writes()<br>
pop_force_uncompressed();<br>
}<br>
<br>
+ if (fp->Base.OutputsWritten & BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK)) {<br>
+ this->current_annotation = "FB write oMask";<br>
+ assert(this->sample_mask.file != BAD_FILE);<br>
+ fs_reg reg = fs_reg(this, glsl_type::int_type);<br>
+<br>
+ /* Hand over gl_SampleMask. Only lower 16 bits are relevant. */<br>
+ emit(AND(this->sample_mask, this->sample_mask, fs_reg(255)));<br>
+ emit(SHL(reg, this->sample_mask, fs_reg(16)));<br>
+<br>
+ this->sample_mask.type = BRW_REGISTER_TYPE_UW; <br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ reg.type = BRW_REGISTER_TYPE_UW;<br>
+ emit(OR(fs_reg(MRF, nr, BRW_REGISTER_TYPE_UW), this->sample_mask, reg));<br></blockquote><div><br></div><div>I agree with Ken that this is going to do the wrong thing. You are filling up all the data in the oMask with values, but you're not filling it with the right values. You're delivering:<br>
<br>sample_mask slot 0 -> oMask slot 0 and 1<br>sample_mask slot 1 -> oMask slot 2 and 3<br>sample_mask slot 2 -> oMask slot 4 and 5<br>sample_mask slot 3 -> oMask slot 6 and 7<br>and sample_mask slots 4-7 are getting delivered to oMask slots 8-15, which have no effect in SIMD8 mode.<br>
<br></div><div>That's definitely not what we want.<br><br>I believe that what we need can actually be done in a single instruction. For SIMD8:<br><br>MOV(8) oMask<1>:uw sample_mask<16;8,2>:uw<br></div><div>
<br></div><div>and for SIMD16:<br><br></div><div>MOV(16) oMask<1>:uw sample_mask<16;8,2>:uw<br><br></div><div>To get that to happen you'll need to create a new FS backend opcode, so that you can adjust the stride on sample_mask.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ nr += 1;<br>
+ }<br>
+<br>
/* Reserve space for color. It'll be filled in per MRT below. */<br>
int color_mrf = nr;<br>
nr += 4 * reg_width;<br>
diff --git a/src/mesa/drivers/dri/i965/brw_wm.h b/src/mesa/drivers/dri/i965/brw_wm.h<br>
index aa786de..fda74ab 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_wm.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_wm.h<br>
@@ -83,6 +83,7 @@ struct brw_wm_compile {<br>
uint8_t source_w_reg;<br>
uint8_t aa_dest_stencil_reg;<br>
uint8_t dest_depth_reg;<br>
+ uint8_t sample_pos_reg;<br>
uint8_t barycentric_coord_reg[BRW_WM_BARYCENTRIC_INTERP_MODE_COUNT];<br>
uint8_t nr_payload_regs;<br>
GLuint source_depth_to_render_target:1;<br>
<span><font color="#888888">--<br>
1.8.1.4<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div><br></div></div>