[Mesa-dev] [PATCH 7/7] [v2] i965/gen9: Support fast clears for 32b float

[Ben Widawsky]

On Thu, Nov 12, 2015 at 11:39:25AM +0100, Neil Roberts wrote:
> Ben Widawsky <benjamin.widawsky at intel.com> writes:
> 
> > Two formats are explicitly disabled because they fail piglit tests,
> > LUMINANCE16F and INTENSITY16F. There is some question about the
> > validity of sampling from these surfaces for all gens, however, there
> > seem to be no other failures, so I'd prefer to leave tackling that for
> > a separate series.
> 
> Did you see my message in another thread pointing out that there are
> other issues with the SKL fast clear? If you clear to white instead of
> black then a bunch of other more normal formats fail such as GL_RGB.
> This isn't related to this 32b float patch because obviously white
> should fit the criteria of being just 0 and 1.

I saw the message, I just didn't realize you were advocating for holding things
up for that. If this is the case, perhaps we can merge all the support and limit
the fast clears to black only for now. Or, merge the support and keep them
disabled on GEN9.

> 
> Perhaps the easiest way to demonstrate this bug is to tweak the formats
> test so that it clears to white instead of black, like this:
> 
> diff --git a/tests/util/piglit-test-pattern.cpp b/tests/util/piglit-test-pattern.cpp
> index 12d9918..bec25bc 100644
> --- a/tests/util/piglit-test-pattern.cpp
> +++ b/tests/util/piglit-test-pattern.cpp
> @@ -664,7 +664,7 @@ ColorGradientSunburst::draw_with_scale_and_offset(const float (*proj)[4],
>         }
>         case GL_UNSIGNED_NORMALIZED:
>         case GL_FLOAT: {
> -               glClearColor(offset, offset, offset, offset);
> +               glClearColor(1.0, 1.0, 1.0, 1.0);
>                 glClear(GL_COLOR_BUFFER_BIT);
>                 break;
>         }
> 
> This passes on HSW but makes a bunch of formats fail on SKL.
> 
> It seems like we're missing something about how we're supposed to
> program the fast colour state. For GL_RGB it seems to be expecting an
> integer value in the range 0???255 instead of a float. Eg, if you program
> the integer 128 you get back 0.5 when reading. However this isn't the
> case for GL_RGBA which does seem to expect float values. I can't find
> anything that would explain this in the specs. It makes no sense!

Interesting. The type of the color values are supposed to be inferred from the
format in the surface state aiui.

> 
> There is this bit in the spec which we are ignoring:
> 
> ???If Number of Multisamples is not MULTISAMPLECOUNT_1, only 0/1 values
>  allowed???

This errata is pretty clearly labeled as early steppings in the places I am
looking. However, I do recall seeing some place where it wasn't labeled as such
and chalked that one up as the error. I suppose the easiest way to verify is the
check what the Windows driver does.

> 
> But that doesn't explain the problem because in this case we are
> clearing to white.
> 
> http://lists.freedesktop.org/archives/mesa-dev/2015-November/099274.html
> 
> Regards,
> - Neil
> 
> >
> > v2: Just reject the two failing types.
> >
> > Cc: Neil Roberts <neil at linux.intel.com
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> >  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c | 12 ++++++++++--
> >  src/mesa/drivers/dri/i965/gen8_surface_state.c  |  8 --------
> >  2 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > index 67dd22d..a024d02 100644
> > --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > @@ -360,10 +360,18 @@ is_color_fast_clear_compatible(struct brw_context *brw,
> >     }
> >  
> >     for (int i = 0; i < 4; i++) {
> > -      if (color->f[i] != 0.0f && color->f[i] != 1.0f &&
> > -          _mesa_format_has_color_component(format, i)) {
> > +      if (!_mesa_format_has_color_component(format, i)) {
> > +         continue;
> > +      }
> > +
> > +      if (brw->gen < 9 &&
> > +          color->f[i] != 0.0f && color->f[i] != 1.0f) {
> >           return false;
> >        }
> > +
> > +      if (format == MESA_FORMAT_L_FLOAT16 ||
> > +          format == MESA_FORMAT_I_FLOAT16)
> > +         return false;
> >     }
> >     return true;
> >  }
> > diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > index 8fe480c..2381c83 100644
> > --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> > @@ -188,14 +188,6 @@ gen8_emit_fast_clear_color(struct brw_context *brw,
> >                             uint32_t *surf)
> >  {
> >     if (brw->gen >= 9) {
> > -#define check_fast_clear_val(x) \
> > -      assert(mt->gen9_fast_clear_color.f[x] == 0.0 || \
> > -             mt->gen9_fast_clear_color.f[x] == 1.0)
> > -      check_fast_clear_val(0);
> > -      check_fast_clear_val(1);
> > -      check_fast_clear_val(2);
> > -      check_fast_clear_val(3);
> > -#undef check_fast_clear_val
> >        surf[12] = mt->gen9_fast_clear_color.ui[0];
> >        surf[13] = mt->gen9_fast_clear_color.ui[1];
> >        surf[14] = mt->gen9_fast_clear_color.ui[2];
> > -- 
> > 2.6.2