[Mesa-dev] [PATCH 1/2] llvmpipe: fix blending with SRC_ALPHA_SATURATE with some formats without alpha

[Zack Rusin]

Looks good to me.

----- Original Message -----
> From: Roland Scheidegger <sroland at vmware.com>
> 
> We were fixing up the blend factor to ZERO, however this only works correctly
> with fixed point render buffers where the input values are clamped to 0/1
> (because src_alpha_saturate is min(As, 1-Ad) so can be negative with
> unclamped
> inputs). Haven't seen any failure anywhere due to that with fixed point SNORM
> buffers (which clamp inputs to -1/1) but it should apply there as well (snorm
> blending is rare, even opengl 4.3 doesn't require snorm rendertargets at all,
> d3d10 requires them but they are not blendable).
> Doesn't look like piglit hits this though (some internal testing hits the
> float case at least). (With legacy OpenGL we could theoretically still use
> the
> fixup to zero if the fragment color clamp is enabled, but we can't detect
> that
> easily since we don't support native clamping hence it gets baked into the
> shader.)
> ---
>  src/gallium/drivers/llvmpipe/lp_bld_blend_aos.c |   18 ++++++++++++++----
>  src/gallium/drivers/llvmpipe/lp_state_fs.c      |   16 ++++++++++++----
>  2 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/src/gallium/drivers/llvmpipe/lp_bld_blend_aos.c
> b/src/gallium/drivers/llvmpipe/lp_bld_blend_aos.c
> index c4d04a2..377eaa5 100644
> --- a/src/gallium/drivers/llvmpipe/lp_bld_blend_aos.c
> +++ b/src/gallium/drivers/llvmpipe/lp_bld_blend_aos.c
> @@ -114,10 +114,20 @@ lp_build_blend_factor_unswizzled(struct
> lp_build_blend_aos_context *bld,
>        if(alpha)
>           return bld->base.one;
>        else {
> -         if(!bld->inv_dst)
> -            bld->inv_dst = lp_build_comp(&bld->base, bld->dst);
> -         if(!bld->saturate)
> -            bld->saturate = lp_build_min(&bld->base, src_alpha,
> bld->inv_dst);
> +         /*
> +          * if there's separate src_alpha there's no dst alpha hence the
> complement
> +          * is zero but for unclamped float inputs min can be non-zero
> (negative).
> +          */
> +         if (bld->src_alpha) {
> +            if (!bld->saturate)
> +               bld->saturate = lp_build_min(&bld->base, src_alpha,
> bld->base.zero);
> +         }
> +         else {
> +            if(!bld->inv_dst)
> +               bld->inv_dst = lp_build_comp(&bld->base, bld->dst);
> +            if(!bld->saturate)
> +               bld->saturate = lp_build_min(&bld->base, src_alpha,
> bld->inv_dst);
> +         }
>           return bld->saturate;
>        }
>     case PIPE_BLENDFACTOR_CONST_COLOR:
> diff --git a/src/gallium/drivers/llvmpipe/lp_state_fs.c
> b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> index afd01e3..a305109 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_fs.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> @@ -2607,7 +2607,7 @@ llvmpipe_set_constant_buffer(struct pipe_context *pipe,
>   * Return the blend factor equivalent to a destination alpha of one.
>   */
>  static INLINE unsigned
> -force_dst_alpha_one(unsigned factor)
> +force_dst_alpha_one(unsigned factor, boolean clamped_zero)
>  {
>     switch(factor) {
>     case PIPE_BLENDFACTOR_DST_ALPHA:
> @@ -2615,7 +2615,10 @@ force_dst_alpha_one(unsigned factor)
>     case PIPE_BLENDFACTOR_INV_DST_ALPHA:
>        return PIPE_BLENDFACTOR_ZERO;
>     case PIPE_BLENDFACTOR_SRC_ALPHA_SATURATE:
> -      return PIPE_BLENDFACTOR_ZERO;
> +      if (clamped_zero)
> +         return PIPE_BLENDFACTOR_ZERO;
> +      else
> +         return PIPE_BLENDFACTOR_SRC_ALPHA_SATURATE;
>     }
>  
>     return factor;
> @@ -2735,8 +2738,13 @@ make_variant_key(struct llvmpipe_context *lp,
>         */
>        if (format_desc->swizzle[3] > UTIL_FORMAT_SWIZZLE_W ||
>            format_desc->swizzle[3] == format_desc->swizzle[0]) {
> -         blend_rt->rgb_src_factor   =
> force_dst_alpha_one(blend_rt->rgb_src_factor);
> -         blend_rt->rgb_dst_factor   =
> force_dst_alpha_one(blend_rt->rgb_dst_factor);
> +         /* Doesn't cover mixed snorm/unorm but can't render to them anyway
> */
> +         boolean clamped_zero = !util_format_is_float(format) &&
> +                                !util_format_is_snorm(format);
> +         blend_rt->rgb_src_factor   =
> force_dst_alpha_one(blend_rt->rgb_src_factor,
> +                                                          clamped_zero);
> +         blend_rt->rgb_dst_factor   =
> force_dst_alpha_one(blend_rt->rgb_dst_factor,
> +                                                          clamped_zero);
>           blend_rt->alpha_func       = blend_rt->rgb_func;
>           blend_rt->alpha_src_factor = blend_rt->rgb_src_factor;
>           blend_rt->alpha_dst_factor = blend_rt->rgb_dst_factor;
> --
> 1.7.9.5
>