| Message ID | 1490621540-25089-1-git-send-email-chuanxiao.dong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 27, 2017 at 09:32:20PM +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. > > v2: use context_status_change instead of execlists_context_status_change > for better understanding (ZhengXiao) > v3: remove the comment as it is obvious and not friendly to > the caller (Kevin) > v4: fix indent issues (Joonas) > rename the context_status_change to intel_gvt_notify_context_status (Chris) > v5: move intel_gvt_notify_context_status to intel_gvt.h (Joonas) > > 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 | 13 +++++++++++++ > drivers/gpu/drm/i915/intel_lrc.c | 21 +++------------------ > 3 files changed, 20 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 991e76e..1223169 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); So this gets called for every request, rather than at the context switch boundaries, and we only once signal the SCHEDULE_OUT. Does that matter? Hmm, shouldn't happen in execlists due to force-single-submission. Does it matter to gvt that we repeat the SCHEDULE_IN after reset (happens for execlists as well)? -Chris
> -----Original Message----- > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > Sent: Monday, March 27, 2017 9:50 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 v5] drm/i915/scheduler: add gvt notification for guc > submission > > On Mon, Mar 27, 2017 at 09:32:20PM +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. > > > > v2: use context_status_change instead of > execlists_context_status_change > > for better understanding (ZhengXiao) > > v3: remove the comment as it is obvious and not friendly to > > the caller (Kevin) > > v4: fix indent issues (Joonas) > > rename the context_status_change to > > intel_gvt_notify_context_status (Chris) > > v5: move intel_gvt_notify_context_status to intel_gvt.h (Joonas) > > > > 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 | 13 +++++++++++++ > > drivers/gpu/drm/i915/intel_lrc.c | 21 +++------------------ > > 3 files changed, 20 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c > > b/drivers/gpu/drm/i915/i915_guc_submission.c > > index 991e76e..1223169 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); > > So this gets called for every request, rather than at the context switch > boundaries, and we only once signal the SCHEDULE_OUT. Does that matter? Hi Chris, each request submitted by Guc should have a SCHEDULE_OUT in i915_guc_irq_handler to match with this SCHEDULE_IN. Any possible reason for this OUT/IN not mached? > > Hmm, shouldn't happen in execlists due to force-single-submission. GuC should also use force-single-submission for GVT request as execlists does. I have another patch to add this. Do you think if I should combine that patch with this one? > > Does it matter to gvt that we repeat the SCHEDULE_IN after reset (happens > for execlists as well)? This should be fine. Thanks Chuanxiao > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Mon, Mar 27, 2017 at 02:22:10PM +0000, Dong, Chuanxiao wrote: > > -----Original Message----- > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > > Sent: Monday, March 27, 2017 9:50 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 v5] drm/i915/scheduler: add gvt notification for guc > > submission > > > > On Mon, Mar 27, 2017 at 09:32:20PM +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. > > > > > > v2: use context_status_change instead of > > execlists_context_status_change > > > for better understanding (ZhengXiao) > > > v3: remove the comment as it is obvious and not friendly to > > > the caller (Kevin) > > > v4: fix indent issues (Joonas) > > > rename the context_status_change to > > > intel_gvt_notify_context_status (Chris) > > > v5: move intel_gvt_notify_context_status to intel_gvt.h (Joonas) > > > > > > 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 | 13 +++++++++++++ > > > drivers/gpu/drm/i915/intel_lrc.c | 21 +++------------------ > > > 3 files changed, 20 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c > > > b/drivers/gpu/drm/i915/i915_guc_submission.c > > > index 991e76e..1223169 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); > > > > So this gets called for every request, rather than at the context switch > > boundaries, and we only once signal the SCHEDULE_OUT. Does that matter? > Hi Chris, each request submitted by Guc should have a SCHEDULE_OUT in i915_guc_irq_handler to match with this SCHEDULE_IN. Any possible reason for this OUT/IN not mached? > > > > > Hmm, shouldn't happen in execlists due to force-single-submission. > GuC should also use force-single-submission for GVT request as execlists does. I have another patch to add this. Do you think if I should combine that patch with this one? Currently, each request is emitting SCHEDULE_IN, but then being amalgamated if in the same context as the earlier. Then we only see SCHEDULE_OUT from the final request in the context batch. It sounds like you need to apply the force-single-submission patch first, and then the context status notifier. Please send them in a single series, in the order they need to be applied. -Chris
> -----Original Message----- > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On > Behalf Of Chris Wilson > Sent: Monday, March 27, 2017 10:33 PM > To: Dong, Chuanxiao > Cc: Zheng, Xiao; intel-gfx@lists.freedesktop.org; > joonas.lahtinen@linux.intel.com; intel-gvt-dev@lists.freedesktop.org; Tian, > Kevin > Subject: Re: [PATCH v5] drm/i915/scheduler: add gvt notification for guc > submission > > On Mon, Mar 27, 2017 at 02:22:10PM +0000, Dong, Chuanxiao wrote: > > > -----Original Message----- > > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > > > Sent: Monday, March 27, 2017 9:50 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 v5] drm/i915/scheduler: add gvt notification for > > > guc submission > > > > > > On Mon, Mar 27, 2017 at 09:32:20PM +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. > > > > > > > > v2: use context_status_change instead of > > > execlists_context_status_change > > > > for better understanding (ZhengXiao) > > > > v3: remove the comment as it is obvious and not friendly to > > > > the caller (Kevin) > > > > v4: fix indent issues (Joonas) > > > > rename the context_status_change to > > > > intel_gvt_notify_context_status (Chris) > > > > v5: move intel_gvt_notify_context_status to intel_gvt.h (Joonas) > > > > > > > > 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 | 13 +++++++++++++ > > > > drivers/gpu/drm/i915/intel_lrc.c | 21 +++------------------ > > > > 3 files changed, 20 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c > > > > b/drivers/gpu/drm/i915/i915_guc_submission.c > > > > index 991e76e..1223169 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); > > > > > > So this gets called for every request, rather than at the context > > > switch boundaries, and we only once signal the SCHEDULE_OUT. Does > that matter? > > Hi Chris, each request submitted by Guc should have a SCHEDULE_OUT in > i915_guc_irq_handler to match with this SCHEDULE_IN. Any possible reason > for this OUT/IN not mached? > > > > > > > > Hmm, shouldn't happen in execlists due to force-single-submission. > > GuC should also use force-single-submission for GVT request as execlists > does. I have another patch to add this. Do you think if I should combine that > patch with this one? > > Currently, each request is emitting SCHEDULE_IN, but then being > amalgamated if in the same context as the earlier. Then we only see > SCHEDULE_OUT from the final request in the context batch. It sounds like > you need to apply the force-single-submission patch first, and then the > context status notifier. Please send them in a single series, in the order they > need to be applied. I see. Thanks Chris. I will drop this one and send out a new serial patches. Thanks Chuanxiao > -Chris > > -- > 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 991e76e..1223169 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); @@ -720,6 +722,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 25df2d6..9175018 100644 --- a/drivers/gpu/drm/i915/intel_gvt.h +++ b/drivers/gpu/drm/i915/intel_gvt.h @@ -32,6 +32,14 @@ void intel_gvt_cleanup(struct drm_i915_private *dev_priv); int intel_gvt_init_device(struct drm_i915_private *dev_priv); void intel_gvt_clean_device(struct drm_i915_private *dev_priv); int intel_gvt_init_host(void); + +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) { @@ -40,6 +48,11 @@ static inline int intel_gvt_init(struct drm_i915_private *dev_priv) static inline void intel_gvt_cleanup(struct drm_i915_private *dev_priv) { } +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 dd0e9d587..8708515 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])); @@ -591,7 +576,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. v2: use context_status_change instead of execlists_context_status_change for better understanding (ZhengXiao) v3: remove the comment as it is obvious and not friendly to the caller (Kevin) v4: fix indent issues (Joonas) rename the context_status_change to intel_gvt_notify_context_status (Chris) v5: move intel_gvt_notify_context_status to intel_gvt.h (Joonas) 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 | 13 +++++++++++++ drivers/gpu/drm/i915/intel_lrc.c | 21 +++------------------ 3 files changed, 20 insertions(+), 18 deletions(-)