| Message ID | 20240517125730.2304-2-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve drm printer code | expand |
On Fri, 17 May 2024, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote: > We already have some drm printk functions that need to duplicate > a code to get a similar format of the final result, for example: > > [ ] 0000:00:00.0: [drm:foo] bar > [ ] 0000:00:00.0: [drm] foo bar > [ ] 0000:00:00.0: [drm] *ERROR* foo > > Add a generic __drm_dev_vprintk() function that can format the > final message like all other existing function do and allows us > to keep the formatting code in one place. Nice idea! > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/drm_print.c | 49 ++++++++++++++++++++----------------- > include/drm/drm_print.h | 3 +++ > 2 files changed, 30 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > index cf2efb44722c..a2b60c8245a1 100644 > --- a/drivers/gpu/drm/drm_print.c > +++ b/drivers/gpu/drm/drm_print.c > @@ -187,19 +187,12 @@ void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf) > const struct drm_device *drm = p->arg; > const struct device *dev = drm ? drm->dev : NULL; > enum drm_debug_category category = p->category; > - const char *prefix = p->prefix ?: ""; > - const char *prefix_pad = p->prefix ? " " : ""; > > if (!__drm_debug_enabled(category)) > return; > > /* Note: __builtin_return_address(0) is useless here. */ > - if (dev) > - dev_printk(KERN_DEBUG, dev, "[" DRM_NAME "]%s%s %pV", > - prefix_pad, prefix, vaf); > - else > - printk(KERN_DEBUG "[" DRM_NAME "]%s%s %pV", > - prefix_pad, prefix, vaf); > + __drm_dev_vprintk(dev, KERN_DEBUG, NULL, p->prefix, vaf); > } > EXPORT_SYMBOL(__drm_printfn_dbg); > > @@ -277,6 +270,29 @@ void drm_print_bits(struct drm_printer *p, unsigned long value, > } > EXPORT_SYMBOL(drm_print_bits); > > +void __drm_dev_vprintk(const struct device *dev, const char *level, > + const void *origin, const char *prefix, > + struct va_format *vaf) > +{ > + const char *prefix_pad = prefix ? " " : (prefix = ""); Too clever, please just keep it simple: const char *prefix_pad = prefix ? " " : ""; if (!prefix) prefix = ""; > + > + if (dev) > + if (origin) > + dev_printk(level, dev, "[" DRM_NAME ":%ps]%s%s %pV", > + origin, prefix_pad, prefix, vaf); > + else > + dev_printk(level, dev, "[" DRM_NAME "]%s%s %pV", > + prefix_pad, prefix, vaf); > + else > + if (origin) > + printk("%s" "[" DRM_NAME ":%ps]%s%s %pV", > + level, origin, prefix_pad, prefix, vaf); > + else > + printk("%s" "[" DRM_NAME "]%s%s %pV", > + level, prefix_pad, prefix, vaf); I'd sprinkle a few curly braces around the top level if-else blocks. Side note, feels like using DRM_NAME makes things harder, not easier. But that's for another patch. > +} > +EXPORT_SYMBOL(__drm_dev_vprintk); AFAICT this could be a non-exported static function. And probably moved earlier in the file to not require a declaration. BR, Jani. > + > void drm_dev_printk(const struct device *dev, const char *level, > const char *format, ...) > { > @@ -287,12 +303,7 @@ void drm_dev_printk(const struct device *dev, const char *level, > vaf.fmt = format; > vaf.va = &args; > > - if (dev) > - dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV", > - __builtin_return_address(0), &vaf); > - else > - printk("%s" "[" DRM_NAME ":%ps] %pV", > - level, __builtin_return_address(0), &vaf); > + __drm_dev_vprintk(dev, level, __builtin_return_address(0), NULL, &vaf); > > va_end(args); > } > @@ -312,12 +323,7 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev, > vaf.fmt = format; > vaf.va = &args; > > - if (dev) > - dev_printk(KERN_DEBUG, dev, "[" DRM_NAME ":%ps] %pV", > - __builtin_return_address(0), &vaf); > - else > - printk(KERN_DEBUG "[" DRM_NAME ":%ps] %pV", > - __builtin_return_address(0), &vaf); > + __drm_dev_vprintk(dev, KERN_DEBUG, __builtin_return_address(0), NULL, &vaf); > > va_end(args); > } > @@ -351,8 +357,7 @@ void __drm_err(const char *format, ...) > vaf.fmt = format; > vaf.va = &args; > > - printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV", > - __builtin_return_address(0), &vaf); > + __drm_dev_vprintk(NULL, KERN_ERR, __builtin_return_address(0), "*ERROR*", &vaf); > > va_end(args); > } > diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h > index 089950ad8681..bb1801c58544 100644 > --- a/include/drm/drm_print.h > +++ b/include/drm/drm_print.h > @@ -366,6 +366,9 @@ static inline struct drm_printer drm_err_printer(struct drm_device *drm, > __printf(3, 4) > void drm_dev_printk(const struct device *dev, const char *level, > const char *format, ...); > +void __drm_dev_vprintk(const struct device *dev, const char *level, > + const void *origin, const char *prefix, > + struct va_format *vaf); > struct _ddebug; > __printf(4, 5) > void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
On 17.05.2024 15:33, Jani Nikula wrote: > On Fri, 17 May 2024, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote: >> We already have some drm printk functions that need to duplicate >> a code to get a similar format of the final result, for example: >> >> [ ] 0000:00:00.0: [drm:foo] bar >> [ ] 0000:00:00.0: [drm] foo bar >> [ ] 0000:00:00.0: [drm] *ERROR* foo >> >> Add a generic __drm_dev_vprintk() function that can format the >> final message like all other existing function do and allows us >> to keep the formatting code in one place. > > Nice idea! > >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/drm_print.c | 49 ++++++++++++++++++++----------------- >> include/drm/drm_print.h | 3 +++ >> 2 files changed, 30 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c >> index cf2efb44722c..a2b60c8245a1 100644 >> --- a/drivers/gpu/drm/drm_print.c >> +++ b/drivers/gpu/drm/drm_print.c >> @@ -187,19 +187,12 @@ void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf) >> const struct drm_device *drm = p->arg; >> const struct device *dev = drm ? drm->dev : NULL; >> enum drm_debug_category category = p->category; >> - const char *prefix = p->prefix ?: ""; >> - const char *prefix_pad = p->prefix ? " " : ""; >> >> if (!__drm_debug_enabled(category)) >> return; >> >> /* Note: __builtin_return_address(0) is useless here. */ >> - if (dev) >> - dev_printk(KERN_DEBUG, dev, "[" DRM_NAME "]%s%s %pV", >> - prefix_pad, prefix, vaf); >> - else >> - printk(KERN_DEBUG "[" DRM_NAME "]%s%s %pV", >> - prefix_pad, prefix, vaf); >> + __drm_dev_vprintk(dev, KERN_DEBUG, NULL, p->prefix, vaf); >> } >> EXPORT_SYMBOL(__drm_printfn_dbg); >> >> @@ -277,6 +270,29 @@ void drm_print_bits(struct drm_printer *p, unsigned long value, >> } >> EXPORT_SYMBOL(drm_print_bits); >> >> +void __drm_dev_vprintk(const struct device *dev, const char *level, >> + const void *origin, const char *prefix, >> + struct va_format *vaf) >> +{ >> + const char *prefix_pad = prefix ? " " : (prefix = ""); > > Too clever, please just keep it simple: > > const char *prefix_pad = prefix ? " " : ""; > > if (!prefix) > prefix = ""; > >> + >> + if (dev) >> + if (origin) >> + dev_printk(level, dev, "[" DRM_NAME ":%ps]%s%s %pV", >> + origin, prefix_pad, prefix, vaf); >> + else >> + dev_printk(level, dev, "[" DRM_NAME "]%s%s %pV", >> + prefix_pad, prefix, vaf); >> + else >> + if (origin) >> + printk("%s" "[" DRM_NAME ":%ps]%s%s %pV", >> + level, origin, prefix_pad, prefix, vaf); >> + else >> + printk("%s" "[" DRM_NAME "]%s%s %pV", >> + level, prefix_pad, prefix, vaf); > > I'd sprinkle a few curly braces around the top level if-else blocks. > > Side note, feels like using DRM_NAME makes things harder, not > easier. But that's for another patch. > >> +} >> +EXPORT_SYMBOL(__drm_dev_vprintk); > > AFAICT this could be a non-exported static function. And probably moved > earlier in the file to not require a declaration. true for now, but I was planning to add Xe GT specific printer that would use this function like this: +static inline void __xe_gt_printfn_dbg(struct drm_printer *p, struct va_format *vaf) +{ + struct xe_gt *gt = p->arg; + const struct device *dev = gt_to_xe(gt)->drm.dev; + char prefix[8]; + + if (!drm_debug_enabled(DRM_UT_DRIVER)) + return; + + /* xe_gt_dbg() callsite decorations are unhelpful */ + snprintf(prefix, sizeof(prefix), "GT%u:", gt->info.id); + __drm_dev_vprintk(dev, KERN_DEBUG, p->origin, prefix, vaf); +} + but I can add this new custom printer to the series right now in v2 to show the usage and avoid any confusion > > BR, > Jani. > >> + >> void drm_dev_printk(const struct device *dev, const char *level, >> const char *format, ...) >> { >> @@ -287,12 +303,7 @@ void drm_dev_printk(const struct device *dev, const char *level, >> vaf.fmt = format; >> vaf.va = &args; >> >> - if (dev) >> - dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV", >> - __builtin_return_address(0), &vaf); >> - else >> - printk("%s" "[" DRM_NAME ":%ps] %pV", >> - level, __builtin_return_address(0), &vaf); >> + __drm_dev_vprintk(dev, level, __builtin_return_address(0), NULL, &vaf); >> >> va_end(args); >> } >> @@ -312,12 +323,7 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev, >> vaf.fmt = format; >> vaf.va = &args; >> >> - if (dev) >> - dev_printk(KERN_DEBUG, dev, "[" DRM_NAME ":%ps] %pV", >> - __builtin_return_address(0), &vaf); >> - else >> - printk(KERN_DEBUG "[" DRM_NAME ":%ps] %pV", >> - __builtin_return_address(0), &vaf); >> + __drm_dev_vprintk(dev, KERN_DEBUG, __builtin_return_address(0), NULL, &vaf); >> >> va_end(args); >> } >> @@ -351,8 +357,7 @@ void __drm_err(const char *format, ...) >> vaf.fmt = format; >> vaf.va = &args; >> >> - printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV", >> - __builtin_return_address(0), &vaf); >> + __drm_dev_vprintk(NULL, KERN_ERR, __builtin_return_address(0), "*ERROR*", &vaf); >> >> va_end(args); >> } >> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h >> index 089950ad8681..bb1801c58544 100644 >> --- a/include/drm/drm_print.h >> +++ b/include/drm/drm_print.h >> @@ -366,6 +366,9 @@ static inline struct drm_printer drm_err_printer(struct drm_device *drm, >> __printf(3, 4) >> void drm_dev_printk(const struct device *dev, const char *level, >> const char *format, ...); >> +void __drm_dev_vprintk(const struct device *dev, const char *level, >> + const void *origin, const char *prefix, >> + struct va_format *vaf); >> struct _ddebug; >> __printf(4, 5) >> void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev, >
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c index cf2efb44722c..a2b60c8245a1 100644 --- a/drivers/gpu/drm/drm_print.c +++ b/drivers/gpu/drm/drm_print.c @@ -187,19 +187,12 @@ void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf) const struct drm_device *drm = p->arg; const struct device *dev = drm ? drm->dev : NULL; enum drm_debug_category category = p->category; - const char *prefix = p->prefix ?: ""; - const char *prefix_pad = p->prefix ? " " : ""; if (!__drm_debug_enabled(category)) return; /* Note: __builtin_return_address(0) is useless here. */ - if (dev) - dev_printk(KERN_DEBUG, dev, "[" DRM_NAME "]%s%s %pV", - prefix_pad, prefix, vaf); - else - printk(KERN_DEBUG "[" DRM_NAME "]%s%s %pV", - prefix_pad, prefix, vaf); + __drm_dev_vprintk(dev, KERN_DEBUG, NULL, p->prefix, vaf); } EXPORT_SYMBOL(__drm_printfn_dbg); @@ -277,6 +270,29 @@ void drm_print_bits(struct drm_printer *p, unsigned long value, } EXPORT_SYMBOL(drm_print_bits); +void __drm_dev_vprintk(const struct device *dev, const char *level, + const void *origin, const char *prefix, + struct va_format *vaf) +{ + const char *prefix_pad = prefix ? " " : (prefix = ""); + + if (dev) + if (origin) + dev_printk(level, dev, "[" DRM_NAME ":%ps]%s%s %pV", + origin, prefix_pad, prefix, vaf); + else + dev_printk(level, dev, "[" DRM_NAME "]%s%s %pV", + prefix_pad, prefix, vaf); + else + if (origin) + printk("%s" "[" DRM_NAME ":%ps]%s%s %pV", + level, origin, prefix_pad, prefix, vaf); + else + printk("%s" "[" DRM_NAME "]%s%s %pV", + level, prefix_pad, prefix, vaf); +} +EXPORT_SYMBOL(__drm_dev_vprintk); + void drm_dev_printk(const struct device *dev, const char *level, const char *format, ...) { @@ -287,12 +303,7 @@ void drm_dev_printk(const struct device *dev, const char *level, vaf.fmt = format; vaf.va = &args; - if (dev) - dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV", - __builtin_return_address(0), &vaf); - else - printk("%s" "[" DRM_NAME ":%ps] %pV", - level, __builtin_return_address(0), &vaf); + __drm_dev_vprintk(dev, level, __builtin_return_address(0), NULL, &vaf); va_end(args); } @@ -312,12 +323,7 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev, vaf.fmt = format; vaf.va = &args; - if (dev) - dev_printk(KERN_DEBUG, dev, "[" DRM_NAME ":%ps] %pV", - __builtin_return_address(0), &vaf); - else - printk(KERN_DEBUG "[" DRM_NAME ":%ps] %pV", - __builtin_return_address(0), &vaf); + __drm_dev_vprintk(dev, KERN_DEBUG, __builtin_return_address(0), NULL, &vaf); va_end(args); } @@ -351,8 +357,7 @@ void __drm_err(const char *format, ...) vaf.fmt = format; vaf.va = &args; - printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV", - __builtin_return_address(0), &vaf); + __drm_dev_vprintk(NULL, KERN_ERR, __builtin_return_address(0), "*ERROR*", &vaf); va_end(args); } diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h index 089950ad8681..bb1801c58544 100644 --- a/include/drm/drm_print.h +++ b/include/drm/drm_print.h @@ -366,6 +366,9 @@ static inline struct drm_printer drm_err_printer(struct drm_device *drm, __printf(3, 4) void drm_dev_printk(const struct device *dev, const char *level, const char *format, ...); +void __drm_dev_vprintk(const struct device *dev, const char *level, + const void *origin, const char *prefix, + struct va_format *vaf); struct _ddebug; __printf(4, 5) void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
We already have some drm printk functions that need to duplicate a code to get a similar format of the final result, for example: [ ] 0000:00:00.0: [drm:foo] bar [ ] 0000:00:00.0: [drm] foo bar [ ] 0000:00:00.0: [drm] *ERROR* foo Add a generic __drm_dev_vprintk() function that can format the final message like all other existing function do and allows us to keep the formatting code in one place. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/drm_print.c | 49 ++++++++++++++++++++----------------- include/drm/drm_print.h | 3 +++ 2 files changed, 30 insertions(+), 22 deletions(-)