Re: [PATCH v3 2/4] drm/i915: Update render power clock state configuration for given context

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

On 11/12/2018 10:14, Ankit Navik wrote:
> From: Praveen Diwakar <praveen.diwakar@intel.com>
> 
> This patch will update power clock state register at runtime base on the
> flag which can set by any governor which computes load and want to update
> rpcs register.
> subsequent patches will have a timer based governor which computes pending
> load/request.
> 
> V2:
>   * Reuse make_rpcs to get rpcs config. (Tvrtko Ursulin)
> 
> V3:
>   * Rebase.
> 
> Cc: Aravindan Muthukumar <aravindan.muthukumar@intel.com>
> Cc: Kedar J Karanje <kedar.j.karanje@intel.com>
> Cc: Yogesh Marathe <yogesh.marathe@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>

Again, I did not give an r-b for this!

> Signed-off-by: Praveen Diwakar <praveen.diwakar@intel.com>
> Signed-off-by: Ankit Navik <ankit.p.navik@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c |  4 ++++
>   drivers/gpu/drm/i915/i915_gem_context.h |  9 +++++++++
>   drivers/gpu/drm/i915/intel_lrc.c        | 12 +++++++++++-
>   3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 0bcbe32..d040858 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -388,6 +388,10 @@ i915_gem_create_context(struct drm_i915_private *dev_priv,
>   
>   	trace_i915_context_create(ctx);
>   	atomic_set(&ctx->req_cnt, 0);
> +	ctx->slice_cnt = hweight8(INTEL_INFO(dev_priv)->sseu.slice_mask);
> +	ctx->subslice_cnt = hweight8(
> +			INTEL_INFO(dev_priv)->sseu.subslice_mask[0]);
> +	ctx->eu_cnt = INTEL_INFO(dev_priv)->sseu.eu_per_subslice;
>   
>   	return ctx;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index e824b15..e000530 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -199,6 +199,15 @@ struct i915_gem_context {
>   	 * go for low/medium/high load configuration of the GPU.
>   	 */
>   	atomic_t req_cnt;
> +
> +	/** slice_cnt: used to set the # of slices to be enabled. */
> +	u8 slice_cnt;
> +
> +	/** subslice_cnt: used to set the # of subslices to be enabled. */
> +	u8 subslice_cnt;
> +
> +	/** eu_cnt: used to set the # of eu to be enabled. */
> +	u8 eu_cnt;
>   };
>   
>   static inline bool i915_gem_context_is_closed(const struct i915_gem_context *ctx)
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d33f5ac..a17f676 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -171,6 +171,7 @@ static void execlists_init_reg_state(u32 *reg_state,
>   				     struct i915_gem_context *ctx,
>   				     struct intel_engine_cs *engine,
>   				     struct intel_ring *ring);
> +static u32 make_rpcs(struct drm_i915_private *dev_priv);
>   
>   static inline struct i915_priolist *to_priolist(struct rb_node *rb)
>   {
> @@ -417,12 +418,21 @@ execlists_update_context_pdps(struct i915_hw_ppgtt *ppgtt, u32 *reg_state)
>   
>   static u64 execlists_update_context(struct i915_request *rq)
>   {
> +	u32 rpcs_config = 0;

Move to if block where it is used.

>   	struct intel_context *ce = rq->hw_context;
> +	u32 *reg_state = ce->lrc_reg_state;

No need to touch this.

> +	struct i915_gem_context *ctx = rq->gem_context;

Can go under the if block as well.

>   	struct i915_hw_ppgtt *ppgtt =
>   		rq->gem_context->ppgtt ?: rq->i915->mm.aliasing_ppgtt;
> -	u32 *reg_state = ce->lrc_reg_state;
>   
>   	reg_state[CTX_RING_TAIL+1] = intel_ring_set_tail(rq->ring, rq->tail);
> +	/* FIXME: To avoid stale rpcs config, move it to context_pin */

This FIXME remains unresolved by the whole series, so that really cannot 
be like that.

I suggest you add https://patchwork.freedesktop.org/patch/261560/ to 
your series and rebase this on top.

Which would actually make this patch not do very much and you should 
probably squash it with the one which actually uses the added fields.

> +	if (ctx->pid && ctx->name && (rq->engine->id == RCS)) {

Why name and pid? You are using them as proxy for something.. but for 
what and why? The answer may hold a hint to the proper solution.

> +		rpcs_config = make_rpcs(ctx->i915);
> +		reg_state[CTX_LRI_HEADER_2] = MI_LOAD_REGISTER_IMM(1);
> +		CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
> +				rpcs_config);
> +	}
>   
>   	/* True 32b PPGTT with dynamic page allocation: update PDP
>   	 * registers and point the unallocated PDPs to scratch page.
> 

Regards,

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