On Thu, Jun 16, 2016 at 11:05 AM, Chad Versace <chad.vers...@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?
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
