<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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">- Enable GEN6_WM_MSDISPMODE_PERSAMPLE, GEN6_WM_POSOFFSET_SAMPLE,<br>
GEN6_WM_OMASK_TO_RENDER_TARGET as per extension's specification.<br>
- Don't enable GEN6_WM_16_DISPATCH_ENABLE when GEN6_WM_MSDISPMODE_PERSAMPLE<br>
is enabled. Refer SNB PRM Vol. 2, Part 1, Page 279 for details.<br>
<br>
Signed-off-by: Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>><br>
---<br>
src/mesa/drivers/dri/i965/gen6_wm_state.c | 67 +++++++++++++++++++++++++++++--<br>
1 file changed, 64 insertions(+), 3 deletions(-)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/gen6_wm_state.c b/src/mesa/drivers/dri/i965/gen6_wm_state.c<br>
index c96a107..4bc25d6 100644<br>
--- a/src/mesa/drivers/dri/i965/gen6_wm_state.c<br>
+++ b/src/mesa/drivers/dri/i965/gen6_wm_state.c<br>
@@ -103,6 +103,7 @@ upload_wm_state(struct brw_context *brw)<br>
<br>
/* _NEW_BUFFERS */<br>
bool multisampled_fbo = ctx->DrawBuffer->Visual.samples > 1;<br>
+ bool msdispmode_persample = false;<br>
<br>
/* CACHE_NEW_WM_PROG */<br>
if (brw->wm.prog_data->nr_params == 0) {<br>
@@ -156,7 +157,17 @@ upload_wm_state(struct brw_context *brw)<br>
<br>
/* CACHE_NEW_WM_PROG */<br>
dw5 |= GEN6_WM_8_DISPATCH_ENABLE;<br>
- if (brw->wm.prog_data->prog_offset_16)<br>
+ msdispmode_persample =<br>
+ ctx->Multisample.Enabled &&<br>
+ (ctx->Multisample.SampleShading ||<br>
+ brw->fragment_program->Base.InputsRead & VARYING_BIT_SAMPLE_ID ||<br>
+ brw->fragment_program->Base.InputsRead & VARYING_BIT_SAMPLE_POS);<br></blockquote><div><br></div><div>The dependency on ctx->Multisample.SampleShading isn't quite right. You also need to account for the value of ctx->Multisample.MinSampleShadingValue. I believe the correct expression is something like:<br>
<br></div><div>(ctx->Multisample.SampleShading && ceil(ctx->Multisample.MinSampleShadingValue * ctx->DrawBuffer->Visual.samples) > 1)<br></div><div><br></div><div>This is necessary to ensure that if, for example, the buffer is 4xMSAA and the client sets MinSampleShadingARB to 0.25 or less, we still get per-fragment shading.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ /* In case of non 1x (i.e 4x, 8x) multisampling with MDISPMODE_PERSAMPLE,<br>
+ * only one of SIMD8 and SIMD16 should be enabled.<br>
+ */<br>
+ if (brw->wm.prog_data->prog_offset_16 &&<br>
+ !(multisampled_fbo && msdispmode_persample))<br>
dw5 |= GEN6_WM_16_DISPATCH_ENABLE;<br>
<br>
/* CACHE_NEW_WM_PROG | _NEW_COLOR */<br>
@@ -185,7 +196,10 @@ upload_wm_state(struct brw_context *brw)<br>
<br>
/* _NEW_COLOR, _NEW_MULTISAMPLE */<br>
if (fp->program.UsesKill || ctx->Color.AlphaEnabled ||<br>
- ctx->Multisample.SampleAlphaToCoverage)<br>
+ ctx->Multisample.SampleAlphaToCoverage ||<br>
+ (ctx->Multisample.SampleShading &&<br>
+ (fp->program.Base.OutputsWritten &<br>
+ BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK))))<br>
dw5 |= GEN6_WM_KILL_ENABLE;<br></blockquote><div><br>There's a problem here. The bspec says "This bit is required to be ENABLED in the following situations: ... The pixel shader kernel generates and outputs oMask.". That means you need to use the same boolean expression here to determine whether to turn on GEN6_WM_KILL_ENABLE as you do in patch 5 (in fs_visitor::emit_fb_writes()) to decide whether to output oMask.<br>
<br></div><div>But here you're using (ctx->Multisample.SampleShading && (fp->program.Base.OutputsWritten & BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK)))<br><br></div><div>Whereas in patch 5 you just use (fp->program.Base.OutputsWritten & BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK))<br>
<br></div><div>I believe the expression in patch 5 is the correct one, because gl_SampleMask should still be able to kill samples even if we're not using per-sample shading.<br><br></div><div>To avoid bugs like this, I recommend adding a boolean to brw_wm_prog_data to indicate whether the fragment shader outputs an oMask (let's say we call it "uses_omask"). fs_visitor::emit_fb_writes() should set it to true if it's outputting oMask. Then, here in gen6_wm_state.c, rather than having to duplicate the boolean expression (and risking bugs if you don't get it right), you can just refer brw->wm.prog_data->uses_omask. We already use a similar mechanism for keeping track of whether dual source blending is enabled (see the dual_src_blend boolean in brw_wm_prog_data). <br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
if (brw_color_buffer_write_enabled(brw) ||<br>
@@ -193,6 +207,19 @@ upload_wm_state(struct brw_context *brw)<br>
dw5 |= GEN6_WM_DISPATCH_ENABLE;<br>
}<br>
<br>
+ /* From the SNB PRM, volume 2 part 1, page 278:<br>
+ * "This bit is inserted in the PS payload header and made available to<br>
+ * the DataPort (either via the message header or via header bypass) to<br>
+ * indicate that oMask data (one or two phases) is included in Render<br>
+ * Target Write messages. If present, the oMask data is used to mask off<br>
+ * samples."<br>
+ * TODO: [DevSNB:A0] This bit must be disabled in A Step.<br>
+ */<br>
+ if (ctx->Extensions.ARB_sample_shading &&<br>
+ (brw->fragment_program->Base.OutputsWritten &<br>
+ BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK)))<br>
+ dw5 |= GEN6_WM_OMASK_TO_RENDER_TARGET;<br>
+<br></blockquote><div><br></div><div>The dependency on ctx->Extensions.ARB_sample_shading is unnecessary and confusing. The front end won't allow the shader to write to the sample mask if the extension isn't supported. And besides, since the purpose of this series is to add support for the extension, ctx->Extensions.ARB_sample_shading will always be true once patch 8 lands.<br>
<br></div><div>Also, my earlier comments about adding a boolean to brw_wm_prog_data apply here too. This really ought to change to just:<br><br>if (brw->wm.prog_data->uses_omask)<br></div><div> dw5 |= GEN6_WM_OMASK_TO_RENDER_TARGET;<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
/* CACHE_NEW_WM_PROG */<br>
dw6 |= brw->wm.prog_data->num_varying_inputs <<<br>
GEN6_WM_NUM_SF_OUTPUTS_SHIFT;<br>
@@ -202,12 +229,46 @@ upload_wm_state(struct brw_context *brw)<br>
dw6 |= GEN6_WM_MSRAST_ON_PATTERN;<br>
else<br>
dw6 |= GEN6_WM_MSRAST_OFF_PIXEL;<br>
- dw6 |= GEN6_WM_MSDISPMODE_PERPIXEL;<br>
+<br>
+ /* From arb_sample_shading specification:<br>
+ * "If MULTISAMPLE or SAMPLE_SHADING_ARB is disabled, sample shading<br>
+ * has no effect."<br>
+ *<br>
+ * "Using gl_SampleID in a fragment shader causes the entire shader<br>
+ * to be evaluated per-sample."<br>
+ * "Using gl_SamplePosition in a fragment shader causes the entire<br>
+ * shader to be evaluated per-sample."<br>
+ *<br>
+ * I interprate the above four lines as enable the sample shading<br>
+ * if fragment shader uses gl_SampleID or gl_SamplePosition.<br>
+ */<br>
+ if (msdispmode_persample)<br>
+ dw6 |= GEN6_WM_MSDISPMODE_PERSAMPLE;<br>
+ else<br>
+ dw6 |= GEN6_WM_MSDISPMODE_PERPIXEL;<br>
} else {<br>
dw6 |= GEN6_WM_MSRAST_OFF_PIXEL;<br>
dw6 |= GEN6_WM_MSDISPMODE_PERSAMPLE;<br>
}<br>
<br>
+ /* _NEW_MULTISAMPLE */<br></blockquote><div><br></div><div>Drop this comment. The code you're adding below doesn't depend on _NEW_MULTISAMPLE.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ /* From the SNB PRM, volume 2 part 1, page 281:<br>
+ * "If the PS kernel does not need the Position XY Offsets<br>
+ * to compute a Position XY value, then this field should be<br>
+ * programmed to POSOFFSET_NONE."<br>
+ *<br>
+ * "SW Recommendation: If the PS kernel needs the Position Offsets<br>
+ * to compute a Position XY value, this field should match Position<br>
+ * ZW Interpolation Mode to ensure a consistent position.xyzw<br>
+ * computation."<br>
+ * We only require XY sample offsets. So, this recommendation doesn't<br>
+ * look useful at the moment. We might need this in future.<br>
+ */<br>
+ if (brw->fragment_program->Base.InputsRead & VARYING_BIT_SAMPLE_POS)<br>
+ dw6 |= GEN6_WM_POSOFFSET_SAMPLE;<br>
+ else<br>
+ dw6 |= GEN6_WM_POSOFFSET_NONE;<br>
+<br></blockquote><div><br></div><div>This is correct, however it is risky for the same reason that the sample mask code above was risky: correct execution depends on this code using the same boolean expression to decide whether to set GEN6_WM_POSOFFSET_SAMPLE as fs_visitor::setup_payload_gen6() uses to decide whether to expect the sample position to be present in the payload.<br>
<br></div><div>I would feel much more comfortable if we did a similar thing to my "uses_omask" suggestion above. That is, add a boolean to brw_wm_prog_data (let's call it "uses_pos_offset"). In fs_visitor::setup_payload_gen6(), set the bit if MSAA position offsets need to be included in the payload. Then, here in gen6_wm_state.c, just consult the value of brw->wm.prog_data->uses_pos_offset rather than duplicating the boolean expression.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
BEGIN_BATCH(9);<br>
OUT_BATCH(_3DSTATE_WM << 16 | (9 - 2));<br>
OUT_BATCH(brw->wm.base.prog_offset);<br>
<span class=""><font color="#888888">--<br>
1.8.1.4<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">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>