[Mesa-dev] [PATCH] glsl: allow local_size qualifiers in compute shader regardless of version

[Miklós Máté]

On 26/08/16 21:17, Ilia Mirkin wrote:
> On Fri, Aug 26, 2016 at 3:08 PM, Miklós Máté <mtmkls at gmail.com> wrote:
>> On 08/26/2016 07:47 PM, Ilia Mirkin wrote:
>>> On Fri, Aug 26, 2016 at 1:39 PM, Miklós Máté <mtmkls at gmail.com> wrote:
>>>> On 08/26/2016 06:46 PM, Ilia Mirkin wrote:
>>>>> On Fri, Aug 26, 2016 at 12:42 PM, Miklós Máté <mtmkls at gmail.com> wrote:
>>>>>> Tomb Raider 2013 uses #version 420 in compute shaders, and current Mesa
>>>>>> rejects them, because the local size qualifiers require 430.
>>>>>>
>>>>>> Signed-off-by: Miklós Máté <mtmkls at gmail.com>
>>>>>> ---
>>>>>>     src/compiler/glsl/glsl_parser.yy | 5 ++---
>>>>>>     1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/src/compiler/glsl/glsl_parser.yy
>>>>>> b/src/compiler/glsl/glsl_parser.yy
>>>>>> index 4043dae..6857443 100644
>>>>>> --- a/src/compiler/glsl/glsl_parser.yy
>>>>>> +++ b/src/compiler/glsl/glsl_parser.yy
>>>>>> @@ -1615,10 +1615,9 @@ layout_qualifier_id:
>>>>>>           for (int i = 0; i < 3; i++) {
>>>>>>              if (match_layout_qualifier(local_size_qualifiers[i], $1,
>>>>>>                                         state) == 0) {
>>>>>> -            if (!state->has_compute_shader()) {
>>>>> IIRC this is the only use of ->has_compute_shader() so you could also
>>>>> remove that function. I sent a similar patch earlier that also removed
>>>>> the GL_ARB_compute_shader enable as it's not anywhere in the specs,
>>>>> but was told that a lot of people had stuck it in their shaders
>>>>> anyways, so I guess that bit has to stay.
>>>>>
>>>>> Chances are, though, that you want to go through and enable bits like
>>>>> shared memory, etc, not just the local_size. Have a look at
>>>>>
>>>>> https://patchwork.freedesktop.org/patch/107119/
>>>>>
>>>>> Feel free to respin it without the removal of the extension enable.
>>>> I think your patch is much more comprehensive, my patch only makes Tomb
>>>> Raider happy. Without the removal of the enable bit, it looks good to me.
>>> BTW, would I be correct in guessing that you have a IVB/Haswell, and
>>> have applied the fp64 patches, but still don't have
>>> GL_ARB_stencil_texturing and thus no GL 4.3? I'm pretty sure others
>>> have been successful in running it on mesa.
>>>
>>>     -ilia
>>
>> I have radeonsi with GL 4.3, but it seems the #version 420 directive is what
>> matters for the compiler. I did the patch based on replaying the trace
>> linked in https://bugs.freedesktop.org/show_bug.cgi?id=95190 with glretrace,
>> The game itself seems to swallow all errors, and it just goes on without the
>> compute shaders silently if they fail to compile. Setting stuff to "ultra"
>> and "tressfx" require compute, as those are unavailable with GL 4.1, but I
>> don't see any difference when they are enabled. At least the hair is
>> supposed to look much different with tressfx AFAIK. Maybe something is still
>> missing...
> Ah, perhaps it was captured with a GL 4.2-exposing driver then. I
> think a lot of these games just stick "#version <latest supported by
> driver>" into their shaders (which IMHO is a mistake, but ... wtvr).
> Although it was Karol doing the trace, which likely means nouveau.
> Perhaps it was during the short period of time that we had all the GL
> 4.3 features available but were too chicken to enable it.
>
>    -ilia

Hi,

sorry for the long delay. I now captured a trace with GL 4.3-capable 
radeonsi, and you're right, the game inserts "#version <the highest 
supported>" into the shaders. When 430 is supported, everything works 
fine. Therefore, this patch is not required for Tomb Raider 2013, but I 
still think that compute shaders are supposed to work with "#version 420".

I found out that I wasn't able to see the difference in the hair 
quality, because the TressFX hair cannot be toggled on-the-fly. The game 
has to be restarted to apply this setting, and AFAICT this is the only 
setting that needs restart. In his video 
https://www.youtube.com/watch?v=kM2bfQ7apkQ TotalBiscuit can toggle it 
on-the-fly at 10:20, so this capability was somehow lost when the game 
was ported to Linux.

MM