Skip to content

Commit

Permalink
iris: Check prog[] instead of uncompiled[] for BLORP state skipping
Browse files Browse the repository at this point in the history
Huge thanks to Tapani Pälli for debugging this issue, figuring out
what was going wrong, proposing fixes, and walking me through where
things were going off the rails.

BLORP always disables tessellation and geometry shaders.  Our handling
tried to look at ice->shaders.uncompiled[] to determine whether the next
draw needed those shaders.  If not, we can leave BLORP's residual state
that disabled those stages in place, and skip looking at it.

Unfortunately, predicting the future is a bit fraught, in part due to
the uncompiled[] and prog[] arrays being slightly out of sync at times.

Consider the following case:

1. Draw with tessellation shaders in place

   => uncompiled[TES] and prog[TES] will both point at valid shaders.

2. Gallium calls pipe->bind_tes_state(NULL).

   => This makes uncompiled[TES] point at NULL, and flags
      IRIS_STAGE_DIRTY_UNCOMPILED_TES.

      Because iris_update_compiled_shaders() hasn't happened yet,
      uncompiled[TES] is NULL but prog[TES] has the stale TES from
      the previous draw still.

3. BLORP operations happen

   => BLORP sees uncompiled[TES] == NULL and decides that tessellation
      is off for the upcoming draws.  So it skips flagging tess state.

4. Gallium calls pipe->bind_tes_state(shader from step android-rpi#1).

   => uncompiled[TES] points at the original shader.
      IRIS_STAGE_DIRTY_UNCOMPILED_TES gets flagged again.

5. Draw again

   => This calls iris_update_compiled_shaders(), which sees that
      a TES is bound, and calls iris_update_compiled_tes().  But
      because the same shader was bound as before, the program it
      comes up with is identical to the one already bound at
      ice->shaders.prog[TES].  So, it thinks it doesn't have to
      flag any tessellation state dirty because it was already
      set up for the last draw.

This random unbind and rebind between draws leads to a situation
where, at step android-rpi#3, BLORP thinks it can skip flagging tessellation
state (nothing is bound), and at step #5, normal state handling
thinks it can skip flagging tessellation state (nothing changed
since last time).  So nobody does, and things break.

This unbind appears to be happening when st_release_variants()
decides it wants to free some shaders.  Then a rebind happens to
put back the actual shader for the draw.  So, it's not theoretical.

To fix this, we change BLORP to look at ice->shaders.prog[] rather
than uncompiled[].  This is equivalent to thinking about the previous
draw, rather than the next.  If the last draw had tessellation off,
then BLORP's disabling was a no-op, and the GPU is still in the same
state as the previous draw.  This is more reliable than predicting
the future.

Cc: mesa-stable
Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/8308
Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/9678
Reviewed-by: Tapani Pälli <[email protected]>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/24880>
(cherry picked from commit d693027)
  • Loading branch information
kaydenl authored and dcbaker-intel committed Sep 22, 2023
1 parent 0ee3d2b commit 77ff616
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 5 deletions.
2 changes: 1 addition & 1 deletion .pick_status.json
Original file line number Diff line number Diff line change
Expand Up @@ -10624,7 +10624,7 @@
"description": "iris: Check prog[] instead of uncompiled[] for BLORP state skipping",
"nominated": true,
"nomination_type": 0,
"resolution": 0,
"resolution": 1,
"main_sha": null,
"because_sha": null,
"notes": null
Expand Down
8 changes: 4 additions & 4 deletions src/gallium/drivers/iris/iris_blorp.c
Original file line number Diff line number Diff line change
Expand Up @@ -364,8 +364,8 @@ iris_blorp_exec_render(struct blorp_batch *blorp_batch,
IRIS_STAGE_DIRTY_SAMPLER_STATES_TES |
IRIS_STAGE_DIRTY_SAMPLER_STATES_GS);

if (!ice->shaders.uncompiled[MESA_SHADER_TESS_EVAL]) {
/* BLORP disabled tessellation, that's fine for the next draw */
if (!ice->shaders.prog[MESA_SHADER_TESS_EVAL]) {
/* BLORP disabled tessellation, but it was already off anyway */
skip_stage_bits |= IRIS_STAGE_DIRTY_TCS |
IRIS_STAGE_DIRTY_TES |
IRIS_STAGE_DIRTY_CONSTANTS_TCS |
Expand All @@ -374,8 +374,8 @@ iris_blorp_exec_render(struct blorp_batch *blorp_batch,
IRIS_STAGE_DIRTY_BINDINGS_TES;
}

if (!ice->shaders.uncompiled[MESA_SHADER_GEOMETRY]) {
/* BLORP disabled geometry shaders, that's fine for the next draw */
if (!ice->shaders.prog[MESA_SHADER_GEOMETRY]) {
/* BLORP disabled geometry shaders, but it was already off anyway */
skip_stage_bits |= IRIS_STAGE_DIRTY_GS |
IRIS_STAGE_DIRTY_CONSTANTS_GS |
IRIS_STAGE_DIRTY_BINDINGS_GS;
Expand Down

0 comments on commit 77ff616

Please sign in to comment.