[Mesa-dev] [PATCH] i965/skl: Set the pulls bary bit in 3DSTATE_PS_EXTRA

[Ben Widawsky]

On Fri, Jul 03, 2015 at 01:15:21PM +0100, Neil Roberts wrote:
> On Gen9+ there is a new bit in 3DSTATE_PS_EXTRA that must be set if
> the shader sends a message to the pixel interpolator. This fixes the
> interpolateAt* tests on SKL, apart from interpolateatsample-nonconst
> but that is not implemented anywhere so it's not a regression.
> ---
>  src/mesa/drivers/dri/i965/brw_context.h   | 1 +
>  src/mesa/drivers/dri/i965/brw_defines.h   | 1 +
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp  | 4 ++++
>  src/mesa/drivers/dri/i965/gen8_ps_state.c | 3 +++
>  4 files changed, 9 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index 3553f6e..7596139 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -415,6 +415,7 @@ struct brw_wm_prog_data {
>     bool uses_pos_offset;
>     bool uses_omask;
>     bool uses_kill;
> +   bool pulls_bary;
>     uint32_t prog_offset_16;
>  
>     /**
> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> index 66b9abc..19489ab 100644
> --- a/src/mesa/drivers/dri/i965/brw_defines.h
> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> @@ -2145,6 +2145,7 @@ enum brw_pixel_shader_computed_depth_mode {
>  # define GEN8_PSX_SHADER_DISABLES_ALPHA_TO_COVERAGE     (1 << 7)
>  # define GEN8_PSX_SHADER_IS_PER_SAMPLE                  (1 << 6)
>  # define GEN8_PSX_SHADER_COMPUTES_STENCIL               (1 << 5)
> +# define GEN9_PSX_SHADER_PULLS_BARY                     (1 << 3)
>  # define GEN8_PSX_SHADER_HAS_UAV                        (1 << 2)
>  # define GEN8_PSX_SHADER_USES_INPUT_COVERAGE_MASK       (1 << 1)
>  
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index bd71404..3ebc3a2 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -1481,6 +1481,10 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr
>     case nir_intrinsic_interp_var_at_centroid:
>     case nir_intrinsic_interp_var_at_sample:
>     case nir_intrinsic_interp_var_at_offset: {
> +      assert(stage == MESA_SHADER_FRAGMENT);
> +
> +      ((struct brw_wm_prog_data *) prog_data)->pulls_bary = true;
> +
>        fs_reg dst_xy = bld.vgrf(BRW_REGISTER_TYPE_F, 2);
>  
>        /* For most messages, we need one reg of ignored data; the hardware
> diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> index a88f109..d544509 100644
> --- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> @@ -58,6 +58,9 @@ gen8_upload_ps_extra(struct brw_context *brw,
>     if (prog_data->uses_omask)
>        dw1 |= GEN8_PSX_OMASK_TO_RENDER_TARGET;
>  
> +   if (brw->gen >= 9 && prog_data->pulls_bary)
> +      dw1 |= GEN9_PSX_SHADER_PULLS_BARY;
> +
>     if (_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx))
>        dw1 |= GEN8_PSX_SHADER_HAS_UAV;
>  

It's unclear to me what the downside to always setting this bit would be (I
assume that's the behavior of previous gens).

I also assume this means you're abandoning the other patch, or doing it on top
of this, else you don't want to do it for the centroid case.

Reviewed-by: Ben Widawsky <ben at bwidawsk.net>