[Mesa-dev] [PATCH] llvmpipe: Fix alpha testing precision on rgba8 formats.

[Roland Scheidegger]

Am 22.05.2012 17:07, schrieb jfonseca at vmware.com:
> From: José Fonseca <jfonseca at vmware.com>
> 
> This is a long standing problem, that recently surfaced with the change
> to enable perspective correct color interpolation.
> 
> A fix for all possible formats is left to the future.
> ---
>  src/gallium/drivers/llvmpipe/lp_bld_alpha.c |   31 +++++++++++++++++++++++++++
>  src/gallium/drivers/llvmpipe/lp_bld_alpha.h |    2 ++
>  src/gallium/drivers/llvmpipe/lp_state_fs.c  |    5 ++++-
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gallium/drivers/llvmpipe/lp_bld_alpha.c b/src/gallium/drivers/llvmpipe/lp_bld_alpha.c
> index 518969c..bf4e8c4 100644
> --- a/src/gallium/drivers/llvmpipe/lp_bld_alpha.c
> +++ b/src/gallium/drivers/llvmpipe/lp_bld_alpha.c
> @@ -32,9 +32,12 @@
>   */
>  
>  #include "pipe/p_state.h"
> +#include "util/u_format.h"
>  
>  #include "gallivm/lp_bld_type.h"
>  #include "gallivm/lp_bld_const.h"
> +#include "gallivm/lp_bld_arit.h"
> +#include "gallivm/lp_bld_conv.h"
>  #include "gallivm/lp_bld_logic.h"
>  #include "gallivm/lp_bld_flow.h"
>  #include "gallivm/lp_bld_debug.h"
> @@ -46,6 +49,7 @@ void
>  lp_build_alpha_test(struct gallivm_state *gallivm,
>                      unsigned func,
>                      struct lp_type type,
> +                    const struct util_format_description *cbuf_format_desc,
>                      struct lp_build_mask_context *mask,
>                      LLVMValueRef alpha,
>                      LLVMValueRef ref,
> @@ -56,6 +60,33 @@ lp_build_alpha_test(struct gallivm_state *gallivm,
>  
>     lp_build_context_init(&bld, gallivm, type);
>  
> +   /*
> +    * Alpha testing needs to be done in the color buffer precision.
> +    *
> +    * TODO: Ideally, instead of duplicating the color conversion code, we would do
> +    * alpha testing after converting the output colors, but that's not very
> +    * convenient, because it needs to be done before depth testing.  Hopefully
> +    * LLVM will detect and remove the duplicate expression.
This is probably not true on x86, with sse2 there are no duplicate
expressions which llvm could recognize, as the color conversion code
will use an optimized path skipping clamping entirely by relying on pack
intrinsics.


> +    *
> +    * FIXME: This should be generalized to formats other than rgba8 variants.
> +    * It is, however, difficult to come up with a truly generic rule that would
> +    * work for an arbitrary format.  For example, how many bits should we use
> +    * when doing alpha testing for B5G5R5X1_UNORM?
> +    */
> +   if (type.floating &&
> +       util_format_is_rgba8_variant(cbuf_format_desc)) {
> +      const unsigned dst_width = 8;
> +
> +      alpha = lp_build_clamp(&bld, alpha, bld.zero, bld.one);
> +      ref   = lp_build_clamp(&bld, ref,   bld.zero, bld.one);
> +
> +      alpha = lp_build_clamped_float_to_unsigned_norm(gallivm, type, dst_width, alpha);
> +      ref   = lp_build_clamped_float_to_unsigned_norm(gallivm, type, dst_width, ref);
> +
> +      type.floating = 0;
> +      lp_build_context_init(&bld, gallivm, type);
> +   }
> +
>     test = lp_build_cmp(&bld, func, alpha, ref);
>  
>     lp_build_name(test, "alpha_mask");
> diff --git a/src/gallium/drivers/llvmpipe/lp_bld_alpha.h b/src/gallium/drivers/llvmpipe/lp_bld_alpha.h
> index 06206a2..15f1284 100644
> --- a/src/gallium/drivers/llvmpipe/lp_bld_alpha.h
> +++ b/src/gallium/drivers/llvmpipe/lp_bld_alpha.h
> @@ -39,6 +39,7 @@
>  #include "gallivm/lp_bld.h"
>  
>  struct pipe_alpha_state;
> +struct util_format_description;
>  struct gallivm_state;
>  struct lp_type;
>  struct lp_build_mask_context;
> @@ -48,6 +49,7 @@ void
>  lp_build_alpha_test(struct gallivm_state *gallivm,
>                      unsigned func,
>                      struct lp_type type,
> +                    const struct util_format_description *cbuf_format_desc,
>                      struct lp_build_mask_context *mask,
>                      LLVMValueRef alpha,
>                      LLVMValueRef ref,
> diff --git a/src/gallium/drivers/llvmpipe/lp_state_fs.c b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> index 11a3871..0bdc17f 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_fs.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> @@ -345,13 +345,16 @@ generate_fs(struct gallivm_state *gallivm,
>                                             0);
>  
>        if (color0 != -1 && outputs[color0][3]) {
> +         const struct util_format_description *cbuf_format_desc;
>           LLVMValueRef alpha = LLVMBuildLoad(builder, outputs[color0][3], "alpha");
>           LLVMValueRef alpha_ref_value;
>  
>           alpha_ref_value = lp_jit_context_alpha_ref_value(gallivm, context_ptr);
>           alpha_ref_value = lp_build_broadcast(gallivm, vec_type, alpha_ref_value);
>  
> -         lp_build_alpha_test(gallivm, key->alpha.func, type,
> +         cbuf_format_desc = util_format_description(key->cbuf_format[0]);
> +
> +         lp_build_alpha_test(gallivm, key->alpha.func, type, cbuf_format_desc,
>                               &mask, alpha, alpha_ref_value,
>                               (depth_mode & LATE_DEPTH_TEST) != 0);
>        }
Reviewed-by: Roland Scheidegger <sroland at vmware.com>

Roland