| Message ID | 20220912052852.1123868-2-jim.cromie@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dyndbg: drm.debug adaptation | expand |
On Sun, 11 Sep 2022, Jim Cromie <jim.cromie@gmail.com> wrote: > enum drm_debug_category has 10 categories, but is initialized with > bitmasks which require 10 bits of underlying storage. By using > natural enumeration, and moving the BIT(cat) into drm_debug_enabled(), > the enum fits in 4 bits, allowing the category to be represented > directly in pr_debug callsites, via the ddebug.class_id field. > > While this slightly pessimizes the bit-test in drm_debug_enabled(), > using dyndbg with JUMP_LABEL will avoid the function entirely. > > NOTE: this change forecloses the possibility of doing: > > drm_dbg(DRM_UT_CORE|DRM_UT_KMS, "weird 2-cat experiment") > > but thats already strongly implied by the use of the enum itself; its > not a normal enum if it can be 2 values simultaneously. The drm.debug module parameter values are, arguably, ABI. There are tons of people, scripts, test environments, documentation, bug reports, etc, etc, referring to specific drm.debug module parameter values to enable specific drm debug logging categories. AFAICT you're not changing any of the values here, but having an enum without the hard coded values makes it more likely to accidentally change the category to bit mapping. At the very least deserves a comment. BR, Jani. > > Signed-off-by: Jim Cromie <jim.cromie@gmail.com> > --- > include/drm/drm_print.h | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h > index 22fabdeed297..b3b470440e46 100644 > --- a/include/drm/drm_print.h > +++ b/include/drm/drm_print.h > @@ -279,49 +279,49 @@ enum drm_debug_category { > * @DRM_UT_CORE: Used in the generic drm code: drm_ioctl.c, drm_mm.c, > * drm_memory.c, ... > */ > - DRM_UT_CORE = 0x01, > + DRM_UT_CORE, > /** > * @DRM_UT_DRIVER: Used in the vendor specific part of the driver: i915, > * radeon, ... macro. > */ > - DRM_UT_DRIVER = 0x02, > + DRM_UT_DRIVER, > /** > * @DRM_UT_KMS: Used in the modesetting code. > */ > - DRM_UT_KMS = 0x04, > + DRM_UT_KMS, > /** > * @DRM_UT_PRIME: Used in the prime code. > */ > - DRM_UT_PRIME = 0x08, > + DRM_UT_PRIME, > /** > * @DRM_UT_ATOMIC: Used in the atomic code. > */ > - DRM_UT_ATOMIC = 0x10, > + DRM_UT_ATOMIC, > /** > * @DRM_UT_VBL: Used for verbose debug message in the vblank code. > */ > - DRM_UT_VBL = 0x20, > + DRM_UT_VBL, > /** > * @DRM_UT_STATE: Used for verbose atomic state debugging. > */ > - DRM_UT_STATE = 0x40, > + DRM_UT_STATE, > /** > * @DRM_UT_LEASE: Used in the lease code. > */ > - DRM_UT_LEASE = 0x80, > + DRM_UT_LEASE, > /** > * @DRM_UT_DP: Used in the DP code. > */ > - DRM_UT_DP = 0x100, > + DRM_UT_DP, > /** > * @DRM_UT_DRMRES: Used in the drm managed resources code. > */ > - DRM_UT_DRMRES = 0x200, > + DRM_UT_DRMRES > }; > > static inline bool drm_debug_enabled(enum drm_debug_category category) > { > - return unlikely(__drm_debug & category); > + return unlikely(__drm_debug & BIT(category)); > } > > /*
On Mon, Sep 12, 2022 at 4:17 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Sun, 11 Sep 2022, Jim Cromie <jim.cromie@gmail.com> wrote: > > enum drm_debug_category has 10 categories, but is initialized with > > bitmasks which require 10 bits of underlying storage. By using > > natural enumeration, and moving the BIT(cat) into drm_debug_enabled(), > > the enum fits in 4 bits, allowing the category to be represented > > directly in pr_debug callsites, via the ddebug.class_id field. > > > > While this slightly pessimizes the bit-test in drm_debug_enabled(), > > using dyndbg with JUMP_LABEL will avoid the function entirely. > > > > NOTE: this change forecloses the possibility of doing: > > > > drm_dbg(DRM_UT_CORE|DRM_UT_KMS, "weird 2-cat experiment") > > > > but thats already strongly implied by the use of the enum itself; its > > not a normal enum if it can be 2 values simultaneously. > > The drm.debug module parameter values are, arguably, ABI. There are tons > of people, scripts, test environments, documentation, bug reports, etc, > etc, referring to specific drm.debug module parameter values to enable > specific drm debug logging categories. > > AFAICT you're not changing any of the values here, but having an enum > without the hard coded values makes it more likely to accidentally > change the category to bit mapping. At the very least deserves a > comment. > hi Jani, You're correct, this is unchanged : echo $script_debug_val > /sys/module/drm/parameters/debug wrt the enum, the next patch adds a comment, enum drm_debug_category { + /* These names must match those in DYNAMIC_DEBUG_CLASSBITS */ /** * @DRM_UT_CORE: Used in the generic drm code: drm_ioctl.c, drm_mm.c, But that comment mostly misses the point youre making. and the specific NAME is stale. and the s/int/ulong/ __drm_debug should go here, with the use of BIT() I will fix this and repost. Is it useful for CI / patchwork / lkp-robot purposes, to branch-and-rebase onto drm-next/drm-next or drm-tip/drm-tip (or dated tags on them ) ? > > BR, > Jani. > > thank you > > > > Signed-off-by: Jim Cromie <jim.cromie@gmail.com> > > --- > > inclu
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h index 22fabdeed297..b3b470440e46 100644 --- a/include/drm/drm_print.h +++ b/include/drm/drm_print.h @@ -279,49 +279,49 @@ enum drm_debug_category { * @DRM_UT_CORE: Used in the generic drm code: drm_ioctl.c, drm_mm.c, * drm_memory.c, ... */ - DRM_UT_CORE = 0x01, + DRM_UT_CORE, /** * @DRM_UT_DRIVER: Used in the vendor specific part of the driver: i915, * radeon, ... macro. */ - DRM_UT_DRIVER = 0x02, + DRM_UT_DRIVER, /** * @DRM_UT_KMS: Used in the modesetting code. */ - DRM_UT_KMS = 0x04, + DRM_UT_KMS, /** * @DRM_UT_PRIME: Used in the prime code. */ - DRM_UT_PRIME = 0x08, + DRM_UT_PRIME, /** * @DRM_UT_ATOMIC: Used in the atomic code. */ - DRM_UT_ATOMIC = 0x10, + DRM_UT_ATOMIC, /** * @DRM_UT_VBL: Used for verbose debug message in the vblank code. */ - DRM_UT_VBL = 0x20, + DRM_UT_VBL, /** * @DRM_UT_STATE: Used for verbose atomic state debugging. */ - DRM_UT_STATE = 0x40, + DRM_UT_STATE, /** * @DRM_UT_LEASE: Used in the lease code. */ - DRM_UT_LEASE = 0x80, + DRM_UT_LEASE, /** * @DRM_UT_DP: Used in the DP code. */ - DRM_UT_DP = 0x100, + DRM_UT_DP, /** * @DRM_UT_DRMRES: Used in the drm managed resources code. */ - DRM_UT_DRMRES = 0x200, + DRM_UT_DRMRES }; static inline bool drm_debug_enabled(enum drm_debug_category category) { - return unlikely(__drm_debug & category); + return unlikely(__drm_debug & BIT(category)); } /*
enum drm_debug_category has 10 categories, but is initialized with bitmasks which require 10 bits of underlying storage. By using natural enumeration, and moving the BIT(cat) into drm_debug_enabled(), the enum fits in 4 bits, allowing the category to be represented directly in pr_debug callsites, via the ddebug.class_id field. While this slightly pessimizes the bit-test in drm_debug_enabled(), using dyndbg with JUMP_LABEL will avoid the function entirely. NOTE: this change forecloses the possibility of doing: drm_dbg(DRM_UT_CORE|DRM_UT_KMS, "weird 2-cat experiment") but thats already strongly implied by the use of the enum itself; its not a normal enum if it can be 2 values simultaneously. Signed-off-by: Jim Cromie <jim.cromie@gmail.com> --- include/drm/drm_print.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)