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@xxxxxxxxx>
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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
