[v2] i965/meta-util: Convert the clear color through the surf format

Submitted by Neil Roberts on Jan. 11, 2018, 1:19 p.m.

Details

Message ID 20180111131940.23448-1-nroberts@igalia.com
State New
Headers show
Series "i965/meta-util: Convert the clear color through the surf format" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Neil Roberts Jan. 11, 2018, 1:19 p.m.
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 having special cases for all of these 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.

Originally this patch was created to fix the following bug, but it is
no longer neccessary since f1fa4be871e13c68b50685aaf64. However I
think this approach is still worthwhile because it has the advantage
of reducing code redundancy.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93338
v2: Rebase and add support for converting int formats
---

This is a rebase of this patch from 2 years ago:

https://patchwork.freedesktop.org/patch/67912/

 src/mesa/drivers/dri/i965/brw_meta_util.c | 107 +++++++++++-------------------
 1 file changed, 37 insertions(+), 70 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_meta_util.c b/src/mesa/drivers/dri/i965/brw_meta_util.c
index 54dc6a5..ec727eb 100644
--- a/src/mesa/drivers/dri/i965/brw_meta_util.c
+++ b/src/mesa/drivers/dri/i965/brw_meta_util.c
@@ -29,6 +29,8 @@ 
 #include "main/blend.h"
 #include "main/fbobject.h"
 #include "util/format_srgb.h"
+#include "main/format_pack.h"
+#include "main/format_unpack.h"
 
 /**
  * Helper function for handling mirror image blits.
@@ -345,6 +347,7 @@  brw_meta_convert_fast_clear_color(const struct brw_context *brw,
                                   const struct intel_mipmap_tree *mt,
                                   const union gl_color_union *color)
 {
+   mesa_format linear_format = _mesa_get_srgb_format_linear(mt->format);
    union isl_color_value override_color = {
       .u32 = {
          color->ui[0],
@@ -354,82 +357,46 @@  brw_meta_convert_fast_clear_color(const struct brw_context *brw,
       },
    };
 
-   /* 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.u32[3] = override_color.u32[0];
-      /* flow through */
-   case GL_LUMINANCE:
-   case GL_LUMINANCE_ALPHA:
-      override_color.u32[1] = override_color.u32[0];
-      override_color.u32[2] = override_color.u32[0];
-      break;
-   default:
-      for (int i = 0; i < 3; i++) {
-         if (!_mesa_format_has_color_component(mt->format, i))
-            override_color.u32[i] = 0;
-      }
-      break;
-   }
-
-   switch (_mesa_get_format_datatype(mt->format)) {
-   case GL_UNSIGNED_NORMALIZED:
-      for (int i = 0; i < 4; i++)
-         override_color.f32[i] = CLAMP(override_color.f32[i], 0.0f, 1.0f);
-      break;
-
-   case GL_SIGNED_NORMALIZED:
-      for (int i = 0; i < 4; i++)
-         override_color.f32[i] = CLAMP(override_color.f32[i], -1.0f, 1.0f);
-      break;
-
-   case GL_UNSIGNED_INT:
-      for (int i = 0; i < 4; i++) {
-         unsigned bits = _mesa_get_format_bits(mt->format, GL_RED_BITS + i);
-         if (bits < 32) {
-            uint32_t max = (1u << bits) - 1;
-            override_color.u32[i] = MIN2(override_color.u32[i], max);
-         }
-      }
-      break;
-
-   case GL_INT:
-      for (int i = 0; i < 4; i++) {
-         unsigned bits = _mesa_get_format_bits(mt->format, GL_RED_BITS + i);
-         if (bits < 32) {
-            int32_t max = (1 << (bits - 1)) - 1;
-            int32_t min = -(1 << (bits - 1));
-            override_color.i32[i] = CLAMP(override_color.i32[i], min, max);
-         }
-      }
-      break;
-
-   case GL_FLOAT:
-      if (!_mesa_is_format_signed(mt->format)) {
-         for (int i = 0; i < 4; i++)
-            override_color.f32[i] = MAX2(override_color.f32[i], 0.0f);
-      }
-      break;
-   }
-
-   if (!_mesa_format_has_color_component(mt->format, 3)) {
-      if (_mesa_is_format_integer_color(mt->format))
-         override_color.u32[3] = 1;
-      else
-         override_color.f32[3] = 1.0f;
-   }
-
    /* Handle linear to 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.f32[i] =
             util_format_linear_to_srgb_float(override_color.f32[i]);
       }
    }
 
+   union gl_color_union tmp_color;
+
+   /* 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_uint_rgba_row(linear_format,
+                               1, /* n_pixels */
+                               (const GLuint (*)[4]) override_color.u32,
+                               &tmp_color);
+      _mesa_unpack_uint_rgba_row(linear_format,
+                                 1, /* n_pixels */
+                                 &tmp_color,
+                                 (GLuint (*)[4]) override_color.u32);
+   } else {
+      _mesa_pack_float_rgba_row(linear_format,
+                                1, /* n_pixels */
+                                (const GLfloat (*)[4]) override_color.f32,
+                                &tmp_color);
+      _mesa_unpack_rgba_row(linear_format,
+                            1, /* n_pixels */
+                            &tmp_color,
+                            (GLfloat (*)[4]) override_color.f32);
+   }
+
    return override_color;
 }

Comments

On Thu, Jan 11, 2018 at 02:19:40PM +0100, Neil Roberts wrote:
> 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 having special cases for all of these 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.
> 
> Originally this patch was created to fix the following bug, but it is
> no longer neccessary since f1fa4be871e13c68b50685aaf64. However I
> think this approach is still worthwhile because it has the advantage
> of reducing code redundancy.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93338
> v2: Rebase and add support for converting int formats
> ---
> 
> This is a rebase of this patch from 2 years ago:
> 
> https://patchwork.freedesktop.org/patch/67912/
> 

