On Mon, Sep 12, 2022 at 4:29 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
>
> On Sun, 11 Sep 2022, Jim Cromie <jim.cromie@xxxxxxxxx> wrote:
> > Use DECLARE_DYNDBG_CLASSMAP across DRM:
> >
> > - in .c files, since macro defines/initializes a record
> >
> > - in drivers, $mod_{drv,drm,param}.c
> > ie where param setup is done, since a classmap is param related
> >
> > - in drm/drm_print.c
> > since existing __drm_debug param is defined there,
> > and we ifdef it, and provide an elaborated alternative.
> >
> > - in drm_*_helper modules:
> > dp/drm_dp - 1st item in makefile target
> > drivers/gpu/drm/drm_crtc_helper.c - random pick iirc.
> >
> > Since these modules all use identical CLASSMAP declarations (ie: names
> > and .class_id's) they will all respond together to "class DRM_UT_*"
> > query-commands:
>
> The commit message could start off by saying each module needs to define
> DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, ...). That is, IIUC.
>
Yes, I see your point.
All the explanations missing here are in preceding commits,
now in GregKHs driver-core/driver-core-next tree,
so I didnt resend them.
> Where's DECLARE_DYNDBG_CLASSMAP defined? linux-next? What's it do? What
> if multiple modules with that are actually builtin?
The commit that adds the macro is at
https://lore.kernel.org/lkml/20220904214134.408619-15-jim.cromie@xxxxxxxxx/
there are many combos of builtin, Ive done at least several:
with caveat that >98% of testing is on virtme (thanks for that tool)
- test_dynamic_debug as module, and builtin.
it has multiple macro uses, showing 1 kind of sharing
- drm as builtin, drivers as modules
that surely pulled in other drm-helpers as builtins
- all loadable modules mostly.
>
> The duplication and requirement that they're identical seems like an
> error prone combo.
I freely acknowledge(d) this is sub-optimal.
There might be a best place for a single declaration that is in-scope
across multiple modules, but I dont know the drm core/driver lifetime
well enough to just drop this into that place.
I may have complicated things by starting with a static struct holding
the classmap, that choice was driven by:
- static declaration into a section solved a problem where the class
definitions
were "registered" (term used in patchset, -v2-3?) too late to be available for
modprobe i915 dyndbg='class DRM_UT_CORE +p'
but worked afterwards
(also true for builtins and boot-time $mod.dyndbg='class notworking +p')
Another subtlety - the "sharing" is due more to: drm_dbg(DRM_UT_*, "")
Im not sure precisely how this might matter.
I also had an "incompleteness" argument circling in my head - something like;
you cant simultaneously allow a drm-wanna-be module to declare "DRM_UT_CORE"
but disallow "DRM_UT_ILL_CONSIDERED". I kind-of stopped there.
Theres also an issue where multiple declarations in a module must
avoid range overlap.
I had no idea how to put that into a BUILD_BUG_ON.
Its done manually, with commentary, in test-dynamic-debug.
Maybe both issues can be improved somewhat by changing the macro
to expect real ENUM symbols, (and stringify _VA_ARGS_ to init the classnames),
not the quoted "DRM_UT_*"s it gets now. That would also obsolete the _base,
since its the value of DRM_UT_CORE (the 1st enum val).
But that still leaves the enum vals enumerated, with possibility of
omission or mixup,
which unlike a spelling error wouldnt get caught, and would be wrong.
I fiddled with the 1st part of that for a while, I lack the macro-fu,
and punted.
Im happy to try an alternative approach, particularly with elaborated
suggestions.
>
> Finally, the choice of placement in e.g. i915_params.c seems completely
> arbitrary, and makes you wonder "what here requires this, nothing?".
acknowledged - I put it there because the access to it is via a parameter,
namely one that already affects it from a distance:
/sys/module/drm/parameters/debug - ie drm.dbg
And its not even i915's parameter.
>
> BR,
> Jani.
>
thanks,
>
> >
> > Signed-off-by: Jim Cromie <jim.cromie@xxxxxxxxx>
