Thanks for the feedback!
On Tue, Jul 20, 2021 at 9:29 AM Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Jul 14, 2021 at 11:51:36AM -0600, Jim Cromie wrote:
drm's debug system uses distinct categories of debug messages, encoded in an enum (DRM_UT_<CATEGORY>), which are mapped to bits in drm.debug. drm_debug_enabled() does a lot of unlikely bit-mask checks on drm.debug; we can use dynamic debug instead, and get all that static_key/jump_label goodness.
Dynamic debug has no concept of category, but we can map the DRM_UT_* to a set of distinct prefixes; "drm:core:", "drm:kms:" etc, and prepend them to the given formats.
Then we can use: `echo module drm format ^drm:core: +p > control`
to enable every such "prefixed" pr_debug with one query. This new prefix changes pr_debug's output, so is user visible, but it seems unlikely to cause trouble for log watchers; they're not relying on the absence of class prefix strings.
This conversion yields ~2100 new callsites on my i7/i915 laptop:
dyndbg: 195 debug prints in module drm_kms_helper dyndbg: 298 debug prints in module drm dyndbg: 1630 debug prints in module i915
CONFIG_DRM_USE_DYNAMIC_DEBUG enables this, and is available if CONFIG_DYNAMIC_DEBUG or CONFIG_DYNAMIC_DEBUG_CORE is chosen, and if CONFIG_JUMP_LABEL is enabled; this because its required to get the promised optimizations.
The indirection/switchover is layered into the macro scheme:
- A new callback on drm.debug which calls dynamic_debug_exec_queries to map those bits to specific query/commands dynamic_debug_exec_queries("format ^drm:kms: +p", "drm*"); here for POC, this should be in dynamic_debug.c with a MODULE_PARAM_DEBUG_BITMAP(__drm_debug, { "prefix-1", "desc-1" }+)
This is really awesome. For merging I think we need to discuss with dyn debug folks whether they're all ok with this, but it's exported already should should be fine.
Yay. FWIW, Im to blame for that export, with this use case in mind. That said, I dont know if that macro can be written as I described, but if not, then { BIT(0), "prefix-0", "description-0" }, { BIT(1) .... } should work. If its been done elsewhere, Id copy it, or imitate it.
A "converted" or "classy" DRM_UT_* map
based on: DRM_UT_* ( symbol => bit-mask ) named it: cDRM_UT_* ( symbol => format-class-prefix-string )
So cDRM_UT_* is either: legacy: cDRM_UT_* <-- DRM_UT_* ( !CONFIG_DRM_USE_DYNAMIC_DEBUG ) enabled: #define cDRM_UT_KMS "drm:kms: " #define cDRM_UT_PRIME "drm:prime: " #define cDRM_UT_ATOMIC "drm:atomic: "
the cDRM looks a bit funny, plus I don't eve have an idea what _UT_ means (and git history isn't helpful either). What about just using DRM_DBG_CLASS_ as the prefix here for these indirection macros, i.e. DRM_DBG_CLASS_KMS.
yes.
Also would be really nice if we could make these a table or something, but I guess with the macro magic that's not possible.
not obvious to me, I'll watch for an opportunity.
DRM_UT_* are unchanged, since theyre used in drm_debug_enabled() and elsewhere.
I think for the production version of these we need to retire/deprecate them, at least for drm core. Otherwise you have an annoying mismatch between drm.debug module option and dyn debug.
I will look at renaming it : __drm_debug_enabled and making a macro for the old name. so enabled, it would end up like if (unlikely(1) && ... )
drivers/gpu/drm/drm_atomic_uapi.c 1457: if (drm_debug_enabled(DRM_UT_STATE))
drivers/gpu/drm/drm_dp_mst_topology.c 1358: if (unlikely(ret == -EIO) && drm_debug_enabled(DRM_UT_DP)) { 2875: if (unlikely(ret) && drm_debug_enabled(DRM_UT_DP)) { 2919: if (drm_debug_enabled(DRM_UT_DP)) {
drm_dev_dbg & drm_debug are renamed (prefixed with '_')
old names are now macros, calling either: legacy: -> to renamed fn enabled: -> dev_dbg & pr_debug, with cDRM-prefix # format.
these names are used in a fat layer of macros (3) which supply the category; those macros are used throughout drm code, yielding the ~2100 new prdbgs reported above.
names in (2) are invoked by DRM_DEBUG_<Category>, drm_dbg_<Category>.
all these macros get "converted" to use cDRM_UT_* to get right token type for both !/!! DRM_USE_DYNAMIC_DEBUG
simplification of __DRM_DEFINE_DBG_RATELIMITED macro
remove DRM_UT_ ## KMS as extra indirection pass both DRM_UT & cDRM_UT, for drm_debug_enabled & drm_dev_dbg
For merging, can we pull out the renames and reorgs from this patch, and then maybe also the reorder the next patch in your series here to be before the dyn debug stuff?
I will drop the 4. RATELIMITED tweaks. FWIW, I have semi-working code to implement rate limiting in dynamic debug, controlled by `+r > control` which would touch this anyway.
wrt reordering 4/5, can you clarify ? the i915 changes are 1/2 POC, so I'll assume you mean after getting 0. into dyndbg
Signed-off-by: Jim Cromie jim.cromie@gmail.com
drivers/gpu/drm/Kconfig | 13 +++++ drivers/gpu/drm/drm_print.c | 75 ++++++++++++++++++++++++-- include/drm/drm_print.h | 102 ++++++++++++++++++++++++++---------- 3 files changed, 158 insertions(+), 32 deletions(-)
I really like this, I think you can drop the RFC. A few more things that I think we need:
- An overview kerneldoc section which explains the interfaces and how it all works together. Essentially your commit message with some light markup to make it look good.
into include/drm/drm_print.h ?
- I think it would be really good to review the driver docs for all this and make sure it's complete. Some of the interface functions aren't documented yet (or maybe the ones that drivers shouldn't used need more __ prefixes to denote them as internal, dunno).
I will look, but I dont have the experience here to make these qualitative judgements
Also, I renamed drm_dev_dbg to _drm_dev_dbg, with a single underscore. I'm now thinking this is reserved, and __ prefix is better (legal) This isnt quite the same is lower-level implementation detail, since it adds a CONFIG dependent alternative impl. But I'll do this unless you have a better name
- I guess deprecation notice for drm_debug_enabled() and all that, so that we have a consistent interface. Doing the conversion will probably highlight the need for a bit more infrastructure and tooling, e.g. the bigger dump functions (like edid hex dump, or also the various decode helpers we have for dp, hdmi infoframes and all that) ideally have a single dyn_debug label to enable all of them instead of line-by-line. Tbh no idea how this should work, might need dyndbg work too.
macrofying drm_debug_enabled() might work for this set. possibly with different prefixes, forex: "drmx:*" or "drm:misc:"
- For the driver side of this we probably want a Documentation/gpu/TODO.rst entry if it's not all easy to convert directly.
so there are 32 uses of drm_debug_enabled(DRM_UT_*) and just 1 used in drm_dev_dbg (with category as arg) I'll try to macrofy it, see if it will handle the 32 cases.
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 7ff89690a976..e4524ccba040 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -57,6 +57,19 @@ config DRM_DEBUG_MM
If in doubt, say "N".+config DRM_USE_DYNAMIC_DEBUG
bool "use dynamic debug to implement drm.debug"default ndepends on DRMdepends on DYNAMIC_DEBUG || DYNAMIC_DEBUG_COREdepends on JUMP_LABELhelpThe drm debug category facility does a lot of unlikely bit-fieldtests at runtime; while cheap individually, the cost accumulates.This option uses dynamic debug facility (if configured andusing jump_label) to avoid those runtime checks, patchingthe kernel when those debugs are desired.Can't we just make this an internal option that's enabled automatically when dyndbg is around? Plus a comment somewhere that we really recommend enabling dyndbg for drm. Or would this mean that in certain dyndbg configurations we'd loose all the debug lines, which would suck?
We could indeed, I took the cautious approach. keeping the CONFIG simplifies comparing DRM_USE_DD=y/n builds, and changing default later is easy, and probably should have some numbers about instructions saved and obj size increase.
Anyway there's a pile of details, but the big picture I really like. Especially that we can make dyndbg seamlessly support drm.debug is really nice.
Cheers, Daniel
thanks, Jim