Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jim Cromie <jim.cromie@gmail.com>
To:     daniel@ffwll.ch, Jim Cromie <jim.cromie@gmail.com>
Cc:     airlied@linux.ie, dri-devel@lists.freedesktop.org,
        intel-gfx@lists.freedesktop.org,
        intel-gvt-dev@lists.freedesktop.org, jani.nikula@linux.intel.com,
        jbaron@akamai.com, joonas.lahtinen@linux.intel.com,
        linux-kernel@vger.kernel.org, maarten.lankhorst@linux.intel.com,
        mripard@kernel.org, rodrigo.vivi@intel.com, tzimmermann@suse.de,
        zhenyuw@linux.intel.com, zhi.a.wang@intel.com
Subject: Re: [PATCH v3 3/5] drm/print: RFC add choice to use dynamic debug in drm-debug
Date:   Sat,  5 Mar 2022 09:06:05 -0700
Message-Id: <20220305160605.88922-1-jim.cromie@gmail.com>
In-Reply-To: <YPbPvm/xcBlTK1wq@phenom.ffwll.local>
References: <YPbPvm/xcBlTK1wq@phenom.ffwll.local>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Daniel Vetter <daniel@ffwll.ch>


Hi Daniel, everyone,

Ive substantially updated this patchset,
and I thought it useful to reply here.

Im noting the biggest changes in context below, trimming heavily otherwize.

Also, Ive lost the msg in my gmail-cloud, so this lacks
the usual "Daniel said: " attribution, and the "> " marks.
Ive prefixed "< " to 1st line of my replies where it helps.

The current patchset is here:

https://patchwork.freedesktop.org/series/100290/
https://lore.kernel.org/lkml/20220217034829.64395-1-jim.cromie@gmail.com/

Its "failing" patchwork CI cuz of a macro warning in dyndbg, which
would be nice for CI to "pass" because its out of DRM tree, and
subject to a separate review process, and in the meantime, it would be
nice to see it under test.


Going item by item:

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>),

  enum is now renumbered to 0-15 (fits in 4 bits)
  drm_debug_enabled() does BIT(cat)
  15 is unclassed/reserved

> Dynamic debug has no concept of category, but we can map the DRM_UT_*

now it has "class" keyword, and class_id:4 field.

   #> echo module drm class 1 +p > control # 1=DRM_UT_KMS iirc

This interface is unused yet, DRM is its A-1 customer, are you shopping ?
Since its unused, it cannot break anything not using it :-)

> CONFIG_DRM_USE_DYNAMIC_DEBUG enables this, and is available if

still true.

> The indirection/switchover is layered into the macro scheme:

Heres where the biggest DRM centric changes (vs old) are:

The cDRM_UT_* => prefix-string mapping is gone; while it worked, it
made the literal format strings config dependent, and was more
complicated to explain.  Fitting category into class_id is much
cleaner.

old way replaced drm*dbg with pr_debug (at surface level)
new way adapts drm*dbg to reuse the class-Factory macro under pr_debug.

This loses pr_debug's log-decorating feature, but DRM has its own
conventions, and extra decorations are unlikely to really add anything.
OTOH, drm could re-use those flags to optionalize some of its conventions.

> 0. A new callback on drm.debug which calls dynamic_debug_exec_queries

now in dyndbg, where it belonged.  now just uses class query inside.


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.

< Your (fresh) endorsement should help :-)
Particularly, the new dyndbg features need a user.

> 
> 1. A "converted" or "classy" DRM_UT_* map
>

repeating, 2nd map is gone, DRM_UT_* is merely renumbered.

>    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 think this isnt relevant now, since symbols are preserved.

Also, the __drm_debug var is used directly by the CLASSBITS macro,
(and therefore the callbacks), so /sys/module/drm/parameters/debug is
preserved at an ABI-ish level. (__drm_debug now ulong, to work with BIT)

> 
> 2. 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.
>

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?

< the above adaptation is re-adapted to use dyndbg's new class-Factory
macro, other stuff is gone.


> 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(-)

update - drm parts are slightly smaller now :-)

