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

[Gurchetan Singh]

On Thu, May 17, 2018 at 4:32 PM Dave Airlie <airlied at gmail.com> wrote:

> > 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;
>          }
>

Ah, that makes sense  -- TGSI uses the remaining portion of src[0] if
(sizeof(GLSL arg1) + sizeof(GLSL arg2) <= sizeof(src[0])) and types match.


> 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.
>
>
Fair enough.


> Thanks,
> Dave.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/virglrenderer-devel/attachments/20180517/c0b102a2/attachment.html>