drm/i915/scheduler: add gvt notification for guc submission

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Chuanxiao Dong
Sent: Monday, March 20, 2017 9:49 AM
To: [email protected]
Cc: [email protected]
Subject: [Intel-gfx] [PATCH] 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.

Signed-off-by Chuanxiao Dong <[email protected]>

---
 drivers/gpu/drm/i915/i915_guc_submission.c | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c           | 15 ---------------
 drivers/gpu/drm/i915/intel_lrc.h           | 14 ++++++++++++++
 3 files changed, 27 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/i915_guc_submission.c +6 -0

index a3636b3..328b11c 100644
@@ -520,6 +520,12 @@ static void __i915_guc_submit(struct
 drm_i915_gem_request *rq)
 	unsigned long flags;
 	int b_ret;
 
+	/* Notify for the context status change schedule in
+	 * Currently only GVT care this notification for manually
+	 * context switch, like when using execlist mode submission
+	 */
+	execlists_context_status_change(rq,
 INTEL_CONTEXT_SCHEDULE_IN);
+
 	/* WA to flush out the pending GMADR writes to ring buffer. */

drivers/gpu/drm/i915/i915_guc_submission.c +7 -0

 	if (i915_vma_is_map_and_fenceable(rq->ring->vma))
 		POSTING_READ_FW(GUC_STATUS);

@@ -623,6 +629,13 @@ 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);
+			/* Notify for the context status change schedule
+			 * out. Currently only GVT care this notification
+			 * for manually context switch, like when using
+			 * execlist mode submission
+			 */
+			execlists_context_status_change(rq,
+
 	INTEL_CONTEXT_SCHEDULE_OUT);
 			i915_gem_request_put(rq);
 			port[0].request = port[1].request;

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

 			port[1].request = NULL;

drivers/gpu/drm/i915/intel_lrc.c +0 -15

index becde55..4f5906b 100644
@@ -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)  { diff --git a/drivers/gpu/drm/i915/intel_lrc.h

b/drivers/gpu/drm/i915/intel_lrc.h
index e8015e7..d3aa108 100644

drivers/gpu/drm/i915/intel_lrc.h +14 -0

@@ -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
+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);
+}
 

#endif /* _INTEL_LRC_H_ */
--
2.7.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Zheng, Xiao
Sent: Monday, March 20, 2017 10:46 AM
To: Dong, Chuanxiao <[email protected]>; intel- [email protected]
Cc: [email protected]
Subject: RE: [Intel-gfx] [PATCH] drm/i915/scheduler: add gvt notification for guc submission

It may consider to change the function name:
execlists_context_status_change to context_status_change_notify () instead. Otherwise confusing GUC submission path.
Thanks.

drivers/gpu/drm/i915/i915_guc_submission.c static void i915_guc_irq_handler(unsigned long

Quoted 43 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Chuanxiao Dong
Sent: Monday, March 20, 2017 9:49 AM
To: [email protected]
Cc: [email protected]
Subject: [Intel-gfx] [PATCH] 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.

Signed-off-by Chuanxiao Dong <[email protected]>

---
 drivers/gpu/drm/i915/i915_guc_submission.c | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c           | 15 ---------------
 drivers/gpu/drm/i915/intel_lrc.h           | 14 ++++++++++++++
 3 files changed, 27 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/i915_guc_submission.c +6 -0

index a3636b3..328b11c 100644
@@ -520,6 +520,12 @@ static void __i915_guc_submit(struct
 drm_i915_gem_request *rq)
 	unsigned long flags;
 	int b_ret;
 
+	/* Notify for the context status change schedule in
+	 * Currently only GVT care this notification for manually
+	 * context switch, like when using execlist mode submission
+	 */
+	execlists_context_status_change(rq,
 INTEL_CONTEXT_SCHEDULE_IN);
+
 	/* WA to flush out the pending GMADR writes to ring buffer. */

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

 	if (i915_vma_is_map_and_fenceable(rq->ring->vma))
 		POSTING_READ_FW(GUC_STATUS);

@@ -623,6 +629,13 @@ static void i915_guc_irq_handler(unsigned long

data)

Quoted 65 lines

drivers/gpu/drm/i915/i915_guc_submission.c +7 -0

 		rq = port[0].request;
 		while (rq && i915_gem_request_completed(rq)) {
 			trace_i915_gem_request_out(rq);
+			/* Notify for the context status change schedule
+			 * out. Currently only GVT care this notification
+			 * for manually context switch, like when using
+			 * execlist mode submission
+			 */
+			execlists_context_status_change(rq,
+
 	INTEL_CONTEXT_SCHEDULE_OUT);
 			i915_gem_request_put(rq);
 			port[0].request = port[1].request;

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

 			port[1].request = NULL;

drivers/gpu/drm/i915/intel_lrc.c +0 -15

index becde55..4f5906b 100644
@@ -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)  { diff --git a/drivers/gpu/drm/i915/intel_lrc.h

b/drivers/gpu/drm/i915/intel_lrc.h
index e8015e7..d3aa108 100644

drivers/gpu/drm/i915/intel_lrc.h +14 -0

@@ -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
+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);
+}
 

Quoted 8 lines

#endif /* _INTEL_LRC_H_ */
--
2.7.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Dong, Chuanxiao
Sent: Monday, March 20, 2017 11:20 AM
To: Zheng, Xiao <[email protected]>; [email protected]
Cc: [email protected]
Subject: RE: [Intel-gfx] [PATCH] drm/i915/scheduler: add gvt notification for guc submission

Quoted 11 lines

-----Original Message-----
From: Zheng, Xiao
Sent: Monday, March 20, 2017 10:46 AM
To: Dong, Chuanxiao <[email protected]>; intel- [email protected]
Cc: [email protected]
Subject: RE: [Intel-gfx] [PATCH] drm/i915/scheduler: add gvt notification for guc submission

It may consider to change the function name:
execlists_context_status_change to context_status_change_notify ()

instead.

Quoted 2 lines

Otherwise confusing GUC submission path.
Thanks.

Hi Xiao, I was considering to use the name of context_status_change_notify, but considering the guc submission is actually has an emulation of execlists on top, so back to use the original function name to align with execlists submission, as well as making the code change less. Is this explanation clear enough to use the original function name?

Thanks
Chuanxiao

Quoted 109 lines

drivers/gpu/drm/i915/i915_guc_submission.c static void i915_guc_irq_handler(unsigned long

Quoted 43 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Chuanxiao Dong
Sent: Monday, March 20, 2017 9:49 AM
To: [email protected]
Cc: [email protected]
Subject: [Intel-gfx] [PATCH] 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.

Signed-off-by Chuanxiao Dong <[email protected]>

---
 drivers/gpu/drm/i915/i915_guc_submission.c | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c           | 15 ---------------
 drivers/gpu/drm/i915/intel_lrc.h           | 14 ++++++++++++++
 3 files changed, 27 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/i915_guc_submission.c +6 -0

index a3636b3..328b11c 100644
@@ -520,6 +520,12 @@ static void __i915_guc_submit(struct
 drm_i915_gem_request *rq)
 	unsigned long flags;
 	int b_ret;
 
+	/* Notify for the context status change schedule in
+	 * Currently only GVT care this notification for manually
+	 * context switch, like when using execlist mode submission
+	 */
+	execlists_context_status_change(rq,
 INTEL_CONTEXT_SCHEDULE_IN);
+
 	/* WA to flush out the pending GMADR writes to ring buffer. */

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

 	if (i915_vma_is_map_and_fenceable(rq->ring->vma))
 		POSTING_READ_FW(GUC_STATUS);

@@ -623,6 +629,13 @@ static void i915_guc_irq_handler(unsigned long

data)

Quoted 65 lines

drivers/gpu/drm/i915/i915_guc_submission.c +7 -0

 		rq = port[0].request;
 		while (rq && i915_gem_request_completed(rq)) {
 			trace_i915_gem_request_out(rq);
+			/* Notify for the context status change schedule
+			 * out. Currently only GVT care this notification
+			 * for manually context switch, like when using
+			 * execlist mode submission
+			 */
+			execlists_context_status_change(rq,
+
 	INTEL_CONTEXT_SCHEDULE_OUT);
 			i915_gem_request_put(rq);
 			port[0].request = port[1].request;

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

 			port[1].request = NULL;

drivers/gpu/drm/i915/intel_lrc.c +0 -15

index becde55..4f5906b 100644
@@ -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)  { diff --git a/drivers/gpu/drm/i915/intel_lrc.h

b/drivers/gpu/drm/i915/intel_lrc.h
index e8015e7..d3aa108 100644

drivers/gpu/drm/i915/intel_lrc.h +14 -0

@@ -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
+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);
+}
 

Quoted 8 lines

Quoted 8 lines

#endif /* _INTEL_LRC_H_ */
--
2.7.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Zheng, Xiao
Sent: Tuesday, March 21, 2017 9:39 AM
To: Dong, Chuanxiao <[email protected]>; intel- [email protected]
Cc: [email protected]
Subject: RE: [Intel-gfx] [PATCH] drm/i915/scheduler: add gvt notification for guc submission

