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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 13 Nov 2023, Luben Tuikov <ltuikov89@gmail.com> wrote:
> Hi Jani,
>
> On 2023-11-10 07:40, Jani Nikula wrote:
>> On Thu, 09 Nov 2023, Luben Tuikov <ltuikov89@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@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* "?

I don't think there's a way to do that using pr_fmt for an entire driver
or subsystem. That really only works for individual compilation units.

We have DRM_ERROR() which does the trick, but the documentation says
it's been deprecated in favor pr_err()... though I think drm_err()
should be preferred over pr_err() where possible.

Maybe we should extend 7911902129a8 ("drm/print: Handle potentially NULL
drm_devices in drm_dbg_*") to __drm_printk() and handle NULL drm device
gracefully.

With just "(drm) ? (drm)->dev : NULL" the output will have "(NULL device
*)" which works but is a bit meh, but maybe something like this is
possible (untested):


diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index a93a387f8a1a..d16affece5b7 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -452,9 +452,13 @@ void __drm_dev_dbg(struct _ddebug *desc, const struct device *dev,
  */
 
 /* Helper for struct drm_device based logging. */
-#define __drm_printk(drm, level, type, fmt, ...)			\
-	dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
-
+#define __drm_printk(drm, level, type, fmt, ...) \
+	do {								\
+		if (drm)						\
+			dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__); \
+		else							\
+			pr_##level##type("[drm] " fmt, ##__VA_ARGS__);	\
+	} while (0)
 
 #define drm_info(drm, fmt, ...)					\
 	__drm_printk((drm), info,, fmt, ##__VA_ARGS__)


Then again that adds quite a bit of overhead all over the place unless
the compiler can be 100% it's one or the other.

BR,
Jani.


-- 
Jani Nikula, Intel