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

Tvrtko Ursulin
2024-08-07T14:25:12Z

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