[Intel-gfx] [PATCH 2/4] drm/i915: Update render power clock state configuration for given context
Navik, Ankit P
ankit.p.navik at intel.com
Tue Nov 6 04:23:10 UTC 2018
Hi Tvrtko,
> -----Original Message-----
> From: Tvrtko Ursulin [mailto:tvrtko.ursulin at linux.intel.com]
> Sent: Friday, September 21, 2018 6:22 PM
> To: J Karanje, Kedar <kedar.j.karanje at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: Diwakar, Praveen <praveen.diwakar at intel.com>; Marathe, Yogesh
> <yogesh.marathe at intel.com>; Navik, Ankit P <ankit.p.navik at intel.com>;
> Muthukumar, Aravindan <aravindan.muthukumar at intel.com>
> Subject: Re: [Intel-gfx] [PATCH 2/4] drm/i915: Update render power clock state
> configuration for given context
>
>
> On 21/09/2018 10:13, kedar.j.karanje at intel.com wrote:
> > From: Praveen Diwakar <praveen.diwakar at intel.com>
> >
> > 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 at intel.com>
> > Signed-off-by: Yogesh Marathe <yogesh.marathe at intel.com>
> > Signed-off-by: Aravindan Muthukumar <aravindan.muthukumar at intel.com>
> > Signed-off-by: Kedar J Karanje <kedar.j.karanje at intel.com>
> > Signed-off-by: Ankit Navik <ankit.p.navik at intel.com>
> > ---
> > 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.
> >
More information about the Intel-gfx
mailing list