[Mesa-dev] [PATCH v3 2/3] i965: Ensure FS execution in presence of atomic buffers

[Francisco Jerez]

Iago Toral Quiroga <itoral at igalia.com> writes:

> On Haswell we need to set the UAV_ONLY WM state bit when there are no colour
> or depth buffer writes and on all hardware we should set the early
> depth/stencil control field to PSEXEC unless early fragment tests are enabled
> to make sure that the fragment shader is executed regardless of whether
> per-fragment tests pass or not as the spec requires.
>
> So far we have been doing this for images only, but we should apply the same
> treatment to all side effectful scenarios. Suggested by Curro.

I think it would be useful to clarify here that this is not strictly
required for compliance with the original ARB_shader_atomic_counters
extension, it's only necessary to get the execution semantics specified
in GL4.2+ right.

> ---
>  src/mesa/drivers/dri/i965/gen7_wm_state.c | 9 +++++----
>  src/mesa/drivers/dri/i965/gen8_ps_state.c | 2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> index a6d1028..74a2cf3 100644
> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> @@ -78,9 +78,10 @@ upload_wm_state(struct brw_context *brw)
>     }
>  
>     /* _NEW_BUFFERS | _NEW_COLOR */
> +   bool active_fs_has_side_effects =

If you mark this const and change the commit message as suggested above
the series is:

Reviewed-by: Francisco Jerez <currojerez at riseup.net>

> +      _mesa_active_fragment_shader_has_side_effects(&brw->ctx);
>     if (brw_color_buffer_write_enabled(brw) || writes_depth ||
> -       _mesa_active_fragment_shader_has_side_effects(&brw->ctx) ||
> -       dw1 & GEN7_WM_KILL_ENABLE) {
> +       active_fs_has_side_effects || dw1 & GEN7_WM_KILL_ENABLE) {
>        dw1 |= GEN7_WM_DISPATCH_ENABLE;
>     }
>     if (multisampled_fbo) {
> @@ -106,7 +107,7 @@ upload_wm_state(struct brw_context *brw)
>     /* BRW_NEW_FS_PROG_DATA */
>     if (prog_data->early_fragment_tests)
>        dw1 |= GEN7_WM_EARLY_DS_CONTROL_PREPS;
> -   else if (prog_data->base.nr_image_params)
> +   else if (active_fs_has_side_effects)
>        dw1 |= GEN7_WM_EARLY_DS_CONTROL_PSEXEC;
>  
>     /* The "UAV access enable" bits are unnecessary on HSW because they only
> @@ -119,7 +120,7 @@ upload_wm_state(struct brw_context *brw)
>      */
>     if (brw->is_haswell &&
>         !(brw_color_buffer_write_enabled(brw) || writes_depth) &&
> -       prog_data->base.nr_image_params)
> +       active_fs_has_side_effects)
>        dw2 |= HSW_WM_UAV_ONLY;
>  
>     BEGIN_BATCH(3);
> diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> index 3cc8c68..74cdcef 100644
> --- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> @@ -156,7 +156,7 @@ upload_wm_state(struct brw_context *brw)
>     /* BRW_NEW_FS_PROG_DATA */
>     if (brw->wm.prog_data->early_fragment_tests)
>        dw1 |= GEN7_WM_EARLY_DS_CONTROL_PREPS;
> -   else if (brw->wm.prog_data->base.nr_image_params)
> +   else if (_mesa_active_fragment_shader_has_side_effects(&brw->ctx))
>        dw1 |= GEN7_WM_EARLY_DS_CONTROL_PSEXEC;
>  
>     BEGIN_BATCH(2);
> -- 
> 1.9.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151221/c11c37d0/attachment-0001.sig>