[Mesa-dev] [PATCH 1/2] i915: set SPRITE_POINT_ENABLE bit correctly

[Eric Anholt]

On Mon, 12 Mar 2012 16:04:00 +0800, Yuanhan Liu <yuanhan.liu at linux.intel.com> wrote:
> 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).
> 
> And more,  we currently handle varying inputs as texture 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.
> 
> Thus we do set SPRITE_POINT_ENABLE bit only when all enabled tex
> coord units need do CoordReplace. Or fallback is needed to make
> sure the rendering is right.
> 
> As we need guarantee the CoordReplace changes handled well and
> be able to fallback when finding something wrong, I added another
> function to handle it at intelRunPipepline, where the drawing
> happened here and tnl pipeline hasn't started yet.
> 
> With handling the bit setup at intel_validate_sprite_point_enable(),
> we don't need the relative 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.
> 
> v2: fallback just when all enabled tex coord units need do
>     CoordReplace(Eric).
> 
> Signed-off-by: Yuanhan Liu <yuanhan.liu at linux.intel.com>
> ---
>  src/mesa/drivers/dri/i915/i915_context.h |    1 +
>  src/mesa/drivers/dri/i915/i915_state.c   |   13 +-------
>  src/mesa/drivers/dri/i915/intel_tris.c   |   52 ++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i915/i915_context.h b/src/mesa/drivers/dri/i915/i915_context.h
> index 8167137..59eeb6e 100644
> --- a/src/mesa/drivers/dri/i915/i915_context.h
> +++ b/src/mesa/drivers/dri/i915/i915_context.h
> @@ -40,6 +40,7 @@
>  #define I915_FALLBACK_POINT_SMOOTH	 0x80000
>  #define I915_FALLBACK_POINT_SPRITE_COORD_ORIGIN	 0x100000
>  #define I915_FALLBACK_DRAW_OFFSET	 0x200000
> +#define I915_FALLBACK_COORD_REPLACE	 0x400000
>  
>  #define I915_UPLOAD_CTX              0x1
>  #define I915_UPLOAD_BUFFERS          0x2
> diff --git a/src/mesa/drivers/dri/i915/i915_state.c b/src/mesa/drivers/dri/i915/i915_state.c
> index 756001f..3c751e4 100644
> --- a/src/mesa/drivers/dri/i915/i915_state.c
> +++ b/src/mesa/drivers/dri/i915/i915_state.c
> @@ -869,18 +869,7 @@ i915Enable(struct gl_context * ctx, GLenum cap, GLboolean state)
>        break;
>  
>     case GL_POINT_SPRITE:
> -      /* This state change is handled in i915_reduced_primitive_state because
> -       * the hardware bit should only be set when rendering points.
> -       */
> -	 dw = i915->state.Ctx[I915_CTXREG_LIS4];
> -      if (state)
> -	 dw |= S4_SPRITE_POINT_ENABLE;
> -      else
> -	 dw &= ~S4_SPRITE_POINT_ENABLE;
> -      if (dw != i915->state.Ctx[I915_CTXREG_LIS4]) {
> -	 i915->state.Ctx[I915_CTXREG_LIS4] = dw;
> -	 I915_STATECHANGE(i915, I915_UPLOAD_CTX);
> -      }
> +      /* Handle it at intel_validate_sprite_point_enable() */
>        break;
>  
>     case GL_POINT_SMOOTH:
> diff --git a/src/mesa/drivers/dri/i915/intel_tris.c b/src/mesa/drivers/dri/i915/intel_tris.c
> index a36011a..58f6a59 100644
> --- a/src/mesa/drivers/dri/i915/intel_tris.c
> +++ b/src/mesa/drivers/dri/i915/intel_tris.c
> @@ -1052,6 +1052,48 @@ static const GLenum reduced_prim[GL_POLYGON + 1] = {
>     GL_TRIANGLES
>  };
>  
> +static void
> +intel_validate_sprite_point_enable(struct intel_context *intel)
> +{
> +   struct gl_context *ctx = &intel->ctx;
> +   struct i915_fragment_program *p =
> +      (struct i915_fragment_program *) ctx->FragmentProgram._Current;
> +   const GLbitfield64 inputsRead = p->FragProg.Base.InputsRead;
> +   struct i915_context *i915 = i915_context(ctx);
> +   GLuint s4 = i915->state.Ctx[I915_CTXREG_LIS4] & ~S4_VFMT_MASK;
> +   int i;
> +   GLuint coord_replace_bits = 0x0;
> +   GLuint tex_coord_unit_bits = 0x0;
> +
> +   for (i = 0; i < ctx->Const.MaxTextureCoordUnits; i++) {
> +      if (ctx->Point.CoordReplace[i] && ctx->Point.PointSprite)
> +         coord_replace_bits |= (1 << i);
> +      if (inputsRead & FRAG_BIT_TEX(i))
> +         tex_coord_unit_bits |= (1 << i);
> +   }

This ignores varyings assigned to texture coordinate inputs.

> +
> +   s4 &= ~S4_SPRITE_POINT_ENABLE;
> +   if (coord_replace_bits) {
> +      if (coord_replace_bits != tex_coord_unit_bits) {
> +         /*
> +          * Here we can't enable the SPRITE_POINT_ENABLE bit due to the
> +          * mis-match of tex_coord_unit_bits and coord_replace_bits, or
> +          * this will make all the other non-point-sprite coords be
> +          * replaced to value (0, 0)-(1, 1).
> +          *
> +          * Thus, a fallback is needed.
> +          */
> +         FALLBACK(intel, I915_FALLBACK_COORD_REPLACE, true);

Here, if you ever fall back for coordinate replacement, you'll never
exit software fallbacks.

>  /**********************************************************************/
>  /*                 High level hooks for t_vb_render.c                 */
> @@ -1070,6 +1112,15 @@ intelRunPipeline(struct gl_context * ctx)
>     if (ctx->NewState)
>        _mesa_update_state_locked(ctx);
>  
> +   /*
> +    * Enable POINT_SPRITE_ENABLE bit when needed here
> +    *
> +    * Handle it at _draw_ time so that we can guarantee the CoordReplace
> +    * changes handled well. And we must do it before the tnl pipeline is
> +    * running so that we can fallback when finding something goes wrong.
> +    */
> +   intel_validate_sprite_point_enable(intel);

Other computed state happens in i915InvalidateState.  Why does this one
go here?  I'm also pretty sure this is also being called on i830.
-------------- 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/20120316/73a17dc9/attachment.pgp>