[Mesa-dev] [PATCH] i965: Fix textureGather with RG32I/UI on Gen7.

[Jordan Justen]

On 2017-01-05 02:51:38, Kenneth Graunke wrote:
> According to the "Gather4 R32G32_FLOAT Bug" internal documentation
> page, the R32G32_UINT and R32G32_SINT formats are affected by the
> same bug as R32G32_FLOAT.  Applying the same workarounds should be
> viable - apparently the R32G32_FLOAT_LD format shouldn't corrupt
> integer data which is NaN or other sketchy floating point values.
> 
> One irritating caveat is that, because it's a FLOAT format, the
> alpha channel or any set to SCS_ONE return 0x3f8 (1.0) rather than
> integer 1.  So we need shader code to whack those channels to 1.
> 
> Fixes GL45-CTS.texture_gather.plain-gather-int-cube-rg on Haswell.
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_wm.c               | 41 ++++++++++++++++++++----
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  4 ++-
>  2 files changed, 37 insertions(+), 8 deletions(-)
> 
> Technically the brw_wm_surface_state.c hunk is enough to pass this test.
> 
> Asking for a dead channel (alpha) or constant value (one) with texture
> gather (which only gives you one channel's worth of data) is fairly
> pointless, so I don't expect anyone would actually do it...but, I tried
> to fix it with the brw_wm.c hunks.  Probably needs tests...
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c
> index b0cd1635521..024e948391e 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm.c
> @@ -353,13 +353,40 @@ brw_populate_sampler_prog_key_data(struct gl_context *ctx,
>                 key->gl_clamp_mask[2] |= 1 << s;
>           }
>  
> -         /* gather4's channel select for green from RG32F is broken; requires
> -          * a shader w/a on IVB; fixable with just SCS on HSW.
> -          */
> -         if (brw->gen == 7 && !brw->is_haswell &&
> -             prog->nir->info->uses_texture_gather) {
> -            if (img->InternalFormat == GL_RG32F)
> -               key->gather_channel_quirk_mask |= 1 << s;
> +         /* gather4 for RG32* is broken in multiple ways on Gen7. */
> +         if (brw->gen == 7 && prog->nir->info->uses_texture_gather) {
> +            switch (img->InternalFormat) {
> +            case GL_RG32I:
> +            case GL_RG32UI: {
> +               /* We have to override the format to R32G32_FLOAT_LD.
> +                * This means that SCS_ALPHA and SCS_ONE will return 0x3f8
> +                * (1.0) rather than integer 1.  This needs shader hacks.
> +                *
> +                * On Ivybridge, we whack W (alpha) to ONE in our key's
> +                * swizzle.  On Haswell, we look at the original texture
> +                * swizzle, and use XYZW with channels overridden to ONE,
> +                * leaving normal texture swizzling to SCS.
> +                */
> +               unsigned src_swizzle =
> +                  brw->is_haswell ? t->_Swizzle : key->swizzles[s];
> +               for (int i = 0; i < 4; i++) {
> +                  unsigned src_comp = GET_SWZ(src_swizzle, i);
> +                  if (src_comp == SWIZZLE_ONE || src_comp == SWIZZLE_W) {
> +                     key->swizzles[i] &= 0x7 << (3 * i);

Looks like the rhs should be inverted.

Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>

> +                     key->swizzles[i] |= SWIZZLE_ONE << (3 * i);
> +                  }
> +               }
> +               /* fallthrough */
> +            }
> +            case GL_RG32F:
> +               /* The channel select for green doesn't work - we have to
> +                * request blue.  Haswell can use SCS for this, but Ivybridge
> +                * needs a shader workaround.
> +                */
> +               if (!brw->is_haswell)
> +                  key->gather_channel_quirk_mask |= 1 << s;
> +               break;
> +            }
>           }
>  
>           /* Gen6's gather4 is broken for UINT/SINT; we treat them as
> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> index 6c44381be13..7a99aa434bf 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> @@ -565,7 +565,9 @@ brw_update_texture_surface(struct gl_context *ctx,
>        /* Implement gen6 and gen7 gather work-around */
>        bool need_green_to_blue = false;
>        if (for_gather) {
> -         if (brw->gen == 7 && format == BRW_SURFACEFORMAT_R32G32_FLOAT) {
> +         if (brw->gen == 7 && (format == BRW_SURFACEFORMAT_R32G32_FLOAT ||
> +                               format == BRW_SURFACEFORMAT_R32G32_SINT ||
> +                               format == BRW_SURFACEFORMAT_R32G32_UINT)) {
>              format = BRW_SURFACEFORMAT_R32G32_FLOAT_LD;
>              need_green_to_blue = brw->is_haswell;
>           } else if (brw->gen == 6) {
> -- 
> 2.11.0
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev