> -----Original Message----- > From: intel-gvt-dev [mailto:intel-gvt-dev-boun...@lists.freedesktop.org] On > Behalf Of Dong, Chuanxiao > Sent: Thursday, April 13, 2017 7:03 PM > To: Chris Wilson <ch...@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org > Cc: Zheng, Xiao <xiao.zh...@intel.com>; Tian, Kevin > <kevin.t...@intel.com>; joonas.lahti...@linux.intel.com; intel-gvt- > d...@lists.freedesktop.org; Wang, Zhi A <zhi.a.w...@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-boun...@lists.freedesktop.org] On Behalf Of > > Dong, Chuanxiao > > Sent: Wednesday, April 12, 2017 5:12 PM > > To: Chris Wilson <ch...@chris-wilson.co.uk> > > Cc: Tian, Kevin <kevin.t...@intel.com>; > > intel-gvt-...@lists.freedesktop.org; > > intel-gfx@lists.freedesktop.org; joonas.lahti...@linux.intel.com; > > Zheng, Xiao <xiao.zh...@intel.com>; Wang, Zhi A <zhi.a.w...@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-boun...@lists.freedesktop.org] On Behalf Of > > > Chris Wilson > > > Sent: Wednesday, April 12, 2017 4:22 PM > > > To: Dong, Chuanxiao <chuanxiao.d...@intel.com> > > > Cc: Tian, Kevin <kevin.t...@intel.com>; > > > intel-gvt-...@lists.freedesktop.org; > > > intel-gfx@lists.freedesktop.org; joonas.lahti...@linux.intel.com; > > > Zheng, Xiao <xiao.zh...@intel.com>; Wang, Zhi A > > > <zhi.a.w...@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-boun...@lists.freedesktop.org] On Behalf > > > > > Of Dong, Chuanxiao > > > > > Sent: Thursday, April 6, 2017 11:19 PM > > > > > To: Chris Wilson <ch...@chris-wilson.co.uk> > > > > > Cc: Tian, Kevin <kevin.t...@intel.com>; > > > > > intel-gvt-...@lists.freedesktop.org; > > > > > intel-gfx@lists.freedesktop.org; > > > > > joonas.lahti...@linux.intel.com; Zheng, Xiao > > > > > <xiao.zh...@intel.com>; Wang, Zhi A <zhi.a.w...@intel.com> > > > > > Subject: RE: [PATCH v2 2/2] drm/i915/scheduler: add gvt > > > > > notification for guc > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] > > > > > > Sent: Thursday, April 6, 2017 11:07 PM > > > > > > To: Dong, Chuanxiao > > > > > > Cc: intel-gfx@lists.freedesktop.org; > > > > > > intel-gvt-...@lists.freedesktop.org; > > > > > > Zheng, Xiao; Tian, Kevin; joonas.lahti...@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:ch...@chris-wilson.co.uk] > > > > > > > > Sent: Thursday, April 6, 2017 10:19 PM > > > > > > > > To: Dong, Chuanxiao > > > > > > > > Cc: intel-gfx@lists.freedesktop.org; > > > > > > > > intel-gvt-...@lists.freedesktop.org; > > > > > > > > Zheng, Xiao; Tian, Kevin; joonas.lahti...@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:ch...@chris-wilson.co.uk] > > > > > > > > > > Sent: Thursday, April 6, 2017 9:32 PM > > > > > > > > > > To: Dong, Chuanxiao > > > > > > > > > > Cc: intel-gfx@lists.freedesktop.org; > > > > > > > > > > intel-gvt-...@lists.freedesktop.org; > > > > > > > > > > Zheng, Xiao; Tian, Kevin; > > > > > > > > > > joonas.lahti...@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.zh...@intel.com > > > > > > > > > > > Cc: kevin.t...@intel.com > > > > > > > > > > > Cc: joonas.lahti...@linux.intel.com > > > > > > > > > > > Cc: ch...@chris-wilson.co.uk > > > > > > > > > > > Signed-off-by: Chuanxiao Dong > > > > > > > > > > > <chuanxiao.d...@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? >
Ping for the review feedback. > > > > > > > > -- > > > Chris Wilson, Intel Open Source Technology Centre > > > _______________________________________________ > > > intel-gvt-dev mailing list > > > intel-gvt-...@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev > > _______________________________________________ > > intel-gvt-dev mailing list > > intel-gvt-...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev > _______________________________________________ > intel-gvt-dev mailing list > intel-gvt-...@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
