Re: [PATCH v3 4/4] drm/i915: Predictive governor to control eu/slice/subslice

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 11/12/2018 10:14, Ankit Navik wrote:
> From: Praveen Diwakar <praveen.diwakar@intel.com>
> 
> High resolution timer is used for predictive governor to control
> eu/slice/subslice based on workloads.
> 
> Debugfs is provided to enable/disable/update timer configuration
> 
> V2:
>   * Fix code style.
>   * Move predictive_load_timer into a drm_i915_private
>     structure.
>   * Make generic function to set optimum config. (Tvrtko Ursulin)
> 
> V3:
>   * Rebase.
>   * Fix race condition for predictive load set.
>   * Add slack to start hrtimer for more power efficient. (Tvrtko Ursulin)
> 
> Cc: Aravindan Muthukumar <aravindan.muthukumar@intel.com>
> Cc: Yogesh Marathe <yogesh.marathe@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>

For the record - no, I did not r-b this. Please remove all false r-b tags.

> Signed-off-by: Praveen Diwakar <praveen.diwakar@intel.com>
> Signed-off-by: Kedar J Karanje <kedar.j.karanje@intel.com>
> Signed-off-by: Ankit Navik <ankit.p.navik@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 90 ++++++++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/i915/i915_drv.c     |  4 ++
>   drivers/gpu/drm/i915/i915_drv.h     |  6 +++
>   3 files changed, 99 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index f9ce35d..861f3c1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4740,6 +4740,92 @@ static const struct drm_info_list i915_debugfs_list[] = {
>   	{"i915_drrs_status", i915_drrs_status, 0},
>   	{"i915_rps_boost_info", i915_rps_boost_info, 0},
>   };
> +
> +#define PENDING_REQ_0	0	/* No active request pending */

Still seems silly - what is the benefit of adding a define with zero in 
it's name, which evaluates to zero? Just remove it.

> +
> +/*
> + * Anything above threshold is considered as HIGH load, less is considered
> + * as LOW load and equal is considered as MEDIUM load.
> + *
> + * The threshold value of three active requests pending.
> + */
> +#define PENDING_THRESHOLD_MEDIUM 3
> +
> +#define SLACK_TIMER_NSEC 1000000 /* Timer range in nano second */
> +
> +enum hrtimer_restart predictive_load_cb(struct hrtimer *hrtimer)
> +{
> +	struct drm_i915_private *dev_priv =
> +			container_of(hrtimer, typeof(*dev_priv), pred_timer);
> +	struct i915_gem_context *ctx;
> +	enum gem_load_type load_type;
> +	unsigned long req_pending;

unsigned int is enough to hold atomic_t.

> +
> +	list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
> +
> +		/* Check for in-valid context */
> +		if (!ctx->name)
> +			continue;

What kind of contexts are these invalid ones? :)

> +
> +		req_pending = atomic_read(&ctx->req_cnt);
> +
> +		/*
> +		 * Transitioning to low state whenever pending request is zero
> +		 * would cause vacillation between low and high state.
> +		 */
> +		if (req_pending == PENDING_REQ_0)
> +			continue;
> +
> +		if (req_pending > PENDING_THRESHOLD_MEDIUM)
> +			load_type = LOAD_TYPE_HIGH;
> +		else if (req_pending == PENDING_THRESHOLD_MEDIUM)
> +			load_type = LOAD_TYPE_MEDIUM;
> +		else
> +			load_type = LOAD_TYPE_LOW;
> +
> +		i915_gem_context_set_load_type(ctx, load_type);
> +	}
> +
> +	hrtimer_forward_now(hrtimer,
> +			ms_to_ktime(dev_priv->predictive_load_enable));
> +
> +	return HRTIMER_RESTART;
> +}
> +
> +static int i915_predictive_load_get(void *data, u64 *val)
> +{
> +	struct drm_i915_private *dev_priv = data;
> +
> +	*val = dev_priv->predictive_load_enable;
> +	return 0;
> +}
> +
> +static int i915_predictive_load_set(void *data, u64 val)
> +{
> +	struct drm_i915_private *dev_priv = data;
> +
> +	mutex_lock(&dev_priv->pred_mutex);
> +
> +	dev_priv->predictive_load_enable = val;
> +
> +	if (dev_priv->predictive_load_enable) {

The period change applies after the current timer expires, okay.

> +		if (!hrtimer_active(&dev_priv->pred_timer))
> +			hrtimer_start_range_ns(&dev_priv->pred_timer,
> +			ms_to_ktime(dev_priv->predictive_load_enable),
> +			SLACK_TIMER_NSEC,
> +			HRTIMER_MODE_REL_PINNED);
> +	} else {
> +		hrtimer_cancel(&dev_priv->pred_timer);
> +	}
> +
> +	mutex_unlock(&dev_priv->pred_mutex);
> +	return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(i915_predictive_load_ctl,
> +			i915_predictive_load_get, i915_predictive_load_set,
> +			"%llu\n");
> +
>   #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>   
>   static const struct i915_debugfs_files {
> @@ -4769,7 +4855,9 @@ static const struct i915_debugfs_files {
>   	{"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops},
>   	{"i915_ipc_status", &i915_ipc_status_fops},
>   	{"i915_drrs_ctl", &i915_drrs_ctl_fops},
> -	{"i915_edp_psr_debug", &i915_edp_psr_debug_fops}
> +	{"i915_edp_psr_debug", &i915_edp_psr_debug_fops},
> +	/* FIXME: When feature will become real, move to sysfs */
> +	{"i915_predictive_load_ctl", &i915_predictive_load_ctl}
>   };
>   
>   int i915_debugfs_register(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f8cfd16..79f4df5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1397,6 +1397,10 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	if (ret < 0)
>   		goto out_cleanup_hw;
>   
> +	/* Timer initialization for predictive load */
> +	hrtimer_init(&dev_priv->pred_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	dev_priv->pred_timer.function = predictive_load_cb;

mutex_init(&dev_priv->pred_mutex) is missing.

And please move this whole block to i915_gem_init_early.

> +
>   	i915_driver_register(dev_priv);
>   
>   	intel_runtime_pm_enable(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4b9a8c5..a78fdbc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1684,6 +1684,11 @@ struct drm_i915_private {
>   	/* optimal slice/subslice/EU configration state */
>   	struct i915_sseu_optimum_config opt_config[LOAD_TYPE_LAST];
>   
> +	/* protects predictive load state */
> +	struct mutex pred_mutex;
> +	struct hrtimer pred_timer;
> +	int predictive_load_enable;
> +
>   	unsigned int fsb_freq, mem_freq, is_ddr3;
>   	unsigned int skl_preferred_vco_freq;
>   	unsigned int max_cdclk_freq;
> @@ -2730,6 +2735,7 @@ extern unsigned long i915_chipset_val(struct drm_i915_private *dev_priv);
>   extern unsigned long i915_mch_val(struct drm_i915_private *dev_priv);
>   extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
>   extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
> +extern enum hrtimer_restart predictive_load_cb(struct hrtimer *hrtimer);
>   int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
>   
>   int intel_engines_init_mmio(struct drm_i915_private *dev_priv);
> 

Looking smaller and better, right?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx