[PATCH 00/20] GuC CTBs changes + a few misc patches

dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 00/20] GuC CTBs changes + a few misc patches
@ 2021-06-03  5:16 Matthew Brost
  2021-06-03  5:16 ` [PATCH 01/20] drm/i915/guc: skip disabling CTBs before sanitizing the GuC Matthew Brost
                   ` (19 more replies)
  0 siblings, 20 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

Single series to merge the following series that have been reviewed /
passed CI:
https://patchwork.freedesktop.org/series/90633/
https://patchwork.freedesktop.org/series/90552/

Signed-off-by: Matthew Brost <matthew.brost@intel.com>

Daniele Ceraolo Spurio (4):
  drm/i915/guc: skip disabling CTBs before sanitizing the GuC
  drm/i915/guc: use probe_error log for CT enablement failure
  drm/i915/guc: enable only the user interrupt when using GuC submission
  drm/i915/guc: Use guc_class instead of engine_class in fw interface

Matthew Brost (2):
  drm/i915/guc: Drop guc->interrupts.enabled
  drm/i915/guc: Ensure H2G buffer updates visible before tail update

Michal Wajdeczko (13):
  drm/i915/guc: Keep strict GuC ABI definitions
  drm/i915/guc: Stop using fence/status from CTB descriptor
  drm/i915: Promote ptrdiff() to i915_utils.h
  drm/i915/guc: Only rely on own CTB size
  drm/i915/guc: Don't repeat CTB layout calculations
  drm/i915/guc: Replace CTB array with explicit members
  drm/i915/guc: Update sizes of CTB buffers
  drm/i915/guc: Relax CTB response timeout
  drm/i915/guc: Start protecting access to CTB descriptors
  drm/i915/guc: Stop using mutex while sending CTB messages
  drm/i915/guc: Don't receive all G2H messages in irq handler
  drm/i915/guc: Always copy CT message to new allocation
  drm/i915/guc: Early initialization of GuC send registers

Rodrigo Vivi (1):
  drm/i915/guc: Remove sample_forcewake h2g action

 drivers/gpu/drm/i915/Kconfig.profile          |  10 +
 drivers/gpu/drm/i915/gt/intel_engine_cs.c     |   6 +-
 drivers/gpu/drm/i915/gt/intel_gt_irq.c        |  18 +-
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |  51 ++
 .../gt/uc/abi/guc_communication_ctb_abi.h     | 106 ++++
 .../gt/uc/abi/guc_communication_mmio_abi.h    |  52 ++
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |  14 +
 .../gpu/drm/i915/gt/uc/abi/guc_messages_abi.h |  21 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.c        |  61 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc.h        |   2 -
 drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    |  20 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c     | 537 +++++++++++-------
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h     |  14 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   | 233 ++------
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  31 -
 drivers/gpu/drm/i915/gt/uc/intel_uc.c         |  10 -
 drivers/gpu/drm/i915/i915_utils.h             |   5 +
 drivers/gpu/drm/i915/i915_vma.h               |   5 -
 18 files changed, 667 insertions(+), 529 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h

-- 
2.28.0


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 01/20] drm/i915/guc: skip disabling CTBs before sanitizing the GuC
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 02/20] drm/i915/guc: use probe_error log for CT enablement failure Matthew Brost
                   ` (18 subsequent siblings)
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

If we're about to sanitize the GuC, something might have going wrong
beforehand, so we should avoid trying to talk to it. Even if GuC is
still running fine, the sanitize will reset its internal state and clear
the CTB registration, so there is still no need to explicitly do so.

References: https://gitlab.freedesktop.org/drm/intel/-/issues/2469
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 6abb8f2dc33d..892c1315ce49 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -504,7 +504,7 @@ static int __uc_init_hw(struct intel_uc *uc)
 
 	ret = intel_guc_sample_forcewake(guc);
 	if (ret)
-		goto err_communication;
+		goto err_log_capture;
 
 	if (intel_uc_uses_guc_submission(uc))
 		intel_guc_submission_enable(guc);
@@ -529,8 +529,6 @@ static int __uc_init_hw(struct intel_uc *uc)
 	/*
 	 * We've failed to load the firmware :(
 	 */
-err_communication:
-	guc_disable_communication(guc);
 err_log_capture:
 	__uc_capture_load_err_log(uc);
 err_out:
@@ -558,9 +556,6 @@ static void __uc_fini_hw(struct intel_uc *uc)
 	if (intel_uc_uses_guc_submission(uc))
 		intel_guc_submission_disable(guc);
 
-	if (guc_communication_enabled(guc))
-		guc_disable_communication(guc);
-
 	__uc_sanitize(uc);
 }
 
@@ -577,7 +572,6 @@ void intel_uc_reset_prepare(struct intel_uc *uc)
 	if (!intel_guc_is_ready(guc))
 		return;
 
-	guc_disable_communication(guc);
 	__uc_sanitize(uc);
 }
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 02/20] drm/i915/guc: use probe_error log for CT enablement failure
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
  2021-06-03  5:16 ` [PATCH 01/20] drm/i915/guc: skip disabling CTBs before sanitizing the GuC Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 03/20] drm/i915/guc: enable only the user interrupt when using GuC submission Matthew Brost
                   ` (17 subsequent siblings)
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

We have a couple of failure injection points in the CT enablement path,
so we need to use i915_probe_error() to select the appropriate log level.
A new macro (CT_PROBE_ERROR) has been added to the set of CT logging
macros to be used in this scenario and upcoming ones.

While adding the new macros, fix the underlying logging mechanics used
by the existing ones (DRM_DEV_* -> drm_*) and move the inlines to
before they're used inside the macros.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 48 ++++++++++++-----------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index fa9e048cc65f..72b48ac9271a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -7,14 +7,36 @@
 #include "intel_guc_ct.h"
 #include "gt/intel_gt.h"
 
+static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct)
+{
+	return container_of(ct, struct intel_guc, ct);
+}
+
+static inline struct intel_gt *ct_to_gt(struct intel_guc_ct *ct)
+{
+	return guc_to_gt(ct_to_guc(ct));
+}
+
+static inline struct drm_i915_private *ct_to_i915(struct intel_guc_ct *ct)
+{
+	return ct_to_gt(ct)->i915;
+}
+
+static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct)
+{
+	return &ct_to_i915(ct)->drm;
+}
+
 #define CT_ERROR(_ct, _fmt, ...) \
-	DRM_DEV_ERROR(ct_to_dev(_ct), "CT: " _fmt, ##__VA_ARGS__)
+	drm_err(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
 #ifdef CONFIG_DRM_I915_DEBUG_GUC
 #define CT_DEBUG(_ct, _fmt, ...) \
-	DRM_DEV_DEBUG_DRIVER(ct_to_dev(_ct), "CT: " _fmt, ##__VA_ARGS__)
+	drm_dbg(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
 #else
 #define CT_DEBUG(...)	do { } while (0)
 #endif
+#define CT_PROBE_ERROR(_ct, _fmt, ...) \
+	i915_probe_error(ct_to_i915(ct), "CT: " _fmt, ##__VA_ARGS__)
 
 struct ct_request {
 	struct list_head link;
@@ -47,26 +69,6 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)
 	INIT_WORK(&ct->requests.worker, ct_incoming_request_worker_func);
 }
 
-static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct)
-{
-	return container_of(ct, struct intel_guc, ct);
-}
-
-static inline struct intel_gt *ct_to_gt(struct intel_guc_ct *ct)
-{
-	return guc_to_gt(ct_to_guc(ct));
-}
-
-static inline struct drm_i915_private *ct_to_i915(struct intel_guc_ct *ct)
-{
-	return ct_to_gt(ct)->i915;
-}
-
-static inline struct device *ct_to_dev(struct intel_guc_ct *ct)
-{
-	return ct_to_i915(ct)->drm.dev;
-}
-
 static inline const char *guc_ct_buffer_type_to_str(u32 type)
 {
 	switch (type) {
@@ -264,7 +266,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 err_deregister:
 	ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV);
 err_out:
-	CT_ERROR(ct, "Failed to open open CT channel (err=%d)\n", err);
+	CT_PROBE_ERROR(ct, "Failed to open channel (err=%d)\n", err);
 	return err;
 }
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 03/20] drm/i915/guc: enable only the user interrupt when using GuC submission
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
  2021-06-03  5:16 ` [PATCH 01/20] drm/i915/guc: skip disabling CTBs before sanitizing the GuC Matthew Brost
  2021-06-03  5:16 ` [PATCH 02/20] drm/i915/guc: use probe_error log for CT enablement failure Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 04/20] drm/i915/guc: Remove sample_forcewake h2g action Matthew Brost
                   ` (16 subsequent siblings)
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

In GuC submission mode the CS is owned by the GuC FW, so all CS status
interrupts are handled by it. We only need the user interrupt as that
signals request completion.

Since we're now starting the engines directly in GuC submission mode
when selected, we can stop switching back and forth between the
execlists and the GuC programming and select directly the correct
interrupt mask.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
Cc: John Harrison <john.c.harrison@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_irq.c        | 18 ++++++-----
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 31 -------------------
 2 files changed, 11 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
index d29126c458ba..f88c10366e58 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
@@ -194,14 +194,18 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
 
 void gen11_gt_irq_postinstall(struct intel_gt *gt)
 {
-	const u32 irqs =
-		GT_CS_MASTER_ERROR_INTERRUPT |
-		GT_RENDER_USER_INTERRUPT |
-		GT_CONTEXT_SWITCH_INTERRUPT |
-		GT_WAIT_SEMAPHORE_INTERRUPT;
 	struct intel_uncore *uncore = gt->uncore;
-	const u32 dmask = irqs << 16 | irqs;
-	const u32 smask = irqs << 16;
+	u32 irqs = GT_RENDER_USER_INTERRUPT;
+	u32 dmask;
+	u32 smask;
+
+	if (!intel_uc_wants_guc_submission(&gt->uc))
+		irqs |= GT_CS_MASTER_ERROR_INTERRUPT |
+			GT_CONTEXT_SWITCH_INTERRUPT |
+			GT_WAIT_SEMAPHORE_INTERRUPT;
+
+	dmask = irqs << 16 | irqs;
+	smask = irqs << 16;
 
 	BUILD_BUG_ON(irqs & 0xffff0000);
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 335719f17490..38cda5d599a6 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -432,32 +432,6 @@ void intel_guc_submission_fini(struct intel_guc *guc)
 	}
 }
 
-static void guc_interrupts_capture(struct intel_gt *gt)
-{
-	struct intel_uncore *uncore = gt->uncore;
-	u32 irqs = GT_CONTEXT_SWITCH_INTERRUPT;
-	u32 dmask = irqs << 16 | irqs;
-
-	GEM_BUG_ON(INTEL_GEN(gt->i915) < 11);
-
-	/* Don't handle the ctx switch interrupt in GuC submission mode */
-	intel_uncore_rmw(uncore, GEN11_RENDER_COPY_INTR_ENABLE, dmask, 0);
-	intel_uncore_rmw(uncore, GEN11_VCS_VECS_INTR_ENABLE, dmask, 0);
-}
-
-static void guc_interrupts_release(struct intel_gt *gt)
-{
-	struct intel_uncore *uncore = gt->uncore;
-	u32 irqs = GT_CONTEXT_SWITCH_INTERRUPT;
-	u32 dmask = irqs << 16 | irqs;
-
-	GEM_BUG_ON(INTEL_GEN(gt->i915) < 11);
-
-	/* Handle ctx switch interrupts again */
-	intel_uncore_rmw(uncore, GEN11_RENDER_COPY_INTR_ENABLE, 0, dmask);
-	intel_uncore_rmw(uncore, GEN11_VCS_VECS_INTR_ENABLE, 0, dmask);
-}
-
 static int guc_context_alloc(struct intel_context *ce)
 {
 	return lrc_alloc(ce, ce->engine);
@@ -722,9 +696,6 @@ int intel_guc_submission_setup(struct intel_engine_cs *engine)
 void intel_guc_submission_enable(struct intel_guc *guc)
 {
 	guc_stage_desc_init(guc);
-
-	/* Take over from manual control of ELSP (execlists) */
-	guc_interrupts_capture(guc_to_gt(guc));
 }
 
 void intel_guc_submission_disable(struct intel_guc *guc)
@@ -735,8 +706,6 @@ void intel_guc_submission_disable(struct intel_guc *guc)
 
 	/* Note: By the time we're here, GuC may have already been reset */
 
-	guc_interrupts_release(gt);
-
 	guc_stage_desc_fini(guc);
 }
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 04/20] drm/i915/guc: Remove sample_forcewake h2g action
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (2 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 03/20] drm/i915/guc: enable only the user interrupt when using GuC submission Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 05/20] drm/i915/guc: Keep strict GuC ABI definitions Matthew Brost
                   ` (15 subsequent siblings)
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Rodrigo Vivi <rodrigo.vivi@intel.com>

This action is no-op in the GuC side for a few versions already
and it is getting entirely removed soon, in an upcoming version.

Time to remove before we face communication issues.

Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Acked-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc.c      | 16 ----------------
 drivers/gpu/drm/i915/gt/uc/intel_guc.h      |  1 -
 drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  4 ----
 drivers/gpu/drm/i915/gt/uc/intel_uc.c       |  4 ----
 4 files changed, 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index adae04c47aab..ab2c8fe8cdfa 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -469,22 +469,6 @@ int intel_guc_to_host_process_recv_msg(struct intel_guc *guc,
 	return 0;
 }
 
-int intel_guc_sample_forcewake(struct intel_guc *guc)
-{
-	struct drm_i915_private *dev_priv = guc_to_gt(guc)->i915;
-	u32 action[2];
-
-	action[0] = INTEL_GUC_ACTION_SAMPLE_FORCEWAKE;
-	/* WaRsDisableCoarsePowerGating:skl,cnl */
-	if (!HAS_RC6(dev_priv) || NEEDS_WaRsDisableCoarsePowerGating(dev_priv))
-		action[1] = 0;
-	else
-		/* bit 0 and 1 are for Render and Media domain separately */
-		action[1] = GUC_FORCEWAKE_RENDER | GUC_FORCEWAKE_MEDIA;
-
-	return intel_guc_send(guc, action, ARRAY_SIZE(action));
-}
-
 /**
  * intel_guc_auth_huc() - Send action to GuC to authenticate HuC ucode
  * @guc: intel_guc structure
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index bc2ba7d0626c..c20f3839de12 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -128,7 +128,6 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
 			u32 *response_buf, u32 response_buf_size);
 int intel_guc_to_host_process_recv_msg(struct intel_guc *guc,
 				       const u32 *payload, u32 len);
-int intel_guc_sample_forcewake(struct intel_guc *guc);
 int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
 int intel_guc_suspend(struct intel_guc *guc);
 int intel_guc_resume(struct intel_guc *guc);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
index 79c560d9c0b6..0f9afcde1d0b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
@@ -302,9 +302,6 @@ struct guc_ct_buffer_desc {
 #define GUC_CT_MSG_ACTION_SHIFT			16
 #define GUC_CT_MSG_ACTION_MASK			0xFFFF
 
-#define GUC_FORCEWAKE_RENDER	(1 << 0)
-#define GUC_FORCEWAKE_MEDIA	(1 << 1)
-
 #define GUC_POWER_UNSPECIFIED	0
 #define GUC_POWER_D0		1
 #define GUC_POWER_D1		2
@@ -558,7 +555,6 @@ enum intel_guc_action {
 	INTEL_GUC_ACTION_ENTER_S_STATE = 0x501,
 	INTEL_GUC_ACTION_EXIT_S_STATE = 0x502,
 	INTEL_GUC_ACTION_SLPC_REQUEST = 0x3003,
-	INTEL_GUC_ACTION_SAMPLE_FORCEWAKE = 0x3005,
 	INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000,
 	INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505,
 	INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER = 0x4506,
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 892c1315ce49..ab0789d66e06 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -502,10 +502,6 @@ static int __uc_init_hw(struct intel_uc *uc)
 
 	intel_huc_auth(huc);
 
-	ret = intel_guc_sample_forcewake(guc);
-	if (ret)
-		goto err_log_capture;
-
 	if (intel_uc_uses_guc_submission(uc))
 		intel_guc_submission_enable(guc);
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 05/20] drm/i915/guc: Keep strict GuC ABI definitions
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (3 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 04/20] drm/i915/guc: Remove sample_forcewake h2g action Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 06/20] drm/i915/guc: Drop guc->interrupts.enabled Matthew Brost
                   ` (14 subsequent siblings)
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

Our fwif.h file is now mix of strict firmware ABI definitions and
set of our helpers. In anticipation of upcoming changes to the GuC
interface try to keep them separate in smaller maintainable files.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
---
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |  51 +++++
 .../gt/uc/abi/guc_communication_ctb_abi.h     | 106 +++++++++
 .../gt/uc/abi/guc_communication_mmio_abi.h    |  52 +++++
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |  14 ++
 .../gpu/drm/i915/gt/uc/abi/guc_messages_abi.h |  21 ++
 drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   | 203 +-----------------
 6 files changed, 250 insertions(+), 197 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
new file mode 100644
index 000000000000..90efef8a73e4
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2014-2021 Intel Corporation
+ */
+
+#ifndef _ABI_GUC_ACTIONS_ABI_H
+#define _ABI_GUC_ACTIONS_ABI_H
+
+enum intel_guc_action {
+	INTEL_GUC_ACTION_DEFAULT = 0x0,
+	INTEL_GUC_ACTION_REQUEST_PREEMPTION = 0x2,
+	INTEL_GUC_ACTION_REQUEST_ENGINE_RESET = 0x3,
+	INTEL_GUC_ACTION_ALLOCATE_DOORBELL = 0x10,
+	INTEL_GUC_ACTION_DEALLOCATE_DOORBELL = 0x20,
+	INTEL_GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE = 0x30,
+	INTEL_GUC_ACTION_UK_LOG_ENABLE_LOGGING = 0x40,
+	INTEL_GUC_ACTION_FORCE_LOG_BUFFER_FLUSH = 0x302,
+	INTEL_GUC_ACTION_ENTER_S_STATE = 0x501,
+	INTEL_GUC_ACTION_EXIT_S_STATE = 0x502,
+	INTEL_GUC_ACTION_SLPC_REQUEST = 0x3003,
+	INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000,
+	INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505,
+	INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER = 0x4506,
+	INTEL_GUC_ACTION_LIMIT
+};
+
+enum intel_guc_preempt_options {
+	INTEL_GUC_PREEMPT_OPTION_DROP_WORK_Q = 0x4,
+	INTEL_GUC_PREEMPT_OPTION_DROP_SUBMIT_Q = 0x8,
+};
+
+enum intel_guc_report_status {
+	INTEL_GUC_REPORT_STATUS_UNKNOWN = 0x0,
+	INTEL_GUC_REPORT_STATUS_ACKED = 0x1,
+	INTEL_GUC_REPORT_STATUS_ERROR = 0x2,
+	INTEL_GUC_REPORT_STATUS_COMPLETE = 0x4,
+};
+
+enum intel_guc_sleep_state_status {
+	INTEL_GUC_SLEEP_STATE_SUCCESS = 0x1,
+	INTEL_GUC_SLEEP_STATE_PREEMPT_TO_IDLE_FAILED = 0x2,
+	INTEL_GUC_SLEEP_STATE_ENGINE_RESET_FAILED = 0x3
+#define INTEL_GUC_SLEEP_STATE_INVALID_MASK 0x80000000
+};
+
+#define GUC_LOG_CONTROL_LOGGING_ENABLED	(1 << 0)
+#define GUC_LOG_CONTROL_VERBOSITY_SHIFT	4
+#define GUC_LOG_CONTROL_VERBOSITY_MASK	(0xF << GUC_LOG_CONTROL_VERBOSITY_SHIFT)
+#define GUC_LOG_CONTROL_DEFAULT_LOGGING	(1 << 8)
+
+#endif /* _ABI_GUC_ACTIONS_ABI_H */
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h
new file mode 100644
index 000000000000..ebd8c3e0e4bb
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h
@@ -0,0 +1,106 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2014-2021 Intel Corporation
+ */
+
+#ifndef _ABI_GUC_COMMUNICATION_CTB_ABI_H
+#define _ABI_GUC_COMMUNICATION_CTB_ABI_H
+
+#include <linux/types.h>
+
+/**
+ * DOC: CTB based communication
+ *
+ * The CTB (command transport buffer) communication between Host and GuC
+ * is based on u32 data stream written to the shared buffer. One buffer can
+ * be used to transmit data only in one direction (one-directional channel).
+ *
+ * Current status of the each buffer is stored in the buffer descriptor.
+ * Buffer descriptor holds tail and head fields that represents active data
+ * stream. The tail field is updated by the data producer (sender), and head
+ * field is updated by the data consumer (receiver)::
+ *
+ *      +------------+
+ *      | DESCRIPTOR |          +=================+============+========+
+ *      +============+          |                 | MESSAGE(s) |        |
+ *      | address    |--------->+=================+============+========+
+ *      +------------+
+ *      | head       |          ^-----head--------^
+ *      +------------+
+ *      | tail       |          ^---------tail-----------------^
+ *      +------------+
+ *      | size       |          ^---------------size--------------------^
+ *      +------------+
+ *
+ * Each message in data stream starts with the single u32 treated as a header,
+ * followed by optional set of u32 data that makes message specific payload::
+ *
+ *      +------------+---------+---------+---------+
+ *      |         MESSAGE                          |
+ *      +------------+---------+---------+---------+
+ *      |   msg[0]   |   [1]   |   ...   |  [n-1]  |
+ *      +------------+---------+---------+---------+
+ *      |   MESSAGE  |       MESSAGE PAYLOAD       |
+ *      +   HEADER   +---------+---------+---------+
+ *      |            |    0    |   ...   |    n    |
+ *      +======+=====+=========+=========+=========+
+ *      | 31:16| code|         |         |         |
+ *      +------+-----+         |         |         |
+ *      |  15:5|flags|         |         |         |
+ *      +------+-----+         |         |         |
+ *      |   4:0|  len|         |         |         |
+ *      +------+-----+---------+---------+---------+
+ *
+ *                   ^-------------len-------------^
+ *
+ * The message header consists of:
+ *
+ * - **len**, indicates length of the message payload (in u32)
+ * - **code**, indicates message code
+ * - **flags**, holds various bits to control message handling
+ */
+
+/*
+ * Describes single command transport buffer.
+ * Used by both guc-master and clients.
+ */
+struct guc_ct_buffer_desc {
+	u32 addr;		/* gfx address */
+	u64 host_private;	/* host private data */
+	u32 size;		/* size in bytes */
+	u32 head;		/* offset updated by GuC*/
+	u32 tail;		/* offset updated by owner */
+	u32 is_in_error;	/* error indicator */
+	u32 fence;		/* fence updated by GuC */
+	u32 status;		/* status updated by GuC */
+	u32 owner;		/* id of the channel owner */
+	u32 owner_sub_id;	/* owner-defined field for extra tracking */
+	u32 reserved[5];
+} __packed;
+
+/* Type of command transport buffer */
+#define INTEL_GUC_CT_BUFFER_TYPE_SEND	0x0u
+#define INTEL_GUC_CT_BUFFER_TYPE_RECV	0x1u
+
+/*
+ * Definition of the command transport message header (DW0)
+ *
+ * bit[4..0]	message len (in dwords)
+ * bit[7..5]	reserved
+ * bit[8]	response (G2H only)
+ * bit[8]	write fence to desc (H2G only)
+ * bit[9]	write status to H2G buff (H2G only)
+ * bit[10]	send status back via G2H (H2G only)
+ * bit[15..11]	reserved
+ * bit[31..16]	action code
+ */
+#define GUC_CT_MSG_LEN_SHIFT			0
+#define GUC_CT_MSG_LEN_MASK			0x1F
+#define GUC_CT_MSG_IS_RESPONSE			(1 << 8)
+#define GUC_CT_MSG_WRITE_FENCE_TO_DESC		(1 << 8)
+#define GUC_CT_MSG_WRITE_STATUS_TO_BUFF		(1 << 9)
+#define GUC_CT_MSG_SEND_STATUS			(1 << 10)
+#define GUC_CT_MSG_ACTION_SHIFT			16
+#define GUC_CT_MSG_ACTION_MASK			0xFFFF
+
+#endif /* _ABI_GUC_COMMUNICATION_CTB_ABI_H */
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h
new file mode 100644
index 000000000000..be066a62e9e0
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2014-2021 Intel Corporation
+ */
+
+#ifndef _ABI_GUC_COMMUNICATION_MMIO_ABI_H
+#define _ABI_GUC_COMMUNICATION_MMIO_ABI_H
+
+/**
+ * DOC: MMIO based communication
+ *
+ * The MMIO based communication between Host and GuC uses software scratch
+ * registers, where first register holds data treated as message header,
+ * and other registers are used to hold message payload.
+ *
+ * For Gen9+, GuC uses software scratch registers 0xC180-0xC1B8,
+ * but no H2G command takes more than 8 parameters and the GuC FW
+ * itself uses an 8-element array to store the H2G message.
+ *
+ *      +-----------+---------+---------+---------+
+ *      |  MMIO[0]  | MMIO[1] |   ...   | MMIO[n] |
+ *      +-----------+---------+---------+---------+
+ *      | header    |      optional payload       |
+ *      +======+====+=========+=========+=========+
+ *      | 31:28|type|         |         |         |
+ *      +------+----+         |         |         |
+ *      | 27:16|data|         |         |         |
+ *      +------+----+         |         |         |
+ *      |  15:0|code|         |         |         |
+ *      +------+----+---------+---------+---------+
+ *
+ * The message header consists of:
+ *
+ * - **type**, indicates message type
+ * - **code**, indicates message code, is specific for **type**
+ * - **data**, indicates message data, optional, depends on **code**
+ *
+ * The following message **types** are supported:
+ *
+ * - **REQUEST**, indicates Host-to-GuC request, requested GuC action code
+ *   must be priovided in **code** field. Optional action specific parameters
+ *   can be provided in remaining payload registers or **data** field.
+ *
+ * - **RESPONSE**, indicates GuC-to-Host response from earlier GuC request,
+ *   action response status will be provided in **code** field. Optional
+ *   response data can be returned in remaining payload registers or **data**
+ *   field.
+ */
+
+#define GUC_MAX_MMIO_MSG_LEN		8
+
+#endif /* _ABI_GUC_COMMUNICATION_MMIO_ABI_H */
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
new file mode 100644
index 000000000000..488b6061ee89
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2014-2021 Intel Corporation
+ */
+
+#ifndef _ABI_GUC_ERRORS_ABI_H
+#define _ABI_GUC_ERRORS_ABI_H
+
+enum intel_guc_response_status {
+	INTEL_GUC_RESPONSE_STATUS_SUCCESS = 0x0,
+	INTEL_GUC_RESPONSE_STATUS_GENERIC_FAIL = 0xF000,
+};
+
+#endif /* _ABI_GUC_ERRORS_ABI_H */
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h
new file mode 100644
index 000000000000..775e21f3058c
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2014-2021 Intel Corporation
+ */
+
+#ifndef _ABI_GUC_MESSAGES_ABI_H
+#define _ABI_GUC_MESSAGES_ABI_H
+
+#define INTEL_GUC_MSG_TYPE_SHIFT	28
+#define INTEL_GUC_MSG_TYPE_MASK		(0xF << INTEL_GUC_MSG_TYPE_SHIFT)
+#define INTEL_GUC_MSG_DATA_SHIFT	16
+#define INTEL_GUC_MSG_DATA_MASK		(0xFFF << INTEL_GUC_MSG_DATA_SHIFT)
+#define INTEL_GUC_MSG_CODE_SHIFT	0
+#define INTEL_GUC_MSG_CODE_MASK		(0xFFFF << INTEL_GUC_MSG_CODE_SHIFT)
+
+enum intel_guc_msg_type {
+	INTEL_GUC_MSG_TYPE_REQUEST = 0x0,
+	INTEL_GUC_MSG_TYPE_RESPONSE = 0xF,
+};
+
+#endif /* _ABI_GUC_MESSAGES_ABI_H */
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
index 0f9afcde1d0b..9bf35240e723 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
@@ -10,6 +10,12 @@
 #include <linux/compiler.h>
 #include <linux/types.h>
 
