[PATCH] drm/sched: Define pr_fmt() for DRM using pr_*()

[Luben Tuikov]

Hi Jani,

On 2023-11-10 07:40, Jani Nikula wrote:
> On Thu, 09 Nov 2023, Luben Tuikov <ltuikov89 at gmail.com> wrote:
>> Define pr_fmt() as "[drm] " for DRM code using pr_*() facilities, especially
>> when no devices are available. This makes it easier to browse kernel logs.
> 
> Please do not merge patches before people have actually had a chance to
> look at them. This was merged *way* too quickly.
> 
> This does not do what you think it does, and it's not robust enough.
> 
> The drm_print.[ch] facilities use very few pr_*() calls directly. The
> users of pr_*() calls do not necessarily include <drm/drm_print.h> at
> all, and really don't have to.
> 
> Even the ones that do include it, usually have <linux/...> includes
> first, and <drm/...> includes next. Notably, <linux/kernel.h> includes
> <linux/printk.h>.
> 
> And, of course, <linux/printk.h> defines pr_fmt() itself if not already
> defined.
> 
>> Signed-off-by: Luben Tuikov <ltuikov89 at gmail.com>
>> ---
>>  include/drm/drm_print.h | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
>> index a93a387f8a1a15..e8fe60d0eb8783 100644
>> --- a/include/drm/drm_print.h
>> +++ b/include/drm/drm_print.h
>> @@ -26,6 +26,20 @@
>>  #ifndef DRM_PRINT_H_
>>  #define DRM_PRINT_H_
>>  
>> +/* Define this before including linux/printk.h, so that the format
>> + * string in pr_*() macros is correctly set for DRM. If a file wants
>> + * to define this to something else, it should do so before including
>> + * this header file.
> 
> The only way this would work is by including <drm/drm_print.h> as the
> very first header, and that's fragile at best.
> 
>> + *
>> + * It is encouraged code using pr_err() to prefix their format with
>> + * the string "*ERROR* ", to make it easier to scan kernel logs. For
>> + * instance,
>> + *   pr_err("*ERROR* <the rest of your format string here>", args).
> 
> No, it's encouraged not to use pr_*() at all, and prefer drm device
> based logging, or device based logging.
> 
> I'd rather this whole thing was just reverted.

The revert has been pushed--thanks for R-B-ing it.

FWIW, I wanted a device-less DRM print, with a prefix "[drm] *ERROR* ",
because this is what we scan for, especially when we get a blank screen at boot/modprobe.
There's a few cases in DRM where when we return -E... it's most likely a blank screen result,
as was the case with a recent debug I had with amdgpu when pushing the variable sched->rq.

So then I went by this, in linux/printk.h:

/**
 * pr_fmt - used by the pr_*() macros to generate the printk format string
 * @fmt: format string passed from a pr_*() macro
 *
 * This macro can be used to generate a unified format string for pr_*()
 * macros. A common use is to prefix all pr_*() messages in a file with a common
 * string. For example, defining this at the top of a source file:
 *
 *        #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 *
 * would prefix all pr_info, pr_emerg... messages in the file with the module
 * name.
 */
#ifndef pr_fmt
#define pr_fmt(fmt) fmt
#endif

Any suggestions as to a device-less DRM print with prefix "[drm] *ERROR* "?
-- 
Regards,
Luben
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0x4C15479431A334AF.asc
Type: application/pgp-keys
Size: 664 bytes
Desc: OpenPGP public key
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231113/186b6150/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 236 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231113/186b6150/attachment.sig>