[Mesa-dev] [PATCH 4/4] i965: Reimplement all the PIPE_CONTROL rules.
Kenneth Graunke
kenneth at whitecape.org
Mon Feb 25 18:32:27 UTC 2019
On Monday, February 25, 2019 6:33:11 AM PST Pohjolainen, Topi wrote:
> On Thu, Nov 01, 2018 at 08:04:21PM -0700, Kenneth Graunke wrote:
> > This implements virtually all documented PIPE_CONTROL restrictions
> > in a centralized helper. You now simply ask for the operations you
> > want, and the pipe control "brain" will figure out exactly what pipe
> > controls to emit to make that happen without tanking your system.
> >
> > The hope is that this will fix some intermittent flushing issues as
> > well as GPU hangs. However, it also has a high risk of causing GPU
> > hangs and other regressions, as this is a particularly sensitive
> > area and poking the bear isn't always advisable.
>
> First I checked I could find all the things in bspec. There was one that I
> couldn't, noted further down.
>
> Second I checked that all the rules earlier were implemented. Found one
> exception, noted further down as well.
>
> I didn't check if the rules still miss something in bspec. That would be
> another exercise...
[snip]
> > + /* Recursive PIPE_CONTROL workarounds --------------------------------
> > + * (http://knowyourmeme.com/memes/xzibit-yo-dawg)
> > + *
> > + * We do these first because we want to look at the original operation,
> > + * rather than any workarounds we set.
> > + */
> > + if (GEN_GEN == 6 && (flags & PIPE_CONTROL_RENDER_TARGET_FLUSH)) {
> > + /* Hardware workaround: SNB B-Spec says:
> > + *
> > + * "[Dev-SNB{W/A}]: Before a PIPE_CONTROL with Write Cache Flush
> > + * Enable = 1, a PIPE_CONTROL with any non-zero post-sync-op is
> > + * required."
> > + */
> > + brw_emit_post_sync_nonzero_flush(brw);
> > + }
> > +
> > + if (GEN_GEN == 9 && (flags & PIPE_CONTROL_VF_CACHE_INVALIDATE)) {
> > + /* The PIPE_CONTROL "VF Cache Invalidation Enable" bit description
> > + * lists several workarounds:
> > + *
> > + * "Project: SKL, KBL, BXT
> > + *
> > + * If the VF Cache Invalidation Enable is set to a 1 in a
> > + * PIPE_CONTROL, a separate Null PIPE_CONTROL, all bitfields
> > + * sets to 0, with the VF Cache Invalidation Enable set to 0
> > + * needs to be sent prior to the PIPE_CONTROL with VF Cache
> > + * Invalidation Enable set to a 1."
> > + */
> > + genX(emit_raw_pipe_control)(brw, 0, NULL, 0, 0);
> > + }
> > +
> > + if (GEN_GEN == 9 && IS_COMPUTE_PIPELINE(brw) && post_sync_flags) {
> > + /* Project: SKL / Argument: LRI Post Sync Operation [23]
> > + *
> > + * "PIPECONTROL command with “Command Streamer Stall Enable” must be
> > + * programmed prior to programming a PIPECONTROL command with "LRI
> > + * Post Sync Operation" in GPGPU mode of operation (i.e when
> > + * PIPELINE_SELECT command is set to GPGPU mode of operation)."
> > + *
> > + * The same text exists a few rows below for Post Sync Op.
> > + */
> > + genX(emit_raw_pipe_control)(brw, PIPE_CONTROL_CS_STALL, bo, offset, imm);
>
> Are bo, offset, imm needed here as well?
No, I don't think so. We should pass NULL, 0, 0 here - we're just doing
a plain CS stall separately. We'll use them for the actual write later.
Good catch!
[snip]
> > - if (GEN_GEN >= 9) {
> > - /* THE PIPE_CONTROL "VF Cache Invalidation Enable" docs continue:
> > - *
> > - * "Project: BDW+
> > - *
> > - * When VF Cache Invalidate is set “Post Sync Operation” must
> > - * be enabled to “Write Immediate Data” or “Write PS Depth
> > - * Count” or “Write Timestamp”."
> > - *
> > - * If there's a BO, we're already doing some kind of write.
> > - * If not, add a write to the workaround BO.
> > - *
> > - * XXX: This causes GPU hangs on Broadwell, so restrict it to
> > - * Gen9+ for now...see this bug for more information:
> > - * https://bugs.freedesktop.org/show_bug.cgi?id=103787
>
> In "Flush Types" workarounds later on you apply this for gen8 as well.
Yes, that's intentional - we're supposed to according to the docs.
I re-tested the Piglit test from bug 103787 on my Broadwell, and it
works fine - no GPU hangs. I think we were just doing it wrong before.
Trying to figure out an ordering for the workarounds is awful... :(
> > - */
> > - if (!bo) {
> > - flags |= PIPE_CONTROL_WRITE_IMMEDIATE;
> > - bo = brw->workaround_bo;
> > - }
> > - }
> > + if (GEN_IS_HASWELL) {
> > + /* From the PIPE_CONTROL page itself:
> > + *
> > + * "HSW - Programming Note: PIPECONTROL with RO Cache Invalidation:
> > + * Prior to programming a PIPECONTROL command with any of the RO
> > + * cache invalidation bit set, program a PIPECONTROL flush command
> > + * with “CS stall” bit and “HDC Flush” bit set."
> > + *
> > + * TODO: Actually implement this. What's an HDC Flush?
>
> There is bit 9 HDC Flush but that is defined for ICL, for HSW I couldn't find
> anything either.
Yeah...I figured I'd call it out, but...I don't think we're doing
anything today either, so this is at least no worse.
> > + */
> > + }
> > +
> > + if (flags & PIPE_CONTROL_FLUSH_LLC) {
> > + /* From the PIPE_CONTROL instruction table, bit 26 (Flush LLC):
> > + *
> > + * "Project: ALL
> > + * SW must always program Post-Sync Operation to "Write Immediate
> > + * Data" when Flush LLC is set."
> > + *
> > + * For now, we just require the caller to do it.
> > + */
> > + assert(flags & PIPE_CONTROL_WRITE_IMMEDIATE);
> > + }
> > +
> > + /* "Post-Sync Operation" workarounds -------------------------------- */
> > +
> > + /* Project: All / Argument: Global Snapshot Count Reset [19]
> > + *
> > + * "This bit must not be exercised on any product.
> > + * Requires stall bit ([20] of DW1) set."
> > + *
> > + * We don't use this, so we just assert that it isn't used. The
> > + * PIPE_CONTROL instruction page indicates that they intended this
> > + * as a debug feature and don't think it is useful in production,
> > + * but it may actually be usable, should we ever want to.
> > + */
> > + assert((flags & PIPE_CONTROL_GLOBAL_SNAPSHOT_COUNT_RESET) == 0);
> > +
> > + if (flags & (PIPE_CONTROL_MEDIA_STATE_CLEAR |
> > + PIPE_CONTROL_INDIRECT_STATE_POINTERS_DISABLE)) {
> > + /* Project: All / Arguments:
> > + *
> > + * - Generic Media State Clear [16]
> > + * - Indirect State Pointers Disable [16]
> > + *
> > + * "Requires stall bit ([20] of DW1) set."
> > + *
> > + * Also, the PIPE_CONTROL instruction table, bit 16 (Generic Media
> > + * State Clear) says:
> > + *
> > + * "PIPECONTROL command with “Command Streamer Stall Enable” must be
> > + * programmed prior to programming a PIPECONTROL command with "Media
> > + * State Clear" set in GPGPU mode of operation"
> > + *
> > + * This is a subset of the earlier rule, so there's nothing to do.
> > + */
> > + flags |= PIPE_CONTROL_CS_STALL;
> > + }
> > +
> > + if (flags & PIPE_CONTROL_STORE_DATA_INDEX) {
> > + /* Project: All / Argument: Store Data Index
> > + *
> > + * "Post-Sync Operation ([15:14] of DW1) must be set to something other
> > + * than '0'."
> > + *
> > + * For now, we just assert that the caller does this. We might want to
> > + * automatically add a write to the workaround BO...
> > + */
> > + assert(non_lri_post_sync_flags != 0);
> > + }
> > +
> > + if (flags & PIPE_CONTROL_SYNC_GFDT) {
> > + /* Project: All / Argument: Sync GFDT
> > + *
> > + * "Post-Sync Operation ([15:14] of DW1) must be set to something other
> > + * than '0' or 0x2520[13] must be set."
> > + *
> > + * For now, we just assert that the caller does this.
> > + */
> > + assert(non_lri_post_sync_flags != 0);
> > + }
> > +
> > + if (IS_GENx10_BETWEEN(60, 75) && (flags & PIPE_CONTROL_TLB_INVALIDATE)) {
> > + /* Project: SNB, IVB, HSW / Argument: TLB inv
> > + *
> > + * "{All SKUs}{All Steppings}: Post-Sync Operation ([15:14] of DW1)
> > + * must be set to something other than '0'."
> > + *
> > + * For now, we just assert that the caller does this.
> > + */
> > + assert(non_lri_post_sync_flags != 0);
> > + }
> > +
> > + if (GEN_GEN >= 7 && (flags & PIPE_CONTROL_TLB_INVALIDATE)) {
> > + /* Project: IVB+ / Argument: TLB inv
> > + *
> > + * "Requires stall bit ([20] of DW1) set."
> > + *
> > + * Also, from the PIPE_CONTROL instruction table:
> > + *
> > + * "Project: SKL+
> > + * Post Sync Operation or CS stall must be set to ensure a TLB
> > + * invalidation occurs. Otherwise no cycle will occur to the TLB
> > + * cache to invalidate."
> > + *
> > + * This is not a subset of the earlier rule, so there's nothing to do.
> > + */
> > + flags |= PIPE_CONTROL_CS_STALL;
> > + }
> > +
> > + if (GEN_GEN == 9 && devinfo->gt == 4) {
> > + /* TODO: The big Skylake GT4 post sync op workaround */
> > + }
> > +
> > + /* "GPGPU specific workarounds" (both post-sync and flush) ------------ */
> > +
> > + if (IS_COMPUTE_PIPELINE(brw)) {
> > + if (GEN_GEN >= 9 && (flags & PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE)) {
> > + /* Project: SKL+ / Argument: Tex Invalidate
> > + * "Requires stall bit ([20] of DW) set for all GPGPU Workloads."
> > + */
> > + flags |= PIPE_CONTROL_CS_STALL;
> > }
> >
> > - if (GEN_GEN == 10)
> > - gen10_add_rcpfe_workaround_bits(&flags);
> > - } else if (GEN_GEN >= 6) {
> > - if (GEN_GEN == 6 &&
> > - (flags & PIPE_CONTROL_RENDER_TARGET_FLUSH)) {
> > - /* Hardware workaround: SNB B-Spec says:
> > + if (GEN_GEN == 8 && (post_sync_flags ||
> > + (flags & (PIPE_CONTROL_NOTIFY_ENABLE |
> > + PIPE_CONTROL_DEPTH_STALL |
> > + PIPE_CONTROL_RENDER_TARGET_FLUSH |
> > + PIPE_CONTROL_DEPTH_CACHE_FLUSH |
> > + PIPE_CONTROL_DATA_CACHE_FLUSH)))) {
> > + /* Project: BDW / Arguments:
> > *
> > - * [Dev-SNB{W/A}]: Before a PIPE_CONTROL with Write Cache Flush
> > - * Enable = 1, a PIPE_CONTROL with any non-zero post-sync-op is
> > - * required.
> > + * - LRI Post Sync Operation [23]
> > + * - Post Sync Op [15:14]
> > + * - Notify En [8]
> > + * - Depth Stall [13]
> > + * - Render Target Cache Flush [12]
> > + * - Depth Cache Flush [0]
> > + * - DC Flush Enable [5]
> > + *
> > + * "Requires stall bit ([20] of DW) set for all GPGPU and Media
> > + * Workloads."
>
> This I couldn't find.
Ah, sorry, this could probably be clearer.
On the "Programming Restrictions for PIPE_CONTROL" subpages ("Post-Sync
Operation", "Flush Types"), there are separate table rows for each bit,
with the same workaround text:
+--------------------------------------------------------------------+
| LRI Post | 23 | Requires stall bit ([20] of DW) set for all | BDW |
| Sync | | GPGPU and Media Workloads. | |
| Operation | | | |
+--------------------------------------------------------------------+
I just grouped them together because they're all the same workaround...
just documented in 8 different table rows.
I'm definitely open to suggestions on how to improve the comment :)
Thanks so much for looking at this! I owe you!
--Ken
-------------- 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/20190225/8a390b81/attachment-0001.sig>
More information about the mesa-dev
mailing list