[Mesa-dev] [PATCH 7/8] i965/gen7: Enable the features required for GL_ARB_sample_shading

[Kenneth Graunke]

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 <anuj.phogat at gmail.com>
> ---
>  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).

Otherwise, the same comments for patch 6 apply.  This looks good.

Patch 8 is Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +   /* 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 <<
>