| Message ID | 20171005181927.7928-1-michel.thierry@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 |
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(-)