Re: [PATCH 01/13] drm/i915/guc: Introduce unified HXG messages

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
>> Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
>> ---
>>   .../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
> 



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux