Re: [Intel-gfx] [PATCH v3] drm/i915: Partial abandonment of legacy DRM logging macros

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/11/2022 11:07, Andrzej Hajda wrote:
> On 09.11.2022 11:46, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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@intel.com>
>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> # v2
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>   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, &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@intel.com>

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