[PATCH 2/3 - small lock v2] drm/i915/gvt: use per vgpu lock
Zhenyu Wang
zhenyuw at linux.intel.com
Tue Jan 30 08:49:05 UTC 2018
On 2018.01.25 16:25:21 +0800, pei.zhang at intel.com wrote:
> From: Pei Zhang <pei.zhang at intel.com>
>
> Use vgpu lock to replace the gvt big lock. By doing this, the
> mmio read/write trap path, vgpu virtual event emulation and other
> vgpu related process, would be protected under per vgpu lock.
>
> v2: add gvt->lock in function gvt_check_vblank_emulation
>
> Signed-off-by: Pei Zhang <pei.zhang at intel.com>
> ---
> drivers/gpu/drm/i915/gvt/display.c | 35 +++++++++++-----------
> drivers/gpu/drm/i915/gvt/gvt.c | 5 +---
> drivers/gpu/drm/i915/gvt/gvt.h | 9 ++++++
> drivers/gpu/drm/i915/gvt/handlers.c | 2 ++
> drivers/gpu/drm/i915/gvt/mmio.c | 12 ++++----
> drivers/gpu/drm/i915/gvt/scheduler.c | 13 ++++++---
> drivers/gpu/drm/i915/gvt/vgpu.c | 56 ++++++++++++++++--------------------
> 7 files changed, 71 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c
> index dd96ffc..1ff88c1 100644
> --- a/drivers/gpu/drm/i915/gvt/display.c
> +++ b/drivers/gpu/drm/i915/gvt/display.c
> @@ -327,26 +327,28 @@ void intel_gvt_check_vblank_emulation(struct intel_gvt *gvt)
> struct intel_gvt_irq *irq = &gvt->irq;
> struct intel_vgpu *vgpu;
> int pipe, id;
> + int found = false;
>
> - if (WARN_ON(!mutex_is_locked(&gvt->lock)))
> - return;
> -
> + mutex_lock(&gvt->lock);
> for_each_active_vgpu(gvt, vgpu, id) {
> for (pipe = 0; pipe < I915_MAX_PIPES; pipe++) {
> - if (pipe_is_enabled(vgpu, pipe))
> - goto out;
> + if (pipe_is_enabled(vgpu, pipe)) {
> + found = true;
> + break;
> + }
The problem is you would walk all vgpu and check other vgpu's state
without taking vgpu_lock which could be possibly unconsistent, as below
mmio path lock would be per-vgpu only.
So for below pipeconf mmio handler lock change, for pipe enable, just
start vbl timer. For pipe deactive, might send request for service
thread to check any active vbl timer required instead.
> }
> + if (found)
> + break;
> }
>
> /* all the pipes are disabled */
> - hrtimer_cancel(&irq->vblank_timer.timer);
> - return;
> -
> -out:
> - hrtimer_start(&irq->vblank_timer.timer,
> - ktime_add_ns(ktime_get(), irq->vblank_timer.period),
> - HRTIMER_MODE_ABS);
> -
> + if (!found)
> + hrtimer_cancel(&irq->vblank_timer.timer);
> + else
> + hrtimer_start(&irq->vblank_timer.timer,
> + ktime_add_ns(ktime_get(), irq->vblank_timer.period),
> + HRTIMER_MODE_ABS);
> + mutex_unlock(&gvt->lock);
> }
>
> static void emulate_vblank_on_pipe(struct intel_vgpu *vgpu, int pipe)
> @@ -383,8 +385,10 @@ static void emulate_vblank(struct intel_vgpu *vgpu)
> {
> int pipe;
>
> + mutex_lock(&vgpu->vgpu_lock);
> for_each_pipe(vgpu->gvt->dev_priv, pipe)
> emulate_vblank_on_pipe(vgpu, pipe);
> + mutex_unlock(&vgpu->vgpu_lock);
> }
>
> /**
> @@ -399,11 +403,10 @@ void intel_gvt_emulate_vblank(struct intel_gvt *gvt)
> struct intel_vgpu *vgpu;
> int id;
>
> - if (WARN_ON(!mutex_is_locked(&gvt->lock)))
> - return;
> -
> + mutex_lock(&gvt->lock);
> for_each_active_vgpu(gvt, vgpu, id)
> emulate_vblank(vgpu);
> + mutex_unlock(&gvt->lock);
> }
>
> /**
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> index 6cd4ca1..d8d5e6c 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.c
> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> @@ -271,11 +271,8 @@ static int gvt_service_thread(void *data)
> continue;
>
> if (test_and_clear_bit(INTEL_GVT_REQUEST_EMULATE_VBLANK,
> - (void *)&gvt->service_request)) {
> - mutex_lock(&gvt->lock);
> + (void *)&gvt->service_request))
> intel_gvt_emulate_vblank(gvt);
> - mutex_unlock(&gvt->lock);
> - }
>
> if (test_bit(INTEL_GVT_REQUEST_SCHED,
> (void *)&gvt->service_request) ||
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index 793224c..76f6c24 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -174,6 +174,7 @@ struct intel_vgpu_submission {
>
> struct intel_vgpu {
> struct intel_gvt *gvt;
> + struct mutex vgpu_lock;
> int id;
> unsigned long handle; /* vGPU handle used by hypervisor MPT modules */
> bool active;
> @@ -181,6 +182,7 @@ struct intel_vgpu {
> bool failsafe;
> unsigned int resetting_eng;
> void *sched_data;
> +
> struct vgpu_sched_ctl sched_ctl;
>
> struct intel_vgpu_fence fence;
> @@ -288,6 +290,9 @@ struct intel_vgpu_type {
> };
>
> struct intel_gvt {
> + /* GVT scope lock, protect GVT itself, and all resource currently
> + * not yet protected by special locks(vgpu and scheduler lock).
> + */
> struct mutex lock;
> struct mutex gtt_lock;
>
> @@ -310,6 +315,10 @@ struct intel_gvt {
>
> struct task_struct *service_thread;
> wait_queue_head_t service_thread_wq;
> +
> + /* service_request is always used in bit operation, we should always
> + * use it with atomic bit ops so that no need to use gvt big lock.
> + */
> unsigned long service_request;
>
> struct engine_mmio *engine_mmio_list;
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
> index 38f3b00..4244a81 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -421,7 +421,9 @@ static int pipeconf_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
> vgpu_vreg(vgpu, offset) |= I965_PIPECONF_ACTIVE;
> else
> vgpu_vreg(vgpu, offset) &= ~I965_PIPECONF_ACTIVE;
> + mutex_unlock(&vgpu->vgpu_lock);
> intel_gvt_check_vblank_emulation(vgpu->gvt);
> + mutex_lock(&vgpu->vgpu_lock);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/i915/gvt/mmio.c b/drivers/gpu/drm/i915/gvt/mmio.c
> index 562b5ad..8161ab9 100644
> --- a/drivers/gpu/drm/i915/gvt/mmio.c
> +++ b/drivers/gpu/drm/i915/gvt/mmio.c
> @@ -99,7 +99,7 @@ static void failsafe_emulate_mmio_rw(struct intel_vgpu *vgpu, uint64_t pa,
> return;
>
> gvt = vgpu->gvt;
> - mutex_lock(&gvt->lock);
> + mutex_lock(&vgpu->vgpu_lock);
> offset = intel_vgpu_gpa_to_mmio_offset(vgpu, pa);
> if (reg_is_mmio(gvt, offset)) {
> if (read)
> @@ -118,7 +118,7 @@ static void failsafe_emulate_mmio_rw(struct intel_vgpu *vgpu, uint64_t pa,
> memcpy(pt, p_data, bytes);
>
> }
> - mutex_unlock(&gvt->lock);
> + mutex_unlock(&vgpu->vgpu_lock);
> }
>
> /**
> @@ -142,7 +142,7 @@ int intel_vgpu_emulate_mmio_read(struct intel_vgpu *vgpu, uint64_t pa,
> failsafe_emulate_mmio_rw(vgpu, pa, p_data, bytes, true);
> return 0;
> }
> - mutex_lock(&gvt->lock);
> + mutex_lock(&vgpu->vgpu_lock);
>
> if (vgpu_gpa_is_aperture(vgpu, pa)) {
> ret = vgpu_aperture_rw(vgpu, pa, p_data, bytes, true);
> @@ -194,7 +194,7 @@ int intel_vgpu_emulate_mmio_read(struct intel_vgpu *vgpu, uint64_t pa,
> gvt_vgpu_err("fail to emulate MMIO read %08x len %d\n",
> offset, bytes);
> out:
> - mutex_unlock(&gvt->lock);
> + mutex_unlock(&vgpu->vgpu_lock);
> return ret;
> }
>
> @@ -220,7 +220,7 @@ int intel_vgpu_emulate_mmio_write(struct intel_vgpu *vgpu, uint64_t pa,
> return 0;
> }
>
> - mutex_lock(&gvt->lock);
> + mutex_lock(&vgpu->vgpu_lock);
>
> if (vgpu_gpa_is_aperture(vgpu, pa)) {
> ret = vgpu_aperture_rw(vgpu, pa, p_data, bytes, false);
> @@ -263,7 +263,7 @@ int intel_vgpu_emulate_mmio_write(struct intel_vgpu *vgpu, uint64_t pa,
> gvt_vgpu_err("fail to emulate MMIO write %08x len %d\n", offset,
> bytes);
> out:
> - mutex_unlock(&gvt->lock);
> + mutex_unlock(&vgpu->vgpu_lock);
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
> index 0056638..907195a 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -563,6 +563,11 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
> gvt_dbg_sched("ring id %d prepare to dispatch workload %p\n",
> ring_id, workload);
>
> + /* TODO.
> + * Use vgpu_lock here is still big grain, cause lock conflict
> + * with the mmio trap path. Split to small parts later.
> + */
> + mutex_lock(&vgpu->vgpu_lock);
> mutex_lock(&dev_priv->drm.struct_mutex);
>
> ret = intel_gvt_scan_and_shadow_workload(workload);
> @@ -587,6 +592,7 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
> }
>
> mutex_unlock(&dev_priv->drm.struct_mutex);
> + mutex_unlock(&vgpu->vgpu_lock);
> return ret;
> }
>
> @@ -744,14 +750,14 @@ static void complete_current_workload(struct intel_gvt *gvt, int ring_id)
> int event;
>
> mutex_lock(&gvt->lock);
> + mutex_lock(&vgpu->vgpu_lock);
>
> /* For the workload w/ request, needs to wait for the context
> * switch to make sure request is completed.
> * For the workload w/o request, directly complete the workload.
> */
> if (workload->req) {
> - struct drm_i915_private *dev_priv =
> - workload->vgpu->gvt->dev_priv;
> + struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> struct intel_engine_cs *engine =
> dev_priv->engine[workload->ring_id];
> wait_event(workload->shadow_ctx_status_wq,
> @@ -822,6 +828,7 @@ static void complete_current_workload(struct intel_gvt *gvt, int ring_id)
> if (gvt->scheduler.need_reschedule)
> intel_gvt_request_service(gvt, INTEL_GVT_REQUEST_EVENT_SCHED);
>
> + mutex_unlock(&vgpu->vgpu_lock);
> mutex_unlock(&gvt->lock);
> }
>
> @@ -874,9 +881,7 @@ static int workload_thread(void *priv)
> intel_uncore_forcewake_get(gvt->dev_priv,
> FORCEWAKE_ALL);
>
> - mutex_lock(&gvt->lock);
> ret = dispatch_workload(workload);
> - mutex_unlock(&gvt->lock);
>
> if (ret) {
> vgpu = workload->vgpu;
> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
> index a8784fa..d8293f3 100644
> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> @@ -223,22 +223,17 @@ void intel_gvt_activate_vgpu(struct intel_vgpu *vgpu)
> */
> void intel_gvt_deactivate_vgpu(struct intel_vgpu *vgpu)
> {
> - struct intel_gvt *gvt = vgpu->gvt;
> -
> - mutex_lock(&gvt->lock);
> + mutex_lock(&vgpu->vgpu_lock);
>
> vgpu->active = false;
>
> - if (atomic_read(&vgpu->submission.running_workload_num)) {
> - mutex_unlock(&gvt->lock);
> + if (atomic_read(&vgpu->submission.running_workload_num))
> intel_gvt_wait_vgpu_idle(vgpu);
> - mutex_lock(&gvt->lock);
> - }
>
> intel_vgpu_stop_schedule(vgpu);
> intel_vgpu_dmabuf_cleanup(vgpu);
>
> - mutex_unlock(&gvt->lock);
> + mutex_unlock(&vgpu->vgpu_lock);
> }
>
> /**
> @@ -252,14 +247,10 @@ void intel_gvt_destroy_vgpu(struct intel_vgpu *vgpu)
> {
> struct intel_gvt *gvt = vgpu->gvt;
>
> - mutex_lock(&gvt->lock);
> -
> WARN(vgpu->active, "vGPU is still active!\n");
>
> + mutex_lock(&vgpu->vgpu_lock);
> intel_gvt_debugfs_remove_vgpu(vgpu);
> - idr_remove(&gvt->vgpu_idr, vgpu->id);
> - if (idr_is_empty(&gvt->vgpu_idr))
> - intel_gvt_clean_irq(gvt);
> intel_vgpu_clean_sched_policy(vgpu);
> intel_vgpu_clean_submission(vgpu);
> intel_vgpu_clean_display(vgpu);
> @@ -269,10 +260,16 @@ void intel_gvt_destroy_vgpu(struct intel_vgpu *vgpu)
> intel_vgpu_free_resource(vgpu);
> intel_vgpu_clean_mmio(vgpu);
> intel_vgpu_dmabuf_cleanup(vgpu);
> - vfree(vgpu);
> + mutex_unlock(&vgpu->vgpu_lock);
>
> + mutex_lock(&gvt->lock);
> + idr_remove(&gvt->vgpu_idr, vgpu->id);
> + if (idr_is_empty(&gvt->vgpu_idr))
> + intel_gvt_clean_irq(gvt);
> intel_gvt_update_vgpu_types(gvt);
> mutex_unlock(&gvt->lock);
> +
> + vfree(vgpu);
> }
>
> #define IDLE_VGPU_IDR 0
> @@ -298,6 +295,7 @@ struct intel_vgpu *intel_gvt_create_idle_vgpu(struct intel_gvt *gvt)
>
> vgpu->id = IDLE_VGPU_IDR;
> vgpu->gvt = gvt;
> + mutex_init(&vgpu->vgpu_lock);
>
> for (i = 0; i < I915_NUM_ENGINES; i++)
> INIT_LIST_HEAD(&vgpu->submission.workload_q_head[i]);
> @@ -324,7 +322,10 @@ struct intel_vgpu *intel_gvt_create_idle_vgpu(struct intel_gvt *gvt)
> */
> void intel_gvt_destroy_idle_vgpu(struct intel_vgpu *vgpu)
> {
> + mutex_lock(&vgpu->vgpu_lock);
> intel_vgpu_clean_sched_policy(vgpu);
> + mutex_unlock(&vgpu->vgpu_lock);
> +
> vfree(vgpu);
> }
>
> @@ -342,8 +343,6 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
> if (!vgpu)
> return ERR_PTR(-ENOMEM);
>
> - mutex_lock(&gvt->lock);
> -
> ret = idr_alloc(&gvt->vgpu_idr, vgpu, IDLE_VGPU_IDR + 1, GVT_MAX_VGPU,
> GFP_KERNEL);
> if (ret < 0)
> @@ -353,6 +352,7 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
> vgpu->handle = param->handle;
> vgpu->gvt = gvt;
> vgpu->sched_ctl.weight = param->weight;
> + mutex_init(&vgpu->vgpu_lock);
> INIT_LIST_HEAD(&vgpu->dmabuf_obj_list_head);
> idr_init(&vgpu->object_idr);
> intel_vgpu_init_cfg_space(vgpu, param->primary);
> @@ -399,8 +399,6 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
> if (ret)
> goto out_clean_sched_policy;
>
> - mutex_unlock(&gvt->lock);
> -
> return vgpu;
>
> out_clean_sched_policy:
> @@ -423,7 +421,6 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
> idr_remove(&gvt->vgpu_idr, vgpu->id);
> out_free_vgpu:
> vfree(vgpu);
> - mutex_unlock(&gvt->lock);
> return ERR_PTR(ret);
> }
>
> @@ -455,12 +452,12 @@ struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
> param.low_gm_sz = BYTES_TO_MB(param.low_gm_sz);
> param.high_gm_sz = BYTES_TO_MB(param.high_gm_sz);
>
> + mutex_lock(&gvt->lock);
> vgpu = __intel_gvt_create_vgpu(gvt, ¶m);
> - if (IS_ERR(vgpu))
> - return vgpu;
> -
> - /* calculate left instance change for types */
> - intel_gvt_update_vgpu_types(gvt);
> + if (!IS_ERR(vgpu))
> + /* calculate left instance change for types */
> + intel_gvt_update_vgpu_types(gvt);
> + mutex_unlock(&gvt->lock);
>
> return vgpu;
> }
> @@ -472,7 +469,7 @@ struct intel_vgpu *intel_gvt_create_vgpu(struct intel_gvt *gvt,
> * @engine_mask: engines to reset for GT reset
> *
> * This function is called when user wants to reset a virtual GPU through
> - * device model reset or GT reset. The caller should hold the gvt lock.
> + * device model reset or GT reset. The caller should hold the vgpu lock.
> *
> * vGPU Device Model Level Reset (DMLR) simulates the PCI level reset to reset
> * the whole vGPU to default state as when it is created. This vGPU function
> @@ -511,11 +508,8 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
> * The current_vgpu will set to NULL after stopping the
> * scheduler when the reset is triggered by current vgpu.
> */
> - if (scheduler->current_vgpu == NULL) {
> - mutex_unlock(&gvt->lock);
> + if (scheduler->current_vgpu == NULL)
> intel_gvt_wait_vgpu_idle(vgpu);
> - mutex_lock(&gvt->lock);
> - }
>
> intel_vgpu_reset_submission(vgpu, resetting_eng);
> /* full GPU reset or device model level reset */
> @@ -554,7 +548,7 @@ void intel_gvt_reset_vgpu_locked(struct intel_vgpu *vgpu, bool dmlr,
> */
> void intel_gvt_reset_vgpu(struct intel_vgpu *vgpu)
> {
> - mutex_lock(&vgpu->gvt->lock);
> + mutex_lock(&vgpu->vgpu_lock);
> intel_gvt_reset_vgpu_locked(vgpu, true, 0);
> - mutex_unlock(&vgpu->gvt->lock);
> + mutex_unlock(&vgpu->vgpu_lock);
> }
> --
> 2.7.4
>
> _______________________________________________
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20180130/41596004/attachment.sig>
More information about the intel-gvt-dev
mailing list