| Message ID | 1490320162-16521-1-git-send-email-chuanxiao.dong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, ping for review v4. It fixed the comments from Joonas and Chris. Would like to know if this patch can be applied in i915 to unblock the GVT in GuC mode. :) Thanks Chuanxiao > -----Original Message----- > From: Dong, Chuanxiao > Sent: Friday, March 24, 2017 9:49 AM > To: intel-gfx@lists.freedesktop.org > Cc: intel-gvt-dev@lists.freedesktop.org; Dong, Chuanxiao > <chuanxiao.dong@intel.com>; Zheng, Xiao <xiao.zheng@intel.com>; Tian, > Kevin <kevin.tian@intel.com>; joonas.lahtinen@linux.intel.com; chris@chris- > wilson.co.uk > Subject: [PATCH v4] drm/i915/scheduler: add gvt notification for guc > submission > > 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) > > 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_lrc.c | 27 ++++++--------------------- > drivers/gpu/drm/i915/intel_lrc.h | 14 ++++++++++++++ > 3 files changed, 24 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c > b/drivers/gpu/drm/i915/i915_guc_submission.c > index 055467a..91a567d 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -520,6 +520,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); > @@ -634,6 +636,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_lrc.c > b/drivers/gpu/drm/i915/intel_lrc.c > index eec1e71..8b0c937 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,16 +335,16 @@ 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_CONTEXT_SCHEDULE_IN); > + 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])); > port[0].count++; > > if (port[1].request) { > GEM_BUG_ON(port[1].count); > - execlists_context_status_change(port[1].request, > - > INTEL_CONTEXT_SCHEDULE_IN); > + 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])); > port[1].count = 1; > @@ -581,8 +566,8 @@ 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_CONTEXT_SCHEDULE_OUT); > + > intel_gvt_notify_context_status(port[0].request, > + > INTEL_CONTEXT_SCHEDULE_OUT); > > > trace_i915_gem_request_out(port[0].request); > i915_gem_request_put(port[0].request); > diff --git a/drivers/gpu/drm/i915/intel_lrc.h > b/drivers/gpu/drm/i915/intel_lrc.h > index e8015e7..304e9f6 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.h > +++ b/drivers/gpu/drm/i915/intel_lrc.h > @@ -87,5 +87,19 @@ uint64_t intel_lr_context_descriptor(struct > i915_gem_context *ctx, > /* Execlists */ > int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv, > int enable_execlists); > +static inline void > +intel_gvt_notify_context_status(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); > +} > > #endif /* _INTEL_LRC_H_ */ > -- > 2.7.4
On pe, 2017-03-24 at 09:49 +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) > > 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> <SNIP> > @@ -634,6 +636,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); Code indent is still broken. > @@ -87,5 +87,19 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx, > /* Execlists */ > int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv, > int enable_execlists); > +static inline void > +intel_gvt_notify_context_status(struct drm_i915_gem_request *rq, > + unsigned long status) With that prefix, this needs to go to intel_gvt.h, where you can take advantage of the existing #ifdef block (which should really be #if IS_ENABLED() too). Regards, Joonas
> -----Original Message----- > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com] > Sent: Monday, March 27, 2017 6:14 PM > To: Dong, Chuanxiao <chuanxiao.dong@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: intel-gvt-dev@lists.freedesktop.org; Zheng, Xiao > <xiao.zheng@intel.com>; Tian, Kevin <kevin.tian@intel.com>; chris@chris- > wilson.co.uk > Subject: Re: [PATCH v4] drm/i915/scheduler: add gvt notification for guc > submission > > On pe, 2017-03-24 at 09:49 +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) > > > > 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> > > <SNIP> > > > @@ -634,6 +636,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); > > Code indent is still broken. Hi Joonas, I am sorry for not getting this code indent issue. The above code is just split by typing an enter due to longer than 80 characters. Are you expecting to see the code like below? The 2nd line will be longer than 80 characters in below case. If this is fine then I will change this in the next version. intel_gvt_notify_context_status(rq, INTEL_CONTEXT_SCHEDULE_OUT); > > > @@ -87,5 +87,19 @@ uint64_t intel_lr_context_descriptor(struct > > i915_gem_context *ctx, > > /* Execlists */ > > int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv, > > int enable_execlists); > > +static inline void > > +intel_gvt_notify_context_status(struct drm_i915_gem_request *rq, > > + unsigned long status) > > With that prefix, this needs to go to intel_gvt.h, where you can take > advantage of the existing #ifdef block (which should really be #if > IS_ENABLED() too). Sure. Will move to intel_gvt.h in the next version. Thanks Chuanxiao > > Regards, Joonas > -- > Joonas Lahtinen > Open Source Technology Center > Intel Corporation
On ma, 2017-03-27 at 10:58 +0000, Dong, Chuanxiao wrote: > > > > > @@ -634,6 +636,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); > > > > Code indent is still broken. > Hi Joonas, I am sorry for not getting this code indent issue. The > above code is just split by typing an enter due to longer than 80 > characters. Are you expecting to see the code like below? The 2nd > line will be longer than 80 characters in below case. If this is fine > then I will change this in the next version. > > intel_gvt_notify_context_status(rq, > > INTEL_CONTEXT_SCHEDULE_OUT); > My bad, I missed one tab. Regards, Joonas
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 055467a..91a567d 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -520,6 +520,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); @@ -634,6 +636,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_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index eec1e71..8b0c937 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,16 +335,16 @@ 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_CONTEXT_SCHEDULE_IN); + 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])); port[0].count++; if (port[1].request) { GEM_BUG_ON(port[1].count); - execlists_context_status_change(port[1].request, - INTEL_CONTEXT_SCHEDULE_IN); + 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])); port[1].count = 1; @@ -581,8 +566,8 @@ 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_CONTEXT_SCHEDULE_OUT); + intel_gvt_notify_context_status(port[0].request, + INTEL_CONTEXT_SCHEDULE_OUT); trace_i915_gem_request_out(port[0].request); i915_gem_request_put(port[0].request); diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h index e8015e7..304e9f6 100644 --- a/drivers/gpu/drm/i915/intel_lrc.h +++ b/drivers/gpu/drm/i915/intel_lrc.h @@ -87,5 +87,19 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx, /* Execlists */ int intel_sanitize_enable_execlists(struct drm_i915_private *dev_priv, int enable_execlists); +static inline void +intel_gvt_notify_context_status(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); +} #endif /* _INTEL_LRC_H_ */
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) 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_lrc.c | 27 ++++++--------------------- drivers/gpu/drm/i915/intel_lrc.h | 14 ++++++++++++++ 3 files changed, 24 insertions(+), 21 deletions(-)