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

Luben Tuikov ltuikov89 at gmail.com
Tue Nov 14 23:57:21 UTC 2023 

On 2023-11-14 07:20, Jani Nikula wrote:
> On Mon, 13 Nov 2023, Luben Tuikov <ltuikov89 at gmail.com> wrote:
>> 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* "?
> 
> 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.

Yes, that's what made me use pr_err() and the pr_fmt() modification...

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

Yeah, that actually would work.

> 
> 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):

So, I don't mind the ternary op, really. So long as we get the "*ERROR* ",
on drm_err(), because it really helps us debug when we get a blank screen. :-)

> 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.

True.

How about extending the commit mentioned above to something like this,

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index a93a387f8a1a15..ce784118e4f762 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -453,7 +453,7 @@ 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__)
+       dev_##level##type(drm ? (drm)->dev : NULL, "[drm] " fmt, ##__VA_ARGS__)

The output would be similar to that if drm->dev were NULL.
-- 
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/20231114/3b396590/attachment-0001.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/20231114/3b396590/attachment-0001.sig>

More information about the dri-devel mailing list