[PATCH] drm/i915/gvt: implement per-vm mmio switching optimization
Dong, Chuanxiao
chuanxiao.dong at intel.com
Tue May 2 07:53:28 UTC 2017
> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On
> Behalf Of changbin.du at gmail.com
> Sent: Tuesday, May 2, 2017 12:52 PM
> To: intel-gvt-dev at lists.freedesktop.org
> Cc: Du, Changbin <changbin.du at intel.com>; Du at freedesktop.org
> Subject: [PATCH] drm/i915/gvt: implement per-vm mmio switching
> optimization
>
> From: Changbin Du <changbin.du at intel.com>
>
> Commit ab9da627906a ("drm/i915: make context status notifier head be per
> engine") gives us a chance to inspect every single request. Then we can
> eliminate unnecessary mmio switching for same vGPU. We only need mmio
> switching for different VMs (including host).
>
> This patch introduced a new general API intel_gvt_switch_mmio() to replace
> the old intel_gvt_load/restore_render_mmio(). This function save the last
> ring id internally which will be used for next switch.
>
> This optimization is very useful if only one guest has plenty of workloads and
> the host is mostly idle. The best case is no mmio switching will happen.
>
> Signed-off-by: Changbin Du <changbin.du at intel.com>
> Signed-off-by: Du, Changbin <changbin.du at intel.com>
> ---
> drivers/gpu/drm/i915/gvt/render.c | 39
> ++++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/i915/gvt/render.h | 4 ++--
> drivers/gpu/drm/i915/gvt/scheduler.c | 29 ++++++++++++++++++++-------
> drivers/gpu/drm/i915/gvt/scheduler.h | 2 ++
> 4 files changed, 63 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/render.c
> b/drivers/gpu/drm/i915/gvt/render.c
> index c6e7972..260ef83 100644
> --- a/drivers/gpu/drm/i915/gvt/render.c
> +++ b/drivers/gpu/drm/i915/gvt/render.c
> @@ -260,7 +260,7 @@ static void restore_mocs(struct intel_vgpu *vgpu, int
> ring_id)
>
> #define CTX_CONTEXT_CONTROL_VAL 0x03
>
> -void intel_gvt_load_render_mmio(struct intel_vgpu *vgpu, int ring_id)
> +static void switch_mmio_to_vgpu(struct intel_vgpu *vgpu, int ring_id)
As it is per-vm switching, is it really OK to only save/restore the mmio according to the specific ring instead of all the rings? As after switch to VGPU, other rings may also be used by VGPU.
> {
> struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> struct render_mmio *mmio;
> @@ -312,7 +312,7 @@ void intel_gvt_load_render_mmio(struct intel_vgpu
> *vgpu, int ring_id)
> handle_tlb_pending_event(vgpu, ring_id); }
>
> -void intel_gvt_restore_render_mmio(struct intel_vgpu *vgpu, int ring_id)
> +static void switch_mmio_to_host(struct intel_vgpu *vgpu, int ring_id)
The same concern as the above.
Thanks
Chuanxiao
> {
> struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> struct render_mmio *mmio;
> @@ -348,3 +348,38 @@ void intel_gvt_restore_render_mmio(struct
> intel_vgpu *vgpu, int ring_id)
> mmio->value, v);
> }
> }
> +
> +/**
> + * intel_gvt_switch_render_mmio - switch mmio context of specific
> +engine
> + * @pre: the last vGPU that own the engine
> + * @next: the vGPU to switch to
> + * @ring_id: specify the engine
> + *
> + * If pre is null indicates that host own the engine. If next is null
> + * indicates that we are switching to host workload.
> + */
> +void intel_gvt_switch_mmio(struct intel_vgpu *pre,
> + struct intel_vgpu *next, int ring_id) {
> +
> + static enum intel_engine_id last_ring = -1;
> +
> + if (WARN_ON((!pre && !next) || (pre && last_ring == -1)))
> + return;
> +
> + gvt_dbg_render("switch mmio from %s to %s, last_ring %d,
> cur_ring %d\n",
> + pre ? "vGPU" : "host", next ? "vGPU" : "HOST",
> + last_ring, ring_id);
> +
> + /**
> + * TODO: Optimize for vGPU to vGPU switch by merging
> + * switch_mmio_to_host() and switch_mmio_to_vgpu().
> + */
> + if (pre)
> + switch_mmio_to_host(pre, last_ring);
> +
> + if (next) {
> + switch_mmio_to_vgpu(next, ring_id);
> + last_ring = ring_id;
> + }
> +}
> diff --git a/drivers/gpu/drm/i915/gvt/render.h
> b/drivers/gpu/drm/i915/gvt/render.h
> index dac1a3c..91db1d3 100644
> --- a/drivers/gpu/drm/i915/gvt/render.h
> +++ b/drivers/gpu/drm/i915/gvt/render.h
> @@ -36,8 +36,8 @@
> #ifndef __GVT_RENDER_H__
> #define __GVT_RENDER_H__
>
> -void intel_gvt_load_render_mmio(struct intel_vgpu *vgpu, int ring_id);
> +void intel_gvt_switch_mmio(struct intel_vgpu *pre,
> + struct intel_vgpu *next, int ring_id);
>
> -void intel_gvt_restore_render_mmio(struct intel_vgpu *vgpu, int ring_id);
>
> #endif
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c
> b/drivers/gpu/drm/i915/gvt/scheduler.c
> index bada32b..1caabea 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -139,21 +139,36 @@ static int shadow_context_status_change(struct
> notifier_block *nb,
> 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];
> + struct intel_vgpu_workload *workload;
> +
> + if (!is_gvt_request(req)) {
> + if (action == INTEL_CONTEXT_SCHEDULE_IN && scheduler-
> >last_vgpu) {
> + /* Switch from vGPU to host. */
> + intel_gvt_switch_mmio(scheduler->last_vgpu, NULL,
> + req->engine->id);
> + }
> +
> + scheduler->last_vgpu = NULL;
> + return NOTIFY_OK;
> + }
>
> - if (!is_gvt_request(req) || unlikely(!workload))
> + workload = scheduler->current_workload[req->engine->id];
> + if (unlikely(!workload))
> return NOTIFY_OK;
>
> switch (action) {
> case INTEL_CONTEXT_SCHEDULE_IN:
> - intel_gvt_load_render_mmio(workload->vgpu,
> - workload->ring_id);
> + if (scheduler->last_vgpu != workload->vgpu) {
> + /* Switch from host to vGPU or from vGPU to vGPU.
> */
> + intel_gvt_switch_mmio(scheduler->last_vgpu,
> + workload->vgpu,
> + req->engine->id);
> + }
> +
> atomic_set(&workload->shadow_ctx_active, 1);
> + scheduler->last_vgpu = workload->vgpu;
> break;
> case INTEL_CONTEXT_SCHEDULE_OUT:
> - intel_gvt_restore_render_mmio(workload->vgpu,
> - workload->ring_id);
> /* If the status is -EINPROGRESS means this workload
> * doesn't meet any issue during dispatching so when
> * get the SCHEDULE_OUT set the status to be zero for diff --
> git a/drivers/gpu/drm/i915/gvt/scheduler.h
> b/drivers/gpu/drm/i915/gvt/scheduler.h
> index 2cd725c..8a06cd9 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.h
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.h
> @@ -38,6 +38,8 @@
>
> struct intel_gvt_workload_scheduler {
> struct intel_vgpu *current_vgpu;
> + /* last_vgpu can be null when running i915 context */
> + struct intel_vgpu *last_vgpu;
> struct intel_vgpu *next_vgpu;
> struct intel_vgpu_workload
> *current_workload[I915_NUM_ENGINES];
> bool need_reschedule;
> --
> 2.7.4
>
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
More information about the intel-gvt-dev
mailing list