[Intel-gfx] [PATCH 01/13] drm/i915/guc: Introduce unified HXG messages
Michal Wajdeczko
michal.wajdeczko at intel.com
Tue Jun 8 07:59:49 UTC 2021
On 08.06.2021 00:46, Daniele Ceraolo Spurio wrote:
>
>
> On 6/7/2021 11:03 AM, Matthew Brost wrote:
>> From: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>
>> New GuC firmware will unify format of MMIO and CTB H2G messages.
>> Introduce their definitions now to allow gradual transition of
>> our code to match new changes.
>>
>> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Michał Winiarski <michal.winiarski at intel.com>
>> ---
>> .../gpu/drm/i915/gt/uc/abi/guc_messages_abi.h | 213 ++++++++++++++++++
>> 1 file changed, 213 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h
>> b/drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h
>> index 775e21f3058c..29ac823acd4c 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_messages_abi.h
>> @@ -6,6 +6,219 @@
>> #ifndef _ABI_GUC_MESSAGES_ABI_H
>> #define _ABI_GUC_MESSAGES_ABI_H
>> +/**
>> + * DOC: HXG Message
>> + *
>> + * All messages exchanged with GuC are defined using 32 bit dwords.
>> + * First dword is treated as a message header. Remaining dwords are
>> optional.
>> + *
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + * | | Bits |
>> Description |
>> + *
>> +===+=======+==============================================================+
>>
>> + * | |
>> | |
>> + * | 0 | 31 | **ORIGIN** - originator of the
>> message |
>> + * | | | - _`GUC_HXG_ORIGIN_HOST` =
>> 0 |
>> + * | | | - _`GUC_HXG_ORIGIN_GUC` =
>> 1 |
>> + * | |
>> | |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 30:28 | **TYPE** - message
>> type |
>> + * | | | - _`GUC_HXG_TYPE_REQUEST` =
>> 0 |
>> + * | | | - _`GUC_HXG_TYPE_EVENT` =
>> 1 |
>> + * | | | - _`GUC_HXG_TYPE_NO_RESPONSE_BUSY` =
>> 3 |
>> + * | | | - _`GUC_HXG_TYPE_NO_RESPONSE_RETRY` =
>> 5 |
>> + * | | | - _`GUC_HXG_TYPE_RESPONSE_FAILURE` =
>> 6 |
>> + * | | | - _`GUC_HXG_TYPE_RESPONSE_SUCCESS` =
>> 7 |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 27:0 | **AUX** - auxiliary data (depends on
>> TYPE) |
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + * | 1 | 31:0
>> | |
>> + *
>> +---+-------+
>> |
>> + * |...| | **PAYLOAD** - optional payload (depends on
>> TYPE) |
>> + *
>> +---+-------+
>> |
>> + * | n | 31:0
>> | |
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + */
>> +
>> +#define GUC_HXG_MSG_MIN_LEN 1u
>> +#define GUC_HXG_MSG_0_ORIGIN (0x1 << 31)
>
> Any reason not to use BIT(31) here? same below with other bits and with
> GENMASK for masks.
initial goal was to have all ABI definitions auto-generated from GuC
spec files, using just pure C syntax to avoid any dependencies.
we can try to wrap some definitions into generic helpers like
HXG_MASK(...) and then remap them to our REG_GENMASK but didn't feel
this is super important
>
>> +#define GUC_HXG_ORIGIN_HOST 0u
>> +#define GUC_HXG_ORIGIN_GUC 1u
>> +#define GUC_HXG_MSG_0_TYPE (0x7 << 28)
>
> I think the masks could use a _MASK postfix
all field definitions are masks, so it would be redundant IMHO
note that previously there were both _MASK and _SHIFT definitions and
then it was required to have extra suffix
>
>> +#define GUC_HXG_TYPE_REQUEST 0u
>> +#define GUC_HXG_TYPE_EVENT 1u
>> +#define GUC_HXG_TYPE_NO_RESPONSE_BUSY 3u
>> +#define GUC_HXG_TYPE_NO_RESPONSE_RETRY 5u
>> +#define GUC_HXG_TYPE_RESPONSE_FAILURE 6u
>> +#define GUC_HXG_TYPE_RESPONSE_SUCCESS 7u
>> +#define GUC_HXG_MSG_0_AUX (0xfffffff << 0)
>> +#define GUC_HXG_MSG_n_PAYLOAD (0xffffffff << 0)
>
> Is a mask that covers the whole u32 really needed? Even for future
> proofing, I find it very unlikely that we'll ever have a case where the
> payload is not an entire dword.
maybe not strictly required but IIRC allows to have consistent
definitions for derived messages
>
>> +
>> +/**
>> + * DOC: HXG Request
>> + *
>> + * The `HXG Request`_ message should be used to initiate synchronous
>> activity
>> + * for which confirmation or return data is expected.
>> + *
>> + * The recipient of this message shall use `HXG Response`_, `HXG
>> Failure`_
>> + * or `HXG Retry`_ message as a definite reply, and may use `HXG Busy`_
>> + * message as a intermediate reply.
>> + *
>> + * Format of @DATA0 and all @DATAn fields depends on the @ACTION code.
>> + *
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + * | | Bits |
>> Description |
>> + *
>> +===+=======+==============================================================+
>>
>> + * | 0 | 31 |
>> ORIGIN |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 30:28 | TYPE =
>> GUC_HXG_TYPE_REQUEST_ |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 27:16 | **DATA0** - request data (depends on
>> ACTION) |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 15:0 | **ACTION** - requested action
>> code |
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + * | 1 | 31:0
>> | |
>> + *
>> +---+-------+
>> |
>> + * |...| | **DATAn** - optional data (depends on
>> ACTION) |
>> + *
>> +---+-------+
>> |
>> + * | n | 31:0
>> | |
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + */
>> +
>> +#define GUC_HXG_REQUEST_MSG_MIN_LEN GUC_HXG_MSG_MIN_LEN
>> +#define GUC_HXG_REQUEST_MSG_0_DATA0 (0xfff << 16)
>> +#define GUC_HXG_REQUEST_MSG_0_ACTION (0xffff << 0)
>> +#define GUC_HXG_REQUEST_MSG_n_DATAn GUC_HXG_MSG_n_PAYLOAD
>> +
>> +/**
>> + * DOC: HXG Event
>> + *
>> + * The `HXG Event`_ message should be used to initiate asynchronous
>> activity
>> + * that does not involves immediate confirmation nor data.
>> + *
>> + * Format of @DATA0 and all @DATAn fields depends on the @ACTION code.
>> + *
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + * | | Bits |
>> Description |
>> + *
>> +===+=======+==============================================================+
>>
>> + * | 0 | 31 |
>> ORIGIN |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 30:28 | TYPE =
>> GUC_HXG_TYPE_EVENT_ |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 27:16 | **DATA0** - event data (depends on
>> ACTION) |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 15:0 | **ACTION** - event action
>> code |
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + * | 1 | 31:0
>> | |
>> + *
>> +---+-------+
>> |
>> + * |...| | **DATAn** - optional event data (depends on
>> ACTION) |
>> + *
>> +---+-------+
>> |
>> + * | n | 31:0
>> | |
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + */
>> +
>> +#define GUC_HXG_EVENT_MSG_MIN_LEN GUC_HXG_MSG_MIN_LEN
>> +#define GUC_HXG_EVENT_MSG_0_DATA0 (0xfff << 16)
>> +#define GUC_HXG_EVENT_MSG_0_ACTION (0xffff << 0)
>> +#define GUC_HXG_EVENT_MSG_n_DATAn GUC_HXG_MSG_n_PAYLOAD
>> +
>> +/**
>> + * DOC: HXG Busy
>> + *
>> + * The `HXG Busy`_ message may be used to acknowledge reception of
>> the `HXG Request`_
>> + * message if the recipient expects that it processing will be longer
>> than default
>> + * timeout.
>> + *
>> + * The @COUNTER field may be used as a progress indicator.
>> + *
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + * | | Bits |
>> Description |
>> + *
>> +===+=======+==============================================================+
>>
>> + * | 0 | 31 |
>> ORIGIN |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 30:28 | TYPE =
>> GUC_HXG_TYPE_NO_RESPONSE_BUSY_ |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 27:0 | **COUNTER** - progress
>> indicator |
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + */
>> +
>> +#define GUC_HXG_BUSY_MSG_LEN GUC_HXG_MSG_MIN_LEN
>> +#define GUC_HXG_BUSY_MSG_0_COUNTER GUC_HXG_MSG_0_AUX
>> +
>> +/**
>> + * DOC: HXG Retry
>> + *
>> + * The `HXG Retry`_ message should be used by recipient to indicate
>> that the
>> + * `HXG Request`_ message was dropped and it should be resent again.
>> + *
>> + * The @REASON field may be used to provide additional information.
>> + *
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + * | | Bits |
>> Description |
>> + *
>> +===+=======+==============================================================+
>>
>> + * | 0 | 31 |
>> ORIGIN |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 30:28 | TYPE =
>> GUC_HXG_TYPE_NO_RESPONSE_RETRY_ |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 27:0 | **REASON** - reason for
>> retry |
>> + * | | | - _`GUC_HXG_RETRY_REASON_UNSPECIFIED` =
>> 0 |
>
> AFAICS in the specs for 62.0.0 this field is actually a MBZ. Where does
> the "reason" classification come from?
some spec revision had these bits defined as "MBZ or debug data" and
this debug data was understood as "REASON", in same fashion as "HINT" in
FAILURE_RESPONSE message.
note that UNSPECIFIED(0) still matches MBZ(0)
>
> Apart from this, all the defines match the specs.
>
> Daniele
>
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + */
>> +
>> +#define GUC_HXG_RETRY_MSG_LEN GUC_HXG_MSG_MIN_LEN
>> +#define GUC_HXG_RETRY_MSG_0_REASON GUC_HXG_MSG_0_AUX
>> +#define GUC_HXG_RETRY_REASON_UNSPECIFIED 0u
>> +
>> +/**
>> + * DOC: HXG Failure
>> + *
>> + * The `HXG Failure`_ message shall be used as a reply to the `HXG
>> Request`_
>> + * message that could not be processed due to an error.
>> + *
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + * | | Bits |
>> Description |
>> + *
>> +===+=======+==============================================================+
>>
>> + * | 0 | 31 |
>> ORIGIN |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 30:28 | TYPE =
>> GUC_HXG_TYPE_RESPONSE_FAILURE_ |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 27:16 | **HINT** - additional error
>> hint |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 15:0 | **ERROR** - error/result
>> code |
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + */
>> +
>> +#define GUC_HXG_FAILURE_MSG_LEN GUC_HXG_MSG_MIN_LEN
>> +#define GUC_HXG_FAILURE_MSG_0_HINT (0xfff << 16)
>> +#define GUC_HXG_FAILURE_MSG_0_ERROR (0xffff << 0)
>> +
>> +/**
>> + * DOC: HXG Response
>> + *
>> + * The `HXG Response`_ message shall be used as a reply to the `HXG
>> Request`_
>> + * message that was successfully processed without an error.
>> + *
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + * | | Bits |
>> Description |
>> + *
>> +===+=======+==============================================================+
>>
>> + * | 0 | 31 |
>> ORIGIN |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 30:28 | TYPE =
>> GUC_HXG_TYPE_RESPONSE_SUCCESS_ |
>> + * |
>> +-------+--------------------------------------------------------------+
>> + * | | 27:0 | **DATA0** - data (depends on ACTION from `HXG
>> Request`_) |
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + * | 1 | 31:0
>> | |
>> + *
>> +---+-------+
>> |
>> + * |...| | **DATAn** - data (depends on ACTION from `HXG
>> Request`_) |
>> + *
>> +---+-------+
>> |
>> + * | n | 31:0
>> | |
>> + *
>> +---+-------+--------------------------------------------------------------+
>>
>> + */
>> +
>> +#define GUC_HXG_RESPONSE_MSG_MIN_LEN GUC_HXG_MSG_MIN_LEN
>> +#define GUC_HXG_RESPONSE_MSG_0_DATA0 GUC_HXG_MSG_0_AUX
>> +#define GUC_HXG_RESPONSE_MSG_n_DATAn GUC_HXG_MSG_n_PAYLOAD
>> +
>> +/* deprecated */
>> #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
>
More information about the Intel-gfx
mailing list