On Wed, Oct 16, 2013 at 3:20 PM, Ian Romanick <i...@freedesktop.org> wrote:
> On 10/16/2013 02:57 PM, Anuj Phogat wrote:
>> On Tue, Oct 15, 2013 at 3:48 PM, Kenneth Graunke <kenn...@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.pho...@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.
>
> So... this code appears in three places? Sounds like it could be a
> helper function. _mesa_is_sample_shading_enabled(ctx, prog).
>
Yeah, making it a helper function would be nice. I'll do that.
>>> Otherwise, the same comments for patch 6 apply. This looks good.
>>>
>>> Patch 8 is Reviewed-by: Kenneth Graunke <kenn...@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 <<
>>>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
