[PATCH] drm/i915/gvt: refine vm engine reset
Li, Weinan Z
weinan.z.li at intel.com
Thu Jan 25 01:14:15 UTC 2018
-----Original Message-----
From: Zhenyu Wang [mailto:zhenyuw at linux.intel.com]
Sent: Wednesday, January 24, 2018 5:00 PM
To: Li, Weinan Z <weinan.z.li at intel.com>
Cc: intel-gvt-dev at lists.freedesktop.org; Gao, Fred <fred.gao at intel.com>; Wang, Zhi A <zhi.a.wang at intel.com>
Subject: Re: [PATCH] drm/i915/gvt: refine vm engine reset
On 2018.01.19 10:13:15 +0800, Weinan Li wrote:
> Use per engine vgpu execlist reset/init instead of per vgpu, ensure to
> only reset vgpu execlist state of the exact engine which gets reset
> request from vm. Otherwise if there is engine reset from vm, GVT-g
> will reset all
Let's write VM in capital for virtual machine.
Sure will follow the rule.
> engines execlist state. If virtual HWSP also be enabled at this time,
> vm will keep the CSB read pointer but does not read from vGPU, then vm
> will get wrong CSB state.
>
> Signed-off-by: Weinan Li <weinan.z.li at intel.com>
> Cc: Fred Gao <fred.gao at intel.com>
> Cc: Zhi Wang <zhi.a.wang at intel.com>
> ---
> drivers/gpu/drm/i915/gvt/execlist.c | 24 ++++++++++++-----------
> drivers/gpu/drm/i915/gvt/gvt.h | 5 +++--
> drivers/gpu/drm/i915/gvt/handlers.c | 10 ++++------
> drivers/gpu/drm/i915/gvt/sched_policy.c | 3 ++-
> drivers/gpu/drm/i915/gvt/scheduler.c | 34 ++++++++++++++++-----------------
> drivers/gpu/drm/i915/gvt/scheduler.h | 1 +
> drivers/gpu/drm/i915/gvt/vgpu.c | 3 +--
> 7 files changed, 41 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/execlist.c
> b/drivers/gpu/drm/i915/gvt/execlist.c
> index 769c1c2..615bc6b 100644
> --- a/drivers/gpu/drm/i915/gvt/execlist.c
> +++ b/drivers/gpu/drm/i915/gvt/execlist.c
> @@ -521,25 +521,26 @@ static void init_vgpu_execlist(struct intel_vgpu
> *vgpu, int ring_id)
>
> ctx_status_ptr_reg = execlist_ring_mmio(vgpu->gvt, ring_id,
> _EL_OFFSET_STATUS_PTR);
> -
> ctx_status_ptr.dw = vgpu_vreg(vgpu, ctx_status_ptr_reg);
> ctx_status_ptr.read_ptr = 0;
> ctx_status_ptr.write_ptr = 0x7;
> vgpu_vreg(vgpu, ctx_status_ptr_reg) = ctx_status_ptr.dw;
> + gvt_dbg_el("init vgpu execlist\n");
This doesn't bring helpful debug info without e.g target init ring info?
Remove it is fine, we can get similar information indirectly in ring_mode_write.
> }
>
> -static void clean_execlist(struct intel_vgpu *vgpu)
> +static void clean_execlist(struct intel_vgpu *vgpu, unsigned long
> +engine_mask)
> {
> - enum intel_engine_id i;
> + unsigned int tmp;
> + struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> struct intel_engine_cs *engine;
> + struct intel_vgpu_submission *s = &vgpu->submission;
>
> - for_each_engine(engine, vgpu->gvt->dev_priv, i) {
> - struct intel_vgpu_submission *s = &vgpu->submission;
> -
> - kfree(s->ring_scan_buffer[i]);
> - s->ring_scan_buffer[i] = NULL;
> - s->ring_scan_buffer_size[i] = 0;
> + for_each_engine_masked(engine, dev_priv, engine_mask, tmp) {
> + kfree(s->ring_scan_buffer[engine->id]);
> + s->ring_scan_buffer[engine->id] = NULL;
> + s->ring_scan_buffer_size[engine->id] = 0;
> }
> + gvt_dbg_el("clean execlist\n");
ditto
> }
>
> static void reset_execlist(struct intel_vgpu *vgpu, @@ -553,9 +554,10
> @@ static void reset_execlist(struct intel_vgpu *vgpu,
> init_vgpu_execlist(vgpu, engine->id); }
>
> -static int init_execlist(struct intel_vgpu *vgpu)
> +static int init_execlist(struct intel_vgpu *vgpu,
> + unsigned long engine_mask)
> {
> - reset_execlist(vgpu, ALL_ENGINES);
> + reset_execlist(vgpu, engine_mask);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h
> b/drivers/gpu/drm/i915/gvt/gvt.h index 7dc7a80..a6a4253 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -152,8 +152,8 @@ enum {
>
> struct intel_vgpu_submission_ops {
> const char *name;
> - int (*init)(struct intel_vgpu *vgpu);
> - void (*clean)(struct intel_vgpu *vgpu);
> + int (*init)(struct intel_vgpu *vgpu, unsigned long engine_mask);
> + void (*clean)(struct intel_vgpu *vgpu, unsigned long engine_mask);
> void (*reset)(struct intel_vgpu *vgpu, unsigned long engine_mask);
> };
>
> @@ -182,6 +182,7 @@ struct intel_vgpu {
> unsigned int resetting_eng;
> void *sched_data;
> struct vgpu_sched_ctl sched_ctl;
> + bool vgpu_sched_active;
>
> struct intel_vgpu_fence fence;
> struct intel_vgpu_gm gm;
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> b/drivers/gpu/drm/i915/gvt/handlers.c
> index 38f3b00..284efd2 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -1494,7 +1494,6 @@ static int elsp_mmio_write(struct intel_vgpu
> *vgpu, unsigned int offset, static int ring_mode_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
> void *p_data, unsigned int bytes)
> {
> - struct intel_vgpu_submission *s = &vgpu->submission;
> u32 data = *(u32 *)p_data;
> int ring_id = intel_gvt_render_mmio_to_ring_id(vgpu->gvt, offset);
> bool enable_execlist;
> @@ -1523,15 +1522,14 @@ static int ring_mode_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
> if (!enable_execlist)
> return 0;
>
> - if (s->active)
> - return 0;
> -
> ret = intel_vgpu_select_submission_ops(vgpu,
> - INTEL_VGPU_EXECLIST_SUBMISSION);
> + ENGINE_MASK(ring_id),
> + INTEL_VGPU_EXECLIST_SUBMISSION);
> if (ret)
> return ret;
>
> - intel_vgpu_start_schedule(vgpu);
> + if (!vgpu->vgpu_sched_active)
> + intel_vgpu_start_schedule(vgpu);
I think it's better to wrap the state track inside of scheduler itself instead of tracking out of sched data. And looks this change is not related to this fix, that can be split.
move 'active' field into vgpu_sched_data, and add one interface to get active state. This change also relate to this fix, but I can split this patch as prefix + fix patches.
> }
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/gvt/sched_policy.c
> b/drivers/gpu/drm/i915/gvt/sched_policy.c
> index eea1a2f..b1bc5f7 100644
> --- a/drivers/gpu/drm/i915/gvt/sched_policy.c
> +++ b/drivers/gpu/drm/i915/gvt/sched_policy.c
> @@ -325,6 +325,7 @@ static void tbs_sched_start_schedule(struct intel_vgpu *vgpu)
> if (!hrtimer_active(&sched_data->timer))
> hrtimer_start(&sched_data->timer, ktime_add_ns(ktime_get(),
> sched_data->period), HRTIMER_MODE_ABS);
> + vgpu->vgpu_sched_active = true;
> }
>
> static void tbs_sched_stop_schedule(struct intel_vgpu *vgpu) @@
> -332,6 +333,7 @@ static void tbs_sched_stop_schedule(struct intel_vgpu *vgpu)
> struct vgpu_sched_data *vgpu_data = vgpu->sched_data;
>
> list_del_init(&vgpu_data->lru_list);
> + vgpu->vgpu_sched_active = false;
> }
>
> static struct intel_gvt_sched_policy_ops tbs_schedule_ops = { @@
> -368,7 +370,6 @@ void intel_vgpu_clean_sched_policy(struct intel_vgpu
> *vgpu) void intel_vgpu_start_schedule(struct intel_vgpu *vgpu) {
> gvt_dbg_core("vgpu%d: start schedule\n", vgpu->id);
> -
> vgpu->gvt->scheduler.sched_ops->start_schedule(vgpu);
> }
>
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c
> b/drivers/gpu/drm/i915/gvt/scheduler.c
> index 0056638..d3b7adc 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -991,7 +991,7 @@ void intel_vgpu_clean_submission(struct intel_vgpu
> *vgpu) {
> struct intel_vgpu_submission *s = &vgpu->submission;
>
> - intel_vgpu_select_submission_ops(vgpu, 0);
> + intel_vgpu_select_submission_ops(vgpu, ALL_ENGINES, 0);
> i915_gem_context_put(s->shadow_ctx);
> kmem_cache_destroy(s->workloads);
> }
> @@ -1009,7 +1009,6 @@ void intel_vgpu_reset_submission(struct intel_vgpu *vgpu,
> unsigned long engine_mask)
> {
> struct intel_vgpu_submission *s = &vgpu->submission;
> -
> if (!s->active)
> return;
>
> @@ -1079,6 +1078,7 @@ int intel_vgpu_setup_submission(struct intel_vgpu *vgpu)
> *
> */
> int intel_vgpu_select_submission_ops(struct intel_vgpu *vgpu,
> + unsigned long engine_mask,
> unsigned int interface)
> {
> struct intel_vgpu_submission *s = &vgpu->submission; @@ -1091,31
> +1091,31 @@ int intel_vgpu_select_submission_ops(struct intel_vgpu *vgpu,
> if (WARN_ON(interface >= ARRAY_SIZE(ops)))
> return -EINVAL;
>
> - if (s->active) {
> - s->ops->clean(vgpu);
> - s->active = false;
> - gvt_dbg_core("vgpu%d: de-select ops [ %s ] \n",
> - vgpu->id, s->ops->name);
> - }
> + if (WARN_ON(interface == 0 && engine_mask != ALL_ENGINES))
> + return -EINVAL;
> +
> + if (s->active)
> + s->ops->clean(vgpu, engine_mask);
>
> if (interface == 0) {
> s->ops = NULL;
> s->virtual_submission_interface = 0;
> - gvt_dbg_core("vgpu%d: no submission ops\n", vgpu->id);
> + s->active = false;
> + gvt_dbg_core("vgpu%d: remove submission ops\n", vgpu->id);
> return 0;
> }
>
> - ret = ops[interface]->init(vgpu);
> + ret = ops[interface]->init(vgpu, engine_mask);
> if (ret)
> return ret;
>
> - s->ops = ops[interface];
> - s->virtual_submission_interface = interface;
> - s->active = true;
> -
> - gvt_dbg_core("vgpu%d: activate ops [ %s ]\n",
> - vgpu->id, s->ops->name);
> -
> + if (!s->active) {
> + s->ops = ops[interface];
> + s->virtual_submission_interface = interface;
> + s->active = true;
> + gvt_dbg_core("vgpu%d: activate ops [ %s ]\n",
> + vgpu->id, s->ops->name);
> + }
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.h
> b/drivers/gpu/drm/i915/gvt/scheduler.h
> index 3de77df..ff175a9 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.h
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.h
> @@ -141,6 +141,7 @@ void intel_vgpu_reset_submission(struct intel_vgpu
> *vgpu, void intel_vgpu_clean_submission(struct intel_vgpu *vgpu);
>
> int intel_vgpu_select_submission_ops(struct intel_vgpu *vgpu,
> + unsigned long engine_mask,
> unsigned int interface);
>
> extern const struct intel_vgpu_submission_ops diff --git
> a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
> index 4688619..6f09a1d 100644
> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> @@ -518,8 +518,7 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
> intel_vgpu_reset_submission(vgpu, resetting_eng);
> /* full GPU reset or device model level reset */
> if (engine_mask == ALL_ENGINES || dmlr) {
> - intel_vgpu_select_submission_ops(vgpu, 0);
> -
> + intel_vgpu_select_submission_ops(vgpu, resetting_eng, 0);
Can be just ALL_ENGINES
Here resetting_eng is equal with ALL_ENGINES, and ALL_ENGINES will be better.
> /*fence will not be reset during virtual reset */
> if (dmlr) {
> intel_vgpu_reset_gtt(vgpu);
> --
> 1.9.1
>
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
More information about the intel-gvt-dev
mailing list