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 > 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 <[email protected]> > > Cc: Joonas Lahtinen <[email protected]> > > Cc: Zhenyu Wang <[email protected]> > > Signed-off-by: Zhi Wang <[email protected]> > > --- > > 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 > -- $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
signature.asc
Description: PGP signature
_______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
