[Mesa-dev] [PATCH 1/3] i965/gen10: Ignore push constant packets during context restore.

[Kenneth Graunke]

On Wednesday, January 24, 2018 4:33:56 PM PST Rafael Antognolli wrote:
> These packets were causing GPU hangs when the context was restored,
> possibly because they were pointing to BO's that were already
> unreferenced. So we tell the hardware to ignore such packets after the
> batch buffer ends, since we know those BO's are not around anymore.
> 
> This change fixes GPU hangs on CNL. The (partial) solution to this
> problem so far was to entirely disable push constants on this platform.
> 
> Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> Cc: "18.0" <mesa-stable at lists.freedesktop.org>
> ---
>  src/mesa/drivers/dri/i965/brw_pipe_control.c  | 49 +++++++++++++++++++++++++++
>  src/mesa/drivers/dri/i965/brw_pipe_control.h  |  1 +
>  src/mesa/drivers/dri/i965/intel_batchbuffer.c |  4 +++
>  3 files changed, 54 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_pipe_control.c b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> index e28be34c8e8..eb8ada63129 100644
> --- a/src/mesa/drivers/dri/i965/brw_pipe_control.c
> +++ b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> @@ -317,6 +317,55 @@ gen7_emit_vs_workaround_flush(struct brw_context *brw)
>                                 brw->workaround_bo, 0, 0);
>  }
>  
> +/**
> + * From the PRM, Volume 2a:
> + *
> + *    "Indirect State Pointers Disable
> + *
> + *    At the completion of the post-sync operation associated with this pipe
> + *    control packet, the indirect state pointers in the hardware are
> + *    considered invalid; the indirect pointers are not saved in the context.
> + *    If any new indirect state commands are executed in the command stream
> + *    while the pipe control is pending, the new indirect state commands are
> + *    preserved.
> + *
> + *    [DevIVB+]: Using Invalidate State Pointer (ISP) only inhibits context
> + *    restoring of Push Constant (3DSTATE_CONSTANT_*) commands. Push Constant
> + *    commands are only considered as Indirect State Pointers. Once ISP is
> + *    issued in a context, SW must initialize by programming push constant
> + *    commands for all the shaders (at least to zero length) before attempting
> + *    any rendering operation for the same context."
> + *
> + * 3DSTATE_CONSTANT_* packets are restored during a context restore,
> + * even though they point to a BO that has been already unreferenced at
> + * the end of the previous batch buffer. This has been fine so far since
> + * we are protected by these scratch page (every address not covered by
> + * a BO should be pointing to the scratch page). But on CNL, it is
> + * causing a GPU hang during context restore at the 3DSTATE_CONSTANT_*
> + * instruction.
> + *
> + * The flag "Indirect State Pointers Disable" in PIPE_CONTROL tells the
> + * hardware to ignore previous 3DSTATE_CONSTANT_* packets during a
> + * context restore, so the mentioned hang doesn't happen. However,
> + * software must program push constant commands for all stages prior to
> + * rendering anything, so we flag them as dirty.
> + */
> +void
> +gen10_emit_isp_disable(struct brw_context *brw)
> +{
> +   const struct gen_device_info *devinfo = &brw->screen->devinfo;
> +
> +   brw_emit_pipe_control_write(brw,
> +                               PIPE_CONTROL_ISP_DIS |
> +                               PIPE_CONTROL_WRITE_IMMEDIATE,
> +                               brw->workaround_bo, 0, 0);
> +
> +   brw->vs.base.push_constants_dirty = true;
> +   brw->tcs.base.push_constants_dirty = true;
> +   brw->tes.base.push_constants_dirty = true;
> +   brw->gs.base.push_constants_dirty = true;
> +   brw->wm.base.push_constants_dirty = true;
> +}
>  
>  /**
>   * Emit a PIPE_CONTROL command for gen7 with the CS Stall bit set.
> diff --git a/src/mesa/drivers/dri/i965/brw_pipe_control.h b/src/mesa/drivers/dri/i965/brw_pipe_control.h
> index 6e9a404870d..651cd4d3e70 100644
> --- a/src/mesa/drivers/dri/i965/brw_pipe_control.h
> +++ b/src/mesa/drivers/dri/i965/brw_pipe_control.h
> @@ -85,5 +85,6 @@ void brw_emit_post_sync_nonzero_flush(struct brw_context *brw);
>  void brw_emit_depth_stall_flushes(struct brw_context *brw);
>  void gen7_emit_vs_workaround_flush(struct brw_context *brw);
>  void gen7_emit_cs_stall_flush(struct brw_context *brw);
> +void gen10_emit_isp_disable(struct brw_context *brw);
>  
>  #endif
> diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> index 02bfd3f333c..86d88a701f8 100644
> --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> @@ -764,6 +764,10 @@ brw_finish_batch(struct brw_context *brw)
>           brw_emit_pipe_control_flush(brw, PIPE_CONTROL_RENDER_TARGET_FLUSH |
>                                            PIPE_CONTROL_CS_STALL);
>        }
> +
> +      /* Do not restore push constant packets during context restore. */
> +      if (devinfo->gen == 10)
> +         gen10_emit_isp_disable(brw);
>     }
>  
>     /* Emit MI_BATCH_BUFFER_END to finish our batch.  Note that execbuf2
> 

We might consider doing this on gen >= 7, as I think it should be safe
and possibly save some unnecessary garbage reads during context restore.

But, we can always do that later.  I'm happy to take your patches as-is
for now.  Series is:

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180124/43047dfb/attachment.sig>