Re: [PATCH 3/3] drm/i915: Add engine name to workaround debug print

Tvrtko Ursulin
2019-07-03T13:53:12Z

On 03/07/2019 08:50, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> There is a debug message in the workaround initialisation path that
> reports how many entries were added of each type. However, whitelist
> workarounds exist for multiple engines but the type name is just
> 'whitelist'. Tvrtko suggested adding the engine name to make the
> message more useful.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_workarounds.c | 15 ++++++++-------
> drivers/gpu/drm/i915/gt/intel_workarounds_types.h | 1 +
> drivers/gpu/drm/i915/gt/selftest_workarounds.c | 13 +++----------
> 3 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 9b401833aceb..cf18bb962708 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -50,9 +50,10 @@
> * - Public functions to init or apply the given workaround type.
> */
>
> -static void wa_init_start(struct i915_wa_list *wal, const char *name)
> +static void wa_init_start(struct i915_wa_list *wal, const char *name, const char *engine_name)
> {
> wal->name = name;
> + wal->engine_name = engine_name;
> }
>
> #define WA_LIST_CHUNK (1 << 4)
> @@ -74,8 +75,8 @@ static void wa_init_finish(struct i915_wa_list *wal)
> if (!wal->count)
> return;
>
> - DRM_DEBUG_DRIVER("Initialized %u %s workarounds\n",
> - wal->wa_count, wal->name);
> + DRM_DEBUG_DRIVER("Initialized %u %s workarounds on %s\n",
> + wal->wa_count, wal->name, wal->engine_name);
> }
>
> static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa)
> @@ -591,7 +592,7 @@ __intel_engine_init_ctx_wa(struct intel_engine_cs *engine,
> if (engine->class != RENDER_CLASS)
> return;
>
> - wa_init_start(wal, name);
> + wa_init_start(wal, name, engine->name);
>
> if (IS_GEN(i915, 11))
> icl_ctx_workarounds_init(engine, wal);
> @@ -921,7 +922,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");
> + wa_init_start(wal, "GT", "global");
> gt_init_workarounds(i915, wal);
> wa_init_finish(wal);
> }
> @@ -1192,7 +1193,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");
> + wa_init_start(w, "whitelist", engine->name);
>
> if (IS_GEN(i915, 11))
> icl_whitelist_build(engine);
> @@ -1384,7 +1385,7 @@ void intel_engine_init_workarounds(struct intel_engine_cs *engine)
> if (GEM_WARN_ON(INTEL_GEN(engine->i915) < 8))
> return;
>
> - wa_init_start(wal, engine->name);
> + wa_init_start(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 42ac1fb99572..e27ab1b710b3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds_types.h
> @@ -20,6 +20,7 @@ struct i915_wa {
>
> struct i915_wa_list {
> const char *name;
> + const char *engine_name;
> struct i915_wa *list;
> unsigned int count;
> unsigned int wa_count;
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index 5cd2b17105ba..ba95d9e28f8a 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -25,11 +25,9 @@ static const struct wo_register {
> { INTEL_GEMINILAKE, 0x731c }
> };
>
> -#define REF_NAME_MAX (INTEL_ENGINE_CS_MAX_NAME + 8)
> struct wa_lists {
> struct i915_wa_list gt_wa_list;
> struct {
> - char name[REF_NAME_MAX];
> struct i915_wa_list wa_list;
> struct i915_wa_list ctx_wa_list;
> } engine[I915_NUM_ENGINES];
> @@ -43,25 +41,20 @@ reference_lists_init(struct drm_i915_private *i915, struct wa_lists *lists)
>
> memset(lists, 0, sizeof(*lists));
>
> - wa_init_start(&lists->gt_wa_list, "GT_REF");
> + wa_init_start(&lists->gt_wa_list, "GT_REF", "global");
> 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);
> + wa_init_start(wal, "REF", engine->name);
> engine_init_workarounds(engine, wal);
> wa_init_finish(wal);
>
> - snprintf(name, REF_NAME_MAX, "%s_CTX_REF", engine->name);
> -
> __intel_engine_init_ctx_wa(engine,
> &lists->engine[id].ctx_wa_list,
> - name);
> + "CTX_REF");
> }
> }
>
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko