From: Tvrtko Ursulin tvrtko.ursulin@intel.com
In current code we check that a workaround is not completely overwriting the existing one, but for instance partial conflict in some bits would get missed, as would problems involving masked registers, courtesy of the mask (wa->clr) being forced to zero for such registers and also being conflated with the readback verification.
Now that previous patches have separated write masks from readback masks, and ensured all masked registers are correctly tagged as such, we can improve the verification checks to also detect partial conflicts, wrong masks and inconsistent register usage.
Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@intel.com Reported-by: Andres Calderon Jaramillo andrescj@google.com --- drivers/gpu/drm/i915/gt/intel_workarounds.c | 81 +++++++++++++++---- .../gpu/drm/i915/gt/intel_workarounds_types.h | 4 + .../gpu/drm/i915/gt/selftest_workarounds.c | 4 +- 3 files changed, 72 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c index cd84c2a86787..c82f165bdd8b 100644 --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c @@ -52,8 +52,11 @@ * - Public functions to init or apply the given workaround type. */
-static void wa_init_start(struct i915_wa_list *wal, const char *name, const char *engine_name) +static void +wa_init_start(struct drm_i915_private *i915, struct i915_wa_list *wal, + const char *name, const char *engine_name) { + wal->i915 = i915; wal->name = name; wal->engine_name = engine_name; } @@ -81,6 +84,59 @@ static void wa_init_finish(struct i915_wa_list *wal) wal->wa_count, wal->name, wal->engine_name); }
+static void +log_bad_wa(const struct i915_wa_list *wal, const struct i915_wa *wa, + const char *msg) +{ + drm_err(&wal->i915->drm, + "Discarding %s workaround! (reg=%x %s=%x set=%x)\n", + msg, i915_mmio_reg_offset(wa->reg), wa->clr ? "clear" : "mask", + wa->clr ?: wa->set >> 16, wa->set); +} + +static bool +check_conflict(const struct i915_wa_list *wal, + const struct i915_wa *old, + const struct i915_wa *new) +{ + u32 new_mask, old_mask, common, new_set, old_set; + + if (new->clr && !old->clr) { + log_bad_wa(wal, new, "mixed masked and regular"); + return true; + } + + if (new->clr) { + new_mask = new->clr; + old_mask = old->clr; + new_set = new->set; + old_set = old->set; + } else { + new_mask = new->set >> 16; + old_mask = old->set >> 16; + new_set = new->set & 0xffff; + old_set = old->set & 0xffff; + } + + if (new_set && (new_set & ~new_mask)) { + log_bad_wa(wal, new, "write outside the mask"); + return true; + } + + common = new_mask & old_mask; + if (common) { + if ((new_set & common) != (old_set & common)) { + log_bad_wa(wal, new, "conflicting"); + return true; + } else if (new_mask == old_mask) { + log_bad_wa(wal, new, "duplicate"); + return true; + } + } + + return false; +} + static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa) { unsigned int addr = i915_mmio_reg_offset(wa->reg); @@ -118,18 +174,13 @@ static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa) } else { wa_ = &wal->list[mid];
- if ((wa->clr | wa_->clr) && !(wa->clr & ~wa_->clr)) { - DRM_ERROR("Discarding overwritten w/a for reg %04x (clear: %08x, set: %08x)\n", - i915_mmio_reg_offset(wa_->reg), - wa_->clr, wa_->set); - - wa_->set &= ~wa->clr; + if (!check_conflict(wal, wa_, wa)) { + wal->wa_count++; + wa_->set |= wa->set; + wa_->clr |= wa->clr; + wa_->read |= wa->read; }
- wal->wa_count++; - wa_->set |= wa->set; - wa_->clr |= wa->clr; - wa_->read |= wa->read; return; } } @@ -716,7 +767,7 @@ __intel_engine_init_ctx_wa(struct intel_engine_cs *engine, if (engine->class != RENDER_CLASS) return;
- wa_init_start(wal, name, engine->name); + wa_init_start(engine->i915, wal, name, engine->name);
if (IS_DG1(i915)) dg1_ctx_workarounds_init(engine, wal); @@ -1232,7 +1283,7 @@ void intel_gt_init_workarounds(struct drm_i915_private *i915) { struct i915_wa_list *wal = &i915->gt_wa_list;
- wa_init_start(wal, "GT", "global"); + wa_init_start(i915, wal, "GT", "global"); gt_init_workarounds(i915, wal); wa_init_finish(wal); } @@ -1575,7 +1626,7 @@ void intel_engine_init_whitelist(struct intel_engine_cs *engine) struct drm_i915_private *i915 = engine->i915; struct i915_wa_list *w = &engine->whitelist;
- wa_init_start(w, "whitelist", engine->name); + wa_init_start(engine->i915, w, "whitelist", engine->name);
if (IS_DG1(i915)) dg1_whitelist_build(engine); @@ -2095,7 +2146,7 @@ void intel_engine_init_workarounds(struct intel_engine_cs *engine) if (INTEL_GEN(engine->i915) < 4) return;
- wa_init_start(wal, "engine", engine->name); + wa_init_start(engine->i915, wal, "engine", engine->name); engine_init_workarounds(engine, wal); wa_init_finish(wal); } diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds_types.h b/drivers/gpu/drm/i915/gt/intel_workarounds_types.h index c214111ea367..b6a9d1582a5c 100644 --- a/drivers/gpu/drm/i915/gt/intel_workarounds_types.h +++ b/drivers/gpu/drm/i915/gt/intel_workarounds_types.h @@ -10,6 +10,8 @@
#include "i915_reg.h"
+struct drm_i915_private; + struct i915_wa { i915_reg_t reg; u32 clr; @@ -18,6 +20,8 @@ struct i915_wa { };
struct i915_wa_list { + struct drm_i915_private *i915; + const char *name; const char *engine_name; struct i915_wa *list; diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c index 64937ec3f2dc..536cbe7889cc 100644 --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c @@ -64,14 +64,14 @@ reference_lists_init(struct intel_gt *gt, struct wa_lists *lists)
memset(lists, 0, sizeof(*lists));
- wa_init_start(&lists->gt_wa_list, "GT_REF", "global"); + wa_init_start(gt->i915, &lists->gt_wa_list, "GT_REF", "global"); gt_init_workarounds(gt->i915, &lists->gt_wa_list); wa_init_finish(&lists->gt_wa_list);
for_each_engine(engine, gt, id) { struct i915_wa_list *wal = &lists->engine[id].wa_list;
- wa_init_start(wal, "REF", engine->name); + wa_init_start(gt->i915, wal, "REF", engine->name); engine_init_workarounds(engine, wal); wa_init_finish(wal);