[Mesa-dev] [PATCH 2/2] llvmpipe: clamp inputs for srgb render buffers

[Jose Fonseca]

Series looks good to me.

Jose

----- Original Message -----
> From: Roland Scheidegger <sroland at vmware.com>
> 
> Usually with fixed point renderbuffers clamping is done as part of
> conversion.
> However, since we blend in float format, we essentially skip all conversion
> steps pre-blend but since this is still a fixed point renderbuffer we must
> still clamp the inputs in this case. Makes no difference for piglit though.
> Obviously we could skip this if fragment color clamping is enabled, but a)
> this is deprecated in OpenGL (d3d never had it) and b) we don't support it
> natively so it gets baked into the shader.
> Also add some comment about logic ops being broken for srgb, luckily no test
> tries to do that as there's no easy fix...
> ---
>  src/gallium/drivers/llvmpipe/lp_state_fs.c |   35
>  ++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/src/gallium/drivers/llvmpipe/lp_state_fs.c
> b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> index a305109..3739941 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_fs.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> @@ -1671,6 +1671,35 @@ generate_unswizzled_blend(struct gallivm_state
> *gallivm,
>     /* Convert */
>     lp_build_conv(gallivm, fs_type, blend_type, &blend_color, 1,
>     &blend_color, 1);
>  
> +   if (out_format_desc->colorspace == UTIL_FORMAT_COLORSPACE_SRGB) {
> +      /*
> +       * since blending is done with floats, there was no conversion.
> +       * However, the rules according to fixed point renderbuffers still
> +       * apply, that is we must clamp inputs to 0.0/1.0.
> +       * (This would apply to separate alpha conversion too but we currently
> +       * force has_alpha to be true.)
> +       * TODO: should skip this with "fake" blend, since post-blend
> conversion
> +       * will clamp anyway.
> +       * TODO: could also skip this if fragment color clamping is enabled.
> We
> +       * don't support it natively so it gets baked into the shader however,
> so
> +       * can't really tell here.
> +       */
> +      struct lp_build_context f32_bld;
> +      assert(row_type.floating);
> +      lp_build_context_init(&f32_bld, gallivm, row_type);
> +      for (i = 0; i < src_count; i++) {
> +         src[i] = lp_build_clamp(&f32_bld, src[i], f32_bld.zero,
> f32_bld.one);
> +      }
> +      if (dual_source_blend) {
> +         for (i = 0; i < src_count; i++) {
> +            src1[i] = lp_build_clamp(&f32_bld, src1[i], f32_bld.zero,
> f32_bld.one);
> +         }
> +      }
> +      /* probably can't be different than row_type but better safe than
> sorry... */
> +      lp_build_context_init(&f32_bld, gallivm, blend_type);
> +      blend_color = lp_build_clamp(&f32_bld, blend_color, f32_bld.zero,
> f32_bld.one);
> +   }
> +
>     /* Extract alpha */
>     blend_alpha = lp_build_extract_broadcast(gallivm, blend_type, row_type,
>     blend_color, lp_build_const_int32(gallivm, 3));
>  
> @@ -1827,6 +1856,12 @@ generate_unswizzled_blend(struct gallivm_state
> *gallivm,
>      */
>     convert_to_blend_type(gallivm, block_size, out_format_desc, dst_type,
>     row_type, dst, src_count);
>  
> +   /*
> +    * FIXME: Really should get logic ops / masks out of generic blend / row
> +    * format. Logic ops will definitely not work on the blend float format
> +    * used for SRGB here and I think OpenGL expects this to work as expected
> +    * (that is incoming values converted to srgb then logic op applied).
> +    */
>     for (i = 0; i < src_count; ++i) {
>        dst[i] = lp_build_blend_aos(gallivm,
>                                    &variant->key.blend,
> --
> 1.7.9.5
>