| Message ID | 20240702215804.2201271-31-jim.cromie@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fix CONFIG_DRM_USE_DYNAMIC_DEBUG=y | expand |
On Tue, Jul 02, 2024 at 03:57:20PM -0600, Jim Cromie wrote: > dyndbg's CLASSMAP-v1 api was broken; DECLARE_DYNDBG_CLASSMAP tried to > do too much. Its replaced by DRM_CLASSMAP_DEFINE, which creates & > EXPORTs the classmap when CONFIG_DRM_USE_DYNAMIC_DEBUG=y, for direct > reference by drivers. > > The drivers still use DECLARE_DYNDBG_CLASSMAP for now, so they still > redundantly re-declare the classmap, but we can convert the drivers > later to DYNDBG_CLASSMAP_USE > > Signed-off-by: Jim Cromie <jim.cromie@gmail.com> > --- > drivers/gpu/drm/drm_print.c | 25 +++++++++++++------------ > include/drm/drm_print.h | 8 ++++++++ > 2 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > index 699b7dbffd7b..4a5f2317229b 100644 > --- a/drivers/gpu/drm/drm_print.c > +++ b/drivers/gpu/drm/drm_print.c > @@ -55,18 +55,19 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat > #if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG) > module_param_named(debug, __drm_debug, ulong, 0600); > #else > -/* classnames must match vals of enum drm_debug_category */ > -DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0, > - "DRM_UT_CORE", > - "DRM_UT_DRIVER", > - "DRM_UT_KMS", > - "DRM_UT_PRIME", > - "DRM_UT_ATOMIC", > - "DRM_UT_VBL", > - "DRM_UT_STATE", > - "DRM_UT_LEASE", > - "DRM_UT_DP", > - "DRM_UT_DRMRES"); > +/* classnames must match value-symbols of enum drm_debug_category */ > +DRM_CLASSMAP_DEFINE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, > + DRM_UT_CORE, > + "DRM_UT_CORE", > + "DRM_UT_DRIVER", > + "DRM_UT_KMS", > + "DRM_UT_PRIME", > + "DRM_UT_ATOMIC", > + "DRM_UT_VBL", > + "DRM_UT_STATE", > + "DRM_UT_LEASE", > + "DRM_UT_DP", > + "DRM_UT_DRMRES"); Looks like this stuff just ends up in an array, so presumably it should be possible to use designated initializers to make this less fragile?
On Tue, Jul 2, 2024 at 5:33 PM Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Tue, Jul 02, 2024 at 03:57:20PM -0600, Jim Cromie wrote: > > dyndbg's CLASSMAP-v1 api was broken; DECLARE_DYNDBG_CLASSMAP tried to > > do too much. Its replaced by DRM_CLASSMAP_DEFINE, which creates & > > EXPORTs the classmap when CONFIG_DRM_USE_DYNAMIC_DEBUG=y, for direct > > reference by drivers. > > > > The drivers still use DECLARE_DYNDBG_CLASSMAP for now, so they still > > redundantly re-declare the classmap, but we can convert the drivers > > later to DYNDBG_CLASSMAP_USE > > > > Signed-off-by: Jim Cromie <jim.cromie@gmail.com> > > --- > > drivers/gpu/drm/drm_print.c | 25 +++++++++++++------------ > > include/drm/drm_print.h | 8 ++++++++ > > 2 files changed, 21 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > > index 699b7dbffd7b..4a5f2317229b 100644 > > --- a/drivers/gpu/drm/drm_print.c > > +++ b/drivers/gpu/drm/drm_print.c > > @@ -55,18 +55,19 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat > > #if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG) > > module_param_named(debug, __drm_debug, ulong, 0600); > > #else > > -/* classnames must match vals of enum drm_debug_category */ > > -DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0, > > - "DRM_UT_CORE", > > - "DRM_UT_DRIVER", > > - "DRM_UT_KMS", > > - "DRM_UT_PRIME", > > - "DRM_UT_ATOMIC", > > - "DRM_UT_VBL", > > - "DRM_UT_STATE", > > - "DRM_UT_LEASE", > > - "DRM_UT_DP", > > - "DRM_UT_DRMRES"); > > +/* classnames must match value-symbols of enum drm_debug_category */ > > +DRM_CLASSMAP_DEFINE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, > > + DRM_UT_CORE, > > + "DRM_UT_CORE", > > + "DRM_UT_DRIVER", > > + "DRM_UT_KMS", > > + "DRM_UT_PRIME", > > + "DRM_UT_ATOMIC", > > + "DRM_UT_VBL", > > + "DRM_UT_STATE", > > + "DRM_UT_LEASE", > > + "DRM_UT_DP", > > + "DRM_UT_DRMRES"); > > Looks like this stuff just ends up in an array, so presumably > it should be possible to use designated initializers to make this > less fragile? Im not sure I got your whole point, but: the fragility is the repetitive re-statement of the map, in those un-modified DECLARE_s, once replaced, the USEs just ref the struct built by the _DEFINE (once, and exported) I dont really like the _DEFINEs restatement of the enum-values: DRM_UT_* especially as "strings". I can automate the stringification with an APPLY_FN_(__stringify, ...) but the enum-list DRM_UT_* (w.o quotes) is still needed as args. unless there is something C can do thats like Enum.values() ? > > -- > Ville Syrjälä > Intel
On Tue, Jul 02, 2024 at 08:34:39PM -0600, jim.cromie@gmail.com wrote: > On Tue, Jul 2, 2024 at 5:33 PM Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > > > On Tue, Jul 02, 2024 at 03:57:20PM -0600, Jim Cromie wrote: > > > dyndbg's CLASSMAP-v1 api was broken; DECLARE_DYNDBG_CLASSMAP tried to > > > do too much. Its replaced by DRM_CLASSMAP_DEFINE, which creates & > > > EXPORTs the classmap when CONFIG_DRM_USE_DYNAMIC_DEBUG=y, for direct > > > reference by drivers. > > > > > > The drivers still use DECLARE_DYNDBG_CLASSMAP for now, so they still > > > redundantly re-declare the classmap, but we can convert the drivers > > > later to DYNDBG_CLASSMAP_USE > > > > > > Signed-off-by: Jim Cromie <jim.cromie@gmail.com> > > > --- > > > drivers/gpu/drm/drm_print.c | 25 +++++++++++++------------ > > > include/drm/drm_print.h | 8 ++++++++ > > > 2 files changed, 21 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > > > index 699b7dbffd7b..4a5f2317229b 100644 > > > --- a/drivers/gpu/drm/drm_print.c > > > +++ b/drivers/gpu/drm/drm_print.c > > > @@ -55,18 +55,19 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat > > > #if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG) > > > module_param_named(debug, __drm_debug, ulong, 0600); > > > #else > > > -/* classnames must match vals of enum drm_debug_category */ > > > -DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0, > > > - "DRM_UT_CORE", > > > - "DRM_UT_DRIVER", > > > - "DRM_UT_KMS", > > > - "DRM_UT_PRIME", > > > - "DRM_UT_ATOMIC", > > > - "DRM_UT_VBL", > > > - "DRM_UT_STATE", > > > - "DRM_UT_LEASE", > > > - "DRM_UT_DP", > > > - "DRM_UT_DRMRES"); > > > +/* classnames must match value-symbols of enum drm_debug_category */ > > > +DRM_CLASSMAP_DEFINE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, > > > + DRM_UT_CORE, > > > + "DRM_UT_CORE", > > > + "DRM_UT_DRIVER", > > > + "DRM_UT_KMS", > > > + "DRM_UT_PRIME", > > > + "DRM_UT_ATOMIC", > > > + "DRM_UT_VBL", > > > + "DRM_UT_STATE", > > > + "DRM_UT_LEASE", > > > + "DRM_UT_DP", > > > + "DRM_UT_DRMRES"); > > > > Looks like this stuff just ends up in an array, so presumably > > it should be possible to use designated initializers to make this > > less fragile? > > Im not sure I got your whole point, but: I mean using [DRM_UT_CORE] = "DRM_UT_CORE" instead of "DRM_UT_CORE" so there is no chance of screwing up the order. Or maybe the order doesn't even matter here? Could also stringify to avoid accidental of typos. > > the fragility is the repetitive re-statement of the map, > in those un-modified DECLARE_s, > once replaced, the USEs just ref the struct built by the _DEFINE > (once, and exported) > > I dont really like the _DEFINEs restatement of the enum-values: DRM_UT_* > especially as "strings". > I can automate the stringification with an APPLY_FN_(__stringify, ...) > but the enum-list DRM_UT_* (w.o quotes) is still needed as args. > > unless there is something C can do thats like Enum.values() ? > > > > > > > -- > > Ville Syrjälä > > Intel
Got it. I had some mental block about passing designated intializers as macro args. it just worked, I needed to eyeball the .i file just to be sure. thanks. I have a fixup patch. whats the best thing to do with it, squash it in for later ? send in reply here ? On Wed, Jul 3, 2024 at 5:52 AM Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Tue, Jul 02, 2024 at 08:34:39PM -0600, jim.cromie@gmail.com wrote: > > On Tue, Jul 2, 2024 at 5:33 PM Ville Syrjälä > > <ville.syrjala@linux.intel.com> wrote: > > > > > > On Tue, Jul 02, 2024 at 03:57:20PM -0600, Jim Cromie wrote: > > > > dyndbg's CLASSMAP-v1 api was broken; DECLARE_DYNDBG_CLASSMAP tried to > > > > do too much. Its replaced by DRM_CLASSMAP_DEFINE, which creates & > > > > EXPORTs the classmap when CONFIG_DRM_USE_DYNAMIC_DEBUG=y, for direct > > > > reference by drivers. > > > > > > > > The drivers still use DECLARE_DYNDBG_CLASSMAP for now, so they still > > > > redundantly re-declare the classmap, but we can convert the drivers > > > > later to DYNDBG_CLASSMAP_USE > > > > > > > > Signed-off-by: Jim Cromie <jim.cromie@gmail.com> > > > > --- > > > > drivers/gpu/drm/drm_print.c | 25 +++++++++++++------------ > > > > include/drm/drm_print.h | 8 ++++++++ > > > > 2 files changed, 21 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > > > > index 699b7dbffd7b..4a5f2317229b 100644 > > > > --- a/drivers/gpu/drm/drm_print.c > > > > +++ b/drivers/gpu/drm/drm_print.c > > > > @@ -55,18 +55,19 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat > > > > #if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG) > > > > module_param_named(debug, __drm_debug, ulong, 0600); > > > > #else > > > > -/* classnames must match vals of enum drm_debug_category */ > > > > -DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0, > > > > - "DRM_UT_CORE", > > > > - "DRM_UT_DRIVER", > > > > - "DRM_UT_KMS", > > > > - "DRM_UT_PRIME", > > > > - "DRM_UT_ATOMIC", > > > > - "DRM_UT_VBL", > > > > - "DRM_UT_STATE", > > > > - "DRM_UT_LEASE", > > > > - "DRM_UT_DP", > > > > - "DRM_UT_DRMRES"); > > > > +/* classnames must match value-symbols of enum drm_debug_category */ > > > > +DRM_CLASSMAP_DEFINE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, > > > > + DRM_UT_CORE, > > > > + "DRM_UT_CORE", > > > > + "DRM_UT_DRIVER", > > > > + "DRM_UT_KMS", > > > > + "DRM_UT_PRIME", > > > > + "DRM_UT_ATOMIC", > > > > + "DRM_UT_VBL", > > > > + "DRM_UT_STATE", > > > > + "DRM_UT_LEASE", > > > > + "DRM_UT_DP", > > > > + "DRM_UT_DRMRES"); > > > > > > Looks like this stuff just ends up in an array, so presumably > > > it should be possible to use designated initializers to make this > > > less fragile? > > > > Im not sure I got your whole point, but: > > I mean using > [DRM_UT_CORE] = "DRM_UT_CORE" > instead of > "DRM_UT_CORE" > so there is no chance of screwing up the order. > Or maybe the order doesn't even matter here? > > Could also stringify to avoid accidental of typos. > > > > > the fragility is the repetitive re-statement of the map, > > in those un-modified DECLARE_s, > > once replaced, the USEs just ref the struct built by the _DEFINE > > (once, and exported) > > > > I dont really like the _DEFINEs restatement of the enum-values: DRM_UT_* > > especially as "strings". > > I can automate the stringification with an APPLY_FN_(__stringify, ...) > > but the enum-list DRM_UT_* (w.o quotes) is still needed as args. > > > > unless there is something C can do thats like Enum.values() ? > > > > > > > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c index 699b7dbffd7b..4a5f2317229b 100644 --- a/drivers/gpu/drm/drm_print.c +++ b/drivers/gpu/drm/drm_print.c @@ -55,18 +55,19 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat #if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG) module_param_named(debug, __drm_debug, ulong, 0600); #else -/* classnames must match vals of enum drm_debug_category */ -DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0, - "DRM_UT_CORE", - "DRM_UT_DRIVER", - "DRM_UT_KMS", - "DRM_UT_PRIME", - "DRM_UT_ATOMIC", - "DRM_UT_VBL", - "DRM_UT_STATE", - "DRM_UT_LEASE", - "DRM_UT_DP", - "DRM_UT_DRMRES"); +/* classnames must match value-symbols of enum drm_debug_category */ +DRM_CLASSMAP_DEFINE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, + DRM_UT_CORE, + "DRM_UT_CORE", + "DRM_UT_DRIVER", + "DRM_UT_KMS", + "DRM_UT_PRIME", + "DRM_UT_ATOMIC", + "DRM_UT_VBL", + "DRM_UT_STATE", + "DRM_UT_LEASE", + "DRM_UT_DP", + "DRM_UT_DRMRES"); static struct ddebug_class_param drm_debug_bitmap = { .bits = &__drm_debug, diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h index 9cc473e5d353..905fc25bf65a 100644 --- a/include/drm/drm_print.h +++ b/include/drm/drm_print.h @@ -140,6 +140,14 @@ enum drm_debug_category { DRM_UT_DRMRES }; +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG +#define DRM_CLASSMAP_DEFINE(...) DYNDBG_CLASSMAP_DEFINE(__VA_ARGS__) +#define DRM_CLASSMAP_USE(name) DYNDBG_CLASSMAP_USE(name) +#else +#define DRM_CLASSMAP_DEFINE(...) +#define DRM_CLASSMAP_USE(name) +#endif + static inline bool drm_debug_enabled_raw(enum drm_debug_category category) { return unlikely(__drm_debug & BIT(category));
dyndbg's CLASSMAP-v1 api was broken; DECLARE_DYNDBG_CLASSMAP tried to do too much. Its replaced by DRM_CLASSMAP_DEFINE, which creates & EXPORTs the classmap when CONFIG_DRM_USE_DYNAMIC_DEBUG=y, for direct reference by drivers. The drivers still use DECLARE_DYNDBG_CLASSMAP for now, so they still redundantly re-declare the classmap, but we can convert the drivers later to DYNDBG_CLASSMAP_USE Signed-off-by: Jim Cromie <jim.cromie@gmail.com> --- drivers/gpu/drm/drm_print.c | 25 +++++++++++++------------ include/drm/drm_print.h | 8 ++++++++ 2 files changed, 21 insertions(+), 12 deletions(-)