[Intel-gfx] [PATCH v4 01/13] drm/i915/guc: Add documentation for MMIO based communication
Michal Wajdeczko
michal.wajdeczko at intel.com
Sat Mar 24 07:09:24 UTC 2018
On Fri, 23 Mar 2018 22:29:21 +0100, Michel Thierry
<michel.thierry at 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 at intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: Kelvin Gardiner <kelvin.gardiner at 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 at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list