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

Fri Mar 16 19:58:27 PDT 2012

On Sat, Mar 17, 2012 at 1:57 AM, Eric Anholt <eric at anholt.net> wrote:
> 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.

Yes, and I did it on purpose, since we want to check the mis-match
case of coord_replace_bits and tex_coord_unit_bits. You can see the
following comments why I did this.

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

Yeah, you are right. Will fix it.

>
>>  /**********************************************************************/
>>  /*                 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?

A nice point. Yeah, I should do the stuff there.

So, how about the following patch

(note: I haven't tested the patch yet since I don't have hardware for
testing at home, but it should work ;)
(send from my gmail account, the format may not good, sorry for that)

---