[Mesa-dev] [PATCH] llvmpipe: Fix rendering to PIPE_FORMAT_R10G10B10A2_UNORM.

[Roland Scheidegger]

Am 20.09.2013 13:59, schrieb jfonseca at vmware.com:
> From: José Fonseca <jfonseca at vmware.com>
> 
> We must take rounding in consideration when re-scaling to narrow
> normalized channels, such as 2-bit normalized alpha.
> ---
>  src/gallium/drivers/llvmpipe/lp_state_fs.c | 84 +++++++++++++++++++++++++++---
>  1 file changed, 78 insertions(+), 6 deletions(-)
> 
> diff --git a/src/gallium/drivers/llvmpipe/lp_state_fs.c b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> index 07e6685..875a3cf 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_fs.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> @@ -876,7 +876,22 @@ lp_blend_type_from_format_desc(const struct util_format_description *format_desc
>  
>  
>  /**
> - * Scale a normalized value from src_bits to dst_bits
> + * Scale a normalized value from src_bits to dst_bits.
> + *
> + * The exact calculation is
> + *
> + *    dst = iround(src * dst_mask / src_mask)
> + *
> + *  or with integer rounding
> + *
> + *    dst = src * (2*dst_mask + sign(src)*src_mask) / (2*src_mask)
> + *
> + *  where
> + *
> + *    src_mask = (1 << src_bits) - 1
> + *    dst_mask = (1 << dst_bits) - 1
> + *
> + * but we try to avoid division and multiplication through shifts.
>   */
>  static INLINE LLVMValueRef
>  scale_bits(struct gallivm_state *gallivm,
> @@ -889,11 +904,68 @@ scale_bits(struct gallivm_state *gallivm,
>     LLVMValueRef result = src;
>  
>     if (dst_bits < src_bits) {
> -      /* Scale down by LShr */
> -      result = LLVMBuildLShr(builder,
> -                             src,
> -                             lp_build_const_int_vec(gallivm, src_type, src_bits - dst_bits),
> -                             "");
> +      int delta_bits = src_bits - dst_bits;
> +
> +      if (delta_bits <= dst_bits) {
> +         /*
> +          * Approximate the rescaling with a single shift.
> +          *
> +          * This gives the wrong rounding.
> +          */
> +
> +         result = LLVMBuildLShr(builder,
> +                                src,
> +                                lp_build_const_int_vec(gallivm, src_type, delta_bits),
> +                                "");
> +
> +      } else {
> +         /*
> +          * Try more accurate rescaling.
> +          */
> +
> +         /*
> +          * Drop the least significant bits to make space for the multiplication.
> +          *
> +          * XXX: A better approach would be to use a wider integer type as intermediate.  But
> +          * this is enough to convert alpha from 16bits -> 2 when rendering to
> +          * PIPE_FORMAT_R10G10B10A2_UNORM.
> +          */
> +         result = LLVMBuildLShr(builder,
> +                                src,
> +                                lp_build_const_int_vec(gallivm, src_type, dst_bits),
> +                                "");
> +
> +
> +         result = LLVMBuildMul(builder,
> +                               result,
> +                               lp_build_const_int_vec(gallivm, src_type, (1LL << dst_bits) - 1),
> +                               "");
> +
> +         /*
> +          * Add a rounding term before the division.
> +          *
> +          * TODO: Handle signed integers too.
> +          */
> +         if (!src_type.sign) {
> +            result = LLVMBuildAdd(builder,
> +                                  result,
> +                                  lp_build_const_int_vec(gallivm, src_type, (1LL << (delta_bits - 1))),
> +                                  "");
> +         }
> +
> +         /*
> +          * Approximate the division by src_mask with a src_bits shift.
> +          *
> +          * Given the src has already been shifted by dst_bits, all we need
> +          * to do is to shift by the difference.
> +          */
> +
> +         result = LLVMBuildLShr(builder,
> +                                result,
> +                                lp_build_const_int_vec(gallivm, src_type, delta_bits),
> +                                "");
> +      }
> +
>     } else if (dst_bits > src_bits) {
>        /* Scale up bits */
>        int db = dst_bits - src_bits;
> 

Looks good to me, though that up/downscaling for A2 looks a bit
complicated but we can't avoid it.

Roland