<div dir="ltr">On 16 October 2013 15:20, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <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"><div class=""><div class="h5">On 10/16/2013 02:57 PM, Anuj Phogat wrote: > On Tue, Oct 15, 2013 at 3:48 PM, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> wrote: >> On 10/14/2013 10:12 AM, Anuj Phogat wrote: >>> - Enable GEN7_WM_MSDISPMODE_PERSAMPLE, GEN7_WM_POSOFFSET_SAMPLE, >>> GEN7_WM_OMASK_TO_RENDER_TARGET as per extension's specification. >>> - Don't enable GEN7_WM_16_DISPATCH_ENABLE when GEN7_WM_MSDISPMODE_PERSAMPLE >>> is enabled. Refer IVB PRM Vol. 2, Part 1, Page 288 for details. >>> >>> Signed-off-by: Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>> >>> --- >>> src/mesa/drivers/dri/i965/gen7_wm_state.c | 70 +++++++++++++++++++++++++++++-- >>> 1 file changed, 67 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c b/src/mesa/drivers/dri/i965/gen7_wm_state.c >>> index a2046c3..0267e0e 100644 >>> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c >>> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c >>> @@ -82,9 +82,15 @@ upload_wm_state(struct brw_context *brw) >>> GEN7_WM_BARYCENTRIC_INTERPOLATION_MODE_SHIFT; >>> >>> /* _NEW_COLOR, _NEW_MULTISAMPLE */ >>> + /* Enable if the pixel shader kernel generates and outputs oMask. >>> + */ >>> if (fp->program.UsesKill || ctx->Color.AlphaEnabled || >>> - ctx->Multisample.SampleAlphaToCoverage) >>> + ctx->Multisample.SampleAlphaToCoverage || >>> + (ctx->Multisample.SampleShading && >>> + (fp->program.Base.OutputsWritten & >>> + BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK)))) { >>> dw1 |= GEN7_WM_KILL_ENABLE; >>> + } >>> >>> /* _NEW_BUFFERS */ >>> if (brw_color_buffer_write_enabled(brw) || writes_depth || >>> @@ -97,7 +103,25 @@ upload_wm_state(struct brw_context *brw) >>> dw1 |= GEN7_WM_MSRAST_ON_PATTERN; >>> else >>> dw1 |= GEN7_WM_MSRAST_OFF_PIXEL; >>> - dw2 |= GEN7_WM_MSDISPMODE_PERPIXEL; >>> + /* From arb_sample_shading specification: >>> + * "If MULTISAMPLE or SAMPLE_SHADING_ARB is disabled, sample shading >>> + * has no effect." >>> + * >>> + * "Using gl_SampleID in a fragment shader causes the entire shader >>> + * to be evaluated per-sample." >>> + * "Using gl_SamplePosition in a fragment shader causes the entire >>> + * shader to be evaluated per-sample." >>> + * >>> + * I interprate the above four lines as enable the sample shading >>> + * if fragment shader uses gl_SampleID or gl_SamplePosition. >>> + */ >>> + if (ctx->Multisample.Enabled && >>> + (ctx->Multisample.SampleShading || >>> + brw->fragment_program->Base.InputsRead & VARYING_BIT_SAMPLE_ID || >>> + brw->fragment_program->Base.InputsRead & VARYING_BIT_SAMPLE_POS)) >>> + dw2 |= GEN7_WM_MSDISPMODE_PERSAMPLE; >>> + else >>> + dw2 |= GEN7_WM_MSDISPMODE_PERPIXEL; >>> } else { >>> dw1 |= GEN7_WM_MSRAST_OFF_PIXEL; >>> dw2 |= GEN7_WM_MSDISPMODE_PERSAMPLE; >>> @@ -129,6 +153,8 @@ upload_ps_state(struct brw_context *brw) >>> uint32_t dw2, dw4, dw5; >>> const int max_threads_shift = brw->is_haswell ? >>> HSW_PS_MAX_THREADS_SHIFT : IVB_PS_MAX_THREADS_SHIFT; >>> + bool msdispmode_persample = false; >>> + bool multisampled_fbo = ctx->DrawBuffer->Visual.samples > 1; >>> >>> /* BRW_NEW_PS_BINDING_TABLE */ >>> BEGIN_BATCH(2); >>> @@ -169,6 +195,34 @@ upload_ps_state(struct brw_context *brw) >>> if (brw->wm.prog_data->nr_params > 0) >>> dw4 |= GEN7_PS_PUSH_CONSTANT_ENABLE; >>> >>> + /* From the IVB PRM, volume 2 part 1, page 287: >>> + * "This bit is inserted in the PS payload header and made available to >>> + * the DataPort (either via the message header or via header bypass) to >>> + * indicate that oMask data (one or two phases) is included in Render >>> + * Target Write messages. If present, the oMask data is used to mask off >>> + * samples." >>> + */ >>> + if (ctx->Extensions.ARB_sample_shading && >>> + (brw->fragment_program->Base.OutputsWritten & >>> + BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK))) >>> + dw4 |= GEN7_PS_OMASK_TO_RENDER_TARGET; >>> + >>> + /* From the IVB PRM, volume 2 part 1, page 287: >>> + * "If the PS kernel does not need the Position XY Offsets to >>> + * compute a Position Value, then this field should be programmed >>> + * to POSOFFSET_NONE." >>> + * "SW Recommendation: If the PS kernel needs the Position Offsets >>> + * to compute a Position XY value, this field should match Position >>> + * ZW Interpolation Mode to ensure a consistent position.xyzw >>> + * computation." >>> + * We only require XY sample offsets. So, this recommendation doesn't >>> + * look useful at the moment. We might need this in future. >>> + */ >>> + if (brw->fragment_program->Base.InputsRead & VARYING_BIT_SAMPLE_POS) >>> + dw4 |= GEN7_PS_POSOFFSET_SAMPLE; >>> + else >>> + dw4 |= GEN7_PS_POSOFFSET_NONE; >>> + >>> /* CACHE_NEW_WM_PROG | _NEW_COLOR >>> * >>> * The hardware wedges if you have this bit set but don't turn on any dual >>> @@ -185,7 +239,17 @@ upload_ps_state(struct brw_context *brw) >>> dw4 |= GEN7_PS_ATTRIBUTE_ENABLE; >>> >>> dw4 |= GEN7_PS_8_DISPATCH_ENABLE; >>> - if (brw->wm.prog_data->prog_offset_16) >>> + msdispmode_persample = >>> + ctx->Multisample.Enabled && >>> + (ctx->Multisample.SampleShading || >>> + brw->fragment_program->Base.InputsRead & VARYING_BIT_SAMPLE_ID || >>> + brw->fragment_program->Base.InputsRead & VARYING_BIT_SAMPLE_POS); >> >> There are two copies of this expression in the Gen7 patch. You should >> probably pull this up earlier, and remove the second copy (like you did >> for the SNB code). >> > These expressions appear in different functions upload_ps_state() and > upload_wm_state(). That's the reason I couldn't do what I did for SNB. </div></div>So... this code appears in three places? Sounds like it could be a helper function. _mesa_is_sample_shading_enabled(ctx, prog). </blockquote><div> </div><div>Intel chips only support two modes: per-sample and per-fragment shading. But ARB_sample_shading is written in such a way that it permits other possibilities (for instance, some hardware might support 4x MSAA, where only 2 unique color values and sets of texture coordinates are dispatched per fragment, and that would be activated by setting MinSampleShadingARB(0.5)). In order to make this function useful for implementations that support those other possibilities, I'd recommend that the function should return the minimum number of fragment shader invocations that should occur for each fragment. That is, if the shader uses gl_SampleID or gl_SamplePosition it should return ctx->DrawBuffer->Visual.samples. Otherwise it should return this value (from the ARB_sample_shading spec): max(ceil(MIN_SAMPLE_SHADING_VALUE_ARB*SAMPLES),1) </div><div>So maybe we should call it something like _mesa_get_min_invocations_per_fragment(). </div><div>Since i965 only supports per-sample and per-fragment dispatch, i965 code can then do: </div><div> (_mesa_get_min_invocations_per_fragment(...) > 1) </div><div>to determine whether to do per-fragment or per-sample shading. </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"> <div class=""><div class="h5"> >> Otherwise, the same comments for patch 6 apply. This looks good. >> >> Patch 8 is Reviewed-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> >> >>> + /* In case of non 1x (i.e 4x, 8x) multisampling with MDISPMODE_PERSAMPLE, >>> + * only one of SIMD8 and SIMD16 should be enabled. >>> + */ >>> + if (brw->wm.prog_data->prog_offset_16 && >>> + !(multisampled_fbo && msdispmode_persample)) >>> dw4 |= GEN7_PS_16_DISPATCH_ENABLE; >>> >>> dw5 |= (brw->wm.prog_data->first_curbe_grf << >>> > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div>