[Mesa-dev] [PATCH 2/3] i965: Do texture swizzling in hardware on Haswell.

Kenneth Graunke kenneth at whitecape.org
Fri Sep 14 02:59:01 PDT 2012


On 09/13/2012 01:50 PM, Paul Berry wrote:
> On 10 September 2012 14:15, Kenneth Graunke <kenneth at whitecape.org
> <mailto:kenneth at whitecape.org>> wrote:
> 
>     Haswell supports EXT_texture_swizzle and legacy DEPTH_TEXTURE_MODE
>     swizzling by setting SURFACE_STATE entries.  This means we don't have to
>     bake the swizzle settings into the shader code by emitting MOV
>     instructions, and thus don't have to recompile shaders whenever the
>     swizzles change.
> 
>     Signed-off-by: Kenneth Graunke <kenneth at whitecape.org
>     <mailto:kenneth at whitecape.org>>
>     ---
>      src/mesa/drivers/dri/i965/brw_wm.c                |  4 +-
>      src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 65
>     +++++++++++++++++++++--
>      2 files changed, 64 insertions(+), 5 deletions(-)
> 
>     This actually regresses two piglit tests which set DEPTH_TEXTURE_MODE to
>     GL_ALPHA and use GLSL 1.30+ style (float return type) shadow sampling.
>     That's fixed in the next patch; I kept it separate because I felt it was
>     easier to review this way.
> 
> 
> I'm ok with that.  Would you mind moving this note into the body of the
> commit message so that in case someone stumbles on this temporary
> regression during a git bisect, they will know what's going on?
>  
> 
> 
>     diff --git a/src/mesa/drivers/dri/i965/brw_wm.c
>     b/src/mesa/drivers/dri/i965/brw_wm.c
>     index d991300..55fb14a 100644
>     --- a/src/mesa/drivers/dri/i965/brw_wm.c
>     +++ b/src/mesa/drivers/dri/i965/brw_wm.c
>     @@ -491,6 +491,8 @@ brw_populate_sampler_prog_key_data(struct
>     gl_context *ctx,
>                                        const struct gl_program *prog,
>                                        struct brw_sampler_prog_key_data
>     *key)
>      {
>     +   struct intel_context *intel = intel_context(ctx);
>     +
>         for (int s = 0; s < MAX_SAMPLERS; s++) {
>            key->swizzles[s] = SWIZZLE_NOOP;
> 
>     @@ -509,7 +511,7 @@ brw_populate_sampler_prog_key_data(struct
>     gl_context *ctx,
>            /* Handle EXT_texture_swizzle and DEPTH_TEXTURE_MODE
>     swizzling for
>             * hardware that doesn't natively support it.
>             */
>     -      if (true) {
>     +      if (!intel->is_haswell) {
>              int swizzles[SWIZZLE_NIL + 1] = {
>                 SWIZZLE_X,
>                 SWIZZLE_Y,
>     diff --git a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
>     b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
>     index 97ae0e2..5e71260 100644
>     --- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
>     +++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
>     @@ -35,6 +35,32 @@
>      #include "brw_defines.h"
>      #include "brw_wm.h"
> 
>     +/**
>     + * Convert an EXT_texture_swizzle enum (i.e. GL_RED) to one of the
>     Gen7.5+
>     + * "Shader Channel Select" enumerations (i.e. HSW_SCS_RED)
>     + */
>     +static unsigned
>     +swizzle_to_scs(GLenum swizzle)
>     +{
>     +   switch (swizzle) {
>     +   case GL_RED:
>     +      return HSW_SCS_RED;
>     +   case GL_GREEN:
>     +      return HSW_SCS_GREEN;
>     +   case GL_BLUE:
>     +      return HSW_SCS_BLUE;
>     +   case GL_ALPHA:
>     +      return HSW_SCS_ALPHA;
>     +   case GL_ZERO:
>     +      return HSW_SCS_ZERO;
>     +   case GL_ONE:
>     +      return HSW_SCS_ONE;
>     +   }
>     +
>     +   assert(!"Should not get here: invalid swizzle mode");
>     +   return HSW_SCS_ZERO;
>     +}
>     +
>      void
>      gen7_set_surface_tiling(struct gen7_surface_state *surf, uint32_t
>     tiling)
>      {
>     @@ -343,10 +369,41 @@ gen7_update_texture_surface(struct gl_context
>     *ctx,
>          */
> 
>         if (brw->intel.is_haswell) {
>     -      surf->ss7.shader_channel_select_r = HSW_SCS_RED;
>     -      surf->ss7.shader_channel_select_g = HSW_SCS_GREEN;
>     -      surf->ss7.shader_channel_select_b = HSW_SCS_BLUE;
>     -      surf->ss7.shader_channel_select_a = HSW_SCS_ALPHA;
>     +      if (firstImage->_BaseFormat == GL_DEPTH_COMPONENT ||
>     +          firstImage->_BaseFormat == GL_DEPTH_STENCIL) {
>     +         /* Handle legacy DEPTH_TEXTURE_MODE swizzling. */
>     +         switch (tObj->DepthMode) {
>     +         case GL_ALPHA:
>     +            surf->ss7.shader_channel_select_r = HSW_SCS_ZERO;
>     +            surf->ss7.shader_channel_select_g = HSW_SCS_ZERO;
>     +            surf->ss7.shader_channel_select_b = HSW_SCS_ZERO;
>     +            surf->ss7.shader_channel_select_a = HSW_SCS_RED;
>     +            break;
>     +         case GL_LUMINANCE:
>     +            surf->ss7.shader_channel_select_r = HSW_SCS_RED;
>     +            surf->ss7.shader_channel_select_g = HSW_SCS_RED;
>     +            surf->ss7.shader_channel_select_b = HSW_SCS_RED;
>     +            surf->ss7.shader_channel_select_a = HSW_SCS_ONE;
>     +            break;
>     +         case GL_INTENSITY:
>     +            surf->ss7.shader_channel_select_r = HSW_SCS_RED;
>     +            surf->ss7.shader_channel_select_g = HSW_SCS_RED;
>     +            surf->ss7.shader_channel_select_b = HSW_SCS_RED;
>     +            surf->ss7.shader_channel_select_a = HSW_SCS_RED;
>     +            break;
>     +         case GL_RED:
>     +            surf->ss7.shader_channel_select_r = HSW_SCS_RED;
>     +            surf->ss7.shader_channel_select_g = HSW_SCS_ZERO;
>     +            surf->ss7.shader_channel_select_b = HSW_SCS_ZERO;
>     +            surf->ss7.shader_channel_select_a = HSW_SCS_ONE;
>     +            break;
>     +         }
>     +      } else {
>     +         surf->ss7.shader_channel_select_r =
>     swizzle_to_scs(tObj->Swizzle[0]);
>     +         surf->ss7.shader_channel_select_g =
>     swizzle_to_scs(tObj->Swizzle[1]);
>     +         surf->ss7.shader_channel_select_b =
>     swizzle_to_scs(tObj->Swizzle[2]);
>     +         surf->ss7.shader_channel_select_a =
>     swizzle_to_scs(tObj->Swizzle[3]);
>     +      }
> 
> 
> I'm bothered by the duplication in logic between this code and the code
> that sets up the swizles array in brw_populate_sampler_prog_key_data().

Yeah, me too.  I'll try and come up with a patch that shares more code.

> I'm also bothered by the fact that this code refers to
> gl_texture_object::Swizzle, whereas the code in
> brw_populate_sampler_prog_key_data() uses gl_texture_object::_Swizzle.

They're the same information, just in different forms.  Swizzle is
GL_RED/GL_GREEN/GL_BLUE/GL_ALPHA enums while _Swizzle is the
prog_instruction.h SWIZZLE_X style enums.  It doesn't matter which you
use...it should just be whichever is more convenient.

> Can we refactor the common logic into a single function that we can call
> from both places?
> 
> Also, in our discussion in person, it sounded like you suspected this
> code would be broken for the combination of EXT_texture_swizzle and
> depth textures.

Yeah.  As I just realized today, my patches completely fail to apply the
EXT_texture_swizzle swizzle /on top/ of the DEPTH_TEXTURE_MODE swizzle:

>From the EXT_texture_swizzle spec:

    4) How does this interact with depth component textures?

    RESOLVED: The swizzle is applied after the DEPTH_TEXTURE_MODE. This
    naturally falls out of specifying the swizzle in terms of Table 3.20.

I hadn't realized it did that.  Oops.  NAK on these patches.

I think fixing that will make me require the rest of the code I didn't
duplicate, so I'll probably end up fixing the duplication in the process.


More information about the mesa-dev mailing list