[Intel-gfx] [PATCH 2/2] drm/i915: make debug printer shown_bug_once variable to drm_i915_private

[Vivi, Rodrigo]

> On Dec 11, 2019, at 7:25 AM, Joonas Lahtinen <joonas.lahtinen at linux.intel.com> wrote:
> 
> Quoting Jani Nikula (2019-12-11 12:36:10)
>> On Fri, 15 Nov 2019, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>>> Quoting Jani Nikula (2019-11-15 11:04:28)
>>>> On Fri, 15 Nov 2019, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>>>>> Quoting Jani Nikula (2019-11-15 10:18:40)
>>>>>> Get rid of the module specific static variable.
>>>>>> 
>>>>>> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/i915/i915_drv.h   | 2 ++
>>>>>> drivers/gpu/drm/i915/i915_utils.c | 9 ++++-----
>>>>>> 2 files changed, 6 insertions(+), 5 deletions(-)
>>>>>> 
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>>>> index 1779f600fcfb..e11ee3268ae3 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>>> @@ -1283,6 +1283,8 @@ struct drm_i915_private {
>>>>>>        /* Mutex to protect the above hdcp component related values. */
>>>>>>        struct mutex hdcp_comp_mutex;
>>>>>> 
>>>>>> +       bool shown_bug_once;
>>>>>> +
>>>>>>        I915_SELFTEST_DECLARE(struct i915_selftest_stash selftest;)
>>>>>> 
>>>>>>        /*
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_utils.c b/drivers/gpu/drm/i915/i915_utils.c
>>>>>> index c47261ae86ea..f434274b0b29 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_utils.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_utils.c
>>>>>> @@ -13,11 +13,10 @@
>>>>>>                    "providing the dmesg log by booting with drm.debug=0xf"
>>>>>> 
>>>>>> void
>>>>>> -__i915_printk(struct drm_i915_private *dev_priv, const char *level,
>>>>>> +__i915_printk(struct drm_i915_private *i915, const char *level,
>>>>>>              const char *fmt, ...)
>>>>>> {
>>>>>> -       static bool shown_bug_once;
>>>>>> -       struct device *kdev = dev_priv->drm.dev;
>>>>>> +       struct device *kdev = i915->drm.dev;
>>>>>>        bool is_error = level[1] <= KERN_ERR[1];
>>>>>>        bool is_debug = level[1] == KERN_DEBUG[1];
>>>>>>        struct va_format vaf;
>>>>>> @@ -39,7 +38,7 @@ __i915_printk(struct drm_i915_private *dev_priv, const char *level,
>>>>>> 
>>>>>>        va_end(args);
>>>>>> 
>>>>>> -       if (is_error && !shown_bug_once) {
>>>>>> +       if (is_error && !i915->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
>>>>>> @@ -47,7 +46,7 @@ __i915_printk(struct drm_i915_private *dev_priv, const char *level,
>>>>>>                 */
>>>>>>                if (!test_taint(TAINT_USER))
>>>>>>                        dev_notice(kdev, "%s", FDO_BUG_MSG);
>>>>> 
>>>>> I feel this plea to the users to file a bug report should be a one-time
>>>>> thing; a true global.
>>>> 
>>>> Fair enough.
>>>> 
>>>> I am wondering if we should have a convention of naming or commenting
>>>> legit globals, both to help automation detecting new accidental ones,
>>>> and to help people figure out not to send another conversion patch such
>>>> as this.
>>> 
>>> global_i915_show_bug_once
>>> module_i915_show_bug_once
>> 
>> I'd be fine with either of the prefixes. Or i915_{global,module}_ for
> 
> My vote goes for i915_global_* or global_i915_* (if we could get some
> Kbuild infrastructure behind detecting global variables).

any of the provided options or combinations works for me. probably global
will be less confusing...

> 
> Regards, Joonas