[PATCH v4 00/13] drm/i915/guc: Support for Guc responses and requests

[PATCH v4 00/13] drm/i915/guc: Support for Guc responses and requests

[Michal Wajdeczko]

With this series we will be able to receive more data from the Guc.
New Guc firmwares will be required to actually use that feature.

v4: respin series after 1/2 year break

Michal Wajdeczko (13):
  drm/i915/guc: Add documentation for MMIO based communication
  drm/i915/guc: Add support for data reporting in GuC responses
  drm/i915/guc: Prepare send() function to accept bigger response
  drm/i915/guc: Implement response handling in send_mmio()
  drm/i915/guc: Make event handler a virtual function
  drm/i915/guc: Prepare to handle messages from CT RECV buffer
  drm/i915/guc: Use better name for helper wait function
  drm/i915/guc: Implement response handling in send_ct()
  drm/i915/guc: Prepare to process incoming requests from CT
  drm/i915/guc: Enable GuC interrupts when using CT
  drm/i915/guc: Handle default action received over CT
  drm/i915/guc: Trace messages from CT while in debug
  HAX: Enable GuC for CI

 drivers/gpu/drm/i915/Kconfig.debug    |  12 +
 drivers/gpu/drm/i915/i915_params.h    |   2 +-
 drivers/gpu/drm/i915/intel_guc.c      |  51 +++-
 drivers/gpu/drm/i915/intel_guc.h      |  29 ++-
 drivers/gpu/drm/i915/intel_guc_ct.c   | 430 +++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_guc_ct.h   |   9 +
 drivers/gpu/drm/i915/intel_guc_fwif.h |  86 +++++--
 drivers/gpu/drm/i915/intel_uc.c       |   4 +-
 8 files changed, 551 insertions(+), 72 deletions(-)

-- 
1.9.1

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

[PATCH v4 01/13] drm/i915/guc: Add documentation for MMIO based communication

[Michal Wajdeczko]

As we are going to extend our use of MMIO based communication,
try to explain its mechanics and update corresponding definitions.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Kelvin Gardiner <kelvin.gardiner@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c      | 20 ++++----
 drivers/gpu/drm/i915/intel_guc_ct.c   |  2 +-
 drivers/gpu/drm/i915/intel_guc_fwif.h | 86 ++++++++++++++++++++++++++++-------
 3 files changed, 80 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 8f93f5b..28075e6 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -329,6 +329,9 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	GEM_BUG_ON(!len);
 	GEM_BUG_ON(len > guc->send_regs.count);
 
+	/* We expect only action code */
+	GEM_BUG_ON(*action & ~INTEL_GUC_MSG_CODE_MASK);
+
 	/* If CT is available, we expect to use MMIO only during init/fini */
 	GEM_BUG_ON(HAS_GUC_CT(dev_priv) &&
 		*action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
@@ -350,18 +353,15 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 	 */
 	ret = __intel_wait_for_register_fw(dev_priv,
 					   guc_send_reg(guc, 0),
-					   INTEL_GUC_RECV_MASK,
-					   INTEL_GUC_RECV_MASK,
+					   INTEL_GUC_MSG_TYPE_MASK,
+					   INTEL_GUC_MSG_TYPE_RESPONSE <<
+					   INTEL_GUC_MSG_TYPE_SHIFT,
 					   10, 10, &status);
-	if (status != INTEL_GUC_STATUS_SUCCESS) {
-		/*
-		 * Either the GuC explicitly returned an error (which
-		 * we convert to -EIO here) or no response at all was
-		 * received within the timeout limit (-ETIMEDOUT)
-		 */
-		if (ret != -ETIMEDOUT)
-			ret = -EIO;
+	/* If GuC explicitly returned an error, convert it to -EIO */
+	if (!ret && !INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(status))
+		ret = -EIO;
 
+	if (ret) {
 		DRM_DEBUG_DRIVER("INTEL_GUC_SEND: Action 0x%X failed;"
 				 " ret=%d status=0x%08X response=0x%08X\n",
 				 action[0], ret, status,
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
index a726283..1dafa7a 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -398,7 +398,7 @@ static int ctch_send(struct intel_guc *guc,
 	err = wait_for_response(desc, fence, status);
 	if (unlikely(err))
 		return err;
-	if (*status != INTEL_GUC_STATUS_SUCCESS)
+	if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status))
 		return -EIO;
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 72941bd..1701879 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -560,7 +560,68 @@ struct guc_shared_ctx_data {
 	struct guc_ctx_report preempt_ctx_report[GUC_MAX_ENGINES_NUM];
 } __packed;
 
-/* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */
+/**
+ * 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
+ *
+ *      +-----------+---------+---------+---------+
+ *      |  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 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,
@@ -597,24 +658,15 @@ enum intel_guc_report_status {
 #define GUC_LOG_CONTROL_VERBOSITY_MASK	(0xF << GUC_LOG_CONTROL_VERBOSITY_SHIFT)
 #define GUC_LOG_CONTROL_DEFAULT_LOGGING	(1 << 8)
 
-/*
- * The GuC sends its response to a command by overwriting the
- * command in SS0. The response is distinguishable from a command
- * by the fact that all the MASK bits are set. The remaining bits
- * give more detail.
- */
-#define	INTEL_GUC_RECV_MASK	((u32)0xF0000000)
-#define	INTEL_GUC_RECV_IS_RESPONSE(x)	((u32)(x) >= INTEL_GUC_RECV_MASK)
-#define	INTEL_GUC_RECV_STATUS(x)	(INTEL_GUC_RECV_MASK | (x))
-
-/* GUC will return status back to SOFT_SCRATCH_O_REG */
-enum intel_guc_status {
-	INTEL_GUC_STATUS_SUCCESS = INTEL_GUC_RECV_STATUS(0x0),
-	INTEL_GUC_STATUS_ALLOCATE_DOORBELL_FAIL = INTEL_GUC_RECV_STATUS(0x10),
-	INTEL_GUC_STATUS_DEALLOCATE_DOORBELL_FAIL = INTEL_GUC_RECV_STATUS(0x20),
-	INTEL_GUC_STATUS_GENERIC_FAIL = INTEL_GUC_RECV_STATUS(0x0000F000)
+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) \
+	((INTEL_GUC_MSG_TO_TYPE(m) == INTEL_GUC_MSG_TYPE_RESPONSE) && \
+	 (INTEL_GUC_MSG_TO_CODE(m) == INTEL_GUC_RESPONSE_STATUS_SUCCESS))
+
 /* This action will be programmed in C1BC - SOFT_SCRATCH_15_REG */
 enum intel_guc_recv_message {
 	INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED = BIT(1),
-- 
1.9.1

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

[PATCH v4 02/13] drm/i915/guc: Add support for data reporting in GuC responses

[Michal Wajdeczko]

GuC may return additional data in the response message.
Format and meaning of this data is action specific. We will
use this non-negative data as a new success return value.
Currently used actions don't return data that way yet.

v2: fix prohibited space after '~' (Michel)
    update commit message (Daniele)
v3: rebase

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com> #2
---
 drivers/gpu/drm/i915/intel_guc.c    |  3 +++
 drivers/gpu/drm/i915/intel_guc_ct.c | 14 ++++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 28075e6..77bf4e6 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -366,6 +366,9 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
 				 " ret=%d status=0x%08X response=0x%08X\n",
 				 action[0], ret, status,
 				 I915_READ(SOFT_SCRATCH(15)));
+	} else {
+		/* Use data from the GuC response as our return value */
+		ret = INTEL_GUC_MSG_TO_DATA(status);
 	}
 
 	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
index 1dafa7a..fa52259 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -400,7 +400,9 @@ static int ctch_send(struct intel_guc *guc,
 		return err;
 	if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status))
 		return -EIO;
-	return 0;
+
+	/* Use data from the GuC status as our return value */
+	return INTEL_GUC_MSG_TO_DATA(*status);
 }
 
 /*
@@ -410,18 +412,18 @@ static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len)
 {
 	struct intel_guc_ct_channel *ctch = &guc->ct.host_channel;
 	u32 status = ~0; /* undefined */
-	int err;
+	int ret;
 
 	mutex_lock(&guc->send_mutex);
 
-	err = ctch_send(guc, ctch, action, len, &status);
-	if (unlikely(err)) {
+	ret = ctch_send(guc, ctch, action, len, &status);
+	if (unlikely(ret < 0)) {
 		DRM_ERROR("CT: send action %#X failed; err=%d status=%#X\n",
-			  action[0], err, status);
+			  action[0], ret, status);
 	}
 
 	mutex_unlock(&guc->send_mutex);
-	return err;
+	return ret;
 }
 
 /**
-- 
1.9.1

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

[PATCH v4 03/13] drm/i915/guc: Prepare send() function to accept bigger response

[Michal Wajdeczko]

This is a preparation step for the upcoming patches.
We already can return some small data decoded from the command
status, but we will need more in the future.

v2: add explicit response buf size
v3: squash with helper patch

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com> #1
---
 drivers/gpu/drm/i915/intel_guc.c    |  6 ++++--
 drivers/gpu/drm/i915/intel_guc.h    | 18 ++++++++++++++----
 drivers/gpu/drm/i915/intel_guc_ct.c |  7 ++++---
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 77bf4e6..a533ff8 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -310,7 +310,8 @@ void intel_guc_init_params(struct intel_guc *guc)
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_BLITTER);
 }
 
-int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
+int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len,
+		       u32 *response_buf, u32 response_buf_size)
 {
 	WARN(1, "Unexpected send: action=%#x\n", *action);
 	return -ENODEV;
@@ -319,7 +320,8 @@ int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
 /*
  * This function implements the MMIO based host to GuC interface.
  */
-int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
+int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
+			u32 *response_buf, u32 response_buf_size)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	u32 status;
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 13f3d1d..7ee0732 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -88,7 +88,8 @@ struct intel_guc {
 	struct mutex send_mutex;
 
 	/* GuC's FW specific send function */
-	int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
+	int (*send)(struct intel_guc *guc, const u32 *data, u32 len,
+		    u32 *response_buf, u32 response_buf_size);
 
 	/* GuC's FW specific notify function */
 	void (*notify)(struct intel_guc *guc);
@@ -97,7 +98,14 @@ struct intel_guc {
 static
 inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
 {
-	return guc->send(guc, action, len);
+	return guc->send(guc, action, len, NULL, 0);
+}
+
+static inline int
+intel_guc_send_and_receive(struct intel_guc *guc, const u32 *action, u32 len,
+			   u32 *response_buf, u32 response_buf_size)
+{
+	return guc->send(guc, action, len, response_buf, response_buf_size);
 }
 
 static inline void intel_guc_notify(struct intel_guc *guc)
@@ -140,8 +148,10 @@ static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc,
 void intel_guc_fini_wq(struct intel_guc *guc);
 int intel_guc_init(struct intel_guc *guc);
 void intel_guc_fini(struct intel_guc *guc);
-int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
-int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
+int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len,
+		       u32 *response_buf, u32 response_buf_size);
+int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
+			u32 *response_buf, u32 response_buf_size);
 void intel_guc_to_host_event_handler(struct intel_guc *guc);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
index fa52259..a54bf58 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -88,7 +88,7 @@ static int guc_action_register_ct_buffer(struct intel_guc *guc,
 	int err;
 
 	/* Can't use generic send(), CT registration must go over MMIO */
-	err = intel_guc_send_mmio(guc, action, ARRAY_SIZE(action));
+	err = intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0);
 	if (err)
 		DRM_ERROR("CT: register %s buffer failed; err=%d\n",
 			  guc_ct_buffer_type_to_str(type), err);
@@ -107,7 +107,7 @@ static int guc_action_deregister_ct_buffer(struct intel_guc *guc,
 	int err;
 
 	/* Can't use generic send(), CT deregistration must go over MMIO */
-	err = intel_guc_send_mmio(guc, action, ARRAY_SIZE(action));
+	err = intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0);
 	if (err)
 		DRM_ERROR("CT: deregister %s buffer failed; owner=%d err=%d\n",
 			  guc_ct_buffer_type_to_str(type), owner, err);
@@ -408,7 +408,8 @@ static int ctch_send(struct intel_guc *guc,
 /*
  * Command Transport (CT) buffer based GuC send function.
  */
-static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len)
+static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len,
+			     u32 *response_buf, u32 response_buf_size)
 {
 	struct intel_guc_ct_channel *ctch = &guc->ct.host_channel;
 	u32 status = ~0; /* undefined */
-- 
1.9.1

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

[PATCH v4 04/13] drm/i915/guc: Implement response handling in send_mmio()

[Michal Wajdeczko]

We're using data encoded in the status MMIO as return value from send
function, but GuC may also write more data in remaining MMIO regs.
Let's copy content of these registers to the buffer provided by caller.

v2: new line (Michel)
v3: updated commit message

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Michel Thierry <michel.thierry@intel.com> #2
---
 drivers/gpu/drm/i915/intel_guc.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index a533ff8..9ce01e5 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -368,11 +368,20 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
 				 " ret=%d status=0x%08X response=0x%08X\n",
 				 action[0], ret, status,
 				 I915_READ(SOFT_SCRATCH(15)));
-	} else {
-		/* Use data from the GuC response as our return value */
-		ret = INTEL_GUC_MSG_TO_DATA(status);
+		goto out;
 	}
 
+	if (response_buf) {
+		int count = min(response_buf_size, guc->send_regs.count - 1);
+
+		for (i = 0; i < count; i++)
+			response_buf[i] = I915_READ(guc_send_reg(guc, i + 1));
+	}
+
+	/* Use data from the GuC response as our return value */
+	ret = INTEL_GUC_MSG_TO_DATA(status);
+
+out:
 	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
 	mutex_unlock(&guc->send_mutex);
 
-- 
1.9.1

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

[PATCH v4 05/13] drm/i915/guc: Make event handler a virtual function

[Michal Wajdeczko]

On platforms with CTB based GuC communications, we will handle
GuC events in a different way. Let's make event handler a virtual
function to allow easy switch between those variants.

Credits-to: Oscar Mateo <oscar.mateo@intel.com>
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c |  8 +++++++-
 drivers/gpu/drm/i915/intel_guc.h | 10 ++++++++++
 drivers/gpu/drm/i915/intel_uc.c  |  2 ++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 9ce01e5..118db81 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -69,6 +69,7 @@ void intel_guc_init_early(struct intel_guc *guc)
 	mutex_init(&guc->send_mutex);
 	spin_lock_init(&guc->irq_lock);
 	guc->send = intel_guc_send_nop;
+	guc->handler = intel_guc_to_host_event_handler_nop;
 	guc->notify = gen8_guc_raise_irq;
 }
 
@@ -317,6 +318,11 @@ int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len,
 	return -ENODEV;
 }
 
+void intel_guc_to_host_event_handler_nop(struct intel_guc *guc)
+{
+	WARN(1, "Unexpected event: no suitable handler\n");
+}
+
 /*
  * This function implements the MMIO based host to GuC interface.
  */
@@ -388,7 +394,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
 	return ret;
 }
 
-void intel_guc_to_host_event_handler(struct intel_guc *guc)
+void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc)
 {
 	struct drm_i915_private *dev_priv = guc_to_i915(guc);
 	u32 msg, val;
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 7ee0732..6dc109a 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -91,6 +91,9 @@ struct intel_guc {
 	int (*send)(struct intel_guc *guc, const u32 *data, u32 len,
 		    u32 *response_buf, u32 response_buf_size);
 
+	/* GuC's FW specific event handler function */
+	void (*handler)(struct intel_guc *guc);
+
 	/* GuC's FW specific notify function */
 	void (*notify)(struct intel_guc *guc);
 };
@@ -113,6 +116,11 @@ static inline void intel_guc_notify(struct intel_guc *guc)
 	guc->notify(guc);
 }
 
