On 10 September 2012 14:15, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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 <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> --- 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. </blockquote><div> 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? </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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]); + } </blockquote><div> 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(). 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. 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. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> } /* Emit relocation to surface contents */ -- 1.7.11.4 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div>