[Mesa-dev] [PATCH] i965: handle gl_PointCoord for Gen4 and Gen5 platform

[Eric Anholt]

On Fri, 17 Feb 2012 16:09:37 +0800, Yuanhan Liu <yuanhan.liu at linux.intel.com> wrote:
> This patch add the support of gl_PointCoord gl builtin var for platform
> gen4 and gen5(ILK).
> 
> We can get the point start coord and the current pixel coord, and the
> only left element needed is the point size. Thus I wrote another simple
> SF routine for that.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45975

You don't mention the piglit tests fixed.  Have you run piglit on this?
Does it fix glsl-fs-pointcoord, for example?

> ---
>   I am somehow not quite sure that this is the right way to implement
>   gl_PointCoord for gen4 and gen5 platform. But it does work on G45 and
>   ILK(the two platform I've tested, Zhao jian will help to test more
>   gen4 platforms).

> diff --git a/src/mesa/drivers/dri/i965/brw_sf.c b/src/mesa/drivers/dri/i965/brw_sf.c
> index 54c27f9..69c507a 100644
> --- a/src/mesa/drivers/dri/i965/brw_sf.c
> +++ b/src/mesa/drivers/dri/i965/brw_sf.c
> @@ -84,7 +84,9 @@ static void compile_sf_prog( struct brw_context *brw,
>        break;
>     case SF_POINTS:
>        c.nr_verts = 1;
> -      if (key->do_point_sprite)
> +      if (key->do_point_sprite && key->do_point_coord)
> +          brw_emit_point_coord_setup( &c, true );
> +      else if (key->do_point_sprite)
>  	  brw_emit_point_sprite_setup( &c, true );

If you have gl_PointCoord usage, is that suppposed to prevent point
sprite texture coordinate setup from working?  I wouldn't think so.

> +/*
> + * A special routine just for handling gl_PointCoord builtin var
> + *
> + * We don't handle the point coord origin here but instead at the
> + * fragment shader code generation time.
> + */
> +void brw_emit_point_coord_setup(struct brw_sf_compile *c, bool allocate)
> +{
> +   struct brw_compile *p = &c->func;
> +   GLuint i;
> +
> +   c->nr_verts = 1;
> +
> +   if (allocate)
> +      alloc_regs(c);
> +
> +   for (i = 0; i < c->nr_setup_regs; i++) {
> +      bool last = (i == c->nr_setup_regs - 1);
> +      /* Caculate 1.0/PointWidth */
> +      brw_math(&c->func,
> +               c->tmp,
> +               BRW_MATH_FUNCTION_INV,
> +               BRW_MATH_SATURATE_NONE,
> +               0,
> +               c->dx0,
> +               BRW_MATH_DATA_SCALAR,
> +               BRW_MATH_PRECISION_FULL);

You didn't set you predicate control, so I think you're doing extra math
here, which is very expensive on this platform.

> +
> +      brw_set_access_mode(p, BRW_ALIGN_16);
> +      /* dA/dx, dA/dy */
> +      brw_MOV(p, brw_writemask(c->m1Cx, WRITEMASK_X), c->tmp);
> +      brw_MOV(p, brw_writemask(c->m2Cy, WRITEMASK_Y), c->tmp);
> +
> +      /* attribute constant offset */
> +      brw_MOV(p, brw_writemask(c->m3C0, WRITEMASK_W), brw_imm_f(1.0));
> +      brw_set_access_mode(p, BRW_ALIGN_1);
> +
> +      /* Copy m0..m3 to URB. */
> +      brw_urb_WRITE(p,
> +                    brw_null_reg(),
> +                    0,
> +                    brw_vec8_grf(0, 0),
> +                    0, 	/* allocate */
> +                    1,	/* used */
> +                    4, 	/* msg len */
> +                    0,	/* response len */
> +                    last, 	/* eot */
> +                    last, 	/* writes complete */
> +                    i*4,	/* urb destination offset */
> +                    BRW_URB_SWIZZLE_TRANSPOSE);
> +   }
> +}

This whole function looks to me like it should really just be a
modification to brw_emit_point_sprite_setup that doesn't care about the
other two components when setting up FRAG_ATTRIB_POINTCOORD.

There's a more major problem here, that the new function is setting up
interpolation for every VUE element, and your FS code appears to just be
picking one varying from the SF's output urb entry and using that for
the source.  But all the varyings aren't supposed to interpolate like
that, they're supposed to interpolate like brw_emit_point_setup().  So
you need to actually allocate space for another attribute in the SF
output and put your pointcoord interpolation there, and then use that
From the FS.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120217/40e9cd28/attachment.pgp>