[Mesa-dev] [PATCH 1/3] st/mesa: do vertex and fragment color clamping in shaders

[Marek Olšák]

Hi Tilman,

Thanks for the info. I didn't consider outputs to be readable, sorry.
A quick fix would be to move the outputs to temps. There is a GLSL pass
for that and it can be enabled by reporting
PIPE_SHADER_CAP_OUTPUT_READ --> 0 in r600_pipe.c. Can you try that and
see if it helps?

To Vadim: Now that we have a GLSL pass for lowering output reads, are
you okay with removing PIPE_SHADER_CAP_OUTPUT_READ? It makes the
fallback for glClampColorARB really non-trivial.

Marek

On Tue, Feb 21, 2012 at 9:17 PM, Tilman Sauerbeck <tilman at code-monkey.de> wrote:
> Tilman Sauerbeck [2012-02-12 11:31]:
>> Marek Olšák [2012-01-23 13:32]:
>> > For ARB_color_buffer_float. Most hardware can't do it and st/mesa is
>> > the perfect place for a fallback.
>>
>> This breaks lighting in Heroes of Newerth on my rv730:
>>   http://files.code-monkey.de/frag_color_clamp_bad.png (after patch)
>>   http://files.code-monkey.de/frag_color_clamp_good.png (before patch)
>> I can provide a trace file that shows the problem (3.5GB, so will take
>> a while to upload).
>
> The problematic fragment shader looks like this:
>
> void main() {
>  gl_FragColor = vec4(0.0);
>  gl_FragColor += texture2D(...);
>  gl_FragColor += texture2D(...);
>  gl_FragColor += texture2D(...);
>  gl_FragColor += texture2D(...);
>  gl_FragColor *= 0.25;
>  gl_FragColor *= v_vColor; // varying vec4
> }
>
> Before this patch landed, mesa would generate the following TGSI for
> that shader:
>
>  0: MOV TEMP[0].x, -CONST[1].xxxx
>  1: MOV TEMP[0].y, -CONST[1].xxxx
>  2: ADD TEMP[0].xy, IN[0].xyyy, TEMP[0].xyyy
>  3: TEX TEMP[0], TEMP[0].xyyy, SAMP[0], 2D
>  4: MOV OUT[0], TEMP[0]
>  5: MOV TEMP[0].x, -CONST[1].xxxx
>  6: MOV TEMP[0].y, CONST[1].xxxx
>  7: ADD TEMP[0].xy, IN[0].xyyy, TEMP[0].xyyy
>  8: TEX TEMP[0], TEMP[0].xyyy, SAMP[0], 2D
>  9: ADD OUT[0], OUT[0], TEMP[0]
>  10: ADD TEMP[0].xy, IN[0].xyyy, CONST[1].xxxx
>  11: TEX TEMP[0], TEMP[0].xyyy, SAMP[0], 2D
>  12: ADD OUT[0], OUT[0], TEMP[0]
>  13: MOV TEMP[0].x, CONST[1].xxxx
>  14: MOV TEMP[0].y, -CONST[1].xxxx
>  15: ADD TEMP[0].xy, IN[0].xyyy, TEMP[0].xyyy
>  16: TEX TEMP[0], TEMP[0].xyyy, SAMP[0], 2D
>  17: ADD TEMP[0], OUT[0], TEMP[0]
>  18: MUL TEMP[0], TEMP[0], IMM[0].xxxx
>  19: MUL OUT[0], TEMP[0], IN[1]
>  20: END
>
> With this patch, instructions 4, 9, 12 and 19 are replaced by their
> _SAT variants.
>
> If I hack the code to only use saturate in the final instruction
> (MUL -> MUL_SAT), the game looks okay.
>
> I'm wondering whether the current behaviour is correct in that it uses
> saturate ops for the intermediate results as well.
>
> Issue 24 of the ARB_color_buffer_float says:
>
>> 24. Does this extension interact with the ARB_fragment_program or
>>     ARB_fragment_shader extensions?
>>
>>     RESOLVED:  Yes.  The only interaction is that the fragment color
>>     clamp enable determines if the final color(s) produced by the
>>     fragment program/shader has its components clamped to [0,1].
>>
>>     However, the fragment color clamp enable affects only the final
>>     result; it does NOT affect any computations performed during
>>     program execution. Note that the same clamping can be done
>>     explicitly in a fragment program or shader.
>>     ARB_fragment_program provides the "_SAT" opcode suffix to clamp
>>     instruction results to [0,1].
>
> If I'm understanding this language correctly, _and_ it applies to GLSL
> too then it seems like Mesa should only use saturation in the final
> operation that's writing to gl_FragColor, but not in the earlier ones?
>
> Regards,
> Tilman
>
> --
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing on usenet and in e-mail?