Re: [PATCH] drm/i915/gvt: Introduce per object locking in GVT scheduler.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2020-09-09 10:06, Zhenyu Wang wrote:
On 2020.09.09 09:43:21 +0800, Colin Xu wrote:
I tested this patch on the suspend/resume case with vGPU created (no need
really activate), can still observer the system freeze issue as mentioned in
another patch I sent. So I suppose we still need decouple context pin/unpin
with submission setup/clean, but move to workload create/destroy?

yeah, I observed that conflict too. How about merging Zhi's ww lock fix first
then next for left suspend/resume fixes? And better if you could pack all fixes
in one series.

Thanks
No problem at all. And I would like to hear more comments if moving the context pin/unpin to workload lifecycle is reasonable.

After made similar changes based on this one, plus the suspend/resume
support patch, below tests can pass:

- Create vGPU then suspend/resume.

- Run VM w/ vGPU then suspend/resume.

https://lists.freedesktop.org/archives/intel-gvt-dev/2020-September/007061.html

On 2020-09-08 04:02, Zhi Wang wrote:
To support ww locking and per-object implemented in i915, GVT scheduler needs
to be refined. Most of the changes are located in shadow batch buffer, shadow
wa context in GVT-g, where use quite a lot of i915 gem object APIs.

Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Cc: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>
Signed-off-by: Zhi Wang <zhi.a.wang@xxxxxxxxx>
---
   drivers/gpu/drm/i915/gvt/scheduler.c | 68 ++++++++++++++++++++++++++++++------
   1 file changed, 57 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
index 1570eb8..fe7ee10 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -396,7 +396,9 @@ static void release_shadow_wa_ctx(struct intel_shadow_wa_ctx *wa_ctx)
   	if (!wa_ctx->indirect_ctx.obj)
   		return;
+	i915_gem_object_lock(wa_ctx->indirect_ctx.obj, NULL);
   	i915_gem_object_unpin_map(wa_ctx->indirect_ctx.obj);
+	i915_gem_object_unlock(wa_ctx->indirect_ctx.obj);
   	i915_gem_object_put(wa_ctx->indirect_ctx.obj);
   	wa_ctx->indirect_ctx.obj = NULL;
@@ -504,6 +506,7 @@ static int prepare_shadow_batch_buffer(struct intel_vgpu_workload *workload)
   	struct intel_gvt *gvt = workload->vgpu->gvt;
   	const int gmadr_bytes = gvt->device_info.gmadr_bytes_in_cmd;
   	struct intel_vgpu_shadow_bb *bb;
+	struct i915_gem_ww_ctx ww;
   	int ret;
   	list_for_each_entry(bb, &workload->shadow_bb, list) {
@@ -528,10 +531,19 @@ static int prepare_shadow_batch_buffer(struct intel_vgpu_workload *workload)
   		 * directly
   		 */
   		if (!bb->ppgtt) {
-			bb->vma = i915_gem_object_ggtt_pin(bb->obj,
-							   NULL, 0, 0, 0);
+			i915_gem_ww_ctx_init(&ww, false);
+retry:
+			i915_gem_object_lock(bb->obj, &ww);
+
+			bb->vma = i915_gem_object_ggtt_pin_ww(bb->obj, &ww,
+							      NULL, 0, 0, 0);
   			if (IS_ERR(bb->vma)) {
   				ret = PTR_ERR(bb->vma);
+				if (ret == -EDEADLK) {
+					ret = i915_gem_ww_ctx_backoff(&ww);
+					if (!ret)
+						goto retry;
+				}
   				goto err;
   			}
@@ -545,13 +557,18 @@ static int prepare_shadow_batch_buffer(struct intel_vgpu_workload *workload)
   						      0);
   			if (ret)
   				goto err;
+
+			/* No one is going to touch shadow bb from now on. */
+			i915_gem_object_flush_map(bb->obj);
+
+			i915_gem_object_unlock(bb->obj);
+			i915_gem_ww_ctx_fini(&ww);
   		}
-		/* No one is going to touch shadow bb from now on. */
-		i915_gem_object_flush_map(bb->obj);
   	}
   	return 0;
   err:
+	i915_gem_ww_ctx_fini(&ww);
   	release_shadow_batch_buffer(workload);
   	return ret;
   }
@@ -578,14 +595,30 @@ static int prepare_shadow_wa_ctx(struct intel_shadow_wa_ctx *wa_ctx)
   	unsigned char *per_ctx_va =
   		(unsigned char *)wa_ctx->indirect_ctx.shadow_va +
   		wa_ctx->indirect_ctx.size;
+	struct i915_gem_ww_ctx ww;
+	int ret;
   	if (wa_ctx->indirect_ctx.size == 0)
   		return 0;
-	vma = i915_gem_object_ggtt_pin(wa_ctx->indirect_ctx.obj, NULL,
-				       0, CACHELINE_BYTES, 0);
-	if (IS_ERR(vma))
-		return PTR_ERR(vma);
+	i915_gem_ww_ctx_init(&ww, false);
+retry:
+	i915_gem_object_lock(wa_ctx->indirect_ctx.obj, &ww);
+
+	vma = i915_gem_object_ggtt_pin_ww(wa_ctx->indirect_ctx.obj, &ww, NULL,
+					  0, CACHELINE_BYTES, 0);
+	if (IS_ERR(vma)) {
+		ret = PTR_ERR(vma);
+		if (ret == -EDEADLK) {
+			ret = i915_gem_ww_ctx_backoff(&ww);
+			if (!ret)
+				goto retry;
+		}
+		return ret;
+	}
+
+	i915_gem_object_unlock(wa_ctx->indirect_ctx.obj);
+	i915_gem_ww_ctx_fini(&ww);
   	/* FIXME: we are not tracking our pinned VMA leaving it
   	 * up to the core to fix up the stray pin_count upon
@@ -619,12 +652,14 @@ static void release_shadow_batch_buffer(struct intel_vgpu_workload *workload)
   	list_for_each_entry_safe(bb, pos, &workload->shadow_bb, list) {
   		if (bb->obj) {
+			i915_gem_object_lock(bb->obj, NULL);
   			if (bb->va && !IS_ERR(bb->va))
   				i915_gem_object_unpin_map(bb->obj);
   			if (bb->vma && !IS_ERR(bb->vma))
   				i915_vma_unpin(bb->vma);
+			i915_gem_object_unlock(bb->obj);
   			i915_gem_object_put(bb->obj);
   		}
   		list_del(&bb->list);
@@ -1337,6 +1372,7 @@ int intel_vgpu_setup_submission(struct intel_vgpu *vgpu)
   	struct intel_vgpu_submission *s = &vgpu->submission;
   	struct intel_engine_cs *engine;
   	struct i915_ppgtt *ppgtt;
+	struct i915_gem_ww_ctx ww;
   	enum intel_engine_id i;
   	int ret;
@@ -1368,11 +1404,20 @@ int intel_vgpu_setup_submission(struct intel_vgpu *vgpu)
   			ce->ring = __intel_context_ring_size(ring_size);
   		}
Cut here
+		i915_gem_ww_ctx_init(&ww, false);
+retry:
+		ret = intel_context_pin_ww(ce, &ww);
+		if (ret) {
+			if (ret == -EDEADLK) {
+				ret = i915_gem_ww_ctx_backoff(&ww);
+				if (!ret)
+					goto retry;
+			}
+			goto out_shadow_ctx;
+		}
I move the piece to create_workload. Similar to the change I made in my
patch sent.
-		ret = intel_context_pin(ce);
   		intel_context_put(ce);
-		if (ret)
-			goto out_shadow_ctx;
+		i915_gem_ww_ctx_fini(&ww);
   		s->shadow[i] = ce;
   	}
@@ -1400,6 +1445,7 @@ int intel_vgpu_setup_submission(struct intel_vgpu *vgpu)
   	return 0;
   out_shadow_ctx:
+	i915_gem_ww_ctx_fini(&ww);
   	i915_context_ppgtt_root_restore(s, ppgtt);
   	for_each_engine(engine, vgpu->gvt->gt, i) {
   		if (IS_ERR(s->shadow[i]))
--
Best Regards,
Colin Xu

--
Best Regards,
Colin Xu

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux