| Message ID | be9baeab281f75999e96cc7ad1c06c6680494bc1.1722951405.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: remove __i915_printk() | expand |
-----Original Message----- From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Jani Nikula Sent: Tuesday, August 6, 2024 6:39 AM To: intel-gfx@lists.freedesktop.org Cc: Nikula, Jani <jani.nikula@intel.com> Subject: [PATCH 3/3] drm/i915: remove __i915_printk() > > With the previous cleanups, the last remaining user of __i915_printk() > is i915_probe_error(). Switch that to use drm_dbg() and drm_err() > instead, dropping the request to report bugs in the few remaining > specific cases. LGTM. Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com> -Jonathan Cavitt > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/i915_utils.c | 41 ------------------------------- > drivers/gpu/drm/i915/i915_utils.h | 13 +++++----- > 2 files changed, 6 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_utils.c b/drivers/gpu/drm/i915/i915_utils.c > index bee32222f0fd..b34a2d3d331d 100644 > --- a/drivers/gpu/drm/i915/i915_utils.c > +++ b/drivers/gpu/drm/i915/i915_utils.c > @@ -11,47 +11,6 @@ > #include "i915_reg.h" > #include "i915_utils.h" > > -#define FDO_BUG_MSG "Please file a bug on drm/i915; see " FDO_BUG_URL " for details." > - > -void > -__i915_printk(struct drm_i915_private *dev_priv, const char *level, > - const char *fmt, ...) > -{ > - static bool shown_bug_once; > - struct device *kdev = dev_priv->drm.dev; > - bool is_error = level[1] <= KERN_ERR[1]; > - bool is_debug = level[1] == KERN_DEBUG[1]; > - struct va_format vaf; > - va_list args; > - > - if (is_debug && !drm_debug_enabled(DRM_UT_DRIVER)) > - return; > - > - va_start(args, fmt); > - > - vaf.fmt = fmt; > - vaf.va = &args; > - > - if (is_error) > - dev_printk(level, kdev, "%pV", &vaf); > - else > - dev_printk(level, kdev, "[" DRM_NAME ":%ps] %pV", > - __builtin_return_address(0), &vaf); > - > - va_end(args); > - > - if (is_error && !shown_bug_once) { > - /* > - * Ask the user to file a bug report for the error, except > - * if they may have caused the bug by fiddling with unsafe > - * module parameters. > - */ > - if (!test_taint(TAINT_USER)) > - dev_notice(kdev, "%s", FDO_BUG_MSG); > - shown_bug_once = true; > - } > -} > - > void add_taint_for_CI(struct drm_i915_private *i915, unsigned int taint) > { > drm_notice(&i915->drm, "CI tainted: %#x by %pS\n", > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h > index feb078ae246f..71bdc89bd621 100644 > --- a/drivers/gpu/drm/i915/i915_utils.h > +++ b/drivers/gpu/drm/i915/i915_utils.h > @@ -45,10 +45,6 @@ struct timer_list; > #define MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \ > __stringify(x), (long)(x)) > > -void __printf(3, 4) > -__i915_printk(struct drm_i915_private *dev_priv, const char *level, > - const char *fmt, ...); > - > #if IS_ENABLED(CONFIG_DRM_I915_DEBUG) > > int __i915_inject_probe_error(struct drm_i915_private *i915, int err, > @@ -66,9 +62,12 @@ bool i915_error_injected(void); > > #define i915_inject_probe_failure(i915) i915_inject_probe_error((i915), -ENODEV) > > -#define i915_probe_error(i915, fmt, ...) \ > - __i915_printk(i915, i915_error_injected() ? KERN_DEBUG : KERN_ERR, \ > - fmt, ##__VA_ARGS__) > +#define i915_probe_error(i915, fmt, ...) ({ \ > + if (i915_error_injected()) \ > + drm_dbg(&(i915)->drm, fmt, ##__VA_ARGS__); \ > + else \ > + drm_err(&(i915)->drm, fmt, ##__VA_ARGS__); \ > +}) > > #define range_overflows(start, size, max) ({ \ > typeof(start) start__ = (start); \ > -- > 2.39.2 > >
On 06/08/2024 14:38, Jani Nikula wrote: > With the previous cleanups, the last remaining user of __i915_printk() > is i915_probe_error(). Switch that to use drm_dbg() and drm_err() > instead, dropping the request to report bugs in the few remaining > specific cases. Aren't those few cases legitimate probe failures, including anything unexpected which results in non-operational GPU (any -EIO from intel_gt_init())? So it is effectively completely(*) removing the request to file bugs, or I miss something remained? Or the unmentioned goal is to encourage fewer i915 bug reports on top of the code base cleanup? Regards, Tvrtko *) Apart from display/intel_dp_aux_backlight.c !? :) > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/i915_utils.c | 41 ------------------------------- > drivers/gpu/drm/i915/i915_utils.h | 13 +++++----- > 2 files changed, 6 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_utils.c b/drivers/gpu/drm/i915/i915_utils.c > index bee32222f0fd..b34a2d3d331d 100644 > --- a/drivers/gpu/drm/i915/i915_utils.c > +++ b/drivers/gpu/drm/i915/i915_utils.c > @@ -11,47 +11,6 @@ > #include "i915_reg.h" > #include "i915_utils.h" > > -#define FDO_BUG_MSG "Please file a bug on drm/i915; see " FDO_BUG_URL " for details." > - > -void > -__i915_printk(struct drm_i915_private *dev_priv, const char *level, > - const char *fmt, ...) > -{ > - static bool shown_bug_once; > - struct device *kdev = dev_priv->drm.dev; > - bool is_error = level[1] <= KERN_ERR[1]; > - bool is_debug = level[1] == KERN_DEBUG[1]; > - struct va_format vaf; > - va_list args; > - > - if (is_debug && !drm_debug_enabled(DRM_UT_DRIVER)) > - return; > - > - va_start(args, fmt); > - > - vaf.fmt = fmt; > - vaf.va = &args; > - > - if (is_error) > - dev_printk(level, kdev, "%pV", &vaf); > - else > - dev_printk(level, kdev, "[" DRM_NAME ":%ps] %pV", > - __builtin_return_address(0), &vaf); > - > - va_end(args); > - > - if (is_error && !shown_bug_once) { > - /* > - * Ask the user to file a bug report for the error, except > - * if they may have caused the bug by fiddling with unsafe > - * module parameters. > - */ > - if (!test_taint(TAINT_USER)) > - dev_notice(kdev, "%s", FDO_BUG_MSG); > - shown_bug_once = true; > - } > -} > - > void add_taint_for_CI(struct drm_i915_private *i915, unsigned int taint) > { > drm_notice(&i915->drm, "CI tainted: %#x by %pS\n", > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h > index feb078ae246f..71bdc89bd621 100644 > --- a/drivers/gpu/drm/i915/i915_utils.h > +++ b/drivers/gpu/drm/i915/i915_utils.h > @@ -45,10 +45,6 @@ struct timer_list; > #define MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \ > __stringify(x), (long)(x)) > > -void __printf(3, 4) > -__i915_printk(struct drm_i915_private *dev_priv, const char *level, > - const char *fmt, ...); > - > #if IS_ENABLED(CONFIG_DRM_I915_DEBUG) > > int __i915_inject_probe_error(struct drm_i915_private *i915, int err, > @@ -66,9 +62,12 @@ bool i915_error_injected(void); > > #define i915_inject_probe_failure(i915) i915_inject_probe_error((i915), -ENODEV) > > -#define i915_probe_error(i915, fmt, ...) \ > - __i915_printk(i915, i915_error_injected() ? KERN_DEBUG : KERN_ERR, \ > - fmt, ##__VA_ARGS__) > +#define i915_probe_error(i915, fmt, ...) ({ \ > + if (i915_error_injected()) \ > + drm_dbg(&(i915)->drm, fmt, ##__VA_ARGS__); \ > + else \ > + drm_err(&(i915)->drm, fmt, ##__VA_ARGS__); \ > +}) > > #define range_overflows(start, size, max) ({ \ > typeof(start) start__ = (start); \
On Wed, 07 Aug 2024, Tvrtko Ursulin <tursulin@ursulin.net> wrote: > On 06/08/2024 14:38, Jani Nikula wrote: >> With the previous cleanups, the last remaining user of __i915_printk() >> is i915_probe_error(). Switch that to use drm_dbg() and drm_err() >> instead, dropping the request to report bugs in the few remaining >> specific cases. > > Aren't those few cases legitimate probe failures, including anything > unexpected which results in non-operational GPU (any -EIO from > intel_gt_init())? They are, and they're still logged as such. Functionally, the only change is removing the bug filing request. > So it is effectively completely(*) removing the request to file bugs, or > I miss something remained? Or the unmentioned goal is to encourage fewer > i915 bug reports on top of the code base cleanup? I should've elaborated this better. My question is, what makes these cases so special that they warrant logging a bug filing request? First, I would assume the init paths are most tested in CI and least likely to trigger a failure on end user machines. Second, even if they did trigger for the end user, a non-operational GPU is most likely to lead to a bug report even without a request. To me it just seems weird, and I opted to remove them, not least because it's not common for drivers to do this at all. (And yes, I'd remove the backlight one too.) The other option is to embrace logging bug reporting requests. But for that I'd rather add a separate function, call it at the relevant places, and not hide it within this complex maze of multi-level debug logging macros. BR, Jani. > > Regards, > > Tvrtko > > *) Apart from display/intel_dp_aux_backlight.c !? :) > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/i915_utils.c | 41 ------------------------------- >> drivers/gpu/drm/i915/i915_utils.h | 13 +++++----- >> 2 files changed, 6 insertions(+), 48 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_utils.c b/drivers/gpu/drm/i915/i915_utils.c >> index bee32222f0fd..b34a2d3d331d 100644 >> --- a/drivers/gpu/drm/i915/i915_utils.c >> +++ b/drivers/gpu/drm/i915/i915_utils.c >> @@ -11,47 +11,6 @@ >> #include "i915_reg.h" >> #include "i915_utils.h" >> >> -#define FDO_BUG_MSG "Please file a bug on drm/i915; see " FDO_BUG_URL " for details." >> - >> -void >> -__i915_printk(struct drm_i915_private *dev_priv, const char *level, >> - const char *fmt, ...) >> -{ >> - static bool shown_bug_once; >> - struct device *kdev = dev_priv->drm.dev; >> - bool is_error = level[1] <= KERN_ERR[1]; >> - bool is_debug = level[1] == KERN_DEBUG[1]; >> - struct va_format vaf; >> - va_list args; >> - >> - if (is_debug && !drm_debug_enabled(DRM_UT_DRIVER)) >> - return; >> - >> - va_start(args, fmt); >> - >> - vaf.fmt = fmt; >> - vaf.va = &args; >> - >> - if (is_error) >> - dev_printk(level, kdev, "%pV", &vaf); >> - else >> - dev_printk(level, kdev, "[" DRM_NAME ":%ps] %pV", >> - __builtin_return_address(0), &vaf); >> - >> - va_end(args); >> - >> - if (is_error && !shown_bug_once) { >> - /* >> - * Ask the user to file a bug report for the error, except >> - * if they may have caused the bug by fiddling with unsafe >> - * module parameters. >> - */ >> - if (!test_taint(TAINT_USER)) >> - dev_notice(kdev, "%s", FDO_BUG_MSG); >> - shown_bug_once = true; >> - } >> -} >> - >> void add_taint_for_CI(struct drm_i915_private *i915, unsigned int taint) >> { >> drm_notice(&i915->drm, "CI tainted: %#x by %pS\n", >> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h >> index feb078ae246f..71bdc89bd621 100644 >> --- a/drivers/gpu/drm/i915/i915_utils.h >> +++ b/drivers/gpu/drm/i915/i915_utils.h >> @@ -45,10 +45,6 @@ struct timer_list; >> #define MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \ >> __stringify(x), (long)(x)) >> >> -void __printf(3, 4) >> -__i915_printk(struct drm_i915_private *dev_priv, const char *level, >> - const char *fmt, ...); >> - >> #if IS_ENABLED(CONFIG_DRM_I915_DEBUG) >> >> int __i915_inject_probe_error(struct drm_i915_private *i915, int err, >> @@ -66,9 +62,12 @@ bool i915_error_injected(void); >> >> #define i915_inject_probe_failure(i915) i915_inject_probe_error((i915), -ENODEV) >> >> -#define i915_probe_error(i915, fmt, ...) \ >> - __i915_printk(i915, i915_error_injected() ? KERN_DEBUG : KERN_ERR, \ >> - fmt, ##__VA_ARGS__) >> +#define i915_probe_error(i915, fmt, ...) ({ \ >> + if (i915_error_injected()) \ >> + drm_dbg(&(i915)->drm, fmt, ##__VA_ARGS__); \ >> + else \ >> + drm_err(&(i915)->drm, fmt, ##__VA_ARGS__); \ >> +}) >> >> #define range_overflows(start, size, max) ({ \ >> typeof(start) start__ = (start); \
On 07/08/2024 12:40, Jani Nikula wrote: > On Wed, 07 Aug 2024, Tvrtko Ursulin <tursulin@ursulin.net> wrote: >> On 06/08/2024 14:38, Jani Nikula wrote: >>> With the previous cleanups, the last remaining user of __i915_printk() >>> is i915_probe_error(). Switch that to use drm_dbg() and drm_err() >>> instead, dropping the request to report bugs in the few remaining >>> specific cases. >> >> Aren't those few cases legitimate probe failures, including anything >> unexpected which results in non-operational GPU (any -EIO from >> intel_gt_init())? > > They are, and they're still logged as such. Functionally, the only > change is removing the bug filing request. > >> So it is effectively completely(*) removing the request to file bugs, or >> I miss something remained? Or the unmentioned goal is to encourage fewer >> i915 bug reports on top of the code base cleanup? > > I should've elaborated this better. > > My question is, what makes these cases so special that they warrant > logging a bug filing request? First, I would assume the init paths are > most tested in CI and least likely to trigger a failure on end user > machines. Second, even if they did trigger for the end user, a > non-operational GPU is most likely to lead to a bug report even without > a request. Yeah I tend to agree. Just wanted to probe a bit more on the motivation. Error captures aside, other places which can fail and which we are discussing here are a bit too varied and I agree it is better to simplify, rather than pretend some are more important than the others. Acked-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> Regards, Tvrtko > To me it just seems weird, and I opted to remove them, not least because > it's not common for drivers to do this at all. (And yes, I'd remove the > backlight one too.) > > The other option is to embrace logging bug reporting requests. But for > that I'd rather add a separate function, call it at the relevant places, > and not hide it within this complex maze of multi-level debug logging > macros. > > > BR, > Jani. > > > >> >> Regards, >> >> Tvrtko >> >> *) Apart from display/intel_dp_aux_backlight.c !? :) >> >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_utils.c | 41 ------------------------------- >>> drivers/gpu/drm/i915/i915_utils.h | 13 +++++----- >>> 2 files changed, 6 insertions(+), 48 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_utils.c b/drivers/gpu/drm/i915/i915_utils.c >>> index bee32222f0fd..b34a2d3d331d 100644 >>> --- a/drivers/gpu/drm/i915/i915_utils.c >>> +++ b/drivers/gpu/drm/i915/i915_utils.c >>> @@ -11,47 +11,6 @@ >>> #include "i915_reg.h" >>> #include "i915_utils.h" >>> >>> -#define FDO_BUG_MSG "Please file a bug on drm/i915; see " FDO_BUG_URL " for details." >>> - >>> -void >>> -__i915_printk(struct drm_i915_private *dev_priv, const char *level, >>> - const char *fmt, ...) >>> -{ >>> - static bool shown_bug_once; >>> - struct device *kdev = dev_priv->drm.dev; >>> - bool is_error = level[1] <= KERN_ERR[1]; >>> - bool is_debug = level[1] == KERN_DEBUG[1]; >>> - struct va_format vaf; >>> - va_list args; >>> - >>> - if (is_debug && !drm_debug_enabled(DRM_UT_DRIVER)) >>> - return; >>> - >>> - va_start(args, fmt); >>> - >>> - vaf.fmt = fmt; >>> - vaf.va = &args; >>> - >>> - if (is_error) >>> - dev_printk(level, kdev, "%pV", &vaf); >>> - else >>> - dev_printk(level, kdev, "[" DRM_NAME ":%ps] %pV", >>> - __builtin_return_address(0), &vaf); >>> - >>> - va_end(args); >>> - >>> - if (is_error && !shown_bug_once) { >>> - /* >>> - * Ask the user to file a bug report for the error, except >>> - * if they may have caused the bug by fiddling with unsafe >>> - * module parameters. >>> - */ >>> - if (!test_taint(TAINT_USER)) >>> - dev_notice(kdev, "%s", FDO_BUG_MSG); >>> - shown_bug_once = true; >>> - } >>> -} >>> - >>> void add_taint_for_CI(struct drm_i915_private *i915, unsigned int taint) >>> { >>> drm_notice(&i915->drm, "CI tainted: %#x by %pS\n", >>> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h >>> index feb078ae246f..71bdc89bd621 100644 >>> --- a/drivers/gpu/drm/i915/i915_utils.h >>> +++ b/drivers/gpu/drm/i915/i915_utils.h >>> @@ -45,10 +45,6 @@ struct timer_list; >>> #define MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \ >>> __stringify(x), (long)(x)) >>> >>> -void __printf(3, 4) >>> -__i915_printk(struct drm_i915_private *dev_priv, const char *level, >>> - const char *fmt, ...); >>> - >>> #if IS_ENABLED(CONFIG_DRM_I915_DEBUG) >>> >>> int __i915_inject_probe_error(struct drm_i915_private *i915, int err, >>> @@ -66,9 +62,12 @@ bool i915_error_injected(void); >>> >>> #define i915_inject_probe_failure(i915) i915_inject_probe_error((i915), -ENODEV) >>> >>> -#define i915_probe_error(i915, fmt, ...) \ >>> - __i915_printk(i915, i915_error_injected() ? KERN_DEBUG : KERN_ERR, \ >>> - fmt, ##__VA_ARGS__) >>> +#define i915_probe_error(i915, fmt, ...) ({ \ >>> + if (i915_error_injected()) \ >>> + drm_dbg(&(i915)->drm, fmt, ##__VA_ARGS__); \ >>> + else \ >>> + drm_err(&(i915)->drm, fmt, ##__VA_ARGS__); \ >>> +}) >>> >>> #define range_overflows(start, size, max) ({ \ >>> typeof(start) start__ = (start); \ >
Hi Jani, On Tue, Aug 06, 2024 at 04:38:32PM +0300, Jani Nikula wrote: > With the previous cleanups, the last remaining user of __i915_printk() > is i915_probe_error(). Switch that to use drm_dbg() and drm_err() > instead, dropping the request to report bugs in the few remaining > specific cases. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Good cleanup, thanks! Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Andi
diff --git a/drivers/gpu/drm/i915/i915_utils.c b/drivers/gpu/drm/i915/i915_utils.c index bee32222f0fd..b34a2d3d331d 100644 --- a/drivers/gpu/drm/i915/i915_utils.c +++ b/drivers/gpu/drm/i915/i915_utils.c @@ -11,47 +11,6 @@ #include "i915_reg.h" #include "i915_utils.h" -#define FDO_BUG_MSG "Please file a bug on drm/i915; see " FDO_BUG_URL " for details." - -void -__i915_printk(struct drm_i915_private *dev_priv, const char *level, - const char *fmt, ...) -{ - static bool shown_bug_once; - struct device *kdev = dev_priv->drm.dev; - bool is_error = level[1] <= KERN_ERR[1]; - bool is_debug = level[1] == KERN_DEBUG[1]; - struct va_format vaf; - va_list args; - - if (is_debug && !drm_debug_enabled(DRM_UT_DRIVER)) - return; - - va_start(args, fmt); - - vaf.fmt = fmt; - vaf.va = &args; - - if (is_error) - dev_printk(level, kdev, "%pV", &vaf); - else - dev_printk(level, kdev, "[" DRM_NAME ":%ps] %pV", - __builtin_return_address(0), &vaf); - - va_end(args); - - if (is_error && !shown_bug_once) { - /* - * Ask the user to file a bug report for the error, except - * if they may have caused the bug by fiddling with unsafe - * module parameters. - */ - if (!test_taint(TAINT_USER)) - dev_notice(kdev, "%s", FDO_BUG_MSG); - shown_bug_once = true; - } -} - void add_taint_for_CI(struct drm_i915_private *i915, unsigned int taint) { drm_notice(&i915->drm, "CI tainted: %#x by %pS\n", diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h index feb078ae246f..71bdc89bd621 100644 --- a/drivers/gpu/drm/i915/i915_utils.h +++ b/drivers/gpu/drm/i915/i915_utils.h @@ -45,10 +45,6 @@ struct timer_list; #define MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \ __stringify(x), (long)(x)) -void __printf(3, 4) -__i915_printk(struct drm_i915_private *dev_priv, const char *level, - const char *fmt, ...); - #if IS_ENABLED(CONFIG_DRM_I915_DEBUG) int __i915_inject_probe_error(struct drm_i915_private *i915, int err, @@ -66,9 +62,12 @@ bool i915_error_injected(void); #define i915_inject_probe_failure(i915) i915_inject_probe_error((i915), -ENODEV) -#define i915_probe_error(i915, fmt, ...) \ - __i915_printk(i915, i915_error_injected() ? KERN_DEBUG : KERN_ERR, \ - fmt, ##__VA_ARGS__) +#define i915_probe_error(i915, fmt, ...) ({ \ + if (i915_error_injected()) \ + drm_dbg(&(i915)->drm, fmt, ##__VA_ARGS__); \ + else \ + drm_err(&(i915)->drm, fmt, ##__VA_ARGS__); \ +}) #define range_overflows(start, size, max) ({ \ typeof(start) start__ = (start); \
With the previous cleanups, the last remaining user of __i915_printk() is i915_probe_error(). Switch that to use drm_dbg() and drm_err() instead, dropping the request to report bugs in the few remaining specific cases. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/i915_utils.c | 41 ------------------------------- drivers/gpu/drm/i915/i915_utils.h | 13 +++++----- 2 files changed, 6 insertions(+), 48 deletions(-)