[Mesa-dev,11/14] i965/gen7: Enable fragment shader dispatch if the program has image uniforms.

Submitted by Francisco Jerez on Feb. 6, 2015, 5:23 p.m.

Details

Message ID 1423243408-24744-11-git-send-email-currojerez@riseup.net
State New
Headers show

Not browsing as part of any series.

Commit Message

Francisco Jerez Feb. 6, 2015, 5:23 p.m.
Shaders with image uniforms may have side effects.  Make sure that
fragment shader threads are dispatched if the shader has any image
uniforms.
---
 src/mesa/drivers/dri/i965/gen7_wm_state.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c b/src/mesa/drivers/dri/i965/gen7_wm_state.c
index 923414e..034ce08 100644
--- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
@@ -39,6 +39,9 @@  upload_wm_state(struct brw_context *brw)
    /* BRW_NEW_FRAGMENT_PROGRAM */
    const struct brw_fragment_program *fp =
       brw_fragment_program_const(brw->fragment_program);
+   struct gl_shader_program *prog = ctx->Shader._CurrentFragmentProgram;
+   struct gl_shader *shader =
+      prog ? prog->_LinkedShaders[MESA_SHADER_FRAGMENT] : NULL;
    /* BRW_NEW_FS_PROG_DATA */
    const struct brw_wm_prog_data *prog_data = brw->wm.prog_data;
    bool writes_depth = prog_data->computed_depth_mode != BRW_PSCDEPTH_OFF;
@@ -76,8 +79,9 @@  upload_wm_state(struct brw_context *brw)
       dw1 |= GEN7_WM_KILL_ENABLE;
    }
 
-   /* _NEW_BUFFERS | _NEW_COLOR */
+   /* _NEW_BUFFERS | _NEW_COLOR, BRW_NEW_FRAGMENT_PROGRAM */
    if (brw_color_buffer_write_enabled(brw) || writes_depth ||
+       (shader && shader->NumImages) ||
        dw1 & GEN7_WM_KILL_ENABLE) {
       dw1 |= GEN7_WM_DISPATCH_ENABLE;
    }

Comments

On Friday, February 06, 2015 07:23:25 PM Francisco Jerez wrote:
> Shaders with image uniforms may have side effects.  Make sure that
> fragment shader threads are dispatched if the shader has any image
> uniforms.
> ---
>  src/mesa/drivers/dri/i965/gen7_wm_state.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> index 923414e..034ce08 100644
> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> @@ -39,6 +39,9 @@ upload_wm_state(struct brw_context *brw)
>     /* BRW_NEW_FRAGMENT_PROGRAM */
>     const struct brw_fragment_program *fp =
>        brw_fragment_program_const(brw->fragment_program);
> +   struct gl_shader_program *prog = ctx->Shader._CurrentFragmentProgram;
> +   struct gl_shader *shader =
> +      prog ? prog->_LinkedShaders[MESA_SHADER_FRAGMENT] : NULL;
>     /* BRW_NEW_FS_PROG_DATA */
>     const struct brw_wm_prog_data *prog_data = brw->wm.prog_data;
>     bool writes_depth = prog_data->computed_depth_mode != BRW_PSCDEPTH_OFF;
> @@ -76,8 +79,9 @@ upload_wm_state(struct brw_context *brw)
>        dw1 |= GEN7_WM_KILL_ENABLE;
>     }
>  
> -   /* _NEW_BUFFERS | _NEW_COLOR */
> +   /* _NEW_BUFFERS | _NEW_COLOR, BRW_NEW_FRAGMENT_PROGRAM */
>     if (brw_color_buffer_write_enabled(brw) || writes_depth ||
> +       (shader && shader->NumImages) ||
>         dw1 & GEN7_WM_KILL_ENABLE) {
>        dw1 |= GEN7_WM_DISPATCH_ENABLE;
>     }
> 

Do you need to make a similar change in gen8_ps_state.c?  I suppose it
could automatically get enabled when UAVs are on, but I haven't checked.

I've been trying to migrate the state upload code to exclusively rely on
prog_data, and not poke at the core Mesa shader structures directly
(and only listen to BRW_NEW_FS_PROG_DATA, not BRW_NEW_FRAGMENT_PROGRAM).

What do you think about adding a "uses_images" or "has_uav" flag to
prog_data and using it here?

While I think I'd prefer that, this should also work, so it gets:
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Kenneth Graunke <kenneth@whitecape.org> writes:

> On Friday, February 06, 2015 07:23:25 PM Francisco Jerez wrote:
>> Shaders with image uniforms may have side effects.  Make sure that
>> fragment shader threads are dispatched if the shader has any image
>> uniforms.
>> ---
>>  src/mesa/drivers/dri/i965/gen7_wm_state.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c b/src/mesa/drivers/dri/i965/gen7_wm_state.c
>> index 923414e..034ce08 100644
>> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
>> @@ -39,6 +39,9 @@ upload_wm_state(struct brw_context *brw)
>>     /* BRW_NEW_FRAGMENT_PROGRAM */
>>     const struct brw_fragment_program *fp =
>>        brw_fragment_program_const(brw->fragment_program);
>> +   struct gl_shader_program *prog = ctx->Shader._CurrentFragmentProgram;
>> +   struct gl_shader *shader =
>> +      prog ? prog->_LinkedShaders[MESA_SHADER_FRAGMENT] : NULL;
>>     /* BRW_NEW_FS_PROG_DATA */
>>     const struct brw_wm_prog_data *prog_data = brw->wm.prog_data;
>>     bool writes_depth = prog_data->computed_depth_mode != BRW_PSCDEPTH_OFF;
>> @@ -76,8 +79,9 @@ upload_wm_state(struct brw_context *brw)
>>        dw1 |= GEN7_WM_KILL_ENABLE;
>>     }
>>  
>> -   /* _NEW_BUFFERS | _NEW_COLOR */
>> +   /* _NEW_BUFFERS | _NEW_COLOR, BRW_NEW_FRAGMENT_PROGRAM */
>>     if (brw_color_buffer_write_enabled(brw) || writes_depth ||
>> +       (shader && shader->NumImages) ||
>>         dw1 & GEN7_WM_KILL_ENABLE) {
>>        dw1 |= GEN7_WM_DISPATCH_ENABLE;
>>     }
>> 
>
> Do you need to make a similar change in gen8_ps_state.c?  I suppose it
> could automatically get enabled when UAVs are on, but I haven't checked.
>

Nope, it's not necessary.  There is no dispatch enable bit on Gen8,
instead it's automatically calculated based on a number of things
including the UAV access bits I'm setting in PATCH 12.

> I've been trying to migrate the state upload code to exclusively rely on
> prog_data, and not poke at the core Mesa shader structures directly
> (and only listen to BRW_NEW_FS_PROG_DATA, not BRW_NEW_FRAGMENT_PROGRAM).
>
Okay, I'll fix that.

> What do you think about adding a "uses_images" or "has_uav" flag to
> prog_data and using it here?
>
I think we could just check prog_data->nr_image_params which is already
there.

> While I think I'd prefer that, this should also work, so it gets:
> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

Thanks!