On Mon, Oct 28, 2013 at 6:48 PM, Paul Berry <stereotype...@gmail.com> wrote:
> On 25 October 2013 16:45, Anuj Phogat <anuj.pho...@gmail.com> wrote:
>
>> - Enable GEN7_WM_MSDISPMODE_PERSAMPLE, GEN7_WM_POSOFFSET_SAMPLE,
>> GEN7_WM_OMASK_TO_RENDER_TARGET as per extension's specification.
>> - Only enable one of GEN7_WM_8_DISPATCH_ENABLE or
>> GEN7_WM_16_DISPATCH_ENABLE
>> when GEN7_WM_MSDISPMODE_PERSAMPLE is enabled. Refer IVB PRM Vol. 2,
>> Part 1,
>> Page 288 for details.
>>
>> V2: - Add a comment explaining why only SIMD8 mode is enabled with
>> MSDISPMODE_PERSAMPLE.
>> - Use shared function _mesa_get_min_invocations_per_fragment().
>> - Use brw_wm_prog_data variables: uses_pos_offset, uses_omask.
>> Signed-off-by: Anuj Phogat <anuj.pho...@gmail.com>
>> ---
>> src/mesa/drivers/dri/i965/gen7_wm_state.c | 53
>> +++++++++++++++++++++++++++++--
>> 1 file changed, 50 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..493b107 100644
>> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
>> @@ -27,6 +27,7 @@
>> #include "brw_defines.h"
>> #include "brw_util.h"
>> #include "brw_wm.h"
>> +#include "program/program.h"
>> #include "program/prog_parameter.h"
>> #include "program/prog_statevars.h"
>> #include "intel_batchbuffer.h"
>> @@ -82,9 +83,13 @@ 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 ||
>> + brw->wm.prog_data->uses_omask) {
>> dw1 |= GEN7_WM_KILL_ENABLE;
>> + }
>>
>> /* _NEW_BUFFERS */
>> if (brw_color_buffer_write_enabled(brw) || writes_depth ||
>> @@ -97,7 +102,11 @@ 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;
>> +
>> + if (_mesa_get_min_invocations_per_fragment(ctx,
>> brw->fragment_program) > 1)
>> + dw2 |= GEN7_WM_MSDISPMODE_PERSAMPLE;
>> + else
>> + dw2 |= GEN7_WM_MSDISPMODE_PERPIXEL;
>> } else {
>> dw1 |= GEN7_WM_MSRAST_OFF_PIXEL;
>> dw2 |= GEN7_WM_MSDISPMODE_PERSAMPLE;
>> @@ -169,6 +178,32 @@ 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 (brw->wm.prog_data->uses_omask)
>> + 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->wm.prog_data->uses_pos_offset)
>> + 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
>> @@ -184,8 +219,20 @@ upload_ps_state(struct brw_context *brw)
>> if (brw->wm.prog_data->num_varying_inputs != 0)
>> dw4 |= GEN7_PS_ATTRIBUTE_ENABLE;
>>
>> + /* In case of non 1x (i.e 4x, 8x) multisampling with
>> MSDISPMODE_PERSAMPLE,
>> + * only one of SIMD8 and SIMD16 should be enabled. So, we have two
>> options
>> + * in that case:
>> + * 'SIMD8 only' dispatch: allowed on gen7.
>> + * 'SIMD16 only' dispatch: allowed on gen7 except when in PERSAMPLE
>> mode
>> + * with number of multisamples >= 8.
>> + * TODO: Currently we enable 'SIMD8 only' dispatch in above mentioned
>> case.
>> + * Make changes to allow 'SIMD16 only' dispatch for
>> multisamples < 8.
>>
>
> I believe this is incorrect (see my comment on patch 10/12). On Gen7, we
> can enable SIMD8 or SIMD16 or both. (I'm fairly certain about this because
> blorp enables SIMD16 mode with 8x PERSAMPLE dispatch, and it's been working
> fine since May 2012).
>
We had this discussion on my earlier patch for Gen6. We reached a
conclusion that on Gen7 with non 1x persample, we can enable either SIMD8
or SIMD16.
The only reason we are not enabling SIMD16 yet is because you had some
> concerns that the code generation for SIMD16 might not be correct.
>
Right. I am also getting GPU hangs with SIMD16 only dispatch. I'll look in
to it after pushing this series.
>
> I think the right way to deal with this is to drop this coment (and the
> remainder of the patch), and instead, disable SIMD16 in the same way we do
> for the other features that we don't have SIMD16 support for yet: by
> calling fail() in brw_fs_visitor.cpp.
>
> That way (a) we won't generate a 16-wide shader that will never get used,
> and (b) later, when we go looking for reasons why SIMD16 shaders aren't
> getting generated, we will only have to consider one reason (shaders
> failing SIMD16 compilation) instead of two.
>
sounds better. I'll call fail() if (dispatch_width == 16 &&
_mesa_get_min_invocations_per_fragment(ctx, fp) > 1)
>
> With that change, this patch is:
>
> Reviewed-by: Paul Berry <stereotype...@gmail.com>
>
>
>> + */
>> dw4 |= GEN7_PS_8_DISPATCH_ENABLE;
>> - if (brw->wm.prog_data->prog_offset_16)
>> +
>> + if (brw->wm.prog_data->prog_offset_16 &&
>> + !(_mesa_get_min_invocations_per_fragment(ctx,
>> + brw->fragment_program )
>> > 1))
>> dw4 |= GEN7_PS_16_DISPATCH_ENABLE;
>>
>> dw5 |= (brw->wm.prog_data->first_curbe_grf <<
>> --
>> 1.8.1.4
>>
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
