[Mesa-dev] [PATCH 1/2] i965: Disable point coords when not rendering points.

[Paul Berry]

On 16 February 2013 13:52, Kenneth Graunke <kenneth at whitecape.org> wrote:

> On 02/16/2013 07:29 AM, Paul Berry wrote:
>
>>  From the Ivy Bridge PRM, p268 (3DSTATE_SBE), in the description of
>> "Point Sprite Texture Coordinate Enable":
>>
>>      "This field must be programmed to 0 when non-point primitives are
>>      rendered."
>>
>> A similar admonishment exists in the Sandy Bridge PRM.
>>
>> Although I'm not aware of any known bugs due to us ignoring this,
>> defying the hardware docs is generally unwise.
>> ---
>>   src/mesa/drivers/dri/i965/**gen6_sf_state.c | 9 ++++++---
>>   src/mesa/drivers/dri/i965/**gen7_sf_state.c | 9 ++++++---
>>   2 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/**gen6_sf_state.c
>> b/src/mesa/drivers/dri/i965/**gen6_sf_state.c
>> index 7071acc..d88c49a 100644
>> --- a/src/mesa/drivers/dri/i965/**gen6_sf_state.c
>> +++ b/src/mesa/drivers/dri/i965/**gen6_sf_state.c
>> @@ -127,6 +127,8 @@ upload_sf_state(struct brw_context *brw)
>>      /* _NEW_BUFFER */
>>      bool render_to_fbo = _mesa_is_user_fbo(brw->intel.**ctx.DrawBuffer);
>>      bool multisampled_fbo = ctx->DrawBuffer->Visual.**samples > 1;
>> +   /* BRW_NEW_REDUCED_PRIMITIVE */
>> +   bool is_point_primitive = intel->reduced_primitive == GL_POINTS;
>>
>
> NAK.  intel->reduced_primitive is only set in brw_set_prim(), which is
> never called on Gen6+.  (gen6_set_prim() gets called instead.)
>

Oops, you're right.


>
> I believe you want brw->primitive and BRW_NEW_PRIMITIVE instead.


The disadvantage of using brw->primitive and BRW_NEW_PRIMITIVE is that it
means this packet will get re-emitted whenever the primitive type changes.
If reduced_primitive were available, then this packet would only be
re-emitted when switching between triangle-like, line-like, and point
primitives (which probably happens a lot less frequently).

What would you think if instead I ported the reduced_primitive code to
gen6+?  (It's only a couple of lines).


>
>
>       int attr = 0, input_index = 0;
>>      int urb_entry_read_offset = 1;
>> @@ -280,13 +282,13 @@ upload_sf_state(struct brw_context *brw)
>>          continue;
>>
>>         /* _NEW_POINT */
>> -      if (ctx->Point.PointSprite &&
>> +      if (is_point_primitive && ctx->Point.PointSprite &&
>>           (attr >= FRAG_ATTRIB_TEX0 && attr <= FRAG_ATTRIB_TEX7) &&
>>           ctx->Point.CoordReplace[attr - FRAG_ATTRIB_TEX0]) {
>>          dw16 |= (1 << input_index);
>>         }
>>
>> -      if (attr == FRAG_ATTRIB_PNTC)
>> +      if (is_point_primitive && attr == FRAG_ATTRIB_PNTC)
>>          dw16 |= (1 << input_index);
>>
>>         /* flat shading */
>> @@ -360,7 +362,8 @@ const struct brw_tracked_state gen6_sf_state = {
>>                 _NEW_POINT |
>>                   _NEW_MULTISAMPLE),
>>         .brw   = (BRW_NEW_CONTEXT |
>> -               BRW_NEW_FRAGMENT_PROGRAM),
>> +               BRW_NEW_FRAGMENT_PROGRAM |
>> +                BRW_NEW_REDUCED_PRIMITIVE),
>>         .cache = CACHE_NEW_VS_PROG
>>      },
>>      .emit = upload_sf_state,
>> diff --git a/src/mesa/drivers/dri/i965/**gen7_sf_state.c
>> b/src/mesa/drivers/dri/i965/**gen7_sf_state.c
>> index 9171eff..0f6252b 100644
>> --- a/src/mesa/drivers/dri/i965/**gen7_sf_state.c
>> +++ b/src/mesa/drivers/dri/i965/**gen7_sf_state.c
>> @@ -46,6 +46,8 @@ upload_sbe_state(struct brw_context *brw)
>>      /* _NEW_BUFFERS */
>>      bool render_to_fbo = _mesa_is_user_fbo(ctx->**DrawBuffer);
>>      uint32_t point_sprite_origin;
>> +   /* BRW_NEW_REDUCED_PRIMITIVE */
>> +   bool is_point_primitive = intel->reduced_primitive == GL_POINTS;
>>
>>      /* FINISHME: Attribute Swizzle Control Mode? */
>>      dw1 = GEN7_SBE_SWIZZLE_ENABLE | num_outputs <<
>> GEN7_SBE_NUM_OUTPUTS_SHIFT;
>> @@ -78,13 +80,13 @@ upload_sbe_state(struct brw_context *brw)
>>         if (!(brw->fragment_program->**Base.InputsRead &
>> BITFIELD64_BIT(attr)))
>>          continue;
>>
>> -      if (ctx->Point.PointSprite &&
>> +      if (is_point_primitive && ctx->Point.PointSprite &&
>>           attr >= FRAG_ATTRIB_TEX0 && attr <= FRAG_ATTRIB_TEX7 &&
>>           ctx->Point.CoordReplace[attr - FRAG_ATTRIB_TEX0]) {
>>          dw10 |= (1 << input_index);
>>         }
>>
>> -      if (attr == FRAG_ATTRIB_PNTC)
>> +      if (is_point_primitive && attr == FRAG_ATTRIB_PNTC)
>>          dw10 |= (1 << input_index);
>>
>>         /* flat shading */
>> @@ -149,7 +151,8 @@ const struct brw_tracked_state gen7_sbe_state = {
>>                 _NEW_POINT |
>>                 _NEW_PROGRAM),
>>         .brw   = (BRW_NEW_CONTEXT |
>> -               BRW_NEW_FRAGMENT_PROGRAM),
>> +               BRW_NEW_FRAGMENT_PROGRAM |
>> +                BRW_NEW_REDUCED_PRIMITIVE),
>>         .cache = CACHE_NEW_VS_PROG
>>      },
>>      .emit = upload_sbe_state,
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130216/b80ff953/attachment-0001.html>