[Intel-gfx] [PATCH 1/3] drm/i915: Use consistent forcewake auto-release timeout across kernel configs

[Dave Gordon]

On 04/04/16 19:58, Chris Wilson wrote:
> On Mon, Apr 04, 2016 at 05:51:09PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> Current implementation releases the forcewake at any time between
>> straight away, and one jiffie from the last put, or first automatic
>> grab.
>
> That isn't the problem though. The problem is that we set the timer on
> first use rather than last use. All you are stating here is that by
> lengthening the timeout on your system you reduce the number of times it
> times out.

I thought that was already done, per my comments on one of your recent 
patches (rename __force_wake_get to __force_wake_auto) and your reply 
(gains are probably worth extra complexity). But of course Tvrtko wasn't 
here the last few days so may not have seen that. I suppose it would 
make sense to fold both changes together.

But changing the timeout to 1ms makes it generally *shorter* than 
before, not longer. The gain can't just be from having a longer timeout, 
'cos it isn't. (Average timeout now 1.5ms vs. previous 8ms?)

I think it should come from the fact that accesses are likely to be 
bursty, having a bimodal distribution in the time domain. When we've 
made one access, we're likely to make another much sooner than 1ms 
later, but once we stop making accesses there will probably be far more 
than 1ms before the next set of accesses.

> Having said that, the conversion to hrtimer seems sensible but to add
> tracking of the last access as opposed to first we either fallback to
> jiffie (in which case hrtimer is moot) or rely on ktime_get_raw() being
> fast enough for every register write. Hmm, my usual response to that has
> been if it matters we avoid the heavyweight macros and use the _FW
> interface - or even raw readl/writel.
>
> Could you try storing ktime_get_raw() on every access and rearming the
> timer if it expires before last-access + min-period?
> -Chris

One more idea - do we want two different expiry periods depending on 
whether the caller used the _auto version or not?

If they did, they're probably anticipating "only a few" accesses in the 
current operation, but perhaps with no idea about when the next set of 
accesses will occur.

If not, then the final decrement means they think they've finished for a 
while, and perhaps don't expect to come back for quite a long time. In 
that situation the timeout only helps avoid rapid off/on cycles by 
unrelated functions, not between logically-related operations.

So would two different timeouts make sense? Or even ...

_auto: longish timeout from the *first* _get() - don't rearm thereafter
other: shortish timeout from the _put() - non-auto _get() will cancel

or is that just too complicated?

.Dave.