+#include "abi/guc_actions_abi.h"
+#include "abi/guc_errors_abi.h"
+#include "abi/guc_communication_mmio_abi.h"
+#include "abi/guc_communication_ctb_abi.h"
+#include "abi/guc_messages_abi.h"
+
 #define GUC_CLIENT_PRIORITY_KMD_HIGH	0
 #define GUC_CLIENT_PRIORITY_HIGH	1
 #define GUC_CLIENT_PRIORITY_KMD_NORMAL	2
@@ -207,101 +213,6 @@ struct guc_stage_desc {
 	u64 desc_private;
 } __packed;
 
-/**
- * DOC: CTB based communication
- *
- * The CTB (command transport buffer) communication between Host and GuC
- * is based on u32 data stream written to the shared buffer. One buffer can
- * be used to transmit data only in one direction (one-directional channel).
- *
- * Current status of the each buffer is stored in the buffer descriptor.
- * Buffer descriptor holds tail and head fields that represents active data
- * stream. The tail field is updated by the data producer (sender), and head
- * field is updated by the data consumer (receiver)::
- *
- *      +------------+
- *      | DESCRIPTOR |          +=================+============+========+
- *      +============+          |                 | MESSAGE(s) |        |
- *      | address    |--------->+=================+============+========+
- *      +------------+
- *      | head       |          ^-----head--------^
- *      +------------+
- *      | tail       |          ^---------tail-----------------^
- *      +------------+
- *      | size       |          ^---------------size--------------------^
- *      +------------+
- *
- * Each message in data stream starts with the single u32 treated as a header,
- * followed by optional set of u32 data that makes message specific payload::
- *
- *      +------------+---------+---------+---------+
- *      |         MESSAGE                          |
- *      +------------+---------+---------+---------+
- *      |   msg[0]   |   [1]   |   ...   |  [n-1]  |
- *      +------------+---------+---------+---------+
- *      |   MESSAGE  |       MESSAGE PAYLOAD       |
- *      +   HEADER   +---------+---------+---------+
- *      |            |    0    |   ...   |    n    |
- *      +======+=====+=========+=========+=========+
- *      | 31:16| code|         |         |         |
- *      +------+-----+         |         |         |
- *      |  15:5|flags|         |         |         |
- *      +------+-----+         |         |         |
- *      |   4:0|  len|         |         |         |
- *      +------+-----+---------+---------+---------+
- *
- *                   ^-------------len-------------^
- *
- * The message header consists of:
- *
- * - **len**, indicates length of the message payload (in u32)
- * - **code**, indicates message code
- * - **flags**, holds various bits to control message handling
- */
-
-/*
- * Describes single command transport buffer.
- * Used by both guc-master and clients.
- */
-struct guc_ct_buffer_desc {
-	u32 addr;		/* gfx address */
-	u64 host_private;	/* host private data */
-	u32 size;		/* size in bytes */
-	u32 head;		/* offset updated by GuC*/
-	u32 tail;		/* offset updated by owner */
-	u32 is_in_error;	/* error indicator */
-	u32 fence;		/* fence updated by GuC */
-	u32 status;		/* status updated by GuC */
-	u32 owner;		/* id of the channel owner */
-	u32 owner_sub_id;	/* owner-defined field for extra tracking */
-	u32 reserved[5];
-} __packed;
-
-/* Type of command transport buffer */
-#define INTEL_GUC_CT_BUFFER_TYPE_SEND	0x0u
-#define INTEL_GUC_CT_BUFFER_TYPE_RECV	0x1u
-
-/*
- * Definition of the command transport message header (DW0)
- *
- * bit[4..0]	message len (in dwords)
- * bit[7..5]	reserved
- * bit[8]	response (G2H only)
- * bit[8]	write fence to desc (H2G only)
- * bit[9]	write status to H2G buff (H2G only)
- * bit[10]	send status back via G2H (H2G only)
- * bit[15..11]	reserved
- * bit[31..16]	action code
- */
-#define GUC_CT_MSG_LEN_SHIFT			0
-#define GUC_CT_MSG_LEN_MASK			0x1F
-#define GUC_CT_MSG_IS_RESPONSE			(1 << 8)
-#define GUC_CT_MSG_WRITE_FENCE_TO_DESC		(1 << 8)
-#define GUC_CT_MSG_WRITE_STATUS_TO_BUFF		(1 << 9)
-#define GUC_CT_MSG_SEND_STATUS			(1 << 10)
-#define GUC_CT_MSG_ACTION_SHIFT			16
-#define GUC_CT_MSG_ACTION_MASK			0xFFFF
-
 #define GUC_POWER_UNSPECIFIED	0
 #define GUC_POWER_D0		1
 #define GUC_POWER_D1		2
@@ -477,119 +388,17 @@ struct guc_shared_ctx_data {
 	struct guc_ctx_report preempt_ctx_report[GUC_MAX_ENGINES_NUM];
 } __packed;
 
-/**
- * DOC: MMIO based communication
- *
- * The MMIO based communication between Host and GuC uses software scratch
- * registers, where first register holds data treated as message header,
- * and other registers are used to hold message payload.
- *
- * For Gen9+, GuC uses software scratch registers 0xC180-0xC1B8,
- * but no H2G command takes more than 8 parameters and the GuC FW
- * itself uses an 8-element array to store the H2G message.
- *
- *      +-----------+---------+---------+---------+
- *      |  MMIO[0]  | MMIO[1] |   ...   | MMIO[n] |
- *      +-----------+---------+---------+---------+
- *      | header    |      optional payload       |
- *      +======+====+=========+=========+=========+
- *      | 31:28|type|         |         |         |
- *      +------+----+         |         |         |
- *      | 27:16|data|         |         |         |
- *      +------+----+         |         |         |
- *      |  15:0|code|         |         |         |
- *      +------+----+---------+---------+---------+
- *
- * The message header consists of:
- *
- * - **type**, indicates message type
- * - **code**, indicates message code, is specific for **type**
- * - **data**, indicates message data, optional, depends on **code**
- *
- * The following message **types** are supported:
- *
- * - **REQUEST**, indicates Host-to-GuC request, requested GuC action code
- *   must be priovided in **code** field. Optional action specific parameters
- *   can be provided in remaining payload registers or **data** field.
- *
- * - **RESPONSE**, indicates GuC-to-Host response from earlier GuC request,
- *   action response status will be provided in **code** field. Optional
- *   response data can be returned in remaining payload registers or **data**
- *   field.
- */
-
-#define GUC_MAX_MMIO_MSG_LEN		8
-
-#define INTEL_GUC_MSG_TYPE_SHIFT	28
-#define INTEL_GUC_MSG_TYPE_MASK		(0xF << INTEL_GUC_MSG_TYPE_SHIFT)
-#define INTEL_GUC_MSG_DATA_SHIFT	16
-#define INTEL_GUC_MSG_DATA_MASK		(0xFFF << INTEL_GUC_MSG_DATA_SHIFT)
-#define INTEL_GUC_MSG_CODE_SHIFT	0
-#define INTEL_GUC_MSG_CODE_MASK		(0xFFFF << INTEL_GUC_MSG_CODE_SHIFT)
-
 #define __INTEL_GUC_MSG_GET(T, m) \
 	(((m) & INTEL_GUC_MSG_ ## T ## _MASK) >> INTEL_GUC_MSG_ ## T ## _SHIFT)
 #define INTEL_GUC_MSG_TO_TYPE(m)	__INTEL_GUC_MSG_GET(TYPE, m)
 #define INTEL_GUC_MSG_TO_DATA(m)	__INTEL_GUC_MSG_GET(DATA, m)
 #define INTEL_GUC_MSG_TO_CODE(m)	__INTEL_GUC_MSG_GET(CODE, m)
 
-enum intel_guc_msg_type {
-	INTEL_GUC_MSG_TYPE_REQUEST = 0x0,
-	INTEL_GUC_MSG_TYPE_RESPONSE = 0xF,
-};
-
 #define __INTEL_GUC_MSG_TYPE_IS(T, m) \
 	(INTEL_GUC_MSG_TO_TYPE(m) == INTEL_GUC_MSG_TYPE_ ## T)
 #define INTEL_GUC_MSG_IS_REQUEST(m)	__INTEL_GUC_MSG_TYPE_IS(REQUEST, m)
 #define INTEL_GUC_MSG_IS_RESPONSE(m)	__INTEL_GUC_MSG_TYPE_IS(RESPONSE, m)
 
-enum intel_guc_action {
-	INTEL_GUC_ACTION_DEFAULT = 0x0,
-	INTEL_GUC_ACTION_REQUEST_PREEMPTION = 0x2,
-	INTEL_GUC_ACTION_REQUEST_ENGINE_RESET = 0x3,
-	INTEL_GUC_ACTION_ALLOCATE_DOORBELL = 0x10,
-	INTEL_GUC_ACTION_DEALLOCATE_DOORBELL = 0x20,
-	INTEL_GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE = 0x30,
-	INTEL_GUC_ACTION_UK_LOG_ENABLE_LOGGING = 0x40,
-	INTEL_GUC_ACTION_FORCE_LOG_BUFFER_FLUSH = 0x302,
-	INTEL_GUC_ACTION_ENTER_S_STATE = 0x501,
-	INTEL_GUC_ACTION_EXIT_S_STATE = 0x502,
-	INTEL_GUC_ACTION_SLPC_REQUEST = 0x3003,
-	INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000,
-	INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505,
-	INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER = 0x4506,
-	INTEL_GUC_ACTION_LIMIT
-};
-
-enum intel_guc_preempt_options {
-	INTEL_GUC_PREEMPT_OPTION_DROP_WORK_Q = 0x4,
-	INTEL_GUC_PREEMPT_OPTION_DROP_SUBMIT_Q = 0x8,
-};
-
-enum intel_guc_report_status {
-	INTEL_GUC_REPORT_STATUS_UNKNOWN = 0x0,
-	INTEL_GUC_REPORT_STATUS_ACKED = 0x1,
-	INTEL_GUC_REPORT_STATUS_ERROR = 0x2,
-	INTEL_GUC_REPORT_STATUS_COMPLETE = 0x4,
-};
-
-enum intel_guc_sleep_state_status {
-	INTEL_GUC_SLEEP_STATE_SUCCESS = 0x1,
-	INTEL_GUC_SLEEP_STATE_PREEMPT_TO_IDLE_FAILED = 0x2,
-	INTEL_GUC_SLEEP_STATE_ENGINE_RESET_FAILED = 0x3
-#define INTEL_GUC_SLEEP_STATE_INVALID_MASK 0x80000000
-};
-
-#define GUC_LOG_CONTROL_LOGGING_ENABLED	(1 << 0)
-#define GUC_LOG_CONTROL_VERBOSITY_SHIFT	4
-#define GUC_LOG_CONTROL_VERBOSITY_MASK	(0xF << GUC_LOG_CONTROL_VERBOSITY_SHIFT)
-#define GUC_LOG_CONTROL_DEFAULT_LOGGING	(1 << 8)
-
-enum intel_guc_response_status {
-	INTEL_GUC_RESPONSE_STATUS_SUCCESS = 0x0,
-	INTEL_GUC_RESPONSE_STATUS_GENERIC_FAIL = 0xF000,
-};
-
 #define INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(m) \
 	 (typecheck(u32, (m)) && \
 	  ((m) & (INTEL_GUC_MSG_TYPE_MASK | INTEL_GUC_MSG_CODE_MASK)) == \
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 06/20] drm/i915/guc: Drop guc->interrupts.enabled
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (4 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 05/20] drm/i915/guc: Keep strict GuC ABI definitions Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 07/20] drm/i915/guc: Stop using fence/status from CTB descriptor Matthew Brost
                   ` (13 subsequent siblings)
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

Drop the variable guc->interrupts.enabled as this variable is just
leading to bugs creeping into the code.

e.g. A full GPU reset disables the GuC interrupts but forgot to clear
guc->interrupts.enabled, guc->interrupts.enabled being true suppresses
interrupts from getting re-enabled and now we are broken.

It is harmless to enable interrupt while already enabled so let's just
delete this variable to avoid bugs like this going forward.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc.c | 27 +++++++++-----------------
 drivers/gpu/drm/i915/gt/uc/intel_guc.h |  1 -
 2 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index ab2c8fe8cdfa..18da9ed15728 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -96,12 +96,9 @@ static void gen9_enable_guc_interrupts(struct intel_guc *guc)
 	assert_rpm_wakelock_held(&gt->i915->runtime_pm);
 
 	spin_lock_irq(&gt->irq_lock);
-	if (!guc->interrupts.enabled) {
-		WARN_ON_ONCE(intel_uncore_read(gt->uncore, GEN8_GT_IIR(2)) &
-			     gt->pm_guc_events);
-		guc->interrupts.enabled = true;
-		gen6_gt_pm_enable_irq(gt, gt->pm_guc_events);
-	}
+	WARN_ON_ONCE(intel_uncore_read(gt->uncore, GEN8_GT_IIR(2)) &
+		     gt->pm_guc_events);
+	gen6_gt_pm_enable_irq(gt, gt->pm_guc_events);
 	spin_unlock_irq(&gt->irq_lock);
 }
 
@@ -112,7 +109,6 @@ static void gen9_disable_guc_interrupts(struct intel_guc *guc)
 	assert_rpm_wakelock_held(&gt->i915->runtime_pm);
 
 	spin_lock_irq(&gt->irq_lock);
-	guc->interrupts.enabled = false;
 
 	gen6_gt_pm_disable_irq(gt, gt->pm_guc_events);
 
@@ -134,18 +130,14 @@ static void gen11_reset_guc_interrupts(struct intel_guc *guc)
 static void gen11_enable_guc_interrupts(struct intel_guc *guc)
 {
 	struct intel_gt *gt = guc_to_gt(guc);
+	u32 events = REG_FIELD_PREP(ENGINE1_MASK, GUC_INTR_GUC2HOST);
 
 	spin_lock_irq(&gt->irq_lock);
-	if (!guc->interrupts.enabled) {
-		u32 events = REG_FIELD_PREP(ENGINE1_MASK, GUC_INTR_GUC2HOST);
-
-		WARN_ON_ONCE(gen11_gt_reset_one_iir(gt, 0, GEN11_GUC));
-		intel_uncore_write(gt->uncore,
-				   GEN11_GUC_SG_INTR_ENABLE, events);
-		intel_uncore_write(gt->uncore,
-				   GEN11_GUC_SG_INTR_MASK, ~events);
-		guc->interrupts.enabled = true;
-	}
+	WARN_ON_ONCE(gen11_gt_reset_one_iir(gt, 0, GEN11_GUC));
+	intel_uncore_write(gt->uncore,
+			   GEN11_GUC_SG_INTR_ENABLE, events);
+	intel_uncore_write(gt->uncore,
+			   GEN11_GUC_SG_INTR_MASK, ~events);
 	spin_unlock_irq(&gt->irq_lock);
 }
 
@@ -154,7 +146,6 @@ static void gen11_disable_guc_interrupts(struct intel_guc *guc)
 	struct intel_gt *gt = guc_to_gt(guc);
 
 	spin_lock_irq(&gt->irq_lock);
-	guc->interrupts.enabled = false;
 
 	intel_uncore_write(gt->uncore, GEN11_GUC_SG_INTR_MASK, ~0);
 	intel_uncore_write(gt->uncore, GEN11_GUC_SG_INTR_ENABLE, 0);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index c20f3839de12..4abc59f6f3cd 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -33,7 +33,6 @@ struct intel_guc {
 	unsigned int msg_enabled_mask;
 
 	struct {
-		bool enabled;
 		void (*reset)(struct intel_guc *guc);
 		void (*enable)(struct intel_guc *guc);
 		void (*disable)(struct intel_guc *guc);
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 07/20] drm/i915/guc: Stop using fence/status from CTB descriptor
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (5 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 06/20] drm/i915/guc: Drop guc->interrupts.enabled Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 08/20] drm/i915: Promote ptrdiff() to i915_utils.h Matthew Brost
                   ` (12 subsequent siblings)
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

Stop using fence/status from CTB descriptor as future GuC ABI will
no longer support replies over CTB descriptor.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 .../gt/uc/abi/guc_communication_ctb_abi.h     |  4 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c     | 72 ++-----------------
 2 files changed, 6 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h
index ebd8c3e0e4bb..d38935f47ecf 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h
@@ -71,8 +71,8 @@ struct guc_ct_buffer_desc {
 	u32 head;		/* offset updated by GuC*/
 	u32 tail;		/* offset updated by owner */
 	u32 is_in_error;	/* error indicator */
-	u32 fence;		/* fence updated by GuC */
-	u32 status;		/* status updated by GuC */
+	u32 reserved1;
+	u32 reserved2;
 	u32 owner;		/* id of the channel owner */
 	u32 owner_sub_id;	/* owner-defined field for extra tracking */
 	u32 reserved[5];
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 72b48ac9271a..d08fa9879921 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -90,13 +90,6 @@ static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc,
 	desc->owner = CTB_OWNER_HOST;
 }
 
-static void guc_ct_buffer_desc_reset(struct guc_ct_buffer_desc *desc)
-{
-	desc->head = 0;
-	desc->tail = 0;
-	desc->is_in_error = 0;
-}
-
 static int guc_action_register_ct_buffer(struct intel_guc *guc,
 					 u32 desc_addr,
 					 u32 type)
@@ -315,8 +308,7 @@ static u32 ct_get_next_fence(struct intel_guc_ct *ct)
 static int ct_write(struct intel_guc_ct *ct,
 		    const u32 *action,
 		    u32 len /* in dwords */,
-		    u32 fence,
-		    bool want_response)
+		    u32 fence)
 {
 	struct intel_guc_ct_buffer *ctb = &ct->ctbs[CTB_SEND];
 	struct guc_ct_buffer_desc *desc = ctb->desc;
@@ -360,8 +352,7 @@ static int ct_write(struct intel_guc_ct *ct,
 	 * DW2+: action data
 	 */
 	header = (len << GUC_CT_MSG_LEN_SHIFT) |
-		 (GUC_CT_MSG_WRITE_FENCE_TO_DESC) |
-		 (want_response ? GUC_CT_MSG_SEND_STATUS : 0) |
+		 GUC_CT_MSG_SEND_STATUS |
 		 (action[0] << GUC_CT_MSG_ACTION_SHIFT);
 
 	CT_DEBUG(ct, "writing %*ph %*ph %*ph\n",
@@ -390,56 +381,6 @@ static int ct_write(struct intel_guc_ct *ct,
 	return -EPIPE;
 }
 
-/**
- * wait_for_ctb_desc_update - Wait for the CT buffer descriptor update.
- * @desc:	buffer descriptor
- * @fence:	response fence
- * @status:	placeholder for status
- *
- * Guc will update CT buffer descriptor with new fence and status
- * after processing the command identified by the fence. Wait for
- * specified fence and then read from the descriptor status of the
- * command.
- *
- * Return:
- * *	0 response received (status is valid)
- * *	-ETIMEDOUT no response within hardcoded timeout
- * *	-EPROTO no response, CT buffer is in error
- */
-static int wait_for_ctb_desc_update(struct guc_ct_buffer_desc *desc,
-				    u32 fence,
-				    u32 *status)
-{
-	int err;
-
-	/*
-	 * Fast commands should complete in less than 10us, so sample quickly
-	 * up to that length of time, then switch to a slower sleep-wait loop.
-	 * No GuC command should ever take longer than 10ms.
-	 */
-#define done (READ_ONCE(desc->fence) == fence)
-	err = wait_for_us(done, 10);
-	if (err)
-		err = wait_for(done, 10);
-#undef done
-
-	if (unlikely(err)) {
-		DRM_ERROR("CT: fence %u failed; reported fence=%u\n",
-			  fence, desc->fence);
-
-		if (WARN_ON(desc->is_in_error)) {
-			/* Something went wrong with the messaging, try to reset
-			 * the buffer and hope for the best
-			 */
-			guc_ct_buffer_desc_reset(desc);
-			err = -EPROTO;
-		}
-	}
-
-	*status = desc->status;
-	return err;
-}
-
 /**
  * wait_for_ct_request_update - Wait for CT request state update.
  * @req:	pointer to pending request
@@ -483,8 +424,6 @@ static int ct_send(struct intel_guc_ct *ct,
 		   u32 response_buf_size,
 		   u32 *status)
 {
-	struct intel_guc_ct_buffer *ctb = &ct->ctbs[CTB_SEND];
-	struct guc_ct_buffer_desc *desc = ctb->desc;
 	struct ct_request request;
 	unsigned long flags;
 	u32 fence;
@@ -505,16 +444,13 @@ static int ct_send(struct intel_guc_ct *ct,
 	list_add_tail(&request.link, &ct->requests.pending);
 	spin_unlock_irqrestore(&ct->requests.lock, flags);
 
-	err = ct_write(ct, action, len, fence, !!response_buf);
+	err = ct_write(ct, action, len, fence);
 	if (unlikely(err))
 		goto unlink;
 
 	intel_guc_notify(ct_to_guc(ct));
 
-	if (response_buf)
-		err = wait_for_ct_request_update(&request, status);
-	else
-		err = wait_for_ctb_desc_update(desc, fence, status);
+	err = wait_for_ct_request_update(&request, status);
 	if (unlikely(err))
 		goto unlink;
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 08/20] drm/i915: Promote ptrdiff() to i915_utils.h
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (6 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 07/20] drm/i915/guc: Stop using fence/status from CTB descriptor Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03 21:35   ` Daniel Vetter
  2021-06-03  5:16 ` [PATCH 09/20] drm/i915/guc: Only rely on own CTB size Matthew Brost
                   ` (11 subsequent siblings)
  19 siblings, 1 reply; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

Generic helpers should be placed in i915_utils.h.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/i915_utils.h | 5 +++++
 drivers/gpu/drm/i915/i915_vma.h   | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
index f02f52ab5070..5259edacde38 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -201,6 +201,11 @@ __check_struct_size(size_t base, size_t arr, size_t count, size_t *size)
 	__T;								\
 })
 
+static __always_inline ptrdiff_t ptrdiff(const void *a, const void *b)
+{
+	return a - b;
+}
+
 /*
  * container_of_user: Extract the superclass from a pointer to a member.
  *
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index dc6926d89626..eca452a9851f 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -151,11 +151,6 @@ static inline void i915_vma_put(struct i915_vma *vma)
 	i915_gem_object_put(vma->obj);
 }
 
-static __always_inline ptrdiff_t ptrdiff(const void *a, const void *b)
-{
-	return a - b;
-}
-
 static inline long
 i915_vma_compare(struct i915_vma *vma,
 		 struct i915_address_space *vm,
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/20] drm/i915: Promote ptrdiff() to i915_utils.h
  2021-06-03  5:16 ` [PATCH 08/20] drm/i915: Promote ptrdiff() to i915_utils.h Matthew Brost
@ 2021-06-03 21:35   ` Daniel Vetter
  2021-06-04  2:02     ` Matthew Brost
  0 siblings, 1 reply; 41+ messages in thread
From: Daniel Vetter @ 2021-06-03 21:35 UTC (permalink / raw)
  To: Matthew Brost; +Cc: daniel.vetter, intel-gfx, dri-devel

On Wed, Jun 02, 2021 at 10:16:18PM -0700, Matthew Brost wrote:
> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 
> Generic helpers should be placed in i915_utils.h.

Random rant, but we're _way_ too happy to just stuff random things into
i915_utils.h without trying to properly upstream it.

For thinks like this the general dumping ground would be kernel.h, there's
a few pointer helpers there already. Follow up patch maybe nice.
-Daniel

> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_utils.h | 5 +++++
>  drivers/gpu/drm/i915/i915_vma.h   | 5 -----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> index f02f52ab5070..5259edacde38 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -201,6 +201,11 @@ __check_struct_size(size_t base, size_t arr, size_t count, size_t *size)
>  	__T;								\
>  })
>  
> +static __always_inline ptrdiff_t ptrdiff(const void *a, const void *b)
> +{
> +	return a - b;
> +}
> +
>  /*
>   * container_of_user: Extract the superclass from a pointer to a member.
>   *
> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> index dc6926d89626..eca452a9851f 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -151,11 +151,6 @@ static inline void i915_vma_put(struct i915_vma *vma)
>  	i915_gem_object_put(vma->obj);
>  }
>  
> -static __always_inline ptrdiff_t ptrdiff(const void *a, const void *b)
> -{
> -	return a - b;
> -}
> -
>  static inline long
>  i915_vma_compare(struct i915_vma *vma,
>  		 struct i915_address_space *vm,
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/20] drm/i915: Promote ptrdiff() to i915_utils.h
  2021-06-03 21:35   ` Daniel Vetter
@ 2021-06-04  2:02     ` Matthew Brost
  2021-06-04  8:11       ` Daniel Vetter
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Brost @ 2021-06-04  2:02 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: daniel.vetter, intel-gfx, dri-devel

On Thu, Jun 03, 2021 at 11:35:28PM +0200, Daniel Vetter wrote:
> On Wed, Jun 02, 2021 at 10:16:18PM -0700, Matthew Brost wrote:
> > From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > 
> > Generic helpers should be placed in i915_utils.h.
> 
> Random rant, but we're _way_ too happy to just stuff random things into
> i915_utils.h without trying to properly upstream it.
> 
> For thinks like this the general dumping ground would be kernel.h, there's
> a few pointer helpers there already. Follow up patch maybe nice.
> -Daniel
> 

Sure. I've added this to a list of follow ups so this comment doesn't
get lost.

Matt

> > 
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_utils.h | 5 +++++
> >  drivers/gpu/drm/i915/i915_vma.h   | 5 -----
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> > index f02f52ab5070..5259edacde38 100644
> > --- a/drivers/gpu/drm/i915/i915_utils.h
> > +++ b/drivers/gpu/drm/i915/i915_utils.h
> > @@ -201,6 +201,11 @@ __check_struct_size(size_t base, size_t arr, size_t count, size_t *size)
> >  	__T;								\
> >  })
> >  
> > +static __always_inline ptrdiff_t ptrdiff(const void *a, const void *b)
> > +{
> > +	return a - b;
> > +}
> > +
> >  /*
> >   * container_of_user: Extract the superclass from a pointer to a member.
> >   *
> > diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> > index dc6926d89626..eca452a9851f 100644
> > --- a/drivers/gpu/drm/i915/i915_vma.h
> > +++ b/drivers/gpu/drm/i915/i915_vma.h
> > @@ -151,11 +151,6 @@ static inline void i915_vma_put(struct i915_vma *vma)
> >  	i915_gem_object_put(vma->obj);
> >  }
> >  
> > -static __always_inline ptrdiff_t ptrdiff(const void *a, const void *b)
> > -{
> > -	return a - b;
> > -}
> > -
> >  static inline long
> >  i915_vma_compare(struct i915_vma *vma,
> >  		 struct i915_address_space *vm,
> > -- 
> > 2.28.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/20] drm/i915: Promote ptrdiff() to i915_utils.h
  2021-06-04  2:02     ` Matthew Brost
@ 2021-06-04  8:11       ` Daniel Vetter
  0 siblings, 0 replies; 41+ messages in thread
From: Daniel Vetter @ 2021-06-04  8:11 UTC (permalink / raw)
  To: Matthew Brost; +Cc: daniel.vetter, intel-gfx, dri-devel

On Thu, Jun 03, 2021 at 07:02:57PM -0700, Matthew Brost wrote:
> On Thu, Jun 03, 2021 at 11:35:28PM +0200, Daniel Vetter wrote:
> > On Wed, Jun 02, 2021 at 10:16:18PM -0700, Matthew Brost wrote:
> > > From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > 
> > > Generic helpers should be placed in i915_utils.h.
> > 
> > Random rant, but we're _way_ too happy to just stuff random things into
> > i915_utils.h without trying to properly upstream it.
> > 
> > For thinks like this the general dumping ground would be kernel.h, there's
> > a few pointer helpers there already. Follow up patch maybe nice.
> > -Daniel
> > 
> 
> Sure. I've added this to a list of follow ups so this comment doesn't
> get lost.

btw I think we probably want to rename it, because it's not just the
well-defined C pointer difference. We convert to void * so we get the
difference in bytes. So maybe ptr_offset or ptrdiff_in_bytes or something
like that which makes it clear.
-Daniel

> 
> Matt
> 
> > > 
> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_utils.h | 5 +++++
> > >  drivers/gpu/drm/i915/i915_vma.h   | 5 -----
> > >  2 files changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> > > index f02f52ab5070..5259edacde38 100644
> > > --- a/drivers/gpu/drm/i915/i915_utils.h
> > > +++ b/drivers/gpu/drm/i915/i915_utils.h
> > > @@ -201,6 +201,11 @@ __check_struct_size(size_t base, size_t arr, size_t count, size_t *size)
> > >  	__T;								\
> > >  })
> > >  
> > > +static __always_inline ptrdiff_t ptrdiff(const void *a, const void *b)
> > > +{
> > > +	return a - b;
> > > +}
> > > +
> > >  /*
> > >   * container_of_user: Extract the superclass from a pointer to a member.
> > >   *
> > > diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> > > index dc6926d89626..eca452a9851f 100644
> > > --- a/drivers/gpu/drm/i915/i915_vma.h
> > > +++ b/drivers/gpu/drm/i915/i915_vma.h
> > > @@ -151,11 +151,6 @@ static inline void i915_vma_put(struct i915_vma *vma)
> > >  	i915_gem_object_put(vma->obj);
> > >  }
> > >  
> > > -static __always_inline ptrdiff_t ptrdiff(const void *a, const void *b)
> > > -{
> > > -	return a - b;
> > > -}
> > > -
> > >  static inline long
> > >  i915_vma_compare(struct i915_vma *vma,
> > >  		 struct i915_address_space *vm,
> > > -- 
> > > 2.28.0
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 09/20] drm/i915/guc: Only rely on own CTB size
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (7 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 08/20] drm/i915: Promote ptrdiff() to i915_utils.h Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 10/20] drm/i915/guc: Don't repeat CTB layout calculations Matthew Brost
                   ` (10 subsequent siblings)
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

In upcoming GuC firmware, CTB size will be removed from the CTB
descriptor so we must keep it locally for any calculations.

While around, improve some debug messages and helpers.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 55 +++++++++++++++++------
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h |  2 +
 2 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index d08fa9879921..079e1a160894 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -90,6 +90,24 @@ static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc,
 	desc->owner = CTB_OWNER_HOST;
 }
 
+static void guc_ct_buffer_reset(struct intel_guc_ct_buffer *ctb, u32 cmds_addr)
+{
+	guc_ct_buffer_desc_init(ctb->desc, cmds_addr, ctb->size);
+}
+
+static void guc_ct_buffer_init(struct intel_guc_ct_buffer *ctb,
+			       struct guc_ct_buffer_desc *desc,
+			       u32 *cmds, u32 size)
+{
+	GEM_BUG_ON(size % 4);
+
+	ctb->desc = desc;
+	ctb->cmds = cmds;
+	ctb->size = size;
+
+	guc_ct_buffer_reset(ctb, 0);
+}
+
 static int guc_action_register_ct_buffer(struct intel_guc *guc,
 					 u32 desc_addr,
 					 u32 type)
@@ -148,7 +166,10 @@ static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type)
 int intel_guc_ct_init(struct intel_guc_ct *ct)
 {
 	struct intel_guc *guc = ct_to_guc(ct);
+	struct guc_ct_buffer_desc *desc;
+	u32 blob_size;
 	void *blob;
+	u32 *cmds;
 	int err;
 	int i;
 
@@ -176,19 +197,24 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 	 * other code will need updating as well.
 	 */
 
-	err = intel_guc_allocate_and_map_vma(guc, PAGE_SIZE, &ct->vma, &blob);
+	blob_size = PAGE_SIZE;
+	err = intel_guc_allocate_and_map_vma(guc, blob_size, &ct->vma, &blob);
 	if (unlikely(err)) {
-		CT_ERROR(ct, "Failed to allocate CT channel (err=%d)\n", err);
+		CT_PROBE_ERROR(ct, "Failed to allocate %u for CTB data (%pe)\n",
+			       blob_size, ERR_PTR(err));
 		return err;
 	}
 
-	CT_DEBUG(ct, "vma base=%#x\n", intel_guc_ggtt_offset(guc, ct->vma));
+	CT_DEBUG(ct, "base=%#x size=%u\n", intel_guc_ggtt_offset(guc, ct->vma), blob_size);
 
 	/* store pointers to desc and cmds */
 	for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {
 		GEM_BUG_ON((i !=  CTB_SEND) && (i != CTB_RECV));
-		ct->ctbs[i].desc = blob + PAGE_SIZE/4 * i;
-		ct->ctbs[i].cmds = blob + PAGE_SIZE/4 * i + PAGE_SIZE/2;
+
+		desc = blob + PAGE_SIZE / 4 * i;
+		cmds = blob + PAGE_SIZE / 4 * i + PAGE_SIZE / 2;
+
+		guc_ct_buffer_init(&ct->ctbs[i], desc, cmds, PAGE_SIZE / 4);
 	}
 
 	return 0;
@@ -217,7 +243,7 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct)
 int intel_guc_ct_enable(struct intel_guc_ct *ct)
 {
 	struct intel_guc *guc = ct_to_guc(ct);
-	u32 base, cmds, size;
+	u32 base, cmds;
 	int err;
 	int i;
 
@@ -232,10 +258,11 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 	 */
 	for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {
 		GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));
+
 		cmds = base + PAGE_SIZE / 4 * i + PAGE_SIZE / 2;
-		size = PAGE_SIZE / 4;
-		CT_DEBUG(ct, "%d: addr=%#x size=%u\n", i, cmds, size);
-		guc_ct_buffer_desc_init(ct->ctbs[i].desc, cmds, size);
+		CT_DEBUG(ct, "%d: cmds addr=%#x\n", i, cmds);
+
+		guc_ct_buffer_reset(&ct->ctbs[i], cmds);
 	}
 
 	/*
@@ -259,7 +286,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 err_deregister:
 	ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV);
 err_out:
-	CT_PROBE_ERROR(ct, "Failed to open channel (err=%d)\n", err);
+	CT_PROBE_ERROR(ct, "Failed to enable CTB (%pe)\n", ERR_PTR(err));
 	return err;
 }
 
@@ -314,7 +341,7 @@ static int ct_write(struct intel_guc_ct *ct,
 	struct guc_ct_buffer_desc *desc = ctb->desc;
 	u32 head = desc->head;
 	u32 tail = desc->tail;
-	u32 size = desc->size;
+	u32 size = ctb->size;
 	u32 used;
 	u32 header;
 	u32 *cmds = ctb->cmds;
@@ -323,7 +350,7 @@ static int ct_write(struct intel_guc_ct *ct,
 	if (unlikely(desc->is_in_error))
 		return -EPIPE;
 
-	if (unlikely(!IS_ALIGNED(head | tail | size, 4) ||
+	if (unlikely(!IS_ALIGNED(head | tail, 4) ||
 		     (tail | head) >= size))
 		goto corrupted;
 
@@ -530,7 +557,7 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data)
 	struct guc_ct_buffer_desc *desc = ctb->desc;
 	u32 head = desc->head;
 	u32 tail = desc->tail;
-	u32 size = desc->size;
+	u32 size = ctb->size;
 	u32 *cmds = ctb->cmds;
 	s32 available;
 	unsigned int len;
@@ -539,7 +566,7 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data)
 	if (unlikely(desc->is_in_error))
 		return -EPIPE;
 
-	if (unlikely(!IS_ALIGNED(head | tail | size, 4) ||
+	if (unlikely(!IS_ALIGNED(head | tail, 4) ||
 		     (tail | head) >= size))
 		goto corrupted;
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
index 494a51a5200f..4009e2dd0de4 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
@@ -29,10 +29,12 @@ struct intel_guc;
  *
  * @desc: pointer to the buffer descriptor
  * @cmds: pointer to the commands buffer
+ * @size: size of the commands buffer
  */
 struct intel_guc_ct_buffer {
 	struct guc_ct_buffer_desc *desc;
 	u32 *cmds;
+	u32 size;
 };
 
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 10/20] drm/i915/guc: Don't repeat CTB layout calculations
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (8 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 09/20] drm/i915/guc: Only rely on own CTB size Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 11/20] drm/i915/guc: Replace CTB array with explicit members Matthew Brost
                   ` (9 subsequent siblings)
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

We can retrieve offsets to cmds buffers and descriptor from
actual pointers that we already keep locally.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 079e1a160894..34c582105860 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -244,6 +244,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 {
 	struct intel_guc *guc = ct_to_guc(ct);
 	u32 base, cmds;
+	void *blob;
 	int err;
 	int i;
 
@@ -251,15 +252,18 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 
 	/* vma should be already allocated and map'ed */
 	GEM_BUG_ON(!ct->vma);
+	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(ct->vma->obj));
 	base = intel_guc_ggtt_offset(guc, ct->vma);
 
-	/* (re)initialize descriptors
-	 * cmds buffers are in the second half of the blob page
-	 */
+	/* blob should start with send descriptor */
+	blob = __px_vaddr(ct->vma->obj);
+	GEM_BUG_ON(blob != ct->ctbs[CTB_SEND].desc);
+
+	/* (re)initialize descriptors */
 	for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {
 		GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));
 
-		cmds = base + PAGE_SIZE / 4 * i + PAGE_SIZE / 2;
+		cmds = base + ptrdiff(ct->ctbs[i].cmds, blob);
 		CT_DEBUG(ct, "%d: cmds addr=%#x\n", i, cmds);
 
 		guc_ct_buffer_reset(&ct->ctbs[i], cmds);
@@ -269,12 +273,12 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 	 * Register both CT buffers starting with RECV buffer.
 	 * Descriptors are in first half of the blob.
 	 */
-	err = ct_register_buffer(ct, base + PAGE_SIZE / 4 * CTB_RECV,
+	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_RECV].desc, blob),
 				 INTEL_GUC_CT_BUFFER_TYPE_RECV);
 	if (unlikely(err))
 		goto err_out;
 
-	err = ct_register_buffer(ct, base + PAGE_SIZE / 4 * CTB_SEND,
+	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_SEND].desc, blob),
 				 INTEL_GUC_CT_BUFFER_TYPE_SEND);
 	if (unlikely(err))
 		goto err_deregister;
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 11/20] drm/i915/guc: Replace CTB array with explicit members
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (9 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 10/20] drm/i915/guc: Don't repeat CTB layout calculations Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  7:25   ` kernel test robot
  2021-06-03  5:16 ` [PATCH 12/20] " Matthew Brost
                   ` (8 subsequent siblings)
  19 siblings, 1 reply; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

Upcoming GuC firmware will always require just two CTBs and we
also plan to configure them with different sizes, so definining
them as array is no longer suitable.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 46 ++++++++++++-----------
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h |  7 +++-
 2 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 34c582105860..6864819b75a9 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -168,10 +168,10 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 	struct intel_guc *guc = ct_to_guc(ct);
 	struct guc_ct_buffer_desc *desc;
 	u32 blob_size;
+	u32 cmds_size;
 	void *blob;
 	u32 *cmds;
 	int err;
-	int i;
 
 	GEM_BUG_ON(ct->vma);
 
@@ -207,15 +207,23 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 
 	CT_DEBUG(ct, "base=%#x size=%u\n", intel_guc_ggtt_offset(guc, ct->vma), blob_size);
 
-	/* store pointers to desc and cmds */
-	for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {
-		GEM_BUG_ON((i !=  CTB_SEND) && (i != CTB_RECV));
+	/* store pointers to desc and cmds for send ctb */
+	desc = blob;
+	cmds = blob + PAGE_SIZE / 2;
+	cmds_size = PAGE_SIZE / 4;
+	CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "send",
+		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
 
-		desc = blob + PAGE_SIZE / 4 * i;
-		cmds = blob + PAGE_SIZE / 4 * i + PAGE_SIZE / 2;
+	guc_ct_buffer_init(&ct->ctbs.send, desc, cmds, cmds_size);
 
-		guc_ct_buffer_init(&ct->ctbs[i], desc, cmds, PAGE_SIZE / 4);
-	}
+	/* store pointers to desc and cmds for recv ctb */
+	desc = blob + PAGE_SIZE / 4;
+	cmds = blob + PAGE_SIZE / 4 + PAGE_SIZE / 2;
+	cmds_size = PAGE_SIZE / 4;
+	CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "recv",
+		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
+
+	guc_ct_buffer_init(&ct->ctbs.recv, desc, cmds, cmds_size);
 
 	return 0;
 }
@@ -246,7 +254,6 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 	u32 base, cmds;
 	void *blob;
 	int err;
-	int i;
 
 	GEM_BUG_ON(ct->enabled);
 
@@ -257,28 +264,25 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 
 	/* blob should start with send descriptor */
 	blob = __px_vaddr(ct->vma->obj);
-	GEM_BUG_ON(blob != ct->ctbs[CTB_SEND].desc);
+	GEM_BUG_ON(blob != ct->ctbs.send.desc);
 
 	/* (re)initialize descriptors */
-	for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {
-		GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));
+	cmds = base + ptrdiff(ct->ctbs.send.cmds, blob);
+	guc_ct_buffer_reset(&ct->ctbs.send, cmds);
 
-		cmds = base + ptrdiff(ct->ctbs[i].cmds, blob);
-		CT_DEBUG(ct, "%d: cmds addr=%#x\n", i, cmds);
-
-		guc_ct_buffer_reset(&ct->ctbs[i], cmds);
-	}
+	cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob);
+	guc_ct_buffer_reset(&ct->ctbs.recv, cmds);
 
 	/*
 	 * Register both CT buffers starting with RECV buffer.
 	 * Descriptors are in first half of the blob.
 	 */
-	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_RECV].desc, blob),
+	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.recv.desc, blob),
 				 INTEL_GUC_CT_BUFFER_TYPE_RECV);
 	if (unlikely(err))
 		goto err_out;
 
-	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_SEND].desc, blob),
+	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.send.desc, blob),
 				 INTEL_GUC_CT_BUFFER_TYPE_SEND);
 	if (unlikely(err))
 		goto err_deregister;
