| Message ID | 1490693921-28692-3-git-send-email-chuanxiao.dong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
> -----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. Thanks Chuanxiao > > -- > Chris Wilson, Intel Open Source Technology Centre
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. -Chris
> -----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.... To make sure the context image is ready, GVT will poll CSB. If CSB is not in idle, just relax cpu and continue. In the real case, CSB is already idle before GVT to poll. Thanks Chuanxiao > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
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
> -----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. Thanks Chuanxiao > > -- > Chris Wilson, Intel Open Source Technology Centre
> -----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. 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
> -----Original Message----- > From: Dong, Chuanxiao > Sent: Monday, April 10, 2017 10:40 AM > To: Dong, Chuanxiao <chuanxiao.dong@intel.com>; 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 > > 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. Hi Chris, may I know if you still have concerns with the above comment? Would like to know if this is acceptable to i915. 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
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); i915_gem_request_put(rq); port[0].request = port[1].request; port[1].request = NULL; diff --git a/drivers/gpu/drm/i915/intel_gvt.h b/drivers/gpu/drm/i915/intel_gvt.h index c0dcd66..813d0f8 100644 --- a/drivers/gpu/drm/i915/intel_gvt.h +++ b/drivers/gpu/drm/i915/intel_gvt.h @@ -38,6 +38,13 @@ intel_gvt_context_single_port_submit(const struct i915_gem_context *ctx) { return i915_gem_context_force_single_submission(ctx); } +static inline void +intel_gvt_notify_context_status(struct drm_i915_gem_request *rq, + unsigned long status) +{ + atomic_notifier_call_chain(&rq->engine->context_status_notifier, + status, rq); +} #else static inline int intel_gvt_init(struct drm_i915_private *dev_priv) { @@ -51,6 +58,11 @@ intel_gvt_context_single_port_submit(const struct i915_gem_context *ctx) { return false; } +static inline void +intel_gvt_notify_context_status(struct drm_i915_gem_request *rq, + unsigned long status) +{ +} #endif #endif /* _INTEL_GVT_H_ */ diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 951540f..2333ffb 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -295,21 +295,6 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx, return ctx->engine[engine->id].lrc_desc; } -static inline void -execlists_context_status_change(struct drm_i915_gem_request *rq, - unsigned long status) -{ - /* - * Only used when GVT-g is enabled now. When GVT-g is disabled, - * The compiler should eliminate this function as dead-code. - */ - if (!IS_ENABLED(CONFIG_DRM_I915_GVT)) - return; - - atomic_notifier_call_chain(&rq->engine->context_status_notifier, - status, rq); -} - static void execlists_update_context_pdps(struct i915_hw_ppgtt *ppgtt, u32 *reg_state) { @@ -350,7 +335,7 @@ static void execlists_submit_ports(struct intel_engine_cs *engine) GEM_BUG_ON(port[0].count > 1); if (!port[0].count) - execlists_context_status_change(port[0].request, + intel_gvt_notify_context_status(port[0].request, INTEL_CONTEXT_SCHEDULE_IN); desc[0] = execlists_update_context(port[0].request); GEM_DEBUG_EXEC(port[0].context_id = upper_32_bits(desc[0])); @@ -358,7 +343,7 @@ static void execlists_submit_ports(struct intel_engine_cs *engine) if (port[1].request) { GEM_BUG_ON(port[1].count); - execlists_context_status_change(port[1].request, + intel_gvt_notify_context_status(port[1].request, INTEL_CONTEXT_SCHEDULE_IN); desc[1] = execlists_update_context(port[1].request); GEM_DEBUG_EXEC(port[1].context_id = upper_32_bits(desc[1])); @@ -574,7 +559,7 @@ static void intel_lrc_irq_handler(unsigned long data) if (--port[0].count == 0) { GEM_BUG_ON(status & GEN8_CTX_STATUS_PREEMPTED); GEM_BUG_ON(!i915_gem_request_completed(port[0].request)); - execlists_context_status_change(port[0].request, + intel_gvt_notify_context_status(port[0].request, INTEL_CONTEXT_SCHEDULE_OUT); trace_i915_gem_request_out(port[0].request);
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(-)