Hi Tvrtko, > -----Original Message----- > From: Tvrtko Ursulin [mailto:tvrtko.ursulin@xxxxxxxxxxxxxxx] > Sent: Friday, September 21, 2018 6:22 PM > To: J Karanje, Kedar <kedar.j.karanje@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx > Cc: Diwakar, Praveen <praveen.diwakar@xxxxxxxxx>; Marathe, Yogesh > <yogesh.marathe@xxxxxxxxx>; Navik, Ankit P <ankit.p.navik@xxxxxxxxx>; > Muthukumar, Aravindan <aravindan.muthukumar@xxxxxxxxx> > Subject: Re: [PATCH 2/4] drm/i915: Update render power clock state > configuration for given context > > > On 21/09/2018 10:13, kedar.j.karanje@xxxxxxxxx wrote: > > From: Praveen Diwakar <praveen.diwakar@xxxxxxxxx> > > > > This patch will update power clock state register at runtime base on > > the flag update_render_config 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. > > > > Change-Id: I4e7d2f484b957d5bd496e1decc59a69e3bc6d186 > > Signed-off-by: Praveen Diwakar <praveen.diwakar@xxxxxxxxx> > > Signed-off-by: Yogesh Marathe <yogesh.marathe@xxxxxxxxx> > > Signed-off-by: Aravindan Muthukumar <aravindan.muthukumar@xxxxxxxxx> > > Signed-off-by: Kedar J Karanje <kedar.j.karanje@xxxxxxxxx> > > Signed-off-by: Ankit Navik <ankit.p.navik@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/i915_gem_context.c | 5 ++++ > > drivers/gpu/drm/i915/i915_gem_context.h | 14 +++++++++++ > > drivers/gpu/drm/i915/intel_lrc.c | 41 > +++++++++++++++++++++++++++++++++ > > 3 files changed, 60 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > > b/drivers/gpu/drm/i915/i915_gem_context.c > > index 30932d9..2838c1d 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -388,6 +388,11 @@ i915_gem_create_context(struct drm_i915_private > > *dev_priv, > > > > trace_i915_context_create(ctx); > > ctx->req_cnt = 0; > > + ctx->update_render_config = 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 243ea22..52e341c 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.h > > +++ b/drivers/gpu/drm/i915/i915_gem_context.h > > @@ -200,6 +200,20 @@ struct i915_gem_context { > > * controlled via a mutex > > */ > > u64 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; > > + > > + /** update_render_config: to track the updates to the render > > + * configuration (S/SS/EU Configuration on the GPU) > > + */ > > + bool update_render_config; > > }; > > > > 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 039fbdb..d2d0e7d 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -364,6 +364,36 @@ execlists_unwind_incomplete_requests(struct > intel_engine_execlists *execlists) > > spin_unlock_irqrestore(&engine->timeline.lock, flags); > > } > > > > +static u32 > > +get_context_rpcs_config(struct i915_gem_context *ctx) { > > + struct drm_i915_private *dev_priv = ctx->i915; > > + u32 rpcs = 0; > > + > > + if (INTEL_GEN(dev_priv) < 8) > > + return 0; > > + > > + if (INTEL_INFO(dev_priv)->sseu.has_slice_pg) { > > + rpcs |= GEN8_RPCS_S_CNT_ENABLE; > > + rpcs |= ctx->slice_cnt << GEN8_RPCS_S_CNT_SHIFT; > > + rpcs |= GEN8_RPCS_ENABLE; > > + } > > + > > + if (INTEL_INFO(dev_priv)->sseu.has_subslice_pg) { > > + rpcs |= GEN8_RPCS_SS_CNT_ENABLE; > > + rpcs |= ctx->subslice_cnt << GEN8_RPCS_SS_CNT_SHIFT; > > + rpcs |= GEN8_RPCS_ENABLE; > > + } > > + > > + if (INTEL_INFO(dev_priv)->sseu.has_eu_pg) { > > + rpcs |= ctx->eu_cnt << GEN8_RPCS_EU_MIN_SHIFT; > > + rpcs |= ctx->eu_cnt << GEN8_RPCS_EU_MAX_SHIFT; > > + rpcs |= GEN8_RPCS_ENABLE; > > + } > > + > > + return rpcs; > > +} > > This function is very similar to make_rpcs so I'd suggest to extract some > commonality and share the code. Incorporated Changes in v2. > > In any case you are likely to get overtaken by > https://patchwork.freedesktop.org/series/48194/ after which you will be able > to use struct intel_sseu to package the data you need in one ctx member, etc. > > > + > > static inline void > > execlists_context_status_change(struct i915_request *rq, unsigned long > status) > > { > > @@ -418,11 +448,22 @@ execlists_update_context_pdps(struct > i915_hw_ppgtt *ppgtt, u32 *reg_state) > > static u64 execlists_update_context(struct i915_request *rq) > > { > > struct intel_context *ce = rq->hw_context; > > + struct i915_gem_context *ctx = rq->gem_context; > > + struct intel_engine_cs *engine = rq->engine; > > struct i915_hw_ppgtt *ppgtt = > > rq->gem_context->ppgtt ?: rq->i915->mm.aliasing_ppgtt; > > u32 *reg_state = ce->lrc_reg_state; > > + u32 rpcs_config = 0; > > > > reg_state[CTX_RING_TAIL+1] = intel_ring_set_tail(rq->ring, > > rq->tail); > > + if (ctx->pid && ctx->name && (rq->engine->id == RCS) && > > + ctx->update_render_config) { > > + rpcs_config = get_context_rpcs_config(ctx); > > + 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); > > For a first submission this works, but when appending to a running context the > RPCS value you write here will probably get overwritten by context save. At least > I would be surprised if lite-restore would write to the register. > > So if I am correct the problem you have is that your configuration changes will > not always become live when you expect it to. And then if you hit lite-restore > you don't try to re-configure it later unless there is a config change. > > But I don't think there is a cheap/reasonable way to fix this fully. So maybe you > just give up on this chunk and rely on context image to be updated on context > pin like in https://patchwork.freedesktop.org/patch/250007/. We will wait for this patch to get merged, because We see changes between two Patch for register access. We will analyse this further for MI_LOAD_REGISTER_IMM Regards, Ankit > > Regards, > > Tvrtko > > > + ctx->update_render_config = 0; > > + } > > > > /* True 32b PPGTT with dynamic page allocation: update PDP > > * registers and point the unallocated PDPs to scratch page. > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
