[Mesa-dev] [PATCH] i965/fs: Don't emit saturates to do UNORM color clamping.

[Kenneth Graunke]

Traditionally, we've implemented fragment color clamping by emitting
"mov.sat" in the fragment shader's final color write code, clamping
the output to [0, 1] prior to blending.  (When clamping is off, we
use a regular "mov".)  This means we have to recompile based on the
GL_CLAMP_FRAGMENT_COLOR state, and because GL_FIXED_ONLY is an option,
we need to consider the render target format.

Modern applications frequently use both UNORM buffers and FLOAT buffers
with color clamping disabled.  (FLOAT with clamping explicitly enabled
and SNORM buffers appear to be less common.)  With all UNORM buffers,
ctx->Color._ClampFragmentColor is true, while it's false in the typical
unclamped FLOAT case.

In other words, the two most common cases disagree about whether we
should emit saturates.  This meant basically all modern applications
needed fragment shader recompiles.

It turns out that the saturates are superfluous for UNORM buffers.  We
always configure BLEND_STATE to enable pre-blend color clamping, using
the render target format's range.  This means the color calculator will
already clamp all UNORM buffers to [0, 1].

By dropping the saturates for the UNORM case, key->clamp_fragment_color
can simply be false in the two most common cases, giving us a clear
value to guess in the precompile.

Unfortunately, it doesn't look like we can rely on the color calculator
to implement fragment color clamping for non-UNORM formats:

For SNORM, the color calculator clamps to [-1, 1] while OpenGL requires
[0, 1] for some reason.  (Technically, the ARB_color_buffer_float spec
does specify [-1, 1], but every other specification contradicts that.)
Requesting BRW_RENDERTARGET_CLAMPRANGE_UNORM is not allowed.

Although we are allowed to request [0, 1] pre-blend clamping for FLOAT
formats, we are also required to enable post-blend clamping, and it uses
the same range.  I believe this is incorrect for OpenGL rules---at the
very least, no values appear to satisfy Piglit's
arb_color_buffer_float-render test.

Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
---
 src/mesa/drivers/dri/i965/brw_fs.cpp | 2 --
 src/mesa/drivers/dri/i965/brw_wm.c   | 3 ++-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
index 1b32d63..bd44952 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
@@ -3616,8 +3616,6 @@ brw_fs_precompile(struct gl_context *ctx, struct gl_shader_program *prog)
                                          BRW_FS_VARYING_INPUT_MASK) > 16)
       key.input_slots_valid = fp->Base.InputsRead | VARYING_BIT_POS;
 
-   key.clamp_fragment_color = ctx->API == API_OPENGL_COMPAT;
-
    unsigned sampler_count = _mesa_fls(fp->Base.SamplersUsed);
    for (unsigned i = 0; i < sampler_count; i++) {
       if (fp->Base.ShadowSamplers & (1 << i)) {
diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c
index 0d0d6ec..baf001f 100644
--- a/src/mesa/drivers/dri/i965/brw_wm.c
+++ b/src/mesa/drivers/dri/i965/brw_wm.c
@@ -488,7 +488,8 @@ static void brw_wm_populate_key( struct brw_context *brw,
    key->flat_shade = (ctx->Light.ShadeModel == GL_FLAT);
 
    /* _NEW_FRAG_CLAMP | _NEW_BUFFERS */
-   key->clamp_fragment_color = ctx->Color._ClampFragmentColor;
+   key->clamp_fragment_color = ctx->Color._ClampFragmentColor &&
+                               ctx->DrawBuffer->_HasSNormOrFloatColorBuffer;
 
    /* _NEW_TEXTURE */
    brw_populate_sampler_prog_key_data(ctx, prog, brw->wm.base.sampler_count,
-- 
1.9.0