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

[Yuanhan Liu]

On Mon, Mar 12, 2012 at 10:14:06AM +0800, Yuanhan Liu wrote:
> 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?

Acutally, it seems we can guarantee that: since i915ValidateFragmentProgram
is called at the _draw_ time, which makes sure the previous call of
CoordReplace is handled. Right?

Anyway, I made another 2 patches, please help to review it.

Thanks,
Yuanhan Liu


> 
> > 
> > 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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev