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

[Chad Versace]

On Thu 16 Jun 2016, Jason Ekstrand wrote:
> 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?

Yes, we should do it for -0. The user inputs to glClearColor may be
calculated in convoluted ways that produce -0.

Also, I think
	s.RedClearColor = info->clear_color.f32[0] == 1.0f
is clearer, but it's not important.