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

[Jason Ekstrand]

On Thu, Jun 16, 2016 at 11:05 AM, Chad Versace <chad.versace at intel.com>
wrote:

> 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;
>

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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160616/680108c4/attachment.html>