From: Michal Wajdeczko <michal.wajdeczko@intel.com>
To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>,
Matthew Brost <matthew.brost@intel.com>,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Cc: john.c.harrison@intel.com
Subject: Re: [PATCH 01/13] drm/i915/guc: Introduce unified HXG messages
Date: Tue, 8 Jun 2021 09:59:49 +0200 [thread overview]
Message-ID: <258111eb-ce42-f0c3-d74f-f79124114519@intel.com> (raw)
In-Reply-To: <2cf32a16-d2db-65e5-5004-d739eeae0d05@intel.com>
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@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@intel.com>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@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
>
next prev parent reply other threads:[~2021-06-08 7:59 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-07 18:03 [PATCH 00/13] Update firmware to v62.0.0 Matthew Brost
2021-06-07 18:03 ` [PATCH 01/13] drm/i915/guc: Introduce unified HXG messages Matthew Brost
2021-06-07 22:46 ` Daniele Ceraolo Spurio
2021-06-08 7:59 ` Michal Wajdeczko [this message]
2021-06-07 18:03 ` [PATCH 02/13] drm/i915/guc: Update MMIO based communication Matthew Brost
2021-06-07 23:06 ` Daniele Ceraolo Spurio
2021-06-08 8:15 ` Michal Wajdeczko
2021-06-09 1:03 ` Daniele Ceraolo Spurio
2021-06-07 18:03 ` [PATCH 03/13] drm/i915/guc: Update CTB response status definition Matthew Brost
2021-06-08 0:05 ` Daniele Ceraolo Spurio
2021-06-08 8:23 ` Michal Wajdeczko
2021-06-07 18:03 ` [PATCH 04/13] drm/i915/guc: Support per context scheduling policies Matthew Brost
2021-06-07 18:03 ` [PATCH 05/13] drm/i915/guc: Add flag for mark broken CTB Matthew Brost
2021-06-07 18:03 ` [PATCH 06/13] drm/i915/guc: New definition of the CTB descriptor Matthew Brost
2021-06-08 0:59 ` Daniele Ceraolo Spurio
2021-06-09 18:28 ` Michal Wajdeczko
2021-06-07 18:03 ` [PATCH 07/13] drm/i915/guc: New definition of the CTB registration action Matthew Brost
2021-06-08 1:23 ` Daniele Ceraolo Spurio
2021-06-09 17:36 ` John Harrison
2021-06-09 20:07 ` Michal Wajdeczko
2021-06-10 4:38 ` Matthew Brost
2021-06-10 13:19 ` Michal Wajdeczko
2021-06-11 18:43 ` Matthew Brost
2021-06-09 19:35 ` Michal Wajdeczko
2021-06-07 18:03 ` [PATCH 08/13] drm/i915/guc: New CTB based communication Matthew Brost
2021-06-08 2:20 ` Daniele Ceraolo Spurio
2021-06-10 4:01 ` Matthew Brost
2021-06-07 18:03 ` [PATCH 09/13] drm/i915/doc: Include GuC ABI documentation Matthew Brost
2021-06-07 17:45 ` [Intel-gfx] " Matthew Brost
2021-06-07 19:38 ` Michal Wajdeczko
2021-06-07 19:35 ` Matthew Brost
2021-06-07 18:03 ` [PATCH 10/13] drm/i915/guc: Kill guc_clients.ct_pool Matthew Brost
2021-06-07 17:57 ` Matthew Brost
2021-06-07 18:03 ` [PATCH 11/13] drm/i915/guc: Kill ads.client_info Matthew Brost
2021-06-07 18:03 ` [PATCH 12/13] drm/i915/guc: Unified GuC log Matthew Brost
2021-06-07 18:05 ` Matthew Brost
2021-06-07 18:03 ` [PATCH 13/13] drm/i915/guc: Update firmware to v62.0.0 Matthew Brost
2021-06-07 19:17 ` [Intel-gfx] " Matthew Brost
2021-06-07 22:19 ` [PATCH 00/13] " Daniele Ceraolo Spurio
2021-06-11 18:44 ` Matthew Brost
-- strict thread matches above, loose matches on Subject: below --
2021-06-10 4:36 Matthew Brost
2021-06-10 4:36 ` [PATCH 01/13] drm/i915/guc: Introduce unified HXG messages Matthew Brost
2021-06-14 17:04 ` Daniele Ceraolo Spurio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=258111eb-ce42-f0c3-d74f-f79124114519@intel.com \
--to=michal.wajdeczko@intel.com \
--cc=daniele.ceraolospurio@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=john.c.harrison@intel.com \
--cc=matthew.brost@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).