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

[Anuj Phogat]

On Tue, Oct 15, 2013 at 3:48 PM, Kenneth Graunke <kenneth at whitecape.org> 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 <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).
>
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.

> 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 <<
>>