On 09/13/2012 01:50 PM, Paul Berry wrote:
> On 10 September 2012 14:15, Kenneth Graunke <kenn...@whitecape.org
> <mailto: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
> <mailto: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().
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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
