[Mesa-dev] [PATCH] i965/meta-fast-clear: Convert the clear color through the surf format

[Neil Roberts]

Bump. Anyone fancy reviewing this small patch? I think it would be good
to have because it makes the code a bit simpler as well as fixing a
corner case and making it more robust.

- Neil

Neil Roberts <neil at linux.intel.com> writes:

> When programming the fast clear color there was previously a chunk of
> code to try to make the color match the constraints of the surface
> format such as by filling in missing components and handling luminance
> formats. These cases are not handled by the hardware. There are some
> additional possible restrictions that the hardware does seem to
> handle, such as clamping to [0,1] for normalised formats. However for
> whatever reason it doesn't clamp to [0,∞] for the special float
> formats that don't have a sign bit. Rather than adding yet another
> special case for this format this patch makes it instead convert the
> color to the actual surface format and back again so that we can be
> sure it will have all of the possible restrictions. Additionally this
> would avoid some other potential surprises such as getting more
> precision for the clear color when fast clears are used.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93338
> ---
>  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c | 57 ++++++++++++-------------
>  1 file changed, 27 insertions(+), 30 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> index cf0e56b..29ae6f0 100644
> --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> @@ -37,6 +37,8 @@
>  #include "main/uniforms.h"
>  #include "main/fbobject.h"
>  #include "main/texobj.h"
> +#include "main/format_unpack.h"
> +#include "main/format_pack.h"
>  
>  #include "main/api_validate.h"
>  #include "main/state.h"
> @@ -397,45 +399,40 @@ set_fast_clear_color(struct brw_context *brw,
>                       struct intel_mipmap_tree *mt,
>                       const union gl_color_union *color)
>  {
> +   mesa_format linear_format = _mesa_get_srgb_format_linear(mt->format);
>     union gl_color_union override_color = *color;
> -
> -   /* The sampler doesn't look at the format of the surface when the fast
> -    * clear color is used so we need to implement luminance, intensity and
> -    * missing components manually.
> -    */
> -   switch (_mesa_get_format_base_format(mt->format)) {
> -   case GL_INTENSITY:
> -      override_color.ui[3] = override_color.ui[0];
> -      /* flow through */
> -   case GL_LUMINANCE:
> -   case GL_LUMINANCE_ALPHA:
> -      override_color.ui[1] = override_color.ui[0];
> -      override_color.ui[2] = override_color.ui[0];
> -      break;
> -   default:
> -      for (int i = 0; i < 3; i++) {
> -         if (!_mesa_format_has_color_component(mt->format, i))
> -            override_color.ui[i] = 0;
> -      }
> -      break;
> -   }
> -
> -   if (!_mesa_format_has_color_component(mt->format, 3)) {
> -      if (_mesa_is_format_integer_color(mt->format))
> -         override_color.ui[3] = 1;
> -      else
> -         override_color.f[3] = 1.0f;
> -   }
> +   union gl_color_union tmp_color;
>  
>     /* Handle linear→SRGB conversion */
> -   if (brw->ctx.Color.sRGBEnabled &&
> -       _mesa_get_srgb_format_linear(mt->format) != mt->format) {
> +   if (brw->ctx.Color.sRGBEnabled && linear_format != mt->format) {
>        for (int i = 0; i < 3; i++) {
>           override_color.f[i] =
>              util_format_linear_to_srgb_float(override_color.f[i]);
>        }
>     }
>  
> +   /* Convert the clear color to the surface format and back so that the color
> +    * returned when sampling is guaranteed to be a value that could be stored
> +    * in the surface. For example if the surface is a luminance format and we
> +    * clear to 0.5,0.75,0.1,0.2 we want the color to come back as
> +    * 0.5,0.5,0.5,1.0. In general the hardware doesn't seem to look at the
> +    * surface format when returning the clear color so we need to do this to
> +    * implement luminance, intensity and missing components. However it does
> +    * seem to look at it in some cases such as to clamp to the range [0,1] for
> +    * unorm formats. Suprisingly however it doesn't clamp to [0,∞] for the
> +    * special float formats that don't have a sign bit.
> +    */
> +   if (!_mesa_is_format_integer_color(linear_format)) {
> +      _mesa_pack_float_rgba_row(linear_format,
> +                                1, /* n_pixels */
> +                                (const GLfloat (*)[4]) override_color.f,
> +                                &tmp_color);
> +      _mesa_unpack_rgba_row(linear_format,
> +                            1, /* n_pixels */
> +                            &tmp_color,
> +                            (GLfloat (*)[4]) override_color.f);
> +   }
> +
>     if (brw->gen >= 9) {
>        mt->gen9_fast_clear_color = override_color;
>     } else {
> -- 
> 1.9.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev