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

[Liu Aleaxander]

On Sat, Feb 18, 2012 at 3:45 AM, Eric Anholt <eric at anholt.net> wrote:
> 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?

Sorry, forgot to test piglit. But, yes, it does fix glsl-fs-pointcoord
and fbo-gl_pointcoord.

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

Yeah, you are right. Thanks.
>
>> +/*
>> + * 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.

Thanks for that info.

>
>> +
>> +      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.

Indeed.

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

Yeah, you are right.  I made 2 new patches based on your comments.
Please help to review it. Appreciate your comments very much!!!

Thanks,
Yuanhan Liu