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

[Neil Roberts]

Ben Widawsky <ben at bwidawsk.net> writes:

>> +   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."
>
> doesn't that mean you need
> override_color.f[3] = 1.0f;

That is handled separately by the bit at the bottom which checks for
_mesa_format_has_color_component(format, 3). It's the same bit of code
that overrides the alpha channel for GL_RGB.

>> +   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)

In the GL 4.5 spec, section 15.2.1 it says “When a texture lookup is
performed in a fragment shader, the GL computes the filtered texture
value τ in the manner described in sections 8.14 and 8.15, and converts
it to a texture base color C b as shown in table 15.1”. Table 15.1 looks
something like this:

Texture Base       Texture base color
Internal Format      Cb         Ab
RED                (Rt, 0, 0)   1
RG                 (Rt, Gt, 0)  1
RGB                (Rt, Gt, Bt) 1
RGBA               (Rt, Gt, Bt) At

In the compatibility spec there is also the luminance, intensity and
alpha formats and they all have 0 for the missing RGB components. I also
tried running the Piglit test I wrote on the nvidia binary blob so if we
can assume that that tests all renderable formats then we can be
confident we at least match what nvidia do.

> 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).

As Ilia mentioned, GL_ALPHA doesn't have a red component.

>> +      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.

You're right, but I thought I'd add it anyway because I didn't think
this is a particularly hot code path and it might make life easier for
whoever eventually adds support for integer fast clears. I'm happy to
take it out thought if you think that'd be better.

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

I haven't been able to find anything yet sadly.

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

Many thanks for the review.

Regards,
- Neil