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

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

[Luben Tuikov]

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.

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.
+ *
+ * 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).
+ */
+#ifndef pr_fmt
+#define pr_fmt(fmt) "[drm] " fmt
+#endif
+
 #include <linux/compiler.h>
 #include <linux/printk.h>
 #include <linux/seq_file.h>

base-commit: f3123c2590005c5ff631653d31428e40cd10c618
-- 
2.42.1

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

[Danilo Krummrich]

On 11/10/23 01:27, Luben Tuikov 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.
> 
> 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.

There are a couple occurrences of pr_fmt in drm code, but it seems like
all of those are correctly defined before any includes.

Acked-by: Danilo Krummrich <dakr@redhat.com>

> + *
> + * 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).
> + */
> +#ifndef pr_fmt
> +#define pr_fmt(fmt) "[drm] " fmt
> +#endif
> +
>   #include <linux/compiler.h>
>   #include <linux/printk.h>
>   #include <linux/seq_file.h>
> 
> base-commit: f3123c2590005c5ff631653d31428e40cd10c618

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

[Jani Nikula]

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.

BR,
Jani.

> + */
> +#ifndef pr_fmt
> +#define pr_fmt(fmt) "[drm] " fmt
> +#endif
> +
>  #include <linux/compiler.h>
>  #include <linux/printk.h>
>  #include <linux/seq_file.h>
>
> base-commit: f3123c2590005c5ff631653d31428e40cd10c618

-- 
Jani Nikula, Intel

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

[Jani Nikula]

On Fri, 10 Nov 2023, Jani Nikula <jani.nikula@linux.intel.com> 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.

Also, what's this "drm/sched" prefix here?

BR,
Jani.

-- 
Jani Nikula, Intel

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

[Luben Tuikov]

[-- Attachment #1.1.1: Type: text/plain, Size: 2218 bytes --]

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.

Agreed.

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

Agreed.

Do I have your R-B for a revert?
-- 
Regards,
Luben

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 677 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Luben Tuikov]

[-- Attachment #1.1.1: Type: text/plain, Size: 3270 bytes --]

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* "?
-- 
Regards,
Luben

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 677 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Jani Nikula]

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

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

[Luben Tuikov]

[-- Attachment #1.1.1: Type: text/plain, Size: 6159 bytes --]

On 2023-11-14 07:20, Jani Nikula wrote:
> 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.

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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 677 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Jani Nikula]

On Tue, 14 Nov 2023, Luben Tuikov <ltuikov89@gmail.com> wrote:
> 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__)

I think that would be an improvement that stands on its own merits.

Please also wrap the first drm in parens (drm).

> The output would be similar to that if drm->dev were NULL.

Yes. I don't know how people will feel about intentionally using
drm_err(NULL, ...) all over the place, but that's another matter. ;)

BR,
Jani.


-- 
Jani Nikula, Intel

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

[Luben Tuikov]

[-- Attachment #1.1.1: Type: text/plain, Size: 1058 bytes --]

On 2023-11-15 03:24, Jani Nikula wrote:
> On Tue, 14 Nov 2023, Luben Tuikov <ltuikov89@gmail.com> wrote:
>> 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__)
> 
> I think that would be an improvement that stands on its own merits.
> 
> Please also wrap the first drm in parens (drm).

Okay.

> 
>> The output would be similar to that if drm->dev were NULL.
> 
> Yes. I don't know how people will feel about intentionally using
> drm_err(NULL, ...) all over the place, but that's another matter. ;)

:-)

-- 
Regards,
Luben

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 677 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]