Draw module (aaline, aapoint, and pstipple stages) will also be broken with PIPE_SHADER_CAP_OUTPUT_READ=1.
llvmpipe too, has analysis which rely on outputs not being read. Nothing of the above matters to r600, of course. Nevertheless, I can't help thinking that PIPE_SHADER_CAP_OUTPUT_READ is a mistake from a semantic point of view, as it really hinders our ability to do simple manipulations of the TGSI, as in this case. And to squeeze maximum performance a pipe driver will need an optimizing compiler regardless, so the benefits are minimal from what I can see. I think it would be better to remove PIPE_SHADER_CAP_OUTPUT_READ, and strictly enforce that outpus can't be read, same way as inputs can't be written. Jose ----- Original Message ----- > 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 > <til...@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? > _______________________________________________ > 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