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

[Tvrtko Ursulin]

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.

It is true the reduction I see is due lengthening of the average timeout.

But with regards to re-arming approach, I thought so initially myself, 
but then, if we expect bursty access then it shouldn't matter and the 
simplicity of doing it like it currently is better.

Even in practice, I noticed the effect of lengthening the timeout is 
much greater than moving the arming to the last access. And to get to 
very few to none auto releases on busy workloads we need in the regions 
of 5ms, which would be a big change. Or maybe not if you consider HZ=100 
kernels.

It is very difficult to know what is actually better considering power 
between the CPU and GPU and performance. So I though the best would be 
to keep it similar to the current timings, just fix the dependency on HZ 
and also slack with hrtimers might help with something.

As a final data point, explicit puts and auto releases seems to be 
relatively balanced in my testing. With this patch T-Rex for example 
auto-releases in the region of 3-4 times in 10ms, with around 5-10 
explicit gets, and 5-10 implicit gets in 10ms.

A different, interrupt heavy workload (~20k irqs/sec) manages similarly 
2-4 auto-releases per 10ms, and has ~250 explicit gets and ~380 implicit 
per 10ms.

Looking at the two I think the fact they manage to auto-release 
relatively similarly, compared to a huge different in fw gets, suggest 
burstyness. So I am not sure that any smarts with the release period 
would be interesting. At least not without serious power/perf measurements.

> 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?

That would be similar to your patch from before my holiday, right? As I 
said above, I did not notice much change with that approach. Just 
extending the timeout has a much greater effect, but as I wrote above, I 
am not sure we want to change it.

Regards,

Tvrtko