[Intel-gfx] [PATCH] drm/i915: Only arm the forcewake release timer on the final put

[Tvrtko Ursulin]

On 24/03/16 13:42, Chris Wilson wrote:
> On Thu, Mar 24, 2016 at 01:32:53PM +0000, Chris Wilson wrote:
>> If we arm the release timer on acquiring the forcewake, we will release
>> the forcewake on the jiffie afterwards. If we only arm the release timer
>> on the final put, we will release the forcewake slightly later instead.
>>
>> Much, much worse, we did not acquire a refcount for the armed timing
>> during the get(), and so unbalanced our forcewake counting.
>>
>> Reported-by: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_uncore.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 96799392c2c7..d857168c6c9b 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -60,6 +60,7 @@ fw_domain_reset(const struct intel_uncore_forcewake_domain *d)
>>   static inline void
>>   fw_domain_arm_timer(struct intel_uncore_forcewake_domain *d)
>>   {
>> +	d->wake_count++;
>>   	mod_timer_pinned(&d->timer, jiffies + 1);
>
> Which raise the obvious issue that we double increment the counter if
> the timer was pending (where we would only then release it once).

I don't see the bug, we got:

1) __intel_uncore_forcewake_put - if refcount reaches zero it will bump 
it and arm the timer to decrement and release. This is used from 
explicit get/put paths.

2) __force_wake_get - used from register reads only, so no explicit put 
will happen. It just bumps the ref count and arms the timer.

I can't spot the bug, if there is one.

Regards,

Tvrtko