+static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)
+{
+	guc->handler(guc);
+}
+
 /* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */
 #define GUC_GGTT_TOP	0xFEE00000
 
@@ -153,6 +161,8 @@ int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len,
 int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
 			u32 *response_buf, u32 response_buf_size);
 void intel_guc_to_host_event_handler(struct intel_guc *guc);
+void intel_guc_to_host_event_handler_nop(struct intel_guc *guc);
+void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc);
 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);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 34f8a2c..8dc6a9c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -234,6 +234,7 @@ static int guc_enable_communication(struct intel_guc *guc)
 		return intel_guc_ct_enable(&guc->ct);
 
 	guc->send = intel_guc_send_mmio;
+	guc->handler = intel_guc_to_host_event_handler_mmio;
 	return 0;
 }
 
@@ -247,6 +248,7 @@ static void guc_disable_communication(struct intel_guc *guc)
 	gen9_disable_guc_interrupts(dev_priv);
 
 	guc->send = intel_guc_send_nop;
+	guc->handler = intel_guc_to_host_event_handler_nop;
 }
 
 int intel_uc_init_misc(struct drm_i915_private *dev_priv)
-- 
1.9.1

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

[PATCH v4 06/13] drm/i915/guc: Prepare to handle messages from CT RECV buffer

[Michal Wajdeczko]

GuC can respond to our commands not only by updating SEND buffer
descriptor, but can also send a response message over RECV buffer.
Guc can also send unsolicited request messages over RECV buffer.
Let's start reading those messages and make placeholders
for actual response/request handlers.

v2: misc improvements (Michal)
v3: change response detection (Michal)
    invalid status is protocol error (Michal)
v4: rebase

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_ct.c | 139 ++++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
index a54bf58..f029ff3 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -427,6 +427,143 @@ static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len,
 	return ret;
 }
 
+static inline unsigned int ct_header_get_len(u32 header)
+{
+	return (header >> GUC_CT_MSG_LEN_SHIFT) & GUC_CT_MSG_LEN_MASK;
+}
+
+static inline unsigned int ct_header_get_action(u32 header)
+{
+	return (header >> GUC_CT_MSG_ACTION_SHIFT) & GUC_CT_MSG_ACTION_MASK;
+}
+
+static inline bool ct_header_is_response(u32 header)
+{
+	return ct_header_get_action(header) == INTEL_GUC_ACTION_DEFAULT;
+}
+
+static int ctb_read(struct intel_guc_ct_buffer *ctb, u32 *data)
+{
+	struct guc_ct_buffer_desc *desc = ctb->desc;
+	u32 head = desc->head / 4;	/* in dwords */
+	u32 tail = desc->tail / 4;	/* in dwords */
+	u32 size = desc->size / 4;	/* in dwords */
+	u32 *cmds = ctb->cmds;
+	s32 available;			/* in dwords */
+	unsigned int len;
+	unsigned int i;
+
+	GEM_BUG_ON(desc->size % 4);
+	GEM_BUG_ON(desc->head % 4);
+	GEM_BUG_ON(desc->tail % 4);
+	GEM_BUG_ON(tail >= size);
+	GEM_BUG_ON(head >= size);
+
+	/* tail == head condition indicates empty */
+	available = tail - head;
+	if (unlikely(available == 0))
+		return -ENODATA;
+
+	/* beware of buffer wrap case */
+	if (unlikely(available < 0))
+		available += size;
+	GEM_BUG_ON(available < 0);
+
+	data[0] = cmds[head];
+	head = (head + 1) % size;
+
+	/* message len with header */
+	len = ct_header_get_len(data[0]) + 1;
+	if (unlikely(len > (u32)available)) {
+		DRM_ERROR("CT: incomplete message %*phn %*phn %*phn\n",
+			  4, data,
+			  4 * (head + available - 1 > size ?
+			       size - head : available - 1), &cmds[head],
+			  4 * (head + available - 1 > size ?
+			       available - 1 - size + head : 0), &cmds[0]);
+		return -EPROTO;
+	}
+
+	for (i = 1; i < len; i++) {
+		data[i] = cmds[head];
+		head = (head + 1) % size;
+	}
+
+	desc->head = head * 4;
+	return 0;
+}
+
+static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
+{
+	u32 header = msg[0];
+	u32 status = msg[2];
+	u32 len = ct_header_get_len(header) + 1; /* total len with header */
+
+	GEM_BUG_ON(!ct_header_is_response(header));
+
+	/* Response message shall at least include header, fence and status */
+	if (unlikely(len < 3)) {
+		DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
+		return -EPROTO;
+	}
+	/* Format of the status follows RESPONSE message */
+	if (unlikely(!INTEL_GUC_MSG_IS_RESPONSE(status))) {
+		DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
+		return -EPROTO;
+	}
+
+	/* XXX */
+	return 0;
+}
+
+static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg)
+{
+	u32 header = msg[0];
+
+	GEM_BUG_ON(ct_header_is_response(header));
+
+	/* XXX */
+	return 0;
+}
+
+static void ct_process_host_channel(struct intel_guc_ct *ct)
+{
+	struct intel_guc_ct_channel *ctch = &ct->host_channel;
+	struct intel_guc_ct_buffer *ctb = &ctch->ctbs[CTB_RECV];
+	u32 msg[GUC_CT_MSG_LEN_MASK+1]; /* one extra dw for the header */
+	int err = 0;
+
+	if (!ctch_is_open(ctch))
+		return;
+
+	do {
+		err = ctb_read(ctb, msg);
+		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);
+
+	if (GEM_WARN_ON(err == -EPROTO)) {
+		DRM_ERROR("CT: corrupted message detected!\n");
+		ctb->desc->is_in_error = 1;
+	}
+}
+
+/*
+ * When we're communicating with the GuC over CT, GuC uses events
+ * to notify us about new messages being posted on the RECV buffer.
+ */
+static void intel_guc_to_host_event_handler_ct(struct intel_guc *guc)
+{
+	struct intel_guc_ct *ct = &guc->ct;
+
+	ct_process_host_channel(ct);
+}
+
 /**
  * intel_guc_ct_enable - Enable buffer based command transport.
  * @ct: pointer to CT struct
@@ -450,6 +587,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
 
 	/* Switch into cmd transport buffer based send() */
 	guc->send = intel_guc_send_ct;
+	guc->handler = intel_guc_to_host_event_handler_ct;
 	DRM_INFO("CT: %s\n", enableddisabled(true));
 	return 0;
 }
@@ -475,5 +613,6 @@ void intel_guc_ct_disable(struct intel_guc_ct *ct)
 
 	/* Disable send */
 	guc->send = intel_guc_send_nop;
+	guc->handler = intel_guc_to_host_event_handler_nop;
 	DRM_INFO("CT: %s\n", enableddisabled(false));
 }
-- 
1.9.1

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

[PATCH v4 07/13] drm/i915/guc: Use better name for helper wait function

[Michal Wajdeczko]

In next patch we will introduce another way of waiting for the response
that will use RECV buffer. To avoid misleading names, rename old wait
function to reflect the fact that it is based on descriptor update.

v2: fix comment style (Michal)

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_ct.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
index f029ff3..b1ab28f 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -332,16 +332,23 @@ static int ctb_write(struct intel_guc_ct_buffer *ctb,
 	return 0;
 }
 
-/* Wait for the response from the GuC.
+/**
+ * wait_for_desc_update - Wait for the descriptor update.
+ * @desc:	buffer descriptor
  * @fence:	response fence
  * @status:	placeholder for status
- * return:	0 response received (status is valid)
- *		-ETIMEDOUT no response within hardcoded timeout
- *		-EPROTO no response, ct buffer was in error
+ *
+ * Guc will update descriptor with new fence and status
+ * after processing 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_response(struct guc_ct_buffer_desc *desc,
-			     u32 fence,
-			     u32 *status)
+static int wait_for_desc_update(struct guc_ct_buffer_desc *desc,
+				u32 fence,
+				u32 *status)
 {
 	int err;
 
@@ -395,7 +402,7 @@ static int ctch_send(struct intel_guc *guc,
 
 	intel_guc_notify(guc);
 
-	err = wait_for_response(desc, fence, status);
+	err = wait_for_desc_update(desc, fence, status);
 	if (unlikely(err))
 		return err;
 	if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status))
-- 
1.9.1

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

[PATCH v4 08/13] drm/i915/guc: Implement response handling in send_ct()

[Michal Wajdeczko]

Instead of returning small data in response status dword,
GuC may append longer data as response message payload.
If caller provides response buffer, we will copy received
data and use number of received data dwords as new success
return value. We will WARN if response from GuC does not
match caller expectation.

v2: fix timeout and checkpatch warnings (Michal)

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_ct.c | 137 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_guc_ct.h |   5 ++
 2 files changed, 128 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
index b1ab28f..6a9aad0 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -24,6 +24,14 @@
 #include "i915_drv.h"
 #include "intel_guc_ct.h"
 
+struct ct_request {
+	struct list_head link;
+	u32 fence;
+	u32 status;
+	u32 response_len;
+	u32 *response_buf;
+};
+
 enum { CTB_SEND = 0, CTB_RECV = 1 };
 
 enum { CTB_OWNER_HOST = 0 };
@@ -36,6 +44,9 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)
 {
 	/* we're using static channel owners */
 	ct->host_channel.owner = CTB_OWNER_HOST;
+
+	spin_lock_init(&ct->lock);
+	INIT_LIST_HEAD(&ct->pending_requests);
 }
 
 static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct)
@@ -276,7 +287,8 @@ static u32 ctch_get_next_fence(struct intel_guc_ct_channel *ctch)
 static int ctb_write(struct intel_guc_ct_buffer *ctb,
 		     const u32 *action,
 		     u32 len /* in dwords */,
-		     u32 fence)
+		     u32 fence,
+		     bool want_response)
 {
 	struct guc_ct_buffer_desc *desc = ctb->desc;
 	u32 head = desc->head / 4;	/* in dwords */
@@ -312,6 +324,7 @@ static int ctb_write(struct intel_guc_ct_buffer *ctb,
 	 */
 	header = (len << GUC_CT_MSG_LEN_SHIFT) |
 		 (GUC_CT_MSG_WRITE_FENCE_TO_DESC) |
+		 (want_response ? GUC_CT_MSG_SEND_STATUS : 0) |
 		 (action[0] << GUC_CT_MSG_ACTION_SHIFT);
 
 	cmds[tail] = header;
@@ -380,36 +393,104 @@ static int wait_for_desc_update(struct guc_ct_buffer_desc *desc,
 	return err;
 }
 
-static int ctch_send(struct intel_guc *guc,
+/**
+ * wait_for_response_msg - Wait for the Guc response.
+ * @req:	pointer to pending request
+ * @status:	placeholder for status
+ *
+ * We will update request status from the response message handler.
+ * Returns:
+ * *	0 response received (status is valid)
+ * *	-ETIMEDOUT no response within hardcoded timeout
+ */
+static int wait_for_response_msg(struct ct_request *req, 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 INTEL_GUC_MSG_IS_RESPONSE(READ_ONCE(req->status))
+	err = wait_for_us(done, 10);
+	if (err)
+		err = wait_for(done, 10);
+#undef done
+
+	if (unlikely(err))
+		DRM_ERROR("CT: fence %u err %d\n", req->fence, err);
+
+	*status = req->status;
+	return err;
+}
+
+static int ctch_send(struct intel_guc_ct *ct,
 		     struct intel_guc_ct_channel *ctch,
 		     const u32 *action,
 		     u32 len,
+		     u32 *response_buf,
+		     u32 response_buf_size,
 		     u32 *status)
 {
 	struct intel_guc_ct_buffer *ctb = &ctch->ctbs[CTB_SEND];
 	struct guc_ct_buffer_desc *desc = ctb->desc;
+	struct ct_request request;
+	unsigned long flags;
 	u32 fence;
 	int err;
 
 	GEM_BUG_ON(!ctch_is_open(ctch));
 	GEM_BUG_ON(!len);
 	GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
+	GEM_BUG_ON(!response_buf && response_buf_size);
 
 	fence = ctch_get_next_fence(ctch);
-	err = ctb_write(ctb, action, len, fence);
+	request.fence = fence;
+	request.status = 0;
+	request.response_len = response_buf_size;
+	request.response_buf = response_buf;
+
+	spin_lock_irqsave(&ct->lock, flags);
+	list_add_tail(&request.link, &ct->pending_requests);
+	spin_unlock_irqrestore(&ct->lock, flags);
+
+	err = ctb_write(ctb, action, len, fence, !!response_buf);
 	if (unlikely(err))
-		return err;
+		goto unlink;
 
-	intel_guc_notify(guc);
+	intel_guc_notify(ct_to_guc(ct));
 
-	err = wait_for_desc_update(desc, fence, status);
+	if (response_buf)
+		err = wait_for_response_msg(&request, status);
+	else
+		err = wait_for_desc_update(desc, fence, status);
 	if (unlikely(err))
-		return err;
-	if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status))
-		return -EIO;
+		goto unlink;
+
+	if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status)) {
+		err = -EIO;
+		goto unlink;
+	}
+
+	if (response_buf) {
+		/* There shall be no data in the status */
+		WARN_ON(INTEL_GUC_MSG_TO_DATA(request.status));
+		/* Return actual response len */
+		err = request.response_len;
+	} else {
+		/* There shall be no response payload */
+		WARN_ON(request.response_len);
+		/* Return data decoded from the status dword */
+		err = INTEL_GUC_MSG_TO_DATA(*status);
+	}
 
-	/* Use data from the GuC status as our return value */
-	return INTEL_GUC_MSG_TO_DATA(*status);
+unlink:
+	spin_lock_irqsave(&ct->lock, flags);
+	list_del(&request.link);
+	spin_unlock_irqrestore(&ct->lock, flags);
+
+	return err;
 }
 
 /*
@@ -418,13 +499,15 @@ static int ctch_send(struct intel_guc *guc,
 static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len,
 			     u32 *response_buf, u32 response_buf_size)
 {
-	struct intel_guc_ct_channel *ctch = &guc->ct.host_channel;
+	struct intel_guc_ct *ct = &guc->ct;
+	struct intel_guc_ct_channel *ctch = &ct->host_channel;
 	u32 status = ~0; /* undefined */
 	int ret;
 
 	mutex_lock(&guc->send_mutex);
 
