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.
>
> # 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.
> + * 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.
> +
> 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.
> 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.
> 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.
> 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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
