Re: [Intel-gfx] [PATCH v5 01/12] drm/i915/guc: Add documentation for MMIO based communication

2018-03-27 Thread Michal Wajdeczko 
On Tue, 27 Mar 2018 12:05:21 +0200, Sagar Arun Kamble  
 wrote:





On 3/27/2018 1:18 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.

v2: fix checkpatch MACRO_ARG_REUSE

Signed-off-by: Michal Wajdeczko 
Cc: Daniele Ceraolo Spurio 
Cc: Sagar Arun Kamble 
Cc: Kelvin Gardiner 
Reviewed-by: Michel Thierry  #1
---




  }
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h  
b/drivers/gpu/drm/i915/intel_guc_fwif.h

index 72941bd..83143e8 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

diagram below should be marked as reST literal block with "::"


Diagram below is in grid-table format [1], not literal block.

[1]  
http://docutils.sourceforge.net/docs/ref/rst/restructuredtext.html#grid-tables



+ *
+ *  +---+-+-+-+
+ *  |  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(0x << 
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)

*_MSG_IS_REQUEST isn't used. Do we plan to use this?


now it is added just for completeness, but yes, we plan to use it.

+#define INTEL_GUC_MSG_IS_RESPONSE(m)	__INTEL_GUC_MSG_TYPE_IS(RESPONSE,  
m)

+
  enum intel_guc_action {
r   INTEL_GUC_ACTION_DEFAULT = 0x0,
INTEL_GUC_ACTION_REQUEST_PREEMPTION = 0x2,
@@ -597,24 +658,17 @@ 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.
- */
-#defineINTEL_GUC_RECV_MASK ((u32)0xF000)
-#defineINTEL_GUC_RECV_IS_RESPONSE(x)   ((u32)(x) >= 
INTEL_GUC_RECV_MASK)
-#defineINTEL_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

Re: [Intel-gfx] [PATCH v5 01/12] drm/i915/guc: Add documentation for MMIO based communication

2018-03-27 Thread Sagar Arun Kamble 



On 3/27/2018 1:18 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.

v2: fix checkpatch MACRO_ARG_REUSE

Signed-off-by: Michal Wajdeczko 
Cc: Daniele Ceraolo Spurio 
Cc: Sagar Arun Kamble 
Cc: Kelvin Gardiner 
Reviewed-by: Michel Thierry  #1
---




  }
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h 
b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 72941bd..83143e8 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

diagram below should be marked as reST literal block with "::"

+ *
+ *  +---+-+-+-+
+ *  |  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(0x << 
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)

*_MSG_IS_REQUEST isn't used. Do we plan to use this?

+#define INTEL_GUC_MSG_IS_RESPONSE(m)   __INTEL_GUC_MSG_TYPE_IS(RESPONSE, m)
+
  enum intel_guc_action {
r   INTEL_GUC_ACTION_DEFAULT = 0x0,
INTEL_GUC_ACTION_REQUEST_PREEMPTION = 0x2,
@@ -597,24 +658,17 @@ 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.
- */
-#defineINTEL_GUC_RECV_MASK ((u32)0xF000)
-#defineINTEL_GUC_RECV_IS_RESPONSE(x)   ((u32)(x) >= 
INTEL_GUC_RECV_MASK)
-#defineINTEL_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(0xF000)
+enum intel_guc_response_status {
+

[Intel-gfx] [PATCH v5 01/12] drm/i915/guc: Add documentation for MMIO based communication

2018-03-26 Thread Michal Wajdeczko 
As we are going to extend our use of MMIO based communication,
try to explain its mechanics and update corresponding definitions.

v2: fix checkpatch MACRO_ARG_REUSE

Signed-off-by: Michal Wajdeczko 
Cc: Daniele Ceraolo Spurio 
Cc: Sagar Arun Kamble 
Cc: Kelvin Gardiner 
Reviewed-by: Michel Thierry  #1
---
 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 | 88 ---
 3 files changed, 82 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, );
-   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..83143e8 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