-	ret = ctch_send(guc, ctch, action, len, &status);
+	ret = ctch_send(ct, ctch, action, len, response_buf, response_buf_size,
+			&status);
 	if (unlikely(ret < 0)) {
 		DRM_ERROR("CT: send action %#X failed; err=%d status=%#X\n",
 			  action[0], ret, status);
@@ -503,8 +586,13 @@ static int ctb_read(struct intel_guc_ct_buffer *ctb, u32 *data)
 static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 {
 	u32 header = msg[0];
+	u32 fence = msg[1];
 	u32 status = msg[2];
 	u32 len = ct_header_get_len(header) + 1; /* total len with header */
+	u32 payload_len = len - 3; /* len<3 is treated as protocol error */
+	struct ct_request *req;
+	bool found = false;
+	unsigned long flags;
 
 	GEM_BUG_ON(!ct_header_is_response(header));
 
@@ -518,8 +606,29 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 		DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
 		return -EPROTO;
 	}
+	spin_lock_irqsave(&ct->lock, flags);
+	list_for_each_entry(req, &ct->pending_requests, link) {
+		if (req->fence != fence) {
+			DRM_DEBUG_DRIVER("CT: request %u awaits response\n",
+					 req->fence);
+			continue;
+		}
+		if (unlikely(payload_len > req->response_len)) {
+			DRM_ERROR("CT: response %u too long %*phn\n",
+				  req->fence, 4*len, msg);
+			payload_len = 0;
+		}
+		if (payload_len)
+			memcpy(req->response_buf, msg + 3, 4*payload_len);
+		req->response_len = payload_len;
+		WRITE_ONCE(req->status, status);
+		found = true;
+		break;
+	}
+	spin_unlock_irqrestore(&ct->lock, flags);
 
-	/* XXX */
+	if (!found)
+		DRM_ERROR("CT: unsolicited response %*phn\n", 4*len, msg);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
index 595c8ad..905566b 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.h
+++ b/drivers/gpu/drm/i915/intel_guc_ct.h
@@ -71,10 +71,15 @@ struct intel_guc_ct_channel {
 /** Holds all command transport channels.
  *
  * @host_channel: main channel used by the host
+ * @lock: spin lock for pending requests list
+ * @pending_requests: list of pending requests
  */
 struct intel_guc_ct {
 	struct intel_guc_ct_channel host_channel;
 	/* other channels are tbd */
+
+	spinlock_t lock;
+	struct list_head pending_requests;
 };
 
 void intel_guc_ct_init_early(struct intel_guc_ct *ct);
-- 
1.9.1

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

[PATCH v4 09/13] drm/i915/guc: Prepare to process incoming requests from CT

[Michal Wajdeczko]

Requests are read from CT in the irq handler, but actual processing
will be done in the work thread. Processing of specific actions will
be added in the upcoming patches.

v2: don't use GEM_BUG_ON (Chris)
    don't kmalloc too large buffer (Michal)
v3: rebased

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_ct.c | 72 ++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_guc_ct.h |  4 +++
 2 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
index 6a9aad0..90aff51 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -32,10 +32,17 @@ struct ct_request {
 	u32 *response_buf;
 };
 
+struct ct_incoming_request {
+	struct list_head link;
+	u32 data[];
+};
+
 enum { CTB_SEND = 0, CTB_RECV = 1 };
 
 enum { CTB_OWNER_HOST = 0 };
 
+static void ct_incoming_request_worker_func(struct work_struct *w);
+
 /**
  * intel_guc_ct_init_early - Initialize CT state without requiring device access
  * @ct: pointer to CT struct
@@ -47,6 +54,8 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)
 
 	spin_lock_init(&ct->lock);
 	INIT_LIST_HEAD(&ct->pending_requests);
+	INIT_LIST_HEAD(&ct->incoming_requests);
+	INIT_WORK(&ct->worker, ct_incoming_request_worker_func);
 }
 
 static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct)
@@ -632,13 +641,74 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 	return 0;
 }
 
+static void ct_dispatch_request(struct intel_guc_ct *ct,
+				u32 action, u32 len, const u32 *payload)
+{
+	switch (action) {
+	default:
+		DRM_ERROR("CT: unexpected request %x %*phn\n",
+			  action, 4*len, payload);
+		break;
+	}
+}
+
+static bool ct_process_incoming_requests(struct intel_guc_ct *ct)
+{
+	unsigned long flags;
+	struct ct_incoming_request *request;
+	bool done;
+
+	spin_lock_irqsave(&ct->lock, flags);
+	request = list_first_entry_or_null(&ct->incoming_requests,
+					   struct ct_incoming_request, link);
+	if (request)
+		list_del(&request->link);
+	done = !!list_empty(&ct->incoming_requests);
+	spin_unlock_irqrestore(&ct->lock, flags);
+
+	if (!request)
+		return true;
+
+	ct_dispatch_request(ct,
+			    ct_header_get_action(request->data[0]),
+			    ct_header_get_len(request->data[0]),
+			    &request->data[1]);
+
+	kfree(request);
+	return done;
+}
+
+static void ct_incoming_request_worker_func(struct work_struct *w)
+{
+	struct intel_guc_ct *ct = container_of(w, struct intel_guc_ct, worker);
+	bool done;
+
+	done = ct_process_incoming_requests(ct);
+	if (!done)
+		queue_work(system_unbound_wq, &ct->worker);
+}
+
 static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg)
 {
 	u32 header = msg[0];
+	u32 len = ct_header_get_len(header) + 1; /* total len with header */
+	struct ct_incoming_request *request;
+	unsigned long flags;
 
 	GEM_BUG_ON(ct_header_is_response(header));
 
-	/* XXX */
+	request = kmalloc(sizeof(*request) + 4*len, GFP_ATOMIC);
+	if (unlikely(!request)) {
+		DRM_ERROR("CT: dropping request %*phn\n", 4*len, msg);
+		return 0; /* XXX: -ENOMEM ? */
+	}
+	memcpy(request->data, msg, 4*len);
+
+	spin_lock_irqsave(&ct->lock, flags);
+	list_add_tail(&request->link, &ct->incoming_requests);
+	spin_unlock_irqrestore(&ct->lock, flags);
+
+	queue_work(system_unbound_wq, &ct->worker);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
index 905566b..0be4ce5 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.h
+++ b/drivers/gpu/drm/i915/intel_guc_ct.h
@@ -73,6 +73,8 @@ struct intel_guc_ct_channel {
  * @host_channel: main channel used by the host
  * @lock: spin lock for pending requests list
  * @pending_requests: list of pending requests
+ * @incoming_requests: list of incoming requests
+ * @worker: worker for handling incoming requests
  */
 struct intel_guc_ct {
 	struct intel_guc_ct_channel host_channel;
@@ -80,6 +82,8 @@ struct intel_guc_ct {
 
 	spinlock_t lock;
 	struct list_head pending_requests;
+	struct list_head incoming_requests;
+	struct work_struct worker;
 };
 
 void intel_guc_ct_init_early(struct intel_guc_ct *ct);
-- 
1.9.1

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

[PATCH v4 10/13] drm/i915/guc: Enable GuC interrupts when using CT

[Michal Wajdeczko]

We will need them in G2H communication to properly handle
responses and requests from the Guc.

v2: keep irq enabled while disabling GuC logging (Oscar)
v3: rebase.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Acked-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 8dc6a9c..9c20b1b 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -481,7 +481,7 @@ int intel_uc_resume(struct drm_i915_private *i915)
 	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return 0;
 
-	if (i915_modparams.guc_log_level)
+	if (HAS_GUC_CT(i915) || i915_modparams.guc_log_level)
 		gen9_enable_guc_interrupts(i915);
 
 	err = intel_guc_resume(guc);
-- 
1.9.1

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

[PATCH v4 11/13] drm/i915/guc: Handle default action received over CT

[Michal Wajdeczko]

When running on platform with CTB based GuC communication enabled,
GuC to Host event data will be delivered as CT request message.
However, content of the data[1] of this CT message follows format
of the scratch register used in MMIO based communication, so some
code reuse is still possible.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/intel_guc.c    | 5 +++++
 drivers/gpu/drm/i915/intel_guc.h    | 1 +
 drivers/gpu/drm/i915/intel_guc_ct.c | 9 +++++++++
 3 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 118db81..b6d2778 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -416,6 +416,11 @@ void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc)
 	I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
 	spin_unlock(&guc->irq_lock);
 
+	intel_guc_to_host_process_recv_msg(guc, msg);
+}
+
+void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32 msg)
+{
 	if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
 		   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED))
 		intel_guc_log_handle_flush_event(&guc->log);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 6dc109a..f1265e1 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -163,6 +163,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
 void intel_guc_to_host_event_handler(struct intel_guc *guc);
 void intel_guc_to_host_event_handler_nop(struct intel_guc *guc);
 void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc);
