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

[Ankit Navik]

Hi Tvrtko,

On Tue, Dec 11, 2018 at 6:06 PM Tvrtko Ursulin <
tvrtko.ursulin at linux.intel.com> wrote:

>
> On 11/12/2018 10:14, Ankit Navik wrote:
> > From: Praveen Diwakar <praveen.diwakar at 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 at intel.com>
> > Cc: Kedar J Karanje <kedar.j.karanje at intel.com>
> > Cc: Yogesh Marathe <yogesh.marathe at intel.com>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>
> Again, I did not give an r-b for this!
>
> > Signed-off-by: Praveen Diwakar <praveen.diwakar at intel.com>
> > Signed-off-by: Ankit Navik <ankit.p.navik at 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.
>

submitted v4 patch and reverted to previous function
i.e., get_context_rpcs_config as make_rpcs is changed in mainline.

Regards, Ankit

>
> > +     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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20190314/8dc1a205/attachment-0001.html>