[Mesa-dev] [PATCH v2] i965: Handle lum, intensity and missing components in the fast clear

[Ben Widawsky]

On Thu, Nov 12, 2015 at 04:54:08PM +0100, Neil Roberts wrote:
> It looks like the sampler hardware doesn't take into account the
> surface format when sampling a cleared color after a fast clear has
> been done. So for example if you clear a GL_RED surface to 1,1,1,1
> then the sampling instructions will return 1,1,1,1 instead of 1,0,0,1.
> This patch makes it override the color that is programmed in the
> surface state in order to swizzle for luminance and intensity as well
> as overriding the missing components.
> 
> v2: Handle luminance and intensity formats
> ---
> 
> I made a more extensive test case which tests all of the formats in
> fbo_formats.h as well as using more than one test color here:
> 
> http://patchwork.freedesktop.org/patch/64578/
> 
> In the process I noticed that there is a similar problem with
> luminance and intensity textures so here is a v2 to cope with that.
> 
> I've made another version of this patch which is rebased on top of
> Ben's skl-fast-clear branch so we can take whichever version depending
> on which lands first:
> 
> https://github.com/bpeel/mesa/commit/da7edcb6dfd93c7dd86b2e148c44dff7
> 
>  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c | 38 +++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 3 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 69fe7b4..3071590 100644
> --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> @@ -361,12 +361,43 @@ is_color_fast_clear_compatible(struct brw_context *brw,
>   * SURFACE_STATE.
>   */
>  static uint32_t
> -compute_fast_clear_color_bits(const union gl_color_union *color)
> +compute_fast_clear_color_bits(mesa_format format,
> +                              const union gl_color_union *color)
>  {
> +   union gl_color_union override_color = *color;
>     uint32_t bits = 0;
> +
> +   /* The sampler doesn't look at the format of the surface when the fast
> +    * clear color is used so we need to implement luminance, intensity and
> +    * missing components manually.
> +    */
> +   switch (_mesa_get_format_base_format(format)) {
> +   case GL_INTENSITY:
> +      override_color.ui[3] = override_color.ui[0];
> +      /* flow through */
> +   case GL_LUMINANCE:
> +   case GL_LUMINANCE_ALPHA:
> +      override_color.ui[1] = override_color.ui[0];
> +      override_color.ui[2] = override_color.ui[0];
> +      break;

The definition for GL_LUMINANCE afaict:
"Each element is a single luminance value.  The GL converts it to
floating point, then assembles it into an RGBA element by replicating the
luminance value three times for red, green, and blue and attaching 1 for alpha.
Each component is then multiplied by the signed scale factor GL_c_SCALE, added
to the signed bias GL_c_BIAS, and clamped to the range [0,1] (see
glPixelTransfer)."

doesn't that mean you need
override_color.f[3] = 1.0f;

> +   default:
> +      for (int i = 0; i < 3; i++) {
> +         if (!_mesa_format_has_color_component(format, i))
> +            override_color.ui[i] = 0;
> +      }

Is there an easy way to verify that all formats want 0 for GB channels? It looks
right to me, but with my knowledge of GL, that doesn't mean much (I am looking
here: https://www.opengl.org/sdk/docs/man/html/glTexImage2D.xhtml)

I also think that component 0 must always have a color, right? (I'm not
requesting a change as such, just making sure my understanding of what you're
trying to do is correct).

> +      break;
> +   }
> +
> +   if (!_mesa_format_has_color_component(format, 3)) {
> +      if (_mesa_is_format_integer_color(format))
> +         override_color.ui[3] = 1;

We shouldn't ever be fast clearing integer formats. We can on GEN8+, but we're
not doing it today. So I think it should be safe to remove this check.

> +      else
> +         override_color.f[3] = 1.0f;
> +   }
> +
>     for (int i = 0; i < 4; i++) {
>        /* Testing for non-0 works for integer and float colors */
> -      if (color->f[i] != 0.0f)
> +      if (override_color.f[i] != 0.0f)
>           bits |= 1 << (GEN7_SURFACE_CLEAR_COLOR_SHIFT + (3 - i));
>     }
>     return bits;
> @@ -505,7 +536,8 @@ brw_meta_fast_clear(struct brw_context *brw, struct gl_framebuffer *fb,
>        switch (clear_type) {
>        case FAST_CLEAR:
>           irb->mt->fast_clear_color_value =
> -            compute_fast_clear_color_bits(&ctx->Color.ClearColor);
> +            compute_fast_clear_color_bits(irb->mt->format,
> +                                          &ctx->Color.ClearColor);
>           irb->need_downsample = true;
>  
>           /* If the buffer is already in INTEL_FAST_CLEAR_STATE_CLEAR, the

Seems like a good patch to me. It would probably be nice to track down a good
spec reference if you manage to find one. I know I've seen such reference in SKL
docs (which aren't SKL specific) - but I am having trouble finding it in PRMs.
My VPN is broken, so I can't look at SKL docs right now.

With the explanation of why the luminance alpha channel isn't 1 (I also claim
incompotence on the GL_LUMINANCE_ALPHA format):
Reviewed-by: Ben Widawsky <benjamin.widawsky at intel.com>