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

[Tvrtko Ursulin <tursulin@xxxxxxxxxxx>]

On 07/08/2024 12:40, Jani Nikula wrote:
> On Wed, 07 Aug 2024, Tvrtko Ursulin <tursulin@xxxxxxxxxxx> 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@xxxxxxxxxx>

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