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

[Jani Nikula <jani.nikula@intel.com>]

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

-- 
Jani Nikula, Intel