Re: [PATCH 3/4] drm/i915: set optimum eu/slice/sub-slice configuration based on load type

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

On 21/09/2018 10:13, kedar.j.karanje@intel.com wrote:
> From: Praveen Diwakar <praveen.diwakar@intel.com>
> 
> This patch will select optimum eu/slice/sub-slice configuration based on type
> of load (low, medium, high) as input.
> Based on our readings and experiments we have predefined set of optimum
> configuration for each platform(CHT, KBL).
> i915_set_optimum_config will select optimum configuration from pre-defined
> optimum configuration table(opt_config).
> 
> Change-Id: I3a6a2a6bdddd01b3d3c97995f5403aef3c6fa989
> Signed-off-by: Praveen Diwakar <praveen.diwakar@intel.com>
> Signed-off-by: Yogesh Marathe <yogesh.marathe@intel.com>
> Signed-off-by: Aravindan Muthukumar <aravindan.muthukumar@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_gem_context.c | 46 +++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_context.h | 32 +++++++++++++++++++++++
>   2 files changed, 78 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 2838c1d..1b76410 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -94,6 +94,32 @@
>   
>   #define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1
>   
> +static struct optimum_config opt_config[TIER_VERSION_MAX][LOAD_TYPE_MAX] = {
> +	{
> +		/* Cherry trail low */
> +		{ 1, 1, 4},
> +		/* Cherry trail medium */
> +		{ 1, 1, 6},
> +		/* Cherry trail high */
> +		{ 1, 2, 6}
> +	},
> +	{
> +		/* kbl gt2 low */
> +		{ 2, 3, 4},
> +		/* kbl gt2 medium */
> +		{ 2, 3, 6},
> +		/* kbl gt2 high */
> +		{ 2, 3, 8}
> +	},
> +	{
> +		/* kbl gt3 low */
> +		{ 2, 3, 4},
> +		/* kbl gt3 medium */
> +		{ 2, 3, 6},
> +		/* kbl gt3 high */
> +		{ 2, 3, 8}
> +	}
> +};
>   static void lut_close(struct i915_gem_context *ctx)
>   {
>   	struct i915_lut_handle *lut, *ln;
> @@ -393,10 +419,30 @@ i915_gem_create_context(struct drm_i915_private *dev_priv,
>   	ctx->subslice_cnt = hweight8(
>   			INTEL_INFO(dev_priv)->sseu.subslice_mask[0]);
>   	ctx->eu_cnt = INTEL_INFO(dev_priv)->sseu.eu_per_subslice;
> +	ctx->load_type = 0;
> +	ctx->prev_load_type = 0;
>   
>   	return ctx;
>   }
>   
> +
> +void i915_set_optimum_config(int type, struct i915_gem_context *ctx,

Type for type should be enum.

Name the function as i915_gem_context_set_load_type(ctx, type).

> +		enum gem_tier_versions version)
> +{
> +	struct intel_context *ce = &ctx->__engine[RCS];
> +	u32 *reg_state = ce->lrc_reg_state;
> +	u32 rpcs_config = 0;

Unused locals?

> +	/* Call opt_config to get correct configuration for eu,slice,subslice */
> +	ctx->slice_cnt = (u8)opt_config[version][type].slice;
> +	ctx->subslice_cnt = (u8)opt_config[version][type].subslice;
> +	ctx->eu_cnt = (u8)opt_config[version][type].eu;

You could store the correct table for the device in dev_priv and use the 
static table to assign/populate on device init time.

> +
> +	/* Enabling this to update the rpcs */
> +	if (ctx->prev_load_type != type)
> +		ctx->update_render_config = 1;

ctx->update_render_config was unused in last patch. So patch ordering is 
a bit suboptimal but I don't know.

> +
> +	ctx->prev_load_type = type;
> +}
>   /**
>    * i915_gem_context_create_gvt - create a GVT GEM context
>    * @dev: drm device *
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index 52e341c..50183e6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -53,6 +53,26 @@ struct intel_context_ops {
>   	void (*destroy)(struct intel_context *ce);
>   };
>   
> +enum gem_load_type {
> +	LOAD_TYPE_LOW,
> +	LOAD_TYPE_MEDIUM,
> +	LOAD_TYPE_HIGH,
> +	LOAD_TYPE_MAX
> +};
> +
> +enum gem_tier_versions {
> +	CHERRYVIEW = 0,
> +	KABYLAKE_GT2,
> +	KABYLAKE_GT3,
> +	TIER_VERSION_MAX
> +};
> +
> +struct optimum_config {
> +	int slice;
> +	int subslice;
> +	int eu;

u8 would do global table definitely.

> +};
> +
>   /**
>    * struct i915_gem_context - client state
>    *
> @@ -210,6 +230,16 @@ struct i915_gem_context {
>   	/** eu_cnt: used to set the # of eu to be enabled. */
>   	u8 eu_cnt;
>   
> +	/** load_type: The designated load_type (high/medium/low) for a given
> +	 * number of pending commands in the command queue.
> +	 */
> +	u8 load_type;

Unused?

> +
> +	/** prev_load_type: The earlier load type that the GPU was configured
> +	 * for (high/medium/low).
> +	 */
> +	u8 prev_load_type;

Would pending_load_type sound more obvious?

Types should be enums for both.

Regards,

Tvrtko

> +
>   	/** update_render_config: to track the updates to the render
>   	 * configuration (S/SS/EU Configuration on the GPU)
>   	 */
> @@ -342,6 +372,8 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
>   				    struct drm_file *file_priv);
>   int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, void *data,
>   				       struct drm_file *file);
> +void i915_set_optimum_config(int type, struct i915_gem_context *ctx,
> +			     enum gem_tier_versions version);
>   
>   struct i915_gem_context *
>   i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx