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

[Dave Gordon <david.s.gordon@intel.com>]

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@linux.intel.com>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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).
> -Chris

'jiffies + 1' might be only a nanosecond away; would it be better to use 
'jiffies + 2'? OTOH that might be quite a long time and therefore 
increase power consumption :( So is there a somewhat higher-resolution 
cyclic timer that we could use?

Also, why mod_timer_pinned() ? I'd think that if this actually happens a 
whole jiffie later, there'd be little correlation between the current 
CPU activity and what's happening when the timer fires, so no real point 
in pinning the timer to current CPU.

Using mod_timer() instead would allow it to apply slack and align the 
callback to other timer activity, maybe reducing CPU overhead at the 
possible cost of a slight increase in GPU power.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx