[Mesa-stable] [Mesa-dev] [PATCH] i965/Gen6-7: Do not replace texcoords with point coord if not drawing points

Chris Forbes chrisf at ijw.co.nz
Tue Nov 25 02:01:37 PST 2014


Changes done and pushed.

On Tue, Nov 25, 2014 at 10:17 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Tuesday, November 25, 2014 10:05:18 PM Chris Forbes wrote:
>> Fixes broken rendering in Windows-based QtQuick2 apps run through Wine.
>> This library sets all texture units' GL_COORD_REPLACE, leaves point
>> sprite mode enabled, and then draws a triangle fan.
>>
>> Will need a slightly different fix for Gen4-5, but I don't have my old
>> machines in a usable state currently.
>>
>> V2: - Simplify patch -- the real changes are no longer duplicated across
>>       the Gen6 and Gen7 atoms.
>>     - Also don't clobber attr overrides -- which matters on Haswell too,
>>       and fixes the other half of the problem
>>     - Fix newly-introduced warnings
>>
>> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
>> Cc: "10.4" <mesa-stable at lists.freedesktop.org>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84651
>> ---
>>  src/mesa/drivers/dri/i965/gen6_sf_state.c | 54 ++++++++++++++++++++++++-------
>>  src/mesa/drivers/dri/i965/gen7_sf_state.c |  3 +-
>>  2 files changed, 45 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c b/src/mesa/drivers/dri/i965/gen6_sf_state.c
>> index 24d2754..778c6d3 100644
>> --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
>> @@ -129,6 +129,18 @@ get_attr_override(const struct brw_vue_map *vue_map, int urb_entry_read_offset,
>>  }
>>
>>
>> +static bool
>> +is_drawing_points(const struct brw_context *brw)
>> +{
>> +   /* Determine if the primitives *reaching the SF* are points */
>> +   const struct gl_context *ctx = &brw->ctx;
>
> It doesn't seem like BRW_NEW_PRIMITIVE and BRW_NEW_VUE_MAP_GEOM_OUT
> are enough to cover the use of OutputType.  I think you want:
>
>       if (brw->geometry_program) {
>           /* BRW_NEW_GEOMETRY_PROGRAM */
>           return brw->geometry_program->OutputType == GL_POINTS;
>       } else {
>           /* BRW_NEW_PRIMITIVE */
>           return brw->primitive == _3DPRIM_POINTLIST;
>       }
>
> and then include BRW_NEW_GEOMETRY_PROGRAM in the atom's dirty flags.
>
> (Using brw->geometry_program should be equivalent to
> ctx->GeometryProgram._Current, but we may as well use the brw copy for
> consistency with the rest of the code.  It's also more obviously connected
> to the BRW_NEW_GEOMETRY_PROGRAM flag.)
>
>> +   if (ctx->GeometryProgram._Current)
>> +      return ctx->GeometryProgram._Current->OutputType == GL_POINTS;
>> +   else
>> +      return brw->primitive == _3DPRIM_POINTLIST;
>> +}
>> +
>> +
>>  /**
>>   * Create the mapping from the FS inputs we produce to the previous pipeline
>>   * stage (GS or VS) outputs they source from.
>> @@ -149,6 +161,23 @@ calculate_attr_overrides(const struct brw_context *brw,
>>     /* _NEW_LIGHT */
>>     bool shade_model_flat = brw->ctx.Light.ShadeModel == GL_FLAT;
>>
>> +   /* From the Ivybridge PRM, Vol 2 Part 1, 3DSTATE_SBE,
>> +    * description of dw10 Point Sprite Texture Coordinate Enable:
>> +    *
>> +    * "This field must be programmed to zero when non-point primitives
>> +    * are rendered."
>> +    *
>> +    * The SandyBridge PRM doesn't explicitly say that point sprite enables
>> +    * must be programmed to zero when rendering non-point primitives, but
>> +    * the IvyBridge PRM does, and if we don't, we get garbage.
>> +    *
>> +    * This is not required on Haswell, as the hardware ignores this state
>> +    * when drawing non-points -- although we do still need to be careful to
>> +    * correctly set the attr overrides.
>> +    */
>> +   /* BRW_NEW_PRIMITIVE | _NEW_PROGRAM */
>> +   bool drawing_points = is_drawing_points(brw);
>> +
>>     /* Initialize all the attr_overrides to 0.  In the loop below we'll modify
>>      * just the ones that correspond to inputs used by the fs.
>>      */
>> @@ -167,18 +196,20 @@ calculate_attr_overrides(const struct brw_context *brw,
>>
>>        /* _NEW_POINT */
>>        bool point_sprite = false;
>> -      if (brw->ctx.Point.PointSprite &&
>> -       (attr >= VARYING_SLOT_TEX0 && attr <= VARYING_SLOT_TEX7) &&
>> -       brw->ctx.Point.CoordReplace[attr - VARYING_SLOT_TEX0]) {
>> -         point_sprite = true;
>> +      if (drawing_points) {
>> +         if (brw->ctx.Point.PointSprite &&
>> +             (attr >= VARYING_SLOT_TEX0 && attr <= VARYING_SLOT_TEX7) &&
>> +             brw->ctx.Point.CoordReplace[attr - VARYING_SLOT_TEX0]) {
>> +            point_sprite = true;
>> +         }
>> +
>> +         if (attr == VARYING_SLOT_PNTC)
>> +            point_sprite = true;
>> +
>> +         if (point_sprite)
>> +            *point_sprite_enables |= (1 << input_index);
>>        }
>>
>> -      if (attr == VARYING_SLOT_PNTC)
>> -         point_sprite = true;
>> -
>> -      if (point_sprite)
>> -      *point_sprite_enables |= (1 << input_index);
>> -
>>        /* flat shading */
>>        if (interp_qualifier == INTERP_QUALIFIER_FLAT ||
>>            (shade_model_flat && is_gl_Color &&
>> @@ -411,7 +442,8 @@ const struct brw_tracked_state gen6_sf_state = {
>>                  _NEW_MULTISAMPLE),
>>        .brw   = (BRW_NEW_CONTEXT |
>>               BRW_NEW_FRAGMENT_PROGRAM |
>
> Add BRW_NEW_GEOMETRY_PROGRAM here, and please add them in alphabetical order.
>
>> -                BRW_NEW_VUE_MAP_GEOM_OUT),
>> +                BRW_NEW_VUE_MAP_GEOM_OUT |
>> +                BRW_NEW_PRIMITIVE),
>
>
>
>>        .cache = CACHE_NEW_WM_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 109b825..371b9de 100644
>> --- a/src/mesa/drivers/dri/i965/gen7_sf_state.c
>> +++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c
>> @@ -93,7 +93,8 @@ const struct brw_tracked_state gen7_sbe_state = {
>>               _NEW_PROGRAM),
>>        .brw   = (BRW_NEW_CONTEXT |
>>               BRW_NEW_FRAGMENT_PROGRAM |
>> -                BRW_NEW_VUE_MAP_GEOM_OUT),
>> +                BRW_NEW_VUE_MAP_GEOM_OUT |
>> +                BRW_NEW_PRIMITIVE),
>
> Ditto - BRW_NEW_GEOMETRY_PROGRAM as well, alphabetize.
>
>>        .cache = CACHE_NEW_WM_PROG
>>     },
>>     .emit = upload_sbe_state,
>>
>
> Otherwise, this looks great to me - thanks for tracking down this obscure bug!
>
> Assuming you make those fixes,
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>


More information about the mesa-stable mailing list