Re: [PATCH 1/3] drm/print: Add generic drm dev printk function
On 17.05.2024 15:33, Jani Nikula wrote:
> On Fri, 17 May 2024, Michal Wajdeczko 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:
>>
>> [ ] :00:00.0: [drm:foo] bar
>> [ ] :00:00.0: [drm] foo bar
>> [ ] :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
>> Cc: Jani Nikula
>> ---
>> 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 =
>>
>> -if (dev)
>> -dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
>> - __builtin_return_address(0), );
>> -else
>> -printk("%s" "[" DRM_NAME ":%ps] %pV",
>> - level, __builtin_return_address(0), );
>> +__drm_dev_vprintk(dev, level, __builtin_return_address(0), NULL, );
>>
>> va_end(args);
>> }
>> @@ -312,12 +323,7 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct
>> device *dev,
>> vaf.fmt = format;
>> vaf.va =
>>
>> -if (dev)
>> -dev_printk(KERN_DEBUG, dev,
Re: [PATCH 1/3] drm/print: Add generic drm dev printk function
On Fri, 17 May 2024, Michal Wajdeczko 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:
>
> [ ] :00:00.0: [drm:foo] bar
> [ ] :00:00.0: [drm] foo bar
> [ ] :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
> Cc: Jani Nikula
> ---
> 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 =
>
> - if (dev)
> - dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
> -__builtin_return_address(0), );
> - else
> - printk("%s" "[" DRM_NAME ":%ps] %pV",
> -level, __builtin_return_address(0), );
> + __drm_dev_vprintk(dev, level, __builtin_return_address(0), NULL, );
>
> va_end(args);
> }
> @@ -312,12 +323,7 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct
> device *dev,
> vaf.fmt = format;
> vaf.va =
>
> - if (dev)
> - dev_printk(KERN_DEBUG, dev, "[" DRM_NAME ":%ps] %pV",
> -__builtin_return_address(0), );
> - else
> - printk(KERN_DEBUG "[" DRM_NAME ":%ps] %pV",
> -__builtin_return_address(0), );
> + __drm_dev_vprintk(dev, KERN_DEBUG, __builtin_return_address(0), NULL,
> );
>
> va_end(args);
> }
> @@ -351,8 +357,7 @@ void __drm_err(const char *format, ...)
> vaf.fmt = format;
> vaf.va =
>
> - printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV",
> -__builtin_return_address(0), );
> + __drm_dev_vprintk(NULL, KERN_ERR, __builtin_return_address(0),
> "*ERROR*", );
>
> 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
[PATCH 1/3] drm/print: Add generic drm dev printk function
We already have some drm printk functions that need to duplicate
a code to get a similar format of the final result, for example:
[ ] :00:00.0: [drm:foo] bar
[ ] :00:00.0: [drm] foo bar
[ ] :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
Cc: Jani Nikula
---
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 = "");
+
+ 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 =
- if (dev)
- dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
- __builtin_return_address(0), );
- else
- printk("%s" "[" DRM_NAME ":%ps] %pV",
- level, __builtin_return_address(0), );
+ __drm_dev_vprintk(dev, level, __builtin_return_address(0), NULL, );
va_end(args);
}
@@ -312,12 +323,7 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct
device *dev,
vaf.fmt = format;
vaf.va =
- if (dev)
- dev_printk(KERN_DEBUG, dev, "[" DRM_NAME ":%ps] %pV",
- __builtin_return_address(0), );
- else
- printk(KERN_DEBUG "[" DRM_NAME ":%ps] %pV",
- __builtin_return_address(0), );
+ __drm_dev_vprintk(dev, KERN_DEBUG, __builtin_return_address(0), NULL,
);
va_end(args);
}
@@ -351,8 +357,7 @@ void __drm_err(const char *format, ...)
vaf.fmt = format;
vaf.va =
- printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV",
- __builtin_return_address(0), );
+ __drm_dev_vprintk(NULL, KERN_ERR, __builtin_return_address(0),
"*ERROR*", );
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,
--
2.43.0
