drm/i915/execlists: WaDisableCtxRestoreArbitration is only needed in gen8

Submitted by Michel Thierry on Oct. 5, 2017, 6:19 p.m.

Details

Message ID 20171005181927.7928-1-michel.thierry@intel.com
State New
Headers show
Series "drm/i915/execlists: WaDisableCtxRestoreArbitration is only needed in gen8" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Michel Thierry Oct. 5, 2017, 6:19 p.m.
WaDisableCtxRestoreArbitration was only applied for bdw and chv, but
this changed after the code got moved to gen8_emit_bb_start (and, at
least in my tree, there is no gen9_emit_bb_start).

Fixes: 3ad7b52d962e ("drm/i915/execlists: Move bdw GPGPU w/a to emit_bb")
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c5b76082d695..d2b7eac0777c 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1619,7 +1619,10 @@  static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
 		return PTR_ERR(cs);
 
 	/* WaDisableCtxRestoreArbitration:bdw,chv */
-	*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
+	if (IS_GEN8(req->i915))
+		*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
+	else
+		*cs++ = MI_NOOP;
 
 	/* FIXME(BDW): Address space and security selectors. */
 	*cs++ = MI_BATCH_BUFFER_START_GEN8 |

Comments

Quoting Michel Thierry (2017-10-05 19:19:27)
> WaDisableCtxRestoreArbitration was only applied for bdw and chv, but
> this changed after the code got moved to gen8_emit_bb_start (and, at
> least in my tree, there is no gen9_emit_bb_start).

But I need to make sure we enable arbitration at some point. :)
It may not need the w/a name, but I was making very sure we didn't start
a BB without ARB, or context-switch into the middle of a batch without
ARB. That was my thinking, that there was no harm in overkill if we set
it more often than strictly necessary.
-Chris
On 10/5/2017 11:28 AM, Chris Wilson wrote:
> Quoting Michel Thierry (2017-10-05 19:19:27)
>> WaDisableCtxRestoreArbitration was only applied for bdw and chv, but
>> this changed after the code got moved to gen8_emit_bb_start (and, at
>> least in my tree, there is no gen9_emit_bb_start).
> 
> But I need to make sure we enable arbitration at some point. :)
> It may not need the w/a name, but I was making very sure we didn't start
> a BB without ARB, or context-switch into the middle of a batch without
> ARB. That was my thinking, that there was no harm in overkill if we set
> it more often than strictly necessary.

Ok, thanks for the detailed explanation and yes, probably no harm in 
setting MI_ARB_ON.
On Thu, 2017-10-05 at 11:54 -0700, Michel Thierry wrote:
> On 10/5/2017 11:28 AM, Chris Wilson wrote:
> > Quoting Michel Thierry (2017-10-05 19:19:27)
> > > WaDisableCtxRestoreArbitration was only applied for bdw and chv, but
> > > this changed after the code got moved to gen8_emit_bb_start (and, at
> > > least in my tree, there is no gen9_emit_bb_start).
> > 
> > But I need to make sure we enable arbitration at some point. :)
> > It may not need the w/a name, but I was making very sure we didn't start
> > a BB without ARB, or context-switch into the middle of a batch without
> > ARB. That was my thinking, that there was no harm in overkill if we set
> > it more often than strictly necessary.
> 
> Ok, thanks for the detailed explanation and yes, probably no harm in 
> setting MI_ARB_ON.

Please add a comment if this is the case. W/As implementing required
logic for platforms not covered by the W/A is confusing when they are
being reviewed or double-checked.

Regards, Joonas