[Mesa-dev] [PATCH 6/7] i965: ensure execution of fragment shader when fragment shader has atomic buffer access

[Kenneth Graunke]

On Friday, April 24, 2015 09:59:09 AM kevin.rogovin at intel.com wrote:
> From: Kevin Rogovin <kevin.rogovin at intel.com>
> 
> Ensure that the GPU spawns the fragment shader thread for those
> fragment shaders with atomic buffer access. 
> 
> ---
>  src/mesa/drivers/dri/i965/gen7_wm_state.c | 7 +++++++
>  src/mesa/drivers/dri/i965/gen8_ps_state.c | 4 ++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> index 82e116c..fa04221 100644
> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> @@ -77,6 +77,13 @@ upload_wm_state(struct brw_context *brw)
>        dw1 |= GEN7_WM_KILL_ENABLE;
>     }
>  
> +   /* pixel shader must run if it has side-effects
> +    */
> +   if (brw->ctx.Shader._CurrentFragmentProgram!=NULL &&
> +       brw->ctx.Shader._CurrentFragmentProgram->NumAtomicBuffers > 0) {
> +     dw1 |= GEN7_WM_DISPATCH_ENABLE;
> +   }
> +

Hi Kevin,

Checking brw->ctx.Shader._CurrentFragmentProgram != NULL is unnecessary.
There is always a valid pixel shader.  (If the application is using
fixed-function, we supply a fragment shader for them.)  Please drop
that check.

Also, this patch conflicts with Curro's ARB_image_load_store series - he
was also setting the UAV bits.  We'll have to sort out which should land
first.  Yours is smaller, but I think he did this in a more complete
manner...

>     /* _NEW_BUFFERS | _NEW_COLOR */
>     if (brw_color_buffer_write_enabled(brw) || writes_depth ||
>         dw1 & GEN7_WM_KILL_ENABLE) {
> diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> index 5f39e12..614bc9b 100644
> --- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> @@ -62,6 +62,10 @@ upload_ps_extra(struct brw_context *brw)
>     if (prog_data->uses_omask)
>        dw1 |= GEN8_PSX_OMASK_TO_RENDER_TARGET;
>  
> +   if (brw->ctx.Shader._CurrentFragmentProgram!=NULL &&
> +       brw->ctx.Shader._CurrentFragmentProgram->NumAtomicBuffers > 0)
> +      dw1 |= GEN8_PSX_SHADER_HAS_UAV;
> +

I thought that UAVs were essentially for Images...I'm not clear why this
is needed.  Perhaps Curro can confirm one way or another.

>     BEGIN_BATCH(2);
>     OUT_BATCH(_3DSTATE_PS_EXTRA << 16 | (2 - 2));
>     OUT_BATCH(dw1);
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150424/a7b9cf55/attachment-0003.sig>