[Intel-gfx] [PATCH v2 2/2] drm/i915/scheduler: add gvt notification for guc

[Chris Wilson]

On Thu, Apr 06, 2017 at 02:49:54PM +0000, Dong, Chuanxiao wrote:
> 
> 
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> > Sent: Thursday, April 6, 2017 10:19 PM
> > To: Dong, Chuanxiao
> > Cc: intel-gfx at lists.freedesktop.org; intel-gvt-dev at lists.freedesktop.org;
> > Zheng, Xiao; Tian, Kevin; joonas.lahtinen at linux.intel.com
> > Subject: Re: [PATCH v2 2/2] drm/i915/scheduler: add gvt notification for guc
> > 
> > On Thu, Apr 06, 2017 at 02:05:15PM +0000, Dong, Chuanxiao wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> > > > Sent: Thursday, April 6, 2017 9:32 PM
> > > > To: Dong, Chuanxiao
> > > > Cc: intel-gfx at lists.freedesktop.org;
> > > > intel-gvt-dev at lists.freedesktop.org;
> > > > Zheng, Xiao; Tian, Kevin; joonas.lahtinen at linux.intel.com
> > > > Subject: Re: [PATCH v2 2/2] drm/i915/scheduler: add gvt notification
> > > > for guc
> > > >
> > > > On Tue, Mar 28, 2017 at 05:38:41PM +0800, Chuanxiao Dong wrote:
> > > > > GVT request needs a manual mmio load/restore. Before GuC submit a
> > > > > request, send notification to gvt for mmio loading. And after the
> > > > > GuC finished this GVT request, notify gvt again for mmio restore.
> > > > > This follows the usage when using execlists submission.
> > > > >
> > > > > Cc: xiao.zheng at intel.com
> > > > > Cc: kevin.tian at intel.com
> > > > > Cc: joonas.lahtinen at linux.intel.com
> > > > > Cc: chris at chris-wilson.co.uk
> > > > > Signed-off-by: Chuanxiao Dong <chuanxiao.dong at intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_guc_submission.c |  4 ++++
> > > > >  drivers/gpu/drm/i915/intel_gvt.h           | 12 ++++++++++++
> > > > >  drivers/gpu/drm/i915/intel_lrc.c           | 21 +++------------------
> > > > >  3 files changed, 19 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > > b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > > index 58087630..d8a5942 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > > @@ -606,6 +606,8 @@ static void __i915_guc_submit(struct
> > > > drm_i915_gem_request *rq)
> > > > >  	unsigned long flags;
> > > > >  	int b_ret;
> > > > >
> > > > > +	intel_gvt_notify_context_status(rq, INTEL_CONTEXT_SCHEDULE_IN);
> > > > > +
> > > > >  	/* WA to flush out the pending GMADR writes to ring buffer. */
> > > > >  	if (i915_vma_is_map_and_fenceable(rq->ring->vma))
> > > > >  		POSTING_READ_FW(GUC_STATUS);
> > > > > @@ -725,6 +727,8 @@ static void i915_guc_irq_handler(unsigned long
> > data)
> > > > >  		rq = port[0].request;
> > > > >  		while (rq && i915_gem_request_completed(rq)) {
> > > > >  			trace_i915_gem_request_out(rq);
> > > > > +			intel_gvt_notify_context_status(rq,
> > > > > +					INTEL_CONTEXT_SCHEDULE_OUT);
> > > >
> > > > This is incorrect though. This is no better than just waiting for
> > > > the request, which is not enough since the idea is that you need to
> > > > wait for the context image to be completely written to memory before
> > you read it.
> > > > -Chris
> > >
> > > The wait for the context image to be completely written will be done in the
> > notification from the GVT, by checking the CSB. If put the wait here will made
> > each i915 request to wait, which seems not necessary.
> > 
> > Urm, no. I hope you mean the wait will be on some other thread than inside
> > this interrupt handler.
> 
> The SCHEDULE_OUT means to stop GuC to submit another request before the current one is completed by GVT so GVT can manually restore the MMIO. So this irq handler should wait until SCHEDULE_OUT is completed. How it possible to make this irq handler to wait for another thread? From the current software architecture there is no other thread....

No. It is not acceptable to have any blocking here. Rather you delegate
the polling of CSB to a thread/worker that you kick off from this notify.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre