[Mesa-dev,2/3] i965/fs: do not disable the FS unit in the presence of shader storage

Submitted by Iago Toral Quiroga on Dec. 15, 2015, 11:51 a.m.

Details

Message ID 1450180308-20143-2-git-send-email-itoral@igalia.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Dec. 15, 2015, 11:51 a.m.
We want to make sure that the driver does not disable the FS unit if
the shader code only has SSBO writes (i.e. no color or depth output).

We could go a step further and check if the shader storage is actually
used for writing, but does not seem worth the trouble. Also, we do the
same thing for atomic buffers.

Fixes the following CTS test:
ES31-CTS.shader_storage_buffer_object.advanced-usage-sync-vsfs
---
 src/mesa/drivers/dri/i965/gen7_wm_state.c | 3 ++-
 src/mesa/drivers/dri/i965/gen8_ps_state.c | 1 +
 2 files changed, 3 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 06d5e65..d292b13 100644
--- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
@@ -77,7 +77,8 @@  upload_wm_state(struct brw_context *brw)
       dw1 |= GEN7_WM_KILL_ENABLE;
    }
 
-   if (_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx)) {
+   if (_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx ) ||
+       _mesa_active_fragment_shader_has_shader_storage(&brw->ctx)) {
       dw1 |= GEN7_WM_DISPATCH_ENABLE;
    }
 
diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c b/src/mesa/drivers/dri/i965/gen8_ps_state.c
index 945f710..8769269 100644
--- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
+++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
@@ -91,6 +91,7 @@  gen8_upload_ps_extra(struct brw_context *brw,
     * BRW_NEW_FS_PROG_DATA | BRW_NEW_FRAGMENT_PROGRAM | _NEW_BUFFERS | _NEW_COLOR
     */
    if ((_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx) ||
+        _mesa_active_fragment_shader_has_shader_storage(&brw->ctx) ||
         prog_data->base.nr_image_params) &&
        !brw_color_buffer_write_enabled(brw))
       dw1 |= GEN8_PSX_SHADER_HAS_UAV;

Comments

On Dec 15, 2015 3:52 AM, "Iago Toral Quiroga" <itoral@igalia.com> wrote:
>
> We want to make sure that the driver does not disable the FS unit if
> the shader code only has SSBO writes (i.e. no color or depth output).
>
> We could go a step further and check if the shader storage is actually
> used for writing, but does not seem worth the trouble. Also, we do the
> same thing for atomic buffers.
>
> Fixes the following CTS test:
> ES31-CTS.shader_storage_buffer_object.advanced-usage-sync-vsfs
> ---
>  src/mesa/drivers/dri/i965/gen7_wm_state.c | 3 ++-
>  src/mesa/drivers/dri/i965/gen8_ps_state.c | 1 +
>  2 files changed, 3 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 06d5e65..d292b13 100644
> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> @@ -77,7 +77,8 @@ upload_wm_state(struct brw_context *brw)
>        dw1 |= GEN7_WM_KILL_ENABLE;
>     }
>
> -   if (_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx)) {
> +   if (_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx ) ||
> +       _mesa_active_fragment_shader_has_shader_storage(&brw->ctx)) {

Ugh... We also need to be checking for images.

How about we change it to active_fragment_shader_has_side_effects and make
it check all three?

>        dw1 |= GEN7_WM_DISPATCH_ENABLE;
>     }
>
> diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c
b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> index 945f710..8769269 100644
> --- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> @@ -91,6 +91,7 @@ gen8_upload_ps_extra(struct brw_context *brw,
>      * BRW_NEW_FS_PROG_DATA | BRW_NEW_FRAGMENT_PROGRAM | _NEW_BUFFERS |
_NEW_COLOR
>      */
>     if ((_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx) ||
> +        _mesa_active_fragment_shader_has_shader_storage(&brw->ctx) ||
>          prog_data->base.nr_image_params) &&
>         !brw_color_buffer_write_enabled(brw))
>        dw1 |= GEN8_PSX_SHADER_HAS_UAV;
> --
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Jason Ekstrand <jason@jlekstrand.net> writes:

> On Dec 15, 2015 3:52 AM, "Iago Toral Quiroga" <itoral@igalia.com> wrote:
>>
>> We want to make sure that the driver does not disable the FS unit if
>> the shader code only has SSBO writes (i.e. no color or depth output).
>>
>> We could go a step further and check if the shader storage is actually
>> used for writing, but does not seem worth the trouble. Also, we do the
>> same thing for atomic buffers.
>>
>> Fixes the following CTS test:
>> ES31-CTS.shader_storage_buffer_object.advanced-usage-sync-vsfs
>> ---
>>  src/mesa/drivers/dri/i965/gen7_wm_state.c | 3 ++-
>>  src/mesa/drivers/dri/i965/gen8_ps_state.c | 1 +
>>  2 files changed, 3 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 06d5e65..d292b13 100644
>> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
>> @@ -77,7 +77,8 @@ upload_wm_state(struct brw_context *brw)
>>        dw1 |= GEN7_WM_KILL_ENABLE;
>>     }
>>
>> -   if (_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx)) {
>> +   if (_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx ) ||
>> +       _mesa_active_fragment_shader_has_shader_storage(&brw->ctx)) {
>
> Ugh... We also need to be checking for images.
>

The same bit is set when the shader has images or a bunch of other
things a couple of lines below.  No idea why atomic counters are handled
separately.

> How about we change it to active_fragment_shader_has_side_effects and make
> it check all three?
>
>>        dw1 |= GEN7_WM_DISPATCH_ENABLE;
>>     }
>>
>> diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c
> b/src/mesa/drivers/dri/i965/gen8_ps_state.c
>> index 945f710..8769269 100644
>> --- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
>> @@ -91,6 +91,7 @@ gen8_upload_ps_extra(struct brw_context *brw,
>>      * BRW_NEW_FS_PROG_DATA | BRW_NEW_FRAGMENT_PROGRAM | _NEW_BUFFERS |
> _NEW_COLOR
>>      */
>>     if ((_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx) ||
>> +        _mesa_active_fragment_shader_has_shader_storage(&brw->ctx) ||
>>          prog_data->base.nr_image_params) &&
>>         !brw_color_buffer_write_enabled(brw))
>>        dw1 |= GEN8_PSX_SHADER_HAS_UAV;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Tue, Dec 15, 2015 at 9:30 AM, Francisco Jerez <currojerez@riseup.net> wrote:
> Jason Ekstrand <jason@jlekstrand.net> writes:
>
>> On Dec 15, 2015 3:52 AM, "Iago Toral Quiroga" <itoral@igalia.com> wrote:
>>>
>>> We want to make sure that the driver does not disable the FS unit if
>>> the shader code only has SSBO writes (i.e. no color or depth output).
>>>
>>> We could go a step further and check if the shader storage is actually
>>> used for writing, but does not seem worth the trouble. Also, we do the
>>> same thing for atomic buffers.
>>>
>>> Fixes the following CTS test:
>>> ES31-CTS.shader_storage_buffer_object.advanced-usage-sync-vsfs
>>> ---
>>>  src/mesa/drivers/dri/i965/gen7_wm_state.c | 3 ++-
>>>  src/mesa/drivers/dri/i965/gen8_ps_state.c | 1 +
>>>  2 files changed, 3 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 06d5e65..d292b13 100644
>>> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
>>> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
>>> @@ -77,7 +77,8 @@ upload_wm_state(struct brw_context *brw)
>>>        dw1 |= GEN7_WM_KILL_ENABLE;
>>>     }
>>>
>>> -   if (_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx)) {
>>> +   if (_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx ) ||
>>> +       _mesa_active_fragment_shader_has_shader_storage(&brw->ctx)) {
>>
>> Ugh... We also need to be checking for images.
>>
>
> The same bit is set when the shader has images or a bunch of other
> things a couple of lines below.  No idea why atomic counters are handled
> separately.

Right.  So I guess this series is correct if not optimal.  I think I'm
still a fan of has_side_effects, but I don't care too much how it's
done as long as we make some effort to be consistent.

>> How about we change it to active_fragment_shader_has_side_effects and make
>> it check all three?
>>
>>>        dw1 |= GEN7_WM_DISPATCH_ENABLE;
>>>     }
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c
>> b/src/mesa/drivers/dri/i965/gen8_ps_state.c
>>> index 945f710..8769269 100644
>>> --- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
>>> +++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
>>> @@ -91,6 +91,7 @@ gen8_upload_ps_extra(struct brw_context *brw,
>>>      * BRW_NEW_FS_PROG_DATA | BRW_NEW_FRAGMENT_PROGRAM | _NEW_BUFFERS |
>> _NEW_COLOR
>>>      */
>>>     if ((_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx) ||
>>> +        _mesa_active_fragment_shader_has_shader_storage(&brw->ctx) ||
>>>          prog_data->base.nr_image_params) &&
>>>         !brw_color_buffer_write_enabled(brw))
>>>        dw1 |= GEN8_PSX_SHADER_HAS_UAV;
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev