[Mesa-dev] [PATCH 1/2] mesa: add a workaround for unigine Tropics

[Martin Peres]

On 30/01/15 00:26, Kenneth Graunke wrote:
> On Thursday, January 29, 2015 11:14:26 PM Martin Peres wrote:
>> While trying to understand a GLSL pass, curro and I tried running Unigine
>> Tropics and the GLSL compilers would not compile the shaders.
>>
>> The reason is due to the fact that the shaders do not specify the needed GLSL
>> version but also use in the same shader keywords that could never co-exist
>> because one got deleted when the other one was added (for instance,
>> gl_TexCoord and gl_InstanceID). The current solution was to use the
>> force_glsl_extensions_warn workaround but it broke when GL_ARB_gpu_shader5
>> got introduced as this workaround also enabled this extension which reserved
>> the name "sample" which is then used by most of Unigine Tropics' shaders.
>>
>> To fix this, the easiest solution seem to introduce a workaround to
>> disable GL_ARB_gpu_shader5 in the GLSL compiler. This is what this patch
>> does along with modifying drirc to enable this workaround on tropics.
>>
>> This patch has been tested on Haswell. It should also work on Gallium-based
>> drivers but I did not test it.
>>
>> I would like to thank curro for helping me understand the whole issue and
>> directed me to the right place in the code.
>>
>> Signed-off-by: Martin Peres <martin.peres at linux.intel.com>
> Hi Martin!
>
> Thanks for fixing this.  One small comment...
>
> [snip]
>> diff --git a/src/mesa/drivers/dri/i915/intel_screen.c b/src/mesa/drivers/dri/i915/intel_screen.c
>> index 00d8580..748eee7 100644
>> --- a/src/mesa/drivers/dri/i915/intel_screen.c
>> +++ b/src/mesa/drivers/dri/i915/intel_screen.c
>> @@ -74,6 +74,7 @@ DRI_CONF_BEGIN
>>         DRI_CONF_FORCE_GLSL_EXTENSIONS_WARN("false")
>>         DRI_CONF_DISABLE_GLSL_LINE_CONTINUATIONS("false")
>>         DRI_CONF_DISABLE_BLEND_FUNC_EXTENDED("false")
>> +      DRI_CONF_DISABLE_GLSL_EXTENSION_GPU_SHADER5("false")
>>   
>>         DRI_CONF_OPT_BEGIN_B(shader_precompile, "true")
>>   	 DRI_CONF_DESC(en, "Perform code generation at shader link time.")
> You can probably drop the i915 change - it doesn't support ARB_gpu_shader5
> anyway.

I do not think this is a good idea for two reasons:
- Not initialising this variable will lead to an assert whenever some 
common code will try to read the parameter.
- This makes the i965/i915 code paths look different when they should 
actually be shared. To ease the work of the poor soul who will have to 
factor out all the drirc configuration parsing for all the classic 
drivers and gallium.

> With that removed, you can add:
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82897

Oh no! I should have checked! It would have saved me quite some time!

> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
Do I still get your RB even if I do not delete it then?