Re: [PATCH v12 06/17] drm/i915/guc/slpc: Allocate/initialize/release SLPC shared data

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Fri, 30 Mar 2018 10:31:51 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

> Populate SLPC shared data with required default values for slice count,
> power source/plan, IA perf MSRs.
>
> v1: Update for SLPC interface version 2015.2.4. intel_slpc_active()
>     returns 1 if slpc initialized (Paulo)
>     change default host_os to "Windows"
>     Spelling fixes (Sagar and Nick Hoath). Added WARN for checking if
>     upper 32bits of GTT offset of shared object are zero. (Chris)
>     Updated commit message and moved POWER_PLAN and POWER_SOURCE defn.
>     from later patch. (Akash)
>     Add struct_mutex locking while allocating/releasing slpc shared
>     object. This was caught by CI BAT. Adding SLPC state variable to
>     determine if it is active as it not just dependent on shared data
>     setup. Rebase with guc_allocate_vma related changes.
>
> v2: WARN_ON for platform_sku validity and space changes.(David)
>     Checkpatch update.
>
> v3: Fixing WARNING in igt@drv_module_reload_basic found in trybot BAT
>     with SLPC Enabled.
>
> v4: Updated support for GuC v9.  
> s/slice_total/hweight8(slice_mask)/(Dave).
>
> v5: SLPC vma map changes and removed explicit type conversions.(Chris).
>     s/freq_unslice_max|min/unslice__max|min_freq.
>
> v6: Commit message update. s/msr_value/val for reuse later.
>
> v7: Set default values for tasks and min frequency parameters. Moved init
>     with allocation of data so that post GuC load earlier params persist.
>
> v8: Added check for SLPC status during cleanup of shared data. SLPC
>     disabling is asynchronous and should complete within 10us.
>
> v9: Enabling Balancer task in SLPC.
>
> v10: Rebase.
>
> v11: Rebase. Added lock specific to SLPC.
>
> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |   5 +
>  drivers/gpu/drm/i915/intel_guc_slpc.c | 204  
> ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc_slpc.h |   3 +
>  3 files changed, 212 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
> b/drivers/gpu/drm/i915/i915_drv.h
> index 5176801..d17e778 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2416,6 +2416,11 @@ intel_info(const struct drm_i915_private  
> *dev_priv)
> #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support)
> +#define IS_ULX_SKU(dev_priv)	(IS_SKL_ULX(dev_priv) ||  
> IS_KBL_ULX(dev_priv))
> +#define IS_ULT_SKU(dev_priv)	(IS_SKL_ULT(dev_priv) || \
> +				 IS_KBL_ULT(dev_priv) || \
> +				 IS_CFL_ULT(dev_priv))
> +
>  #define SKL_REVID_A0		0x0
>  #define SKL_REVID_B0		0x1
>  #define SKL_REVID_C0		0x2
> diff --git a/drivers/gpu/drm/i915/intel_guc_slpc.c  
> b/drivers/gpu/drm/i915/intel_guc_slpc.c
> index 63f100c..974a3c0 100644
> --- a/drivers/gpu/drm/i915/intel_guc_slpc.c
> +++ b/drivers/gpu/drm/i915/intel_guc_slpc.c
> @@ -4,10 +4,203 @@
>   * Copyright © 2015-2018 Intel Corporation
>   */
> +#include <asm/msr-index.h>
> +
> +#include "i915_drv.h"
>  #include "intel_guc_slpc.h"
> +static inline struct intel_guc *slpc_to_guc(struct intel_guc_slpc *slpc)
> +{
> +	return container_of(slpc, struct intel_guc, slpc);
> +}
> +
> +static unsigned int slpc_get_platform_sku(struct drm_i915_private  
> *dev_priv)

If you want to use 'slpc_' prefix for your functions, then always pass
struct intel_guc_slpc *slpc as first param

> +{
> +	enum slpc_platform_sku platform_sku;
> +
> +	if (IS_ULX_SKU(dev_priv))
> +		platform_sku = SLPC_PLATFORM_SKU_ULX;
> +	else if (IS_ULT_SKU(dev_priv))
> +		platform_sku = SLPC_PLATFORM_SKU_ULT;
> +	else
> +		platform_sku = SLPC_PLATFORM_SKU_DT;

Do we really need to pass this to SLPC/GuC?

> +
> +	WARN_ON(platform_sku > 0xFF);

Maybe just define this function to return u16 ?

> +
> +	return platform_sku;
> +}
> +
> +static unsigned int slpc_get_slice_count(struct drm_i915_private  
> *dev_priv)

Is this really slpc specific function ?
Maybe this can be added as inline from intel_device_info.h ?

> +{
> +	unsigned int slice_count = 1;
> +
> +	if (IS_SKYLAKE(dev_priv))
> +		slice_count = hweight8(INTEL_INFO(dev_priv)->sseu.slice_mask);
> +
> +	return slice_count;
> +}
> +
> +static void slpc_mem_set_param(struct slpc_shared_data *data,
> +			       u32 id,
> +			       u32 value)
> +{
> +	data->override_params_set_bits[id >> 5] |= (1 << (id % 32));
> +	data->override_params_values[id] = value;
> +}
> +
> +static void slpc_mem_unset_param(struct slpc_shared_data *data,
> +				 u32 id)
> +{
> +	data->override_params_set_bits[id >> 5] &= (~(1 << (id % 32)));
> +	data->override_params_values[id] = 0;
> +}
> +
> +static int slpc_mem_task_control(struct slpc_shared_data *data,
> +				 u64 val, u32 enable_id, u32 disable_id)
> +{
> +	int ret = 0;
> +
> +	if (val == SLPC_PARAM_TASK_DEFAULT) {
> +		/* set default */
> +		slpc_mem_unset_param(data, enable_id);
> +		slpc_mem_unset_param(data, disable_id);
> +	} else if (val == SLPC_PARAM_TASK_ENABLED) {
> +		/* set enable */
> +		slpc_mem_set_param(data, enable_id, 1);
> +		slpc_mem_unset_param(data, disable_id);
> +	} else if (val == SLPC_PARAM_TASK_DISABLED) {
> +		/* set disable */
> +		slpc_mem_set_param(data, disable_id, 1);
> +		slpc_mem_unset_param(data, enable_id);
> +	} else {
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static void slpc_shared_data_init(struct intel_guc_slpc *slpc)
> +{
> +	struct intel_guc *guc = slpc_to_guc(slpc);
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	struct slpc_platform_info *platform_info;
> +	struct slpc_shared_data *data;
> +	struct page *page;
> +	u64 val;
> +
> +	lockdep_assert_held(&slpc->lock);
> +
> +	page = i915_vma_first_page(slpc->vma);
> +	data = kmap_atomic(page);
> +	platform_info = &data->platform_info;
> +
> +	memset(data, 0, sizeof(struct slpc_shared_data));
> +
> +	data->shared_data_size = sizeof(struct slpc_shared_data);
> +	data->global_state = SLPC_GLOBAL_STATE_NOT_RUNNING;
> +	platform_info->sku = slpc_get_platform_sku(dev_priv);
> +	platform_info->slice_count = slpc_get_slice_count(dev_priv);
> +	platform_info->power_plan_source =
> +			SLPC_POWER_PLAN_SOURCE(SLPC_POWER_PLAN_BALANCED,
> +					       SLPC_POWER_SOURCE_AC);
> +
> +	rdmsrl(MSR_TURBO_RATIO_LIMIT, val);
> +	platform_info->p0_freq = val;
> +	rdmsrl(MSR_PLATFORM_INFO, val);
> +	platform_info->p1_freq = val >> 8;
> +	platform_info->pe_freq = val >> 40;
> +	platform_info->pn_freq = val >> 48;
> +
> +	/* Enable only GTPERF task, Disable others */
> +	val = SLPC_PARAM_TASK_ENABLED;
> +	slpc_mem_task_control(data, val,
> +			      SLPC_PARAM_TASK_ENABLE_GTPERF,
> +			      SLPC_PARAM_TASK_DISABLE_GTPERF);
> +
> +	slpc_mem_task_control(data, val,
> +			      SLPC_PARAM_TASK_ENABLE_BALANCER,
> +			      SLPC_PARAM_TASK_DISABLE_BALANCER);
> +
> +	val = SLPC_PARAM_TASK_DISABLED;
> +	slpc_mem_task_control(data, val,
> +			      SLPC_PARAM_TASK_ENABLE_DCC,
> +			      SLPC_PARAM_TASK_DISABLE_DCC);
> +
> +	slpc_mem_set_param(data,
> +			   SLPC_PARAM_GTPERF_THRESHOLD_MAX_FPS,
> +			   0);
> +
> +	slpc_mem_set_param(data,
> +			   SLPC_PARAM_GTPERF_ENABLE_FRAMERATE_STALLING,
> +			   0);
> +
> +	slpc_mem_set_param(data,
> +			   SLPC_PARAM_GLOBAL_ENABLE_IA_GT_BALANCING,
> +			   1);
> +
> +	slpc_mem_set_param(data,
> +			   SLPC_PARAM_GLOBAL_ENABLE_ADAPTIVE_BURST_TURBO,
> +			   0);
> +
> +	slpc_mem_set_param(data,
> +			   SLPC_PARAM_GLOBAL_ENABLE_EVAL_MODE,
> +			   0);
> +
> +	slpc_mem_set_param(data,
> +			   SLPC_PARAM_GLOBAL_ENABLE_BALANCER_IN_NON_GAMING_MODE,
> +			   1);
> +
> +	slpc_mem_set_param(data,
> +			   SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
> +			   intel_gpu_freq(dev_priv,
> +					  dev_priv->gt_pm.rps.efficient_freq));
> +
> +	slpc_mem_set_param(data,
> +			   SLPC_PARAM_GLOBAL_MIN_GT_SLICE_FREQ_MHZ,
> +			   intel_gpu_freq(dev_priv,
> +					  dev_priv->gt_pm.rps.efficient_freq));
> +
> +	kunmap_atomic(data);
> +}
> +
> +/**
> + * intel_guc_slpc_init() - Initialize the SLPC shared data structure.
> + * @slpc: pointer to intel_guc_slpc.
> + *
> + * This function will create object to be shared with GuC SLPC and
> + * initialize it with required initial parameter values for various
> + * SLPC knobs such as min frequency limit, enabling/disabling of SLPC
> + * tasks etc.
> + *
> + * Return: 0 on success, non-zero error code on failure.
> + */
> +
>  int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
>  {
> +	struct intel_guc *guc = slpc_to_guc(slpc);
> +	u32 size = PAGE_ALIGN(sizeof(struct slpc_shared_data));
> +	struct i915_vma *vma;
> +
> +	mutex_init(&slpc->lock);
> +
> +	mutex_lock(&slpc->lock);

Do we need this lock here ?

> +
> +	vma = slpc->vma;
> +	if (!vma) {
> +		/* Allocate shared data structure */
> +		vma = intel_guc_allocate_vma(guc, size);
> +		if (IS_ERR(vma)) {
> +			DRM_ERROR("Shared data allocation failed\n");
> +			mutex_unlock(&slpc->lock);
> +			return PTR_ERR(vma);
> +		}
> +		slpc->vma = vma;
> +	}
> +
> +	slpc_shared_data_init(slpc);
> +
> +	mutex_unlock(&slpc->lock);
> +
>  	return 0;
>  }
> @@ -20,6 +213,17 @@ void intel_guc_slpc_disable(struct intel_guc_slpc  
> *slpc)
>  {
>  }
> +/**
> + * intel_guc_slpc_fini() - Release the SLPC shared data structure.
> + * @slpc: pointer to intel_guc_slpc.
> + *
> + * This function will release the shared data. SLPC needs to be disabled
> + * prior to this.
> + */
>  void intel_guc_slpc_fini(struct intel_guc_slpc *slpc)
>  {
> +	/* Release shared data structure */
> +	mutex_lock(&slpc->lock);
> +	i915_vma_unpin_and_release(&slpc->vma);
> +	mutex_unlock(&slpc->lock);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_guc_slpc.h  
> b/drivers/gpu/drm/i915/intel_guc_slpc.h
> index 81250c0..87dac07 100644
> --- a/drivers/gpu/drm/i915/intel_guc_slpc.h
> +++ b/drivers/gpu/drm/i915/intel_guc_slpc.h
> @@ -9,6 +9,9 @@
>  #include <intel_guc_slpc_fwif.h>
> struct intel_guc_slpc {
> +	/* Protects access to vma and SLPC actions */
> +	struct mutex lock;
> +	struct i915_vma *vma;
>  };
> int intel_guc_slpc_init(struct intel_guc_slpc *slpc);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx