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

[Roland Scheidegger]

Am 19.03.2014 10:44, schrieb 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.
Are you sure? mesa does not seem to enable color clamps if there's all
unorm buffers.
void
_mesa_update_clamp_fragment_color(struct gl_context *ctx)
{
   struct gl_framebuffer *fb = ctx->DrawBuffer;

   /* Don't clamp if:
    * - there is no colorbuffer
    * - all colorbuffers are unsigned normalized, so clamping has no effect
    * - there is an integer colorbuffer
    */
   if (!fb || !fb->_HasSNormOrFloatColorBuffer || fb->_IntegerColor)
      ctx->Color._ClampFragmentColor = GL_FALSE;
   else
      ctx->Color._ClampFragmentColor = _mesa_get_clamp_fragment_color(ctx);
}



> 
> 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.

I agree color clamping is a mess. It doesn't help that GL has TWO clamps
- one for shader outputs (this is what will clamp to 0/1 apparently even
for signed normalized buffers when enabled), plus the always active
pre-blend clamp (this one will more meaningfully clamp to -1/1 for
signed normalized, though only mentioned correctly in 4.2+ - apparently
this is what your hw will do). At least the former clamp is legacy only...

I'm not really convinced the _ClampFragmentColor bit is actually fully
correct neither, I think the evaluation if clamping is needed or not
would need to be done per color buffer.

Roland

> 
> 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,
>