This is a really nice clean up. A couple of suggestions below:

>  src/mesa/drivers/dri/i965/brw_meta_util.c | 107 +++++++++++-------------------
>  1 file changed, 37 insertions(+), 70 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_meta_util.c b/src/mesa/drivers/dri/i965/brw_meta_util.c
> index 54dc6a5..ec727eb 100644
> --- a/src/mesa/drivers/dri/i965/brw_meta_util.c
> +++ b/src/mesa/drivers/dri/i965/brw_meta_util.c
> @@ -29,6 +29,8 @@
>  #include "main/blend.h"
>  #include "main/fbobject.h"
>  #include "util/format_srgb.h"
> +#include "main/format_pack.h"
> +#include "main/format_unpack.h"
>  
>  /**
>   * Helper function for handling mirror image blits.
> @@ -345,6 +347,7 @@ brw_meta_convert_fast_clear_color(const struct brw_context *brw,
>                                    const struct intel_mipmap_tree *mt,
>                                    const union gl_color_union *color)
>  {
> +   mesa_format linear_format = _mesa_get_srgb_format_linear(mt->format);
>     union isl_color_value override_color = {
>        .u32 = {
>           color->ui[0],
> @@ -354,82 +357,46 @@ brw_meta_convert_fast_clear_color(const struct brw_context *brw,
>        },
>     };
>  
> -   /* 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.u32[3] = override_color.u32[0];
> -      /* flow through */
> -   case GL_LUMINANCE:
> -   case GL_LUMINANCE_ALPHA:
> -      override_color.u32[1] = override_color.u32[0];
> -      override_color.u32[2] = override_color.u32[0];
> -      break;
> -   default:
> -      for (int i = 0; i < 3; i++) {
> -         if (!_mesa_format_has_color_component(mt->format, i))
> -            override_color.u32[i] = 0;
> -      }
> -      break;
> -   }
> -
> -   switch (_mesa_get_format_datatype(mt->format)) {
> -   case GL_UNSIGNED_NORMALIZED:
> -      for (int i = 0; i < 4; i++)
> -         override_color.f32[i] = CLAMP(override_color.f32[i], 0.0f, 1.0f);
> -      break;
> -
> -   case GL_SIGNED_NORMALIZED:
> -      for (int i = 0; i < 4; i++)
> -         override_color.f32[i] = CLAMP(override_color.f32[i], -1.0f, 1.0f);
> -      break;
> -
> -   case GL_UNSIGNED_INT:
> -      for (int i = 0; i < 4; i++) {
> -         unsigned bits = _mesa_get_format_bits(mt->format, GL_RED_BITS + i);
> -         if (bits < 32) {
> -            uint32_t max = (1u << bits) - 1;
> -            override_color.u32[i] = MIN2(override_color.u32[i], max);
> -         }
> -      }
> -      break;
> -
> -   case GL_INT:
> -      for (int i = 0; i < 4; i++) {
> -         unsigned bits = _mesa_get_format_bits(mt->format, GL_RED_BITS + i);
> -         if (bits < 32) {
> -            int32_t max = (1 << (bits - 1)) - 1;
> -            int32_t min = -(1 << (bits - 1));
> -            override_color.i32[i] = CLAMP(override_color.i32[i], min, max);
> -         }
> -      }
> -      break;
> -
> -   case GL_FLOAT:
> -      if (!_mesa_is_format_signed(mt->format)) {
> -         for (int i = 0; i < 4; i++)
> -            override_color.f32[i] = MAX2(override_color.f32[i], 0.0f);
> -      }
> -      break;
> -   }
> -
> -   if (!_mesa_format_has_color_component(mt->format, 3)) {
> -      if (_mesa_is_format_integer_color(mt->format))
> -         override_color.u32[3] = 1;
> -      else
> -         override_color.f32[3] = 1.0f;
> -   }
> -
>     /* Handle linear to 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.f32[i] =
>              util_format_linear_to_srgb_float(override_color.f32[i]);
>        }
>     }
>  
> +   union gl_color_union tmp_color;

I think it'd be more descriptive and clearer to use something like:

	char tmp_pixel[16];

> +
> +   /* 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_uint_rgba_row(linear_format,
> +                               1, /* n_pixels */
> +                               (const GLuint (*)[4]) override_color.u32,
> +                               &tmp_color);
> +      _mesa_unpack_uint_rgba_row(linear_format,
> +                                 1, /* n_pixels */
> +                                 &tmp_color,
> +                                 (GLuint (*)[4]) override_color.u32);
> +   } else {

I think we should let the following packing function handle the sRGB
conversion. I've prototyped the core of this suggestion to something like:

      mesa_format mt_format_linear = _mesa_get_srgb_format_linear(mt->format);
      bool mt_format_is_srgb = mt->format != mt_format_linear;
      mesa_format pack_format = srgb_enabled && mt_format_is_srgb ?
                                mt->format : linear_format;


-Nanley

> +      _mesa_pack_float_rgba_row(linear_format,
> +                                1, /* n_pixels */
> +                                (const GLfloat (*)[4]) override_color.f32,
> +                                &tmp_color);
> +      _mesa_unpack_rgba_row(linear_format,
> +                            1, /* n_pixels */
> +                            &tmp_color,
> +                            (GLfloat (*)[4]) override_color.f32);
> +   }
> +
>     return override_color;
>  }
> -- 
> 2.9.5
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev