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

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> 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...
>
Meh.  I'm OK with resolving the conflicts if this patch lands first.  I
haven't merged my patch yet (even though it has your R-b) because it
depends on some other patches in the same series that haven't been
reviewed yet.

>>     /* _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.
>
Yeah.  I told him to enable this bit because it influences the
calculation of the WM_INT::ThreadDispatchEnable signal on BDW [it also
influences the cross-draw UAV coherency stuff but we don't currently
need that].  Technically atomic counters are implemented using the
hardware support for UAVs so it seems reasonable to me to set the bit.

Another possibility would be to enable Force Thread Dispatch in
3DSTATE_WM which according to the B-Spec "must always be set to Normal,
except for driver debug" -- Sounds like it may not have been properly
validated?

>>     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: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150424/0394100d/attachment-0003.sig>