Re: [PATCH 3/3] drm/i915: remove __i915_printk()

[Tvrtko Ursulin <tursulin@ursulin.net>]

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); \