[Mesa-dev] [PATCH] i915: fallback when point sprite is enabled while handling varying inputs

[Yuanhan Liu]

On Fri, Mar 09, 2012 at 10:35:33AM -0800, Eric Anholt wrote:
> On Thu, 8 Mar 2012 19:21:23 +0800, Yuanhan Liu <yuanhan.liu at linux.intel.com> wrote:
> > From ddd1a9d8f0d82c2f5fcb78a471608a005a6a077c Mon Sep 17 00:00:00 2001
> > From: Yuanhan Liu <yuanhan.liu at linux.intel.com>
> > Date: Thu, 8 Mar 2012 18:48:54 +0800
> > Subject: [PATCH] i915: set SPRITE_POINT_ENABLE bit just when we need do coord
> >  replace
> > 
> > When SPRITE_POINT_ENABLE bit is set, the texture coord would be
> > replaced, and this is only needed when we called something like
> > glTexEnvi(GL_POINT_SPRITE, GL_COORD_REPLACE, GL_TRUE).
> > 
> > Since we currently handling varying inputs as tex coord, we would be
> > careful when setting this bit and set it just when needed, or you will
> > find the value of varying input is not right and changed.
> > 
> > With handling the bit setup at i915ValidateFragmentProgram, we don't
> > need the code at i915Enable then.
> > 
> > This patch would _really_ fix the webglc point-size.html test case and
> > of course, not regress piglit point-sprite and glean-pointSprite testcase.
> > 
> > NOTE: This is a candidate for stable release branches.
> > 
> > Signed-off-by: Yuanhan Liu <yuanhan.liu at linux.intel.com>
> > ---
> >  src/mesa/drivers/dri/i915/i915_fragprog.c |    5 +++++
> >  src/mesa/drivers/dri/i915/i915_state.c    |   13 +------------
> >  2 files changed, 6 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i915/i915_fragprog.c b/src/mesa/drivers/dri/i915/i915_fragprog.c
> > index 5b7e93e..8829e8d 100644
> > --- a/src/mesa/drivers/dri/i915/i915_fragprog.c
> > +++ b/src/mesa/drivers/dri/i915/i915_fragprog.c
> > @@ -1379,7 +1379,12 @@ i915ValidateFragmentProgram(struct i915_context *i915)
> >        EMIT_ATTR(_TNL_ATTRIB_FOG, EMIT_1F, S4_VFMT_FOG_PARAM, 4);
> >     }
> >  
> > +   s4 &= ~S4_SPRITE_POINT_ENABLE;
> >     for (i = 0; i < p->ctx->Const.MaxTextureCoordUnits; i++) {
> > +      /* Do set SPRITE_POINT_ENABLE bit when we need do coord replace */
> > +      if (ctx->Point.CoordReplace[i] && ctx->Point.PointSprite)
> > +         s4 |= S4_SPRITE_POINT_ENABLE;
> > +
> >        if (inputsRead & FRAG_BIT_TEX(i)) {
> >           int sz = VB->AttribPtr[_TNL_ATTRIB_TEX0 + i]->size;
> 
> I don't think you've done anything to guarantee that this code is called
> when CoordReplace changes.

Yes, you are right. Maybe we can do it at the final _draw_ time, say
i915_reduced_primitive_state?

> 
> A more general problem: you're turning on point sprite if coord replace
> is set on any texcoord.  i915 replaces all texcoords with (0,0)-(1,1)
> when point sprite is enabled, which breaks any non-point-sprite
> coordinates, plus varyings as you noted. 

Well, I guess that's also what the current code do. Since
SPRITE_POINT_ENABLE is just _one_ bit instead of a set of mask bits.

> If you need point sprite
> coordinates and actual texcoords, a fallback should be done.

Yes, agreed acoording to the above state.

> 
> (Well, if we did better compiling, we could route a couple of varyings
> through color/secondarycolor while still getting point sprite
> coordinates on the texcoords)

That's a good hint. But I'd like to do the fallback first. Since I don't
know how many works should be done to get the better compiling.

Thanks,
Yuanhan Liu