[Mesa-dev] [PATCH 3/5] i965/fs: Add support for textureGather(.., comp)

[Kenneth Graunke]

On 10/05/2013 03:38 AM, Chris Forbes wrote:
> - For HSW: Select the channel based on the component selected (swizzle
>   is done in HW)
> - For IVB: Select the channel based on the swizzle state for the
>   component selected. Only apply the RG32F w/a if we actually want
>   green -- we're about to flag it regardless of swizzle state.
> 
> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
> ---
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index 6141009..5508cdc 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -1475,7 +1475,8 @@ fs_visitor::visit(ir_texture *ir)
>        /* When tg4 is used with the degenerate ZERO/ONE swizzles, don't bother
>         * emitting anything other than setting up the constant result.
>         */
> -      int swiz = GET_SWZ(c->key.tex.swizzles[sampler], 0);
> +      ir_constant *chan = ir->lod_info.component->as_constant();
> +      int swiz = GET_SWZ(c->key.tex.swizzles[sampler], chan->value.i[0]);

Cool!  On my first pass through the spec I hadn't realized that the
channel parameter was required to be a constant integer expression, and
you indeed enforced that by using ir_var_const_in.  Makes life here so
much easier.

There's only one problem: right now, nothing enforces the constraint
that the component parameter must be 0, 1, 2, or 3.  Although it isn't
explicitly stated in the spec, I believe specifying another value should
be a compile error.  I think this has to get handled at the ast_to_hir
step, and can probably be an ugly special case for now.

It looks like currently, the bitshifts in GET_SWZ will probably return
0, resulting in the red channel being selected.  So there isn't an out
of bounds access problem...just undefined results.  A compile error is
definitely more friendly, though.


>        if (swiz == SWIZZLE_ZERO || swiz == SWIZZLE_ONE) {
>  
>           fs_reg res = fs_reg(this, glsl_type::vec4_type);
> @@ -1594,14 +1595,16 @@ fs_visitor::visit(ir_texture *ir)
>  uint32_t
>  fs_visitor::gather_channel(ir_texture *ir, int sampler)
>  {
> -   int swiz = GET_SWZ(c->key.tex.swizzles[sampler], 0 /* red */);
> -   if (c->key.tex.gather_channel_quirk_mask & (1<<sampler))
> -      return 2;   /* gather4 sampler is broken for green channel on RG32F --
> -                   * we must ask for blue instead.
> -                   */
> +   ir_constant *chan = ir->lod_info.component->as_constant();
> +   int swiz = GET_SWZ(c->key.tex.swizzles[sampler], chan->value.i[0] /* red */);

The /* red */ comment here doesn't make sense anymore...I'd drop it.

>     switch (swiz) {
>        case SWIZZLE_X: return 0;
> -      case SWIZZLE_Y: return 1;
> +      case SWIZZLE_Y:
> +         if (c->key.tex.gather_channel_quirk_mask & (1<<sampler))
> +            return 2;   /* gather4 sampler is broken for green channel on RG32F --
> +                         * we must ask for blue instead.
> +                         */
> +         return 1;

I would like to see the comment above the block, rather than off to the
side like this.  Ditto for the next patch.

>        case SWIZZLE_Z: return 2;
>        case SWIZZLE_W: return 3;
>        default:
> 

Patches 3-4 are:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

(since my comment about bounds checking 'comp' would go in a different
patch anyway)