[Intel-gfx] [RFC 1/4] drm/i915: split out uncore_mmio_debug

[Daniele Ceraolo Spurio]

On 6/26/19 10:58 AM, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2019-06-26 18:38:45)
>>
>>
>> On 6/26/19 3:02 AM, Chris Wilson wrote:
>>> Quoting Daniele Ceraolo Spurio (2019-06-24 21:31:49)
>>>> @@ -605,18 +614,20 @@ void intel_uncore_forcewake_get(struct intel_uncore *uncore,
>>>>    void intel_uncore_forcewake_user_get(struct intel_uncore *uncore)
>>>>    {
>>>>           spin_lock_irq(&uncore->lock);
>>>> +       spin_lock(&uncore->debug->lock);
>>>>           if (!uncore->user_forcewake.count++) {
>>>
>>> Afaict, uncore->user_forcewake.count is only guarded by uncore->lock
>>> and we only need to take debug->lock for the debug->unclaimed_mmio_check
>>> manipulation. But there needs to be a shared usage counter around the
>>> debug as it is shared state.
>>>
>>>>                   intel_uncore_forcewake_get__locked(uncore, FORCEWAKE_ALL);
>>>>    
>>>>                   /* Save and disable mmio debugging for the user bypass */
>>>>                   uncore->user_forcewake.saved_mmio_check =
>>>> -                       uncore->unclaimed_mmio_check;
>>>> +                       uncore->debug->unclaimed_mmio_check;
>>>>                   uncore->user_forcewake.saved_mmio_debug =
>>>>                           i915_modparams.mmio_debug;
>>>
>>> Something more like
>>>
>>> spin_lock_irq(&uncore->lock);
>>> if (!uncore->user_forcewake_count++) {
>>>        spin_lock(&uncore->debug->lock);
>>>        if (!uncore->debug->usage_count++) {
>>>                ...
>>>        }
>>>        spin_unlock(&uncore->debug->lock);
>>> }
>>> ...
>>> spin_unlock_irq(&uncore->lock);
>>> ?
>>> -Chris
>>>
>>
>> I do something similar in the next patch in the series (with the lock
>> still on the outside of the if). I've split that out because I've added
>> some functional changes to the flow. I can squash the 2 patches if you
>> thing it is better that way.
> 
> Yes. Looking at the second patch, that is clearer wrt what data we are
> guarding with the locks.
> 
> Don't drop the intel_ prefix from intel_uncore_debug as it looks to
> still be visible outside of its enclave (it's getting long, but it should

I'm assuming you're referring to the 2 new functions. These are actually 
meant to only used within the file and not be externally visible, I just 
forgot the static tag (and sparse complained for that). Everything else 
should still have the intel_ prefix.

Daniele

> be more or less internal to the various intel_uncore implementations) and
> squash these 2 as I feel more comfortable with intel_uncore_debug taking
> control of the locking as required for sharing and reviewing the locking
> wrt to the shared data.
> -Chris
>