[PATCH 2/3] drm/i915/gvt: Only assign ppgtt root at dispatch time

[Zhenyu Wang]

On 2019.03.01 02:56:45 +0000, Zhang, Xiong Y wrote:
> > This moves ppgtt root hook out of scan and shadow function, as it's only
> > required at dispatch time. Also make sure this checks against shadow mm to
> > be ready, otherwise bail earlier.
> [Zhang, Xiong Y] Don't see the benefit for this.

Current symptom is that when some hang happens, set_context_ppgtt_from_shadow()
always failed, but still need to find out that reason, as its function is not
related to scan and shadow, we did have place to call early scan, so moving this
only for dispatch time.

> I indeed found a potential issue in our code: intel_vgpu_create_workload() call intel_gvt_scan_and_shadow_workload() which operate shadow_ctx, but shadow_ctx may be used by other engine at this time, this will cause other engine trouble. I see before calling scan_and_shadow, it call mutex_lock(drm.struct_mutex), I don't think this could prevent the issue.
>

yeah, I think that's valid concern, currently we have a window between workload dispatch
and complete, which workload thread won't hold vgpu lock to guard against another new
submission from guest, so potentially new dispatch could happen during that on another
engine's thread. So looks at least we need to have shadow ctx for each engine.

> thanks
> > 
> > Cc: Xiong Zhang <xiong.y.zhang at intel.com>
> > Signed-off-by: Zhenyu Wang <zhenyuw at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/scheduler.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c
> > b/drivers/gpu/drm/i915/gvt/scheduler.c
> > index 5b59cba9f93a..e28a3b3513a4 100644
> > --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> > +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> > @@ -346,7 +346,7 @@ static int set_context_ppgtt_from_shadow(struct
> > intel_vgpu_workload *workload,
> >  	int i = 0;
> > 
> >  	if (mm->type != INTEL_GVT_MM_PPGTT
> > || !mm->ppgtt_mm.shadowed)
> > -		return -1;
> > +		return -EINVAL;
> > 
> >  	if (mm->ppgtt_mm.root_entry_type ==
> > GTT_TYPE_PPGTT_ROOT_L4_ENTRY) {
> >  		px_dma(&ppgtt->pml4) = mm->ppgtt_mm.shadow_pdps[0]; @@
> > -410,12 +410,6 @@ int intel_gvt_scan_and_shadow_workload(struct
> > intel_vgpu_workload *workload)
> >  	if (workload->shadow)
> >  		return 0;
> > 
> > -	ret = set_context_ppgtt_from_shadow(workload, shadow_ctx);
> > -	if (ret < 0) {
> > -		gvt_vgpu_err("workload shadow ppgtt isn't ready\n");
> > -		return ret;
> > -	}
> > -
> >  	/* 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 @@ -678,6 +672,8 @@ 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;
> > @@ -688,6 +684,12 @@ 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);
> > +	if (ret < 0) {
> > +		gvt_vgpu_err("workload shadow ppgtt isn't ready\n");
> > +		goto err_req;
> > +	}
> > +
> >  	ret = intel_gvt_workload_req_alloc(workload);
> >  	if (ret)
> >  		goto err_req;
> > --
> > 2.20.1
> 

-- 
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/20190301/9607072e/attachment.sig>