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

Kenneth Graunke kenneth at whitecape.org
Tue Nov 25 01:17:25 PST 2014


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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-stable/attachments/20141125/b2e0dfbc/attachment.sig>


More information about the mesa-stable mailing list