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

[Tvrtko Ursulin]

On 07/08/2024 12:40, Jani Nikula wrote:
> On Wed, 07 Aug 2024, Tvrtko Ursulin <tursulin at 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 at 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 at 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); \
>