[virglrenderer-devel] [PATCH 06/12] arb_gpu_shader5: add support for enhanced texture gather

[Dave Airlie]

> I assume we're trying to implement the shadow variants of textureGather:
>
> vec4 textureGather( gsampler2DShadow sampler, vec2 P, float refZ);
>
> TGSI specifies src[1], not src[0] should be source for the refZ argument.

you missed this line:
(with SM5 - cube array shadow)
>
>      coord = src0
>      compare = src1
>      dst = texture_gather (uint, coord, compare)
>
This only applies to the cube array shadow case which needs 5 values
so needs 2 srcs.

>
> The new case for TGSI_TEXTURE_SHADOWCUBE_ARRAY makes sense, but the older
> code does not.  Why are we using src[0] (especially with different
> swizzling depending on the shadow texture type) for some shadow cases?

The refZ will get stuck in the src0 Z or W depending on other things:
         if ((sampler_type->sampler_dimensionality == GLSL_SAMPLER_DIM_2D &&
              sampler_type->sampler_array) ||
             sampler_type->sampler_dimensionality == GLSL_SAMPLER_DIM_CUBE) {
            coord_dst.writemask = WRITEMASK_W;
         } else {
            coord_dst.writemask = WRITEMASK_Z;
         }
is the code from the state tracker.

>
>> +      } else if (tg4_has_component) {
>> +         if (inst->Texture.NumOffsets == 0) {
>> +            if (inst->Texture.Texture == TGSI_TEXTURE_2D ||
>> +                inst->Texture.Texture == TGSI_TEXTURE_RECT ||
>> +                inst->Texture.Texture == TGSI_TEXTURE_CUBE ||
>> +                inst->Texture.Texture == TGSI_TEXTURE_2D_ARRAY ||
>> +                inst->Texture.Texture == TGSI_TEXTURE_CUBE_ARRAY)
>> +               snprintf(bias, 64, ", int(%s)", srcs[1]);
>> +         } else if (inst->Texture.NumOffsets) {
>> +            if (inst->Texture.Texture == TGSI_TEXTURE_2D ||
>> +                inst->Texture.Texture == TGSI_TEXTURE_RECT ||
>> +                inst->Texture.Texture == TGSI_TEXTURE_2D_ARRAY)
>> +               snprintf(bias, 64, ", int(%s)", srcs[1]);
>> +         }
>
> The cases for inst->Texture.NumOffsets == 0 and inst->Texture.NumOffsets
> result in the same bias -- int(src[1]) -- do we need separate cases for
> them?
>

The result is the name, but the numoffsets case doesn't apply to
cube/cube_array,
so it might be possible to write it a bit cleaner, but I'm guessing it
could also end
up ugly writiing it in one if statement.

Thanks,
Dave.