[PATCH] drm/i915/gvt: GVT pin/unpin shadow context

[Dong, Chuanxiao]

> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw at linux.intel.com]
> Sent: Thursday, March 16, 2017 12:49 PM
> To: Dong, Chuanxiao <chuanxiao.dong at intel.com>
> Cc: intel-gvt-dev at lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/gvt: GVT pin/unpin shadow context
> 
> On 2017.03.16 09:47:58 +0800, Chuanxiao Dong wrote:
> > When handling guest request, GVT needs to populate/update shadow_ctx
> > with guest context. This behavior needs to make sure the shadow_ctx is
> > pinned. The current implementation is relying on i195 allocate request
> > to pin but this way cannot guarantee the i915 not to unpin the
> > shadow_ctx when GVT update the guest context from shadow_ctx. So GVT
> > should pin/unpin the shadow_ctx by itself.
> >
> > Signed-off-by: Chuanxiao Dong <chuanxiao.dong at intel.com>
> > ---
> 
> My previous comment for Pei's old change was that even shadow_ctx might
> not be pinned but gvt could still access its mem, so could fix this by changing
> any shadow ctx manipulating code. Although this one is better than previous,
> still could we add pin only around required steps to ensure it's pinned when
> used?

Hi Zhenyu, pin/unpin during the entire life of the shadow context in one i915 request may have less code change than pin/unpin when used.

Thanks
Chuanxiao

> 
> >  drivers/gpu/drm/i915/gvt/scheduler.c | 27
> +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c
> > b/drivers/gpu/drm/i915/gvt/scheduler.c
> > index d29e435..cc76f26 100644
> > --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> > +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> > @@ -175,6 +175,7 @@ static int dispatch_workload(struct
> intel_vgpu_workload *workload)
> >  	int ring_id = workload->ring_id;
> >  	struct i915_gem_context *shadow_ctx = workload->vgpu-
> >shadow_ctx;
> >  	struct drm_i915_private *dev_priv = workload->vgpu->gvt->dev_priv;
> > +	struct intel_engine_cs *engine = dev_priv->engine[ring_id];
> >  	struct drm_i915_gem_request *rq;
> >  	struct intel_vgpu *vgpu = workload->vgpu;
> >  	int ret;
> > @@ -188,6 +189,21 @@ static int dispatch_workload(struct
> > intel_vgpu_workload *workload)
> >
> >  	mutex_lock(&dev_priv->drm.struct_mutex);
> >
> > +	/* 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.
> > +	 */
> > +	ret = engine->context_pin(engine, shadow_ctx);
> > +	if (ret) {
> > +		gvt_vgpu_err("fail to pin shadow context\n");
> > +		workload->status = ret;
> > +		mutex_unlock(&dev_priv->drm.struct_mutex);
> > +		return ret;
> > +	}
> > +
> >  	rq = i915_gem_request_alloc(dev_priv->engine[ring_id],
> shadow_ctx);
> >  	if (IS_ERR(rq)) {
> >  		gvt_vgpu_err("fail to allocate gem request\n"); @@ -228,6
> +244,9 @@
> > static int dispatch_workload(struct intel_vgpu_workload *workload)
> >
> >  	if (!IS_ERR_OR_NULL(rq))
> >  		i915_add_request_no_flush(rq);
> > +	else
> > +		engine->context_unpin(engine, shadow_ctx);
> > +
> >  	mutex_unlock(&dev_priv->drm.struct_mutex);
> >  	return ret;
> >  }
> > @@ -377,6 +396,10 @@ static void complete_current_workload(struct
> intel_gvt *gvt, int ring_id)
> >  	 * 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 intel_engine_cs *engine =
> > +			dev_priv->engine[workload->ring_id];
> >  		wait_event(workload->shadow_ctx_status_wq,
> >  			   !atomic_read(&workload->shadow_ctx_active));
> >
> > @@ -389,6 +412,10 @@ static void complete_current_workload(struct
> intel_gvt *gvt, int ring_id)
> >  					 INTEL_GVT_EVENT_MAX)
> >  				intel_vgpu_trigger_virtual_event(vgpu,
> event);
> >  		}
> > +		mutex_lock(&dev_priv->drm.struct_mutex);
> > +		/* unpin shadow ctx as the shadow_ctx update is done */
> > +		engine->context_unpin(engine, workload->vgpu-
> >shadow_ctx);
> > +		mutex_unlock(&dev_priv->drm.struct_mutex);
> >  	}
> >
> >  	gvt_dbg_sched("ring id %d complete workload %p status %d\n",
> > --
> > 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