| Message ID | 20170309112724.5263-1-changbin.du@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 09, 2017 at 07:27:24PM +0800, changbin.du@intel.com wrote: > From: Changbin Du <changbin.du@intel.com> > > GVTg has introduced the context status notifier to schedule the GVTg > workload. At that time, the notifier is bound to GVTg context only, > so GVTg is not aware of host workloads. > > Now we are going to improve GVTg's guest workload scheduler policy, > and add Guc emulation support for new Gen graphics. Both these two > features require acknowledgment for all contexts running on hardware. > (But will not alter host workload.) So here try to make some change. > > The change is simple: > 1. Move the context status notifier head from i915_gem_context to > intel_engine_cs. Which means there is a notifier head per engine > instead of per context. Execlist driver still call notifier for > each context sched-in/out events of current engine. > 2. At GVTg side, it binds a notifier_block for each physical engine > at GVTg initialization period. Then GVTg can hear all context > status events. > > In this patch, GVTg do nothing for host context event, but later > will add a function there. But in any case, the notifier callback is > a noop if this is no active vGPU. > > Since intel_gvt_init() is called at early initialization stage and > require the status notifier head has been initiated, I initiate it in > intel_engine_setup(). > > Signed-off-by: Changbin Du <changbin.du@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> I presume you will apply this via gvt? > diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c > index d3a56c9..64875ec 100644 > --- a/drivers/gpu/drm/i915/gvt/scheduler.c > +++ b/drivers/gpu/drm/i915/gvt/scheduler.c > @@ -127,15 +127,14 @@ static int populate_shadow_context(struct intel_vgpu_workload *workload) > return 0; > } > > + Bonus newline > static int shadow_context_status_change(struct notifier_block *nb, > unsigned long action, void *data) > { > - struct intel_vgpu *vgpu = container_of(nb, > - struct intel_vgpu, shadow_ctx_notifier_block); > - struct drm_i915_gem_request *req = > - (struct drm_i915_gem_request *)data; > - struct intel_gvt_workload_scheduler *scheduler = > - &vgpu->gvt->scheduler; > + struct drm_i915_gem_request *req = (struct drm_i915_gem_request *)data; > + struct intel_gvt *gvt = container_of(nb, struct intel_gvt, > + shadow_ctx_notifier_block[req->engine->id]); > + struct intel_gvt_workload_scheduler *scheduler = &gvt->scheduler; > struct intel_vgpu_workload *workload = > scheduler->current_workload[req->engine->id]; >
hi, chris, On Fri, Mar 10, 2017 at 05:17:17PM +0000, Chris Wilson wrote: > On Thu, Mar 09, 2017 at 07:27:24PM +0800, changbin.du@intel.com wrote: > > From: Changbin Du <changbin.du@intel.com> > > > > GVTg has introduced the context status notifier to schedule the GVTg > > workload. At that time, the notifier is bound to GVTg context only, > > so GVTg is not aware of host workloads. > > > > Now we are going to improve GVTg's guest workload scheduler policy, > > and add Guc emulation support for new Gen graphics. Both these two > > features require acknowledgment for all contexts running on hardware. > > (But will not alter host workload.) So here try to make some change. > > > > The change is simple: > > 1. Move the context status notifier head from i915_gem_context to > > intel_engine_cs. Which means there is a notifier head per engine > > instead of per context. Execlist driver still call notifier for > > each context sched-in/out events of current engine. > > 2. At GVTg side, it binds a notifier_block for each physical engine > > at GVTg initialization period. Then GVTg can hear all context > > status events. > > > > In this patch, GVTg do nothing for host context event, but later > > will add a function there. But in any case, the notifier callback is > > a noop if this is no active vGPU. > > > > Since intel_gvt_init() is called at early initialization stage and > > require the status notifier head has been initiated, I initiate it in > > intel_engine_setup(). > > > > Signed-off-by: Changbin Du <changbin.du@intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > I presume you will apply this via gvt? > Sure, I'll sync with zhenyu about this. Then update patch with below 'bonus newline' fixed. Thanks. > > diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c > > index d3a56c9..64875ec 100644 > > --- a/drivers/gpu/drm/i915/gvt/scheduler.c > > +++ b/drivers/gpu/drm/i915/gvt/scheduler.c > > @@ -127,15 +127,14 @@ static int populate_shadow_context(struct intel_vgpu_workload *workload) > > return 0; > > } > > > > + > > Bonus newline > > > static int shadow_context_status_change(struct notifier_block *nb, > > unsigned long action, void *data) > > { > > - struct intel_vgpu *vgpu = container_of(nb, > > - struct intel_vgpu, shadow_ctx_notifier_block); > > - struct drm_i915_gem_request *req = > > - (struct drm_i915_gem_request *)data; > > - struct intel_gvt_workload_scheduler *scheduler = > > - &vgpu->gvt->scheduler; > > + struct drm_i915_gem_request *req = (struct drm_i915_gem_request *)data; > > + struct intel_gvt *gvt = container_of(nb, struct intel_gvt, > > + shadow_ctx_notifier_block[req->engine->id]); > > + struct intel_gvt_workload_scheduler *scheduler = &gvt->scheduler; > > struct intel_vgpu_workload *workload = > > scheduler->current_workload[req->engine->id]; > > > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h index 2379192..6dfc48b 100644 --- a/drivers/gpu/drm/i915/gvt/gvt.h +++ b/drivers/gpu/drm/i915/gvt/gvt.h @@ -162,7 +162,6 @@ struct intel_vgpu { atomic_t running_workload_num; DECLARE_BITMAP(tlb_handle_pending, I915_NUM_ENGINES); struct i915_gem_context *shadow_ctx; - struct notifier_block shadow_ctx_notifier_block; #if IS_ENABLED(CONFIG_DRM_I915_GVT_KVMGT) struct { @@ -233,6 +232,7 @@ struct intel_gvt { struct intel_gvt_gtt gtt; struct intel_gvt_opregion opregion; struct intel_gvt_workload_scheduler scheduler; + struct notifier_block shadow_ctx_notifier_block[I915_NUM_ENGINES]; DECLARE_HASHTABLE(cmd_table, GVT_CMD_HASH_BITS); struct intel_vgpu_type *types; unsigned int num_types; diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c index d3a56c9..64875ec 100644 --- a/drivers/gpu/drm/i915/gvt/scheduler.c +++ b/drivers/gpu/drm/i915/gvt/scheduler.c @@ -127,15 +127,14 @@ static int populate_shadow_context(struct intel_vgpu_workload *workload) return 0; } + static int shadow_context_status_change(struct notifier_block *nb, unsigned long action, void *data) { - struct intel_vgpu *vgpu = container_of(nb, - struct intel_vgpu, shadow_ctx_notifier_block); - struct drm_i915_gem_request *req = - (struct drm_i915_gem_request *)data; - struct intel_gvt_workload_scheduler *scheduler = - &vgpu->gvt->scheduler; + struct drm_i915_gem_request *req = (struct drm_i915_gem_request *)data; + struct intel_gvt *gvt = container_of(nb, struct intel_gvt, + shadow_ctx_notifier_block[req->engine->id]); + struct intel_gvt_workload_scheduler *scheduler = &gvt->scheduler; struct intel_vgpu_workload *workload = scheduler->current_workload[req->engine->id]; @@ -513,15 +512,16 @@ void intel_gvt_wait_vgpu_idle(struct intel_vgpu *vgpu) void intel_gvt_clean_workload_scheduler(struct intel_gvt *gvt) { struct intel_gvt_workload_scheduler *scheduler = &gvt->scheduler; - int i; + struct intel_engine_cs *engine; + enum intel_engine_id i; gvt_dbg_core("clean workload scheduler\n"); - for (i = 0; i < I915_NUM_ENGINES; i++) { - if (scheduler->thread[i]) { - kthread_stop(scheduler->thread[i]); - scheduler->thread[i] = NULL; - } + for_each_engine(engine, gvt->dev_priv, i) { + atomic_notifier_chain_unregister( + &engine->context_status_notifier, + &gvt->shadow_ctx_notifier_block[i]); + kthread_stop(scheduler->thread[i]); } } @@ -529,18 +529,15 @@ int intel_gvt_init_workload_scheduler(struct intel_gvt *gvt) { struct intel_gvt_workload_scheduler *scheduler = &gvt->scheduler; struct workload_thread_param *param = NULL; + struct intel_engine_cs *engine; + enum intel_engine_id i; int ret; - int i; gvt_dbg_core("init workload scheduler\n"); init_waitqueue_head(&scheduler->workload_complete_wq); - for (i = 0; i < I915_NUM_ENGINES; i++) { - /* check ring mask at init time */ - if (!HAS_ENGINE(gvt->dev_priv, i)) - continue; - + for_each_engine(engine, gvt->dev_priv, i) { init_waitqueue_head(&scheduler->waitq[i]); param = kzalloc(sizeof(*param), GFP_KERNEL); @@ -559,6 +556,11 @@ int intel_gvt_init_workload_scheduler(struct intel_gvt *gvt) ret = PTR_ERR(scheduler->thread[i]); goto err; } + + gvt->shadow_ctx_notifier_block[i].notifier_call = + shadow_context_status_change; + atomic_notifier_chain_register(&engine->context_status_notifier, + &gvt->shadow_ctx_notifier_block[i]); } return 0; err: @@ -570,9 +572,6 @@ int intel_gvt_init_workload_scheduler(struct intel_gvt *gvt) void intel_vgpu_clean_gvt_context(struct intel_vgpu *vgpu) { - atomic_notifier_chain_unregister(&vgpu->shadow_ctx->status_notifier, - &vgpu->shadow_ctx_notifier_block); - i915_gem_context_put_unlocked(vgpu->shadow_ctx); } @@ -587,10 +586,5 @@ int intel_vgpu_init_gvt_context(struct intel_vgpu *vgpu) vgpu->shadow_ctx->engine[RCS].initialised = true; - vgpu->shadow_ctx_notifier_block.notifier_call = - shadow_context_status_change; - - atomic_notifier_chain_register(&vgpu->shadow_ctx->status_notifier, - &vgpu->shadow_ctx_notifier_block); return 0; } diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index baceca1..0005085 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -318,7 +318,6 @@ __create_hw_context(struct drm_i915_private *dev_priv, ctx->ring_size = 4 * PAGE_SIZE; ctx->desc_template = default_desc_template(dev_priv, dev_priv->mm.aliasing_ppgtt); - ATOMIC_INIT_NOTIFIER_HEAD(&ctx->status_notifier); /* GuC requires the ring to be placed above GUC_WOPCM_TOP. If GuC is not * present or not in use we still need a small bias as ring wraparound diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h index 81268c9..4af2ab94 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.h +++ b/drivers/gpu/drm/i915/i915_gem_context.h @@ -158,9 +158,6 @@ struct i915_gem_context { /** desc_template: invariant fields for the HW context descriptor */ u32 desc_template; - /** status_notifier: list of callbacks for context-switch changes */ - struct atomic_notifier_head status_notifier; - /** guilty_count: How many times this context has caused a GPU hang. */ unsigned int guilty_count; /** diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 73fe718..695693a 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -105,6 +105,8 @@ intel_engine_setup(struct drm_i915_private *dev_priv, /* Nothing to do here, execute in order of dependencies */ engine->schedule = NULL; + ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier); + dev_priv->engine[id] = engine; return 0; } diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 89f38e7..808ad46 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -306,7 +306,8 @@ execlists_context_status_change(struct drm_i915_gem_request *rq, if (!IS_ENABLED(CONFIG_DRM_I915_GVT)) return; - atomic_notifier_call_chain(&rq->ctx->status_notifier, status, rq); + atomic_notifier_call_chain(&rq->engine->context_status_notifier, + status, rq); } static void diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 0ef491d..b7d42a5 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -408,6 +408,9 @@ struct intel_engine_cs { */ struct i915_gem_context *legacy_active_context; + /* status_notifier: list of callbacks for context-switch changes */ + struct atomic_notifier_head context_status_notifier; + struct intel_engine_hangcheck hangcheck; bool needs_cmd_parser;