+void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32 msg);
 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);
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
index 90aff51..9bc8738 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -644,8 +644,17 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 static void ct_dispatch_request(struct intel_guc_ct *ct,
 				u32 action, u32 len, const u32 *payload)
 {
+	struct intel_guc *guc = ct_to_guc(ct);
+
 	switch (action) {
+	case INTEL_GUC_ACTION_DEFAULT:
+		if (unlikely(len < 1))
+			goto fail_unexpected;
+		intel_guc_to_host_process_recv_msg(guc, *payload);
+		break;
+
 	default:
+fail_unexpected:
 		DRM_ERROR("CT: unexpected request %x %*phn\n",
 			  action, 4*len, payload);
 		break;
-- 
1.9.1

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

[PATCH v4 12/13] drm/i915/guc: Trace messages from CT while in debug

[Michal Wajdeczko]

During debug we may want to investigate all communication
from the Guc. Add proper tracing macros in debug config.

v2: convert remaining DRM_DEBUG into new CT_DEBUG (Michal)
v3: use dedicated Kconfig (Daniele)

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Acked-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/Kconfig.debug  | 12 ++++++++++
 drivers/gpu/drm/i915/intel_guc_ct.c | 45 +++++++++++++++++++++++++++----------
 2 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
index dd5bf63..80efee1 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -90,6 +90,18 @@ config DRM_I915_SW_FENCE_CHECK_DAG
 
           If in doubt, say "N".
 
+config DRM_I915_DEBUG_GUC
+        bool "Enable additional driver debugging for GuC"
+        depends on DRM_I915
+        default n
+        help
+          Choose this option to turn on extra driver debugging that may affect
+          performance but will help resolve GuC related issues.
+
+          Recommended for driver developers only.
+
+          If in doubt, say "N".
+
 config DRM_I915_SELFTEST
 	bool "Enable selftests upon driver load"
 	depends on DRM_I915
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
index 9bc8738..565502c 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -24,6 +24,12 @@
 #include "i915_drv.h"
 #include "intel_guc_ct.h"
 
+#ifdef CONFIG_DRM_I915_DEBUG_GUC
+#define CT_DEBUG_DRIVER(...)	DRM_DEBUG_DRIVER(__VA_ARGS__)
+#else
+#define CT_DEBUG_DRIVER(...)
+#endif
+
 struct ct_request {
 	struct list_head link;
 	u32 fence;
@@ -78,8 +84,8 @@ static inline const char *guc_ct_buffer_type_to_str(u32 type)
 static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc,
 				    u32 cmds_addr, u32 size, u32 owner)
 {
-	DRM_DEBUG_DRIVER("CT: desc %p init addr=%#x size=%u owner=%u\n",
-			 desc, cmds_addr, size, owner);
+	CT_DEBUG_DRIVER("CT: desc %p init addr=%#x size=%u owner=%u\n",
+			desc, cmds_addr, size, owner);
 	memset(desc, 0, sizeof(*desc));
 	desc->addr = cmds_addr;
 	desc->size = size;
@@ -88,8 +94,8 @@ static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc,
 
 static void guc_ct_buffer_desc_reset(struct guc_ct_buffer_desc *desc)
 {
-	DRM_DEBUG_DRIVER("CT: desc %p reset head=%u tail=%u\n",
-			 desc, desc->head, desc->tail);
+	CT_DEBUG_DRIVER("CT: desc %p reset head=%u tail=%u\n",
+			desc, desc->head, desc->tail);
 	desc->head = 0;
 	desc->tail = 0;
 	desc->is_in_error = 0;
@@ -185,8 +191,8 @@ static int ctch_init(struct intel_guc *guc,
 		err = PTR_ERR(blob);
 		goto err_vma;
 	}
-	DRM_DEBUG_DRIVER("CT: vma base=%#x\n",
-			 intel_guc_ggtt_offset(guc, ctch->vma));
+	CT_DEBUG_DRIVER("CT: vma base=%#x\n",
+			intel_guc_ggtt_offset(guc, ctch->vma));
 
 	/* store pointers to desc and cmds */
 	for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) {
@@ -200,8 +206,8 @@ static int ctch_init(struct intel_guc *guc,
 err_vma:
 	i915_vma_unpin_and_release(&ctch->vma);
 err_out:
-	DRM_DEBUG_DRIVER("CT: channel %d initialization failed; err=%d\n",
-			 ctch->owner, err);
+	CT_DEBUG_DRIVER("CT: channel %d initialization failed; err=%d\n",
+			ctch->owner, err);
 	return err;
 }
 
@@ -221,8 +227,8 @@ static int ctch_open(struct intel_guc *guc,
 	int err;
 	int i;
 
-	DRM_DEBUG_DRIVER("CT: channel %d reopen=%s\n",
-			 ctch->owner, yesno(ctch_is_open(ctch)));
+	CT_DEBUG_DRIVER("CT: channel %d reopen=%s\n",
+			ctch->owner, yesno(ctch_is_open(ctch)));
 
 	if (!ctch->vma) {
 		err = ctch_init(guc, ctch);
@@ -336,6 +342,10 @@ static int ctb_write(struct intel_guc_ct_buffer *ctb,
 		 (want_response ? GUC_CT_MSG_SEND_STATUS : 0) |
 		 (action[0] << GUC_CT_MSG_ACTION_SHIFT);
 
+	CT_DEBUG_DRIVER("CT: writing %*phn %*phn %*phn\n",
+			4, &header, 4, &fence,
+			4*(len - 1), &action[1]);
+
 	cmds[tail] = header;
 	tail = (tail + 1) % size;
 
@@ -520,6 +530,9 @@ static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len,
 	if (unlikely(ret < 0)) {
 		DRM_ERROR("CT: send action %#X failed; err=%d status=%#X\n",
 			  action[0], ret, status);
+	} else if (unlikely(ret)) {
+		CT_DEBUG_DRIVER("CT: send action %#x returned %d (%#x)\n",
+				action[0], ret, ret);
 	}
 
 	mutex_unlock(&guc->send_mutex);
@@ -566,10 +579,12 @@ static int ctb_read(struct intel_guc_ct_buffer *ctb, u32 *data)
 	/* beware of buffer wrap case */
 	if (unlikely(available < 0))
 		available += size;
+	CT_DEBUG_DRIVER("CT: available %d (%u:%u)\n", available, head, tail);
 	GEM_BUG_ON(available < 0);
 
 	data[0] = cmds[head];
 	head = (head + 1) % size;
+	CT_DEBUG_DRIVER("CT: header %#x\n", data[0]);
 
 	/* message len with header */
 	len = ct_header_get_len(data[0]) + 1;
@@ -587,6 +602,7 @@ static int ctb_read(struct intel_guc_ct_buffer *ctb, u32 *data)
 		data[i] = cmds[head];
 		head = (head + 1) % size;
 	}
+	CT_DEBUG_DRIVER("CT: received %*phn\n", 4*len, data);
 
 	desc->head = head * 4;
 	return 0;
@@ -615,11 +631,14 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 		DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
 		return -EPROTO;
 	}
+
+	CT_DEBUG_DRIVER("CT: response fence %u status %#x\n", fence, status);
+
 	spin_lock_irqsave(&ct->lock, flags);
 	list_for_each_entry(req, &ct->pending_requests, link) {
 		if (req->fence != fence) {
-			DRM_DEBUG_DRIVER("CT: request %u awaits response\n",
-					 req->fence);
+			CT_DEBUG_DRIVER("CT: request %u awaits response\n",
+					req->fence);
 			continue;
 		}
 		if (unlikely(payload_len > req->response_len)) {
@@ -646,6 +665,8 @@ static void ct_dispatch_request(struct intel_guc_ct *ct,
 {
 	struct intel_guc *guc = ct_to_guc(ct);
 
+	CT_DEBUG_DRIVER("CT: request %x %*phn\n", action, 4*len, payload);
+
 	switch (action) {
 	case INTEL_GUC_ACTION_DEFAULT:
 		if (unlikely(len < 1))
-- 
1.9.1

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

[PATCH v4 13/13] HAX: Enable GuC for CI

[Michal Wajdeczko]

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c963603..53037b5 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -47,7 +47,7 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc, 0) \
+	param(int, enable_guc, -1) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
-- 
1.9.1

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

✗ Fi.CI.CHECKPATCH: warning for drm/i915/guc: Support for Guc responses and requests (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Support for Guc responses and requests (rev2)
URL   : https://patchwork.freedesktop.org/series/28393/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
a60fcd18ab16 drm/i915/guc: Add documentation for MMIO based communication
-:166: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'm' - possible side-effects?
#166: FILE: drivers/gpu/drm/i915/intel_guc_fwif.h:666:
+#define INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(m) \
+	((INTEL_GUC_MSG_TO_TYPE(m) == INTEL_GUC_MSG_TYPE_RESPONSE) && \
+	 (INTEL_GUC_MSG_TO_CODE(m) == INTEL_GUC_RESPONSE_STATUS_SUCCESS))

total: 0 errors, 0 warnings, 1 checks, 142 lines checked
a38b1b1e3992 drm/i915/guc: Add support for data reporting in GuC responses
ba4f9e887323 drm/i915/guc: Prepare send() function to accept bigger response
ab9a3a86d8af drm/i915/guc: Implement response handling in send_mmio()
c2ad84a29d7c drm/i915/guc: Make event handler a virtual function
7a71bdc5c387 drm/i915/guc: Prepare to handle messages from CT RECV buffer
-:106: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#106: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:506:
+		DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
 		                                             ^

-:111: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#111: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:511:
+		DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
 		                                             ^

-:133: CHECK:SPACING: spaces preferred around that '+' (ctx:VxV)
#133: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:533:
+	u32 msg[GUC_CT_MSG_LEN_MASK+1]; /* one extra dw for the header */
 	                           ^

total: 0 errors, 0 warnings, 3 checks, 156 lines checked
1bce5e250d4f drm/i915/guc: Use better name for helper wait function
3c6c074c2832 drm/i915/guc: Implement response handling in send_ct()
-:227: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#227: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:618:
+				  req->fence, 4*len, msg);
 				               ^

-:231: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#231: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:622:
+			memcpy(req->response_buf, msg + 3, 4*payload_len);
 			                                    ^

-:241: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#241: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:631:
+		DRM_ERROR("CT: unsolicited response %*phn\n", 4*len, msg);
 		                                               ^

-:260: CHECK:UNCOMMENTED_DEFINITION: spinlock_t definition without comment
#260: FILE: drivers/gpu/drm/i915/intel_guc_ct.h:81:
+	spinlock_t lock;

total: 0 errors, 0 warnings, 4 checks, 228 lines checked
a6ef9e1ee9b7 drm/i915/guc: Prepare to process incoming requests from CT
-:60: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#60: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:650:
+			  action, 4*len, payload);
 			           ^

-:111: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#111: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:700:
+	request = kmalloc(sizeof(*request) + 4*len, GFP_ATOMIC);
 	                                      ^

-:113: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#113: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:702:
+		DRM_ERROR("CT: dropping request %*phn\n", 4*len, msg);
 		                                           ^

-:116: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#116: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:705:
+	memcpy(request->data, msg, 4*len);
 	                            ^

total: 0 errors, 0 warnings, 4 checks, 116 lines checked
ddde59b7d24c drm/i915/guc: Enable GuC interrupts when using CT
acf08ee20d5a drm/i915/guc: Handle default action received over CT
aca333d32f3e drm/i915/guc: Trace messages from CT while in debug
-:118: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#118: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:347:
+			4*(len - 1), &action[1]);
 			 ^

-:150: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#150: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:605:
+	CT_DEBUG_DRIVER("CT: received %*phn\n", 4*len, data);
 	                                         ^

-:175: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#175: FILE: drivers/gpu/drm/i915/intel_guc_ct.c:668:
+	CT_DEBUG_DRIVER("CT: request %x %*phn\n", action, 4*len, payload);
 	                                                   ^

total: 0 errors, 0 warnings, 3 checks, 142 lines checked
6f34cddf2ca7 HAX: Enable GuC for CI

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

✗ Fi.CI.SPARSE: warning for drm/i915/guc: Support for Guc responses and requests (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Support for Guc responses and requests (rev2)
URL   : https://patchwork.freedesktop.org/series/28393/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/guc: Add documentation for MMIO based communication
Okay!

Commit: drm/i915/guc: Add support for data reporting in GuC responses
Okay!

Commit: drm/i915/guc: Prepare send() function to accept bigger response
Okay!

Commit: drm/i915/guc: Implement response handling in send_mmio()
Okay!

Commit: drm/i915/guc: Make event handler a virtual function
Okay!

Commit: drm/i915/guc: Prepare to handle messages from CT RECV buffer
Okay!

Commit: drm/i915/guc: Use better name for helper wait function
Okay!

Commit: drm/i915/guc: Implement response handling in send_ct()
Okay!

Commit: drm/i915/guc: Prepare to process incoming requests from CT
Okay!

Commit: drm/i915/guc: Enable GuC interrupts when using CT
Okay!

Commit: drm/i915/guc: Handle default action received over CT
Okay!

Commit: drm/i915/guc: Trace messages from CT while in debug
+Error in reading or end of file.

Commit: HAX: Enable GuC for CI
Okay!

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

✓ Fi.CI.BAT: success for drm/i915/guc: Support for Guc responses and requests (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Support for Guc responses and requests (rev2)
URL   : https://patchwork.freedesktop.org/series/28393/
State : success

== Summary ==

Series 28393v2 drm/i915/guc: Support for Guc responses and requests
https://patchwork.freedesktop.org/api/1.0/series/28393/revisions/2/mbox/

---- Possible new issues:

Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                incomplete -> PASS       (fi-bxt-dsi)

---- Known issues:

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                notrun     -> INCOMPLETE (fi-bxt-dsi) fdo#103927

fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:434s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:441s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:379s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:540s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:296s
fi-bxt-dsi       total:243  pass:216  dwarn:0   dfail:0   fail:0   skip:26 
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:521s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:521s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:510s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:408s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:510s
fi-cnl-y3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:583s
fi-elk-e7500     total:285  pass:225  dwarn:1   dfail:0   fail:0   skip:59  time:421s
fi-gdg-551       total:285  pass:176  dwarn:0   dfail:0   fail:1   skip:108 time:315s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:403s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:423s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:481s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:428s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:472s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:463s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:513s
fi-pnv-d510      total:285  pass:219  dwarn:1   dfail:0   fail:0   skip:65  time:649s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:441s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:537s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:502s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:506s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:430s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:446s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:568s
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:400s
Blacklisted hosts:
fi-cfl-s3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:565s
fi-cnl-psr       total:224  pass:198  dwarn:0   dfail:0   fail:1   skip:24 
fi-glk-j4005     total:285  pass:226  dwarn:30  dfail:0   fail:0   skip:29  time:488s

157295bdb3516bc0972daffcef016d668f549d79 drm-tip: 2018y-03m-23d-15h-22m-25s UTC integration manifest
6f34cddf2ca7 HAX: Enable GuC for CI
aca333d32f3e drm/i915/guc: Trace messages from CT while in debug
acf08ee20d5a drm/i915/guc: Handle default action received over CT
ddde59b7d24c drm/i915/guc: Enable GuC interrupts when using CT
a6ef9e1ee9b7 drm/i915/guc: Prepare to process incoming requests from CT
3c6c074c2832 drm/i915/guc: Implement response handling in send_ct()
1bce5e250d4f drm/i915/guc: Use better name for helper wait function
7a71bdc5c387 drm/i915/guc: Prepare to handle messages from CT RECV buffer
c2ad84a29d7c drm/i915/guc: Make event handler a virtual function
ab9a3a86d8af drm/i915/guc: Implement response handling in send_mmio()
ba4f9e887323 drm/i915/guc: Prepare send() function to accept bigger response
a38b1b1e3992 drm/i915/guc: Add support for data reporting in GuC responses
a60fcd18ab16 drm/i915/guc: Add documentation for MMIO based communication

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8476/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for drm/i915/guc: Support for Guc responses and requests (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Support for Guc responses and requests (rev2)
URL   : https://patchwork.freedesktop.org/series/28393/
State : failure

== Summary ==

---- Possible new issues:

Test kms_frontbuffer_tracking:
        Subgroup fbc-rgb101010-draw-blt:
                fail       -> PASS       (shard-apl)
Test perf:
        Subgroup gen8-unprivileged-single-ctx-counters:
                pass       -> FAIL       (shard-apl)
Test pm_rpm:
        Subgroup debugfs-read:
                pass       -> DMESG-WARN (shard-apl)

---- Known issues:

Test drv_missed_irq:
                pass       -> SKIP       (shard-apl) fdo#103199
Test kms_cursor_crc:
        Subgroup cursor-128x128-suspend:
                incomplete -> PASS       (shard-hsw) fdo#103540
Test kms_flip:
        Subgroup 2x-flip-vs-expired-vblank:
                pass       -> FAIL       (shard-hsw) fdo#102887
        Subgroup plain-flip-ts-check:
                pass       -> FAIL       (shard-hsw) fdo#100368 +1
Test kms_sysfs_edid_timing:
                pass       -> WARN       (shard-apl) fdo#100047

fdo#103199 https://bugs.freedesktop.org/show_bug.cgi?id=103199
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047

shard-apl        total:3484 pass:1817 dwarn:2   dfail:0   fail:8   skip:1656 time:13392s
shard-hsw        total:3484 pass:1771 dwarn:1   dfail:0   fail:4   skip:1707 time:11607s
shard-snb        total:3484 pass:1363 dwarn:1   dfail:0   fail:3   skip:2117 time:7060s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8476/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 01/13] drm/i915/guc: Add documentation for MMIO based communication

[Michel Thierry]

On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
> As we are going to extend our use of MMIO based communication,
> try to explain its mechanics and update corresponding definitions.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Kelvin Gardiner <kelvin.gardiner@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc.c      | 20 ++++----
>   drivers/gpu/drm/i915/intel_guc_ct.c   |  2 +-
>   drivers/gpu/drm/i915/intel_guc_fwif.h | 86 ++++++++++++++++++++++++++++-------
>   3 files changed, 80 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 8f93f5b..28075e6 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -329,6 +329,9 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>          GEM_BUG_ON(!len);
>          GEM_BUG_ON(len > guc->send_regs.count);
> 
> +       /* We expect only action code */
> +       GEM_BUG_ON(*action & ~INTEL_GUC_MSG_CODE_MASK);
> +
>          /* If CT is available, we expect to use MMIO only during init/fini */
>          GEM_BUG_ON(HAS_GUC_CT(dev_priv) &&
>                  *action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
> @@ -350,18 +353,15 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>           */
>          ret = __intel_wait_for_register_fw(dev_priv,
>                                             guc_send_reg(guc, 0),
> -                                          INTEL_GUC_RECV_MASK,
> -                                          INTEL_GUC_RECV_MASK,
> +                                          INTEL_GUC_MSG_TYPE_MASK,
> +                                          INTEL_GUC_MSG_TYPE_RESPONSE <<
> +                                          INTEL_GUC_MSG_TYPE_SHIFT,
>                                             10, 10, &status);
> -       if (status != INTEL_GUC_STATUS_SUCCESS) {
> -               /*
> -                * Either the GuC explicitly returned an error (which
> -                * we convert to -EIO here) or no response at all was
> -                * received within the timeout limit (-ETIMEDOUT)
> -                */
> -               if (ret != -ETIMEDOUT)
> -                       ret = -EIO;
> +       /* If GuC explicitly returned an error, convert it to -EIO */
> +       if (!ret && !INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(status))
> +               ret = -EIO;
> 
> +       if (ret) {
>                  DRM_DEBUG_DRIVER("INTEL_GUC_SEND: Action 0x%X failed;"
>                                   " ret=%d status=0x%08X response=0x%08X\n",
>                                   action[0], ret, status,
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
> index a726283..1dafa7a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> @@ -398,7 +398,7 @@ static int ctch_send(struct intel_guc *guc,
>          err = wait_for_response(desc, fence, status);
>          if (unlikely(err))
>                  return err;
> -       if (*status != INTEL_GUC_STATUS_SUCCESS)
> +       if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status))
>                  return -EIO;
>          return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 72941bd..1701879 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -560,7 +560,68 @@ struct guc_shared_ctx_data {
>          struct guc_ctx_report preempt_ctx_report[GUC_MAX_ENGINES_NUM];
>   } __packed;
> 
> -/* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */
> +/**
> + * 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
> + *
> + *      +-----------+---------+---------+---------+
> + *      |  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* > + *

Looks ok to me. Just one comment, I would have used 'cmd' or 'action' 
instead of 'code' but that's personal preference (the archaic fw docs 
use 'action code' for this field, is it why you decided to use code?).

Plus it isn't like we want to keep the same names, looking at 
intel_guc_fwif.h vs the 'original', we've managed to change the name of 
almost every single item.

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

> + * 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 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,
> @@ -597,24 +658,15 @@ enum intel_guc_report_status {
>   #define GUC_LOG_CONTROL_VERBOSITY_MASK (0xF << GUC_LOG_CONTROL_VERBOSITY_SHIFT)
>   #define GUC_LOG_CONTROL_DEFAULT_LOGGING        (1 << 8)
> 
> -/*
> - * The GuC sends its response to a command by overwriting the
> - * command in SS0. The response is distinguishable from a command
> - * by the fact that all the MASK bits are set. The remaining bits
> - * give more detail.
> - */
> -#define        INTEL_GUC_RECV_MASK     ((u32)0xF0000000)
> -#define        INTEL_GUC_RECV_IS_RESPONSE(x)   ((u32)(x) >= INTEL_GUC_RECV_MASK)
> -#define        INTEL_GUC_RECV_STATUS(x)        (INTEL_GUC_RECV_MASK | (x))
> -
> -/* GUC will return status back to SOFT_SCRATCH_O_REG */
> -enum intel_guc_status {
> -       INTEL_GUC_STATUS_SUCCESS = INTEL_GUC_RECV_STATUS(0x0),
> -       INTEL_GUC_STATUS_ALLOCATE_DOORBELL_FAIL = INTEL_GUC_RECV_STATUS(0x10),
> -       INTEL_GUC_STATUS_DEALLOCATE_DOORBELL_FAIL = INTEL_GUC_RECV_STATUS(0x20),
> -       INTEL_GUC_STATUS_GENERIC_FAIL = INTEL_GUC_RECV_STATUS(0x0000F000)
> +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) \
> +       ((INTEL_GUC_MSG_TO_TYPE(m) == INTEL_GUC_MSG_TYPE_RESPONSE) && \
> +        (INTEL_GUC_MSG_TO_CODE(m) == INTEL_GUC_RESPONSE_STATUS_SUCCESS))
> +
>   /* This action will be programmed in C1BC - SOFT_SCRATCH_15_REG */
>   enum intel_guc_recv_message {
>          INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED = BIT(1),
> --
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 02/13] drm/i915/guc: Add support for data reporting in GuC responses

[Michel Thierry]

On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
> GuC may return additional data in the response message.
> Format and meaning of this data is action specific. We will
> use this non-negative data as a new success return value.
> Currently used actions don't return data that way yet.
> 
> v2: fix prohibited space after '~' (Michel)
>      update commit message (Daniele)
> v3: rebase
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com> #2

The r-b stands for v3.


> ---
>   drivers/gpu/drm/i915/intel_guc.c    |  3 +++
>   drivers/gpu/drm/i915/intel_guc_ct.c | 14 ++++++++------
>   2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 28075e6..77bf4e6 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -366,6 +366,9 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
>   				 " ret=%d status=0x%08X response=0x%08X\n",
>   				 action[0], ret, status,
>   				 I915_READ(SOFT_SCRATCH(15)));
> +	} else {
> +		/* Use data from the GuC response as our return value */
> +		ret = INTEL_GUC_MSG_TO_DATA(status);
>   	}
>   
>   	intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
> index 1dafa7a..fa52259 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> @@ -400,7 +400,9 @@ static int ctch_send(struct intel_guc *guc,
>   		return err;
>   	if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status))
>   		return -EIO;
> -	return 0;
> +
> +	/* Use data from the GuC status as our return value */
> +	return INTEL_GUC_MSG_TO_DATA(*status);
>   }
>   
>   /*
> @@ -410,18 +412,18 @@ static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len)
>   {
>   	struct intel_guc_ct_channel *ctch = &guc->ct.host_channel;
>   	u32 status = ~0; /* undefined */
> -	int err;
> +	int ret;
>   
>   	mutex_lock(&guc->send_mutex);
>   
> -	err = ctch_send(guc, ctch, action, len, &status);
> -	if (unlikely(err)) {
> +	ret = ctch_send(guc, ctch, action, len, &status);
> +	if (unlikely(ret < 0)) {
>   		DRM_ERROR("CT: send action %#X failed; err=%d status=%#X\n",
> -			  action[0], err, status);
> +			  action[0], ret, status);
>   	}
>   
>   	mutex_unlock(&guc->send_mutex);
> -	return err;
> +	return ret;
>   }
>   
>   /**
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 03/13] drm/i915/guc: Prepare send() function to accept bigger response

[Michel Thierry]

On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
> This is a preparation step for the upcoming patches.
> We already can return some small data decoded from the command
> status, but we will need more in the future.
> 
> v2: add explicit response buf size
> v3: squash with helper patch
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com> #1

  r-b still applies to v3.

> ---
>   drivers/gpu/drm/i915/intel_guc.c    |  6 ++++--
>   drivers/gpu/drm/i915/intel_guc.h    | 18 ++++++++++++++----
>   drivers/gpu/drm/i915/intel_guc_ct.c |  7 ++++---
>   3 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 77bf4e6..a533ff8 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -310,7 +310,8 @@ void intel_guc_init_params(struct intel_guc *guc)
>   	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_BLITTER);
>   }
>   
> -int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
> +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len,
> +		       u32 *response_buf, u32 response_buf_size)
>   {
>   	WARN(1, "Unexpected send: action=%#x\n", *action);
>   	return -ENODEV;
> @@ -319,7 +320,8 @@ int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
>   /*
>    * This function implements the MMIO based host to GuC interface.
>    */
> -int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> +int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
> +			u32 *response_buf, u32 response_buf_size)
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   	u32 status;
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 13f3d1d..7ee0732 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -88,7 +88,8 @@ struct intel_guc {
>   	struct mutex send_mutex;
>   
>   	/* GuC's FW specific send function */
> -	int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
> +	int (*send)(struct intel_guc *guc, const u32 *data, u32 len,
> +		    u32 *response_buf, u32 response_buf_size);
>   
>   	/* GuC's FW specific notify function */
>   	void (*notify)(struct intel_guc *guc);
> @@ -97,7 +98,14 @@ struct intel_guc {
>   static
>   inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
>   {
> -	return guc->send(guc, action, len);
> +	return guc->send(guc, action, len, NULL, 0);
> +}
> +
> +static inline int
> +intel_guc_send_and_receive(struct intel_guc *guc, const u32 *action, u32 len,
> +			   u32 *response_buf, u32 response_buf_size)
> +{
> +	return guc->send(guc, action, len, response_buf, response_buf_size);
>   }
>   
>   static inline void intel_guc_notify(struct intel_guc *guc)
> @@ -140,8 +148,10 @@ static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc,
>   void intel_guc_fini_wq(struct intel_guc *guc);
>   int intel_guc_init(struct intel_guc *guc);
>   void intel_guc_fini(struct intel_guc *guc);
> -int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
> -int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
> +int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len,
> +		       u32 *response_buf, u32 response_buf_size);
> +int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
> +			u32 *response_buf, u32 response_buf_size);
>   void intel_guc_to_host_event_handler(struct intel_guc *guc);
>   int intel_guc_sample_forcewake(struct intel_guc *guc);
>   int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
> index fa52259..a54bf58 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> @@ -88,7 +88,7 @@ static int guc_action_register_ct_buffer(struct intel_guc *guc,
>   	int err;
>   
>   	/* Can't use generic send(), CT registration must go over MMIO */
> -	err = intel_guc_send_mmio(guc, action, ARRAY_SIZE(action));
> +	err = intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0);
>   	if (err)
>   		DRM_ERROR("CT: register %s buffer failed; err=%d\n",
>   			  guc_ct_buffer_type_to_str(type), err);
> @@ -107,7 +107,7 @@ static int guc_action_deregister_ct_buffer(struct intel_guc *guc,
>   	int err;
>   
>   	/* Can't use generic send(), CT deregistration must go over MMIO */
> -	err = intel_guc_send_mmio(guc, action, ARRAY_SIZE(action));
> +	err = intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0);
>   	if (err)
>   		DRM_ERROR("CT: deregister %s buffer failed; owner=%d err=%d\n",
>   			  guc_ct_buffer_type_to_str(type), owner, err);
> @@ -408,7 +408,8 @@ static int ctch_send(struct intel_guc *guc,
>   /*
>    * Command Transport (CT) buffer based GuC send function.
>    */
> -static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len)
> +static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len,
> +			     u32 *response_buf, u32 response_buf_size)
>   {
>   	struct intel_guc_ct_channel *ctch = &guc->ct.host_channel;
>   	u32 status = ~0; /* undefined */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 04/13] drm/i915/guc: Implement response handling in send_mmio()

[Michel Thierry]

On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
> We're using data encoded in the status MMIO as return value from send
> function, but GuC may also write more data in remaining MMIO regs.
> Let's copy content of these registers to the buffer provided by caller.
> 
> v2: new line (Michel)
> v3: updated commit message
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com> #2
> ---
>   drivers/gpu/drm/i915/intel_guc.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index a533ff8..9ce01e5 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -368,11 +368,20 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
>                                   " ret=%d status=0x%08X response=0x%08X\n",
>                                   action[0], ret, status,
>                                   I915_READ(SOFT_SCRATCH(15)));
> -       } else {
> -               /* Use data from the GuC response as our return value */
> -               ret = INTEL_GUC_MSG_TO_DATA(status);
> +               goto out;
>          }
> 

I'm not a big fan of goto's, so I would have added the response handling 
in the else part.

But it's still correct, so my old r-b still stands.

-Michel

> +       if (response_buf) {
> +               int count = min(response_buf_size, guc->send_regs.count - 1);
> +
> +               for (i = 0; i < count; i++)
> +                       response_buf[i] = I915_READ(guc_send_reg(guc, i + 1));
> +       }
> +
> +       /* Use data from the GuC response as our return value */
> +       ret = INTEL_GUC_MSG_TO_DATA(status);
> +
> +out:
>          intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
>          mutex_unlock(&guc->send_mutex);
> 
> --
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 05/13] drm/i915/guc: Make event handler a virtual function

[Michel Thierry]

On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
> On platforms with CTB based GuC communications, we will handle
> GuC events in a different way. Let's make event handler a virtual
> function to allow easy switch between those variants.
> 
> Credits-to: Oscar Mateo <oscar.mateo@intel.com>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc.c |  8 +++++++-
>   drivers/gpu/drm/i915/intel_guc.h | 10 ++++++++++
>   drivers/gpu/drm/i915/intel_uc.c  |  2 ++
>   3 files changed, 19 insertions(+), 1 deletion(-)
> 

I've gotten used to 'receive', but 'handler' makes sense too.

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 9ce01e5..118db81 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -69,6 +69,7 @@ void intel_guc_init_early(struct intel_guc *guc)
>          mutex_init(&guc->send_mutex);
>          spin_lock_init(&guc->irq_lock);
>          guc->send = intel_guc_send_nop;
> +       guc->handler = intel_guc_to_host_event_handler_nop;
>          guc->notify = gen8_guc_raise_irq;
>   }
> 
> @@ -317,6 +318,11 @@ int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len,
>          return -ENODEV;
>   }
> 
> +void intel_guc_to_host_event_handler_nop(struct intel_guc *guc)
> +{
> +       WARN(1, "Unexpected event: no suitable handler\n");
> +}
> +
>   /*
>    * This function implements the MMIO based host to GuC interface.
>    */
> @@ -388,7 +394,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
>          return ret;
>   }
> 
> -void intel_guc_to_host_event_handler(struct intel_guc *guc)
> +void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc)
>   {
>          struct drm_i915_private *dev_priv = guc_to_i915(guc);
>          u32 msg, val;
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 7ee0732..6dc109a 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -91,6 +91,9 @@ struct intel_guc {
>          int (*send)(struct intel_guc *guc, const u32 *data, u32 len,
>                      u32 *response_buf, u32 response_buf_size);
> 
> +       /* GuC's FW specific event handler function */
> +       void (*handler)(struct intel_guc *guc);
> +
>          /* GuC's FW specific notify function */
>          void (*notify)(struct intel_guc *guc);
>   };
> @@ -113,6 +116,11 @@ static inline void intel_guc_notify(struct intel_guc *guc)
>          guc->notify(guc);
>   }
> 
> +static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)
> +{
> +       guc->handler(guc);
> +}
> +
>   /* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */
>   #define GUC_GGTT_TOP   0xFEE00000
> 
> @@ -153,6 +161,8 @@ int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len,
>   int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
>                          u32 *response_buf, u32 response_buf_size);
>   void intel_guc_to_host_event_handler(struct intel_guc *guc);
> +void intel_guc_to_host_event_handler_nop(struct intel_guc *guc);
> +void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc);
>   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);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 34f8a2c..8dc6a9c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -234,6 +234,7 @@ static int guc_enable_communication(struct intel_guc *guc)
>                  return intel_guc_ct_enable(&guc->ct);
> 
>          guc->send = intel_guc_send_mmio;
> +       guc->handler = intel_guc_to_host_event_handler_mmio;
>          return 0;
>   }
> 
> @@ -247,6 +248,7 @@ static void guc_disable_communication(struct intel_guc *guc)
>          gen9_disable_guc_interrupts(dev_priv);
> 
>          guc->send = intel_guc_send_nop;
> +       guc->handler = intel_guc_to_host_event_handler_nop;
>   }
> 
>   int intel_uc_init_misc(struct drm_i915_private *dev_priv)
> --
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 07/13] drm/i915/guc: Use better name for helper wait function

[Michel Thierry]

On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
> In next patch we will introduce another way of waiting for the response
> that will use RECV buffer. To avoid misleading names, rename old wait
> function to reflect the fact that it is based on descriptor update.
> 
> v2: fix comment style (Michal)
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_ct.c | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
> index f029ff3..b1ab28f 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> @@ -332,16 +332,23 @@ static int ctb_write(struct intel_guc_ct_buffer *ctb,
>          return 0;
>   }
> 
> -/* Wait for the response from the GuC.
> +/**
> + * wait_for_desc_update - Wait for the descriptor update.
> + * @desc:      buffer descriptor
>    * @fence:     response fence
>    * @status:    placeholder for status
> - * return:     0 response received (status is valid)
> - *             -ETIMEDOUT no response within hardcoded timeout
> - *             -EPROTO no response, ct buffer was in error
> + *
> + * Guc will update descriptor with new fence and status
> + * after processing 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_response(struct guc_ct_buffer_desc *desc,
> -                            u32 fence,
> -                            u32 *status)
> +static int wait_for_desc_update(struct guc_ct_buffer_desc *desc,

Would you consider 'wait_for_ctb_desc_update'?
I know this is only used with-in the guc_ct.c file, but there's so many 
other descriptors already.

With or without that,

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

> +                               u32 fence,
> +                               u32 *status)
>   {
>          int err;
> 
> @@ -395,7 +402,7 @@ static int ctch_send(struct intel_guc *guc,
> 
>          intel_guc_notify(guc);
> 
> -       err = wait_for_response(desc, fence, status);
> +       err = wait_for_desc_update(desc, fence, status);
>          if (unlikely(err))
>                  return err;
>          if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status))
> --
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 06/13] drm/i915/guc: Prepare to handle messages from CT RECV buffer

[Michel Thierry]

On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
> GuC can respond to our commands not only by updating SEND buffer
> descriptor, but can also send a response message over RECV buffer.
> Guc can also send unsolicited request messages over RECV buffer.
> Let's start reading those messages and make placeholders
> for actual response/request handlers.
> 
> v2: misc improvements (Michal)
> v3: change response detection (Michal)
>      invalid status is protocol error (Michal)
> v4: rebase
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_ct.c | 139 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 139 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
> index a54bf58..f029ff3 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> @@ -427,6 +427,143 @@ static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len,
>   	return ret;
>   }
>   
> +static inline unsigned int ct_header_get_len(u32 header)
> +{
> +	return (header >> GUC_CT_MSG_LEN_SHIFT) & GUC_CT_MSG_LEN_MASK;
> +}
> +
> +static inline unsigned int ct_header_get_action(u32 header)
> +{
> +	return (header >> GUC_CT_MSG_ACTION_SHIFT) & GUC_CT_MSG_ACTION_MASK;
> +}
> +
> +static inline bool ct_header_is_response(u32 header)
> +{
> +	return ct_header_get_action(header) == INTEL_GUC_ACTION_DEFAULT;
> +}
> +
> +static int ctb_read(struct intel_guc_ct_buffer *ctb, u32 *data)
> +{
> +	struct guc_ct_buffer_desc *desc = ctb->desc;
> +	u32 head = desc->head / 4;	/* in dwords */
> +	u32 tail = desc->tail / 4;	/* in dwords */
> +	u32 size = desc->size / 4;	/* in dwords */
> +	u32 *cmds = ctb->cmds;
> +	s32 available;			/* in dwords */
> +	unsigned int len;
> +	unsigned int i;
> +
> +	GEM_BUG_ON(desc->size % 4);
> +	GEM_BUG_ON(desc->head % 4);
> +	GEM_BUG_ON(desc->tail % 4);
> +	GEM_BUG_ON(tail >= size);
> +	GEM_BUG_ON(head >= size);
> +
> +	/* tail == head condition indicates empty */
> +	available = tail - head;
> +	if (unlikely(available == 0))
> +		return -ENODATA;
> +
> +	/* beware of buffer wrap case */
> +	if (unlikely(available < 0))
> +		available += size;
> +	GEM_BUG_ON(available < 0);
> +
> +	data[0] = cmds[head];
> +	head = (head + 1) % size;
> +
> +	/* message len with header */
> +	len = ct_header_get_len(data[0]) + 1;
> +	if (unlikely(len > (u32)available)) {
> +		DRM_ERROR("CT: incomplete message %*phn %*phn %*phn\n",
> +			  4, data,
> +			  4 * (head + available - 1 > size ?
> +			       size - head : available - 1), &cmds[head],
> +			  4 * (head + available - 1 > size ?
> +			       available - 1 - size + head : 0), &cmds[0]);
> +		return -EPROTO;
> +	}
> +
> +	for (i = 1; i < len; i++) {
> +		data[i] = cmds[head];
> +		head = (head + 1) % size;
> +	}
> +
> +	desc->head = head * 4;
> +	return 0;
> +}
> +
> +static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
> +{
> +	u32 header = msg[0];
> +	u32 status = msg[2];
> +	u32 len = ct_header_get_len(header) + 1; /* total len with header */
> +
> +	GEM_BUG_ON(!ct_header_is_response(header));
> +
> +	/* Response message shall at least include header, fence and status */
> +	if (unlikely(len < 3)) {
> +		DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
Fi.CI.CHECKPATCH wants a space around that '4*len'

> +		return -EPROTO;
> +	}
> +	/* Format of the status follows RESPONSE message */
> +	if (unlikely(!INTEL_GUC_MSG_IS_RESPONSE(status))) {
> +		DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
also here

> +		return -EPROTO;
> +	}
> +
> +	/* XXX */
> +	return 0;
> +}
> +
> +static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg)
> +{
> +	u32 header = msg[0];
> +
> +	GEM_BUG_ON(ct_header_is_response(header));
> +
> +	/* XXX */
> +	return 0;
> +}
> +
> +static void ct_process_host_channel(struct intel_guc_ct *ct)
> +{
> +	struct intel_guc_ct_channel *ctch = &ct->host_channel;
> +	struct intel_guc_ct_buffer *ctb = &ctch->ctbs[CTB_RECV];
> +	u32 msg[GUC_CT_MSG_LEN_MASK+1]; /* one extra dw for the header *and here.

Otherwise

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

> +	int err = 0;
> +
> +	if (!ctch_is_open(ctch))
> +		return;
> +
> +	do {
> +		err = ctb_read(ctb, msg);
> +		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);
> +
> +	if (GEM_WARN_ON(err == -EPROTO)) {
> +		DRM_ERROR("CT: corrupted message detected!\n");
> +		ctb->desc->is_in_error = 1;
> +	}
> +}
> +
> +/*
> + * When we're communicating with the GuC over CT, GuC uses events
> + * to notify us about new messages being posted on the RECV buffer.
> + */
> +static void intel_guc_to_host_event_handler_ct(struct intel_guc *guc)
> +{
> +	struct intel_guc_ct *ct = &guc->ct;
> +
> +	ct_process_host_channel(ct);
> +}
> +
>   /**
>    * intel_guc_ct_enable - Enable buffer based command transport.
>    * @ct: pointer to CT struct
> @@ -450,6 +587,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
>   
>   	/* Switch into cmd transport buffer based send() */
>   	guc->send = intel_guc_send_ct;
> +	guc->handler = intel_guc_to_host_event_handler_ct;
>   	DRM_INFO("CT: %s\n", enableddisabled(true));
>   	return 0;
>   }
> @@ -475,5 +613,6 @@ void intel_guc_ct_disable(struct intel_guc_ct *ct)
>   
>   	/* Disable send */
>   	guc->send = intel_guc_send_nop;
> +	guc->handler = intel_guc_to_host_event_handler_nop;
>   	DRM_INFO("CT: %s\n", enableddisabled(false));
>   }
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 08/13] drm/i915/guc: Implement response handling in send_ct()

[Michel Thierry]

On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
> Instead of returning small data in response status dword,
> GuC may append longer data as response message payload.
> If caller provides response buffer, we will copy received
> data and use number of received data dwords as new success
> return value. We will WARN if response from GuC does not
> match caller expectation.
> 
> v2: fix timeout and checkpatch warnings (Michal)
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_ct.c | 137 ++++++++++++++++++++++++++++++++----
>   drivers/gpu/drm/i915/intel_guc_ct.h |   5 ++
>   2 files changed, 128 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
> index b1ab28f..6a9aad0 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> @@ -24,6 +24,14 @@
>   #include "i915_drv.h"
>   #include "intel_guc_ct.h"
>   
> +struct ct_request {
> +	struct list_head link;
> +	u32 fence;
> +	u32 status;
> +	u32 response_len;
> +	u32 *response_buf;
> +};
> +
>   enum { CTB_SEND = 0, CTB_RECV = 1 };
>   
>   enum { CTB_OWNER_HOST = 0 };
> @@ -36,6 +44,9 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)
>   {
>   	/* we're using static channel owners */
>   	ct->host_channel.owner = CTB_OWNER_HOST;
> +
> +	spin_lock_init(&ct->lock);
> +	INIT_LIST_HEAD(&ct->pending_requests);
>   }
>   
>   static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct)
> @@ -276,7 +287,8 @@ static u32 ctch_get_next_fence(struct intel_guc_ct_channel *ctch)
>   static int ctb_write(struct intel_guc_ct_buffer *ctb,
>   		     const u32 *action,
>   		     u32 len /* in dwords */,
> -		     u32 fence)
> +		     u32 fence,
> +		     bool want_response)
>   {
>   	struct guc_ct_buffer_desc *desc = ctb->desc;
>   	u32 head = desc->head / 4;	/* in dwords */
> @@ -312,6 +324,7 @@ static int ctb_write(struct intel_guc_ct_buffer *ctb,
>   	 */
>   	header = (len << GUC_CT_MSG_LEN_SHIFT) |
>   		 (GUC_CT_MSG_WRITE_FENCE_TO_DESC) |
> +		 (want_response ? GUC_CT_MSG_SEND_STATUS : 0) |
>   		 (action[0] << GUC_CT_MSG_ACTION_SHIFT);
>   
>   	cmds[tail] = header;
> @@ -380,36 +393,104 @@ static int wait_for_desc_update(struct guc_ct_buffer_desc *desc,
>   	return err;
>   }
>   
> -static int ctch_send(struct intel_guc *guc,
> +/**
> + * wait_for_response_msg - Wait for the Guc response.
> + * @req:	pointer to pending request
> + * @status:	placeholder for status
> + *
> + * We will update request status from the response message handler.
> + * Returns:
> + * *	0 response received (status is valid)
> + * *	-ETIMEDOUT no response within hardcoded timeout
> + */
> +static int wait_for_response_msg(struct ct_request *req, 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 INTEL_GUC_MSG_IS_RESPONSE(READ_ONCE(req->status))
> +	err = wait_for_us(done, 10);
> +	if (err)
> +		err = wait_for(done, 10);
> +#undef done
> +
> +	if (unlikely(err))
> +		DRM_ERROR("CT: fence %u err %d\n", req->fence, err);
> +
> +	*status = req->status;
> +	return err;
> +}
> +
> +static int ctch_send(struct intel_guc_ct *ct,
>   		     struct intel_guc_ct_channel *ctch,
>   		     const u32 *action,
>   		     u32 len,
> +		     u32 *response_buf,
> +		     u32 response_buf_size,
>   		     u32 *status)
>   {
>   	struct intel_guc_ct_buffer *ctb = &ctch->ctbs[CTB_SEND];
>   	struct guc_ct_buffer_desc *desc = ctb->desc;
> +	struct ct_request request;
> +	unsigned long flags;
>   	u32 fence;
>   	int err;
>   
>   	GEM_BUG_ON(!ctch_is_open(ctch));
>   	GEM_BUG_ON(!len);
>   	GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> +	GEM_BUG_ON(!response_buf && response_buf_size);
>   
>   	fence = ctch_get_next_fence(ctch);
> -	err = ctb_write(ctb, action, len, fence);
> +	request.fence = fence;
> +	request.status = 0;
> +	request.response_len = response_buf_size;
> +	request.response_buf = response_buf;
> +
> +	spin_lock_irqsave(&ct->lock, flags);
> +	list_add_tail(&request.link, &ct->pending_requests);
> +	spin_unlock_irqrestore(&ct->lock, flags);
> +
> +	err = ctb_write(ctb, action, len, fence, !!response_buf);
>   	if (unlikely(err))
> -		return err;
> +		goto unlink;
>   
> -	intel_guc_notify(guc);
> +	intel_guc_notify(ct_to_guc(ct));
>   
> -	err = wait_for_desc_update(desc, fence, status);
> +	if (response_buf)
> +		err = wait_for_response_msg(&request, status);
> +	else
> +		err = wait_for_desc_update(desc, fence, status);
>   	if (unlikely(err))
> -		return err;
> -	if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status))
> -		return -EIO;
> +		goto unlink;
> +
> +	if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status)) {
> +		err = -EIO;
> +		goto unlink;
> +	}
> +
> +	if (response_buf) {
> +		/* There shall be no data in the status */
> +		WARN_ON(INTEL_GUC_MSG_TO_DATA(request.status));
> +		/* Return actual response len */
> +		err = request.response_len;
> +	} else {
> +		/* There shall be no response payload */
> +		WARN_ON(request.response_len);
> +		/* Return data decoded from the status dword */
> +		err = INTEL_GUC_MSG_TO_DATA(*status);
> +	}
>   
> -	/* Use data from the GuC status as our return value */
> -	return INTEL_GUC_MSG_TO_DATA(*status);
> +unlink:
> +	spin_lock_irqsave(&ct->lock, flags);
> +	list_del(&request.link);
> +	spin_unlock_irqrestore(&ct->lock, flags);
> +
> +	return err;
>   }
>   
>   /*
> @@ -418,13 +499,15 @@ static int ctch_send(struct intel_guc *guc,
>   static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len,
>   			     u32 *response_buf, u32 response_buf_size)
>   {
> -	struct intel_guc_ct_channel *ctch = &guc->ct.host_channel;
> +	struct intel_guc_ct *ct = &guc->ct;
> +	struct intel_guc_ct_channel *ctch = &ct->host_channel;
>   	u32 status = ~0; /* undefined */
>   	int ret;
>   
>   	mutex_lock(&guc->send_mutex);
>   
> -	ret = ctch_send(guc, ctch, action, len, &status);
> +	ret = ctch_send(ct, ctch, action, len, response_buf, response_buf_size,
> +			&status);
>   	if (unlikely(ret < 0)) {
>   		DRM_ERROR("CT: send action %#X failed; err=%d status=%#X\n",
>   			  action[0], ret, status);
> @@ -503,8 +586,13 @@ static int ctb_read(struct intel_guc_ct_buffer *ctb, u32 *data)
>   static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>   {
>   	u32 header = msg[0];
> +	u32 fence = msg[1];
>   	u32 status = msg[2];
>   	u32 len = ct_header_get_len(header) + 1; /* total len with header */
> +	u32 payload_len = len - 3; /* len<3 is treated as protocol error */
> +	struct ct_request *req;
> +	bool found = false;
> +	unsigned long flags;
>   
>   	GEM_BUG_ON(!ct_header_is_response(header));
>   
> @@ -518,8 +606,29 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>   		DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
>   		return -EPROTO;
>   	}
> +	spin_lock_irqsave(&ct->lock, flags);
> +	list_for_each_entry(req, &ct->pending_requests, link) {
> +		if (req->fence != fence) {
> +			DRM_DEBUG_DRIVER("CT: request %u awaits response\n",
> +					 req->fence);
> +			continue;
> +		}
> +		if (unlikely(payload_len > req->response_len)) {
> +			DRM_ERROR("CT: response %u too long %*phn\n",
> +				  req->fence, 4*len, msg);
> +			payload_len = 0;
> +		}
> +		if (payload_len)
> +			memcpy(req->response_buf, msg + 3, 4*payload_len);
> +		req->response_len = payload_len;
> +		WRITE_ONCE(req->status, status);
> +		found = true;
> +		break;
> +	}
> +	spin_unlock_irqrestore(&ct->lock, flags);
>   
> -	/* XXX */
> +	if (!found)
> +		DRM_ERROR("CT: unsolicited response %*phn\n", 4*len, msg);
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
> index 595c8ad..905566b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.h
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.h
> @@ -71,10 +71,15 @@ struct intel_guc_ct_channel {
>   /** Holds all command transport channels.
>    *
>    * @host_channel: main channel used by the host
> + * @lock: spin lock for pending requests list
> + * @pending_requests: list of pending requests
>    */
>   struct intel_guc_ct {
>   	struct intel_guc_ct_channel host_channel;
>   	/* other channels are tbd */
> +
> +	spinlock_t lock;

I know you documented that this lock is for in the header, but the 
checkpatch.pl wants it in the line before the definition :|

And there are a couple of 4*len and 4*payload_len

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

> +	struct list_head pending_requests;
>   };
>   
>   void intel_guc_ct_init_early(struct intel_guc_ct *ct);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 10/13] drm/i915/guc: Enable GuC interrupts when using CT

[Michel Thierry]

On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
> We will need them in G2H communication to properly handle
> responses and requests from the Guc.
> 
> v2: keep irq enabled while disabling GuC logging (Oscar)
> v3: rebase.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Acked-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 8dc6a9c..9c20b1b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -481,7 +481,7 @@ int intel_uc_resume(struct drm_i915_private *i915)
>   	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>   		return 0;
>   
> -	if (i915_modparams.guc_log_level)
> +	if (HAS_GUC_CT(i915) || i915_modparams.guc_log_level)
>   		gen9_enable_guc_interrupts(i915);
>   
>   	err = intel_guc_resume(guc);
> 

This is no longer needed when this patch gets merged 
https://patchwork.freedesktop.org/patch/211397/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 09/13] drm/i915/guc: Prepare to process incoming requests from CT

[Michel Thierry]

On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
> Requests are read from CT in the irq handler, but actual processing
> will be done in the work thread. Processing of specific actions will
> be added in the upcoming patches.
> 
> v2: don't use GEM_BUG_ON (Chris)
>      don't kmalloc too large buffer (Michal)
> v3: rebased
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_ct.c | 72 ++++++++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/i915/intel_guc_ct.h |  4 +++
>   2 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
> index 6a9aad0..90aff51 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> @@ -32,10 +32,17 @@ struct ct_request {
>   	u32 *response_buf;
>   };
>   
> +struct ct_incoming_request {
> +	struct list_head link;
> +	u32 data[];
> +};
> +
>   enum { CTB_SEND = 0, CTB_RECV = 1 };
>   
>   enum { CTB_OWNER_HOST = 0 };
>   
> +static void ct_incoming_request_worker_func(struct work_struct *w);
> +
>   /**
>    * intel_guc_ct_init_early - Initialize CT state without requiring device access
>    * @ct: pointer to CT struct
> @@ -47,6 +54,8 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)
>   
>   	spin_lock_init(&ct->lock);
>   	INIT_LIST_HEAD(&ct->pending_requests);
> +	INIT_LIST_HEAD(&ct->incoming_requests);
> +	INIT_WORK(&ct->worker, ct_incoming_request_worker_func);
>   }
>   
>   static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct)
> @@ -632,13 +641,74 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>   	return 0;
>   }
>   
> +static void ct_dispatch_request(struct intel_guc_ct *ct,
> +				u32 action, u32 len, const u32 *payload)

Would ct_consume_request be better? It would be clear that this is for 
an incoming request.

To me 'dispatch' sounds like it will send something, while here the code 
will be handling the received g2h request.

With ct_dispatch_request, I would think this is at the same level as 
ct_handle_request.

Thanks,

> +{
> +	switch (action) {
> +	default:
> +		DRM_ERROR("CT: unexpected request %x %*phn\n",
> +			  action, 4*len, payload);
> +		break;
> +	}
> +}
> +
> +static bool ct_process_incoming_requests(struct intel_guc_ct *ct)
> +{
> +	unsigned long flags;
> +	struct ct_incoming_request *request;
> +	bool done;
> +
> +	spin_lock_irqsave(&ct->lock, flags);
> +	request = list_first_entry_or_null(&ct->incoming_requests,
> +					   struct ct_incoming_request, link);
> +	if (request)
> +		list_del(&request->link);
> +	done = !!list_empty(&ct->incoming_requests);
> +	spin_unlock_irqrestore(&ct->lock, flags);
> +
> +	if (!request)
> +		return true;
> +
> +	ct_dispatch_request(ct,
> +			    ct_header_get_action(request->data[0]),
> +			    ct_header_get_len(request->data[0]),
> +			    &request->data[1]);
> +
> +	kfree(request);
> +	return done;
> +}
> +
> +static void ct_incoming_request_worker_func(struct work_struct *w)
> +{
> +	struct intel_guc_ct *ct = container_of(w, struct intel_guc_ct, worker);
> +	bool done;
> +
> +	done = ct_process_incoming_requests(ct);
> +	if (!done)
> +		queue_work(system_unbound_wq, &ct->worker);
> +}
> +
>   static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg)
>   {
>   	u32 header = msg[0];
> +	u32 len = ct_header_get_len(header) + 1; /* total len with header */
> +	struct ct_incoming_request *request;
> +	unsigned long flags;
>   
>   	GEM_BUG_ON(ct_header_is_response(header));
>   
> -	/* XXX */
> +	request = kmalloc(sizeof(*request) + 4*len, GFP_ATOMIC);
> +	if (unlikely(!request)) {
> +		DRM_ERROR("CT: dropping request %*phn\n", 4*len, msg);
> +		return 0; /* XXX: -ENOMEM ? */
> +	}
> +	memcpy(request->data, msg, 4*len);
> +
> +	spin_lock_irqsave(&ct->lock, flags);
> +	list_add_tail(&request->link, &ct->incoming_requests);
> +	spin_unlock_irqrestore(&ct->lock, flags);
> +
> +	queue_work(system_unbound_wq, &ct->worker);
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
> index 905566b..0be4ce5 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.h
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.h
> @@ -73,6 +73,8 @@ struct intel_guc_ct_channel {
>    * @host_channel: main channel used by the host
>    * @lock: spin lock for pending requests list
>    * @pending_requests: list of pending requests
> + * @incoming_requests: list of incoming requests
> + * @worker: worker for handling incoming requests
>    */
>   struct intel_guc_ct {
>   	struct intel_guc_ct_channel host_channel;
> @@ -80,6 +82,8 @@ struct intel_guc_ct {
>   
>   	spinlock_t lock;
>   	struct list_head pending_requests;
> +	struct list_head incoming_requests;
> +	struct work_struct worker;
>   };
>   
>   void intel_guc_ct_init_early(struct intel_guc_ct *ct);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 11/13] drm/i915/guc: Handle default action received over CT

[Michel Thierry]

On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
> When running on platform with CTB based GuC communication enabled,
> GuC to Host event data will be delivered as CT request message.
> However, content of the data[1] of this CT message follows format
> of the scratch register used in MMIO based communication, so some
> code reuse is still possible.
> 

Spoiler alert, some g2h messages (reset-engine and preemption afaik) 
will send us more data, so just passing request->data[1] won't be enough 
¯\_(ツ)_/¯

> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> ---

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

>   drivers/gpu/drm/i915/intel_guc.c    | 5 +++++
>   drivers/gpu/drm/i915/intel_guc.h    | 1 +
>   drivers/gpu/drm/i915/intel_guc_ct.c | 9 +++++++++
>   3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 118db81..b6d2778 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -416,6 +416,11 @@ void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc)
>          I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
>          spin_unlock(&guc->irq_lock);
> 
> +       intel_guc_to_host_process_recv_msg(guc, msg);
> +}
> +
> +void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32 msg)
> +{
>          if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
>                     INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED))
>                  intel_guc_log_handle_flush_event(&guc->log);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 6dc109a..f1265e1 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -163,6 +163,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
>   void intel_guc_to_host_event_handler(struct intel_guc *guc);
>   void intel_guc_to_host_event_handler_nop(struct intel_guc *guc);
>   void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc);
> +void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32 msg);
>   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);
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c
> index 90aff51..9bc8738 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> @@ -644,8 +644,17 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>   static void ct_dispatch_request(struct intel_guc_ct *ct,
>                                  u32 action, u32 len, const u32 *payload)
>   {
> +       struct intel_guc *guc = ct_to_guc(ct);
> +
>          switch (action) {
> +       case INTEL_GUC_ACTION_DEFAULT:
> +               if (unlikely(len < 1))
> +                       goto fail_unexpected;
> +               intel_guc_to_host_process_recv_msg(guc, *payload);
> +               break;
> +
>          default:
> +fail_unexpected:
>                  DRM_ERROR("CT: unexpected request %x %*phn\n",
>                            action, 4*len, payload);
>                  break;
> --
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 01/13] drm/i915/guc: Add documentation for MMIO based communication

[Michal Wajdeczko]

On Fri, 23 Mar 2018 22:29:21 +0100, Michel Thierry  
<michel.thierry@intel.com> wrote:

> On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
>> As we are going to extend our use of MMIO based communication,
>> try to explain its mechanics and update corresponding definitions.
>>  Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Kelvin Gardiner <kelvin.gardiner@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_guc.c      | 20 ++++----
>>   drivers/gpu/drm/i915/intel_guc_ct.c   |  2 +-
>>   drivers/gpu/drm/i915/intel_guc_fwif.h | 86  
>> ++++++++++++++++++++++++++++-------
>>   3 files changed, 80 insertions(+), 28 deletions(-)
>>  diff --git a/drivers/gpu/drm/i915/intel_guc.c  
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 8f93f5b..28075e6 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -329,6 +329,9 @@ int intel_guc_send_mmio(struct intel_guc *guc,  
>> const u32 *action, u32 len)
>>          GEM_BUG_ON(!len);
>>          GEM_BUG_ON(len > guc->send_regs.count);
>>  +       /* We expect only action code */
>> +       GEM_BUG_ON(*action & ~INTEL_GUC_MSG_CODE_MASK);
>> +
>>          /* If CT is available, we expect to use MMIO only during  
>> init/fini */
>>          GEM_BUG_ON(HAS_GUC_CT(dev_priv) &&
>>                  *action !=  
>> INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&
>> @@ -350,18 +353,15 @@ int intel_guc_send_mmio(struct intel_guc *guc,  
>> const u32 *action, u32 len)
>>           */
>>          ret = __intel_wait_for_register_fw(dev_priv,
>>                                             guc_send_reg(guc, 0),
>> -                                          INTEL_GUC_RECV_MASK,
>> -                                          INTEL_GUC_RECV_MASK,
>> +                                          INTEL_GUC_MSG_TYPE_MASK,
>> +                                          INTEL_GUC_MSG_TYPE_RESPONSE  
>> <<
>> +                                          INTEL_GUC_MSG_TYPE_SHIFT,
>>                                             10, 10, &status);
>> -       if (status != INTEL_GUC_STATUS_SUCCESS) {
>> -               /*
>> -                * Either the GuC explicitly returned an error (which
>> -                * we convert to -EIO here) or no response at all was
>> -                * received within the timeout limit (-ETIMEDOUT)
>> -                */
>> -               if (ret != -ETIMEDOUT)
>> -                       ret = -EIO;
>> +       /* If GuC explicitly returned an error, convert it to -EIO */
>> +       if (!ret && !INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(status))
>> +               ret = -EIO;
>>  +       if (ret) {
>>                  DRM_DEBUG_DRIVER("INTEL_GUC_SEND: Action 0x%X failed;"
>>                                   " ret=%d status=0x%08X  
>> response=0x%08X\n",
>>                                   action[0], ret, status,
>> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c  
>> b/drivers/gpu/drm/i915/intel_guc_ct.c
>> index a726283..1dafa7a 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_ct.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
>> @@ -398,7 +398,7 @@ static int ctch_send(struct intel_guc *guc,
>>          err = wait_for_response(desc, fence, status);
>>          if (unlikely(err))
>>                  return err;
>> -       if (*status != INTEL_GUC_STATUS_SUCCESS)
>> +       if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status))
>>                  return -EIO;
>>          return 0;
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h  
>> b/drivers/gpu/drm/i915/intel_guc_fwif.h
>> index 72941bd..1701879 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
>> @@ -560,7 +560,68 @@ struct guc_shared_ctx_data {
>>          struct guc_ctx_report preempt_ctx_report[GUC_MAX_ENGINES_NUM];
>>   } __packed;
>>  -/* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */
>> +/**
>> + * 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
>> + *
>> + *      +-----------+---------+---------+---------+
>> + *      |  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* >  
>> + *
>
> Looks ok to me. Just one comment, I would have used 'cmd' or 'action'  
> instead of 'code' but that's personal preference (the archaic fw docs  
> use 'action code' for this field, is it why you decided to use code?).

'cmd' or 'action' is specific to REQUEST message, so it's not applicable
to other message types, like RESPONSE, where 'code' holds 'status'

imho 'code' name is quite universal ;)

Thanks,
/m

>
> Plus it isn't like we want to keep the same names, looking at  
> intel_guc_fwif.h vs the 'original', we've managed to change the name of  
> almost every single item.
>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>
>
>> + * 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 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,
>> @@ -597,24 +658,15 @@ enum intel_guc_report_status {
>>   #define GUC_LOG_CONTROL_VERBOSITY_MASK (0xF <<  
>> GUC_LOG_CONTROL_VERBOSITY_SHIFT)
>>   #define GUC_LOG_CONTROL_DEFAULT_LOGGING        (1 << 8)
>>  -/*
>> - * The GuC sends its response to a command by overwriting the
>> - * command in SS0. The response is distinguishable from a command
>> - * by the fact that all the MASK bits are set. The remaining bits
>> - * give more detail.
>> - */
>> -#define        INTEL_GUC_RECV_MASK     ((u32)0xF0000000)
>> -#define        INTEL_GUC_RECV_IS_RESPONSE(x)   ((u32)(x) >=  
>> INTEL_GUC_RECV_MASK)
>> -#define        INTEL_GUC_RECV_STATUS(x)        (INTEL_GUC_RECV_MASK |  
>> (x))
>> -
>> -/* GUC will return status back to SOFT_SCRATCH_O_REG */
>> -enum intel_guc_status {
>> -       INTEL_GUC_STATUS_SUCCESS = INTEL_GUC_RECV_STATUS(0x0),
>> -       INTEL_GUC_STATUS_ALLOCATE_DOORBELL_FAIL =  
>> INTEL_GUC_RECV_STATUS(0x10),
>> -       INTEL_GUC_STATUS_DEALLOCATE_DOORBELL_FAIL =  
>> INTEL_GUC_RECV_STATUS(0x20),
>> -       INTEL_GUC_STATUS_GENERIC_FAIL =  
>> INTEL_GUC_RECV_STATUS(0x0000F000)
>> +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) \
>> +       ((INTEL_GUC_MSG_TO_TYPE(m) == INTEL_GUC_MSG_TYPE_RESPONSE) && \
>> +        (INTEL_GUC_MSG_TO_CODE(m) ==  
>> INTEL_GUC_RESPONSE_STATUS_SUCCESS))
>> +
>>   /* This action will be programmed in C1BC - SOFT_SCRATCH_15_REG */
>>   enum intel_guc_recv_message {
>>          INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED = BIT(1),
>> --
>> 1.9.1
>>  _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 04/13] drm/i915/guc: Implement response handling in send_mmio()

[Michal Wajdeczko]

On Fri, 23 Mar 2018 22:55:09 +0100, Michel Thierry  
<michel.thierry@intel.com> wrote:

> On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
>> We're using data encoded in the status MMIO as return value from send
>> function, but GuC may also write more data in remaining MMIO regs.
>> Let's copy content of these registers to the buffer provided by caller.
>>  v2: new line (Michel)
>> v3: updated commit message
>>  Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Reviewed-by: Michel Thierry <michel.thierry@intel.com> #2
>> ---
>>   drivers/gpu/drm/i915/intel_guc.c | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>  diff --git a/drivers/gpu/drm/i915/intel_guc.c  
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index a533ff8..9ce01e5 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -368,11 +368,20 @@ int intel_guc_send_mmio(struct intel_guc *guc,  
>> const u32 *action, u32 len,
>>                                   " ret=%d status=0x%08X  
>> response=0x%08X\n",
>>                                   action[0], ret, status,
>>                                   I915_READ(SOFT_SCRATCH(15)));
>> -       } else {
>> -               /* Use data from the GuC response as our return value */
>> -               ret = INTEL_GUC_MSG_TO_DATA(status);
>> +               goto out;
>>          }
>>
>
> I'm not a big fan of goto's, so I would have added the response handling  
> in the else part.

me too, but there were too many indents and code was getting less readable
due to 80 column limit

/m

>
> But it's still correct, so my old r-b still stands.
>
> -Michel
>
>> +       if (response_buf) {
>> +               int count = min(response_buf_size, guc->send_regs.count  
>> - 1);
>> +
>> +               for (i = 0; i < count; i++)
>> +                       response_buf[i] = I915_READ(guc_send_reg(guc, i  
>> + 1));
>> +       }
>> +
>> +       /* Use data from the GuC response as our return value */
>> +       ret = INTEL_GUC_MSG_TO_DATA(status);
>> +
>> +out:
>>          intel_uncore_forcewake_put(dev_priv,  
>> guc->send_regs.fw_domains);
>>          mutex_unlock(&guc->send_mutex);
>>  --
>> 1.9.1
>>  _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 05/13] drm/i915/guc: Make event handler a virtual function

[Michal Wajdeczko]

On Fri, 23 Mar 2018 23:25:58 +0100, Michel Thierry  
<michel.thierry@intel.com> wrote:

> On 3/23/2018 7:47 AM, Michal Wajdeczko wrote:
>> On platforms with CTB based GuC communications, we will handle
>> GuC events in a different way. Let's make event handler a virtual
>> function to allow easy switch between those variants.
>>  Credits-to: Oscar Mateo <oscar.mateo@intel.com>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_guc.c |  8 +++++++-
>>   drivers/gpu/drm/i915/intel_guc.h | 10 ++++++++++
>>   drivers/gpu/drm/i915/intel_uc.c  |  2 ++
>>   3 files changed, 19 insertions(+), 1 deletion(-)
>>
>
> I've gotten used to 'receive', but 'handler' makes sense too.

Name of the function that handles irq events from GuC was
recently changed to intel_guc_to_host_event_handler() hence
'handler' as vptr name.

/m

>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>
>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 9ce01e5..118db81 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -69,6 +69,7 @@ void intel_guc_init_early(struct intel_guc *guc)
>>          mutex_init(&guc->send_mutex);
>>          spin_lock_init(&guc->irq_lock);
>>          guc->send = intel_guc_send_nop;
>> +       guc->handler = intel_guc_to_host_event_handler_nop;
>>          guc->notify = gen8_guc_raise_irq;
>>   }
>>  @@ -317,6 +318,11 @@ int intel_guc_send_nop(struct intel_guc *guc,  
>> const u32 *action, u32 len,
>>          return -ENODEV;
>>   }
>>  +void intel_guc_to_host_event_handler_nop(struct intel_guc *guc)
>> +{
>> +       WARN(1, "Unexpected event: no suitable handler\n");
>> +}
>> +
>>   /*
>>    * This function implements the MMIO based host to GuC interface.
>>    */
>> @@ -388,7 +394,7 @@ int intel_guc_send_mmio(struct intel_guc *guc,  
>> const u32 *action, u32 len,
>>          return ret;
>>   }
>>  -void intel_guc_to_host_event_handler(struct intel_guc *guc)
>> +void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc)
>>   {
>>          struct drm_i915_private *dev_priv = guc_to_i915(guc);
>>          u32 msg, val;
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index 7ee0732..6dc109a 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -91,6 +91,9 @@ struct intel_guc {
>>          int (*send)(struct intel_guc *guc, const u32 *data, u32 len,
>>                      u32 *response_buf, u32 response_buf_size);
>>  +       /* GuC's FW specific event handler function */
>> +       void (*handler)(struct intel_guc *guc);
>> +
>>          /* GuC's FW specific notify function */
>>          void (*notify)(struct intel_guc *guc);
>>   };
>> @@ -113,6 +116,11 @@ static inline void intel_guc_notify(struct  
>> intel_guc *guc)
>>          guc->notify(guc);
>>   }
>>  +static inline void intel_guc_to_host_event_handler(struct intel_guc  
>> *guc)
>> +{
>> +       guc->handler(guc);
>> +}
>> +
>>   /* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */
>>   #define GUC_GGTT_TOP   0xFEE00000
>>  @@ -153,6 +161,8 @@ int intel_guc_send_nop(struct intel_guc *guc,  
>> const u32 *action, u32 len,
>>   int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32  
>> len,
>>                          u32 *response_buf, u32 response_buf_size);
>>   void intel_guc_to_host_event_handler(struct intel_guc *guc);
>> +void intel_guc_to_host_event_handler_nop(struct intel_guc *guc);
>> +void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc);
>>   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);
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 34f8a2c..8dc6a9c 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -234,6 +234,7 @@ static int guc_enable_communication(struct  
>> intel_guc *guc)
>>                  return intel_guc_ct_enable(&guc->ct);
>>           guc->send = intel_guc_send_mmio;
>> +       guc->handler = intel_guc_to_host_event_handler_mmio;
>>          return 0;
>>   }
>>  @@ -247,6 +248,7 @@ static void guc_disable_communication(struct  
>> intel_guc *guc)
>>          gen9_disable_guc_interrupts(dev_priv);
>>           guc->send = intel_guc_send_nop;
>> +       guc->handler = intel_guc_to_host_event_handler_nop;
>>   }
>>    int intel_uc_init_misc(struct drm_i915_private *dev_priv)
>> --
>> 1.9.1
>>  _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 08/13] drm/i915/guc: Implement response handling in send_ct()