The new guc submission scheduler is not to emulation Execlist mode in driver. You can keep existing execlist_xx function name if insisted it is better.

Quoted 24 lines

-----Original Message-----
From: Dong, Chuanxiao
Sent: Monday, March 20, 2017 11:20 AM
To: Zheng, Xiao <[email protected]>; [email protected]
Cc: [email protected]
Subject: RE: [Intel-gfx] [PATCH] drm/i915/scheduler: add gvt notification for guc submission

Quoted 11 lines

-----Original Message-----
From: Zheng, Xiao
Sent: Monday, March 20, 2017 10:46 AM
To: Dong, Chuanxiao <[email protected]>; intel- [email protected]
Cc: [email protected]
Subject: RE: [Intel-gfx] [PATCH] drm/i915/scheduler: add gvt notification for guc submission

It may consider to change the function name:
execlists_context_status_change to context_status_change_notify ()

instead.

Quoted 8 lines

Quoted 2 lines

Otherwise confusing GUC submission path.
Thanks.

Hi Xiao, I was considering to use the name of context_status_change_notify, but considering the guc submission is actually has an emulation of execlists on top, so back to use the original function name to align with execlists submission, as well as making the code change less. Is this explanation clear enough to use the

original function name?

Quoted 114 lines

Thanks
Chuanxiao

Quoted 110 lines

Quoted 43 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Chuanxiao Dong
Sent: Monday, March 20, 2017 9:49 AM
To: [email protected]
Cc: [email protected]
Subject: [Intel-gfx] [PATCH] 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.

Signed-off-by Chuanxiao Dong <[email protected]>

---
 drivers/gpu/drm/i915/i915_guc_submission.c | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c           | 15 ---------------
 drivers/gpu/drm/i915/intel_lrc.h           | 14 ++++++++++++++
 3 files changed, 27 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/i915_guc_submission.c +6 -0

index a3636b3..328b11c 100644
@@ -520,6 +520,12 @@ static void __i915_guc_submit(struct
 drm_i915_gem_request *rq)
 	unsigned long flags;
 	int b_ret;
 
+	/* Notify for the context status change schedule in
+	 * Currently only GVT care this notification for manually
+	 * context switch, like when using execlist mode submission
+	 */
+	execlists_context_status_change(rq,
 INTEL_CONTEXT_SCHEDULE_IN);
+
 	/* WA to flush out the pending GMADR writes to ring buffer. */

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

 	if (i915_vma_is_map_and_fenceable(rq->ring->vma))
 		POSTING_READ_FW(GUC_STATUS);

@@ -623,6 +629,13 @@ static void i915_guc_irq_handler(unsigned

Quoted 1 line

long

data)

Quoted 65 lines

drivers/gpu/drm/i915/i915_guc_submission.c +7 -0

 		rq = port[0].request;
 		while (rq && i915_gem_request_completed(rq)) {
 			trace_i915_gem_request_out(rq);
+			/* Notify for the context status change schedule
+			 * out. Currently only GVT care this notification
+			 * for manually context switch, like when using
+			 * execlist mode submission
+			 */
+			execlists_context_status_change(rq,
+

	INTEL_CONTEXT_SCHEDULE_OUT);

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

 			i915_gem_request_put(rq);
 			port[0].request = port[1].request;
 			port[1].request = NULL;

drivers/gpu/drm/i915/intel_lrc.c +0 -15

index becde55..4f5906b 100644
@@ -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)  { diff --git a/drivers/gpu/drm/i915/intel_lrc.h

b/drivers/gpu/drm/i915/intel_lrc.h
index e8015e7..d3aa108 100644

drivers/gpu/drm/i915/intel_lrc.h +14 -0

@@ -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
+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);
+}
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

#endif /* _INTEL_LRC_H_ */
--
2.7.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Dong, Chuanxiao
Sent: Wednesday, March 22, 2017 2:35 PM
To: [email protected]
Cc: [email protected]; Dong, Chuanxiao <[email protected]>; Zheng, Xiao <[email protected]>; Tian, Kevin <[email protected]>
Subject: [PATCH v3] 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)

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

---
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 +++
 drivers/gpu/drm/i915/intel_lrc.c           | 21 +++------------------
 drivers/gpu/drm/i915/intel_lrc.h           | 13 +++++++++++++
 3 files changed, 19 insertions(+), 18 deletions(-)

drivers/gpu/drm/i915/i915_guc_submission.c +2 -0

index 055467a..0195547 100644
@@ -520,6 +520,8 @@ static void __i915_guc_submit(struct
 drm_i915_gem_request *rq)
 	unsigned long flags;
 	int b_ret;
 
+	context_status_change(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))

drivers/gpu/drm/i915/i915_guc_submission.c +1 -0

 		POSTING_READ_FW(GUC_STATUS);

@@ -634,6 +636,7 @@ 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);
+			context_status_change(rq,
 INTEL_CONTEXT_SCHEDULE_OUT);
 			i915_gem_request_put(rq);
 			port[0].request = port[1].request;

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

 			port[1].request = NULL;

drivers/gpu/drm/i915/intel_lrc.c +0 -15

index eec1e71..24c69b5 100644
@@ -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)

drivers/gpu/drm/i915/intel_lrc.c +1 -1

 	GEM_BUG_ON(port[0].count > 1);
 	if (!port[0].count)
-		execlists_context_status_change(port[0].request,
+		context_status_change(port[0].request,

	INTEL_CONTEXT_SCHEDULE_IN);

drivers/gpu/drm/i915/intel_lrc.c +1 -1

 	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,
+		context_status_change(port[1].request,
 
 	INTEL_CONTEXT_SCHEDULE_IN);

drivers/gpu/drm/i915/intel_lrc.c +0 -0

 		desc[1] = execlists_update_context(port[1].request);
 		GEM_DEBUG_EXEC(port[1].context_id =

upper_32_bits(desc[1])); @@ -581,7 +566,7 @@ static void intel_lrc_irq_handler(unsigned long data)

drivers/gpu/drm/i915/intel_lrc.c +0 -0

 			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,

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+				context_status_change(port[0].request,

	INTEL_CONTEXT_SCHEDULE_OUT);


	trace_i915_gem_request_out(port[0].request);

drivers/gpu/drm/i915/intel_lrc.h +13 -0

index e8015e7..51e1be9 100644
@@ -87,5 +87,18 @@ 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
+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);
+}
 

#endif /* _INTEL_LRC_H_ */
--
2.7.4

From: Dong, Chuanxiao
Sent: Thursday, March 23, 2017 1:29 PM

Ping for review. GVT relies on the notification before a request submitted and after a request completed, just like when using execlist mode submission.

Quoted 23 lines

-----Original Message-----
From: Dong, Chuanxiao
Sent: Wednesday, March 22, 2017 2:35 PM
To: [email protected]
Cc: [email protected]; Dong, Chuanxiao <[email protected]>; Zheng, Xiao <[email protected]>; Tian, Kevin <[email protected]>
Subject: [PATCH v3] 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)

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

drivers/gpu/drm/i915/i915_guc_submission.c static void i915_guc_irq_handler(unsigned long

Quoted 22 lines

---
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 +++
 drivers/gpu/drm/i915/intel_lrc.c           | 21 +++------------------
 drivers/gpu/drm/i915/intel_lrc.h           | 13 +++++++++++++
 3 files changed, 19 insertions(+), 18 deletions(-)

drivers/gpu/drm/i915/i915_guc_submission.c +2 -0

index 055467a..0195547 100644
@@ -520,6 +520,8 @@ static void __i915_guc_submit(struct
 drm_i915_gem_request *rq)
 	unsigned long flags;
 	int b_ret;
 
+	context_status_change(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))

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

 		POSTING_READ_FW(GUC_STATUS);

@@ -634,6 +636,7 @@ static void i915_guc_irq_handler(unsigned long

data)

Quoted 43 lines

drivers/gpu/drm/i915/i915_guc_submission.c +1 -0

 		rq = port[0].request;
 		while (rq && i915_gem_request_completed(rq)) {
 			trace_i915_gem_request_out(rq);
+			context_status_change(rq,
 INTEL_CONTEXT_SCHEDULE_OUT);
 			i915_gem_request_put(rq);
 			port[0].request = port[1].request;

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

 			port[1].request = NULL;

drivers/gpu/drm/i915/intel_lrc.c +0 -15

index eec1e71..24c69b5 100644
@@ -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)

drivers/gpu/drm/i915/intel_lrc.c +1 -1

 	GEM_BUG_ON(port[0].count > 1);
 	if (!port[0].count)
-		execlists_context_status_change(port[0].request,
+		context_status_change(port[0].request,

	INTEL_CONTEXT_SCHEDULE_IN);

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submit_ports(struct intel_engine_cs *engine) · line 341

Quoted 2 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

 	desc[0] = execlists_update_context(port[0].request);
 	GEM_DEBUG_EXEC(port[0].context_id = upper_32_bits(desc[0]));

@@

drivers/gpu/drm/i915/intel_lrc.c

Quoted 11 lines

-358,7 +343,7 @@ static void execlists_submit_ports(struct intel_engine_cs *engine)

drivers/gpu/drm/i915/intel_lrc.c +1 -1

 	if (port[1].request) {
 		GEM_BUG_ON(port[1].count);
-		execlists_context_status_change(port[1].request,
+		context_status_change(port[1].request,

	INTEL_CONTEXT_SCHEDULE_IN);

drivers/gpu/drm/i915/intel_lrc.c +0 -0

 		desc[1] = execlists_update_context(port[1].request);
 		GEM_DEBUG_EXEC(port[1].context_id =

upper_32_bits(desc[1])); @@

Quoted 38 lines

-581,7 +566,7 @@ static void intel_lrc_irq_handler(unsigned long data)

drivers/gpu/drm/i915/intel_lrc.c +0 -0

 			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,

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+				context_status_change(port[0].request,

	INTEL_CONTEXT_SCHEDULE_OUT);


	trace_i915_gem_request_out(port[0].request);

drivers/gpu/drm/i915/intel_lrc.h +13 -0

index e8015e7..51e1be9 100644
@@ -87,5 +87,18 @@ 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
+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);
+}
 

Quoted 3 lines

#endif /* _INTEL_LRC_H_ */
--
2.7.4

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)

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

(quoted diff) +0 -0

@@ -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)

(quoted diff) +1 -1

-		execlists_context_status_change(port[0].request,
+		context_status_change(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]));

(quoted diff) +0 -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);

drivers/gpu/drm/i915/intel_lrc.c +1 -1

-		execlists_context_status_change(port[1].request,
+		context_status_change(port[1].request,

  INTEL_CONTEXT_SCHEDULE_IN);

drivers/gpu/drm/i915/intel_lrc.h uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx, · line 91

Quoted 6 lines · Chuanxiao Dong

drivers/gpu/drm/i915/intel_lrc.h +2 -0

@@ -87,5 +87,18 @@ 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
+context_status_change(struct drm_i915_gem_request *rq, unsigned long status)

This needs intel_lr_ prefix now that it's in .h file.

-----Original Message-----
From: intel-gvt-dev [mailto:[email protected]] On Behalf Of Chris Wilson
Sent: Thursday, March 23, 2017 5:43 PM
To: Joonas Lahtinen
Cc: [email protected]; [email protected]; Dong, Chuanxiao
Subject: Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification for guc submission

Quoted 11 lines · Joonas Lahtinen

drivers/gpu/drm/i915/intel_lrc.h uint64_t intel_lr_context_descriptor(struct · line 93

Quoted 8 lines · Chuanxiao Dong

drivers/gpu/drm/i915/intel_lrc.h +3 -0

@@ -87,5 +87,18 @@ 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
+context_status_change(struct drm_i915_gem_request *rq, unsigned
+long status)

This needs intel_lr_ prefix now that it's in .h file.

But not in this header. The event is not strictly tied to execlists, it consumes request for starters - but on the other hand it is gvt specific.

I'm tempted to say intel_gvt_notify_context_status().

-Chris

--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
intel-gvt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

-----Original Message-----
From: Joonas Lahtinen [mailto:[email protected]]
Sent: Thursday, March 23, 2017 5:38 PM
To: Dong, Chuanxiao; [email protected]
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification for guc submission

Quoted 14 lines · Chuanxiao Dong

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)

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

<SNIP>

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submit_ports(struct · line 340

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -1

@@ -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,
+		context_status_change(port[0].request,
 

	INTEL_CONTEXT_SCHEDULE_IN);

Fix indent.

Quoted 2 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

  	desc[0] = execlists_update_context(port[0].request);

  GEM_DEBUG_EXEC(port[0].context_id = upper_32_bits(desc[0]));

@@

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submit_ports(struct intel_engine_cs *engine) · line 346

Quoted 8 lines

-358,7 +343,7 @@ static void execlists_submit_ports(struct intel_engine_cs *engine)

  if (port[1].request) {
  GEM_BUG_ON(port[1].count);

drivers/gpu/drm/i915/intel_lrc.c +1 -1

-		execlists_context_status_change(port[1].request,
+		context_status_change(port[1].request,

	INTEL_CONTEXT_SCHEDULE_IN);

Ditto.

Quoted 3 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

@@ -581,7 +566,7 @@ static void intel_lrc_irq_handler(unsigned long data)
  			if (--port[0].count == 0) {

  GEM_BUG_ON(status &

GEN8_CTX_STATUS_PREEMPTED);

Quoted 1 line

	GEM_BUG_ON(!i915_gem_request_completed(port[0].request));

Quoted 1 line

-

execlists_context_status_change(port[0].request,

Review

Quoted 2 lines

(quoted diff) +1 -0

+				context_status_change(port[0].request,

	INTEL_CONTEXT_SCHEDULE_OUT);

Ditto.

drivers/gpu/drm/i915/intel_lrc.h uint64_t intel_lr_context_descriptor(struct · line 93

Quoted 8 lines

drivers/gpu/drm/i915/intel_lrc.h +3 -0

@@ -87,5 +87,18 @@ 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
+context_status_change(struct drm_i915_gem_request *rq, unsigned long
+status)

This needs intel_lr_ prefix now that it's in .h file.

With those changes (make sure context_status_change doesn't become over character 80 line), this is;

Reviewed-by Joonas Lahtinen <[email protected]>

Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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)

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

---
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 +++
 drivers/gpu/drm/i915/intel_lrc.c           | 21 +++------------------
 drivers/gpu/drm/i915/intel_lrc.h           | 13 +++++++++++++
 3 files changed, 19 insertions(+), 18 deletions(-)

drivers/gpu/drm/i915/i915_guc_submission.c +2 -0

index 055467a..0195547 100644
@@ -520,6 +520,8 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq)
 	unsigned long flags;
 	int b_ret;
 
+	context_status_change(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);

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

 			i915_gem_request_put(rq);
 			port[0].request = port[1].request;
 			port[1].request = NULL;

drivers/gpu/drm/i915/intel_lrc.c +3 -18

index eec1e71..24c69b5 100644
@@ -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,
+		context_status_change(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,
+		context_status_change(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]));

@@ -581,7 +566,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,
+				context_status_change(port[0].request,
 								INTEL_CONTEXT_SCHEDULE_OUT);
 
 				trace_i915_gem_request_out(port[0].request);

drivers/gpu/drm/i915/intel_lrc.h +5 -0

index e8015e7..51e1be9 100644
@@ -87,5 +87,18 @@ 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
+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,

-----Original Message-----
From: Tvrtko Ursulin [mailto:[email protected]]
Sent: Thursday, March 23, 2017 5:52 PM
To: Dong, Chuanxiao; [email protected]
Cc: [email protected]
Subject: Re: [Intel-gfx] [PATCH v3] drm/i915/scheduler: add gvt notification for guc submission

Quoted 6 lines · Chuanxiao Dong

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

drivers/gpu/drm/i915/i915_guc_submission.c static void __i915_guc_submit(struct

Quoted 19 lines

for better understanding (ZhengXiao)
v3: remove the comment as it is obvious and not friendly to
the caller (Kevin)

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

---
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 +++
 drivers/gpu/drm/i915/intel_lrc.c           | 21 +++------------------
 drivers/gpu/drm/i915/intel_lrc.h           | 13 +++++++++++++
 3 files changed, 19 insertions(+), 18 deletions(-)

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

index 055467a..0195547 100644
@@ -520,6 +520,8 @@ static void __i915_guc_submit(struct

drm_i915_gem_request *rq)

drivers/gpu/drm/i915/i915_guc_submission.c static void i915_guc_irq_handler(unsigned long data) · line 639

Quoted 13 lines

drivers/gpu/drm/i915/i915_guc_submission.c +3 -0

 	unsigned long flags;
 	int b_ret;
 
+	context_status_change(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,7 @@ 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);
+			context_status_change(rq,

INTEL_CONTEXT_SCHEDULE_OUT);

Does GVT care that the context will still be active on the GPU for a small window after this notification? (User interrupt happens before context complete, which GuC hides from the driver.)

Regards,

Tvrtko

drivers/gpu/drm/i915/intel_lrc.c uint64_t intel_lr_context_descriptor(struct

Quoted 9 lines

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

 			i915_gem_request_put(rq);
 			port[0].request = port[1].request;
 			port[1].request = NULL;

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index eec1e71..24c69b5 100644
@@ -295,21 +295,6 @@ uint64_t intel_lr_context_descriptor(struct

i915_gem_context *ctx,

Quoted 23 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -15

 	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)

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submit_ports(struct intel_engine_cs *engine) · line 338

Quoted 4 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -1

 	GEM_BUG_ON(port[0].count > 1);
 	if (!port[0].count)
-		execlists_context_status_change(port[0].request,
+		context_status_change(port[0].request,

	INTEL_CONTEXT_SCHEDULE_IN);

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submit_ports(struct intel_engine_cs *engine) · line 341

Quoted 2 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

 	desc[0] = execlists_update_context(port[0].request);
 	GEM_DEBUG_EXEC(port[0].context_id = upper_32_bits(desc[0]));

@@

drivers/gpu/drm/i915/intel_lrc.c static void execlists_submit_ports(struct intel_engine_cs *engine) · line 346

Quoted 8 lines

-358,7 +343,7 @@ static void execlists_submit_ports(struct intel_engine_cs *engine)

drivers/gpu/drm/i915/intel_lrc.c +1 -1

 	if (port[1].request) {
 		GEM_BUG_ON(port[1].count);
-		execlists_context_status_change(port[1].request,
+		context_status_change(port[1].request,

	INTEL_CONTEXT_SCHEDULE_IN);

drivers/gpu/drm/i915/intel_lrc.c

Quoted 2 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

 		desc[1] = execlists_update_context(port[1].request);
 		GEM_DEBUG_EXEC(port[1].context_id =

upper_32_bits(desc[1])); @@

drivers/gpu/drm/i915/intel_lrc.c

Quoted 3 lines

-581,7 +566,7 @@ static void intel_lrc_irq_handler(unsigned long data)

drivers/gpu/drm/i915/intel_lrc.c +0 -0

 			if (--port[0].count == 0) {
 				GEM_BUG_ON(status &

GEN8_CTX_STATUS_PREEMPTED);

Quoted 1 line

	GEM_BUG_ON(!i915_gem_request_completed(port[0].request));

Quoted 1 line

-

execlists_context_status_change(port[0].request,

drivers/gpu/drm/i915/intel_lrc.c

Quoted 2 lines

drivers/gpu/drm/i915/intel_lrc.c +1 -0

+				context_status_change(port[0].request,

	INTEL_CONTEXT_SCHEDULE_OUT);

Quoted 2 lines

	trace_i915_gem_request_out(port[0].request);

Quoted 24 lines

drivers/gpu/drm/i915/intel_lrc.h +13 -0

index e8015e7..51e1be9 100644
@@ -87,5 +87,18 @@ 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
+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);
+}
 

Quoted 2 lines

#endif /* _INTEL_LRC_H_ */

-----Original Message-----
From: Dong, Chuanxiao
Sent: Friday, March 24, 2017 9:49 AM
To: [email protected]
Cc: [email protected]; Dong, Chuanxiao <[email protected]>; Zheng, Xiao <[email protected]>; Tian, Kevin <[email protected]>; [email protected]; 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 [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_guc_submission.c +2 -0

index 055467a..91a567d 100644
@@ -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))

drivers/gpu/drm/i915/i915_guc_submission.c +2 -0

 		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;

drivers/gpu/drm/i915/intel_lrc.c +0 -15

index eec1e71..8b0c937 100644
@@ -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)

drivers/gpu/drm/i915/intel_lrc.c +0 -2

 	GEM_BUG_ON(port[0].count > 1);
 	if (!port[0].count)
-		execlists_context_status_change(port[0].request,
-

	INTEL_CONTEXT_SCHEDULE_IN);

drivers/gpu/drm/i915/intel_lrc.c +2 -2

+		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);

drivers/gpu/drm/i915/intel_lrc.c +2 -0

+		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]));

drivers/gpu/drm/i915/intel_lrc.c +0 -2

 		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);

drivers/gpu/drm/i915/intel_lrc.c +0 -0

 				i915_gem_request_put(port[0].request);

drivers/gpu/drm/i915/intel_lrc.h +14 -0

index e8015e7..304e9f6 100644
@@ -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

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 [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

-----Original Message-----
From: Joonas Lahtinen [mailto:[email protected]]
Sent: Monday, March 27, 2017 6:14 PM
To: Dong, Chuanxiao <[email protected]>; intel- [email protected]
Cc: [email protected]; Zheng, Xiao <[email protected]>; Tian, Kevin <[email protected]>; chris@chris- wilson.co.uk
Subject: Re: [PATCH v4] drm/i915/scheduler: add gvt notification for guc submission

Quoted 19 lines · Chuanxiao Dong

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 [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

<SNIP>

Review static void i915_guc_irq_handler(unsigned long

Quoted 1 line

(quoted diff) +0 -0

@@ -634,6 +636,8 @@ static void i915_guc_irq_handler(unsigned long

data)

drivers/gpu/drm/i915/i915_guc_submission.c static void i915_guc_irq_handler(unsigned long data) · line 640

Quoted 5 lines

drivers/gpu/drm/i915/i915_guc_submission.c +2 -0

  		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.

drivers/gpu/drm/i915/intel_lrc.h uint64_t intel_lr_context_descriptor(struct · line 93

Quoted 9 lines

drivers/gpu/drm/i915/intel_lrc.h +3 -0

@@ -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
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Quoted 10 lines

drivers/gpu/drm/i915/i915_guc_submission.c static void i915_guc_irq_handler(unsigned long data) · line 641

Quoted 8 lines

drivers/gpu/drm/i915/i915_guc_submission.c +2 -0

@@ -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.

Quoted 2 lines

			intel_gvt_notify_context_status(rq,
							INTEL_CONTEXT_SCHEDULE_OUT);

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 [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

index 991e76e..1223169 100644

-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Monday, March 27, 2017 9:50 PM
To: Dong, Chuanxiao
Cc: [email protected]; [email protected]; Zheng, Xiao; Tian, Kevin; [email protected]
Subject: Re: [PATCH v5] drm/i915/scheduler: add gvt notification for guc submission

Quoted 6 lines · Chuanxiao Dong

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

drivers/gpu/drm/i915/i915_guc_submission.c static void __i915_guc_submit(struct

Quoted 25 lines

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 [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

index 991e76e..1223169 100644
@@ -606,6 +606,8 @@ static void __i915_guc_submit(struct

drm_i915_gem_request *rq)

drivers/gpu/drm/i915/i915_guc_submission.c static void __i915_guc_submit(struct drm_i915_gem_request *rq) · line 609

Quoted 4 lines

drivers/gpu/drm/i915/i915_guc_submission.c +1 -0

 	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

--
Chris Wilson, Intel Open Source Technology Centre

Quoted 43 lines

-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Monday, March 27, 2017 9:50 PM
To: Dong, Chuanxiao
Cc: [email protected]; [email protected]; Zheng, Xiao; Tian, Kevin; [email protected]
Subject: Re: [PATCH v5] drm/i915/scheduler: add gvt notification for guc submission

Quoted 6 lines · Chuanxiao Dong

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

drivers/gpu/drm/i915/i915_guc_submission.c static void __i915_guc_submit(struct

Quoted 25 lines

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 [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

index 991e76e..1223169 100644
@@ -606,6 +606,8 @@ static void __i915_guc_submit(struct

drm_i915_gem_request *rq)

Quoted 7 lines

drivers/gpu/drm/i915/i915_guc_submission.c static void __i915_guc_submit(struct drm_i915_gem_request *rq) · line 609

Quoted 4 lines

drivers/gpu/drm/i915/i915_guc_submission.c +1 -0

 	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?

Quoted 2 lines

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?

-----Original Message-----
From: intel-gvt-dev [mailto:[email protected]] On Behalf Of Chris Wilson
Sent: Monday, March 27, 2017 10:33 PM
To: Dong, Chuanxiao
Cc: Zheng, Xiao; [email protected]; [email protected]; [email protected]; Tian, Kevin
Subject: Re: [PATCH v5] drm/i915/scheduler: add gvt notification for guc submission

Quoted 44 lines · Chuanxiao

Quoted 44 lines

-----Original Message-----
From: Chris Wilson [mailto:[email protected]]
Sent: Monday, March 27, 2017 9:50 PM
To: Dong, Chuanxiao
Cc: [email protected]; [email protected];
Zheng, Xiao; Tian, Kevin; [email protected]
Subject: Re: [PATCH v5] drm/i915/scheduler: add gvt notification for guc submission

Quoted 6 lines · Chuanxiao Dong

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

drivers/gpu/drm/i915/i915_guc_submission.c static void __i915_guc_submit(struct

Quoted 25 lines

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 [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Chuanxiao Dong <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_guc_submission.c +0 -0

index 991e76e..1223169 100644
@@ -606,6 +606,8 @@ static void __i915_guc_submit(struct

drm_i915_gem_request *rq)

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/i915/i915_guc_submission.c static void __i915_guc_submit(struct drm_i915_gem_request *rq) · line 609

Quoted 4 lines

drivers/gpu/drm/i915/i915_guc_submission.c +1 -0

 	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?

Quoted 1 line

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?

Quoted 4 lines

Quoted 3 lines

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

--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
intel-gvt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev