[Mesa-dev] [PATCH V2 11/12] i965/gen7: Enable the features required for GL_ARB_sample_shading
Paul Berry
stereotype441 at gmail.com
Tue Oct 29 02:48:51 CET 2013
On 25 October 2013 16:45, Anuj Phogat <anuj.phogat at 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.phogat at 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). 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.
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.
With that change, this patch is:
Reviewed-by: Paul Berry <stereotype441 at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131028/74cd37de/attachment.html>
More information about the mesa-dev
mailing list