Re: [Intel-gfx] [PATCH v2 0/5] Add module oriented dmesg output

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

On 21/11/2022 18:21, John Harrison wrote:
> On 11/18/2022 02:52, Jani Nikula wrote:
>> On Thu, 17 Nov 2022, John.C.Harrison@Intel.com wrote:
>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>
>>> When trying to analyse bug reports from CI, customers, etc. it can be
>>> difficult to work out exactly what is happening on which GT in a
>>> multi-GT system. So add GT oriented debug/error message wrappers. If
>>> used instead of the drm_ equivalents, you get the same output but with
>>> a GT# prefix on it.
>>>
>>> It was also requested to extend this further to submodules in order to
>>> factor out the repeated structure accessing constructs and common
>>> string prefixes. So, add versions for GuC, HuC and GuC CTB as well.
>>>
>>> This patch set updates all the gt/uc files to use the new helpers as a
>>> first step. The intention would be to convert all output messages that
>>> have access to a GT structure.
>>>
>>> v2: Go back to using lower case names, add more wrapper sets (combined
>>> review feedback). Also, wrap up probe injection and WARN entries.
>>>
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> For adding the wrappers in general, I'm going to disagree and
>> commit. I'll leave it up to Tvrtko and Joonas.
>>
>> Regarding the placement of the macros, I insist you add individual
>> header files for the wrappers and include them only where needed.
>>
>> We have a fairly serious problem with everything including everything in
>> i915 that I've been slowly trying to tackle. Touch one thing, rebuild
>> everything. About a third of our headers cause the rebuild of the entire
>> driver when modified. We need to reduce the surface of things that cause
>> rebuilds.
>>
>> For example, intel_gt.h is included by 97 files, intel_guc.h by 332
>> files, and intel_huc.h by 329 files (counting recursively).
>>
>> There's absolutely no reason any of the display code, for example, needs
>> to have these logging macros in their build. Long term, the headers
>> should be reorganized to reduce the interdependencies, and this is what
>> I've been doing in i915_drv.h and display/ in general. But the least we
>> can do is not make the problem worse.
> @Tvrtko/@Michal W, any other review comments or feedback? I'd rather not 
> spend time fixing up the header file issue and reposting only to have 
> someone point out another issue that could have been resolved at the 
> same time.

I read through the patches when you posted them and it looked nice and 
clean to me. I think I spotted one instance of a debug build only 
message getting upgraded to production build, and one loss of stack 
trace on a warning, but it wasn't a concern to me AFAIR.

Regards,

Tvrtko

> 
> John.
> 
>> BR,
>> Jani.
>>
>>>
>>> John Harrison (5):
>>>    drm/i915/gt: Start adding module oriented dmesg output
>>>    drm/i915/huc: Add HuC specific debug print wrappers
>>>    drm/i915/guc: Add GuC specific debug print wrappers
>>>    drm/i915/guc: Add GuC CT specific debug print wrappers
>>>    drm/i915/uc: Update the gt/uc code to use gt_err and friends
>>>
>>>   drivers/gpu/drm/i915/gt/intel_gt.c            |  96 ++++----
>>>   drivers/gpu/drm/i915/gt/intel_gt.h            |  35 +++
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc.c        |  32 +--
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc.h        |  35 +++
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    |   8 +-
>>>   .../gpu/drm/i915/gt/uc/intel_guc_capture.c    |  48 ++--
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c     | 222 +++++++++---------
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c     |  19 +-
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_log.c    |  37 ++-
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c     |   7 +-
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c   |  55 ++---
>>>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  62 +++--
>>>   drivers/gpu/drm/i915/gt/uc/intel_huc.c        |  31 +--
>>>   drivers/gpu/drm/i915/gt/uc/intel_huc.h        |  23 ++
>>>   drivers/gpu/drm/i915/gt/uc/intel_uc.c         | 108 ++++-----
>>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c      |  98 ++++----
>>>   drivers/gpu/drm/i915/gt/uc/selftest_guc.c     |  34 +--
>>>   .../drm/i915/gt/uc/selftest_guc_hangcheck.c   |  22 +-
>>>   .../drm/i915/gt/uc/selftest_guc_multi_lrc.c   |  10 +-
>>>   19 files changed, 507 insertions(+), 475 deletions(-)
>