[13/36] drm/i915: Relinquish forcewake immediately after manual grouping
diff mbox series

Message ID 20200601072446.19548-13-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [01/36] drm/i915: Handle very early engine initialisation failure
Related show

Commit Message

Chris Wilson June 1, 2020, 7:24 a.m. UTC
Our forcewake utilisation is split into categories: automatic and
manual. Around bare register reads, we look up the right forcewake
domain and automatically acquire and release [upon a timer] the
forcewake domain. For other access, where we know we require the
forcewake across a group of register reads, we manually acquire the
forcewake domain and release it at the end. Again, this currently arms
the domain timer for a later release.

However, looking at some energy utilisation profiles, we have tried to
avoid using forcewake [and rely on the natural wake up to post register
updates] due to that even keep the fw active for a brief period
contributes to a significant power draw [i.e. when the gpu is sleeping
with rc6 at high clocks]. But as it turns out, not posting the writes
immediately also has unintended consequences, such as not reducing the
clocks and so conserving power while busy.

As a compromise, let us only arm the domain timer for automatic
forcewake usage around bare register access, but immediately release the
forcewake when manually acquired by intel_uncore_forcewake_get/_put.

The corollary to this is that we may instead have to take forcewake more
often, and so incur a latency penalty in doing so. For Sandybridge this
was significant, and even on the latest machines, taking forcewake at
interrupt frequency is a huge impact. [So we don't do that anymore!
Hopefully, this will spare us from still needing the mitigation of the
timer for steady state execution.]

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mika Kuoppala June 1, 2020, 12:20 p.m. UTC | #1
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Our forcewake utilisation is split into categories: automatic and
> manual. Around bare register reads, we look up the right forcewake
> domain and automatically acquire and release [upon a timer] the
> forcewake domain. For other access, where we know we require the
> forcewake across a group of register reads, we manually acquire the
> forcewake domain and release it at the end. Again, this currently arms
> the domain timer for a later release.
>
> However, looking at some energy utilisation profiles, we have tried to
> avoid using forcewake [and rely on the natural wake up to post register
> updates] due to that even keep the fw active for a brief period
> contributes to a significant power draw [i.e. when the gpu is sleeping
> with rc6 at high clocks]. But as it turns out, not posting the writes
> immediately also has unintended consequences, such as not reducing the
> clocks and so conserving power while busy.
>
> As a compromise, let us only arm the domain timer for automatic
> forcewake usage around bare register access, but immediately release the
> forcewake when manually acquired by intel_uncore_forcewake_get/_put.
>
> The corollary to this is that we may instead have to take forcewake more
> often, and so incur a latency penalty in doing so. For Sandybridge this
> was significant, and even on the latest machines, taking forcewake at
> interrupt frequency is a huge impact. [So we don't do that anymore!
> Hopefully, this will spare us from still needing the mitigation of the
> timer for steady state execution.]
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>

I am not a fan of having explicit put relying on timer,
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index a61cb8ca4d50..7d6b9ae7403c 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -709,7 +709,7 @@ static void __intel_uncore_forcewake_put(struct intel_uncore *uncore,
>  			continue;
>  		}
>  
> -		fw_domain_arm_timer(domain);
> +		uncore->funcs.force_wake_put(uncore, domain->mask);
>  	}
>  }
>  
> -- 
> 2.20.1

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index a61cb8ca4d50..7d6b9ae7403c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -709,7 +709,7 @@  static void __intel_uncore_forcewake_put(struct intel_uncore *uncore,
 			continue;
 		}
 
-		fw_domain_arm_timer(domain);
+		uncore->funcs.force_wake_put(uncore, domain->mask);
 	}
 }