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

Paul Berry stereotype441 at gmail.com
Thu Feb 21 06:56:24 PST 2013


On 16 February 2013 15:16, Paul Berry <stereotype441 at gmail.com> wrote:

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

After further investigation and testing, I have an additional reason to NAK
this patch.  It turns out that we also need for point coordinates to work
when rendering polygons and glPolygonMode() has been used to set either
front- or back-facing polygons to GL_POINT mode (there's even a Glean test
for this).  Since glPolygonMode() allows independent control of the
behaviour of front-facing and back-facing polygons, it isn't actually
possible to get correct rendering and still ensure that "Point Sprite
Texture Coordinate Enable" is 0 whenever non-point primitives are rendered.

Given that the code seems to work fine as is, I'm now inclined to just
leave well enough alone, unless someone else has a better idea.


>
>
>>
>>
>>       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/20130221/6b9de0a8/attachment-0001.html>


More information about the mesa-dev mailing list