Re: [PATCH v2] drm/i915: Keep the forcewake timer alive for 1ms past the most recent use

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 12/05/2017 23:16, Chris Wilson wrote:
> Currently the timer is armed for 1ms after the first use and is killed
> immediately, dropping the forcewake as early as possible. However, for

Correct for implicit grabs, but for explicit it is 1-2ms after the last use.

> very frequent operations the forcewake dance has a large impact on
> latency and keeping the timer alive until we are idle is preferred. To

What workloads see the difference and by how much?

At the time I've fixed the auto-release to go from 0-1 jiffies to 1-2ms, 
we talked about this conundrum - whether to consider the first grab or 
last put for the timer. But we decided thorough testing is needed to see 
if this would make a difference and what power side effects it might have.

> achieve this, if we call intel_uncore_forcewake_get whilst the timer is
> alive (repeated use), then set a flag to restart the timer on expiry
> rather than drop the forcewake usage count. The timer is racy, the
> consequence of the race is to expire the timer earlier than is now
> desired but does not impact on correct behaviour. The offset the race
> slightly, we set the active flag again on intel_uncore_forcewake_put.

Using the hrtimer API to modify the timer was too expensive?

>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 15 ++++++++++++---
>  drivers/gpu/drm/i915/intel_uncore.h |  1 +
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 7eaa592aed26..2fd0989805eb 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -216,6 +216,9 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
>
>  	assert_rpm_device_not_suspended(dev_priv);
>
> +	if (xchg(&domain->active, false))
> +		return HRTIMER_RESTART;
> +
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  	if (WARN_ON(domain->wake_count == 0))
>  		domain->wake_count++;
> @@ -246,6 +249,7 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>  		active_domains = 0;
>
>  		for_each_fw_domain(domain, dev_priv, tmp) {
> +			smp_store_mb(domain->active, false);
>  			if (hrtimer_cancel(&domain->timer) == 0)
>  				continue;
>
> @@ -453,9 +457,12 @@ static void __intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
>
>  	fw_domains &= dev_priv->uncore.fw_domains;
>
> -	for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp)
> -		if (domain->wake_count++)
> +	for_each_fw_domain_masked(domain, fw_domains, dev_priv, tmp) {
> +		if (domain->wake_count++) {
>  			fw_domains &= ~domain->mask;
> +			domain->active = true;
> +		}
> +	}
>
>  	if (fw_domains)
>  		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
> @@ -520,8 +527,10 @@ static void __intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
>  		if (WARN_ON(domain->wake_count == 0))
>  			continue;
>
> -		if (--domain->wake_count)
> +		if (--domain->wake_count) {
> +			domain->active = true;
>  			continue;
> +		}
>
>  		fw_domain_arm_timer(domain);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 5fec5fd4346c..dfead585835c 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -95,6 +95,7 @@ struct intel_uncore {
>  	struct intel_uncore_forcewake_domain {
>  		unsigned int mask;
>  		unsigned int wake_count;
> +		bool active;
>  		struct hrtimer timer;
>  		i915_reg_t reg_set;
>  		i915_reg_t reg_ack;
>

Minus the possible commit message improvements and discussion looks 
correct to me.

Regards,

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