[Mesa-dev] [PATCH 17/64] isl/state: Refactor the setup of clear colors

[Chad Versace]

On Sat 11 Jun 2016, Jason Ekstrand wrote:
> This commit switches clear colors to use #if's instead of a C if.  This
> lets us properly handle SNB where the clear color field doesn't exist.
> ---
>  src/intel/isl/isl_surface_state.c | 44 +++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/src/intel/isl/isl_surface_state.c b/src/intel/isl/isl_surface_state.c
> index db90936..aa720d8 100644
> --- a/src/intel/isl/isl_surface_state.c
> +++ b/src/intel/isl/isl_surface_state.c
> @@ -372,31 +372,31 @@ isl_genX(surf_fill_state_s)(const struct isl_device *dev, void *state,
>     }
>  #endif
>  
> -   if (GEN_GEN <= 8) {
> -      /* Prior to Sky Lake, we only have one bit for the clear color which
> -       * gives us 0 or 1 in whatever the surface's format happens to be.
> -       */
> -      if (isl_format_has_int_channel(info->view->format)) {
> -         for (unsigned i = 0; i < 4; i++) {
> -            assert(info->clear_color.u32[i] == 0 ||
> -                   info->clear_color.u32[i] == 1);
> -         }
> -      } else {
> -         for (unsigned i = 0; i < 4; i++) {
> -            assert(info->clear_color.f32[i] == 0.0f ||
> -                   info->clear_color.f32[i] == 1.0f);
> -         }
> +#if GEN_GEN >= 9
> +   s.RedClearColor = info->clear_color.u32[0];
> +   s.GreenClearColor = info->clear_color.u32[1];
> +   s.BlueClearColor = info->clear_color.u32[2];
> +   s.AlphaClearColor = info->clear_color.u32[3];

The gen9 block looks good.

> +#elif GEN_GEN >= 7
> +   /* Prior to Sky Lake, we only have one bit for the clear color which
> +    * gives us 0 or 1 in whatever the surface's format happens to be.
> +    */
> +   if (isl_format_has_int_channel(info->view->format)) {
> +      for (unsigned i = 0; i < 4; i++) {
> +         assert(info->clear_color.u32[i] == 0 ||
> +                info->clear_color.u32[i] == 1);
>        }
> -      s.RedClearColor = info->clear_color.u32[0] != 0;
> -      s.GreenClearColor = info->clear_color.u32[1] != 0;
> -      s.BlueClearColor = info->clear_color.u32[2] != 0;
> -      s.AlphaClearColor = info->clear_color.u32[3] != 0;
>     } else {
> -      s.RedClearColor = info->clear_color.u32[0];
> -      s.GreenClearColor = info->clear_color.u32[1];
> -      s.BlueClearColor = info->clear_color.u32[2];
> -      s.AlphaClearColor = info->clear_color.u32[3];
> +      for (unsigned i = 0; i < 4; i++) {
> +         assert(info->clear_color.f32[i] == 0.0f ||
> +                info->clear_color.f32[i] == 1.0f);
> +      }
>     }
> +   s.RedClearColor = info->clear_color.u32[0] != 0;
> +   s.GreenClearColor = info->clear_color.u32[1] != 0;
> +   s.BlueClearColor = info->clear_color.u32[2] != 0;
> +   s.AlphaClearColor = info->clear_color.u32[3] != 0;

This block looks broken, both pre- and post-patch. Is this code actually
used anywhere in the Vulkan driver? I expect not, as the driver doesn't
use the CCS yet.

In the for loop, the code asserts clear_color.f32[i] is 0.0f or 1.0f.
I believe the assignment expressions should also use f32, not u32. That
is,

	s.RedClearColor = info->clear_color.f32[0] != 0.0f;
	s.GreenClearColor = info->clear_color.f32[1] != 0.0f;
	s.BlueClearColor = info->clear_color.f32[2] != 0.0f;
	s.AlphaClearColor = info->clear_color.f32[3] != 0.0f;