[Michał Winiarski]

On Fri, Mar 23, 2018 at 02:47:23PM +0000, Michal Wajdeczko wrote:
> Instead of returning small data in response status dword,
> GuC may append longer data as response message payload.
> If caller provides response buffer, we will copy received
> data and use number of received data dwords as new success
> return value. We will WARN if response from GuC does not
> match caller expectation.
> 
> v2: fix timeout and checkpatch warnings (Michal)
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_ct.c | 137 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_guc_ct.h |   5 ++
>  2 files changed, 128 insertions(+), 14 deletions(-)
> 

[SNIP]

> @@ -418,13 +499,15 @@ static int ctch_send(struct intel_guc *guc,
>  static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len,
>  			     u32 *response_buf, u32 response_buf_size)
>  {
> -	struct intel_guc_ct_channel *ctch = &guc->ct.host_channel;
> +	struct intel_guc_ct *ct = &guc->ct;
> +	struct intel_guc_ct_channel *ctch = &ct->host_channel;
>  	u32 status = ~0; /* undefined */
>  	int ret;
>  
>  	mutex_lock(&guc->send_mutex);
>  
> -	ret = ctch_send(guc, ctch, action, len, &status);
> +	ret = ctch_send(ct, ctch, action, len, response_buf, response_buf_size,
> +			&status);
>  	if (unlikely(ret < 0)) {
>  		DRM_ERROR("CT: send action %#X failed; err=%d status=%#X\n",
>  			  action[0], ret, status);
> @@ -503,8 +586,13 @@ static int ctb_read(struct intel_guc_ct_buffer *ctb, u32 *data)
>  static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>  {
>  	u32 header = msg[0];
> +	u32 fence = msg[1];
>  	u32 status = msg[2];
>  	u32 len = ct_header_get_len(header) + 1; /* total len with header */
> +	u32 payload_len = len - 3; /* len<3 is treated as protocol error */

Magic numbers, please ether define 3 as min payload length or hide this behind
macro.

> +	struct ct_request *req;
> +	bool found = false;
> +	unsigned long flags;
>  
>  	GEM_BUG_ON(!ct_header_is_response(header));
>  
> @@ -518,8 +606,29 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>  		DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
>  		return -EPROTO;
>  	}
> +	spin_lock_irqsave(&ct->lock, flags);

Isn't this called from the irq? We can use plain spin_lock here.

> +	list_for_each_entry(req, &ct->pending_requests, link) {
> +		if (req->fence != fence) {
> +			DRM_DEBUG_DRIVER("CT: request %u awaits response\n",
> +					 req->fence);
> +			continue;

Is this expected?
In other words - do we expect out of order responses?
Can we extract this into a helper (find request)?

-Michał

> +		}
> +		if (unlikely(payload_len > req->response_len)) {
> +			DRM_ERROR("CT: response %u too long %*phn\n",
> +				  req->fence, 4*len, msg);
> +			payload_len = 0;
> +		}
> +		if (payload_len)
> +			memcpy(req->response_buf, msg + 3, 4*payload_len);
> +		req->response_len = payload_len;
> +		WRITE_ONCE(req->status, status);
> +		found = true;
> +		break;
> +	}
> +	spin_unlock_irqrestore(&ct->lock, flags);
>  
> -	/* XXX */
> +	if (!found)
> +		DRM_ERROR("CT: unsolicited response %*phn\n", 4*len, msg);
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
> index 595c8ad..905566b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ct.h
> +++ b/drivers/gpu/drm/i915/intel_guc_ct.h
> @@ -71,10 +71,15 @@ struct intel_guc_ct_channel {
>  /** Holds all command transport channels.
>   *
>   * @host_channel: main channel used by the host
> + * @lock: spin lock for pending requests list
> + * @pending_requests: list of pending requests
>   */
>  struct intel_guc_ct {
>  	struct intel_guc_ct_channel host_channel;
>  	/* other channels are tbd */
> +
> +	spinlock_t lock;
> +	struct list_head pending_requests;
>  };
>  
>  void intel_guc_ct_init_early(struct intel_guc_ct *ct);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 08/13] drm/i915/guc: Implement response handling in send_ct()

[Jani Nikula]

On Mon, 26 Mar 2018, Michał Winiarski <michal.winiarski@intel.com> wrote:
> On Fri, Mar 23, 2018 at 02:47:23PM +0000, Michal Wajdeczko wrote:
>> Instead of returning small data in response status dword,
>> GuC may append longer data as response message payload.
>> If caller provides response buffer, we will copy received
>> data and use number of received data dwords as new success
>> return value. We will WARN if response from GuC does not
>> match caller expectation.
>> 
>> v2: fix timeout and checkpatch warnings (Michal)
>> 
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc_ct.c | 137 ++++++++++++++++++++++++++++++++----
>>  drivers/gpu/drm/i915/intel_guc_ct.h |   5 ++
>>  2 files changed, 128 insertions(+), 14 deletions(-)
>> 
>
> [SNIP]
>
>> @@ -418,13 +499,15 @@ static int ctch_send(struct intel_guc *guc,
>>  static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len,
>>  			     u32 *response_buf, u32 response_buf_size)
>>  {
>> -	struct intel_guc_ct_channel *ctch = &guc->ct.host_channel;
>> +	struct intel_guc_ct *ct = &guc->ct;
>> +	struct intel_guc_ct_channel *ctch = &ct->host_channel;
>>  	u32 status = ~0; /* undefined */
>>  	int ret;
>>  
>>  	mutex_lock(&guc->send_mutex);
>>  
>> -	ret = ctch_send(guc, ctch, action, len, &status);
>> +	ret = ctch_send(ct, ctch, action, len, response_buf, response_buf_size,
>> +			&status);
>>  	if (unlikely(ret < 0)) {
>>  		DRM_ERROR("CT: send action %#X failed; err=%d status=%#X\n",
>>  			  action[0], ret, status);
>> @@ -503,8 +586,13 @@ static int ctb_read(struct intel_guc_ct_buffer *ctb, u32 *data)
>>  static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>>  {
>>  	u32 header = msg[0];
>> +	u32 fence = msg[1];
>>  	u32 status = msg[2];
>>  	u32 len = ct_header_get_len(header) + 1; /* total len with header */
>> +	u32 payload_len = len - 3; /* len<3 is treated as protocol error */
>
> Magic numbers, please ether define 3 as min payload length or hide this behind
> macro.

And check len >= 3 before substracting 3.

BR,
Jani.


>
>> +	struct ct_request *req;
>> +	bool found = false;
>> +	unsigned long flags;
>>  
>>  	GEM_BUG_ON(!ct_header_is_response(header));
>>  
>> @@ -518,8 +606,29 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>>  		DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
>>  		return -EPROTO;
>>  	}
>> +	spin_lock_irqsave(&ct->lock, flags);
>
> Isn't this called from the irq? We can use plain spin_lock here.
>
>> +	list_for_each_entry(req, &ct->pending_requests, link) {
>> +		if (req->fence != fence) {
>> +			DRM_DEBUG_DRIVER("CT: request %u awaits response\n",
>> +					 req->fence);
>> +			continue;
>
> Is this expected?
> In other words - do we expect out of order responses?
> Can we extract this into a helper (find request)?
>
> -Michał
>
>> +		}
>> +		if (unlikely(payload_len > req->response_len)) {
>> +			DRM_ERROR("CT: response %u too long %*phn\n",
>> +				  req->fence, 4*len, msg);
>> +			payload_len = 0;
>> +		}
>> +		if (payload_len)
>> +			memcpy(req->response_buf, msg + 3, 4*payload_len);
>> +		req->response_len = payload_len;
>> +		WRITE_ONCE(req->status, status);
>> +		found = true;
>> +		break;
>> +	}
>> +	spin_unlock_irqrestore(&ct->lock, flags);
>>  
>> -	/* XXX */
>> +	if (!found)
>> +		DRM_ERROR("CT: unsolicited response %*phn\n", 4*len, msg);
>>  	return 0;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h
>> index 595c8ad..905566b 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_ct.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_ct.h
>> @@ -71,10 +71,15 @@ struct intel_guc_ct_channel {
>>  /** Holds all command transport channels.
>>   *
>>   * @host_channel: main channel used by the host
>> + * @lock: spin lock for pending requests list
>> + * @pending_requests: list of pending requests
>>   */
>>  struct intel_guc_ct {
>>  	struct intel_guc_ct_channel host_channel;
>>  	/* other channels are tbd */
>> +
>> +	spinlock_t lock;
>> +	struct list_head pending_requests;
>>  };
>>  
>>  void intel_guc_ct_init_early(struct intel_guc_ct *ct);
>> -- 
>> 1.9.1
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 08/13] drm/i915/guc: Implement response handling in send_ct()

[Michal Wajdeczko]

On Mon, 26 Mar 2018 17:29:32 +0200, Michał Winiarski  
<michal.winiarski@intel.com> wrote:

> On Fri, Mar 23, 2018 at 02:47:23PM +0000, Michal Wajdeczko wrote:
>> Instead of returning small data in response status dword,
>> GuC may append longer data as response message payload.
>> If caller provides response buffer, we will copy received
>> data and use number of received data dwords as new success
>> return value. We will WARN if response from GuC does not
>> match caller expectation.
>>
>> v2: fix timeout and checkpatch warnings (Michal)
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc_ct.c | 137  
>> ++++++++++++++++++++++++++++++++----
>>  drivers/gpu/drm/i915/intel_guc_ct.h |   5 ++
>>  2 files changed, 128 insertions(+), 14 deletions(-)
>>
>
> [SNIP]
>
>> @@ -418,13 +499,15 @@ static int ctch_send(struct intel_guc *guc,
>>  static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action,  
>> u32 len,
>>  			     u32 *response_buf, u32 response_buf_size)
>>  {
>> -	struct intel_guc_ct_channel *ctch = &guc->ct.host_channel;
>> +	struct intel_guc_ct *ct = &guc->ct;
>> +	struct intel_guc_ct_channel *ctch = &ct->host_channel;
>>  	u32 status = ~0; /* undefined */
>>  	int ret;
>>
>>  	mutex_lock(&guc->send_mutex);
>>
>> -	ret = ctch_send(guc, ctch, action, len, &status);
>> +	ret = ctch_send(ct, ctch, action, len, response_buf,  
>> response_buf_size,
>> +			&status);
>>  	if (unlikely(ret < 0)) {
>>  		DRM_ERROR("CT: send action %#X failed; err=%d status=%#X\n",
>>  			  action[0], ret, status);
>> @@ -503,8 +586,13 @@ static int ctb_read(struct intel_guc_ct_buffer  
>> *ctb, u32 *data)
>>  static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>>  {
>>  	u32 header = msg[0];
>> +	u32 fence = msg[1];
>>  	u32 status = msg[2];
>>  	u32 len = ct_header_get_len(header) + 1; /* total len with header */
>> +	u32 payload_len = len - 3; /* len<3 is treated as protocol error */
>
> Magic numbers, please ether define 3 as min payload length or hide this  
> behind
> macro.

Well, it looks that detailed CTB documentation can't wait any more...
(simplified doc was already there:

	/* Response message shall at least include header, fence and status */

I'll try to include more docs in next series spin to make these numbers  
more
clear for everyone

>
>> +	struct ct_request *req;
>> +	bool found = false;
>> +	unsigned long flags;
>>
>>  	GEM_BUG_ON(!ct_header_is_response(header));
>>
>> @@ -518,8 +606,29 @@ static int ct_handle_response(struct intel_guc_ct  
>> *ct, const u32 *msg)
>>  		DRM_ERROR("CT: corrupted response %*phn\n", 4*len, msg);
>>  		return -EPROTO;
>>  	}
>> +	spin_lock_irqsave(&ct->lock, flags);
>
> Isn't this called from the irq? We can use plain spin_lock here.

yes, will update, thanks

>
>> +	list_for_each_entry(req, &ct->pending_requests, link) {
>> +		if (req->fence != fence) {
>> +			DRM_DEBUG_DRIVER("CT: request %u awaits response\n",
>> +					 req->fence);
>> +			continue;
>
> Is this expected?

rather not, tempting to mark that with 'unlikely'

> In other words - do we expect out of order responses?

not today, since we're using send_mutex to serialize all requests
(serialization was required for MMIO based comm, but not for CT)

> Can we extract this into a helper (find request)?

will try, but can't promise

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

Re: [PATCH v4 08/13] drm/i915/guc: Implement response handling in send_ct()

[Michal Wajdeczko]

On Mon, 26 Mar 2018 17:35:00 +0200, Jani Nikula  
<jani.nikula@linux.intel.com> wrote:

> On Mon, 26 Mar 2018, Michał Winiarski <michal.winiarski@intel.com> wrote:
>> On Fri, Mar 23, 2018 at 02:47:23PM +0000, Michal Wajdeczko wrote:
>>> Instead of returning small data in response status dword,
>>> GuC may append longer data as response message payload.
>>> If caller provides response buffer, we will copy received
>>> data and use number of received data dwords as new success
>>> return value. We will WARN if response from GuC does not
>>> match caller expectation.
>>>
>>> v2: fix timeout and checkpatch warnings (Michal)
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>>> Cc: Michel Thierry <michel.thierry@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_guc_ct.c | 137  
>>> ++++++++++++++++++++++++++++++++----
>>>  drivers/gpu/drm/i915/intel_guc_ct.h |   5 ++
>>>  2 files changed, 128 insertions(+), 14 deletions(-)
>>>
>>
>> [SNIP]
>>
>>> @@ -418,13 +499,15 @@ static int ctch_send(struct intel_guc *guc,
>>>  static int intel_guc_send_ct(struct intel_guc *guc, const u32  
>>> *action, u32 len,
>>>  			     u32 *response_buf, u32 response_buf_size)
>>>  {
>>> -	struct intel_guc_ct_channel *ctch = &guc->ct.host_channel;
>>> +	struct intel_guc_ct *ct = &guc->ct;
>>> +	struct intel_guc_ct_channel *ctch = &ct->host_channel;
>>>  	u32 status = ~0; /* undefined */
>>>  	int ret;
>>>
>>>  	mutex_lock(&guc->send_mutex);
>>>
>>> -	ret = ctch_send(guc, ctch, action, len, &status);
>>> +	ret = ctch_send(ct, ctch, action, len, response_buf,  
>>> response_buf_size,
>>> +			&status);
>>>  	if (unlikely(ret < 0)) {
>>>  		DRM_ERROR("CT: send action %#X failed; err=%d status=%#X\n",
>>>  			  action[0], ret, status);
>>> @@ -503,8 +586,13 @@ static int ctb_read(struct intel_guc_ct_buffer  
>>> *ctb, u32 *data)
>>>  static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>>>  {
>>>  	u32 header = msg[0];
>>> +	u32 fence = msg[1];
>>>  	u32 status = msg[2];
>>>  	u32 len = ct_header_get_len(header) + 1; /* total len with header */
>>> +	u32 payload_len = len - 3; /* len<3 is treated as protocol error */
>>
>> Magic numbers, please ether define 3 as min payload length or hide this  
>> behind
>> macro.
>
> And check len >= 3 before substracting 3.
>

This was speculative, same as reading 'fence' and 'status', hence comment
about protocol error, but also note that all of them were used after proper
check for unlikely len<3 condition later in the function.

I will reorder existing code to avoid such tricks

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