[PATCH 1/3] drm/print: Add generic drm dev printk function

[Michal Wajdeczko]

On 17.05.2024 15:33, Jani Nikula wrote:
> On Fri, 17 May 2024, Michal Wajdeczko <michal.wajdeczko at 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 at intel.com>
>> Cc: Jani Nikula <jani.nikula at 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,
>