[Intel-gfx] [PATCH v2 1/2] drm/i915/selftests: recreate WA lists inside the selftest
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Jan 10 09:46:56 UTC 2019
On 10/01/2019 01:32, Daniele Ceraolo Spurio wrote:
> By using the wa lists inside the live driver structures, we won't
> catch issues where those are incorrectly setup or corrupted.
> To cover this gap, update the workaround framework to allow saving the
> wa lists to independent structures and use them in the selftests.
>
> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> ---
> drivers/gpu/drm/i915/intel_workarounds.c | 117 +++++++++---------
> .../drm/i915/selftests/intel_workarounds.c | 69 +++++++++--
> 2 files changed, 119 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
> index 480c53a2ecb5..3210ad4e08f7 100644
> --- a/drivers/gpu/drm/i915/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/intel_workarounds.c
> @@ -639,10 +639,9 @@ wa_write_or(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
> wa_write_masked_or(wal, reg, val, val);
> }
>
> -static void gen9_gt_workarounds_init(struct drm_i915_private *i915)
> +static void
> +gen9_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
> {
> - struct i915_wa_list *wal = &i915->gt_wa_list;
> -
> /* WaDisableKillLogic:bxt,skl,kbl */
> if (!IS_COFFEELAKE(i915))
> wa_write_or(wal,
> @@ -666,11 +665,10 @@ static void gen9_gt_workarounds_init(struct drm_i915_private *i915)
> BDW_DISABLE_HDC_INVALIDATION);
> }
>
> -static void skl_gt_workarounds_init(struct drm_i915_private *i915)
> +static void
> +skl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
> {
> - struct i915_wa_list *wal = &i915->gt_wa_list;
> -
> - gen9_gt_workarounds_init(i915);
> + gen9_gt_workarounds_init(i915, wal);
>
> /* WaDisableGafsUnitClkGating:skl */
> wa_write_or(wal,
> @@ -684,11 +682,10 @@ static void skl_gt_workarounds_init(struct drm_i915_private *i915)
> GAMT_ECO_ENABLE_IN_PLACE_DECOMPRESS);
> }
>
> -static void bxt_gt_workarounds_init(struct drm_i915_private *i915)
> +static void
> +bxt_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
> {
> - struct i915_wa_list *wal = &i915->gt_wa_list;
> -
> - gen9_gt_workarounds_init(i915);
> + gen9_gt_workarounds_init(i915, wal);
>
> /* WaInPlaceDecompressionHang:bxt */
> wa_write_or(wal,
> @@ -696,11 +693,10 @@ static void bxt_gt_workarounds_init(struct drm_i915_private *i915)
> GAMT_ECO_ENABLE_IN_PLACE_DECOMPRESS);
> }
>
> -static void kbl_gt_workarounds_init(struct drm_i915_private *i915)
> +static void
> +kbl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
> {
> - struct i915_wa_list *wal = &i915->gt_wa_list;
> -
> - gen9_gt_workarounds_init(i915);
> + gen9_gt_workarounds_init(i915, wal);
>
> /* WaDisableDynamicCreditSharing:kbl */
> if (IS_KBL_REVID(i915, 0, KBL_REVID_B0))
> @@ -719,16 +715,16 @@ static void kbl_gt_workarounds_init(struct drm_i915_private *i915)
> GAMT_ECO_ENABLE_IN_PLACE_DECOMPRESS);
> }
>
> -static void glk_gt_workarounds_init(struct drm_i915_private *i915)
> +static void
> +glk_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
> {
> - gen9_gt_workarounds_init(i915);
> + gen9_gt_workarounds_init(i915, wal);
> }
>
> -static void cfl_gt_workarounds_init(struct drm_i915_private *i915)
> +static void
> +cfl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
> {
> - struct i915_wa_list *wal = &i915->gt_wa_list;
> -
> - gen9_gt_workarounds_init(i915);
> + gen9_gt_workarounds_init(i915, wal);
>
> /* WaDisableGafsUnitClkGating:cfl */
> wa_write_or(wal,
> @@ -741,10 +737,10 @@ static void cfl_gt_workarounds_init(struct drm_i915_private *i915)
> GAMT_ECO_ENABLE_IN_PLACE_DECOMPRESS);
> }
>
> -static void wa_init_mcr(struct drm_i915_private *dev_priv)
> +static void
> +wa_init_mcr(struct drm_i915_private *dev_priv, struct i915_wa_list *wal)
> {
> const struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)->sseu;
> - struct i915_wa_list *wal = &dev_priv->gt_wa_list;
> u32 mcr_slice_subslice_mask;
>
> /*
> @@ -804,11 +800,10 @@ static void wa_init_mcr(struct drm_i915_private *dev_priv)
> intel_calculate_mcr_s_ss_select(dev_priv));
> }
>
> -static void cnl_gt_workarounds_init(struct drm_i915_private *i915)
> +static void
> +cnl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
> {
> - struct i915_wa_list *wal = &i915->gt_wa_list;
> -
> - wa_init_mcr(i915);
> + wa_init_mcr(i915, wal);
>
> /* WaDisableI2mCycleOnWRPort:cnl (pre-prod) */
> if (IS_CNL_REVID(i915, CNL_REVID_B0, CNL_REVID_B0))
> @@ -822,11 +817,10 @@ static void cnl_gt_workarounds_init(struct drm_i915_private *i915)
> GAMT_ECO_ENABLE_IN_PLACE_DECOMPRESS);
> }
>
> -static void icl_gt_workarounds_init(struct drm_i915_private *i915)
> +static void
> +icl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
> {
> - struct i915_wa_list *wal = &i915->gt_wa_list;
> -
> - wa_init_mcr(i915);
> + wa_init_mcr(i915, wal);
>
> /* WaInPlaceDecompressionHang:icl */
> wa_write_or(wal,
> @@ -879,12 +873,9 @@ static void icl_gt_workarounds_init(struct drm_i915_private *i915)
> GAMT_CHKN_DISABLE_L3_COH_PIPE);
> }
>
> -void intel_gt_init_workarounds(struct drm_i915_private *i915)
> +static void
> +gt_init_workarounds(struct drm_i915_private *i915, struct i915_wa_list *wal)
> {
> - struct i915_wa_list *wal = &i915->gt_wa_list;
> -
> - wa_init_start(wal, "GT");
> -
> if (INTEL_GEN(i915) < 8)
> return;
> else if (IS_BROADWELL(i915))
> @@ -892,22 +883,29 @@ void intel_gt_init_workarounds(struct drm_i915_private *i915)
> else if (IS_CHERRYVIEW(i915))
> return;
> else if (IS_SKYLAKE(i915))
> - skl_gt_workarounds_init(i915);
> + skl_gt_workarounds_init(i915, wal);
> else if (IS_BROXTON(i915))
> - bxt_gt_workarounds_init(i915);
> + bxt_gt_workarounds_init(i915, wal);
> else if (IS_KABYLAKE(i915))
> - kbl_gt_workarounds_init(i915);
> + kbl_gt_workarounds_init(i915, wal);
> else if (IS_GEMINILAKE(i915))
> - glk_gt_workarounds_init(i915);
> + glk_gt_workarounds_init(i915, wal);
> else if (IS_COFFEELAKE(i915))
> - cfl_gt_workarounds_init(i915);
> + cfl_gt_workarounds_init(i915, wal);
> else if (IS_CANNONLAKE(i915))
> - cnl_gt_workarounds_init(i915);
> + cnl_gt_workarounds_init(i915, wal);
> else if (IS_ICELAKE(i915))
> - icl_gt_workarounds_init(i915);
> + icl_gt_workarounds_init(i915, wal);
> else
> MISSING_CASE(INTEL_GEN(i915));
> +}
> +
> +void intel_gt_init_workarounds(struct drm_i915_private *i915)
> +{
> + struct i915_wa_list *wal = &i915->gt_wa_list;
>
> + wa_init_start(wal, "GT");
> + gt_init_workarounds(i915, wal);
> wa_init_finish(wal);
> }
>
> @@ -1126,10 +1124,10 @@ void intel_engine_apply_whitelist(struct intel_engine_cs *engine)
> i915_mmio_reg_offset(RING_NOPID(base)));
> }
>
> -static void rcs_engine_wa_init(struct intel_engine_cs *engine)
> +static void
> +rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
> {
> struct drm_i915_private *i915 = engine->i915;
> - struct i915_wa_list *wal = &engine->wa_list;
>
> if (IS_ICELAKE(i915)) {
> /* This is not an Wa. Enable for better image quality */
> @@ -1233,10 +1231,10 @@ static void rcs_engine_wa_init(struct intel_engine_cs *engine)
> }
> }
>
> -static void xcs_engine_wa_init(struct intel_engine_cs *engine)
> +static void
> +xcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
> {
> struct drm_i915_private *i915 = engine->i915;
> - struct i915_wa_list *wal = &engine->wa_list;
>
> /* WaKBLVECSSemaphoreWaitPoll:kbl */
> if (IS_KBL_REVID(i915, KBL_REVID_A0, KBL_REVID_E0)) {
> @@ -1246,6 +1244,18 @@ static void xcs_engine_wa_init(struct intel_engine_cs *engine)
> }
> }
>
> +static void
> +engine_init_workarounds(struct intel_engine_cs *engine, struct i915_wa_list *wal)
> +{
> + if (I915_SELFTEST_ONLY(INTEL_GEN(engine->i915) < 8))
> + return;
> +
> + if (engine->id == RCS)
> + rcs_engine_wa_init(engine, wal);
> + else
> + xcs_engine_wa_init(engine, wal);
> +}
> +
> void intel_engine_init_workarounds(struct intel_engine_cs *engine)
> {
> struct i915_wa_list *wal = &engine->wa_list;
> @@ -1254,12 +1264,7 @@ void intel_engine_init_workarounds(struct intel_engine_cs *engine)
> return;
>
> wa_init_start(wal, engine->name);
> -
> - if (engine->id == RCS)
> - rcs_engine_wa_init(engine);
> - else
> - xcs_engine_wa_init(engine);
> -
> + engine_init_workarounds(engine, wal);
> wa_init_finish(wal);
> }
>
> @@ -1269,11 +1274,5 @@ void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
> }
>
> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> -static bool intel_engine_verify_workarounds(struct intel_engine_cs *engine,
> - const char *from)
> -{
> - return wa_list_verify(engine->i915, &engine->wa_list, from);
> -}
> -
> #include "selftests/intel_workarounds.c"
> #endif
> diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> index c2b3cd8fcc34..e5875cca88fb 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> @@ -12,6 +12,51 @@
> #include "igt_wedge_me.h"
> #include "mock_context.h"
>
> +#define REF_NAME_MAX (INTEL_ENGINE_CS_MAX_NAME + 4)
> +struct wa_lists {
> + struct i915_wa_list gt_wa_list;
> + struct {
> + char name[REF_NAME_MAX];
> + struct i915_wa_list wa_list;
> + } engine[I915_NUM_ENGINES];
> +};
> +
> +static void
> +reference_lists_init(struct drm_i915_private *i915, struct wa_lists *lists)
> +{
> + struct intel_engine_cs *engine;
> + enum intel_engine_id id;
> +
> + memset(lists, 0 , sizeof(*lists));
> +
> + wa_init_start(&lists->gt_wa_list, "GT_REF");
> + gt_init_workarounds(i915, &lists->gt_wa_list);
> + wa_init_finish(&lists->gt_wa_list);
> +
> + for_each_engine(engine, i915, id) {
> + struct i915_wa_list *wal = &lists->engine[id].wa_list;
> + char *name = lists->engine[id].name;
> +
> + snprintf(name, REF_NAME_MAX, "%s_REF", engine->name);
> +
> + wa_init_start(wal, name);
> + engine_init_workarounds(engine, wal);
> + wa_init_finish(wal);
> + }
> +}
> +
> +static void
> +reference_lists_fini(struct drm_i915_private *i915, struct wa_lists *lists)
> +{
> + struct intel_engine_cs *engine;
> + enum intel_engine_id id;
> +
> + for_each_engine(engine, i915, id)
> + intel_wa_list_free(&lists->engine[id].wa_list);
> +
> + intel_wa_list_free(&lists->gt_wa_list);
> +}
> +
> static struct drm_i915_gem_object *
> read_nonprivs(struct i915_gem_context *ctx, struct intel_engine_cs *engine)
> {
> @@ -326,15 +371,17 @@ static int live_reset_whitelist(void *arg)
> return err;
> }
>
> -static bool verify_gt_engine_wa(struct drm_i915_private *i915, const char *str)
> +static bool verify_gt_engine_wa(struct drm_i915_private *i915,
> + struct wa_lists *lists, const char *str)
> {
> struct intel_engine_cs *engine;
> enum intel_engine_id id;
> bool ok = true;
>
> - ok &= intel_gt_verify_workarounds(i915, str);
> + ok &= wa_list_verify(i915, &lists->gt_wa_list, str);
> +
> for_each_engine(engine, i915, id)
> - ok &= intel_engine_verify_workarounds(engine, str);
> + ok &= wa_list_verify(i915, &lists->engine[id].wa_list, str);
Any point in checking that the two copies of each list also match?
(Separate subtest, for easy problem detection.) Say rename
wa_list_verify to wa_list_verify_mmio and add a new wa_list_compare
which would compare the two lists.
Regards,
Tvrtko
>
> return ok;
> }
> @@ -344,6 +391,7 @@ live_gpu_reset_gt_engine_workarounds(void *arg)
> {
> struct drm_i915_private *i915 = arg;
> struct i915_gpu_error *error = &i915->gpu_error;
> + struct wa_lists lists;
> bool ok;
>
> if (!intel_has_gpu_reset(i915))
> @@ -353,17 +401,19 @@ live_gpu_reset_gt_engine_workarounds(void *arg)
>
> igt_global_reset_lock(i915);
> intel_runtime_pm_get(i915);
> + reference_lists_init(i915, &lists);
>
> - ok = verify_gt_engine_wa(i915, "before reset");
> + ok = verify_gt_engine_wa(i915, &lists, "before reset");
> if (!ok)
> goto out;
>
> set_bit(I915_RESET_HANDOFF, &error->flags);
> i915_reset(i915, ALL_ENGINES, "live_workarounds");
>
> - ok = verify_gt_engine_wa(i915, "after reset");
> + ok = verify_gt_engine_wa(i915, &lists, "after reset");
>
> out:
> + reference_lists_fini(i915, &lists);
> intel_runtime_pm_put(i915);
> igt_global_reset_unlock(i915);
>
> @@ -379,6 +429,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
> struct igt_spinner spin;
> enum intel_engine_id id;
> struct i915_request *rq;
> + struct wa_lists lists;
> int ret = 0;
>
> if (!intel_has_reset_engine(i915))
> @@ -390,13 +441,14 @@ live_engine_reset_gt_engine_workarounds(void *arg)
>
> igt_global_reset_lock(i915);
> intel_runtime_pm_get(i915);
> + reference_lists_init(i915, &lists);
>
> for_each_engine(engine, i915, id) {
> bool ok;
>
> pr_info("Verifying after %s reset...\n", engine->name);
>
> - ok = verify_gt_engine_wa(i915, "before reset");
> + ok = verify_gt_engine_wa(i915, &lists, "before reset");
> if (!ok) {
> ret = -ESRCH;
> goto err;
> @@ -404,7 +456,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
>
> i915_reset_engine(engine, "live_workarounds");
>
> - ok = verify_gt_engine_wa(i915, "after idle reset");
> + ok = verify_gt_engine_wa(i915, &lists, "after idle reset");
> if (!ok) {
> ret = -ESRCH;
> goto err;
> @@ -435,7 +487,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
> igt_spinner_end(&spin);
> igt_spinner_fini(&spin);
>
> - ok = verify_gt_engine_wa(i915, "after busy reset");
> + ok = verify_gt_engine_wa(i915, &lists, "after busy reset");
> if (!ok) {
> ret = -ESRCH;
> goto err;
> @@ -443,6 +495,7 @@ live_engine_reset_gt_engine_workarounds(void *arg)
> }
>
> err:
> + reference_lists_fini(i915, &lists);
> intel_runtime_pm_put(i915);
> igt_global_reset_unlock(i915);
> kernel_context_close(ctx);
>
More information about the Intel-gfx
mailing list