diff mbox

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

Message ID 20180323144728.61548-2-michal.wajdeczko@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michal Wajdeczko March 23, 2018, 2:47 p.m. UTC
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(-)

Comments

Michel Thierry March 23, 2018, 9:29 p.m. UTC | #1
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
>
Michal Wajdeczko March 24, 2018, 7:09 a.m. UTC | #2
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
diff mbox

Patch

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