[jimc@frodo wk-next]$ git diff --stat master
 Documentation/admin-guide/dynamic-debug-howto.rst |   7 +++++
 drivers/gpu/drm/Kconfig                           |  12 ++++++++
 drivers/gpu/drm/Makefile                          |   2 ++
 drivers/gpu/drm/drm_print.c                       |  56 ++++++++++++++++++++++------------
 include/drm/drm_print.h                           |  80 +++++++++++++++++++++++++++++
 include/linux/dynamic_debug.h                     | 113 +++++++++++++++++++++++++++++
 lib/dynamic_debug.c                               | 140 +++++++++++++++++++++++++++++
 7 files changed, 323 insertions(+), 87 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.

< not sure anything worth documenting has survived.

- 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 guess deprecation notice for drm_debug_enabled() and all that, so that
  we have a consistent interface. Doing the conversion will probably

< this keeps drm_debug_enabled(), and __drm_debug, so those users are fine.
__drm_debug_enabled() is optimized version for macro-wrapped sites

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.

< Factory-macros should help here, havent looked.

- For the driver side of this we probably want a
  Documentation/gpu/TODO.rst entry if it's not all easy to convert
  directly.


> +config DRM_USE_DYNAMIC_DEBUG
> +	bool "use dynamic debug to implement drm.debug"
> +	default n
> +	depends on DRM
> +	depends on DYNAMIC_DEBUG || DYNAMIC_DEBUG_CORE
> +	depends on JUMP_LABEL
> +	help
> +	  The drm debug category facility does a lot of unlikely bit-field
> +	  tests at runtime; while cheap individually, the cost accumulates.
> +	  This option uses dynamic debug facility (if configured and
> +	  using jump_label) to avoid those runtime checks, patching
> +	  the 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?

< I have 'default y' now.

WRT losing debug lines, I cant think how, but it raises a thought; cuz
drm.debug uses .class_id, the mod,func,file info isnt needed to
support queries (other than ad-hoc at >control), drm could drop them.
Its already lost pr_debug's optional mod.func.ln prefix, and cannot
use them for that.  (but Im foreshadowing, see bottom)


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


< Wait, theres more ! (here comes the tease) :-D

0. this patchset and others below are at:

ghlinux-ro	https://github.com/jimc/linux.git (fetch)

lkp-robot <lkp@intel.com> has been helping a lot.

1. dd-drm branch - this patchset

commit 46a5bd89e47d0f7e4ad63b4f574815f01d4290a0
(HEAD -> dd-drm, ghlinux/dd-drm, ghlinux-ro/dd-drm)

passing robot tests.

2. dyn-drm-trc branch - on top of 1.dd-drm

commit 0473a8ecdb15ec0e63644b77da575905c5851462
(ghlinux/dyn-drm-trc, ghlinux-ro/dyn-drm-trc, dyn-drm-trc)

It starts with Vincent Whitchurch's patch:
  https://lore.kernel.org/lkml/20211209150910.GA23668@axis.com/

which (oversimplifying) added a trace-event and called it from
pr_debug, and which met with some approval from Steve Rostedt.

It then recapitulates those changes in drm_*dbg.

3. dd-diet-next - on top of 2.dyn-drm-trc.

passes lkp-robot tests, but still problematic

This splits struct _ddebug in 2, then de-duplicates the _ddebug_site
records (containing repetitive module,filename,function).  In my
fedora-ish config kernel containing ~3100 callsites in builtin
modules, I see ~40% reduction in 24/64 of the full data footprint.
Id expect a little more in modules, due to higher prdbgs/fns ratio.

forex:  (69% compression here)
  dyndbg: 3883 debug prints in module amdgpu (in 1190 functions)

Its incomplete 2x: needs some union-fu, and more work to drop the
.site pointer.  This would drop 8/40 from struct _ddebug, recover another 8/56 of total footprint.

A noteworthy side-effect of this is that it becomes practical to drop
all _ddebug_site data for modules which don't need/want it.

DRM (in 2.dyn-drm-trc) is such a user; it gets the control it needs
using .class_id, and has no log-decoration facility which would use
the 3 fields.  It should just get lots of "_na_"s in cat control.

Dyndbg can be easily fixed to not apply query-commands which specify
those terms to callsites which do not have the data.


Clearly, getting to here is gonna be a many-step process.
DRM buy-in will certainly help :-)


Cheers, Jim