[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-dev
mailing list