On 2023-11-14 07:20, Jani Nikula wrote:
> On Mon, 13 Nov 2023, Luben Tuikov <ltuikov89@xxxxxxxxx> wrote:
>> Hi Jani,
>>
>> On 2023-11-10 07:40, Jani Nikula wrote:
>>> On Thu, 09 Nov 2023, Luben Tuikov <ltuikov89@xxxxxxxxx> 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@xxxxxxxxx>
>>>> ---
>>>> 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
Attachment:
OpenPGP_0x4C15479431A334AF.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
