Re: [PATCH 02/10] drm/i915/gvt: Pin the per-engine GVT shadow contexts

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

 



Quoting Chris Wilson (2019-04-25 06:42:02)
> Our eventual goal is to rid request construction of struct_mutex, with
> the short term step of lifting the struct_mutex requirements into the
> higher levels (i.e. the caller must ensure that the context is already
> pinned into the GTT). In this patch, we pin GVT's shadow context upon
> allocation and so keep them pinned into the GGTT for as long as the
> virtual machine is alive, and so we can use the simpler request
> construction path safe in the knowledge that the hard work is already
> done.
> 
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>

Hi Zhenyu, could you check through this patch and make sure I haven't
broken gvt in the process?

The end result is that the gvt shadow context is always pinned into the
ggtt, avoids any eviction/shrinking, and so allows gvt to use the faster
paths for request allocation.

> ---
>  drivers/gpu/drm/i915/gvt/gvt.h          |   2 +-
>  drivers/gpu/drm/i915/gvt/kvmgt.c        |   2 +-
>  drivers/gpu/drm/i915/gvt/mmio_context.c |   3 +-
>  drivers/gpu/drm/i915/gvt/scheduler.c    | 137 ++++++++++++------------
>  4 files changed, 73 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index f5a328b5290a..b54f2bdc13a4 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -149,9 +149,9 @@ struct intel_vgpu_submission_ops {
>  struct intel_vgpu_submission {
>         struct intel_vgpu_execlist execlist[I915_NUM_ENGINES];
>         struct list_head workload_q_head[I915_NUM_ENGINES];
> +       struct intel_context *shadow[I915_NUM_ENGINES];
>         struct kmem_cache *workloads;
>         atomic_t running_workload_num;
> -       struct i915_gem_context *shadow_ctx;
>         union {
>                 u64 i915_context_pml4;
>                 u64 i915_context_pdps[GEN8_3LVL_PDPES];
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index a68addf95c23..144301b778df 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1576,7 +1576,7 @@ hw_id_show(struct device *dev, struct device_attribute *attr,
>                 struct intel_vgpu *vgpu = (struct intel_vgpu *)
>                         mdev_get_drvdata(mdev);
>                 return sprintf(buf, "%u\n",
> -                              vgpu->submission.shadow_ctx->hw_id);
> +                              vgpu->submission.shadow[0]->gem_context->hw_id);
>         }
>         return sprintf(buf, "\n");
>  }
> diff --git a/drivers/gpu/drm/i915/gvt/mmio_context.c b/drivers/gpu/drm/i915/gvt/mmio_context.c
> index e7e14c842be4..b8823495022b 100644
> --- a/drivers/gpu/drm/i915/gvt/mmio_context.c
> +++ b/drivers/gpu/drm/i915/gvt/mmio_context.c
> @@ -495,8 +495,7 @@ static void switch_mmio(struct intel_vgpu *pre,
>                          * itself.
>                          */
>                         if (mmio->in_context &&
> -                           !is_inhibit_context(intel_context_lookup(s->shadow_ctx,
> -                                                                    dev_priv->engine[ring_id])))
> +                           !is_inhibit_context(s->shadow[ring_id]))
>                                 continue;
>  
>                         if (mmio->mask)
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
> index 8998fa5ab198..40d9f549a0cd 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -36,6 +36,7 @@
>  #include <linux/kthread.h>
>  
>  #include "i915_drv.h"
> +#include "i915_gem_pm.h"
>  #include "gvt.h"
>  
>  #define RING_CTX_OFF(x) \
> @@ -277,18 +278,23 @@ static int shadow_context_status_change(struct notifier_block *nb,
>         return NOTIFY_OK;
>  }
>  
> -static void shadow_context_descriptor_update(struct intel_context *ce)
> +static void
> +shadow_context_descriptor_update(struct intel_context *ce,
> +                                struct intel_vgpu_workload *workload)
>  {
> -       u64 desc = 0;
> -
> -       desc = ce->lrc_desc;
> +       u64 desc = ce->lrc_desc;
>  
> -       /* Update bits 0-11 of the context descriptor which includes flags
> +       /*
> +        * Update bits 0-11 of the context descriptor which includes flags
>          * like GEN8_CTX_* cached in desc_template
>          */
>         desc &= U64_MAX << 12;
>         desc |= ce->gem_context->desc_template & ((1ULL << 12) - 1);
>  
> +       desc &= ~(0x3 << GEN8_CTX_ADDRESSING_MODE_SHIFT);
> +       desc |= workload->ctx_desc.addressing_mode <<
> +               GEN8_CTX_ADDRESSING_MODE_SHIFT;
> +
>         ce->lrc_desc = desc;
>  }
>  
> @@ -365,26 +371,22 @@ intel_gvt_workload_req_alloc(struct intel_vgpu_workload *workload)
>  {
>         struct intel_vgpu *vgpu = workload->vgpu;
>         struct intel_vgpu_submission *s = &vgpu->submission;
> -       struct i915_gem_context *shadow_ctx = s->shadow_ctx;
>         struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> -       struct intel_engine_cs *engine = dev_priv->engine[workload->ring_id];
>         struct i915_request *rq;
> -       int ret = 0;
>  
>         lockdep_assert_held(&dev_priv->drm.struct_mutex);
>  
>         if (workload->req)
> -               goto out;
> +               return 0;
>  
> -       rq = i915_request_alloc(engine, shadow_ctx);
> +       rq = i915_request_create(s->shadow[workload->ring_id]);
>         if (IS_ERR(rq)) {
>                 gvt_vgpu_err("fail to allocate gem request\n");
> -               ret = PTR_ERR(rq);
> -               goto out;
> +               return PTR_ERR(rq);
>         }
> +
>         workload->req = i915_request_get(rq);
> -out:
> -       return ret;
> +       return 0;
>  }
>  
>  /**
> @@ -399,10 +401,7 @@ int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload)
>  {
>         struct intel_vgpu *vgpu = workload->vgpu;
>         struct intel_vgpu_submission *s = &vgpu->submission;
> -       struct i915_gem_context *shadow_ctx = s->shadow_ctx;
>         struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> -       struct intel_engine_cs *engine = dev_priv->engine[workload->ring_id];
> -       struct intel_context *ce;
>         int ret;
>  
>         lockdep_assert_held(&dev_priv->drm.struct_mutex);
> @@ -410,29 +409,13 @@ int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload)
>         if (workload->shadow)
>                 return 0;
>  
> -       /* pin shadow context by gvt even the shadow context will be pinned
> -        * when i915 alloc request. That is because gvt will update the guest
> -        * context from shadow context when workload is completed, and at that
> -        * moment, i915 may already unpined the shadow context to make the
> -        * shadow_ctx pages invalid. So gvt need to pin itself. After update
> -        * the guest context, gvt can unpin the shadow_ctx safely.
> -        */
> -       ce = intel_context_pin(shadow_ctx, engine);
> -       if (IS_ERR(ce)) {
> -               gvt_vgpu_err("fail to pin shadow context\n");
> -               return PTR_ERR(ce);
> -       }
> -
> -       shadow_ctx->desc_template &= ~(0x3 << GEN8_CTX_ADDRESSING_MODE_SHIFT);
> -       shadow_ctx->desc_template |= workload->ctx_desc.addressing_mode <<
> -                                   GEN8_CTX_ADDRESSING_MODE_SHIFT;
> -
>         if (!test_and_set_bit(workload->ring_id, s->shadow_ctx_desc_updated))
> -               shadow_context_descriptor_update(ce);
> +               shadow_context_descriptor_update(s->shadow[workload->ring_id],
> +                                                workload);
>  
>         ret = intel_gvt_scan_and_shadow_ringbuffer(workload);
>         if (ret)
> -               goto err_unpin;
> +               return ret;
>  
>         if (workload->ring_id == RCS0 && workload->wa_ctx.indirect_ctx.size) {
>                 ret = intel_gvt_scan_and_shadow_wa_ctx(&workload->wa_ctx);
> @@ -444,8 +427,6 @@ int intel_gvt_scan_and_shadow_workload(struct intel_vgpu_workload *workload)
>         return 0;
>  err_shadow:
>         release_shadow_wa_ctx(&workload->wa_ctx);
> -err_unpin:
> -       intel_context_unpin(ce);
>         return ret;
>  }
>  
> @@ -672,7 +653,6 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
>         struct intel_vgpu *vgpu = workload->vgpu;
>         struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
>         struct intel_vgpu_submission *s = &vgpu->submission;
> -       struct i915_gem_context *shadow_ctx = s->shadow_ctx;
>         struct i915_request *rq;
>         int ring_id = workload->ring_id;
>         int ret;
> @@ -683,7 +663,8 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
>         mutex_lock(&vgpu->vgpu_lock);
>         mutex_lock(&dev_priv->drm.struct_mutex);
>  
> -       ret = set_context_ppgtt_from_shadow(workload, shadow_ctx);
> +       ret = set_context_ppgtt_from_shadow(workload,
> +                                           s->shadow[ring_id]->gem_context);
>         if (ret < 0) {
>                 gvt_vgpu_err("workload shadow ppgtt isn't ready\n");
>                 goto err_req;
> @@ -994,8 +975,6 @@ static int workload_thread(void *priv)
>                                 workload->ring_id, workload,
>                                 workload->vgpu->id);
>  
> -               intel_runtime_pm_get(gvt->dev_priv);
> -
>                 gvt_dbg_sched("ring id %d will dispatch workload %p\n",
>                                 workload->ring_id, workload);
>  
> @@ -1025,7 +1004,6 @@ static int workload_thread(void *priv)
>                         intel_uncore_forcewake_put(&gvt->dev_priv->uncore,
>                                         FORCEWAKE_ALL);
>  
> -               intel_runtime_pm_put_unchecked(gvt->dev_priv);
>                 if (ret && (vgpu_is_vm_unhealthy(ret)))
>                         enter_failsafe_mode(vgpu, GVT_FAILSAFE_GUEST_ERR);
>         }
> @@ -1108,17 +1086,17 @@ int intel_gvt_init_workload_scheduler(struct intel_gvt *gvt)
>  }
>  
>  static void
> -i915_context_ppgtt_root_restore(struct intel_vgpu_submission *s)
> +i915_context_ppgtt_root_restore(struct intel_vgpu_submission *s,
> +                               struct i915_hw_ppgtt *ppgtt)
>  {
> -       struct i915_hw_ppgtt *i915_ppgtt = s->shadow_ctx->ppgtt;
>         int i;
>  
> -       if (i915_vm_is_4lvl(&i915_ppgtt->vm)) {
> -               px_dma(&i915_ppgtt->pml4) = s->i915_context_pml4;
> +       if (i915_vm_is_4lvl(&ppgtt->vm)) {
> +               px_dma(&ppgtt->pml4) = s->i915_context_pml4;
>         } else {
>                 for (i = 0; i < GEN8_3LVL_PDPES; i++)
> -                       px_dma(i915_ppgtt->pdp.page_directory[i]) =
> -                                               s->i915_context_pdps[i];
> +                       px_dma(ppgtt->pdp.page_directory[i]) =
> +                               s->i915_context_pdps[i];
>         }
>  }
>  
> @@ -1132,10 +1110,15 @@ i915_context_ppgtt_root_restore(struct intel_vgpu_submission *s)
>  void intel_vgpu_clean_submission(struct intel_vgpu *vgpu)
>  {
>         struct intel_vgpu_submission *s = &vgpu->submission;
> +       struct intel_engine_cs *engine;
> +       enum intel_engine_id id;
>  
>         intel_vgpu_select_submission_ops(vgpu, ALL_ENGINES, 0);
> -       i915_context_ppgtt_root_restore(s);
> -       i915_gem_context_put(s->shadow_ctx);
> +
> +       i915_context_ppgtt_root_restore(s, s->shadow[0]->gem_context->ppgtt);
> +       for_each_engine(engine, vgpu->gvt->dev_priv, id)
> +               intel_context_unpin(s->shadow[id]);
> +
>         kmem_cache_destroy(s->workloads);
>  }
>  
> @@ -1161,17 +1144,17 @@ void intel_vgpu_reset_submission(struct intel_vgpu *vgpu,
>  }
>  
>  static void
> -i915_context_ppgtt_root_save(struct intel_vgpu_submission *s)
> +i915_context_ppgtt_root_save(struct intel_vgpu_submission *s,
> +                            struct i915_hw_ppgtt *ppgtt)
>  {
> -       struct i915_hw_ppgtt *i915_ppgtt = s->shadow_ctx->ppgtt;
>         int i;
>  
> -       if (i915_vm_is_4lvl(&i915_ppgtt->vm))
> -               s->i915_context_pml4 = px_dma(&i915_ppgtt->pml4);
> -       else {
> +       if (i915_vm_is_4lvl(&ppgtt->vm)) {
> +               s->i915_context_pml4 = px_dma(&ppgtt->pml4);
> +       } else {
>                 for (i = 0; i < GEN8_3LVL_PDPES; i++)
>                         s->i915_context_pdps[i] =
> -                               px_dma(i915_ppgtt->pdp.page_directory[i]);
> +                               px_dma(ppgtt->pdp.page_directory[i]);
>         }
>  }
>  
> @@ -1188,16 +1171,31 @@ i915_context_ppgtt_root_save(struct intel_vgpu_submission *s)
>  int intel_vgpu_setup_submission(struct intel_vgpu *vgpu)
>  {
>         struct intel_vgpu_submission *s = &vgpu->submission;
> -       enum intel_engine_id i;
>         struct intel_engine_cs *engine;
> +       struct i915_gem_context *ctx;
> +       enum intel_engine_id i;
>         int ret;
>  
> -       s->shadow_ctx = i915_gem_context_create_gvt(
> -                       &vgpu->gvt->dev_priv->drm);
> -       if (IS_ERR(s->shadow_ctx))
> -               return PTR_ERR(s->shadow_ctx);
> +       ctx = i915_gem_context_create_gvt(&vgpu->gvt->dev_priv->drm);
> +       if (IS_ERR(ctx))
> +               return PTR_ERR(ctx);
> +
> +       i915_context_ppgtt_root_save(s, ctx->ppgtt);
> +
> +       for_each_engine(engine, vgpu->gvt->dev_priv, i) {
> +               struct intel_context *ce;
> +
> +               INIT_LIST_HEAD(&s->workload_q_head[i]);
> +               s->shadow[i] = ERR_PTR(-EINVAL);
>  
> -       i915_context_ppgtt_root_save(s);
> +               ce = intel_context_pin(ctx, engine);
> +               if (IS_ERR(ce)) {
> +                       ret = PTR_ERR(ce);
> +                       goto out_shadow_ctx;
> +               }
> +
> +               s->shadow[i] = ce;
> +       }
>  
>         bitmap_zero(s->shadow_ctx_desc_updated, I915_NUM_ENGINES);
>  
> @@ -1213,16 +1211,21 @@ int intel_vgpu_setup_submission(struct intel_vgpu *vgpu)
>                 goto out_shadow_ctx;
>         }
>  
> -       for_each_engine(engine, vgpu->gvt->dev_priv, i)
> -               INIT_LIST_HEAD(&s->workload_q_head[i]);
> -
>         atomic_set(&s->running_workload_num, 0);
>         bitmap_zero(s->tlb_handle_pending, I915_NUM_ENGINES);
>  
> +       i915_gem_context_put(ctx);
>         return 0;
>  
>  out_shadow_ctx:
> -       i915_gem_context_put(s->shadow_ctx);
> +       i915_context_ppgtt_root_restore(s, ctx->ppgtt);
> +       for_each_engine(engine, vgpu->gvt->dev_priv, i) {
> +               if (IS_ERR(s->shadow[i]))
> +                       break;
> +
> +               intel_context_unpin(s->shadow[i]);
> +       }
> +       i915_gem_context_put(ctx);
>         return ret;
>  }
>  
> -- 
> 2.20.1
> 
_______________________________________________
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