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

[Jose Fonseca]

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
> <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?
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>