@@ -341,7 +345,7 @@ static int ct_write(struct intel_guc_ct *ct,
 		    u32 len /* in dwords */,
 		    u32 fence)
 {
-	struct intel_guc_ct_buffer *ctb = &ct->ctbs[CTB_SEND];
+	struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
 	struct guc_ct_buffer_desc *desc = ctb->desc;
 	u32 head = desc->head;
 	u32 tail = desc->tail;
@@ -557,7 +561,7 @@ static inline bool ct_header_is_response(u32 header)
 
 static int ct_read(struct intel_guc_ct *ct, u32 *data)
 {
-	struct intel_guc_ct_buffer *ctb = &ct->ctbs[CTB_RECV];
+	struct intel_guc_ct_buffer *ctb = &ct->ctbs.recv;
 	struct guc_ct_buffer_desc *desc = ctb->desc;
 	u32 head = desc->head;
 	u32 tail = desc->tail;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
index 4009e2dd0de4..fc9486779e87 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
@@ -47,8 +47,11 @@ struct intel_guc_ct {
 	struct i915_vma *vma;
 	bool enabled;
 
-	/* buffers for sending(0) and receiving(1) commands */
-	struct intel_guc_ct_buffer ctbs[2];
+	/* buffers for sending and receiving commands */
+	struct {
+		struct intel_guc_ct_buffer send;
+		struct intel_guc_ct_buffer recv;
+	} ctbs;
 
 	struct {
 		u32 last_fence; /* last fence used to send request */
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [PATCH 11/20] drm/i915/guc: Replace CTB array with explicit members
  2021-06-03  5:16 ` [PATCH 11/20] drm/i915/guc: Replace CTB array with explicit members Matthew Brost
@ 2021-06-03  7:25   ` kernel test robot
  2021-06-03 21:37     ` [Intel-gfx] " Daniel Vetter
  0 siblings, 1 reply; 41+ messages in thread
From: kernel test robot @ 2021-06-03  7:25 UTC (permalink / raw)
  To: Matthew Brost, intel-gfx, dri-devel; +Cc: daniel.vetter, kbuild-all

[-- Attachment #1: Type: text/plain, Size: 12002 bytes --]

Hi Matthew,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-tip/drm-tip]
[also build test ERROR on drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.13-rc4 next-20210602]
[cannot apply to drm-intel/for-linux-next drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Matthew-Brost/GuC-CTBs-changes-a-few-misc-patches/20210603-130102
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: i386-randconfig-a015-20210603 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/7672d3ecddcd8af3a2a856facac49ca93d4bdf9e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Matthew-Brost/GuC-CTBs-changes-a-few-misc-patches/20210603-130102
        git checkout 7672d3ecddcd8af3a2a856facac49ca93d4bdf9e
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/drm/drm_mm.h:49,
                    from include/drm/drm_vma_manager.h:26,
                    from include/drm/drm_gem.h:40,
                    from drivers/gpu/drm/i915/i915_drv.h:54,
                    from drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:6:
   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c: In function 'intel_guc_ct_init':
>> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:34:26: error: format '%lx' expects argument of type 'long unsigned int', but argument 5 has type 'ptrdiff_t' {aka 'int'} [-Werror=format=]
      34 |  drm_dbg(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
         |                          ^~~~~~
   ......
     215 |    ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
         |    ~~~~~~~~~~~~~~~~~~~    
         |    |
         |    ptrdiff_t {aka int}
   include/drm/drm_print.h:448:56: note: in definition of macro 'drm_dbg'
     448 |  drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
         |                                                        ^~~
   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:214:2: note: in expansion of macro 'CT_DEBUG'
     214 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "send",
         |  ^~~~~~~~
   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:214:27: note: format string is defined here
     214 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "send",
         |                        ~~~^
         |                           |
         |                           long unsigned int
         |                        %#x
   In file included from include/drm/drm_mm.h:49,
                    from include/drm/drm_vma_manager.h:26,
                    from include/drm/drm_gem.h:40,
                    from drivers/gpu/drm/i915/i915_drv.h:54,
                    from drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:6:
   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:34:26: error: format '%lx' expects argument of type 'long unsigned int', but argument 6 has type 'ptrdiff_t' {aka 'int'} [-Werror=format=]
      34 |  drm_dbg(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
         |                          ^~~~~~
   ......
     215 |    ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
         |                         ~~~~~~~~~~~~~~~~~~~
         |                         |
         |                         ptrdiff_t {aka int}
   include/drm/drm_print.h:448:56: note: in definition of macro 'drm_dbg'
     448 |  drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
         |                                                        ^~~
   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:214:2: note: in expansion of macro 'CT_DEBUG'
     214 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "send",
         |  ^~~~~~~~
   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:214:37: note: format string is defined here
     214 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "send",
         |                                  ~~~^
         |                                     |
         |                                     long unsigned int
         |                                  %#x
   In file included from include/drm/drm_mm.h:49,
                    from include/drm/drm_vma_manager.h:26,
                    from include/drm/drm_gem.h:40,
                    from drivers/gpu/drm/i915/i915_drv.h:54,
                    from drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:6:
>> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:34:26: error: format '%lx' expects argument of type 'long unsigned int', but argument 5 has type 'ptrdiff_t' {aka 'int'} [-Werror=format=]
      34 |  drm_dbg(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
         |                          ^~~~~~
   ......
     224 |    ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
         |    ~~~~~~~~~~~~~~~~~~~    
         |    |
         |    ptrdiff_t {aka int}
   include/drm/drm_print.h:448:56: note: in definition of macro 'drm_dbg'
     448 |  drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
         |                                                        ^~~
   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:223:2: note: in expansion of macro 'CT_DEBUG'
     223 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "recv",
         |  ^~~~~~~~
   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:223:27: note: format string is defined here
     223 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "recv",
         |                        ~~~^
         |                           |
         |                           long unsigned int
         |                        %#x
   In file included from include/drm/drm_mm.h:49,
                    from include/drm/drm_vma_manager.h:26,
                    from include/drm/drm_gem.h:40,
                    from drivers/gpu/drm/i915/i915_drv.h:54,
                    from drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:6:
   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:34:26: error: format '%lx' expects argument of type 'long unsigned int', but argument 6 has type 'ptrdiff_t' {aka 'int'} [-Werror=format=]
      34 |  drm_dbg(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
         |                          ^~~~~~
   ......
     224 |    ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
         |                         ~~~~~~~~~~~~~~~~~~~
         |                         |
         |                         ptrdiff_t {aka int}
   include/drm/drm_print.h:448:56: note: in definition of macro 'drm_dbg'
     448 |  drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
         |                                                        ^~~
   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:223:2: note: in expansion of macro 'CT_DEBUG'
     223 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "recv",
         |  ^~~~~~~~
   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:223:37: note: format string is defined here
     223 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "recv",
         |                                  ~~~^
         |                                     |
         |                                     long unsigned int
         |                                  %#x
   cc1: all warnings being treated as errors


vim +34 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c

f8a58d639dd95b drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2017-05-26  @6  #include "i915_drv.h"
f8a58d639dd95b drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2017-05-26   7  #include "intel_guc_ct.h"
18c8832523c850 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Michal Wajdeczko       2020-01-11   8  #include "gt/intel_gt.h"
f8a58d639dd95b drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2017-05-26   9  
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  10  static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct)
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  11  {
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  12  	return container_of(ct, struct intel_guc, ct);
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  13  }
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  14  
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  15  static inline struct intel_gt *ct_to_gt(struct intel_guc_ct *ct)
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  16  {
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  17  	return guc_to_gt(ct_to_guc(ct));
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  18  }
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  19  
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  20  static inline struct drm_i915_private *ct_to_i915(struct intel_guc_ct *ct)
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  21  {
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  22  	return ct_to_gt(ct)->i915;
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  23  }
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  24  
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  25  static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct)
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  26  {
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  27  	return &ct_to_i915(ct)->drm;
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  28  }
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  29  
18c8832523c850 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Michal Wajdeczko       2020-01-11  30  #define CT_ERROR(_ct, _fmt, ...) \
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  31  	drm_err(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
0a015ff9730c16 drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2018-03-26  32  #ifdef CONFIG_DRM_I915_DEBUG_GUC
77b20896d57e91 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Michal Wajdeczko       2020-01-17  33  #define CT_DEBUG(_ct, _fmt, ...) \
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02 @34  	drm_dbg(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
0a015ff9730c16 drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2018-03-26  35  #else
77b20896d57e91 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Michal Wajdeczko       2020-01-17  36  #define CT_DEBUG(...)	do { } while (0)
0a015ff9730c16 drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2018-03-26  37  #endif
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  38  #define CT_PROBE_ERROR(_ct, _fmt, ...) \
a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  39  	i915_probe_error(ct_to_i915(ct), "CT: " _fmt, ##__VA_ARGS__)
0a015ff9730c16 drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2018-03-26  40  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40845 bytes --]

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Intel-gfx] [PATCH 11/20] drm/i915/guc: Replace CTB array with explicit members
  2021-06-03  7:25   ` kernel test robot
@ 2021-06-03 21:37     ` Daniel Vetter
  2021-06-03 22:44       ` [PATCH 1/2] " Matthew Brost
  2021-06-03 23:04       ` [v3 PATCH 1/2] drm/i915/guc: Replace CTB array with explicit members Matthew Brost
  0 siblings, 2 replies; 41+ messages in thread
From: Daniel Vetter @ 2021-06-03 21:37 UTC (permalink / raw)
  To: kernel test robot
  Cc: Matthew Brost, daniel.vetter, intel-gfx, kbuild-all, dri-devel

On Thu, Jun 03, 2021 at 03:25:29PM +0800, kernel test robot wrote:
> Hi Matthew,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on drm-tip/drm-tip]
> [also build test ERROR on drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.13-rc4 next-20210602]
> [cannot apply to drm-intel/for-linux-next drm/drm-next]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Matthew-Brost/GuC-CTBs-changes-a-few-misc-patches/20210603-130102
> base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
> config: i386-randconfig-a015-20210603 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> reproduce (this is a W=1 build):
>         # https://github.com/0day-ci/linux/commit/7672d3ecddcd8af3a2a856facac49ca93d4bdf9e
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Matthew-Brost/GuC-CTBs-changes-a-few-misc-patches/20210603-130102
>         git checkout 7672d3ecddcd8af3a2a856facac49ca93d4bdf9e
>         # save the attached .config to linux build tree
>         make W=1 ARCH=i386 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from include/drm/drm_mm.h:49,
>                     from include/drm/drm_vma_manager.h:26,
>                     from include/drm/drm_gem.h:40,
>                     from drivers/gpu/drm/i915/i915_drv.h:54,
>                     from drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:6:
>    drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c: In function 'intel_guc_ct_init':
> >> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:34:26: error: format '%lx' expects argument of type 'long unsigned int', but argument 5 has type 'ptrdiff_t' {aka 'int'} [-Werror=format=]
>       34 |  drm_dbg(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
>          |                          ^~~~~~
>    ......
>      215 |    ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
>          |    ~~~~~~~~~~~~~~~~~~~    
>          |    |
>          |    ptrdiff_t {aka int}
>    include/drm/drm_print.h:448:56: note: in definition of macro 'drm_dbg'
>      448 |  drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
>          |                                                        ^~~
>    drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:214:2: note: in expansion of macro 'CT_DEBUG'
>      214 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "send",
>          |  ^~~~~~~~
>    drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:214:27: note: format string is defined here
>      214 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "send",
>          |                        ~~~^
>          |                           |
>          |                           long unsigned int

32bit ptrdiff vs 64bit printk modifier. You need something else here.

I've stuffed all the previous patches into drm-intel-gt-next. If you
resend with just this one fixed here I'll continue tomorrow (--in-reply-to
resend I mean).

Thanks, Daniel

>          |                        %#x
>    In file included from include/drm/drm_mm.h:49,
>                     from include/drm/drm_vma_manager.h:26,
>                     from include/drm/drm_gem.h:40,
>                     from drivers/gpu/drm/i915/i915_drv.h:54,
>                     from drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:6:
>    drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:34:26: error: format '%lx' expects argument of type 'long unsigned int', but argument 6 has type 'ptrdiff_t' {aka 'int'} [-Werror=format=]
>       34 |  drm_dbg(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
>          |                          ^~~~~~
>    ......
>      215 |    ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
>          |                         ~~~~~~~~~~~~~~~~~~~
>          |                         |
>          |                         ptrdiff_t {aka int}
>    include/drm/drm_print.h:448:56: note: in definition of macro 'drm_dbg'
>      448 |  drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
>          |                                                        ^~~
>    drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:214:2: note: in expansion of macro 'CT_DEBUG'
>      214 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "send",
>          |  ^~~~~~~~
>    drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:214:37: note: format string is defined here
>      214 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "send",
>          |                                  ~~~^
>          |                                     |
>          |                                     long unsigned int
>          |                                  %#x
>    In file included from include/drm/drm_mm.h:49,
>                     from include/drm/drm_vma_manager.h:26,
>                     from include/drm/drm_gem.h:40,
>                     from drivers/gpu/drm/i915/i915_drv.h:54,
>                     from drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:6:
> >> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:34:26: error: format '%lx' expects argument of type 'long unsigned int', but argument 5 has type 'ptrdiff_t' {aka 'int'} [-Werror=format=]
>       34 |  drm_dbg(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
>          |                          ^~~~~~
>    ......
>      224 |    ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
>          |    ~~~~~~~~~~~~~~~~~~~    
>          |    |
>          |    ptrdiff_t {aka int}
>    include/drm/drm_print.h:448:56: note: in definition of macro 'drm_dbg'
>      448 |  drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
>          |                                                        ^~~
>    drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:223:2: note: in expansion of macro 'CT_DEBUG'
>      223 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "recv",
>          |  ^~~~~~~~
>    drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:223:27: note: format string is defined here
>      223 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "recv",
>          |                        ~~~^
>          |                           |
>          |                           long unsigned int
>          |                        %#x
>    In file included from include/drm/drm_mm.h:49,
>                     from include/drm/drm_vma_manager.h:26,
>                     from include/drm/drm_gem.h:40,
>                     from drivers/gpu/drm/i915/i915_drv.h:54,
>                     from drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:6:
>    drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:34:26: error: format '%lx' expects argument of type 'long unsigned int', but argument 6 has type 'ptrdiff_t' {aka 'int'} [-Werror=format=]
>       34 |  drm_dbg(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
>          |                          ^~~~~~
>    ......
>      224 |    ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
>          |                         ~~~~~~~~~~~~~~~~~~~
>          |                         |
>          |                         ptrdiff_t {aka int}
>    include/drm/drm_print.h:448:56: note: in definition of macro 'drm_dbg'
>      448 |  drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
>          |                                                        ^~~
>    drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:223:2: note: in expansion of macro 'CT_DEBUG'
>      223 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "recv",
>          |  ^~~~~~~~
>    drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:223:37: note: format string is defined here
>      223 |  CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "recv",
>          |                                  ~~~^
>          |                                     |
>          |                                     long unsigned int
>          |                                  %#x
>    cc1: all warnings being treated as errors
> 
> 
> vim +34 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> 
> f8a58d639dd95b drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2017-05-26  @6  #include "i915_drv.h"
> f8a58d639dd95b drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2017-05-26   7  #include "intel_guc_ct.h"
> 18c8832523c850 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Michal Wajdeczko       2020-01-11   8  #include "gt/intel_gt.h"
> f8a58d639dd95b drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2017-05-26   9  
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  10  static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct)
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  11  {
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  12  	return container_of(ct, struct intel_guc, ct);
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  13  }
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  14  
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  15  static inline struct intel_gt *ct_to_gt(struct intel_guc_ct *ct)
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  16  {
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  17  	return guc_to_gt(ct_to_guc(ct));
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  18  }
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  19  
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  20  static inline struct drm_i915_private *ct_to_i915(struct intel_guc_ct *ct)
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  21  {
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  22  	return ct_to_gt(ct)->i915;
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  23  }
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  24  
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  25  static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct)
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  26  {
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  27  	return &ct_to_i915(ct)->drm;
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  28  }
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  29  
> 18c8832523c850 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Michal Wajdeczko       2020-01-11  30  #define CT_ERROR(_ct, _fmt, ...) \
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  31  	drm_err(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
> 0a015ff9730c16 drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2018-03-26  32  #ifdef CONFIG_DRM_I915_DEBUG_GUC
> 77b20896d57e91 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Michal Wajdeczko       2020-01-17  33  #define CT_DEBUG(_ct, _fmt, ...) \
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02 @34  	drm_dbg(ct_to_drm(_ct), "CT: " _fmt, ##__VA_ARGS__)
> 0a015ff9730c16 drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2018-03-26  35  #else
> 77b20896d57e91 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Michal Wajdeczko       2020-01-17  36  #define CT_DEBUG(...)	do { } while (0)
> 0a015ff9730c16 drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2018-03-26  37  #endif
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  38  #define CT_PROBE_ERROR(_ct, _fmt, ...) \
> a4c78275ba7e01 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c Daniele Ceraolo Spurio 2021-06-02  39  	i915_probe_error(ct_to_i915(ct), "CT: " _fmt, ##__VA_ARGS__)
> 0a015ff9730c16 drivers/gpu/drm/i915/intel_guc_ct.c       Michal Wajdeczko       2018-03-26  40  
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org


> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 1/2] drm/i915/guc: Replace CTB array with explicit members
  2021-06-03 21:37     ` [Intel-gfx] " Daniel Vetter
@ 2021-06-03 22:44       ` Matthew Brost
  2021-06-03 22:44         ` [PATCH 2/2] drm/i915/guc: Update sizes of CTB buffers Matthew Brost
  2021-06-03 23:04       ` [v3 PATCH 1/2] drm/i915/guc: Replace CTB array with explicit members Matthew Brost
  1 sibling, 1 reply; 41+ messages in thread
From: Matthew Brost @ 2021-06-03 22:44 UTC (permalink / raw)
  To: daniel.vetter; +Cc: intel-gfx, dri-devel

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

Upcoming GuC firmware will always require just two CTBs and we
also plan to configure them with different sizes, so definining
them as array is no longer suitable.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 46 ++++++++++++-----------
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h |  7 +++-
 2 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 34c582105860..882ae1ed41b0 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -168,10 +168,10 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 	struct intel_guc *guc = ct_to_guc(ct);
 	struct guc_ct_buffer_desc *desc;
 	u32 blob_size;
+	u32 cmds_size;
 	void *blob;
 	u32 *cmds;
 	int err;
-	int i;
 
 	GEM_BUG_ON(ct->vma);
 
@@ -207,15 +207,23 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 
 	CT_DEBUG(ct, "base=%#x size=%u\n", intel_guc_ggtt_offset(guc, ct->vma), blob_size);
 
-	/* store pointers to desc and cmds */
-	for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {
-		GEM_BUG_ON((i !=  CTB_SEND) && (i != CTB_RECV));
+	/* store pointers to desc and cmds for send ctb */
+	desc = blob;
+	cmds = blob + PAGE_SIZE / 2;
+	cmds_size = PAGE_SIZE / 4;
+	CT_DEBUG(ct, "%s desc %#p cmds %#p size %u\n", "send",
+		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
 
-		desc = blob + PAGE_SIZE / 4 * i;
-		cmds = blob + PAGE_SIZE / 4 * i + PAGE_SIZE / 2;
+	guc_ct_buffer_init(&ct->ctbs.send, desc, cmds, cmds_size);
 
-		guc_ct_buffer_init(&ct->ctbs[i], desc, cmds, PAGE_SIZE / 4);
-	}
+	/* store pointers to desc and cmds for recv ctb */
+	desc = blob + PAGE_SIZE / 4;
+	cmds = blob + PAGE_SIZE / 4 + PAGE_SIZE / 2;
+	cmds_size = PAGE_SIZE / 4;
+	CT_DEBUG(ct, "%s desc %#p cmds %#p size %u\n", "recv",
+		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
+
+	guc_ct_buffer_init(&ct->ctbs.recv, desc, cmds, cmds_size);
 
 	return 0;
 }
@@ -246,7 +254,6 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 	u32 base, cmds;
 	void *blob;
 	int err;
-	int i;
 
 	GEM_BUG_ON(ct->enabled);
 
@@ -257,28 +264,25 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 
 	/* blob should start with send descriptor */
 	blob = __px_vaddr(ct->vma->obj);
-	GEM_BUG_ON(blob != ct->ctbs[CTB_SEND].desc);
+	GEM_BUG_ON(blob != ct->ctbs.send.desc);
 
 	/* (re)initialize descriptors */
-	for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {
-		GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));
+	cmds = base + ptrdiff(ct->ctbs.send.cmds, blob);
+	guc_ct_buffer_reset(&ct->ctbs.send, cmds);
 
-		cmds = base + ptrdiff(ct->ctbs[i].cmds, blob);
-		CT_DEBUG(ct, "%d: cmds addr=%#x\n", i, cmds);
-
-		guc_ct_buffer_reset(&ct->ctbs[i], cmds);
-	}
+	cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob);
+	guc_ct_buffer_reset(&ct->ctbs.recv, cmds);
 
 	/*
 	 * Register both CT buffers starting with RECV buffer.
 	 * Descriptors are in first half of the blob.
 	 */
-	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_RECV].desc, blob),
+	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.recv.desc, blob),
 				 INTEL_GUC_CT_BUFFER_TYPE_RECV);
 	if (unlikely(err))
 		goto err_out;
 
-	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_SEND].desc, blob),
+	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.send.desc, blob),
 				 INTEL_GUC_CT_BUFFER_TYPE_SEND);
 	if (unlikely(err))
 		goto err_deregister;
@@ -341,7 +345,7 @@ static int ct_write(struct intel_guc_ct *ct,
 		    u32 len /* in dwords */,
 		    u32 fence)
 {
-	struct intel_guc_ct_buffer *ctb = &ct->ctbs[CTB_SEND];
+	struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
 	struct guc_ct_buffer_desc *desc = ctb->desc;
 	u32 head = desc->head;
 	u32 tail = desc->tail;
@@ -557,7 +561,7 @@ static inline bool ct_header_is_response(u32 header)
 
 static int ct_read(struct intel_guc_ct *ct, u32 *data)
 {
-	struct intel_guc_ct_buffer *ctb = &ct->ctbs[CTB_RECV];
+	struct intel_guc_ct_buffer *ctb = &ct->ctbs.recv;
 	struct guc_ct_buffer_desc *desc = ctb->desc;
 	u32 head = desc->head;
 	u32 tail = desc->tail;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
index 4009e2dd0de4..fc9486779e87 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
@@ -47,8 +47,11 @@ struct intel_guc_ct {
 	struct i915_vma *vma;
 	bool enabled;
 
-	/* buffers for sending(0) and receiving(1) commands */
-	struct intel_guc_ct_buffer ctbs[2];
+	/* buffers for sending and receiving commands */
+	struct {
+		struct intel_guc_ct_buffer send;
+		struct intel_guc_ct_buffer recv;
+	} ctbs;
 
 	struct {
 		u32 last_fence; /* last fence used to send request */
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 2/2] drm/i915/guc: Update sizes of CTB buffers
  2021-06-03 22:44       ` [PATCH 1/2] " Matthew Brost
@ 2021-06-03 22:44         ` Matthew Brost
  0 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03 22:44 UTC (permalink / raw)
  To: daniel.vetter; +Cc: intel-gfx, dri-devel

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

Future GuC will require CTB buffers sizes to be multiple of 4K.
Make these changes now as this shouldn't impact us too much.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
Cc: John Harrison <john.c.harrison@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 60 ++++++++++++-----------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 882ae1ed41b0..8d70f637640b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -38,6 +38,32 @@ static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct)
 #define CT_PROBE_ERROR(_ct, _fmt, ...) \
 	i915_probe_error(ct_to_i915(ct), "CT: " _fmt, ##__VA_ARGS__)
 
+/**
+ * DOC: CTB Blob
+ *
+ * We allocate single blob to hold both CTB descriptors and buffers:
+ *
+ *      +--------+-----------------------------------------------+------+
+ *      | offset | contents                                      | size |
+ *      +========+===============================================+======+
+ *      | 0x0000 | H2G `CTB Descriptor`_ (send)                  |      |
+ *      +--------+-----------------------------------------------+  4K  |
+ *      | 0x0800 | G2H `CTB Descriptor`_ (recv)                  |      |
+ *      +--------+-----------------------------------------------+------+
+ *      | 0x1000 | H2G `CT Buffer`_ (send)                       | n*4K |
+ *      |        |                                               |      |
+ *      +--------+-----------------------------------------------+------+
+ *      | 0x1000 | G2H `CT Buffer`_ (recv)                       | m*4K |
+ *      | + n*4K |                                               |      |
+ *      +--------+-----------------------------------------------+------+
+ *
+ * Size of each `CT Buffer`_ must be multiple of 4K.
+ * As we don't expect too many messages, for now use minimum sizes.
+ */
+#define CTB_DESC_SIZE		ALIGN(sizeof(struct guc_ct_buffer_desc), SZ_2K)
+#define CTB_H2G_BUFFER_SIZE	(SZ_4K)
+#define CTB_G2H_BUFFER_SIZE	(SZ_4K)
+
 struct ct_request {
 	struct list_head link;
 	u32 fence;
@@ -175,29 +201,7 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 
 	GEM_BUG_ON(ct->vma);
 
-	/* We allocate 1 page to hold both descriptors and both buffers.
-	 *       ___________.....................
-	 *      |desc (SEND)|                   :
-	 *      |___________|                   PAGE/4
-	 *      :___________....................:
-	 *      |desc (RECV)|                   :
-	 *      |___________|                   PAGE/4
-	 *      :_______________________________:
-	 *      |cmds (SEND)                    |
-	 *      |                               PAGE/4
-	 *      |_______________________________|
-	 *      |cmds (RECV)                    |
-	 *      |                               PAGE/4
-	 *      |_______________________________|
-	 *
-	 * Each message can use a maximum of 32 dwords and we don't expect to
-	 * have more than 1 in flight at any time, so we have enough space.
-	 * Some logic further ahead will rely on the fact that there is only 1
-	 * page and that it is always mapped, so if the size is changed the
-	 * other code will need updating as well.
-	 */
-
-	blob_size = PAGE_SIZE;
+	blob_size = 2 * CTB_DESC_SIZE + CTB_H2G_BUFFER_SIZE + CTB_G2H_BUFFER_SIZE;
 	err = intel_guc_allocate_and_map_vma(guc, blob_size, &ct->vma, &blob);
 	if (unlikely(err)) {
 		CT_PROBE_ERROR(ct, "Failed to allocate %u for CTB data (%pe)\n",
@@ -209,17 +213,17 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 
 	/* store pointers to desc and cmds for send ctb */
 	desc = blob;
-	cmds = blob + PAGE_SIZE / 2;
-	cmds_size = PAGE_SIZE / 4;
+	cmds = blob + 2 * CTB_DESC_SIZE;
+	cmds_size = CTB_H2G_BUFFER_SIZE;
 	CT_DEBUG(ct, "%s desc %#p cmds %#p size %u\n", "send",
 		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
 
 	guc_ct_buffer_init(&ct->ctbs.send, desc, cmds, cmds_size);
 
 	/* store pointers to desc and cmds for recv ctb */
-	desc = blob + PAGE_SIZE / 4;
-	cmds = blob + PAGE_SIZE / 4 + PAGE_SIZE / 2;
-	cmds_size = PAGE_SIZE / 4;
+	desc = blob + CTB_DESC_SIZE;
+	cmds = blob + 2 * CTB_DESC_SIZE + CTB_H2G_BUFFER_SIZE;
+	cmds_size = CTB_G2H_BUFFER_SIZE;
 	CT_DEBUG(ct, "%s desc %#p cmds %#p size %u\n", "recv",
 		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [v3 PATCH 1/2] drm/i915/guc: Replace CTB array with explicit members
  2021-06-03 21:37     ` [Intel-gfx] " Daniel Vetter
  2021-06-03 22:44       ` [PATCH 1/2] " Matthew Brost
@ 2021-06-03 23:04       ` Matthew Brost
  2021-06-03 23:04         ` [v3 PATCH 2/2] drm/i915/guc: Update sizes of CTB buffers Matthew Brost
  1 sibling, 1 reply; 41+ messages in thread
From: Matthew Brost @ 2021-06-03 23:04 UTC (permalink / raw)
  To: daniel.vetter; +Cc: intel-gfx, dri-devel

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

Upcoming GuC firmware will always require just two CTBs and we
also plan to configure them with different sizes, so definining
them as array is no longer suitable.

v2: Use %p for ptrdiff print
v3: Use %tx for ptrdiff print

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 46 ++++++++++++-----------
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h |  7 +++-
 2 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 34c582105860..ec795d7c3a7d 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -168,10 +168,10 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 	struct intel_guc *guc = ct_to_guc(ct);
 	struct guc_ct_buffer_desc *desc;
 	u32 blob_size;
+	u32 cmds_size;
 	void *blob;
 	u32 *cmds;
 	int err;
-	int i;
 
 	GEM_BUG_ON(ct->vma);
 
@@ -207,15 +207,23 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 
 	CT_DEBUG(ct, "base=%#x size=%u\n", intel_guc_ggtt_offset(guc, ct->vma), blob_size);
 
-	/* store pointers to desc and cmds */
-	for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {
-		GEM_BUG_ON((i !=  CTB_SEND) && (i != CTB_RECV));
+	/* store pointers to desc and cmds for send ctb */
+	desc = blob;
+	cmds = blob + PAGE_SIZE / 2;
+	cmds_size = PAGE_SIZE / 4;
+	CT_DEBUG(ct, "%s desc %#tx cmds %#tx size %u\n", "send",
+		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
 
-		desc = blob + PAGE_SIZE / 4 * i;
-		cmds = blob + PAGE_SIZE / 4 * i + PAGE_SIZE / 2;
+	guc_ct_buffer_init(&ct->ctbs.send, desc, cmds, cmds_size);
 
-		guc_ct_buffer_init(&ct->ctbs[i], desc, cmds, PAGE_SIZE / 4);
-	}
+	/* store pointers to desc and cmds for recv ctb */
+	desc = blob + PAGE_SIZE / 4;
+	cmds = blob + PAGE_SIZE / 4 + PAGE_SIZE / 2;
+	cmds_size = PAGE_SIZE / 4;
+	CT_DEBUG(ct, "%s desc %#tx cmds %#tx size %u\n", "recv",
+		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
+
+	guc_ct_buffer_init(&ct->ctbs.recv, desc, cmds, cmds_size);
 
 	return 0;
 }
@@ -246,7 +254,6 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 	u32 base, cmds;
 	void *blob;
 	int err;
-	int i;
 
 	GEM_BUG_ON(ct->enabled);
 
@@ -257,28 +264,25 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 
 	/* blob should start with send descriptor */
 	blob = __px_vaddr(ct->vma->obj);
-	GEM_BUG_ON(blob != ct->ctbs[CTB_SEND].desc);
+	GEM_BUG_ON(blob != ct->ctbs.send.desc);
 
 	/* (re)initialize descriptors */
-	for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) {
-		GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));
+	cmds = base + ptrdiff(ct->ctbs.send.cmds, blob);
+	guc_ct_buffer_reset(&ct->ctbs.send, cmds);
 
-		cmds = base + ptrdiff(ct->ctbs[i].cmds, blob);
-		CT_DEBUG(ct, "%d: cmds addr=%#x\n", i, cmds);
-
-		guc_ct_buffer_reset(&ct->ctbs[i], cmds);
-	}
+	cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob);
+	guc_ct_buffer_reset(&ct->ctbs.recv, cmds);
 
 	/*
 	 * Register both CT buffers starting with RECV buffer.
 	 * Descriptors are in first half of the blob.
 	 */
-	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_RECV].desc, blob),
+	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.recv.desc, blob),
 				 INTEL_GUC_CT_BUFFER_TYPE_RECV);
 	if (unlikely(err))
 		goto err_out;
 
-	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs[CTB_SEND].desc, blob),
+	err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.send.desc, blob),
 				 INTEL_GUC_CT_BUFFER_TYPE_SEND);
 	if (unlikely(err))
 		goto err_deregister;
@@ -341,7 +345,7 @@ static int ct_write(struct intel_guc_ct *ct,
 		    u32 len /* in dwords */,
 		    u32 fence)
 {
-	struct intel_guc_ct_buffer *ctb = &ct->ctbs[CTB_SEND];
+	struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
 	struct guc_ct_buffer_desc *desc = ctb->desc;
 	u32 head = desc->head;
 	u32 tail = desc->tail;
@@ -557,7 +561,7 @@ static inline bool ct_header_is_response(u32 header)
 
 static int ct_read(struct intel_guc_ct *ct, u32 *data)
 {
-	struct intel_guc_ct_buffer *ctb = &ct->ctbs[CTB_RECV];
+	struct intel_guc_ct_buffer *ctb = &ct->ctbs.recv;
 	struct guc_ct_buffer_desc *desc = ctb->desc;
 	u32 head = desc->head;
 	u32 tail = desc->tail;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
index 4009e2dd0de4..fc9486779e87 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
@@ -47,8 +47,11 @@ struct intel_guc_ct {
 	struct i915_vma *vma;
 	bool enabled;
 
-	/* buffers for sending(0) and receiving(1) commands */
-	struct intel_guc_ct_buffer ctbs[2];
+	/* buffers for sending and receiving commands */
+	struct {
+		struct intel_guc_ct_buffer send;
+		struct intel_guc_ct_buffer recv;
+	} ctbs;
 
 	struct {
 		u32 last_fence; /* last fence used to send request */
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [v3 PATCH 2/2] drm/i915/guc: Update sizes of CTB buffers
  2021-06-03 23:04       ` [v3 PATCH 1/2] drm/i915/guc: Replace CTB array with explicit members Matthew Brost
@ 2021-06-03 23:04         ` Matthew Brost
  2021-06-04  8:20           ` Daniel Vetter
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Brost @ 2021-06-03 23:04 UTC (permalink / raw)
  To: daniel.vetter; +Cc: intel-gfx, dri-devel

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

Future GuC will require CTB buffers sizes to be multiple of 4K.
Make these changes now as this shouldn't impact us too much.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
Cc: John Harrison <john.c.harrison@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 60 ++++++++++++-----------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index ec795d7c3a7d..8d1173032431 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -38,6 +38,32 @@ static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct)
 #define CT_PROBE_ERROR(_ct, _fmt, ...) \
 	i915_probe_error(ct_to_i915(ct), "CT: " _fmt, ##__VA_ARGS__)
 
+/**
+ * DOC: CTB Blob
+ *
+ * We allocate single blob to hold both CTB descriptors and buffers:
+ *
+ *      +--------+-----------------------------------------------+------+
+ *      | offset | contents                                      | size |
+ *      +========+===============================================+======+
+ *      | 0x0000 | H2G `CTB Descriptor`_ (send)                  |      |
+ *      +--------+-----------------------------------------------+  4K  |
+ *      | 0x0800 | G2H `CTB Descriptor`_ (recv)                  |      |
+ *      +--------+-----------------------------------------------+------+
+ *      | 0x1000 | H2G `CT Buffer`_ (send)                       | n*4K |
+ *      |        |                                               |      |
+ *      +--------+-----------------------------------------------+------+
+ *      | 0x1000 | G2H `CT Buffer`_ (recv)                       | m*4K |
+ *      | + n*4K |                                               |      |
+ *      +--------+-----------------------------------------------+------+
+ *
+ * Size of each `CT Buffer`_ must be multiple of 4K.
+ * As we don't expect too many messages, for now use minimum sizes.
+ */
+#define CTB_DESC_SIZE		ALIGN(sizeof(struct guc_ct_buffer_desc), SZ_2K)
+#define CTB_H2G_BUFFER_SIZE	(SZ_4K)
+#define CTB_G2H_BUFFER_SIZE	(SZ_4K)
+
 struct ct_request {
 	struct list_head link;
 	u32 fence;
@@ -175,29 +201,7 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 
 	GEM_BUG_ON(ct->vma);
 
-	/* We allocate 1 page to hold both descriptors and both buffers.
-	 *       ___________.....................
-	 *      |desc (SEND)|                   :
-	 *      |___________|                   PAGE/4
-	 *      :___________....................:
-	 *      |desc (RECV)|                   :
-	 *      |___________|                   PAGE/4
-	 *      :_______________________________:
-	 *      |cmds (SEND)                    |
-	 *      |                               PAGE/4
-	 *      |_______________________________|
-	 *      |cmds (RECV)                    |
-	 *      |                               PAGE/4
-	 *      |_______________________________|
-	 *
-	 * Each message can use a maximum of 32 dwords and we don't expect to
-	 * have more than 1 in flight at any time, so we have enough space.
-	 * Some logic further ahead will rely on the fact that there is only 1
-	 * page and that it is always mapped, so if the size is changed the
-	 * other code will need updating as well.
-	 */
-
-	blob_size = PAGE_SIZE;
+	blob_size = 2 * CTB_DESC_SIZE + CTB_H2G_BUFFER_SIZE + CTB_G2H_BUFFER_SIZE;
 	err = intel_guc_allocate_and_map_vma(guc, blob_size, &ct->vma, &blob);
 	if (unlikely(err)) {
 		CT_PROBE_ERROR(ct, "Failed to allocate %u for CTB data (%pe)\n",
@@ -209,17 +213,17 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 
 	/* store pointers to desc and cmds for send ctb */
 	desc = blob;
-	cmds = blob + PAGE_SIZE / 2;
-	cmds_size = PAGE_SIZE / 4;
+	cmds = blob + 2 * CTB_DESC_SIZE;
+	cmds_size = CTB_H2G_BUFFER_SIZE;
 	CT_DEBUG(ct, "%s desc %#tx cmds %#tx size %u\n", "send",
 		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
 
 	guc_ct_buffer_init(&ct->ctbs.send, desc, cmds, cmds_size);
 
 	/* store pointers to desc and cmds for recv ctb */
-	desc = blob + PAGE_SIZE / 4;
-	cmds = blob + PAGE_SIZE / 4 + PAGE_SIZE / 2;
-	cmds_size = PAGE_SIZE / 4;
+	desc = blob + CTB_DESC_SIZE;
+	cmds = blob + 2 * CTB_DESC_SIZE + CTB_H2G_BUFFER_SIZE;
+	cmds_size = CTB_G2H_BUFFER_SIZE;
 	CT_DEBUG(ct, "%s desc %#tx cmds %#tx size %u\n", "recv",
 		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [v3 PATCH 2/2] drm/i915/guc: Update sizes of CTB buffers
  2021-06-03 23:04         ` [v3 PATCH 2/2] drm/i915/guc: Update sizes of CTB buffers Matthew Brost
@ 2021-06-04  8:20           ` Daniel Vetter
  2021-06-04  8:49             ` Michal Wajdeczko
  0 siblings, 1 reply; 41+ messages in thread
From: Daniel Vetter @ 2021-06-04  8:20 UTC (permalink / raw)
  To: Matthew Brost; +Cc: daniel.vetter, intel-gfx, dri-devel

On Thu, Jun 03, 2021 at 04:04:08PM -0700, Matthew Brost wrote:
> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 
> Future GuC will require CTB buffers sizes to be multiple of 4K.
> Make these changes now as this shouldn't impact us too much.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> Cc: John Harrison <john.c.harrison@intel.com>

Assuming this was just rebased?

> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 60 ++++++++++++-----------
>  1 file changed, 32 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> index ec795d7c3a7d..8d1173032431 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -38,6 +38,32 @@ static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct)
>  #define CT_PROBE_ERROR(_ct, _fmt, ...) \
>  	i915_probe_error(ct_to_i915(ct), "CT: " _fmt, ##__VA_ARGS__)
>  
> +/**
> + * DOC: CTB Blob

These are supposed to be pulled into the kerneldoc builds, but that's not
happening in this patch?

Now I think the GuC docs in general are fairly outdated, so is the DOC
review coming later, or is the DOC: header here simply cargo-culted :-)

If it's not coming later we need to do a JIRA to clean this all up and
link all the new/changed kerneldoc into our GuC doc structure.
-Daniel

> + *
> + * We allocate single blob to hold both CTB descriptors and buffers:
> + *
> + *      +--------+-----------------------------------------------+------+
> + *      | offset | contents                                      | size |
> + *      +========+===============================================+======+
> + *      | 0x0000 | H2G `CTB Descriptor`_ (send)                  |      |
> + *      +--------+-----------------------------------------------+  4K  |
> + *      | 0x0800 | G2H `CTB Descriptor`_ (recv)                  |      |
> + *      +--------+-----------------------------------------------+------+
> + *      | 0x1000 | H2G `CT Buffer`_ (send)                       | n*4K |
> + *      |        |                                               |      |
> + *      +--------+-----------------------------------------------+------+
> + *      | 0x1000 | G2H `CT Buffer`_ (recv)                       | m*4K |
> + *      | + n*4K |                                               |      |
> + *      +--------+-----------------------------------------------+------+
> + *
> + * Size of each `CT Buffer`_ must be multiple of 4K.
> + * As we don't expect too many messages, for now use minimum sizes.
> + */
> +#define CTB_DESC_SIZE		ALIGN(sizeof(struct guc_ct_buffer_desc), SZ_2K)
> +#define CTB_H2G_BUFFER_SIZE	(SZ_4K)
> +#define CTB_G2H_BUFFER_SIZE	(SZ_4K)
> +
>  struct ct_request {
>  	struct list_head link;
>  	u32 fence;
> @@ -175,29 +201,7 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
>  
>  	GEM_BUG_ON(ct->vma);
>  
> -	/* We allocate 1 page to hold both descriptors and both buffers.
> -	 *       ___________.....................
> -	 *      |desc (SEND)|                   :
> -	 *      |___________|                   PAGE/4
> -	 *      :___________....................:
> -	 *      |desc (RECV)|                   :
> -	 *      |___________|                   PAGE/4
> -	 *      :_______________________________:
> -	 *      |cmds (SEND)                    |
> -	 *      |                               PAGE/4
> -	 *      |_______________________________|
> -	 *      |cmds (RECV)                    |
> -	 *      |                               PAGE/4
> -	 *      |_______________________________|
> -	 *
> -	 * Each message can use a maximum of 32 dwords and we don't expect to
> -	 * have more than 1 in flight at any time, so we have enough space.
> -	 * Some logic further ahead will rely on the fact that there is only 1
> -	 * page and that it is always mapped, so if the size is changed the
> -	 * other code will need updating as well.
> -	 */
> -
> -	blob_size = PAGE_SIZE;
> +	blob_size = 2 * CTB_DESC_SIZE + CTB_H2G_BUFFER_SIZE + CTB_G2H_BUFFER_SIZE;
>  	err = intel_guc_allocate_and_map_vma(guc, blob_size, &ct->vma, &blob);
>  	if (unlikely(err)) {
>  		CT_PROBE_ERROR(ct, "Failed to allocate %u for CTB data (%pe)\n",
> @@ -209,17 +213,17 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
>  
>  	/* store pointers to desc and cmds for send ctb */
>  	desc = blob;
> -	cmds = blob + PAGE_SIZE / 2;
> -	cmds_size = PAGE_SIZE / 4;
> +	cmds = blob + 2 * CTB_DESC_SIZE;
> +	cmds_size = CTB_H2G_BUFFER_SIZE;
>  	CT_DEBUG(ct, "%s desc %#tx cmds %#tx size %u\n", "send",
>  		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
>  
>  	guc_ct_buffer_init(&ct->ctbs.send, desc, cmds, cmds_size);
>  
>  	/* store pointers to desc and cmds for recv ctb */
> -	desc = blob + PAGE_SIZE / 4;
> -	cmds = blob + PAGE_SIZE / 4 + PAGE_SIZE / 2;
> -	cmds_size = PAGE_SIZE / 4;
> +	desc = blob + CTB_DESC_SIZE;
> +	cmds = blob + 2 * CTB_DESC_SIZE + CTB_H2G_BUFFER_SIZE;
> +	cmds_size = CTB_G2H_BUFFER_SIZE;
>  	CT_DEBUG(ct, "%s desc %#tx cmds %#tx size %u\n", "recv",
>  		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
>  
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [v3 PATCH 2/2] drm/i915/guc: Update sizes of CTB buffers
  2021-06-04  8:20           ` Daniel Vetter
@ 2021-06-04  8:49             ` Michal Wajdeczko
  0 siblings, 0 replies; 41+ messages in thread
From: Michal Wajdeczko @ 2021-06-04  8:49 UTC (permalink / raw)
  To: Daniel Vetter, Matthew Brost; +Cc: daniel.vetter, intel-gfx, dri-devel



On 04.06.2021 10:20, Daniel Vetter wrote:
> On Thu, Jun 03, 2021 at 04:04:08PM -0700, Matthew Brost wrote:
>> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>
>> Future GuC will require CTB buffers sizes to be multiple of 4K.
>> Make these changes now as this shouldn't impact us too much.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>> Cc: John Harrison <john.c.harrison@intel.com>
> 
> Assuming this was just rebased?
> 
>> ---
>>  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 60 ++++++++++++-----------
>>  1 file changed, 32 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>> index ec795d7c3a7d..8d1173032431 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>> @@ -38,6 +38,32 @@ static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct)
>>  #define CT_PROBE_ERROR(_ct, _fmt, ...) \
>>  	i915_probe_error(ct_to_i915(ct), "CT: " _fmt, ##__VA_ARGS__)
>>  
>> +/**
>> + * DOC: CTB Blob
> 
> These are supposed to be pulled into the kerneldoc builds, but that's not
> happening in this patch?

true, but as you already noticed below, some of the docs are outdated,
so it is hard to connect fresh/cleanup docs with rest of the stale
documentation

we should first complete update of individual items before we can start
cleaning/refactoring master GuC kerneldoc section

> 
> Now I think the GuC docs in general are fairly outdated, so is the DOC
> review coming later, or is the DOC: header here simply cargo-culted :-)
> 
> If it's not coming later we need to do a JIRA to clean this all up and
> link all the new/changed kerneldoc into our GuC doc structure.

there are some pending patches that links these items, but task of
general update of the master GuC kerneldoc section likely must be added
to Matt's todo list (my focus was mainly on individual doc items, since
this was my secondary job ;)

Thanks,
Michal

> -Daniel
> 
>> + *
>> + * We allocate single blob to hold both CTB descriptors and buffers:
>> + *
>> + *      +--------+-----------------------------------------------+------+
>> + *      | offset | contents                                      | size |
>> + *      +========+===============================================+======+
>> + *      | 0x0000 | H2G `CTB Descriptor`_ (send)                  |      |
>> + *      +--------+-----------------------------------------------+  4K  |
>> + *      | 0x0800 | G2H `CTB Descriptor`_ (recv)                  |      |
>> + *      +--------+-----------------------------------------------+------+
>> + *      | 0x1000 | H2G `CT Buffer`_ (send)                       | n*4K |
>> + *      |        |                                               |      |
>> + *      +--------+-----------------------------------------------+------+
>> + *      | 0x1000 | G2H `CT Buffer`_ (recv)                       | m*4K |
>> + *      | + n*4K |                                               |      |
>> + *      +--------+-----------------------------------------------+------+
>> + *
>> + * Size of each `CT Buffer`_ must be multiple of 4K.
>> + * As we don't expect too many messages, for now use minimum sizes.
>> + */
>> +#define CTB_DESC_SIZE		ALIGN(sizeof(struct guc_ct_buffer_desc), SZ_2K)
>> +#define CTB_H2G_BUFFER_SIZE	(SZ_4K)
>> +#define CTB_G2H_BUFFER_SIZE	(SZ_4K)
>> +
>>  struct ct_request {
>>  	struct list_head link;
>>  	u32 fence;
>> @@ -175,29 +201,7 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
>>  
>>  	GEM_BUG_ON(ct->vma);
>>  
>> -	/* We allocate 1 page to hold both descriptors and both buffers.
>> -	 *       ___________.....................
>> -	 *      |desc (SEND)|                   :
>> -	 *      |___________|                   PAGE/4
>> -	 *      :___________....................:
>> -	 *      |desc (RECV)|                   :
>> -	 *      |___________|                   PAGE/4
>> -	 *      :_______________________________:
>> -	 *      |cmds (SEND)                    |
>> -	 *      |                               PAGE/4
>> -	 *      |_______________________________|
>> -	 *      |cmds (RECV)                    |
>> -	 *      |                               PAGE/4
>> -	 *      |_______________________________|
>> -	 *
>> -	 * Each message can use a maximum of 32 dwords and we don't expect to
>> -	 * have more than 1 in flight at any time, so we have enough space.
>> -	 * Some logic further ahead will rely on the fact that there is only 1
>> -	 * page and that it is always mapped, so if the size is changed the
>> -	 * other code will need updating as well.
>> -	 */
>> -
>> -	blob_size = PAGE_SIZE;
>> +	blob_size = 2 * CTB_DESC_SIZE + CTB_H2G_BUFFER_SIZE + CTB_G2H_BUFFER_SIZE;
>>  	err = intel_guc_allocate_and_map_vma(guc, blob_size, &ct->vma, &blob);
>>  	if (unlikely(err)) {
>>  		CT_PROBE_ERROR(ct, "Failed to allocate %u for CTB data (%pe)\n",
>> @@ -209,17 +213,17 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
>>  
>>  	/* store pointers to desc and cmds for send ctb */
>>  	desc = blob;
>> -	cmds = blob + PAGE_SIZE / 2;
>> -	cmds_size = PAGE_SIZE / 4;
>> +	cmds = blob + 2 * CTB_DESC_SIZE;
>> +	cmds_size = CTB_H2G_BUFFER_SIZE;
>>  	CT_DEBUG(ct, "%s desc %#tx cmds %#tx size %u\n", "send",
>>  		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
>>  
>>  	guc_ct_buffer_init(&ct->ctbs.send, desc, cmds, cmds_size);
>>  
>>  	/* store pointers to desc and cmds for recv ctb */
>> -	desc = blob + PAGE_SIZE / 4;
>> -	cmds = blob + PAGE_SIZE / 4 + PAGE_SIZE / 2;
>> -	cmds_size = PAGE_SIZE / 4;
>> +	desc = blob + CTB_DESC_SIZE;
>> +	cmds = blob + 2 * CTB_DESC_SIZE + CTB_H2G_BUFFER_SIZE;
>> +	cmds_size = CTB_G2H_BUFFER_SIZE;
>>  	CT_DEBUG(ct, "%s desc %#tx cmds %#tx size %u\n", "recv",
>>  		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
>>  
>> -- 
>> 2.28.0
>>
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 12/20] drm/i915/guc: Update sizes of CTB buffers
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (10 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 11/20] drm/i915/guc: Replace CTB array with explicit members Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 13/20] drm/i915/guc: Relax CTB response timeout Matthew Brost
                   ` (7 subsequent siblings)
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

Future GuC will require CTB buffers sizes to be multiple of 4K.
Make these changes now as this shouldn't impact us too much.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
Cc: John Harrison <john.c.harrison@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 60 ++++++++++++-----------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 6864819b75a9..916c2b80c841 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -38,6 +38,32 @@ static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct)
 #define CT_PROBE_ERROR(_ct, _fmt, ...) \
 	i915_probe_error(ct_to_i915(ct), "CT: " _fmt, ##__VA_ARGS__)
 
+/**
+ * DOC: CTB Blob
+ *
+ * We allocate single blob to hold both CTB descriptors and buffers:
+ *
+ *      +--------+-----------------------------------------------+------+
+ *      | offset | contents                                      | size |
+ *      +========+===============================================+======+
+ *      | 0x0000 | H2G `CTB Descriptor`_ (send)                  |      |
+ *      +--------+-----------------------------------------------+  4K  |
+ *      | 0x0800 | G2H `CTB Descriptor`_ (recv)                  |      |
+ *      +--------+-----------------------------------------------+------+
+ *      | 0x1000 | H2G `CT Buffer`_ (send)                       | n*4K |
+ *      |        |                                               |      |
+ *      +--------+-----------------------------------------------+------+
+ *      | 0x1000 | G2H `CT Buffer`_ (recv)                       | m*4K |
+ *      | + n*4K |                                               |      |
+ *      +--------+-----------------------------------------------+------+
+ *
+ * Size of each `CT Buffer`_ must be multiple of 4K.
+ * As we don't expect too many messages, for now use minimum sizes.
+ */
+#define CTB_DESC_SIZE		ALIGN(sizeof(struct guc_ct_buffer_desc), SZ_2K)
+#define CTB_H2G_BUFFER_SIZE	(SZ_4K)
+#define CTB_G2H_BUFFER_SIZE	(SZ_4K)
+
 struct ct_request {
 	struct list_head link;
 	u32 fence;
@@ -175,29 +201,7 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 
 	GEM_BUG_ON(ct->vma);
 
-	/* We allocate 1 page to hold both descriptors and both buffers.
-	 *       ___________.....................
-	 *      |desc (SEND)|                   :
-	 *      |___________|                   PAGE/4
-	 *      :___________....................:
-	 *      |desc (RECV)|                   :
-	 *      |___________|                   PAGE/4
-	 *      :_______________________________:
-	 *      |cmds (SEND)                    |
-	 *      |                               PAGE/4
-	 *      |_______________________________|
-	 *      |cmds (RECV)                    |
-	 *      |                               PAGE/4
-	 *      |_______________________________|
-	 *
-	 * Each message can use a maximum of 32 dwords and we don't expect to
-	 * have more than 1 in flight at any time, so we have enough space.
-	 * Some logic further ahead will rely on the fact that there is only 1
-	 * page and that it is always mapped, so if the size is changed the
-	 * other code will need updating as well.
-	 */
-
-	blob_size = PAGE_SIZE;
+	blob_size = 2 * CTB_DESC_SIZE + CTB_H2G_BUFFER_SIZE + CTB_G2H_BUFFER_SIZE;
 	err = intel_guc_allocate_and_map_vma(guc, blob_size, &ct->vma, &blob);
 	if (unlikely(err)) {
 		CT_PROBE_ERROR(ct, "Failed to allocate %u for CTB data (%pe)\n",
@@ -209,17 +213,17 @@ int intel_guc_ct_init(struct intel_guc_ct *ct)
 
 	/* store pointers to desc and cmds for send ctb */
 	desc = blob;
-	cmds = blob + PAGE_SIZE / 2;
-	cmds_size = PAGE_SIZE / 4;
+	cmds = blob + 2 * CTB_DESC_SIZE;
+	cmds_size = CTB_H2G_BUFFER_SIZE;
 	CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "send",
 		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
 
 	guc_ct_buffer_init(&ct->ctbs.send, desc, cmds, cmds_size);
 
 	/* store pointers to desc and cmds for recv ctb */
-	desc = blob + PAGE_SIZE / 4;
-	cmds = blob + PAGE_SIZE / 4 + PAGE_SIZE / 2;
-	cmds_size = PAGE_SIZE / 4;
+	desc = blob + CTB_DESC_SIZE;
+	cmds = blob + 2 * CTB_DESC_SIZE + CTB_H2G_BUFFER_SIZE;
+	cmds_size = CTB_G2H_BUFFER_SIZE;
 	CT_DEBUG(ct, "%s desc %#lx cmds %#lx size %u\n", "recv",
 		 ptrdiff(desc, blob), ptrdiff(cmds, blob), cmds_size);
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 13/20] drm/i915/guc: Relax CTB response timeout
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (11 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 12/20] " Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-04  8:33   ` [Intel-gfx] " Daniel Vetter
  2021-06-03  5:16 ` [PATCH 14/20] drm/i915/guc: Start protecting access to CTB descriptors Matthew Brost
                   ` (6 subsequent siblings)
  19 siblings, 1 reply; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

In upcoming patch we will allow more CTB requests to be sent in
parallel to the GuC for processing, so we shouldn't assume any more
that GuC will always reply without 10ms.

Use bigger value from CONFIG_DRM_I915_GUC_CTB_TIMEOUT instead.

v2: Add CONFIG_DRM_I915_GUC_CTB_TIMEOUT config option

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/Kconfig.profile      | 10 ++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c |  5 ++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
index 39328567c200..0d5475b5f28a 100644
--- a/drivers/gpu/drm/i915/Kconfig.profile
+++ b/drivers/gpu/drm/i915/Kconfig.profile
@@ -38,6 +38,16 @@ config DRM_I915_USERFAULT_AUTOSUSPEND
 	  May be 0 to disable the extra delay and solely use the device level
 	  runtime pm autosuspend delay tunable.
 
+config DRM_I915_GUC_CTB_TIMEOUT
+	int "How long to wait for the GuC to make forward progress on CTBs (ms)"
+	default 1500 # milliseconds
+	range 10 60000
+	help
+	  Configures the default timeout waiting for GuC the to make forward
+	  progress on CTBs. e.g. Waiting for a response to a requeset.
+
+	  A range of 10 ms to 60000 ms is allowed.
+
 config DRM_I915_HEARTBEAT_INTERVAL
 	int "Interval between heartbeat pulses (ms)"
 	default 2500 # milliseconds
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 916c2b80c841..cf1fb09ef766 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -436,6 +436,7 @@ static int ct_write(struct intel_guc_ct *ct,
  */
 static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
 {
+	long timeout;
 	int err;
 
 	/*
@@ -443,10 +444,12 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
 	 * up to that length of time, then switch to a slower sleep-wait loop.
 	 * No GuC command should ever take longer than 10ms.
 	 */
+	timeout = CONFIG_DRM_I915_GUC_CTB_TIMEOUT;
+
 #define done INTEL_GUC_MSG_IS_RESPONSE(READ_ONCE(req->status))
 	err = wait_for_us(done, 10);
 	if (err)
-		err = wait_for(done, 10);
+		err = wait_for(done, timeout);
 #undef done
 
 	if (unlikely(err))
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [Intel-gfx] [PATCH 13/20] drm/i915/guc: Relax CTB response timeout
  2021-06-03  5:16 ` [PATCH 13/20] drm/i915/guc: Relax CTB response timeout Matthew Brost
@ 2021-06-04  8:33   ` Daniel Vetter
  2021-06-04 18:35     ` Matthew Brost
  0 siblings, 1 reply; 41+ messages in thread
From: Daniel Vetter @ 2021-06-04  8:33 UTC (permalink / raw)
  To: Matthew Brost; +Cc: daniel.vetter, intel-gfx, dri-devel

On Wed, Jun 02, 2021 at 10:16:23PM -0700, Matthew Brost wrote:
> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 
> In upcoming patch we will allow more CTB requests to be sent in
> parallel to the GuC for processing, so we shouldn't assume any more
> that GuC will always reply without 10ms.
> 
> Use bigger value from CONFIG_DRM_I915_GUC_CTB_TIMEOUT instead.
> 
> v2: Add CONFIG_DRM_I915_GUC_CTB_TIMEOUT config option
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>

So this is a rant, but for upstream we really need to do better than
internal:

- The driver must work by default in the optimal configuration.

- Any config change that we haven't validated _must_ taint the kernel
  (this is especially for module options, but also for config settings)

- Config need a real reason beyond "was useful for bring-up".

Our internal tree is an absolute disaster right now, with multi-line
kernel configs (different on each platform) and bespoke kernel config or
the driver just fails. We're the expert on our own hw, we should know how
it works, not offload that to users essentially asking them "how shitty do
you think Intel hw is in responding timely".

Yes I know there's a lot of these there already, they don't make a lot of
sense either.

Except if there's a real reason for this (aside from us just offloading
testing to our users instead of doing it ourselves properly) I think we
should hardcode this, with a comment explaining why. Maybe with a switch
between the PF/VF case once that's landed.

> ---
>  drivers/gpu/drm/i915/Kconfig.profile      | 10 ++++++++++
>  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c |  5 ++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
> index 39328567c200..0d5475b5f28a 100644
> --- a/drivers/gpu/drm/i915/Kconfig.profile
> +++ b/drivers/gpu/drm/i915/Kconfig.profile
> @@ -38,6 +38,16 @@ config DRM_I915_USERFAULT_AUTOSUSPEND
>  	  May be 0 to disable the extra delay and solely use the device level
>  	  runtime pm autosuspend delay tunable.
>  
> +config DRM_I915_GUC_CTB_TIMEOUT
> +	int "How long to wait for the GuC to make forward progress on CTBs (ms)"
> +	default 1500 # milliseconds
> +	range 10 60000

Also range is definitely off, drm/scheduler will probably nuke you
beforehand :-)

That's kinda another issue I have with all these kconfig knobs: Maybe we
need a knob for "relax with reset attempts, my workloads overload my gpus
routinely", which then scales _all_ timeouts proportionally. But letting
the user set them all, with silly combiniations like resetting the
workload before heartbeat or stuff like that doesn't make much sense.

Anyway, tiny patch so hopefully I can leave this one out for now until
we've closed this.
-Daniel

> +	help
> +	  Configures the default timeout waiting for GuC the to make forward
> +	  progress on CTBs. e.g. Waiting for a response to a requeset.
> +
> +	  A range of 10 ms to 60000 ms is allowed.
> +
>  config DRM_I915_HEARTBEAT_INTERVAL
>  	int "Interval between heartbeat pulses (ms)"
>  	default 2500 # milliseconds
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> index 916c2b80c841..cf1fb09ef766 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -436,6 +436,7 @@ static int ct_write(struct intel_guc_ct *ct,
>   */
>  static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
>  {
> +	long timeout;
>  	int err;
>  
>  	/*
> @@ -443,10 +444,12 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
>  	 * up to that length of time, then switch to a slower sleep-wait loop.
>  	 * No GuC command should ever take longer than 10ms.
>  	 */
> +	timeout = CONFIG_DRM_I915_GUC_CTB_TIMEOUT;
> +
>  #define done INTEL_GUC_MSG_IS_RESPONSE(READ_ONCE(req->status))
>  	err = wait_for_us(done, 10);
>  	if (err)
> -		err = wait_for(done, 10);
> +		err = wait_for(done, timeout);
>  #undef done
>  
>  	if (unlikely(err))
> -- 
> 2.28.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Intel-gfx] [PATCH 13/20] drm/i915/guc: Relax CTB response timeout
  2021-06-04  8:33   ` [Intel-gfx] " Daniel Vetter
@ 2021-06-04 18:35     ` Matthew Brost
  2021-06-09 13:24       ` Daniel Vetter
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Brost @ 2021-06-04 18:35 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: daniel.vetter, intel-gfx, dri-devel

On Fri, Jun 04, 2021 at 10:33:07AM +0200, Daniel Vetter wrote:
> On Wed, Jun 02, 2021 at 10:16:23PM -0700, Matthew Brost wrote:
> > From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > 
> > In upcoming patch we will allow more CTB requests to be sent in
> > parallel to the GuC for processing, so we shouldn't assume any more
> > that GuC will always reply without 10ms.
> > 
> > Use bigger value from CONFIG_DRM_I915_GUC_CTB_TIMEOUT instead.
> > 
> > v2: Add CONFIG_DRM_I915_GUC_CTB_TIMEOUT config option
> > 
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> 
> So this is a rant, but for upstream we really need to do better than
> internal:
> 
> - The driver must work by default in the optimal configuration.
> 
> - Any config change that we haven't validated _must_ taint the kernel
>   (this is especially for module options, but also for config settings)
> 
> - Config need a real reason beyond "was useful for bring-up".
> 
> Our internal tree is an absolute disaster right now, with multi-line
> kernel configs (different on each platform) and bespoke kernel config or
> the driver just fails. We're the expert on our own hw, we should know how
> it works, not offload that to users essentially asking them "how shitty do
> you think Intel hw is in responding timely".
> 
> Yes I know there's a lot of these there already, they don't make a lot of
> sense either.
> 
> Except if there's a real reason for this (aside from us just offloading
> testing to our users instead of doing it ourselves properly) I think we
> should hardcode this, with a comment explaining why. Maybe with a switch
> between the PF/VF case once that's landed.
> 
> > ---
> >  drivers/gpu/drm/i915/Kconfig.profile      | 10 ++++++++++
> >  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c |  5 ++++-
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
> > index 39328567c200..0d5475b5f28a 100644
> > --- a/drivers/gpu/drm/i915/Kconfig.profile
> > +++ b/drivers/gpu/drm/i915/Kconfig.profile
> > @@ -38,6 +38,16 @@ config DRM_I915_USERFAULT_AUTOSUSPEND
> >  	  May be 0 to disable the extra delay and solely use the device level
> >  	  runtime pm autosuspend delay tunable.
> >  
> > +config DRM_I915_GUC_CTB_TIMEOUT
> > +	int "How long to wait for the GuC to make forward progress on CTBs (ms)"
> > +	default 1500 # milliseconds
> > +	range 10 60000
> 
> Also range is definitely off, drm/scheduler will probably nuke you
> beforehand :-)
> 
> That's kinda another issue I have with all these kconfig knobs: Maybe we
> need a knob for "relax with reset attempts, my workloads overload my gpus
> routinely", which then scales _all_ timeouts proportionally. But letting
> the user set them all, with silly combiniations like resetting the
> workload before heartbeat or stuff like that doesn't make much sense.
>

Yes, the code as is the user could do some wacky stuff that doesn't make
sense at all.
 
> Anyway, tiny patch so hopefully I can leave this one out for now until
> we've closed this.

No issue leaving this out as blocking CTBs are never really used anyways
until SRIOV aside from setup / debugging. That being said, we might
still want a higher hardcoded value in the meantime, perhaps around a
second. I can follow up on that if needed.

Matt

> -Daniel
> 
> > +	help
> > +	  Configures the default timeout waiting for GuC the to make forward
> > +	  progress on CTBs. e.g. Waiting for a response to a requeset.
> > +
> > +	  A range of 10 ms to 60000 ms is allowed.
> > +
> >  config DRM_I915_HEARTBEAT_INTERVAL
> >  	int "Interval between heartbeat pulses (ms)"
> >  	default 2500 # milliseconds
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > index 916c2b80c841..cf1fb09ef766 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > @@ -436,6 +436,7 @@ static int ct_write(struct intel_guc_ct *ct,
> >   */
> >  static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
> >  {
> > +	long timeout;
> >  	int err;
> >  
> >  	/*
> > @@ -443,10 +444,12 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
> >  	 * up to that length of time, then switch to a slower sleep-wait loop.
> >  	 * No GuC command should ever take longer than 10ms.
> >  	 */
> > +	timeout = CONFIG_DRM_I915_GUC_CTB_TIMEOUT;
> > +
> >  #define done INTEL_GUC_MSG_IS_RESPONSE(READ_ONCE(req->status))
> >  	err = wait_for_us(done, 10);
> >  	if (err)
> > -		err = wait_for(done, 10);
> > +		err = wait_for(done, timeout);
> >  #undef done
> >  
> >  	if (unlikely(err))
> > -- 
> > 2.28.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Intel-gfx] [PATCH 13/20] drm/i915/guc: Relax CTB response timeout
  2021-06-04 18:35     ` Matthew Brost
@ 2021-06-09 13:24       ` Daniel Vetter
  0 siblings, 0 replies; 41+ messages in thread
From: Daniel Vetter @ 2021-06-09 13:24 UTC (permalink / raw)
  To: Matthew Brost; +Cc: daniel.vetter, intel-gfx, dri-devel

On Fri, Jun 04, 2021 at 11:35:39AM -0700, Matthew Brost wrote:
> On Fri, Jun 04, 2021 at 10:33:07AM +0200, Daniel Vetter wrote:
> > On Wed, Jun 02, 2021 at 10:16:23PM -0700, Matthew Brost wrote:
> > > From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > 
> > > In upcoming patch we will allow more CTB requests to be sent in
> > > parallel to the GuC for processing, so we shouldn't assume any more
> > > that GuC will always reply without 10ms.
> > > 
> > > Use bigger value from CONFIG_DRM_I915_GUC_CTB_TIMEOUT instead.
> > > 
> > > v2: Add CONFIG_DRM_I915_GUC_CTB_TIMEOUT config option
> > > 
> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > 
> > So this is a rant, but for upstream we really need to do better than
> > internal:
> > 
> > - The driver must work by default in the optimal configuration.
> > 
> > - Any config change that we haven't validated _must_ taint the kernel
> >   (this is especially for module options, but also for config settings)
> > 
> > - Config need a real reason beyond "was useful for bring-up".
> > 
> > Our internal tree is an absolute disaster right now, with multi-line
> > kernel configs (different on each platform) and bespoke kernel config or
> > the driver just fails. We're the expert on our own hw, we should know how
> > it works, not offload that to users essentially asking them "how shitty do
> > you think Intel hw is in responding timely".
> > 
> > Yes I know there's a lot of these there already, they don't make a lot of
> > sense either.
> > 
> > Except if there's a real reason for this (aside from us just offloading
> > testing to our users instead of doing it ourselves properly) I think we
> > should hardcode this, with a comment explaining why. Maybe with a switch
> > between the PF/VF case once that's landed.
> > 
> > > ---
> > >  drivers/gpu/drm/i915/Kconfig.profile      | 10 ++++++++++
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c |  5 ++++-
> > >  2 files changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
> > > index 39328567c200..0d5475b5f28a 100644
> > > --- a/drivers/gpu/drm/i915/Kconfig.profile
> > > +++ b/drivers/gpu/drm/i915/Kconfig.profile
> > > @@ -38,6 +38,16 @@ config DRM_I915_USERFAULT_AUTOSUSPEND
> > >  	  May be 0 to disable the extra delay and solely use the device level
> > >  	  runtime pm autosuspend delay tunable.
> > >  
> > > +config DRM_I915_GUC_CTB_TIMEOUT
> > > +	int "How long to wait for the GuC to make forward progress on CTBs (ms)"
> > > +	default 1500 # milliseconds
> > > +	range 10 60000
> > 
> > Also range is definitely off, drm/scheduler will probably nuke you
> > beforehand :-)
> > 
> > That's kinda another issue I have with all these kconfig knobs: Maybe we
> > need a knob for "relax with reset attempts, my workloads overload my gpus
> > routinely", which then scales _all_ timeouts proportionally. But letting
> > the user set them all, with silly combiniations like resetting the
> > workload before heartbeat or stuff like that doesn't make much sense.
> >
> 
> Yes, the code as is the user could do some wacky stuff that doesn't make
> sense at all.
>  
> > Anyway, tiny patch so hopefully I can leave this one out for now until
> > we've closed this.
> 
> No issue leaving this out as blocking CTBs are never really used anyways
> until SRIOV aside from setup / debugging. That being said, we might
> still want a higher hardcoded value in the meantime, perhaps around a
> second. I can follow up on that if needed.

Yeah just patch with updated hardcoded value sounds good to me.
-Daniel

> 
> Matt
> 
> > -Daniel
> > 
> > > +	help
> > > +	  Configures the default timeout waiting for GuC the to make forward
> > > +	  progress on CTBs. e.g. Waiting for a response to a requeset.
> > > +
> > > +	  A range of 10 ms to 60000 ms is allowed.
> > > +
> > >  config DRM_I915_HEARTBEAT_INTERVAL
> > >  	int "Interval between heartbeat pulses (ms)"
> > >  	default 2500 # milliseconds
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > > index 916c2b80c841..cf1fb09ef766 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > > @@ -436,6 +436,7 @@ static int ct_write(struct intel_guc_ct *ct,
> > >   */
> > >  static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
> > >  {
> > > +	long timeout;
> > >  	int err;
> > >  
> > >  	/*
> > > @@ -443,10 +444,12 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
> > >  	 * up to that length of time, then switch to a slower sleep-wait loop.
> > >  	 * No GuC command should ever take longer than 10ms.
> > >  	 */
> > > +	timeout = CONFIG_DRM_I915_GUC_CTB_TIMEOUT;
> > > +
> > >  #define done INTEL_GUC_MSG_IS_RESPONSE(READ_ONCE(req->status))
> > >  	err = wait_for_us(done, 10);
> > >  	if (err)
> > > -		err = wait_for(done, 10);
> > > +		err = wait_for(done, timeout);
> > >  #undef done
> > >  
> > >  	if (unlikely(err))
> > > -- 
> > > 2.28.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 14/20] drm/i915/guc: Start protecting access to CTB descriptors
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (12 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 13/20] drm/i915/guc: Relax CTB response timeout Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-04  8:35   ` Daniel Vetter
  2021-06-03  5:16 ` [PATCH 15/20] drm/i915/guc: Ensure H2G buffer updates visible before tail update Matthew Brost
                   ` (5 subsequent siblings)
  19 siblings, 1 reply; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

We want to stop using guc.send_mutex while sending CTB messages
so we have to start protecting access to CTB send descriptor.

For completeness protect also CTB receive descriptor.

Add spinlock to struct intel_guc_ct_buffer and start using it.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 14 ++++++++++++--
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h |  2 ++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index cf1fb09ef766..80976fe40fbf 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -89,6 +89,8 @@ static void ct_incoming_request_worker_func(struct work_struct *w);
  */
 void intel_guc_ct_init_early(struct intel_guc_ct *ct)
 {
+	spin_lock_init(&ct->ctbs.send.lock);
+	spin_lock_init(&ct->ctbs.recv.lock);
 	spin_lock_init(&ct->requests.lock);
 	INIT_LIST_HEAD(&ct->requests.pending);
 	INIT_LIST_HEAD(&ct->requests.incoming);
@@ -476,17 +478,22 @@ static int ct_send(struct intel_guc_ct *ct,
 	GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
 	GEM_BUG_ON(!response_buf && response_buf_size);
 
+	spin_lock_irqsave(&ct->ctbs.send.lock, flags);
+
 	fence = ct_get_next_fence(ct);
 	request.fence = fence;
 	request.status = 0;
 	request.response_len = response_buf_size;
 	request.response_buf = response_buf;
 
-	spin_lock_irqsave(&ct->requests.lock, flags);
+	spin_lock(&ct->requests.lock);
 	list_add_tail(&request.link, &ct->requests.pending);
-	spin_unlock_irqrestore(&ct->requests.lock, flags);
+	spin_unlock(&ct->requests.lock);
 
 	err = ct_write(ct, action, len, fence);
+
+	spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
+
 	if (unlikely(err))
 		goto unlink;
 
@@ -822,6 +829,7 @@ static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg)
 void intel_guc_ct_event_handler(struct intel_guc_ct *ct)
 {
 	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
+	unsigned long flags;
 	int err = 0;
 
 	if (unlikely(!ct->enabled)) {
@@ -830,7 +838,9 @@ void intel_guc_ct_event_handler(struct intel_guc_ct *ct)
 	}
 
 	do {
+		spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
 		err = ct_read(ct, msg);
+		spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
 		if (err)
 			break;
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
index fc9486779e87..bc52dc479a14 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
@@ -27,11 +27,13 @@ struct intel_guc;
  * record (command transport buffer descriptor) and the actual buffer which
  * holds the commands.
  *
+ * @lock: protects access to the commands buffer and buffer descriptor
  * @desc: pointer to the buffer descriptor
  * @cmds: pointer to the commands buffer
  * @size: size of the commands buffer
  */
 struct intel_guc_ct_buffer {
+	spinlock_t lock;
 	struct guc_ct_buffer_desc *desc;
 	u32 *cmds;
 	u32 size;
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [PATCH 14/20] drm/i915/guc: Start protecting access to CTB descriptors
  2021-06-03  5:16 ` [PATCH 14/20] drm/i915/guc: Start protecting access to CTB descriptors Matthew Brost
@ 2021-06-04  8:35   ` Daniel Vetter
  0 siblings, 0 replies; 41+ messages in thread
From: Daniel Vetter @ 2021-06-04  8:35 UTC (permalink / raw)
  To: Matthew Brost; +Cc: daniel.vetter, intel-gfx, dri-devel

On Wed, Jun 02, 2021 at 10:16:24PM -0700, Matthew Brost wrote:
> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 
> We want to stop using guc.send_mutex while sending CTB messages
> so we have to start protecting access to CTB send descriptor.
> 
> For completeness protect also CTB receive descriptor.
> 
> Add spinlock to struct intel_guc_ct_buffer and start using it.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 14 ++++++++++++--
>  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h |  2 ++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> index cf1fb09ef766..80976fe40fbf 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -89,6 +89,8 @@ static void ct_incoming_request_worker_func(struct work_struct *w);
>   */
>  void intel_guc_ct_init_early(struct intel_guc_ct *ct)
>  {
> +	spin_lock_init(&ct->ctbs.send.lock);
> +	spin_lock_init(&ct->ctbs.recv.lock);
>  	spin_lock_init(&ct->requests.lock);
>  	INIT_LIST_HEAD(&ct->requests.pending);
>  	INIT_LIST_HEAD(&ct->requests.incoming);
> @@ -476,17 +478,22 @@ static int ct_send(struct intel_guc_ct *ct,
>  	GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
>  	GEM_BUG_ON(!response_buf && response_buf_size);
>  
> +	spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> +
>  	fence = ct_get_next_fence(ct);
>  	request.fence = fence;
>  	request.status = 0;
>  	request.response_len = response_buf_size;
>  	request.response_buf = response_buf;
>  
> -	spin_lock_irqsave(&ct->requests.lock, flags);
> +	spin_lock(&ct->requests.lock);
>  	list_add_tail(&request.link, &ct->requests.pending);
> -	spin_unlock_irqrestore(&ct->requests.lock, flags);
> +	spin_unlock(&ct->requests.lock);
>  
>  	err = ct_write(ct, action, len, fence);
> +
> +	spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
> +
>  	if (unlikely(err))
>  		goto unlink;
>  
> @@ -822,6 +829,7 @@ static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg)
>  void intel_guc_ct_event_handler(struct intel_guc_ct *ct)
>  {
>  	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
> +	unsigned long flags;
>  	int err = 0;
>  
>  	if (unlikely(!ct->enabled)) {
> @@ -830,7 +838,9 @@ void intel_guc_ct_event_handler(struct intel_guc_ct *ct)
>  	}
>  
>  	do {
> +		spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
>  		err = ct_read(ct, msg);
> +		spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
>  		if (err)
>  			break;
>  
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
> index fc9486779e87..bc52dc479a14 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
> @@ -27,11 +27,13 @@ struct intel_guc;
>   * record (command transport buffer descriptor) and the actual buffer which
>   * holds the commands.
>   *
> + * @lock: protects access to the commands buffer and buffer descriptor
>   * @desc: pointer to the buffer descriptor
>   * @cmds: pointer to the commands buffer
>   * @size: size of the commands buffer
>   */
>  struct intel_guc_ct_buffer {
> +	spinlock_t lock;

For struct members inline comments _much_ preferred, since then you can
make them multi-line and actually explain stuff - the header format
strictly speaking also allows multi-line, but not with nice formatting and
so encourages people to lean way too much towards brevity.
Plus it's closer where people look (for big structs).

I guess add that to the kerneldoc cleanup work.
-Daniel

>  	struct guc_ct_buffer_desc *desc;
>  	u32 *cmds;
>  	u32 size;
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 15/20] drm/i915/guc: Ensure H2G buffer updates visible before tail update
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (13 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 14/20] drm/i915/guc: Start protecting access to CTB descriptors Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  9:44   ` [Intel-gfx] " Michal Wajdeczko
  2021-06-03  5:16 ` [PATCH 16/20] drm/i915/guc: Stop using mutex while sending CTB messages Matthew Brost
                   ` (4 subsequent siblings)
  19 siblings, 1 reply; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

Ensure H2G buffer updates are visible before descriptor tail updates by
inserting a barrier between the H2G buffer update and the tail. The
barrier is simple wmb() for SMEM and is register write for LMEM. This is
needed if more than 1 H2G can be inflight at once.

If this barrier is not inserted it is possible the descriptor tail
update is scene by the GuC before H2G buffer update which results in the
GuC reading a corrupt H2G value. This can bring down the H2G channel
among other bad things.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 28 +++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 80976fe40fbf..31f83956bfc3 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -328,6 +328,28 @@ static u32 ct_get_next_fence(struct intel_guc_ct *ct)
 	return ++ct->requests.last_fence;
 }
 
+static void write_barrier(struct intel_guc_ct *ct)
+{
+	struct intel_guc *guc = ct_to_guc(ct);
+	struct intel_gt *gt = guc_to_gt(guc);
+
+	if (i915_gem_object_is_lmem(guc->ct.vma->obj)) {
+		GEM_BUG_ON(guc->send_regs.fw_domains);
+		/*
+		 * This register is used by the i915 and GuC for MMIO based
+		 * communication. Once we are in this code CTBs are the only
+		 * method the i915 uses to communicate with the GuC so it is
+		 * safe to write to this register (a value of 0 is NOP for MMIO
+		 * communication). If we ever start mixing CTBs and MMIOs a new
+		 * register will have to be chosen.
+		 */
+		intel_uncore_write_fw(gt->uncore, GEN11_SOFT_SCRATCH(0), 0);
+	} else {
+		/* wmb() sufficient for a barrier if in smem */
+		wmb();
+	}
+}
+
 /**
  * DOC: CTB Host to GuC request
  *
@@ -411,6 +433,12 @@ static int ct_write(struct intel_guc_ct *ct,
 	}
 	GEM_BUG_ON(tail > size);
 
+	/*
+	 * make sure H2G buffer update and LRC tail update (if this triggering a
+	 * submission) are visible before updating the descriptor tail
+	 */
+	write_barrier(ct);
+
 	/* now update desc tail (back in bytes) */
 	desc->tail = tail * 4;
 	return 0;
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [Intel-gfx] [PATCH 15/20] drm/i915/guc: Ensure H2G buffer updates visible before tail update
  2021-06-03  5:16 ` [PATCH 15/20] drm/i915/guc: Ensure H2G buffer updates visible before tail update Matthew Brost
@ 2021-06-03  9:44   ` Michal Wajdeczko
  2021-06-03 16:10     ` Matthew Brost
  0 siblings, 1 reply; 41+ messages in thread
From: Michal Wajdeczko @ 2021-06-03  9:44 UTC (permalink / raw)
  To: Matthew Brost, intel-gfx, dri-devel; +Cc: daniel.vetter



On 03.06.2021 07:16, Matthew Brost wrote:
> Ensure H2G buffer updates are visible before descriptor tail updates by
> inserting a barrier between the H2G buffer update and the tail. The
> barrier is simple wmb() for SMEM and is register write for LMEM. This is
> needed if more than 1 H2G can be inflight at once.
> 
> If this barrier is not inserted it is possible the descriptor tail
> update is scene by the GuC before H2G buffer update which results in the
> GuC reading a corrupt H2G value. This can bring down the H2G channel
> among other bad things.
> 
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 28 +++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> index 80976fe40fbf..31f83956bfc3 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -328,6 +328,28 @@ static u32 ct_get_next_fence(struct intel_guc_ct *ct)
>  	return ++ct->requests.last_fence;
>  }
>  
> +static void write_barrier(struct intel_guc_ct *ct)
> +{
> +	struct intel_guc *guc = ct_to_guc(ct);
> +	struct intel_gt *gt = guc_to_gt(guc);
> +
> +	if (i915_gem_object_is_lmem(guc->ct.vma->obj)) {
> +		GEM_BUG_ON(guc->send_regs.fw_domains);
> +		/*
> +		 * This register is used by the i915 and GuC for MMIO based
> +		 * communication. Once we are in this code CTBs are the only
> +		 * method the i915 uses to communicate with the GuC so it is
> +		 * safe to write to this register (a value of 0 is NOP for MMIO
> +		 * communication). If we ever start mixing CTBs and MMIOs a new
> +		 * register will have to be chosen.
> +		 */
> +		intel_uncore_write_fw(gt->uncore, GEN11_SOFT_SCRATCH(0), 0);

can't we at least start with SOFT_SCRATCH register that is not used for
GuC MMIO based communication on Gen12 LMEM platforms? see [1]

I really don't feel comfortable that we are touching a register that
elsewhere is protected with the mutex. And mixing CTBs and MMIO is not
far away.

Michal

[1]
https://lore.kernel.org/intel-gfx/51b9bd05-7d6f-29f1-de0f-3a14bade6c9c@intel.com/

> +	} else {
> +		/* wmb() sufficient for a barrier if in smem */
> +		wmb();
> +	}
> +}
> +
>  /**
>   * DOC: CTB Host to GuC request
>   *
> @@ -411,6 +433,12 @@ static int ct_write(struct intel_guc_ct *ct,
>  	}
>  	GEM_BUG_ON(tail > size);
>  
> +	/*
> +	 * make sure H2G buffer update and LRC tail update (if this triggering a
> +	 * submission) are visible before updating the descriptor tail
> +	 */
> +	write_barrier(ct);
> +
>  	/* now update desc tail (back in bytes) */
>  	desc->tail = tail * 4;
>  	return 0;
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Intel-gfx] [PATCH 15/20] drm/i915/guc: Ensure H2G buffer updates visible before tail update
  2021-06-03  9:44   ` [Intel-gfx] " Michal Wajdeczko
@ 2021-06-03 16:10     ` Matthew Brost
  2021-06-04  8:39       ` Daniel Vetter
  0 siblings, 1 reply; 41+ messages in thread
From: Matthew Brost @ 2021-06-03 16:10 UTC (permalink / raw)
  To: Michal Wajdeczko; +Cc: daniel.vetter, intel-gfx, dri-devel

On Thu, Jun 03, 2021 at 11:44:57AM +0200, Michal Wajdeczko wrote:
> 
> 
> On 03.06.2021 07:16, Matthew Brost wrote:
> > Ensure H2G buffer updates are visible before descriptor tail updates by
> > inserting a barrier between the H2G buffer update and the tail. The
> > barrier is simple wmb() for SMEM and is register write for LMEM. This is
> > needed if more than 1 H2G can be inflight at once.
> > 
> > If this barrier is not inserted it is possible the descriptor tail
> > update is scene by the GuC before H2G buffer update which results in the
> > GuC reading a corrupt H2G value. This can bring down the H2G channel
> > among other bad things.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 28 +++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > index 80976fe40fbf..31f83956bfc3 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > @@ -328,6 +328,28 @@ static u32 ct_get_next_fence(struct intel_guc_ct *ct)
> >  	return ++ct->requests.last_fence;
> >  }
> >  
> > +static void write_barrier(struct intel_guc_ct *ct)
> > +{
> > +	struct intel_guc *guc = ct_to_guc(ct);
> > +	struct intel_gt *gt = guc_to_gt(guc);
> > +
> > +	if (i915_gem_object_is_lmem(guc->ct.vma->obj)) {
> > +		GEM_BUG_ON(guc->send_regs.fw_domains);
> > +		/*
> > +		 * This register is used by the i915 and GuC for MMIO based
> > +		 * communication. Once we are in this code CTBs are the only
> > +		 * method the i915 uses to communicate with the GuC so it is
> > +		 * safe to write to this register (a value of 0 is NOP for MMIO
> > +		 * communication). If we ever start mixing CTBs and MMIOs a new
> > +		 * register will have to be chosen.
> > +		 */
> > +		intel_uncore_write_fw(gt->uncore, GEN11_SOFT_SCRATCH(0), 0);
> 
> can't we at least start with SOFT_SCRATCH register that is not used for
> GuC MMIO based communication on Gen12 LMEM platforms? see [1]
> 

We likely can use this but I really don't feel comfortable switching the
register without some more testing first (e.g. let's change in this in
internal, let it soak for bit, then make the change upstream).

> I really don't feel comfortable that we are touching a register that
> elsewhere is protected with the mutex. And mixing CTBs and MMIO is not
> far away.
>

The only code that mixes CTBs and MMIOs is SRIOV which is a ways away
from landing.

Matt
 
> Michal
> 
> [1]
> https://lore.kernel.org/intel-gfx/51b9bd05-7d6f-29f1-de0f-3a14bade6c9c@intel.com/
> 
> > +	} else {
> > +		/* wmb() sufficient for a barrier if in smem */
> > +		wmb();
> > +	}
> > +}
> > +
> >  /**
> >   * DOC: CTB Host to GuC request
> >   *
> > @@ -411,6 +433,12 @@ static int ct_write(struct intel_guc_ct *ct,
> >  	}
> >  	GEM_BUG_ON(tail > size);
> >  
> > +	/*
> > +	 * make sure H2G buffer update and LRC tail update (if this triggering a
> > +	 * submission) are visible before updating the descriptor tail
> > +	 */
> > +	write_barrier(ct);
> > +
> >  	/* now update desc tail (back in bytes) */
> >  	desc->tail = tail * 4;
> >  	return 0;
> > 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [Intel-gfx] [PATCH 15/20] drm/i915/guc: Ensure H2G buffer updates visible before tail update
  2021-06-03 16:10     ` Matthew Brost
@ 2021-06-04  8:39       ` Daniel Vetter
  0 siblings, 0 replies; 41+ messages in thread
From: Daniel Vetter @ 2021-06-04  8:39 UTC (permalink / raw)
  To: Matthew Brost; +Cc: daniel.vetter, intel-gfx, dri-devel, Michal Wajdeczko

On Thu, Jun 03, 2021 at 09:10:14AM -0700, Matthew Brost wrote:
> On Thu, Jun 03, 2021 at 11:44:57AM +0200, Michal Wajdeczko wrote:
> > 
> > 
> > On 03.06.2021 07:16, Matthew Brost wrote:
> > > Ensure H2G buffer updates are visible before descriptor tail updates by
> > > inserting a barrier between the H2G buffer update and the tail. The
> > > barrier is simple wmb() for SMEM and is register write for LMEM. This is
> > > needed if more than 1 H2G can be inflight at once.
> > > 
> > > If this barrier is not inserted it is possible the descriptor tail
> > > update is scene by the GuC before H2G buffer update which results in the
> > > GuC reading a corrupt H2G value. This can bring down the H2G channel
> > > among other bad things.
> > > 
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 28 +++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > > index 80976fe40fbf..31f83956bfc3 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > > @@ -328,6 +328,28 @@ static u32 ct_get_next_fence(struct intel_guc_ct *ct)
> > >  	return ++ct->requests.last_fence;
> > >  }
> > >  
> > > +static void write_barrier(struct intel_guc_ct *ct)
> > > +{
> > > +	struct intel_guc *guc = ct_to_guc(ct);
> > > +	struct intel_gt *gt = guc_to_gt(guc);
> > > +
> > > +	if (i915_gem_object_is_lmem(guc->ct.vma->obj)) {
> > > +		GEM_BUG_ON(guc->send_regs.fw_domains);
> > > +		/*
> > > +		 * This register is used by the i915 and GuC for MMIO based
> > > +		 * communication. Once we are in this code CTBs are the only
> > > +		 * method the i915 uses to communicate with the GuC so it is
> > > +		 * safe to write to this register (a value of 0 is NOP for MMIO
> > > +		 * communication). If we ever start mixing CTBs and MMIOs a new
> > > +		 * register will have to be chosen.
> > > +		 */
> > > +		intel_uncore_write_fw(gt->uncore, GEN11_SOFT_SCRATCH(0), 0);
> > 
> > can't we at least start with SOFT_SCRATCH register that is not used for
> > GuC MMIO based communication on Gen12 LMEM platforms? see [1]
> > 
> 
> We likely can use this but I really don't feel comfortable switching the
> register without some more testing first (e.g. let's change in this in
> internal, let it soak for bit, then make the change upstream).
> 
> > I really don't feel comfortable that we are touching a register that
> > elsewhere is protected with the mutex. And mixing CTBs and MMIO is not
> > far away.
> >
> 
> The only code that mixes CTBs and MMIOs is SRIOV which is a ways away
> from landing.

Maybe add a FIXME note as part of the SRIOV patch stack in internal to
track this?
-Daniel

> 
> Matt
>  
> > Michal
> > 
> > [1]
> > https://lore.kernel.org/intel-gfx/51b9bd05-7d6f-29f1-de0f-3a14bade6c9c@intel.com/
> > 
> > > +	} else {
> > > +		/* wmb() sufficient for a barrier if in smem */
> > > +		wmb();
> > > +	}
> > > +}
> > > +
> > >  /**
> > >   * DOC: CTB Host to GuC request
> > >   *
> > > @@ -411,6 +433,12 @@ static int ct_write(struct intel_guc_ct *ct,
> > >  	}
> > >  	GEM_BUG_ON(tail > size);
> > >  
> > > +	/*
> > > +	 * make sure H2G buffer update and LRC tail update (if this triggering a
> > > +	 * submission) are visible before updating the descriptor tail
> > > +	 */
> > > +	write_barrier(ct);
> > > +
> > >  	/* now update desc tail (back in bytes) */
> > >  	desc->tail = tail * 4;
> > >  	return 0;
> > > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 16/20] drm/i915/guc: Stop using mutex while sending CTB messages
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (14 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 15/20] drm/i915/guc: Ensure H2G buffer updates visible before tail update Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 17/20] drm/i915/guc: Don't receive all G2H messages in irq handler Matthew Brost
                   ` (3 subsequent siblings)
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

We are no longer using descriptor to hold G2H replies and we are
protecting access to the descriptor and command buffer by the
separate spinlock, so we can stop using mutex.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 31f83956bfc3..372735c7f5e7 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -562,7 +562,6 @@ static int ct_send(struct intel_guc_ct *ct,
 int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,
 		      u32 *response_buf, u32 response_buf_size)
 {
-	struct intel_guc *guc = ct_to_guc(ct);
 	u32 status = ~0; /* undefined */
 	int ret;
 
@@ -571,8 +570,6 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,
 		return -ENODEV;
 	}
 
-	mutex_lock(&guc->send_mutex);
-
 	ret = ct_send(ct, action, len, response_buf, response_buf_size, &status);
 	if (unlikely(ret < 0)) {
 		CT_ERROR(ct, "Sending action %#x failed (err=%d status=%#X)\n",
@@ -582,7 +579,6 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,
 			 action[0], ret, ret);
 	}
 
-	mutex_unlock(&guc->send_mutex);
 	return ret;
 }
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 17/20] drm/i915/guc: Don't receive all G2H messages in irq handler
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (15 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 16/20] drm/i915/guc: Stop using mutex while sending CTB messages Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 18/20] drm/i915/guc: Always copy CT message to new allocation Matthew Brost
                   ` (2 subsequent siblings)
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

In irq handler try to receive just single G2H message, let other
messages to be received from tasklet.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 67 ++++++++++++++++-------
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h |  3 +
 2 files changed, 50 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 372735c7f5e7..4fac9e4bced4 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -81,6 +81,7 @@ enum { CTB_SEND = 0, CTB_RECV = 1 };
 
 enum { CTB_OWNER_HOST = 0 };
 
+static void ct_receive_tasklet_func(struct tasklet_struct *t);
 static void ct_incoming_request_worker_func(struct work_struct *w);
 
 /**
@@ -95,6 +96,7 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)
 	INIT_LIST_HEAD(&ct->requests.pending);
 	INIT_LIST_HEAD(&ct->requests.incoming);
 	INIT_WORK(&ct->requests.worker, ct_incoming_request_worker_func);
+	tasklet_setup(&ct->receive_tasklet, ct_receive_tasklet_func);
 }
 
 static inline const char *guc_ct_buffer_type_to_str(u32 type)
@@ -244,6 +246,7 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct)
 {
 	GEM_BUG_ON(ct->enabled);
 
+	tasklet_kill(&ct->receive_tasklet);
 	i915_vma_unpin_and_release(&ct->vma, I915_VMA_RELEASE_MAP);
 	memset(ct, 0, sizeof(*ct));
 }
@@ -654,7 +657,7 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data)
 	CT_DEBUG(ct, "received %*ph\n", 4 * len, data);
 
 	desc->head = head * 4;
-	return 0;
+	return available - len;
 
 corrupted:
 	CT_ERROR(ct, "Corrupted descriptor addr=%#x head=%u tail=%u size=%u\n",
@@ -690,10 +693,10 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 	u32 status;
 	u32 datalen;
 	struct ct_request *req;
+	unsigned long flags;
 	bool found = false;
 
 	GEM_BUG_ON(!ct_header_is_response(header));
-	GEM_BUG_ON(!in_irq());
 
 	/* Response payload shall at least include fence and status */
 	if (unlikely(len < 2)) {
@@ -713,7 +716,7 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 
 	CT_DEBUG(ct, "response fence %u status %#x\n", fence, status);
 
-	spin_lock(&ct->requests.lock);
+	spin_lock_irqsave(&ct->requests.lock, flags);
 	list_for_each_entry(req, &ct->requests.pending, link) {
 		if (unlikely(fence != req->fence)) {
 			CT_DEBUG(ct, "request %u awaits response\n",
@@ -732,7 +735,7 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 		found = true;
 		break;
 	}
-	spin_unlock(&ct->requests.lock);
+	spin_unlock_irqrestore(&ct->requests.lock, flags);
 
 	if (!found)
 		CT_ERROR(ct, "Unsolicited response %*ph\n", msgsize, msg);
@@ -846,31 +849,55 @@ static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg)
 	return 0;
 }
 
+static int ct_receive(struct intel_guc_ct *ct)
+{
+	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
+	ret = ct_read(ct, msg);
+	spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
+	if (ret < 0)
+		return ret;
+
+	if (ct_header_is_response(msg[0]))
+		ct_handle_response(ct, msg);
+	else
+		ct_handle_request(ct, msg);
+
+	return ret;
+}
+
+static void ct_try_receive_message(struct intel_guc_ct *ct)
+{
+	int ret;
+
+	if (GEM_WARN_ON(!ct->enabled))
+		return;
+
+	ret = ct_receive(ct);
+	if (ret > 0)
+		tasklet_hi_schedule(&ct->receive_tasklet);
+}
+
+static void ct_receive_tasklet_func(struct tasklet_struct *t)
+{
+	struct intel_guc_ct *ct = from_tasklet(ct, t, receive_tasklet);
+
+	ct_try_receive_message(ct);
+}
+
 /*
  * When we're communicating with the GuC over CT, GuC uses events
  * to notify us about new messages being posted on the RECV buffer.
  */
 void intel_guc_ct_event_handler(struct intel_guc_ct *ct)
 {
-	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
-	unsigned long flags;
-	int err = 0;
-
 	if (unlikely(!ct->enabled)) {
 		WARN(1, "Unexpected GuC event received while CT disabled!\n");
 		return;
 	}
 
-	do {
-		spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
-		err = ct_read(ct, msg);
-		spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
-		if (err)
-			break;
-
-		if (ct_header_is_response(msg[0]))
-			err = ct_handle_response(ct, msg);
-		else
-			err = ct_handle_request(ct, msg);
-	} while (!err);
+	ct_try_receive_message(ct);
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
index bc52dc479a14..cb222f202301 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
@@ -6,6 +6,7 @@
 #ifndef _INTEL_GUC_CT_H_
 #define _INTEL_GUC_CT_H_
 
+#include <linux/interrupt.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 
@@ -55,6 +56,8 @@ struct intel_guc_ct {
 		struct intel_guc_ct_buffer recv;
 	} ctbs;
 
+	struct tasklet_struct receive_tasklet;
+
 	struct {
 		u32 last_fence; /* last fence used to send request */
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 18/20] drm/i915/guc: Always copy CT message to new allocation
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (16 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 17/20] drm/i915/guc: Don't receive all G2H messages in irq handler Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 19/20] drm/i915/guc: Early initialization of GuC send registers Matthew Brost
  2021-06-03  5:16 ` [PATCH 20/20] drm/i915/guc: Use guc_class instead of engine_class in fw interface Matthew Brost
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

Since most of future CT traffic will be based on G2H requests,
instead of copying incoming CT message to static buffer and then
create new allocation for such request, always copy incoming CT
message to new allocation. Also by doing it while reading CT
header, we can safely fallback if that atomic allocation fails.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
Cc: Piotr Piórkowski <piotr.piorkowski@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 180 ++++++++++++++--------
 1 file changed, 120 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index 4fac9e4bced4..8869f9ebb4c8 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -72,8 +72,9 @@ struct ct_request {
 	u32 *response_buf;
 };
 
-struct ct_incoming_request {
+struct ct_incoming_msg {
 	struct list_head link;
+	u32 size;
 	u32 msg[];
 };
 
@@ -600,7 +601,26 @@ static inline bool ct_header_is_response(u32 header)
 	return !!(header & GUC_CT_MSG_IS_RESPONSE);
 }
 
-static int ct_read(struct intel_guc_ct *ct, u32 *data)
+static struct ct_incoming_msg *ct_alloc_msg(u32 num_dwords)
+{
+	struct ct_incoming_msg *msg;
+
+	msg = kmalloc(sizeof(*msg) + sizeof(u32) * num_dwords, GFP_ATOMIC);
+	if (msg)
+		msg->size = num_dwords;
+	return msg;
+}
+
+static void ct_free_msg(struct ct_incoming_msg *msg)
+{
+	kfree(msg);
+}
+
+/*
+ * Return: number available remaining dwords to read (0 if empty)
+ *         or a negative error code on failure
+ */
+static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg)
 {
 	struct intel_guc_ct_buffer *ctb = &ct->ctbs.recv;
 	struct guc_ct_buffer_desc *desc = ctb->desc;
@@ -611,6 +631,7 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data)
 	s32 available;
 	unsigned int len;
 	unsigned int i;
+	u32 header;
 
 	if (unlikely(desc->is_in_error))
 		return -EPIPE;
@@ -626,8 +647,10 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data)
 
 	/* tail == head condition indicates empty */
 	available = tail - head;
-	if (unlikely(available == 0))
-		return -ENODATA;
+	if (unlikely(available == 0)) {
+		*msg = NULL;
+		return 0;
+	}
 
 	/* beware of buffer wrap case */
 	if (unlikely(available < 0))
@@ -635,14 +658,14 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data)
 	CT_DEBUG(ct, "available %d (%u:%u)\n", available, head, tail);
 	GEM_BUG_ON(available < 0);
 
-	data[0] = cmds[head];
+	header = cmds[head];
 	head = (head + 1) % size;
 
 	/* message len with header */
-	len = ct_header_get_len(data[0]) + 1;
+	len = ct_header_get_len(header) + 1;
 	if (unlikely(len > (u32)available)) {
 		CT_ERROR(ct, "Incomplete message %*ph %*ph %*ph\n",
-			 4, data,
+			 4, &header,
 			 4 * (head + available - 1 > size ?
 			      size - head : available - 1), &cmds[head],
 			 4 * (head + available - 1 > size ?
@@ -650,11 +673,24 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data)
 		goto corrupted;
 	}
 
+	*msg = ct_alloc_msg(len);
+	if (!*msg) {
+		CT_ERROR(ct, "No memory for message %*ph %*ph %*ph\n",
+			 4, &header,
+			 4 * (head + available - 1 > size ?
+			      size - head : available - 1), &cmds[head],
+			 4 * (head + available - 1 > size ?
+			      available - 1 - size + head : 0), &cmds[0]);
+		return available;
+	}
+
+	(*msg)->msg[0] = header;
+
 	for (i = 1; i < len; i++) {
-		data[i] = cmds[head];
+		(*msg)->msg[i] = cmds[head];
 		head = (head + 1) % size;
 	}
-	CT_DEBUG(ct, "received %*ph\n", 4 * len, data);
+	CT_DEBUG(ct, "received %*ph\n", 4 * len, (*msg)->msg);
 
 	desc->head = head * 4;
 	return available - len;
@@ -684,33 +720,33 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data)
  *                   ^-----------------------len-----------------------^
  */
 
-static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
+static int ct_handle_response(struct intel_guc_ct *ct, struct ct_incoming_msg *response)
 {
-	u32 header = msg[0];
+	u32 header = response->msg[0];
 	u32 len = ct_header_get_len(header);
-	u32 msgsize = (len + 1) * sizeof(u32); /* msg size in bytes w/header */
 	u32 fence;
 	u32 status;
 	u32 datalen;
 	struct ct_request *req;
 	unsigned long flags;
 	bool found = false;
+	int err = 0;
 
 	GEM_BUG_ON(!ct_header_is_response(header));
 
 	/* Response payload shall at least include fence and status */
 	if (unlikely(len < 2)) {
-		CT_ERROR(ct, "Corrupted response %*ph\n", msgsize, msg);
+		CT_ERROR(ct, "Corrupted response (len %u)\n", len);
 		return -EPROTO;
 	}
 
-	fence = msg[1];
-	status = msg[2];
+	fence = response->msg[1];
+	status = response->msg[2];
 	datalen = len - 2;
 
 	/* Format of the status follows RESPONSE message */
 	if (unlikely(!INTEL_GUC_MSG_IS_RESPONSE(status))) {
-		CT_ERROR(ct, "Corrupted response %*ph\n", msgsize, msg);
+		CT_ERROR(ct, "Corrupted response (status %#x)\n", status);
 		return -EPROTO;
 	}
 
@@ -724,12 +760,13 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 			continue;
 		}
 		if (unlikely(datalen > req->response_len)) {
-			CT_ERROR(ct, "Response for %u is too long %*ph\n",
-				 req->fence, msgsize, msg);
-			datalen = 0;
+			CT_ERROR(ct, "Response %u too long (datalen %u > %u)\n",
+				 req->fence, datalen, req->response_len);
+			datalen = min(datalen, req->response_len);
+			err = -EMSGSIZE;
 		}
 		if (datalen)
-			memcpy(req->response_buf, msg + 3, 4 * datalen);
+			memcpy(req->response_buf, response->msg + 3, 4 * datalen);
 		req->response_len = datalen;
 		WRITE_ONCE(req->status, status);
 		found = true;
@@ -737,45 +774,61 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 	}
 	spin_unlock_irqrestore(&ct->requests.lock, flags);
 
-	if (!found)
-		CT_ERROR(ct, "Unsolicited response %*ph\n", msgsize, msg);
+	if (!found) {
+		CT_ERROR(ct, "Unsolicited response (fence %u)\n", fence);
+		return -ENOKEY;
+	}
+
+	if (unlikely(err))
+		return err;
+
+	ct_free_msg(response);
 	return 0;
 }
 
-static void ct_process_request(struct intel_guc_ct *ct,
-			       u32 action, u32 len, const u32 *payload)
+static int ct_process_request(struct intel_guc_ct *ct, struct ct_incoming_msg *request)
 {
 	struct intel_guc *guc = ct_to_guc(ct);
+	u32 header, action, len;
+	const u32 *payload;
 	int ret;
 
+	header = request->msg[0];
+	payload = &request->msg[1];
+	action = ct_header_get_action(header);
+	len = ct_header_get_len(header);
+
 	CT_DEBUG(ct, "request %x %*ph\n", action, 4 * len, payload);
 
 	switch (action) {
 	case INTEL_GUC_ACTION_DEFAULT:
 		ret = intel_guc_to_host_process_recv_msg(guc, payload, len);
-		if (unlikely(ret))
-			goto fail_unexpected;
 		break;
-
 	default:
-fail_unexpected:
-		CT_ERROR(ct, "Unexpected request %x %*ph\n",
-			 action, 4 * len, payload);
+		ret = -EOPNOTSUPP;
 		break;
 	}
+
+	if (unlikely(ret)) {
+		CT_ERROR(ct, "Failed to process request %04x (%pe)\n",
+			 action, ERR_PTR(ret));
+		return ret;
+	}
+
+	ct_free_msg(request);
+	return 0;
 }
 
 static bool ct_process_incoming_requests(struct intel_guc_ct *ct)
 {
 	unsigned long flags;
-	struct ct_incoming_request *request;
-	u32 header;
-	u32 *payload;
+	struct ct_incoming_msg *request;
 	bool done;
+	int err;
 
 	spin_lock_irqsave(&ct->requests.lock, flags);
 	request = list_first_entry_or_null(&ct->requests.incoming,
-					   struct ct_incoming_request, link);
+					   struct ct_incoming_msg, link);
 	if (request)
 		list_del(&request->link);
 	done = !!list_empty(&ct->requests.incoming);
@@ -784,14 +837,13 @@ static bool ct_process_incoming_requests(struct intel_guc_ct *ct)
 	if (!request)
 		return true;
 
-	header = request->msg[0];
-	payload = &request->msg[1];
-	ct_process_request(ct,
-			   ct_header_get_action(header),
-			   ct_header_get_len(header),
-			   payload);
+	err = ct_process_request(ct, request);
+	if (unlikely(err)) {
+		CT_ERROR(ct, "Failed to process CT message (%pe) %*ph\n",
+			 ERR_PTR(err), 4 * request->size, request->msg);
+		ct_free_msg(request);
+	}
 
-	kfree(request);
 	return done;
 }
 
@@ -824,22 +876,11 @@ static void ct_incoming_request_worker_func(struct work_struct *w)
  *                   ^-----------------------len-----------------------^
  */
 
-static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg)
+static int ct_handle_request(struct intel_guc_ct *ct, struct ct_incoming_msg *request)
 {
-	u32 header = msg[0];
-	u32 len = ct_header_get_len(header);
-	u32 msgsize = (len + 1) * sizeof(u32); /* msg size in bytes w/header */
-	struct ct_incoming_request *request;
 	unsigned long flags;
 
-	GEM_BUG_ON(ct_header_is_response(header));
-
-	request = kmalloc(sizeof(*request) + msgsize, GFP_ATOMIC);
-	if (unlikely(!request)) {
-		CT_ERROR(ct, "Dropping request %*ph\n", msgsize, msg);
-		return 0; /* XXX: -ENOMEM ? */
-	}
-	memcpy(request->msg, msg, msgsize);
+	GEM_BUG_ON(ct_header_is_response(request->msg[0]));
 
 	spin_lock_irqsave(&ct->requests.lock, flags);
 	list_add_tail(&request->link, &ct->requests.incoming);
@@ -849,22 +890,41 @@ static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg)
 	return 0;
 }
 
+static void ct_handle_msg(struct intel_guc_ct *ct, struct ct_incoming_msg *msg)
+{
+	u32 header = msg->msg[0];
+	int err;
+
+	if (ct_header_is_response(header))
+		err = ct_handle_response(ct, msg);
+	else
+		err = ct_handle_request(ct, msg);
+
+	if (unlikely(err)) {
+		CT_ERROR(ct, "Failed to process CT message (%pe) %*ph\n",
+			 ERR_PTR(err), 4 * msg->size, msg->msg);
+		ct_free_msg(msg);
+	}
+}
+
+/*
+ * Return: number available remaining dwords to read (0 if empty)
+ *         or a negative error code on failure
+ */
 static int ct_receive(struct intel_guc_ct *ct)
 {
-	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
+	struct ct_incoming_msg *msg = NULL;
 	unsigned long flags;
 	int ret;
 
 	spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
-	ret = ct_read(ct, msg);
+	ret = ct_read(ct, &msg);
 	spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
 	if (ret < 0)
 		return ret;
 
-	if (ct_header_is_response(msg[0]))
-		ct_handle_response(ct, msg);
-	else
-		ct_handle_request(ct, msg);
+	if (msg)
+		ct_handle_msg(ct, msg);
 
 	return ret;
 }
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 19/20] drm/i915/guc: Early initialization of GuC send registers
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (17 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 18/20] drm/i915/guc: Always copy CT message to new allocation Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-03  5:16 ` [PATCH 20/20] drm/i915/guc: Use guc_class instead of engine_class in fw interface Matthew Brost
  19 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Michal Wajdeczko <michal.wajdeczko@intel.com>

Base offset and count of the GuC scratch registers, used for
sending MMIO messages to GuC, can be initialized earlier with
other GuC members that also depends on platform.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index 18da9ed15728..fcfa4fd93841 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -60,15 +60,8 @@ void intel_guc_init_send_regs(struct intel_guc *guc)
 	enum forcewake_domains fw_domains = 0;
 	unsigned int i;
 
-	if (INTEL_GEN(gt->i915) >= 11) {
-		guc->send_regs.base =
-				i915_mmio_reg_offset(GEN11_SOFT_SCRATCH(0));
-		guc->send_regs.count = GEN11_SOFT_SCRATCH_COUNT;
-	} else {
-		guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
-		guc->send_regs.count = GUC_MAX_MMIO_MSG_LEN;
-		BUILD_BUG_ON(GUC_MAX_MMIO_MSG_LEN > SOFT_SCRATCH_COUNT);
-	}
+	GEM_BUG_ON(!guc->send_regs.base);
+	GEM_BUG_ON(!guc->send_regs.count);
 
 	for (i = 0; i < guc->send_regs.count; i++) {
 		fw_domains |= intel_uncore_forcewake_for_reg(gt->uncore,
@@ -172,11 +165,18 @@ void intel_guc_init_early(struct intel_guc *guc)
 		guc->interrupts.reset = gen11_reset_guc_interrupts;
 		guc->interrupts.enable = gen11_enable_guc_interrupts;
 		guc->interrupts.disable = gen11_disable_guc_interrupts;
+		guc->send_regs.base =
+			i915_mmio_reg_offset(GEN11_SOFT_SCRATCH(0));
+		guc->send_regs.count = GEN11_SOFT_SCRATCH_COUNT;
+
 	} else {
 		guc->notify_reg = GUC_SEND_INTERRUPT;
 		guc->interrupts.reset = gen9_reset_guc_interrupts;
 		guc->interrupts.enable = gen9_enable_guc_interrupts;
 		guc->interrupts.disable = gen9_disable_guc_interrupts;
+		guc->send_regs.base = i915_mmio_reg_offset(SOFT_SCRATCH(0));
+		guc->send_regs.count = GUC_MAX_MMIO_MSG_LEN;
+		BUILD_BUG_ON(GUC_MAX_MMIO_MSG_LEN > SOFT_SCRATCH_COUNT);
 	}
 }
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 20/20] drm/i915/guc: Use guc_class instead of engine_class in fw interface
  2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
                   ` (18 preceding siblings ...)
  2021-06-03  5:16 ` [PATCH 19/20] drm/i915/guc: Early initialization of GuC send registers Matthew Brost
@ 2021-06-03  5:16 ` Matthew Brost
  2021-06-04  8:44   ` Daniel Vetter
  19 siblings, 1 reply; 41+ messages in thread
From: Matthew Brost @ 2021-06-03  5:16 UTC (permalink / raw)
  To: intel-gfx, dri-devel; +Cc: daniel.vetter

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

GuC has its own defines for the engine classes. They're currently
mapping 1:1 to the defines used by the driver, but there is no guarantee
this will continue in the future. Given that we've been caught off-guard
in the past by similar divergences, we can prepare for the changes by
introducing helper functions to convert from engine class to GuC class and
back again.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c   |  6 +++--
 drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c  | 20 +++++++++-------
 drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 26 +++++++++++++++++++++
 3 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 3f9a811eb02b..69281b5aba51 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -265,6 +265,7 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
 	const struct engine_info *info = &intel_engines[id];
 	struct drm_i915_private *i915 = gt->i915;
 	struct intel_engine_cs *engine;
+	u8 guc_class;
 
 	BUILD_BUG_ON(MAX_ENGINE_CLASS >= BIT(GEN11_ENGINE_CLASS_WIDTH));
 	BUILD_BUG_ON(MAX_ENGINE_INSTANCE >= BIT(GEN11_ENGINE_INSTANCE_WIDTH));
@@ -293,9 +294,10 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
 	engine->i915 = i915;
 	engine->gt = gt;
 	engine->uncore = gt->uncore;
-	engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
 	engine->hw_id = info->hw_id;
-	engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
+	guc_class = engine_class_to_guc_class(info->class);
+	engine->guc_id = MAKE_GUC_ID(guc_class, info->instance);
+	engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
 
 	engine->irq_handler = nop_irq_handler;
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
index 17526717368c..efdce309b6f1 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
@@ -6,6 +6,7 @@
 #include "gt/intel_gt.h"
 #include "gt/intel_lrc.h"
 #include "intel_guc_ads.h"
+#include "intel_guc_fwif.h"
 #include "intel_uc.h"
 #include "i915_drv.h"
 
@@ -104,7 +105,7 @@ static void guc_mapping_table_init(struct intel_gt *gt,
 				GUC_MAX_INSTANCES_PER_CLASS;
 
 	for_each_engine(engine, gt, id) {
-		u8 guc_class = engine->class;
+		u8 guc_class = engine_class_to_guc_class(engine->class);
 
 		system_info->mapping_table[guc_class][engine->instance] =
 			engine->instance;
@@ -124,7 +125,7 @@ static void __guc_ads_init(struct intel_guc *guc)
 	struct __guc_ads_blob *blob = guc->ads_blob;
 	const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE;
 	u32 base;
-	u8 engine_class;
+	u8 engine_class, guc_class;
 
 	/* GuC scheduling policies */
 	guc_policies_init(&blob->policies);
@@ -140,22 +141,25 @@ static void __guc_ads_init(struct intel_guc *guc)
 	for (engine_class = 0; engine_class <= MAX_ENGINE_CLASS; ++engine_class) {
 		if (engine_class == OTHER_CLASS)
 			continue;
+
+		guc_class = engine_class_to_guc_class(engine_class);
+
 		/*
 		 * TODO: Set context pointer to default state to allow
 		 * GuC to re-init guilty contexts after internal reset.
 		 */
-		blob->ads.golden_context_lrca[engine_class] = 0;
-		blob->ads.eng_state_size[engine_class] =
+		blob->ads.golden_context_lrca[guc_class] = 0;
+		blob->ads.eng_state_size[guc_class] =
 			intel_engine_context_size(guc_to_gt(guc),
 						  engine_class) -
 			skipped_size;
 	}
 
 	/* System info */
-	blob->system_info.engine_enabled_masks[RENDER_CLASS] = 1;
-	blob->system_info.engine_enabled_masks[COPY_ENGINE_CLASS] = 1;
-	blob->system_info.engine_enabled_masks[VIDEO_DECODE_CLASS] = VDBOX_MASK(gt);
-	blob->system_info.engine_enabled_masks[VIDEO_ENHANCEMENT_CLASS] = VEBOX_MASK(gt);
+	blob->system_info.engine_enabled_masks[GUC_RENDER_CLASS] = 1;
+	blob->system_info.engine_enabled_masks[GUC_BLITTER_CLASS] = 1;
+	blob->system_info.engine_enabled_masks[GUC_VIDEO_CLASS] = VDBOX_MASK(gt);
+	blob->system_info.engine_enabled_masks[GUC_VIDEOENHANCE_CLASS] = VEBOX_MASK(gt);
 
 	blob->system_info.generic_gt_sysinfo[GUC_GENERIC_GT_SYSINFO_SLICE_ENABLED] =
 		hweight8(gt->info.sseu.slice_mask);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
index 9bf35240e723..e9a9d85e2aa3 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
@@ -9,6 +9,7 @@
 #include <linux/bits.h>
 #include <linux/compiler.h>
 #include <linux/types.h>
+#include "gt/intel_engine_types.h"
 
 #include "abi/guc_actions_abi.h"
 #include "abi/guc_errors_abi.h"
@@ -32,6 +33,12 @@
 #define GUC_VIDEO_ENGINE2		4
 #define GUC_MAX_ENGINES_NUM		(GUC_VIDEO_ENGINE2 + 1)
 
+#define GUC_RENDER_CLASS		0
+#define GUC_VIDEO_CLASS			1
+#define GUC_VIDEOENHANCE_CLASS		2
+#define GUC_BLITTER_CLASS		3
+#define GUC_RESERVED_CLASS		4
+#define GUC_LAST_ENGINE_CLASS		GUC_RESERVED_CLASS
 #define GUC_MAX_ENGINE_CLASSES		16
 #define GUC_MAX_INSTANCES_PER_CLASS	32
 
@@ -129,6 +136,25 @@
 #define GUC_ID_TO_ENGINE_INSTANCE(guc_id) \
 	(((guc_id) & GUC_ENGINE_INSTANCE_MASK) >> GUC_ENGINE_INSTANCE_SHIFT)
 
+static inline u8 engine_class_to_guc_class(u8 class)
+{
+	BUILD_BUG_ON(GUC_RENDER_CLASS != RENDER_CLASS);
+	BUILD_BUG_ON(GUC_BLITTER_CLASS != COPY_ENGINE_CLASS);
+	BUILD_BUG_ON(GUC_VIDEO_CLASS != VIDEO_DECODE_CLASS);
+	BUILD_BUG_ON(GUC_VIDEOENHANCE_CLASS != VIDEO_ENHANCEMENT_CLASS);
+	GEM_BUG_ON(class > MAX_ENGINE_CLASS || class == OTHER_CLASS);
+
+	return class;
+}
+
+static inline u8 guc_class_to_engine_class(u8 guc_class)
+{
+	GEM_BUG_ON(guc_class > GUC_LAST_ENGINE_CLASS);
+	GEM_BUG_ON(guc_class == GUC_RESERVED_CLASS);
+
+	return guc_class;
+}
+
 /* Work item for submitting workloads into work queue of GuC. */
 struct guc_wq_item {
 	u32 header;
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [PATCH 20/20] drm/i915/guc: Use guc_class instead of engine_class in fw interface
  2021-06-03  5:16 ` [PATCH 20/20] drm/i915/guc: Use guc_class instead of engine_class in fw interface Matthew Brost
@ 2021-06-04  8:44   ` Daniel Vetter
  2021-06-04 18:12     ` Matthew Brost
  0 siblings, 1 reply; 41+ messages in thread
From: Daniel Vetter @ 2021-06-04  8:44 UTC (permalink / raw)
  To: Matthew Brost; +Cc: daniel.vetter, intel-gfx, dri-devel

On Wed, Jun 02, 2021 at 10:16:30PM -0700, Matthew Brost wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> GuC has its own defines for the engine classes. They're currently
> mapping 1:1 to the defines used by the driver, but there is no guarantee
> this will continue in the future. Given that we've been caught off-guard
> in the past by similar divergences, we can prepare for the changes by
> introducing helper functions to convert from engine class to GuC class and
> back again.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>

Applied all up to this, except the Kconfig one.

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c   |  6 +++--
>  drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c  | 20 +++++++++-------
>  drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 26 +++++++++++++++++++++
>  3 files changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 3f9a811eb02b..69281b5aba51 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -265,6 +265,7 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
>  	const struct engine_info *info = &intel_engines[id];
>  	struct drm_i915_private *i915 = gt->i915;
>  	struct intel_engine_cs *engine;
> +	u8 guc_class;
>  
>  	BUILD_BUG_ON(MAX_ENGINE_CLASS >= BIT(GEN11_ENGINE_CLASS_WIDTH));
>  	BUILD_BUG_ON(MAX_ENGINE_INSTANCE >= BIT(GEN11_ENGINE_INSTANCE_WIDTH));
> @@ -293,9 +294,10 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
>  	engine->i915 = i915;
>  	engine->gt = gt;
>  	engine->uncore = gt->uncore;
> -	engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
>  	engine->hw_id = info->hw_id;
> -	engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
> +	guc_class = engine_class_to_guc_class(info->class);
> +	engine->guc_id = MAKE_GUC_ID(guc_class, info->instance);
> +	engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
>  
>  	engine->irq_handler = nop_irq_handler;
>  
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> index 17526717368c..efdce309b6f1 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> @@ -6,6 +6,7 @@
>  #include "gt/intel_gt.h"
>  #include "gt/intel_lrc.h"
>  #include "intel_guc_ads.h"
> +#include "intel_guc_fwif.h"
>  #include "intel_uc.h"
>  #include "i915_drv.h"
>  
> @@ -104,7 +105,7 @@ static void guc_mapping_table_init(struct intel_gt *gt,
>  				GUC_MAX_INSTANCES_PER_CLASS;
>  
>  	for_each_engine(engine, gt, id) {
> -		u8 guc_class = engine->class;
> +		u8 guc_class = engine_class_to_guc_class(engine->class);
>  
>  		system_info->mapping_table[guc_class][engine->instance] =
>  			engine->instance;
> @@ -124,7 +125,7 @@ static void __guc_ads_init(struct intel_guc *guc)
>  	struct __guc_ads_blob *blob = guc->ads_blob;
>  	const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE;
>  	u32 base;
> -	u8 engine_class;
> +	u8 engine_class, guc_class;
>  
>  	/* GuC scheduling policies */
>  	guc_policies_init(&blob->policies);
> @@ -140,22 +141,25 @@ static void __guc_ads_init(struct intel_guc *guc)
>  	for (engine_class = 0; engine_class <= MAX_ENGINE_CLASS; ++engine_class) {
>  		if (engine_class == OTHER_CLASS)
>  			continue;
> +
> +		guc_class = engine_class_to_guc_class(engine_class);
> +
>  		/*
>  		 * TODO: Set context pointer to default state to allow
>  		 * GuC to re-init guilty contexts after internal reset.
>  		 */
> -		blob->ads.golden_context_lrca[engine_class] = 0;
> -		blob->ads.eng_state_size[engine_class] =
> +		blob->ads.golden_context_lrca[guc_class] = 0;
> +		blob->ads.eng_state_size[guc_class] =
>  			intel_engine_context_size(guc_to_gt(guc),
>  						  engine_class) -
>  			skipped_size;
>  	}
>  
>  	/* System info */
> -	blob->system_info.engine_enabled_masks[RENDER_CLASS] = 1;
> -	blob->system_info.engine_enabled_masks[COPY_ENGINE_CLASS] = 1;
> -	blob->system_info.engine_enabled_masks[VIDEO_DECODE_CLASS] = VDBOX_MASK(gt);
> -	blob->system_info.engine_enabled_masks[VIDEO_ENHANCEMENT_CLASS] = VEBOX_MASK(gt);
> +	blob->system_info.engine_enabled_masks[GUC_RENDER_CLASS] = 1;
> +	blob->system_info.engine_enabled_masks[GUC_BLITTER_CLASS] = 1;
> +	blob->system_info.engine_enabled_masks[GUC_VIDEO_CLASS] = VDBOX_MASK(gt);
> +	blob->system_info.engine_enabled_masks[GUC_VIDEOENHANCE_CLASS] = VEBOX_MASK(gt);
>  
>  	blob->system_info.generic_gt_sysinfo[GUC_GENERIC_GT_SYSINFO_SLICE_ENABLED] =
>  		hweight8(gt->info.sseu.slice_mask);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> index 9bf35240e723..e9a9d85e2aa3 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -9,6 +9,7 @@
>  #include <linux/bits.h>
>  #include <linux/compiler.h>
>  #include <linux/types.h>
> +#include "gt/intel_engine_types.h"
>  
>  #include "abi/guc_actions_abi.h"
>  #include "abi/guc_errors_abi.h"
> @@ -32,6 +33,12 @@
>  #define GUC_VIDEO_ENGINE2		4
>  #define GUC_MAX_ENGINES_NUM		(GUC_VIDEO_ENGINE2 + 1)
>  
> +#define GUC_RENDER_CLASS		0
> +#define GUC_VIDEO_CLASS			1
> +#define GUC_VIDEOENHANCE_CLASS		2
> +#define GUC_BLITTER_CLASS		3
> +#define GUC_RESERVED_CLASS		4
> +#define GUC_LAST_ENGINE_CLASS		GUC_RESERVED_CLASS
>  #define GUC_MAX_ENGINE_CLASSES		16
>  #define GUC_MAX_INSTANCES_PER_CLASS	32
>  
> @@ -129,6 +136,25 @@
>  #define GUC_ID_TO_ENGINE_INSTANCE(guc_id) \
>  	(((guc_id) & GUC_ENGINE_INSTANCE_MASK) >> GUC_ENGINE_INSTANCE_SHIFT)
>  
> +static inline u8 engine_class_to_guc_class(u8 class)
> +{
> +	BUILD_BUG_ON(GUC_RENDER_CLASS != RENDER_CLASS);
> +	BUILD_BUG_ON(GUC_BLITTER_CLASS != COPY_ENGINE_CLASS);
> +	BUILD_BUG_ON(GUC_VIDEO_CLASS != VIDEO_DECODE_CLASS);
> +	BUILD_BUG_ON(GUC_VIDEOENHANCE_CLASS != VIDEO_ENHANCEMENT_CLASS);
> +	GEM_BUG_ON(class > MAX_ENGINE_CLASS || class == OTHER_CLASS);
> +
> +	return class;
> +}
> +
> +static inline u8 guc_class_to_engine_class(u8 guc_class)
> +{
> +	GEM_BUG_ON(guc_class > GUC_LAST_ENGINE_CLASS);
> +	GEM_BUG_ON(guc_class == GUC_RESERVED_CLASS);
> +
> +	return guc_class;
> +}
> +
>  /* Work item for submitting workloads into work queue of GuC. */
>  struct guc_wq_item {
>  	u32 header;
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 20/20] drm/i915/guc: Use guc_class instead of engine_class in fw interface
  2021-06-04  8:44   ` Daniel Vetter
@ 2021-06-04 18:12     ` Matthew Brost
  0 siblings, 0 replies; 41+ messages in thread
From: Matthew Brost @ 2021-06-04 18:12 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: daniel.vetter, intel-gfx, dri-devel

On Fri, Jun 04, 2021 at 10:44:31AM +0200, Daniel Vetter wrote:
> On Wed, Jun 02, 2021 at 10:16:30PM -0700, Matthew Brost wrote:
> > From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > 
> > GuC has its own defines for the engine classes. They're currently
> > mapping 1:1 to the defines used by the driver, but there is no guarantee
> > this will continue in the future. Given that we've been caught off-guard
> > in the past by similar divergences, we can prepare for the changes by
> > introducing helper functions to convert from engine class to GuC class and
> > back again.
> > 
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > Cc: John Harrison <John.C.Harrison@Intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 
> Applied all up to this, except the Kconfig one.
> 

Thanks, that one isn't really a blocker to next post which will update
the GuC firmware to the latest and greatest version.

Let me look at the issue with that patch now.

Matt

> Thanks, Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/gt/intel_engine_cs.c   |  6 +++--
> >  drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c  | 20 +++++++++-------
> >  drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 26 +++++++++++++++++++++
> >  3 files changed, 42 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > index 3f9a811eb02b..69281b5aba51 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > @@ -265,6 +265,7 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
> >  	const struct engine_info *info = &intel_engines[id];
> >  	struct drm_i915_private *i915 = gt->i915;
> >  	struct intel_engine_cs *engine;
> > +	u8 guc_class;
> >  
> >  	BUILD_BUG_ON(MAX_ENGINE_CLASS >= BIT(GEN11_ENGINE_CLASS_WIDTH));
> >  	BUILD_BUG_ON(MAX_ENGINE_INSTANCE >= BIT(GEN11_ENGINE_INSTANCE_WIDTH));
> > @@ -293,9 +294,10 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
> >  	engine->i915 = i915;
> >  	engine->gt = gt;
> >  	engine->uncore = gt->uncore;
> > -	engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
> >  	engine->hw_id = info->hw_id;
> > -	engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
> > +	guc_class = engine_class_to_guc_class(info->class);
> > +	engine->guc_id = MAKE_GUC_ID(guc_class, info->instance);
> > +	engine->mmio_base = __engine_mmio_base(i915, info->mmio_bases);
> >  
> >  	engine->irq_handler = nop_irq_handler;
> >  
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > index 17526717368c..efdce309b6f1 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > @@ -6,6 +6,7 @@
> >  #include "gt/intel_gt.h"
> >  #include "gt/intel_lrc.h"
> >  #include "intel_guc_ads.h"
> > +#include "intel_guc_fwif.h"
> >  #include "intel_uc.h"
> >  #include "i915_drv.h"
> >  
> > @@ -104,7 +105,7 @@ static void guc_mapping_table_init(struct intel_gt *gt,
> >  				GUC_MAX_INSTANCES_PER_CLASS;
> >  
> >  	for_each_engine(engine, gt, id) {
> > -		u8 guc_class = engine->class;
> > +		u8 guc_class = engine_class_to_guc_class(engine->class);
> >  
> >  		system_info->mapping_table[guc_class][engine->instance] =
> >  			engine->instance;
> > @@ -124,7 +125,7 @@ static void __guc_ads_init(struct intel_guc *guc)
> >  	struct __guc_ads_blob *blob = guc->ads_blob;
> >  	const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE;
> >  	u32 base;
> > -	u8 engine_class;
> > +	u8 engine_class, guc_class;
> >  
> >  	/* GuC scheduling policies */
> >  	guc_policies_init(&blob->policies);
> > @@ -140,22 +141,25 @@ static void __guc_ads_init(struct intel_guc *guc)
> >  	for (engine_class = 0; engine_class <= MAX_ENGINE_CLASS; ++engine_class) {
> >  		if (engine_class == OTHER_CLASS)
> >  			continue;
> > +
> > +		guc_class = engine_class_to_guc_class(engine_class);
> > +
> >  		/*
> >  		 * TODO: Set context pointer to default state to allow
> >  		 * GuC to re-init guilty contexts after internal reset.
> >  		 */
> > -		blob->ads.golden_context_lrca[engine_class] = 0;
> > -		blob->ads.eng_state_size[engine_class] =
> > +		blob->ads.golden_context_lrca[guc_class] = 0;
> > +		blob->ads.eng_state_size[guc_class] =
> >  			intel_engine_context_size(guc_to_gt(guc),
> >  						  engine_class) -
> >  			skipped_size;
> >  	}
> >  
> >  	/* System info */
> > -	blob->system_info.engine_enabled_masks[RENDER_CLASS] = 1;
> > -	blob->system_info.engine_enabled_masks[COPY_ENGINE_CLASS] = 1;
> > -	blob->system_info.engine_enabled_masks[VIDEO_DECODE_CLASS] = VDBOX_MASK(gt);
> > -	blob->system_info.engine_enabled_masks[VIDEO_ENHANCEMENT_CLASS] = VEBOX_MASK(gt);
> > +	blob->system_info.engine_enabled_masks[GUC_RENDER_CLASS] = 1;
> > +	blob->system_info.engine_enabled_masks[GUC_BLITTER_CLASS] = 1;
> > +	blob->system_info.engine_enabled_masks[GUC_VIDEO_CLASS] = VDBOX_MASK(gt);
> > +	blob->system_info.engine_enabled_masks[GUC_VIDEOENHANCE_CLASS] = VEBOX_MASK(gt);
> >  
> >  	blob->system_info.generic_gt_sysinfo[GUC_GENERIC_GT_SYSINFO_SLICE_ENABLED] =
> >  		hweight8(gt->info.sseu.slice_mask);
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> > index 9bf35240e723..e9a9d85e2aa3 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> > @@ -9,6 +9,7 @@
> >  #include <linux/bits.h>
> >  #include <linux/compiler.h>
> >  #include <linux/types.h>
> > +#include "gt/intel_engine_types.h"
> >  
> >  #include "abi/guc_actions_abi.h"
> >  #include "abi/guc_errors_abi.h"
> > @@ -32,6 +33,12 @@
> >  #define GUC_VIDEO_ENGINE2		4
> >  #define GUC_MAX_ENGINES_NUM		(GUC_VIDEO_ENGINE2 + 1)
> >  
> > +#define GUC_RENDER_CLASS		0
> > +#define GUC_VIDEO_CLASS			1
> > +#define GUC_VIDEOENHANCE_CLASS		2
> > +#define GUC_BLITTER_CLASS		3
> > +#define GUC_RESERVED_CLASS		4
> > +#define GUC_LAST_ENGINE_CLASS		GUC_RESERVED_CLASS
> >  #define GUC_MAX_ENGINE_CLASSES		16
> >  #define GUC_MAX_INSTANCES_PER_CLASS	32
> >  
> > @@ -129,6 +136,25 @@
> >  #define GUC_ID_TO_ENGINE_INSTANCE(guc_id) \
> >  	(((guc_id) & GUC_ENGINE_INSTANCE_MASK) >> GUC_ENGINE_INSTANCE_SHIFT)
> >  
> > +static inline u8 engine_class_to_guc_class(u8 class)
> > +{
> > +	BUILD_BUG_ON(GUC_RENDER_CLASS != RENDER_CLASS);
> > +	BUILD_BUG_ON(GUC_BLITTER_CLASS != COPY_ENGINE_CLASS);
> > +	BUILD_BUG_ON(GUC_VIDEO_CLASS != VIDEO_DECODE_CLASS);
> > +	BUILD_BUG_ON(GUC_VIDEOENHANCE_CLASS != VIDEO_ENHANCEMENT_CLASS);
> > +	GEM_BUG_ON(class > MAX_ENGINE_CLASS || class == OTHER_CLASS);
> > +
> > +	return class;
> > +}
> > +
> > +static inline u8 guc_class_to_engine_class(u8 guc_class)
> > +{
> > +	GEM_BUG_ON(guc_class > GUC_LAST_ENGINE_CLASS);
> > +	GEM_BUG_ON(guc_class == GUC_RESERVED_CLASS);
> > +
> > +	return guc_class;
> > +}
> > +
> >  /* Work item for submitting workloads into work queue of GuC. */
> >  struct guc_wq_item {
> >  	u32 header;
> > -- 
> > 2.28.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 41+ messages in thread

end of thread, other threads:[~2021-06-09 13:24 UTC | newest]

Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-03  5:16 [PATCH 00/20] GuC CTBs changes + a few misc patches Matthew Brost
2021-06-03  5:16 ` [PATCH 01/20] drm/i915/guc: skip disabling CTBs before sanitizing the GuC Matthew Brost
2021-06-03  5:16 ` [PATCH 02/20] drm/i915/guc: use probe_error log for CT enablement failure Matthew Brost
2021-06-03  5:16 ` [PATCH 03/20] drm/i915/guc: enable only the user interrupt when using GuC submission Matthew Brost
2021-06-03  5:16 ` [PATCH 04/20] drm/i915/guc: Remove sample_forcewake h2g action Matthew Brost
2021-06-03  5:16 ` [PATCH 05/20] drm/i915/guc: Keep strict GuC ABI definitions Matthew Brost
2021-06-03  5:16 ` [PATCH 06/20] drm/i915/guc: Drop guc->interrupts.enabled Matthew Brost
2021-06-03  5:16 ` [PATCH 07/20] drm/i915/guc: Stop using fence/status from CTB descriptor Matthew Brost
2021-06-03  5:16 ` [PATCH 08/20] drm/i915: Promote ptrdiff() to i915_utils.h Matthew Brost
2021-06-03 21:35   ` Daniel Vetter
2021-06-04  2:02     ` Matthew Brost
2021-06-04  8:11       ` Daniel Vetter
2021-06-03  5:16 ` [PATCH 09/20] drm/i915/guc: Only rely on own CTB size Matthew Brost
2021-06-03  5:16 ` [PATCH 10/20] drm/i915/guc: Don't repeat CTB layout calculations Matthew Brost
2021-06-03  5:16 ` [PATCH 11/20] drm/i915/guc: Replace CTB array with explicit members Matthew Brost
2021-06-03  7:25   ` kernel test robot
2021-06-03 21:37     ` [Intel-gfx] " Daniel Vetter
2021-06-03 22:44       ` [PATCH 1/2] " Matthew Brost
2021-06-03 22:44         ` [PATCH 2/2] drm/i915/guc: Update sizes of CTB buffers Matthew Brost
2021-06-03 23:04       ` [v3 PATCH 1/2] drm/i915/guc: Replace CTB array with explicit members Matthew Brost
2021-06-03 23:04         ` [v3 PATCH 2/2] drm/i915/guc: Update sizes of CTB buffers Matthew Brost
2021-06-04  8:20           ` Daniel Vetter
2021-06-04  8:49             ` Michal Wajdeczko
2021-06-03  5:16 ` [PATCH 12/20] " Matthew Brost
2021-06-03  5:16 ` [PATCH 13/20] drm/i915/guc: Relax CTB response timeout Matthew Brost
2021-06-04  8:33   ` [Intel-gfx] " Daniel Vetter
2021-06-04 18:35     ` Matthew Brost
2021-06-09 13:24       ` Daniel Vetter
2021-06-03  5:16 ` [PATCH 14/20] drm/i915/guc: Start protecting access to CTB descriptors Matthew Brost
2021-06-04  8:35   ` Daniel Vetter
2021-06-03  5:16 ` [PATCH 15/20] drm/i915/guc: Ensure H2G buffer updates visible before tail update Matthew Brost
2021-06-03  9:44   ` [Intel-gfx] " Michal Wajdeczko
2021-06-03 16:10     ` Matthew Brost
2021-06-04  8:39       ` Daniel Vetter
2021-06-03  5:16 ` [PATCH 16/20] drm/i915/guc: Stop using mutex while sending CTB messages Matthew Brost
2021-06-03  5:16 ` [PATCH 17/20] drm/i915/guc: Don't receive all G2H messages in irq handler Matthew Brost
2021-06-03  5:16 ` [PATCH 18/20] drm/i915/guc: Always copy CT message to new allocation Matthew Brost
2021-06-03  5:16 ` [PATCH 19/20] drm/i915/guc: Early initialization of GuC send registers Matthew Brost
2021-06-03  5:16 ` [PATCH 20/20] drm/i915/guc: Use guc_class instead of engine_class in fw interface Matthew Brost
2021-06-04  8:44   ` Daniel Vetter
2021-06-04 18:12     ` Matthew Brost

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).