Re: [PATCH v6 2/7] drm/i915: Record the sseu configuration per-context & engine

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

On 22/05/2018 18:59, Lionel Landwerlin wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> We want to expose the ability to reconfigure the slices, subslice and
> eu per context and per engine. To facilitate that, store the current
> configuration on the context for each engine, which is initially set
> to the device default upon creation.
> 
> v2: record sseu configuration per context & engine (Chris)
> 
> v3: introduce the i915_gem_context_sseu to store powergating
>      programming, sseu_dev_info has grown quite a bit (Lionel)
> 
> v4: rename i915_gem_sseu into intel_sseu (Chris)
>      use to_intel_context() (Chris)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c |  9 +++++++++
>   drivers/gpu/drm/i915/i915_gem_context.h | 17 +++++++++++++++++
>   drivers/gpu/drm/i915/i915_request.h     | 13 +++++++++++++
>   drivers/gpu/drm/i915/intel_lrc.c        | 22 +++++++++++-----------
>   4 files changed, 50 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index b69b18ef8120..ea9ae1046827 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -260,6 +260,8 @@ __create_hw_context(struct drm_i915_private *dev_priv,
>   		    struct drm_i915_file_private *file_priv)
>   {
>   	struct i915_gem_context *ctx;
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
>   	unsigned int n;
>   	int ret;
>   
> @@ -315,6 +317,13 @@ __create_hw_context(struct drm_i915_private *dev_priv,
>   	 * is no remap info, it will be a NOP. */
>   	ctx->remap_slice = ALL_L3_SLICES(dev_priv);
>   
> +	/* On all engines, use the whole device by default */
> +	for_each_engine(engine, dev_priv, id) {
> +		struct intel_context *ce = to_intel_context(ctx, engine);
> +
> +		ce->sseu = intel_sseu_from_device_sseu(&INTEL_INFO(dev_priv)->sseu);
> +	}
> +
>   	i915_gem_context_set_bannable(ctx);
>   	ctx->ring_size = 4 * PAGE_SIZE;
>   	ctx->desc_template =
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index c3262b4dd2ee..b18d870c4932 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -30,6 +30,7 @@
>   #include <linux/radix-tree.h>
>   
>   #include "i915_gem.h"
> +#include "intel_device_info.h"
>   
>   struct pid;
>   
> @@ -157,6 +158,9 @@ struct i915_gem_context {
>   		int pin_count;
>   
>   		const struct intel_context_ops *ops;
> +		
> +		/** sseu: Control eu/slice partitioning */
> +		union intel_sseu sseu;
>   	} __engine[I915_NUM_ENGINES];
>   
>   	/** ring_size: size for allocating the per-engine ring buffer */
> @@ -335,4 +339,17 @@ static inline void i915_gem_context_put(struct i915_gem_context *ctx)
>   	kref_put(&ctx->ref, i915_gem_context_release);
>   }
>   
> +static inline union intel_sseu
> +intel_sseu_from_device_sseu(const struct sseu_dev_info *sseu)
> +{
> +	union intel_sseu value = {
> +		.slice_mask = sseu->slice_mask,
> +		.subslice_mask = sseu->subslice_mask[0],
> +		.min_eus_per_subslice = sseu->max_eus_per_subslice,
> +		.max_eus_per_subslice = sseu->max_eus_per_subslice,
> +	};
> +

Why do we need two internal representations for SSEU configuration?

(Comes back later.) Ok, the INTEL_INFO one is much larger and intel_sseu 
presumably is designed to only contain data which is dynamically changeable?

> +	return value;
> +}
> +
>   #endif /* !__I915_GEM_CONTEXT_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index 1bbbb7a9fa03..aca60895582b 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -39,6 +39,19 @@ struct drm_i915_gem_object;
>   struct i915_request;
>   struct i915_timeline;
>   
> +/*
> + * Powergating configuration for a particular (context,engine).
> + */
> +union intel_sseu {
> +	struct {
> +		u8 slice_mask;
> +		u8 subslice_mask;
> +		u8 min_eus_per_subslice;
> +		u8 max_eus_per_subslice;
> +	};
> +	u64 value;

Why is the union needed?

> +};
> +
>   struct intel_wait {
>   	struct rb_node node;
>   	struct task_struct *tsk;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index c2500c209c63..f875be03eadb 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2481,8 +2481,8 @@ int logical_xcs_ring_init(struct intel_engine_cs *engine)
>   	return logical_ring_init(engine);
>   }
>   
> -static u32
> -make_rpcs(struct drm_i915_private *dev_priv)
> +static u32 make_rpcs(const struct sseu_dev_info *sseu,
> +		     union intel_sseu ctx_sseu)
>   {
>   	u32 rpcs = 0;
>   
> @@ -2492,24 +2492,23 @@ make_rpcs(struct drm_i915_private *dev_priv)
>   	 * must make an explicit request through RPCS for full
>   	 * enablement.
>   	*/
> -	if (INTEL_INFO(dev_priv)->sseu.has_slice_pg) {
> +	if (sseu->has_slice_pg) {
>   		rpcs |= GEN8_RPCS_S_CNT_ENABLE;
> -		rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.slice_mask) <<
> -			GEN8_RPCS_S_CNT_SHIFT;
> +		rpcs |= hweight8(ctx_sseu.slice_mask) << GEN8_RPCS_S_CNT_SHIFT;
>   		rpcs |= GEN8_RPCS_ENABLE;
>   	}
>   
> -	if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) {
> +	if (sseu->has_subslice_pg) {
>   		rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
> -		rpcs |= hweight8(INTEL_INFO(dev_priv)->sseu.subslice_mask[0]) <<
> +		rpcs |= hweight8(ctx_sseu.subslice_mask) <<
>   			GEN8_RPCS_SS_CNT_SHIFT;
>   		rpcs |= GEN8_RPCS_ENABLE;
>   	}
>   
> -	if (INTEL_INFO(dev_priv)->sseu.has_eu_pg) {
> -		rpcs |= INTEL_INFO(dev_priv)->sseu.eu_per_subslice <<
> +	if (sseu->has_eu_pg) {
> +		rpcs |= ctx_sseu.min_eus_per_subslice <<
>   			GEN8_RPCS_EU_MIN_SHIFT;
> -		rpcs |= INTEL_INFO(dev_priv)->sseu.eu_per_subslice <<
> +		rpcs |= ctx_sseu.max_eus_per_subslice <<
>   			GEN8_RPCS_EU_MAX_SHIFT;
>   		rpcs |= GEN8_RPCS_ENABLE;
>   	}
> @@ -2633,7 +2632,8 @@ static void execlists_init_reg_state(u32 *regs,
>   	if (rcs) {
>   		regs[CTX_LRI_HEADER_2] = MI_LOAD_REGISTER_IMM(1);
>   		CTX_REG(regs, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
> -			make_rpcs(dev_priv));
> +			make_rpcs(&INTEL_INFO(dev_priv)->sseu,
> +				  ctx->__engine[engine->id].sseu));

to_intel_context(ctx, engine)->sseu ?

>   
>   		i915_oa_init_reg_state(engine, ctx, regs);
>   	}
> 

Regards,

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