| Message ID | 20221118015858.2548106-1-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add module oriented dmesg output | expand |
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. 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(-)
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. 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(-)
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(-) >
On 18.11.2022 11: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. do you mean: intel_gt_print.h intel_guc_print.h intel_huc_print.h with just macros or also with all functions that work with drm_printer? > > 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. to solve this we should really consider splitting out GuC and HuC definitions to dedicated _types.h files and only include them in i915_drv.h (and print macros are orthogonal for this problem) Michal > > 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(-) >
On Tue, 22 Nov 2022, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote: > On 18.11.2022 11: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. > > do you mean: > > intel_gt_print.h > intel_guc_print.h > intel_huc_print.h > > with just macros or also with all functions that work with drm_printer? At least for the macros being added now. If adding others does not require you to pull in a bunch of additional header dependencies, you can add more. And that can be separate patches. > >> >> 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. > > to solve this we should really consider splitting out GuC and HuC > definitions to dedicated _types.h files and only include them in > i915_drv.h (and print macros are orthogonal for this problem) It's an orthogonal problem, but IMO with the current headers there's no reason to make the problem worse by adding somewhat independent new stuff to the headers. --- I've looked at untangling this a bunch of times, but I've always felt that it's really not my area of expertise, and it would inevitably conflict with someone else's work in progress and someone else's idea of how the headers should be refactored. There are chains like this: i915_drv.h:47:#include "gt/intel_gt_types.h" gt/intel_gt_types.h:19:#include "uc/intel_uc.h" gt/uc/intel_uc.h:9:#include "intel_guc.h" gt/uc/intel_guc.h:15:#include "intel_guc_fwif.h" gt/uc/intel_guc_fwif.h:14:#include "abi/guc_actions_abi.h" gt/uc/intel_guc_fwif.h:15:#include "abi/guc_actions_slpc_abi.h" gt/uc/intel_guc_fwif.h:16:#include "abi/guc_errors_abi.h" gt/uc/intel_guc_fwif.h:17:#include "abi/guc_communication_mmio_abi.h" gt/uc/intel_guc_fwif.h:18:#include "abi/guc_communication_ctb_abi.h" gt/uc/intel_guc_fwif.h:19:#include "abi/guc_klvs_abi.h" gt/uc/intel_guc_fwif.h:20:#include "abi/guc_messages_abi.h" They need to be broken up at some point. There are a bunch of headers where only minimal amount of info is actually needed in other headers, and the rest is used in a limited number of .c files only. It's a lot of tedious work to refactor and nobody's going to notice the impact directly, they'll just be less grumpy about the build being slow and the organization of the headers being messy. And if they don't build the driver a lot (like me) or don't refactor the driver a lot (like me) maybe they'll never notice. BR, Jani. > > Michal > >> >> 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(-) >>