On 10/11/2022 11:07, Andrzej Hajda wrote:
On 09.11.2022 11:46, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Convert some usages of legacy DRM logging macros into versions which tell
us on which device have the events occurred.
v2:
* Don't have struct drm_device as local. (Jani, Ville)
v3:
* Store gt, not i915, in workaround list. (John)
Neither gt neither i915 does fit into wa list IMHO.
The best solution would be provide context (i915/gt/whatever)
as a function parameter, every time it is necessary.
On the other side it should not block the patch.
More below.
I thought about the very same lines but then concluded that the only _current_ usage of the lists is that they belong to a gt (directly or via engine). So having a back pointer felt passable.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx> # v2
Acked-by: Jani Nikula <jani.nikula@xxxxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +-
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 26 ++++++++----
.../drm/i915/gt/intel_execlists_submission.c | 13 +++---
drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 4 +-
drivers/gpu/drm/i915/gt/intel_gt.c | 4 +-
drivers/gpu/drm/i915/gt/intel_gt_irq.c | 8 ++--
drivers/gpu/drm/i915/gt/intel_rps.c | 6 ++-
drivers/gpu/drm/i915/gt/intel_workarounds.c | 42 +++++++++++--------
.../gpu/drm/i915/gt/intel_workarounds_types.h | 3 ++
.../gpu/drm/i915/gt/selftest_workarounds.c | 4 +-
drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/i915_getparam.c | 2 +-
drivers/gpu/drm/i915/i915_irq.c | 12 +++---
drivers/gpu/drm/i915/i915_perf.c | 14 ++++---
drivers/gpu/drm/i915/i915_query.c | 12 +++---
drivers/gpu/drm/i915/i915_sysfs.c | 3 +-
drivers/gpu/drm/i915/i915_vma.c | 16 +++----
drivers/gpu/drm/i915/intel_uncore.c | 21 ++++++----
19 files changed, 117 insertions(+), 81 deletions(-)
(...)
@@ -1749,7 +1755,7 @@ wa_list_apply(struct intel_gt *gt, const struct
i915_wa_list *wal)
intel_gt_mcr_read_any_fw(gt, wa->mcr_reg) :
intel_uncore_read_fw(uncore, wa->reg);
- wa_verify(wa, val, wal->name, "application");
+ wa_verify(wal->gt, wa, val, wal->name, "application");
This looks confusing at 1st sight, why wa_verify(wal->gt,...) and not
wa_verify(gt,...). Can they differ? and similar questions as in case of
redundant vars.
Would be always the same in current code. But point taken, it is confusing.. hm..
./gt/intel_workarounds.c: wa_list_apply(gt, >->wa_list);
./gt/intel_workarounds.c: wa_list_apply(engine->gt, &engine->wa_list);
Could drop the gt argument now that gt is available in the wa list.
The same apply to wal->engine_name, which is almost unused anyway?
Also AFAIK there is always sequence:
1. wa_init_start
2. *init_workarounds*
3. wa_init_finish - btw funny name.
Why funny? :) Because init collides with finish? Start of initialisation, initialisation, end of initialisation. :)
Why not 1 and 3 embed in 2? Do we need this sequence.
It's just some common code so it doesn't have to be duplicated in the callers.
Anyway all these comments are for wa handling, which should be addressed
in other patch. So my r-b still holds, either with wal->i915, either
with wal->gt.
Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
Thanks, I think I'll go with v3 and follow up with wa_list_apply cleanup, so that my logging changes in gt/ are in before further CI delays and people can freely work on the GT logging macros without conflicts.
Regards,
Tvrtko
