Re: [PATCH v2 2/2] drm/i915/scheduler: add gvt notification for guc

["Dong, Chuanxiao" <chuanxiao.dong@intel.com>]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Dong, Chuanxiao
> Sent: Wednesday, April 12, 2017 5:12 PM
> To: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tian, Kevin <kevin.tian@intel.com>; intel-gvt-dev@lists.freedesktop.org;
> intel-gfx@lists.freedesktop.org; joonas.lahtinen@linux.intel.com; Zheng,
> Xiao <xiao.zheng@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>
> Subject: RE: [PATCH v2 2/2] drm/i915/scheduler: add gvt notification for guc
> 
> 
> 
> > -----Original Message-----
> > From: intel-gvt-dev
> > [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of
> > Chris Wilson
> > Sent: Wednesday, April 12, 2017 4:22 PM
> > To: Dong, Chuanxiao <chuanxiao.dong@intel.com>
> > Cc: Tian, Kevin <kevin.tian@intel.com>;
> > intel-gvt-dev@lists.freedesktop.org;
> > intel-gfx@lists.freedesktop.org; joonas.lahtinen@linux.intel.com;
> > Zheng, Xiao <xiao.zheng@intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>
> > Subject: Re: [PATCH v2 2/2] drm/i915/scheduler: add gvt notification
> > for guc
> >
> > On Mon, Apr 10, 2017 at 02:40:24AM +0000, Dong, Chuanxiao wrote:
> > > > -----Original Message-----
> > > > From: intel-gvt-dev
> > > > [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of
> > > > Dong, Chuanxiao
> > > > Sent: Thursday, April 6, 2017 11:19 PM
> > > > To: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Tian, Kevin <kevin.tian@intel.com>;
> > > > intel-gvt-dev@lists.freedesktop.org;
> > > > intel-gfx@lists.freedesktop.org; joonas.lahtinen@linux.intel.com;
> > > > Zheng, Xiao <xiao.zheng@intel.com>; Wang, Zhi A
> > > > <zhi.a.wang@intel.com>
> > > > Subject: RE: [PATCH v2 2/2] drm/i915/scheduler: add gvt
> > > > notification for guc
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > > > Sent: Thursday, April 6, 2017 11:07 PM
> > > > > To: Dong, Chuanxiao
> > > > > Cc: intel-gfx@lists.freedesktop.org;
> > > > > intel-gvt-dev@lists.freedesktop.org;
> > > > > Zheng, Xiao; Tian, Kevin; joonas.lahtinen@linux.intel.com; Wang,
> > > > > Zhi A
> > > > > Subject: Re: [PATCH v2 2/2] drm/i915/scheduler: add gvt
> > > > > notification for guc
> > > > >
> > > > > On Thu, Apr 06, 2017 at 02:49:54PM +0000, Dong, Chuanxiao wrote:
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > > > > > Sent: Thursday, April 6, 2017 10:19 PM
> > > > > > > To: Dong, Chuanxiao
> > > > > > > Cc: intel-gfx@lists.freedesktop.org;
> > > > > > > intel-gvt-dev@lists.freedesktop.org;
> > > > > > > Zheng, Xiao; Tian, Kevin; joonas.lahtinen@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@chris-wilson.co.uk]
> > > > > > > > > Sent: Thursday, April 6, 2017 9:32 PM
> > > > > > > > > To: Dong, Chuanxiao
> > > > > > > > > Cc: intel-gfx@lists.freedesktop.org;
> > > > > > > > > intel-gvt-dev@lists.freedesktop.org;
> > > > > > > > > Zheng, Xiao; Tian, Kevin;
> > > > > > > > > joonas.lahtinen@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@intel.com
> > > > > > > > > > Cc: kevin.tian@intel.com
> > > > > > > > > > Cc: joonas.lahtinen@linux.intel.com
> > > > > > > > > > Cc: chris@chris-wilson.co.uk
> > > > > > > > > > Signed-off-by: Chuanxiao Dong
> > > > > > > > > > <chuanxiao.dong@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
> > > >
> > > > The major issue is that we should wait for the context image to be
> > > > completely written to memory before GVT read it. I will double
> > > > check if we are really reading from context image in this
> > > > SCHEDULE_OUT event and return back later.
> > >
> > > Hi Chris,
> > >
> > > Had a discussion with Zhi, actually the context image is accessed
> > > from the
> > workload_thread to update the guest context but not directly from the
> > SCHEDULE_OUT event. So my previous comment is wrong and the CSB
> > waiting should be in workload_thread instead of this IRQ handler.
> >
> > That's better, still a little worried about having to remember to
> > review these hooks later.
> >
> > The other concern I express at the start of this was:
> Sorry, I missed this one.
> 
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > b/drivers/gpu/drm/i915/intel_lrc.c
> > index 9e327049b735..5a86abd715df 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -430,16 +430,6 @@ static void execlists_dequeue(struct
> > intel_engine_cs *engine)
> >                         if (port != engine->execlist_port)
> >                                 break;
> >
> > -                       /* If GVT overrides us we only ever submit port[0],
> > -                        * leaving port[1] empty. Note that we also have
> > -                        * to be careful that we don't queue the same
> > -                        * context (even though a different request) to
> > -                        * the second port.
> > -                        */
> > -                       if (ctx_single_port_submission(last->ctx) ||
> > -                           ctx_single_port_submission(cursor->ctx))
> > -                               break;
> > -
> >
> >
> > Afaict that requirement is completely bogus (since your workloads do
> > only active on gvt requests and there will only ever be one
> > scheduled-in at any
> > time) and burdensome on non-gvt.
> > -Chris
> 
> My understanding about this is to prevent both port[0] and port[1] submitted
> if any of the last/cursor is a GVT request. Without this check, 1) last is a GVT
> request in port[0] and cursor is an i915 request, this i915 request will be
> submitted to port[1]; 2) last is an i915 request in port[0] and cursor is a GVT
> request, this GVT request will be submitted to port[1]; 3) Last is a GVT
> request from vgpu1 in port[0] and cursor is a GVT request from vgpu2, the
> GVT request from vgpu2 will be submitted to port[1];
> 
> Is this ok without the check? Or my understanding is wrong? Please help me
> to get this. :)
> 
Hi Chris,

Had a discussed this with Zhi about removing this check, and we can see after removing this check, port[0] and port[1] can be submitted both for the above 3 cases. For GVT request, only one of the port can be used and the other one should be empty, just like the code comment said. The reason is that GVT still needs to do some MMIO restore manually in the SCHEDULE_OUT event before i915 starts to handle another request. If removing the check and have both port[0] and port[1] submitted, i915 will start to process port[1] automatically once port[0] is completed. So this check is meaningful to GVT and cannot be removed. Do you agree to keep this check?

Thanks
Chuanxiao 
> 
> >
> > --
> > Chris Wilson, Intel Open Source Technology Centre
> > _______________________________________________
> > intel-gvt-dev mailing list
> > intel-gvt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx