| Message ID | 20231110002659.113208-2-ltuikov89@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sched: Define pr_fmt() for DRM using pr_*() | expand |
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
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
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.
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?
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* "?
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.
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.
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.
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. ;) :-)
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>
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(+) base-commit: f3123c2590005c5ff631653d31428e40cd10c618