Re: [PATCH v6 06/23] drm/i915/slpc: Use intel_slpc_* functions if supported

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux