<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 16, 2016 at 11:05 AM, Chad Versace <span dir="ltr"><<a href="mailto:chad.versace@intel.com" target="_blank">chad.versace@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Sat 11 Jun 2016, Jason Ekstrand wrote:<br>
> This commit switches clear colors to use #if's instead of a C if.  This<br>
> lets us properly handle SNB where the clear color field doesn't exist.<br>
> ---<br>
>  src/intel/isl/isl_surface_state.c | 44 +++++++++++++++++++--------------------<br>
>  1 file changed, 22 insertions(+), 22 deletions(-)<br>
><br>
> diff --git a/src/intel/isl/isl_surface_state.c b/src/intel/isl/isl_surface_state.c<br>
> index db90936..aa720d8 100644<br>
> --- a/src/intel/isl/isl_surface_state.c<br>
> +++ b/src/intel/isl/isl_surface_state.c<br>
> @@ -372,31 +372,31 @@ isl_genX(surf_fill_state_s)(const struct isl_device *dev, void *state,<br>
>     }<br>
>  #endif<br>
><br>
> -   if (GEN_GEN <= 8) {<br>
> -      /* Prior to Sky Lake, we only have one bit for the clear color which<br>
> -       * gives us 0 or 1 in whatever the surface's format happens to be.<br>
> -       */<br>
> -      if (isl_format_has_int_channel(info->view->format)) {<br>
> -         for (unsigned i = 0; i < 4; i++) {<br>
> -            assert(info->clear_color.u32[i] == 0 ||<br>
> -                   info->clear_color.u32[i] == 1);<br>
> -         }<br>
> -      } else {<br>
> -         for (unsigned i = 0; i < 4; i++) {<br>
> -            assert(info->clear_color.f32[i] == 0.0f ||<br>
> -                   info->clear_color.f32[i] == 1.0f);<br>
> -         }<br>
> +#if GEN_GEN >= 9<br>
> +   s.RedClearColor = info->clear_color.u32[0];<br>
> +   s.GreenClearColor = info->clear_color.u32[1];<br>
> +   s.BlueClearColor = info->clear_color.u32[2];<br>
> +   s.AlphaClearColor = info->clear_color.u32[3];<br>
<br>
</div></div>The gen9 block looks good.<br>
<div><div class="h5"><br>
> +#elif GEN_GEN >= 7<br>
> +   /* Prior to Sky Lake, we only have one bit for the clear color which<br>
> +    * gives us 0 or 1 in whatever the surface's format happens to be.<br>
> +    */<br>
> +   if (isl_format_has_int_channel(info->view->format)) {<br>
> +      for (unsigned i = 0; i < 4; i++) {<br>
> +         assert(info->clear_color.u32[i] == 0 ||<br>
> +                info->clear_color.u32[i] == 1);<br>
>        }<br>
> -      s.RedClearColor = info->clear_color.u32[0] != 0;<br>
> -      s.GreenClearColor = info->clear_color.u32[1] != 0;<br>
> -      s.BlueClearColor = info->clear_color.u32[2] != 0;<br>
> -      s.AlphaClearColor = info->clear_color.u32[3] != 0;<br>
>     } else {<br>
> -      s.RedClearColor = info->clear_color.u32[0];<br>
> -      s.GreenClearColor = info->clear_color.u32[1];<br>
> -      s.BlueClearColor = info->clear_color.u32[2];<br>
> -      s.AlphaClearColor = info->clear_color.u32[3];<br>
> +      for (unsigned i = 0; i < 4; i++) {<br>
> +         assert(info->clear_color.f32[i] == 0.0f ||<br>
> +                info->clear_color.f32[i] == 1.0f);<br>
> +      }<br>
>     }<br>
> +   s.RedClearColor = info->clear_color.u32[0] != 0;<br>
> +   s.GreenClearColor = info->clear_color.u32[1] != 0;<br>
> +   s.BlueClearColor = info->clear_color.u32[2] != 0;<br>
> +   s.AlphaClearColor = info->clear_color.u32[3] != 0;<br>
<br>
</div></div>This block looks broken, both pre- and post-patch. Is this code actually<br>
used anywhere in the Vulkan driver? I expect not, as the driver doesn't<br>
use the CCS yet.<br>
<br>
In the for loop, the code asserts clear_color.f32[i] is 0.0f or 1.0f.<br>
I believe the assignment expressions should also use f32, not u32. That<br>
is,<br>
<br>
        s.RedClearColor = info->clear_color.f32[0] != 0.0f;<br>
        s.GreenClearColor = info->clear_color.f32[1] != 0.0f;<br>
        s.BlueClearColor = info->clear_color.f32[2] != 0.0f;<br>
        s.AlphaClearColor = info->clear_color.f32[3] != 0.0f;<br>
</blockquote></div><br></div><div class="gmail_extra">Right.  That's probably more correct but given that 0.0f is 0u as bits, I don't know if it actually matters.  Maybe for -0?<br></div></div>