On 10 September 2012 14:15, Kenneth Graunke <kenn...@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 <kenn...@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().
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.
> }
>
> /* Emit relocation to surface contents */
> --
> 1.7.11.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
