On 3/17/2017 2:35 AM, Chris Wilson wrote:
On Thu, Mar 16, 2017 at 11:58:10PM +0530, Sagar Arun Kamble wrote:
On platforms with SLPC support: call intel_slpc_*() functions from
intel_*_gt_powersave() functions and GuC setup functions and do not
use rps functions. intel_slpc_enable is tied to GuC setup.
With SLPC, intel_enable_gt_powersave will only handle RC6 and ring
frequencies. intel_init_gt_powersave will check if SLPC has started
running through shared data and update initial state that i915 needs
like frequency limits.
Host will not manage GT frequency with this change.
v1: Return void instead of ignored error code (Paulo)
enable/disable RC6 in SLPC flows (Sagar)
replace HAS_SLPC() use with intel_slpc_enabled()
or intel_slpc_active() (Paulo)
Fix for renaming gen9_disable_rps to gen9_disable_rc6 in
"drm/i915/bxt: Explicitly clear the Turbo control register"
Defer RC6 and SLPC enabling to intel_gen6_powersave_work. (Sagar)
Performance drop with SLPC was happening as ring frequency table
was not programmed when SLPC was enabled. This patch programs ring
frequency table with SLPC. Initial reset of SLPC is based on kernel
parameter as planning to add slpc state in intel_slpc_active. Cleanup
is also based on kernel parameter as SLPC gets disabled in
disable/suspend.(Sagar)
v2: Usage of INTEL_GEN instead of INTEL_INFO->gen (David)
Checkpatch update.
v3: Rebase
v4: Removed reset functions to comply with *_gt_powersave routines.
(Sagar)
v5: Removed intel_slpc_active. Relying on slpc.active for control flows
that are based on SLPC active status in GuC. State setup/cleanup needed
for SLPC is handled using kernel parameter i915.enable_slpc. Moved SLPC
init and enabling to GuC enable path as SLPC in GuC can start doing the
setup post GuC init. Commit message update. (Sagar)
v6: Rearranged function definitions.
Signed-off-by: Tom O'Rourke <Tom.O'Rourke@xxxxxxxxx>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
---
drivers/gpu/drm/i915/Makefile | 3 +-
drivers/gpu/drm/i915/i915_drv.c | 2 ++
drivers/gpu/drm/i915/i915_gem.c | 8 +++++
drivers/gpu/drm/i915/i915_guc_submission.c | 3 ++
drivers/gpu/drm/i915/intel_pm.c | 51 ++++++++++++++++++++++++------
drivers/gpu/drm/i915/intel_slpc.c | 46 +++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_slpc.h | 38 ++++++++++++++++++++++
drivers/gpu/drm/i915/intel_uc.c | 11 +++++++
drivers/gpu/drm/i915/intel_uc.h | 3 ++
9 files changed, 154 insertions(+), 11 deletions(-)
create mode 100644 drivers/gpu/drm/i915/intel_slpc.c
create mode 100644 drivers/gpu/drm/i915/intel_slpc.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 2cf0450..a4a8e0b 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -60,7 +60,8 @@ i915-y += intel_uc.o \
intel_guc_log.o \
intel_guc_loader.o \
intel_huc.o \
- i915_guc_submission.o
+ i915_guc_submission.o \
+ intel_slpc.o
Something here is not in alphabetical order.
Yes. Will fix this.
# autogenerated null render state
i915-y += intel_renderstate_gen6.o \
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0302d24..c0eb3d2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1762,6 +1762,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
hsw_disable_pc8(dev_priv);
}
+ dev_priv->guc.slpc.active = false;
+
intel_uncore_sanitize(dev_priv);
if (IS_GEN9_LP(dev_priv) ||
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d87983b..db55285 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2900,6 +2900,14 @@ void i915_gem_reset(struct drm_i915_private *dev_priv)
i915_gem_restore_fences(dev_priv);
+ /*
+ * GPU is reset at this point, Hence mark SLPC as inactive to
First sentence is redundant.
Will fix this.
+ * not send h2g action to shutdown SLPC as that will fail.
+ * enable_gt_powersave will setup RC6 and ring frequencies and
+ * SLPC will be enabled post GuC initialization.
+ */
+ dev_priv->guc.slpc.active = false;
It suggests we should be hooking guc into the reset prepare/do/finish
more thoroughly.
Yes. Might have to rework w.r.t these flows. Will update.
+
if (dev_priv->gt.awake) {
intel_sanitize_gt_powersave(dev_priv);
intel_enable_gt_powersave(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 5ec2cbd..1c9f859 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -902,6 +902,9 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
intel_guc_log_create(guc);
guc_addon_create(guc);
+ if (i915.enable_slpc)
+ intel_slpc_init(dev_priv);
This is not a hotpath, move the test to the callee.
Ok. Will update.
guc->execbuf_client = guc_client_alloc(dev_priv,
INTEL_INFO(dev_priv)->ring_mask,
GUC_CTX_PRIORITY_KMD_NORMAL,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8e41596..9c47d65 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5283,6 +5283,9 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
void gen6_rps_busy(struct drm_i915_private *dev_priv)
{
+ if (i915.enable_slpc)
+ return;
Feels very wrong. Anticipating this should be more akin to disabling at
the intel_set_rps() (or a nop callback), or just using rps.enabled to
skip.
Will make this return based on rps.enabled.
void intel_sanitize_gt_powersave(struct drm_i915_private *dev_priv)
{
- dev_priv->rps.enabled = true; /* force disabling */
+ if (!i915.enable_slpc)
+ dev_priv->rps.enabled = true; /* force disabling */
Nope. Please consider the point of this function to ensure that whatever
we inherit from the BIOS is cleared.
Forceful disabling here clears the RP_CONTROL register which controls the SLPC frequency requests.
Would need to preserve the BIOS set value so will copy prior to clearing here and set again when enabling SLPC.
dev_priv->rps.rc6_enabled = true;
+
intel_disable_gt_powersave(dev_priv);
- gen6_reset_rps_interrupts(dev_priv);
+ if (!i915.enable_slpc)
+ gen6_reset_rps_interrupts(dev_priv);
}
void intel_disable_gt_powersave(struct drm_i915_private *dev_priv)
{
- if (!READ_ONCE(dev_priv->rps.enabled)) {
So what is preventing us from just not marking rps.enabled as enabled
under slpc?
In all, i915.enable_slpc should be rare.
-Chris
RP_CONTROL clearing was the issue. Will update to rely on rps.enabled as much as possible.
Will hopefully be able to reduce the i915.enable_slpc occurrences.
Thanks for the review.
Thanks
Sagar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
