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

On 6/7/2021 11:03 AM, Matthew Brost wrote:

From: Michal Wajdeczko michal.wajdeczko@intel.com

The MMIO based Host-to-GuC communication protocol has been updated to use unified HXG messages.

Update our intel_guc_send_mmio() function by correctly handle BUSY, RETRY and FAILURE replies. Also update our documentation.

GuC: 55.0.0 Signed-off-by: Matthew Brost matthew.brost@intel.com Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: Piotr Piórkowski piotr.piorkowski@intel.com Cc: Michal Winiarski michal.winiarski@intel.com #v3

---

.../gt/uc/abi/guc_communication_mmio_abi.h | 63 ++++++------- drivers/gpu/drm/i915/gt/uc/intel_guc.c | 92 ++++++++++++++----- 2 files changed, 97 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h index be066a62e9e0..3f9039e3ef9d 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_mmio_abi.h @@ -7,46 +7,43 @@ #define _ABI_GUC_COMMUNICATION_MMIO_ABI_H

/**

• • DOC: MMIO based communication

• • DOC: GuC 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.

• • The MMIO based communication between Host and GuC relies on special
• • hardware registers which format could be defined by the software
• • (so called scratch registers).

• • For Gen9+, GuC uses software scratch registers 0xC180-0xC1B8,
• • but no H2G command takes more than 8 parameters and the GuC FW
• • itself uses an 8-element array to store the H2G message.

• •  +-----------+---------+---------+---------+
    

• •  |  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**

• • Each MMIO based message, both Host to GuC (H2G) and GuC to Host (G2H)
• • messages, which maximum length depends on number of available scratch
• • registers, is directly written into those scratch registers.

• • The following message **types** are supported:

• • For Gen9+, there are 16 software scratch registers 0xC180-0xC1B8,
• • but no H2G command takes more than 8 parameters and the GuC firmware
• • itself uses an 8-element array to store the H2G message.

Is this statement still true? I believe no MMIO H2G is over 4 DWs (given the limitation of the new gen11+ scratch regs), while CTB messages can be longer than 8 DWs.

• • • **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.

• • For Gen11+, there are additional 4 registers 0x190240-0x19024C, which
• • are, regardless on lower count, preffered over legacy ones.

typo: preffered -> preferred

• • • **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.

• • The MMIO based communication is mainly used during driver initialization
• • phase to setup the `CTB based communication`_ that will be used afterwards.

  */

  #define GUC_MAX_MMIO_MSG_LEN 8

See comment above. Reduce this to 4?

+/**

• • DOC: MMIO HXG Message
• • Format of the MMIO messages follows definitions of `HXG Message`_.
• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31:0 | +--------------------------------------------------------+ |
• • +---+-------+ | | |
• • |...| | | Embedded `HXG Message`_ | |
• • +---+-------+ | | |
• • | n | 31:0 | +--------------------------------------------------------+ |
• • +---+-------+--------------------------------------------------------------+
• */
• #endif /* _ABI_GUC_COMMUNICATION_MMIO_ABI_H */

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index f147cb389a20..b773567cb080 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -376,29 +376,27 @@ void intel_guc_fini(struct intel_guc *guc) /*

• This function implements the MMIO based host to GuC interface.

*/ -int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len, +int intel_guc_send_mmio(struct intel_guc *guc, const u32 *request, u32 len, u32 *response_buf, u32 response_buf_size) {

• struct drm_i915_private *i915 = guc_to_gt(guc)->i915; struct intel_uncore *uncore = guc_to_gt(guc)->uncore;

• u32 status;

• u32 header; int i; int ret;

  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(*action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER &&

•    *action != INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER);
  

• GEM_BUG_ON(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, request[0]) != GUC_HXG_ORIGIN_HOST);

• GEM_BUG_ON(FIELD_GET(GUC_HXG_MSG_0_TYPE, request[0]) != GUC_HXG_TYPE_REQUEST);

  mutex_lock(&guc->send_mutex); intel_uncore_forcewake_get(uncore, guc->send_regs.fw_domains);

+retry: for (i = 0; i < len; i++)

• intel_uncore_write(uncore, guc_send_reg(guc, i), action[i]);
  

• intel_uncore_write(uncore, guc_send_reg(guc, i), request[i]);
  
  

  intel_uncore_posting_read(uncore, guc_send_reg(guc, i - 1));

@@ -410,30 +408,74 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len, */ ret = __intel_wait_for_register_fw(uncore, guc_send_reg(guc, 0),

• 			   INTEL_GUC_MSG_TYPE_MASK,
  

• 			   INTEL_GUC_MSG_TYPE_RESPONSE <<
  

• 			   INTEL_GUC_MSG_TYPE_SHIFT,
  

• 			   10, 10, &status);
  

• /* If GuC explicitly returned an error, convert it to -EIO */
• if (!ret && !INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(status))

• ret = -EIO;
  

• 			   GUC_HXG_MSG_0_ORIGIN,
  

• 			   FIELD_PREP(GUC_HXG_MSG_0_ORIGIN,
  

• 				      GUC_HXG_ORIGIN_GUC),
  

• 			   10, 10, &header);
  

• if (unlikely(ret)) {

+timeout:

• drm_err(&i915->drm, "mmio request %#x: no reply %x\n",
  

• 	request[0], header);
  

• goto out;
  

• }

• if (ret) {

• DRM_ERROR("MMIO: GuC action %#x failed with error %d %#x\n",
  

• 	  action[0], ret, status);
  

• if (FIELD_GET(GUC_HXG_MSG_0_TYPE, header) == GUC_HXG_TYPE_NO_RESPONSE_BUSY) {

+#define done ({ header = intel_uncore_read(uncore, guc_send_reg(guc, 0)); \

• FIELD_GET(GUC_HXG_MSG_0_ORIGIN, header) != GUC_HXG_ORIGIN_GUC || \
  

• FIELD_GET(GUC_HXG_MSG_0_TYPE, header) != GUC_HXG_TYPE_NO_RESPONSE_BUSY; })
  

• ret = wait_for(done, 1000);
  

• if (unlikely(ret))
  

• 	goto timeout;
  

• if (unlikely(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, header) !=
  

• 		       GUC_HXG_ORIGIN_GUC))
  

• 	goto proto;
  

+#undef done

• }
• if (FIELD_GET(GUC_HXG_MSG_0_TYPE, header) == GUC_HXG_TYPE_NO_RESPONSE_RETRY) {

• u32 reason = FIELD_GET(GUC_HXG_RETRY_MSG_0_REASON, header);
  

• drm_dbg(&i915->drm, "mmio request %#x: retrying, reason %u\n",
  

• 	request[0], reason);
  

• goto retry;
  

• }
• if (FIELD_GET(GUC_HXG_MSG_0_TYPE, header) == GUC_HXG_TYPE_RESPONSE_FAILURE) {

• u32 hint = FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, header);
  

• u32 error = FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, header);
  

• drm_err(&i915->drm, "mmio request %#x: failure %x/%u\n",
  

• 	request[0], error, hint);
  

• ret = -ENXIO;
  

• goto out;
  

• }
• if (FIELD_GET(GUC_HXG_MSG_0_TYPE, header) != GUC_HXG_TYPE_RESPONSE_SUCCESS) {

+proto:

• drm_err(&i915->drm, "mmio request %#x: unexpected reply %#x\n",
  

• 	request[0], header);
  

• ret = -EPROTO;
  

  goto out; }

  if (response_buf) {

• int count = min(response_buf_size, guc->send_regs.count - 1);
  

• int count = min(response_buf_size, guc->send_regs.count);
  
  

• for (i = 0; i < count; i++)
  

• GEM_BUG_ON(!count);
  

• response_buf[0] = header;
  

• for (i = 1; i < count; i++)
  response_buf[i] = intel_uncore_read(uncore,
  

• 					    guc_send_reg(guc, i + 1));
  

• }

• 					    guc_send_reg(guc, i));
  

This could use a note in the commit message to remark that we have no users for the returned data yet and therefore nothing will break if we change what we return through it.

Apart from the nits, the logic looks good to me. Daniele

• /* Use data from the GuC response as our return value */
• ret = INTEL_GUC_MSG_TO_DATA(status);

• /* Use number of copied dwords as our return value */
  

• ret = count;
  

• } else {

• /* Use data from the GuC response as our return value */
  

• ret = FIELD_GET(GUC_HXG_RESPONSE_MSG_0_DATA0, header);
  

• }

  out: intel_uncore_forcewake_put(uncore, guc->send_regs.fw_domains);

<snip>

...

...

#endif /* _ABI_GUC_COMMUNICATION_MMIO_ABI_H */ diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index f147cb389a20..b773567cb080 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -376,29 +376,27 @@ void intel_guc_fini(struct intel_guc *guc)   /*    * This function implements the MMIO based host to GuC interface.    */ -int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len, +int intel_guc_send_mmio(struct intel_guc *guc, const u32 *request, u32 len,               u32 *response_buf, u32 response_buf_size)   { +    struct drm_i915_private *i915 = guc_to_gt(guc)->i915;       struct intel_uncore *uncore = guc_to_gt(guc)->uncore; -    u32 status; +    u32 header;       int i;       int ret;         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(*action != INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER && -           *action != INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER); +    GEM_BUG_ON(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, request[0]) != GUC_HXG_ORIGIN_HOST); +    GEM_BUG_ON(FIELD_GET(GUC_HXG_MSG_0_TYPE, request[0]) != GUC_HXG_TYPE_REQUEST);         mutex_lock(&guc->send_mutex);       intel_uncore_forcewake_get(uncore, guc->send_regs.fw_domains);   +retry:       for (i = 0; i < len; i++) -        intel_uncore_write(uncore, guc_send_reg(guc, i), action[i]); +        intel_uncore_write(uncore, guc_send_reg(guc, i), request[i]);         intel_uncore_posting_read(uncore, guc_send_reg(guc, i - 1));   @@ -410,30 +408,74 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,        */       ret = __intel_wait_for_register_fw(uncore,                          guc_send_reg(guc, 0), -                       INTEL_GUC_MSG_TYPE_MASK, -                       INTEL_GUC_MSG_TYPE_RESPONSE << -                       INTEL_GUC_MSG_TYPE_SHIFT, -                       10, 10, &status); -    /* If GuC explicitly returned an error, convert it to -EIO */ -    if (!ret && !INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(status)) -        ret = -EIO; +                       GUC_HXG_MSG_0_ORIGIN, +                       FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, +                              GUC_HXG_ORIGIN_GUC), +                       10, 10, &header); +    if (unlikely(ret)) { +timeout: +        drm_err(&i915->drm, "mmio request %#x: no reply %x\n", +            request[0], header); +        goto out; +    }   -    if (ret) { -        DRM_ERROR("MMIO: GuC action %#x failed with error %d %#x\n", -              action[0], ret, status); +    if (FIELD_GET(GUC_HXG_MSG_0_TYPE, header) == GUC_HXG_TYPE_NO_RESPONSE_BUSY) { +#define done ({ header = intel_uncore_read(uncore, guc_send_reg(guc, 0)); \ +        FIELD_GET(GUC_HXG_MSG_0_ORIGIN, header) != GUC_HXG_ORIGIN_GUC || \ +        FIELD_GET(GUC_HXG_MSG_0_TYPE, header) != GUC_HXG_TYPE_NO_RESPONSE_BUSY; })

+        ret = wait_for(done, 1000); +        if (unlikely(ret)) +            goto timeout; +        if (unlikely(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, header) != +                       GUC_HXG_ORIGIN_GUC)) +            goto proto; +#undef done +    }

+    if (FIELD_GET(GUC_HXG_MSG_0_TYPE, header) == GUC_HXG_TYPE_NO_RESPONSE_RETRY) { +        u32 reason = FIELD_GET(GUC_HXG_RETRY_MSG_0_REASON, header);

+        drm_dbg(&i915->drm, "mmio request %#x: retrying, reason %u\n", +            request[0], reason); +        goto retry; +    }

+    if (FIELD_GET(GUC_HXG_MSG_0_TYPE, header) == GUC_HXG_TYPE_RESPONSE_FAILURE) { +        u32 hint = FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, header); +        u32 error = FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, header);

+        drm_err(&i915->drm, "mmio request %#x: failure %x/%u\n", +            request[0], error, hint); +        ret = -ENXIO; +        goto out; +    }

+    if (FIELD_GET(GUC_HXG_MSG_0_TYPE, header) != GUC_HXG_TYPE_RESPONSE_SUCCESS) { +proto: +        drm_err(&i915->drm, "mmio request %#x: unexpected reply %#x\n", +            request[0], header); +        ret = -EPROTO;           goto out;       }         if (response_buf) { -        int count = min(response_buf_size, guc->send_regs.count - 1); +        int count = min(response_buf_size, guc->send_regs.count);   -        for (i = 0; i < count; i++) +        GEM_BUG_ON(!count);

+        response_buf[0] = header;

+        for (i = 1; i < count; i++)               response_buf[i] = intel_uncore_read(uncore, -                                guc_send_reg(guc, i + 1)); -    } +                                guc_send_reg(guc, i));

This could use a note in the commit message to remark that we have no users for the returned data yet and therefore nothing will break if we change what we return through it.

I hope this will do the work:

"Since some of the new MMIO actions may use DATA0 from MMIO HXG response, we must update intel_guc_send_mmio() to copy full response, including HXG header. There will be no impact to existing users as all of them are only relying just on return code."

Yes it does.

Daniele

...

Apart from the nits, the logic looks good to me. Daniele

...

-    /* Use data from the GuC response as our return value */ -    ret = INTEL_GUC_MSG_TO_DATA(status); +        /* Use number of copied dwords as our return value */ +        ret = count; +    } else { +        /* Use data from the GuC response as our return value */ +        ret = FIELD_GET(GUC_HXG_RESPONSE_MSG_0_DATA0, header); +    }     out:       intel_uncore_forcewake_put(uncore, guc->send_regs.fw_domains);

On 6/7/2021 11:03 AM, Matthew Brost wrote:

From: Michal Wajdeczko michal.wajdeczko@intel.com

Format of the STATUS dword in CTB response message now follows definition of the HXG header. Update our code and remove any obsolete legacy definitions.

This is kind of hard to review on its own against the specs, because there are larger changes to the CTB flows which AFAICS are part of patch 8. If what you wanted to do here was a simple replacement of defines to keep the later patch simpler, then, considering all patches are going to be squashed anyway:

Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com

One suggestion below.

GuC: 55.0.0 Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Acked-by: Piotr Piórkowski piotr.piorkowski@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 14 ++++++++------ drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 17 ----------------- 2 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 8f7b148fef58..3f7f48611487 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -477,7 +477,9 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) * up to that length of time, then switch to a slower sleep-wait loop. * No GuC command should ever take longer than 10ms. */ -#define done INTEL_GUC_MSG_IS_RESPONSE(READ_ONCE(req->status)) +#define done \

• (FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \

• GUC_HXG_ORIGIN_GUC)
  

  err = wait_for_us(done, 10); if (err) err = wait_for(done, 10);

@@ -532,21 +534,21 @@ static int ct_send(struct intel_guc_ct *ct, if (unlikely(err)) goto unlink;

• if (!INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(*status)) {

• if (FIELD_GET(GUC_HXG_MSG_0_TYPE, *status) != GUC_HXG_TYPE_RESPONSE_SUCCESS) { err = -EIO; goto unlink; }

  if (response_buf) { /* There shall be no data in the status */

• WARN_ON(INTEL_GUC_MSG_TO_DATA(request.status));
  

• WARN_ON(FIELD_GET(GUC_HXG_RESPONSE_MSG_0_DATA0, request.status));
  

  /* Return actual response len */ err = request.response_len; } else { /* There shall be no response payload */ WARN_ON(request.response_len); /* Return data decoded from the status dword */

• err = INTEL_GUC_MSG_TO_DATA(*status);
  

• err = FIELD_GET(GUC_HXG_RESPONSE_MSG_0_DATA0, *status);
  

Given that the same FIELD_GET() are repeated multiple times, IMO we could've kept some helper macros, something like:

INTEL_GUC_HXG_RESPONSE_TO_DATA(hxg) \ FIELD_GET(GUC_HXG_RESPONSE_MSG_0_DATA0, hxg)

INTEL_GUC_HXG_ORIGIN_IS_GUC(hxg) \ (FIELD_GET(GUC_HXG_MSG_0_ORIGIN, hxg) == GUC_HXG_ORIGIN_GUC)

INTEL_GUC_HXG_TYPE(hxg) \ FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg)

Which could be useful in the mmio code as well. Not sure how this changes in patch 8 though, I might put some more comments on that patch.

Daniele

}

unlink: @@ -741,8 +743,8 @@ static int ct_handle_response(struct intel_guc_ct *ct, struct ct_incoming_msg *r status = response->msg[2]; datalen = len - 2;

• /* Format of the status follows RESPONSE message */
• if (unlikely(!INTEL_GUC_MSG_IS_RESPONSE(status))) {

• /* Format of the status dword follows HXG header */
• if (unlikely(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, status) != GUC_HXG_ORIGIN_GUC)) { CT_ERROR(ct, "Corrupted response (status %#x)\n", status); return -EPROTO; }

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h index e9a9d85e2aa3..fb04e2211b79 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h @@ -414,23 +414,6 @@ struct guc_shared_ctx_data { struct guc_ctx_report preempt_ctx_report[GUC_MAX_ENGINES_NUM]; } __packed;

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

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

-#define INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(m) \

• (typecheck(u32, (m)) && \
  

•  ((m) & (INTEL_GUC_MSG_TYPE_MASK | INTEL_GUC_MSG_CODE_MASK)) == \
  

•  ((INTEL_GUC_MSG_TYPE_RESPONSE << INTEL_GUC_MSG_TYPE_SHIFT) | \
  

•   (INTEL_GUC_RESPONSE_STATUS_SUCCESS << INTEL_GUC_MSG_CODE_SHIFT)))
  

• /* 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),

On 08.06.2021 02:59, Daniele Ceraolo Spurio wrote:

On 6/7/2021 11:03 AM, Matthew Brost wrote:

...

From: Michal Wajdeczko michal.wajdeczko@intel.com

Definition of the CTB descriptor has changed, leaving only minimal shared fields like HEAD/TAIL/STATUS.

Both HEAD and TAIL are now in dwords.

Add some ABI documentation and implement required changes.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

.../gt/uc/abi/guc_communication_ctb_abi.h     | 70 ++++++++++++++-----   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c     | 70 +++++++++----------   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h     |  2 +-   3 files changed, 85 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h index d38935f47ecf..c2a069a78e01 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h @@ -7,6 +7,58 @@   #define _ABI_GUC_COMMUNICATION_CTB_ABI_H     #include <linux/types.h> +#include <linux/build_bug.h>

+#include "guc_messages_abi.h"

+/**

• • DOC: CT Buffer
• • TBD

What's the plan with this TBD here?

Plan was to add some updated text based on old "DOC: CTB based communication" section

...

• */

+/**

• • DOC: CTB Descriptor
• *

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

• *

+===+=======+==============================================================+

• *  | 0 |  31:0 | **HEAD** - offset (in dwords) to the last dword

that was     |

• *  |   |       | read from the `CT

Buffer`_.                                  |

• *  |   |       | It can only be updated by the

receiver.                      |

• *

+---+-------+--------------------------------------------------------------+

• *  | 1 |  31:0 | **TAIL** - offset (in dwords) to the last dword

that was     |

• *  |   |       | written to the `CT

Buffer`_.                                 |

• *  |   |       | It can only be updated by the

sender.                        |

• *

+---+-------+--------------------------------------------------------------+

• *  | 2 |  31:0 | **STATUS** - status of the

CTB                               |

• *  |   |

|                                                              |

• *  |   |       |   - _`GUC_CTB_STATUS_NO_ERROR` = 0 (normal

operation)        |

• *  |   |       |   - _`GUC_CTB_STATUS_OVERFLOW` = 1 (head/tail too

large)     |

• *  |   |       |   - _`GUC_CTB_STATUS_UNDERFLOW` = 2 (truncated

message)      |

• *  |   |       |   - _`GUC_CTB_STATUS_MISMATCH` = 4 (head/tail

modified)      |

• *  |   |       |   - _`GUC_CTB_STATUS_NO_BACKCHANNEL` =

8                     |

• *  |   |       |   - _`GUC_CTB_STATUS_MALFORMED_MSG` =

16                     |

I don't see the last 2 error (8 & 16) in the 62.0.0 specs. Where is the reference for them?

both were discussed on various meetings but likely didn't make into final spec 62, so for now we can drop them both

...

• *

+---+-------+--------------------------------------------------------------+

• *  |...|       | RESERVED =

MBZ                                               |

• *

+---+-------+--------------------------------------------------------------+

• *  | 15|  31:0 | RESERVED =

MBZ                                               |

• *

+---+-------+--------------------------------------------------------------+

• */

+struct guc_ct_buffer_desc { +    u32 head; +    u32 tail; +    u32 status; +#define GUC_CTB_STATUS_NO_ERROR                0 +#define GUC_CTB_STATUS_OVERFLOW                (1 << 0) +#define GUC_CTB_STATUS_UNDERFLOW            (1 << 1) +#define GUC_CTB_STATUS_MISMATCH                (1 << 2) +#define GUC_CTB_STATUS_NO_BACKCHANNEL            (1 << 3) +#define GUC_CTB_STATUS_MALFORMED_MSG            (1 << 4)

use BIT() ?

as explained before, on ABI headers we didn't want any dependency and just use plain C

...

+    u32 reserved[13]; +} __packed; +static_assert(sizeof(struct guc_ct_buffer_desc) == 64);     /**    * DOC: CTB based communication @@ -60,24 +112,6 @@    * - **flags**, holds various bits to control message handling    */   -/*

• • Describes single command transport buffer.
• • Used by both guc-master and clients.
• */

-struct guc_ct_buffer_desc { -    u32 addr;        /* gfx address */ -    u64 host_private;    /* host private data */ -    u32 size;        /* size in bytes */ -    u32 head;        /* offset updated by GuC*/ -    u32 tail;        /* offset updated by owner */ -    u32 is_in_error;    /* error indicator */ -    u32 reserved1; -    u32 reserved2; -    u32 owner;        /* id of the channel owner */ -    u32 owner_sub_id;    /* owner-defined field for extra tracking */ -    u32 reserved[5]; -} __packed;

/* Type of command transport buffer */   #define INTEL_GUC_CT_BUFFER_TYPE_SEND    0x0u   #define INTEL_GUC_CT_BUFFER_TYPE_RECV    0x1u diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 63056ea0631e..3241a477196f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -112,32 +112,28 @@ static inline const char *guc_ct_buffer_type_to_str(u32 type)       }   }   -static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc, -                    u32 cmds_addr, u32 size) +static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc)

this function is called from only 1 place and only does a memset now, so IMO we can just drop it and inline the memset.

ok, but without enthusiasm ;)

The logic below matches the specs.

Daniele

...

{       memset(desc, 0, sizeof(*desc)); -    desc->addr = cmds_addr; -    desc->size = size; -    desc->owner = CTB_OWNER_HOST;   }   -static void guc_ct_buffer_reset(struct intel_guc_ct_buffer *ctb, u32 cmds_addr) +static void guc_ct_buffer_reset(struct intel_guc_ct_buffer *ctb)   {       ctb->broken = false; -    guc_ct_buffer_desc_init(ctb->desc, cmds_addr, ctb->size); +    guc_ct_buffer_desc_init(ctb->desc);   }     static void guc_ct_buffer_init(struct intel_guc_ct_buffer *ctb,                      struct guc_ct_buffer_desc *desc, -                   u32 *cmds, u32 size) +                   u32 *cmds, u32 size_in_bytes)   { -    GEM_BUG_ON(size % 4); +    GEM_BUG_ON(size_in_bytes % 4);         ctb->desc = desc;       ctb->cmds = cmds; -    ctb->size = size; +    ctb->size = size_in_bytes / 4;   -    guc_ct_buffer_reset(ctb, 0); +    guc_ct_buffer_reset(ctb);   }     static int guc_action_register_ct_buffer(struct intel_guc *guc, @@ -279,10 +275,10 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)         /* (re)initialize descriptors */       cmds = base + ptrdiff(ct->ctbs.send.cmds, blob); -    guc_ct_buffer_reset(&ct->ctbs.send, cmds); +    guc_ct_buffer_reset(&ct->ctbs.send);         cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob); -    guc_ct_buffer_reset(&ct->ctbs.recv, cmds); +    guc_ct_buffer_reset(&ct->ctbs.recv);         /*        * Register both CT buffers starting with RECV buffer. @@ -391,17 +387,15 @@ static int ct_write(struct intel_guc_ct *ct,       if (unlikely(ctb->broken))           return -EPIPE;   -    if (unlikely(desc->is_in_error)) +    if (unlikely(desc->status))           goto corrupted;   -    if (unlikely(!IS_ALIGNED(head | tail, 4) || -             (tail | head) >= size)) +    if (unlikely((tail | head) >= size)) { +        CT_ERROR(ct, "Invalid offsets head=%u tail=%u (size=%u)\n", +             head, tail, size); +        desc->status |= GUC_CTB_STATUS_OVERFLOW;           goto corrupted;

-    /* later calculations will be done in dwords */ -    head /= 4; -    tail /= 4; -    size /= 4; +    }         /*        * tail == head condition indicates empty. GuC FW does not support @@ -447,14 +441,14 @@ static int ct_write(struct intel_guc_ct *ct,        */       write_barrier(ct);   -    /* now update desc tail (back in bytes) */ -    desc->tail = tail * 4; +    /* now update descriptor */ +    WRITE_ONCE(desc->tail, tail);

return 0;     corrupted: -    CT_ERROR(ct, "Corrupted descriptor addr=%#x head=%u tail=%u size=%u\n", -         desc->addr, desc->head, desc->tail, desc->size); -    desc->is_in_error = 1; +    CT_ERROR(ct, "Corrupted descriptor head=%u tail=%u status=%#x\n", +         desc->head, desc->tail, desc->status);       ctb->broken = true;       return -EPIPE;   } @@ -640,17 +634,15 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg)       if (unlikely(ctb->broken))           return -EPIPE;   -    if (unlikely(desc->is_in_error)) +    if (unlikely(desc->status))           goto corrupted;   -    if (unlikely(!IS_ALIGNED(head | tail, 4) || -             (tail | head) >= size)) +    if (unlikely((tail | head) >= size)) { +        CT_ERROR(ct, "Invalid offsets head=%u tail=%u (size=%u)\n", +             head, tail, size); +        desc->status |= GUC_CTB_STATUS_OVERFLOW;           goto corrupted;

-    /* later calculations will be done in dwords */ -    head /= 4; -    tail /= 4; -    size /= 4; +    }         /* tail == head condition indicates empty */       available = tail - head; @@ -677,6 +669,7 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg)                     size - head : available - 1), &cmds[head],                4 * (head + available - 1 > size ?                     available - 1 - size + head : 0), &cmds[0]); +        desc->status |= GUC_CTB_STATUS_UNDERFLOW;           goto corrupted;       }   @@ -699,13 +692,14 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg)       }       CT_DEBUG(ct, "received %*ph\n", 4 * len, (*msg)->msg);   -    desc->head = head * 4; +    /* now update descriptor */ +    WRITE_ONCE(desc->head, head);

return available - len;     corrupted: -    CT_ERROR(ct, "Corrupted descriptor addr=%#x head=%u tail=%u size=%u\n", -         desc->addr, desc->head, desc->tail, desc->size); -    desc->is_in_error = 1; +    CT_ERROR(ct, "Corrupted descriptor head=%u tail=%u status=%#x\n", +         desc->head, desc->tail, desc->status);       ctb->broken = true;       return -EPIPE;   } diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h index 7d3cd375d6a7..905202caaad3 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h @@ -31,7 +31,7 @@ struct intel_guc;    * @lock: protects access to the commands buffer and buffer descriptor    * @desc: pointer to the buffer descriptor    * @cmds: pointer to the commands buffer

• • @size: size of the commands buffer

• • @size: size of the commands buffer in dwords

* @broken: flag to indicate if descriptor data is broken    */   struct intel_guc_ct_buffer {

On 6/7/2021 11:03 AM, Matthew Brost wrote:

From: Michal Wajdeczko michal.wajdeczko@intel.com

Definition of the CTB registration action has changed. Add some ABI documentation and implement required changes.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: Piotr Piórkowski piotr.piorkowski@intel.com #4

---

.../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 107 ++++++++++++++++++ .../gt/uc/abi/guc_communication_ctb_abi.h | 4 - drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 76 ++++++++----- 3 files changed, 152 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h index 90efef8a73e4..6426fc183692 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h @@ -6,6 +6,113 @@ #ifndef _ABI_GUC_ACTIONS_ABI_H #define _ABI_GUC_ACTIONS_ABI_H

+/**

• • DOC: HOST2GUC_REGISTER_CTB
• • This message is used as part of the `CTB based communication`_ setup.
• • This message must be sent as `MMIO HXG Message`_.
• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31 | ORIGIN = GUC_HXG_ORIGIN_HOST_ |
• • | +-------+--------------------------------------------------------------+
• • | | 30:28 | TYPE = GUC_HXG_TYPE_REQUEST_ |
• • | +-------+--------------------------------------------------------------+
• • | | 27:16 | DATA0 = MBZ |
• • | +-------+--------------------------------------------------------------+
• • | | 15:0 | ACTION = _`GUC_ACTION_HOST2GUC_REGISTER_CTB` = 0x5200 |

Specs says 4505

• • +---+-------+--------------------------------------------------------------+
• • | 1 | 31:12 | RESERVED = MBZ |
• • | +-------+--------------------------------------------------------------+
• • | | 11:8 | **TYPE** - type for the `CT Buffer`_ |
• • | | | |
• • | | | - _`GUC_CTB_TYPE_HOST2GUC` = 0 |
• • | | | - _`GUC_CTB_TYPE_GUC2HOST` = 1 |
• • | +-------+--------------------------------------------------------------+
• • | | 7:0 | **SIZE** - size of the `CT Buffer`_ in 4K units minus 1 |
• • +---+-------+--------------------------------------------------------------+
• • | 2 | 31:0 | **DESC_ADDR** - GGTT address of the `CTB Descriptor`_ |
• • +---+-------+--------------------------------------------------------------+
• • | 3 | 31:0 | **BUFF_ADDF** - GGTT address of the `CT Buffer`_ |
• • +---+-------+--------------------------------------------------------------+

+*

• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31 | ORIGIN = GUC_HXG_ORIGIN_GUC_ |
• • | +-------+--------------------------------------------------------------+
• • | | 30:28 | TYPE = GUC_HXG_TYPE_RESPONSE_SUCCESS_ |
• • | +-------+--------------------------------------------------------------+
• • | | 27:0 | DATA0 = MBZ |
• • +---+-------+--------------------------------------------------------------+
• */

+#define GUC_ACTION_HOST2GUC_REGISTER_CTB 0x4505 // FIXME 0x5200

Why FIXME? AFAICS the specs still says 4505, even if we plan to update at some point I don;t think this deserves a FIXME since nothing is incorrect.

+#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_LEN (GUC_HXG_REQUEST_MSG_MIN_LEN + 3u) +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_0_MBZ GUC_HXG_REQUEST_MSG_0_DATA0 +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_MBZ (0xfffff << 12) +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_TYPE (0xf << 8) +#define GUC_CTB_TYPE_HOST2GUC 0u +#define GUC_CTB_TYPE_GUC2HOST 1u +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_SIZE (0xff << 0) +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_2_DESC_ADDR GUC_HXG_REQUEST_MSG_n_DATAn +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_3_BUFF_ADDR GUC_HXG_REQUEST_MSG_n_DATAn

The full mask still seems like overkill to me and I still think we should use BIT()/GENMASK() and a _MASK prefix, but not going to block on it.

+#define HOST2GUC_REGISTER_CTB_RESPONSE_MSG_LEN GUC_HXG_RESPONSE_MSG_MIN_LEN +#define HOST2GUC_REGISTER_CTB_RESPONSE_MSG_0_MBZ GUC_HXG_RESPONSE_MSG_0_DATA0

+/**

• • DOC: HOST2GUC_DEREGISTER_CTB
• • This message is used as part of the `CTB based communication`_ teardown.
• • This message must be sent as `MMIO HXG Message`_.
• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31 | ORIGIN = GUC_HXG_ORIGIN_HOST_ |
• • | +-------+--------------------------------------------------------------+
• • | | 30:28 | TYPE = GUC_HXG_TYPE_REQUEST_ |
• • | +-------+--------------------------------------------------------------+
• • | | 27:16 | DATA0 = MBZ |
• • | +-------+--------------------------------------------------------------+
• • | | 15:0 | ACTION = _`GUC_ACTION_HOST2GUC_DEREGISTER_CTB` = 0x5201 |

Specs says 4506

• • +---+-------+--------------------------------------------------------------+
• • | 1 | 31:12 | RESERVED = MBZ |
• • | +-------+--------------------------------------------------------------+
• • | | 11:8 | **TYPE** - type of the `CT Buffer`_ |
• • | | | |
• • | | | see `GUC_ACTION_HOST2GUC_REGISTER_CTB`_ |
• • | +-------+--------------------------------------------------------------+
• • | | 7:0 | RESERVED = MBZ |
• • +---+-------+--------------------------------------------------------------+

+*

• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31 | ORIGIN = GUC_HXG_ORIGIN_GUC_ |
• • | +-------+--------------------------------------------------------------+
• • | | 30:28 | TYPE = GUC_HXG_TYPE_RESPONSE_SUCCESS_ |
• • | +-------+--------------------------------------------------------------+
• • | | 27:0 | DATA0 = MBZ |
• • +---+-------+--------------------------------------------------------------+
• */

+#define GUC_ACTION_HOST2GUC_DEREGISTER_CTB 0x4506 // FIXME 0x5201

Same comment for the FIXME as above

+#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_LEN (GUC_HXG_REQUEST_MSG_MIN_LEN + 1u) +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_0_MBZ GUC_HXG_REQUEST_MSG_0_DATA0 +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_MBZ (0xfffff << 12) +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_TYPE (0xf << 8) +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_MBZ2 (0xff << 0)

+#define HOST2GUC_DEREGISTER_CTB_RESPONSE_MSG_LEN GUC_HXG_RESPONSE_MSG_MIN_LEN +#define HOST2GUC_DEREGISTER_CTB_RESPONSE_MSG_0_MBZ GUC_HXG_RESPONSE_MSG_0_DATA0

+/* legacy definitions */

• enum intel_guc_action { INTEL_GUC_ACTION_DEFAULT = 0x0, INTEL_GUC_ACTION_REQUEST_PREEMPTION = 0x2,

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h index c2a069a78e01..127b256a662c 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h @@ -112,10 +112,6 @@ static_assert(sizeof(struct guc_ct_buffer_desc) == 64);

• • **flags**, holds various bits to control message handling

*/

-/* Type of command transport buffer */ -#define INTEL_GUC_CT_BUFFER_TYPE_SEND 0x0u -#define INTEL_GUC_CT_BUFFER_TYPE_RECV 0x1u

• /*
  • Definition of the command transport message header (DW0)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 3241a477196f..6a29be779cc9 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -103,9 +103,9 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct) static inline const char *guc_ct_buffer_type_to_str(u32 type) { switch (type) {

• case INTEL_GUC_CT_BUFFER_TYPE_SEND:

• case GUC_CTB_TYPE_HOST2GUC: return "SEND";

• case INTEL_GUC_CT_BUFFER_TYPE_RECV:

• case GUC_CTB_TYPE_GUC2HOST: return "RECV"; default: return "<invalid>";

@@ -136,25 +136,33 @@ static void guc_ct_buffer_init(struct intel_guc_ct_buffer *ctb, guc_ct_buffer_reset(ctb); }

-static int guc_action_register_ct_buffer(struct intel_guc *guc,

• 			 u32 desc_addr,
  

• 			 u32 type)
  

+static int guc_action_register_ct_buffer(struct intel_guc *guc, u32 type,

• 			 u32 desc_addr, u32 buff_addr, u32 size)
  

  {

• u32 action[] = {

• INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER,
  

• desc_addr,
  

• sizeof(struct guc_ct_buffer_desc),
  

• type
  

• u32 request[HOST2GUC_REGISTER_CTB_REQUEST_MSG_LEN] = {

• FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) |
  

• FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
  

• FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, GUC_ACTION_HOST2GUC_REGISTER_CTB),
  

IMO we could use a macro or 2 for the HXG header, to avoid all these lines, which are hard to read. something like:

GUC_HXG_HEADER(origin, type, data, action) \     (FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, origin) | \      FIELD_PREP(GUC_HXG_MSG_0_TYPE, type) | \ FIELD_PREP(GUC_HXG_MSG_0_DATA0, data) | \      FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, action))

H2G_HEADER(type, data, action) \     GUC_HXG_HEADER(GUC_HXG_ORIGIN_HOST, type, data, action)

and then call

H2G_HEADER(GUC_HXG_TYPE_REQUEST, 0, GUC_ACTION_HOST2GUC_REGISTER_CTB)

Not a blocker.

Daniele

• FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_SIZE, size / SZ_4K - 1) |
  

• FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_TYPE, type),
  

• FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_2_DESC_ADDR, desc_addr),
  

• FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_3_BUFF_ADDR, buff_addr),
  

  };

• /* Can't use generic send(), CT registration must go over MMIO */
• return intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0);

• GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != GUC_CTB_TYPE_GUC2HOST);
• GEM_BUG_ON(size % SZ_4K);
• /* CT registration must go over MMIO */
• return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0); }

-static int ct_register_buffer(struct intel_guc_ct *ct, u32 desc_addr, u32 type) +static int ct_register_buffer(struct intel_guc_ct *ct, u32 type,

• 	      u32 desc_addr, u32 buff_addr, u32 size)
  

  {

• int err = guc_action_register_ct_buffer(ct_to_guc(ct), desc_addr, type);

• int err;

• err = guc_action_register_ct_buffer(ct_to_guc(ct), type,

• 			    desc_addr, buff_addr, size);
  

  if (unlikely(err)) CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n", guc_ct_buffer_type_to_str(type), err);

@@ -163,14 +171,17 @@ static int ct_register_buffer(struct intel_guc_ct *ct, u32 desc_addr, u32 type)

static int guc_action_deregister_ct_buffer(struct intel_guc *guc, u32 type) {

• u32 action[] = {

• INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER,
  

• CTB_OWNER_HOST,
  

• type
  

• u32 request[HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_LEN] = {

• FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) |
  

• FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
  

• FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, GUC_ACTION_HOST2GUC_DEREGISTER_CTB),
  

• FIELD_PREP(HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_TYPE, type),
  

  };

• /* Can't use generic send(), CT deregistration must go over MMIO */
• return intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0);

• GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != GUC_CTB_TYPE_GUC2HOST);

• /* CT deregistration must go over MMIO */

• return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0); }

  static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type)

@@ -258,7 +269,7 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct) int intel_guc_ct_enable(struct intel_guc_ct *ct) { struct intel_guc *guc = ct_to_guc(ct);

• u32 base, cmds;

• u32 base, desc, cmds; void *blob; int err;

@@ -274,23 +285,26 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) GEM_BUG_ON(blob != ct->ctbs.send.desc);

/* (re)initialize descriptors */

• cmds = base + ptrdiff(ct->ctbs.send.cmds, blob); guc_ct_buffer_reset(&ct->ctbs.send);

• cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob); guc_ct_buffer_reset(&ct->ctbs.recv);

  /*

  • Register both CT buffers starting with RECV buffer.
  • Descriptors are in first half of the blob.

  */

• err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.recv.desc, blob),

• 		 INTEL_GUC_CT_BUFFER_TYPE_RECV);
  

• desc = base + ptrdiff(ct->ctbs.recv.desc, blob);
• cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob);
• err = ct_register_buffer(ct, GUC_CTB_TYPE_GUC2HOST,

• 		 desc, cmds, ct->ctbs.recv.size * 4);
  

• if (unlikely(err)) goto err_out;

• err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.send.desc, blob),

• 		 INTEL_GUC_CT_BUFFER_TYPE_SEND);
  

• desc = base + ptrdiff(ct->ctbs.send.desc, blob);
• cmds = base + ptrdiff(ct->ctbs.send.cmds, blob);
• err = ct_register_buffer(ct, GUC_CTB_TYPE_HOST2GUC,

• 		 desc, cmds, ct->ctbs.send.size * 4);
  

• if (unlikely(err)) goto err_deregister;

@@ -299,7 +313,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) return 0;

err_deregister:

• ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV);

• ct_deregister_buffer(ct, GUC_CTB_TYPE_GUC2HOST); err_out: CT_PROBE_ERROR(ct, "Failed to enable CTB (%pe)\n", ERR_PTR(err)); return err;

@@ -318,8 +332,8 @@ void intel_guc_ct_disable(struct intel_guc_ct *ct) ct->enabled = false;

if (intel_guc_is_fw_running(guc)) {

• ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_SEND);
  

• ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV);
  

• ct_deregister_buffer(ct, GUC_CTB_TYPE_HOST2GUC);
  

• ct_deregister_buffer(ct, GUC_CTB_TYPE_GUC2HOST);
  

  } }

On 09.06.2021 19:36, John Harrison wrote:

On 6/7/2021 18:23, Daniele Ceraolo Spurio wrote:

...

On 6/7/2021 11:03 AM, Matthew Brost wrote:

...

From: Michal Wajdeczko michal.wajdeczko@intel.com

Definition of the CTB registration action has changed. Add some ABI documentation and implement required changes.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: Piotr Piórkowski piotr.piorkowski@intel.com #4

---

.../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  | 107 ++++++++++++++++++   .../gt/uc/abi/guc_communication_ctb_abi.h     |   4 -   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c     |  76 ++++++++-----   3 files changed, 152 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h index 90efef8a73e4..6426fc183692 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h @@ -6,6 +6,113 @@   #ifndef _ABI_GUC_ACTIONS_ABI_H   #define _ABI_GUC_ACTIONS_ABI_H   +/**

• • DOC: HOST2GUC_REGISTER_CTB
• • This message is used as part of the `CTB based communication`_

setup.

• • This message must be sent as `MMIO HXG Message`_.

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

+===+=======+==============================================================+

• *  | 0 |    31 | ORIGIN =

GUC_HXG_ORIGIN_HOST_                                |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 30:28 | TYPE =

GUC_HXG_TYPE_REQUEST_                                 |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 27:16 | DATA0 =

MBZ                                                  |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  15:0 | ACTION = _`GUC_ACTION_HOST2GUC_REGISTER_CTB` =

0x5200        |

Specs says 4505

...

+---+-------+--------------------------------------------------------------+

• *  | 1 | 31:12 | RESERVED =

MBZ                                               |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  11:8 | **TYPE** - type for the `CT

Buffer`_                         |

• *  |   |

|                                                              |

• *  |   |       |   - _`GUC_CTB_TYPE_HOST2GUC` =

0                             |

• *  |   |       |   - _`GUC_CTB_TYPE_GUC2HOST` =

1                             |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |   7:0 | **SIZE** - size of the `CT Buffer`_ in 4K units

minus 1      |

+---+-------+--------------------------------------------------------------+

• *  | 2 |  31:0 | **DESC_ADDR** - GGTT address of the `CTB

Descriptor`_        |

+---+-------+--------------------------------------------------------------+

• *  | 3 |  31:0 | **BUFF_ADDF** - GGTT address of the `CT

Buffer`_             |

+---+-------+--------------------------------------------------------------+

+*

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

+===+=======+==============================================================+

• *  | 0 |    31 | ORIGIN =

GUC_HXG_ORIGIN_GUC_                                 |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 30:28 | TYPE =

GUC_HXG_TYPE_RESPONSE_SUCCESS_                        |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  27:0 | DATA0 =

MBZ                                                  |

+---+-------+--------------------------------------------------------------+

• */

+#define GUC_ACTION_HOST2GUC_REGISTER_CTB        0x4505 // FIXME 0x5200

Why FIXME? AFAICS the specs still says 4505, even if we plan to update at some point I don;t think this deserves a FIXME since nothing is incorrect.

...

+#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_LEN (GUC_HXG_REQUEST_MSG_MIN_LEN + 3u) +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_0_MBZ GUC_HXG_REQUEST_MSG_0_DATA0 +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_MBZ        (0xfffff << 12) +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_TYPE    (0xf << 8) +#define   GUC_CTB_TYPE_HOST2GUC                0u +#define   GUC_CTB_TYPE_GUC2HOST                1u +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_SIZE    (0xff << 0) +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_2_DESC_ADDR GUC_HXG_REQUEST_MSG_n_DATAn +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_3_BUFF_ADDR GUC_HXG_REQUEST_MSG_n_DATAn

The full mask still seems like overkill to me and I still think we should use BIT()/GENMASK() and a _MASK prefix, but not going to block on it.

...

+#define HOST2GUC_REGISTER_CTB_RESPONSE_MSG_LEN GUC_HXG_RESPONSE_MSG_MIN_LEN +#define HOST2GUC_REGISTER_CTB_RESPONSE_MSG_0_MBZ GUC_HXG_RESPONSE_MSG_0_DATA0

+/**

• • DOC: HOST2GUC_DEREGISTER_CTB
• • This message is used as part of the `CTB based communication`_

teardown.

• • This message must be sent as `MMIO HXG Message`_.

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

+===+=======+==============================================================+

• *  | 0 |    31 | ORIGIN =

GUC_HXG_ORIGIN_HOST_                                |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 30:28 | TYPE =

GUC_HXG_TYPE_REQUEST_                                 |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 27:16 | DATA0 =

MBZ                                                  |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  15:0 | ACTION = _`GUC_ACTION_HOST2GUC_DEREGISTER_CTB` =

0x5201      |

Specs says 4506

I would say that the enum value should not be included in the structure definition. I would also argue that there is no point in repeating the common header structure for every single H2G action definition. That is just overly verbose and makes it harder to read the spec. It implies that every action has a different header structure and must be coded individually.

but some actions are defined as REQUEST some as EVENT, so we need to say that, also each REQUEST action may define its own DATA0, so again we still need to define these bits somewhere

Personally, I don't believe we should be replicating the entire GuC API spec in the driver header files anyway. This is not something that is defined by the i915 driver so the i915 driver should not be defining it! Instead, just include a link or pointer to where the actual spec can be found. We don't copy the entire bspec page for every register that the driver touches, so why should this be any different?

to some extend we have to replicate at least part of the GuC ABI spec, part that defines all bits, and IMHO there is nothing wrong if it comes with full message layout definitions, especially if you compare that with previous approach, were H2G action definitions were limited just to single enum value (and to find out how to use given H2G you had to look into firmware source code)

so while we keep these abi.h files in kernel repo, they shall be treated as read-only imported external interface definitions, from which we just use all #define for coding and DOC: for documentation (latter at least until GuC will release its spec to the public)

John.

...

...

+---+-------+--------------------------------------------------------------+

• *  | 1 | 31:12 | RESERVED =

MBZ                                               |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  11:8 | **TYPE** - type of the `CT

Buffer`_                          |

• *  |   |

|                                                              |

• *  |   |       | see

`GUC_ACTION_HOST2GUC_REGISTER_CTB`_                      |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |   7:0 | RESERVED =

MBZ                                               |

+---+-------+--------------------------------------------------------------+

+*

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

+===+=======+==============================================================+

• *  | 0 |    31 | ORIGIN =

GUC_HXG_ORIGIN_GUC_                                 |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 30:28 | TYPE =

GUC_HXG_TYPE_RESPONSE_SUCCESS_                        |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  27:0 | DATA0 =

MBZ                                                  |

+---+-------+--------------------------------------------------------------+

• */

+#define GUC_ACTION_HOST2GUC_DEREGISTER_CTB        0x4506 // FIXME 0x5201

Same comment for the FIXME as above

...

+#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_LEN (GUC_HXG_REQUEST_MSG_MIN_LEN + 1u) +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_0_MBZ GUC_HXG_REQUEST_MSG_0_DATA0 +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_MBZ    (0xfffff << 12) +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_TYPE    (0xf << 8) +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_MBZ2    (0xff << 0)

+#define HOST2GUC_DEREGISTER_CTB_RESPONSE_MSG_LEN GUC_HXG_RESPONSE_MSG_MIN_LEN +#define HOST2GUC_DEREGISTER_CTB_RESPONSE_MSG_0_MBZ GUC_HXG_RESPONSE_MSG_0_DATA0

+/* legacy definitions */

enum intel_guc_action {       INTEL_GUC_ACTION_DEFAULT = 0x0,       INTEL_GUC_ACTION_REQUEST_PREEMPTION = 0x2, diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h index c2a069a78e01..127b256a662c 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h @@ -112,10 +112,6 @@ static_assert(sizeof(struct guc_ct_buffer_desc) == 64);    * - **flags**, holds various bits to control message handling    */   -/* Type of command transport buffer */ -#define INTEL_GUC_CT_BUFFER_TYPE_SEND    0x0u -#define INTEL_GUC_CT_BUFFER_TYPE_RECV    0x1u

/*    * Definition of the command transport message header (DW0)    * diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 3241a477196f..6a29be779cc9 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -103,9 +103,9 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)   static inline const char *guc_ct_buffer_type_to_str(u32 type)   {       switch (type) { -    case INTEL_GUC_CT_BUFFER_TYPE_SEND: +    case GUC_CTB_TYPE_HOST2GUC:           return "SEND"; -    case INTEL_GUC_CT_BUFFER_TYPE_RECV: +    case GUC_CTB_TYPE_GUC2HOST:           return "RECV";       default:           return "<invalid>"; @@ -136,25 +136,33 @@ static void guc_ct_buffer_init(struct intel_guc_ct_buffer *ctb,       guc_ct_buffer_reset(ctb);   }   -static int guc_action_register_ct_buffer(struct intel_guc *guc, -                     u32 desc_addr, -                     u32 type) +static int guc_action_register_ct_buffer(struct intel_guc *guc, u32 type, +                     u32 desc_addr, u32 buff_addr, u32 size)   { -    u32 action[] = { -        INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER, -        desc_addr, -        sizeof(struct guc_ct_buffer_desc), -        type +    u32 request[HOST2GUC_REGISTER_CTB_REQUEST_MSG_LEN] = { +        FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) | +        FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) | +        FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, GUC_ACTION_HOST2GUC_REGISTER_CTB),

IMO we could use a macro or 2 for the HXG header, to avoid all these lines, which are hard to read. something like:

GUC_HXG_HEADER(origin, type, data, action) \     (FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, origin) | \      FIELD_PREP(GUC_HXG_MSG_0_TYPE, type) | \ FIELD_PREP(GUC_HXG_MSG_0_DATA0, data) | \      FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, action))

H2G_HEADER(type, data, action) \     GUC_HXG_HEADER(GUC_HXG_ORIGIN_HOST, type, data, action)

and then call

H2G_HEADER(GUC_HXG_TYPE_REQUEST, 0, GUC_ACTION_HOST2GUC_REGISTER_CTB)

Not a blocker.

Daniele

...

• FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_SIZE, size / SZ_4K -

1. |

+        FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_TYPE, type),

• FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_2_DESC_ADDR, desc_addr),
• FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_3_BUFF_ADDR, buff_addr),

};   -    /* Can't use generic send(), CT registration must go over MMIO */ -    return intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0); +    GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != GUC_CTB_TYPE_GUC2HOST); +    GEM_BUG_ON(size % SZ_4K);

+    /* CT registration must go over MMIO */ +    return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0);   }   -static int ct_register_buffer(struct intel_guc_ct *ct, u32 desc_addr, u32 type) +static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, +                  u32 desc_addr, u32 buff_addr, u32 size)   { -    int err = guc_action_register_ct_buffer(ct_to_guc(ct), desc_addr, type); +    int err;   +    err = guc_action_register_ct_buffer(ct_to_guc(ct), type, +                        desc_addr, buff_addr, size);       if (unlikely(err))           CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n",                guc_ct_buffer_type_to_str(type), err); @@ -163,14 +171,17 @@ static int ct_register_buffer(struct intel_guc_ct *ct, u32 desc_addr, u32 type)     static int guc_action_deregister_ct_buffer(struct intel_guc *guc, u32 type)   { -    u32 action[] = { -        INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER, -        CTB_OWNER_HOST, -        type +    u32 request[HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_LEN] = { +        FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) | +        FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) | +        FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, GUC_ACTION_HOST2GUC_DEREGISTER_CTB), +        FIELD_PREP(HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_TYPE, type),       };   -    /* Can't use generic send(), CT deregistration must go over MMIO */ -    return intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0); +    GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != GUC_CTB_TYPE_GUC2HOST);

+    /* CT deregistration must go over MMIO */ +    return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0);   }     static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type) @@ -258,7 +269,7 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct)   int intel_guc_ct_enable(struct intel_guc_ct *ct)   {       struct intel_guc *guc = ct_to_guc(ct); -    u32 base, cmds; +    u32 base, desc, cmds;       void *blob;       int err;   @@ -274,23 +285,26 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)       GEM_BUG_ON(blob != ct->ctbs.send.desc);         /* (re)initialize descriptors */ -    cmds = base + ptrdiff(ct->ctbs.send.cmds, blob);       guc_ct_buffer_reset(&ct->ctbs.send);

-    cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob);       guc_ct_buffer_reset(&ct->ctbs.recv);         /*        * Register both CT buffers starting with RECV buffer.        * Descriptors are in first half of the blob.        */ -    err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.recv.desc, blob), -                 INTEL_GUC_CT_BUFFER_TYPE_RECV); +    desc = base + ptrdiff(ct->ctbs.recv.desc, blob); +    cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob); +    err = ct_register_buffer(ct, GUC_CTB_TYPE_GUC2HOST, +                 desc, cmds, ct->ctbs.recv.size * 4);

if (unlikely(err))           goto err_out;   -    err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.send.desc, blob), -                 INTEL_GUC_CT_BUFFER_TYPE_SEND); +    desc = base + ptrdiff(ct->ctbs.send.desc, blob); +    cmds = base + ptrdiff(ct->ctbs.send.cmds, blob); +    err = ct_register_buffer(ct, GUC_CTB_TYPE_HOST2GUC, +                 desc, cmds, ct->ctbs.send.size * 4);

if (unlikely(err))           goto err_deregister;   @@ -299,7 +313,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)       return 0;     err_deregister: -    ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV); +    ct_deregister_buffer(ct, GUC_CTB_TYPE_GUC2HOST);   err_out:       CT_PROBE_ERROR(ct, "Failed to enable CTB (%pe)\n", ERR_PTR(err));       return err; @@ -318,8 +332,8 @@ void intel_guc_ct_disable(struct intel_guc_ct *ct)       ct->enabled = false;         if (intel_guc_is_fw_running(guc)) { -        ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_SEND); -        ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV); +        ct_deregister_buffer(ct, GUC_CTB_TYPE_HOST2GUC); +        ct_deregister_buffer(ct, GUC_CTB_TYPE_GUC2HOST);       }   }

On 10.06.2021 06:38, Matthew Brost wrote:

On Wed, Jun 09, 2021 at 10:07:21PM +0200, Michal Wajdeczko wrote:

...

On 09.06.2021 19:36, John Harrison wrote:

...

On 6/7/2021 18:23, Daniele Ceraolo Spurio wrote:

...

On 6/7/2021 11:03 AM, Matthew Brost wrote:

...

From: Michal Wajdeczko michal.wajdeczko@intel.com

Definition of the CTB registration action has changed. Add some ABI documentation and implement required changes.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: Piotr Piórkowski piotr.piorkowski@intel.com #4

---

.../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  | 107 ++++++++++++++++++   .../gt/uc/abi/guc_communication_ctb_abi.h     |   4 -   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c     |  76 ++++++++-----   3 files changed, 152 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h index 90efef8a73e4..6426fc183692 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h @@ -6,6 +6,113 @@   #ifndef _ABI_GUC_ACTIONS_ABI_H   #define _ABI_GUC_ACTIONS_ABI_H   +/**

• • DOC: HOST2GUC_REGISTER_CTB
• • This message is used as part of the `CTB based communication`_

setup.

• • This message must be sent as `MMIO HXG Message`_.

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

+===+=======+==============================================================+

• *  | 0 |    31 | ORIGIN =

GUC_HXG_ORIGIN_HOST_                                |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 30:28 | TYPE =

GUC_HXG_TYPE_REQUEST_                                 |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 27:16 | DATA0 =

MBZ                                                  |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  15:0 | ACTION = _`GUC_ACTION_HOST2GUC_REGISTER_CTB` =

0x5200        |

Specs says 4505

...

+---+-------+--------------------------------------------------------------+

• *  | 1 | 31:12 | RESERVED =

MBZ                                               |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  11:8 | **TYPE** - type for the `CT

Buffer`_                         |

• *  |   |

|                                                              |

• *  |   |       |   - _`GUC_CTB_TYPE_HOST2GUC` =

0                             |

• *  |   |       |   - _`GUC_CTB_TYPE_GUC2HOST` =

1                             |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |   7:0 | **SIZE** - size of the `CT Buffer`_ in 4K units

minus 1      |

+---+-------+--------------------------------------------------------------+

• *  | 2 |  31:0 | **DESC_ADDR** - GGTT address of the `CTB

Descriptor`_        |

+---+-------+--------------------------------------------------------------+

• *  | 3 |  31:0 | **BUFF_ADDF** - GGTT address of the `CT

Buffer`_             |

+---+-------+--------------------------------------------------------------+

+*

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

+===+=======+==============================================================+

• *  | 0 |    31 | ORIGIN =

GUC_HXG_ORIGIN_GUC_                                 |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 30:28 | TYPE =

GUC_HXG_TYPE_RESPONSE_SUCCESS_                        |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  27:0 | DATA0 =

MBZ                                                  |

+---+-------+--------------------------------------------------------------+

• */

+#define GUC_ACTION_HOST2GUC_REGISTER_CTB        0x4505 // FIXME 0x5200

Why FIXME? AFAICS the specs still says 4505, even if we plan to update at some point I don;t think this deserves a FIXME since nothing is incorrect.

...

+#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_LEN (GUC_HXG_REQUEST_MSG_MIN_LEN + 3u) +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_0_MBZ GUC_HXG_REQUEST_MSG_0_DATA0 +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_MBZ        (0xfffff << 12) +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_TYPE    (0xf << 8) +#define   GUC_CTB_TYPE_HOST2GUC                0u +#define   GUC_CTB_TYPE_GUC2HOST                1u +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_SIZE    (0xff << 0) +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_2_DESC_ADDR GUC_HXG_REQUEST_MSG_n_DATAn +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_3_BUFF_ADDR GUC_HXG_REQUEST_MSG_n_DATAn

The full mask still seems like overkill to me and I still think we should use BIT()/GENMASK() and a _MASK prefix, but not going to block on it.

...

+#define HOST2GUC_REGISTER_CTB_RESPONSE_MSG_LEN GUC_HXG_RESPONSE_MSG_MIN_LEN +#define HOST2GUC_REGISTER_CTB_RESPONSE_MSG_0_MBZ GUC_HXG_RESPONSE_MSG_0_DATA0

+/**

• • DOC: HOST2GUC_DEREGISTER_CTB
• • This message is used as part of the `CTB based communication`_

teardown.

• • This message must be sent as `MMIO HXG Message`_.

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

+===+=======+==============================================================+

• *  | 0 |    31 | ORIGIN =

GUC_HXG_ORIGIN_HOST_                                |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 30:28 | TYPE =

GUC_HXG_TYPE_REQUEST_                                 |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 27:16 | DATA0 =

MBZ                                                  |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  15:0 | ACTION = _`GUC_ACTION_HOST2GUC_DEREGISTER_CTB` =

0x5201      |

Specs says 4506

I would say that the enum value should not be included in the structure definition. I would also argue that there is no point in repeating the common header structure for every single H2G action definition. That is just overly verbose and makes it harder to read the spec. It implies that every action has a different header structure and must be coded individually.

but some actions are defined as REQUEST some as EVENT, so we need to say that, also each REQUEST action may define its own DATA0, so again we still need to define these bits somewhere

...

Personally, I don't believe we should be replicating the entire GuC API spec in the driver header files anyway. This is not something that is defined by the i915 driver so the i915 driver should not be defining it! Instead, just include a link or pointer to where the actual spec can be found. We don't copy the entire bspec page for every register that the driver touches, so why should this be any different?

I agree with John on this one. We plan publishing the GuC, right? Let's

Do you know of any ETA? I don't

and likely the same promise was given few years back when GuC was introduced in upstream, I don't want to have just code that we can't compare with specification (in any form)

just point to it in the kernel DOC.

Also at some all these defines really should be auto-generated. I suppose if these headers are auto-generated, I could live with these

I was also hoping to get these ABI headers auto-generated before we start to used them for good, unfortunately it was quite the opposite: for some time these hand crafted tables were used as input for discussion and then to prepare machine readable formats, but the only tool currently available (and still WIP) is for generating spec documentation

files generating kernel DOC. I can't really live with having to maintain a table like this for every action manually.

the goal is to freeze ABI so no maintenance will be necessary, except adding new actions, and that's also the reason to keep these ABI files separate from the rest of our headers, where we can add/modify/improve any helpers/wrappers as we want.

and I don't recall that you were forced to modify any of such tables yet, nor were asked to manually prepare them for the rest of the existing actions, especially GuC submission ones, so why complain?

Matt

...

to some extend we have to replicate at least part of the GuC ABI spec, part that defines all bits, and IMHO there is nothing wrong if it comes with full message layout definitions, especially if you compare that with previous approach, were H2G action definitions were limited just to single enum value (and to find out how to use given H2G you had to look into firmware source code)

so while we keep these abi.h files in kernel repo, they shall be treated as read-only imported external interface definitions, from which we just use all #define for coding and DOC: for documentation (latter at least until GuC will release its spec to the public)

...

John.

...

...

+---+-------+--------------------------------------------------------------+

• *  | 1 | 31:12 | RESERVED =

MBZ                                               |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  11:8 | **TYPE** - type of the `CT

Buffer`_                          |

• *  |   |

|                                                              |

• *  |   |       | see

`GUC_ACTION_HOST2GUC_REGISTER_CTB`_                      |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |   7:0 | RESERVED =

MBZ                                               |

+---+-------+--------------------------------------------------------------+

+*

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

+===+=======+==============================================================+

• *  | 0 |    31 | ORIGIN =

GUC_HXG_ORIGIN_GUC_                                 |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 30:28 | TYPE =

GUC_HXG_TYPE_RESPONSE_SUCCESS_                        |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  27:0 | DATA0 =

MBZ                                                  |

+---+-------+--------------------------------------------------------------+

• */

+#define GUC_ACTION_HOST2GUC_DEREGISTER_CTB        0x4506 // FIXME 0x5201

Same comment for the FIXME as above

...

+#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_LEN (GUC_HXG_REQUEST_MSG_MIN_LEN + 1u) +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_0_MBZ GUC_HXG_REQUEST_MSG_0_DATA0 +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_MBZ    (0xfffff << 12) +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_TYPE    (0xf << 8) +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_MBZ2    (0xff << 0)

+#define HOST2GUC_DEREGISTER_CTB_RESPONSE_MSG_LEN GUC_HXG_RESPONSE_MSG_MIN_LEN +#define HOST2GUC_DEREGISTER_CTB_RESPONSE_MSG_0_MBZ GUC_HXG_RESPONSE_MSG_0_DATA0

+/* legacy definitions */

enum intel_guc_action {       INTEL_GUC_ACTION_DEFAULT = 0x0,       INTEL_GUC_ACTION_REQUEST_PREEMPTION = 0x2, diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h index c2a069a78e01..127b256a662c 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h @@ -112,10 +112,6 @@ static_assert(sizeof(struct guc_ct_buffer_desc) == 64);    * - **flags**, holds various bits to control message handling    */   -/* Type of command transport buffer */ -#define INTEL_GUC_CT_BUFFER_TYPE_SEND    0x0u -#define INTEL_GUC_CT_BUFFER_TYPE_RECV    0x1u

/*    * Definition of the command transport message header (DW0)    * diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 3241a477196f..6a29be779cc9 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -103,9 +103,9 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)   static inline const char *guc_ct_buffer_type_to_str(u32 type)   {       switch (type) { -    case INTEL_GUC_CT_BUFFER_TYPE_SEND: +    case GUC_CTB_TYPE_HOST2GUC:           return "SEND"; -    case INTEL_GUC_CT_BUFFER_TYPE_RECV: +    case GUC_CTB_TYPE_GUC2HOST:           return "RECV";       default:           return "<invalid>"; @@ -136,25 +136,33 @@ static void guc_ct_buffer_init(struct intel_guc_ct_buffer *ctb,       guc_ct_buffer_reset(ctb);   }   -static int guc_action_register_ct_buffer(struct intel_guc *guc, -                     u32 desc_addr, -                     u32 type) +static int guc_action_register_ct_buffer(struct intel_guc *guc, u32 type, +                     u32 desc_addr, u32 buff_addr, u32 size)   { -    u32 action[] = { -        INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER, -        desc_addr, -        sizeof(struct guc_ct_buffer_desc), -        type +    u32 request[HOST2GUC_REGISTER_CTB_REQUEST_MSG_LEN] = { +        FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) | +        FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) | +        FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, GUC_ACTION_HOST2GUC_REGISTER_CTB),

IMO we could use a macro or 2 for the HXG header, to avoid all these lines, which are hard to read. something like:

GUC_HXG_HEADER(origin, type, data, action) \     (FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, origin) | \      FIELD_PREP(GUC_HXG_MSG_0_TYPE, type) | \ FIELD_PREP(GUC_HXG_MSG_0_DATA0, data) | \      FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, action))

H2G_HEADER(type, data, action) \     GUC_HXG_HEADER(GUC_HXG_ORIGIN_HOST, type, data, action)

and then call

H2G_HEADER(GUC_HXG_TYPE_REQUEST, 0, GUC_ACTION_HOST2GUC_REGISTER_CTB)

Not a blocker.

Daniele

...

• FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_SIZE, size / SZ_4K -

1. |

+        FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_TYPE, type),

• FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_2_DESC_ADDR, desc_addr),
• FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_3_BUFF_ADDR, buff_addr),

};   -    /* Can't use generic send(), CT registration must go over MMIO */ -    return intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0); +    GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != GUC_CTB_TYPE_GUC2HOST); +    GEM_BUG_ON(size % SZ_4K);

+    /* CT registration must go over MMIO */ +    return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0);   }   -static int ct_register_buffer(struct intel_guc_ct *ct, u32 desc_addr, u32 type) +static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, +                  u32 desc_addr, u32 buff_addr, u32 size)   { -    int err = guc_action_register_ct_buffer(ct_to_guc(ct), desc_addr, type); +    int err;   +    err = guc_action_register_ct_buffer(ct_to_guc(ct), type, +                        desc_addr, buff_addr, size);       if (unlikely(err))           CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n",                guc_ct_buffer_type_to_str(type), err); @@ -163,14 +171,17 @@ static int ct_register_buffer(struct intel_guc_ct *ct, u32 desc_addr, u32 type)     static int guc_action_deregister_ct_buffer(struct intel_guc *guc, u32 type)   { -    u32 action[] = { -        INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER, -        CTB_OWNER_HOST, -        type +    u32 request[HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_LEN] = { +        FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) | +        FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) | +        FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, GUC_ACTION_HOST2GUC_DEREGISTER_CTB), +        FIELD_PREP(HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_TYPE, type),       };   -    /* Can't use generic send(), CT deregistration must go over MMIO */ -    return intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0); +    GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != GUC_CTB_TYPE_GUC2HOST);

+    /* CT deregistration must go over MMIO */ +    return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0);   }     static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type) @@ -258,7 +269,7 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct)   int intel_guc_ct_enable(struct intel_guc_ct *ct)   {       struct intel_guc *guc = ct_to_guc(ct); -    u32 base, cmds; +    u32 base, desc, cmds;       void *blob;       int err;   @@ -274,23 +285,26 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)       GEM_BUG_ON(blob != ct->ctbs.send.desc);         /* (re)initialize descriptors */ -    cmds = base + ptrdiff(ct->ctbs.send.cmds, blob);       guc_ct_buffer_reset(&ct->ctbs.send);

-    cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob);       guc_ct_buffer_reset(&ct->ctbs.recv);         /*        * Register both CT buffers starting with RECV buffer.        * Descriptors are in first half of the blob.        */ -    err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.recv.desc, blob), -                 INTEL_GUC_CT_BUFFER_TYPE_RECV); +    desc = base + ptrdiff(ct->ctbs.recv.desc, blob); +    cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob); +    err = ct_register_buffer(ct, GUC_CTB_TYPE_GUC2HOST, +                 desc, cmds, ct->ctbs.recv.size * 4);

if (unlikely(err))           goto err_out;   -    err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.send.desc, blob), -                 INTEL_GUC_CT_BUFFER_TYPE_SEND); +    desc = base + ptrdiff(ct->ctbs.send.desc, blob); +    cmds = base + ptrdiff(ct->ctbs.send.cmds, blob); +    err = ct_register_buffer(ct, GUC_CTB_TYPE_HOST2GUC, +                 desc, cmds, ct->ctbs.send.size * 4);

if (unlikely(err))           goto err_deregister;   @@ -299,7 +313,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)       return 0;     err_deregister: -    ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV); +    ct_deregister_buffer(ct, GUC_CTB_TYPE_GUC2HOST);   err_out:       CT_PROBE_ERROR(ct, "Failed to enable CTB (%pe)\n", ERR_PTR(err));       return err; @@ -318,8 +332,8 @@ void intel_guc_ct_disable(struct intel_guc_ct *ct)       ct->enabled = false;         if (intel_guc_is_fw_running(guc)) { -        ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_SEND); -        ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV); +        ct_deregister_buffer(ct, GUC_CTB_TYPE_HOST2GUC); +        ct_deregister_buffer(ct, GUC_CTB_TYPE_GUC2HOST);       }   }

On 08.06.2021 03:23, Daniele Ceraolo Spurio wrote:

On 6/7/2021 11:03 AM, Matthew Brost wrote:

...

From: Michal Wajdeczko michal.wajdeczko@intel.com

Definition of the CTB registration action has changed. Add some ABI documentation and implement required changes.

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: Piotr Piórkowski piotr.piorkowski@intel.com #4

---

.../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  | 107 ++++++++++++++++++   .../gt/uc/abi/guc_communication_ctb_abi.h     |   4 -   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c     |  76 ++++++++-----   3 files changed, 152 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h index 90efef8a73e4..6426fc183692 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h @@ -6,6 +6,113 @@   #ifndef _ABI_GUC_ACTIONS_ABI_H   #define _ABI_GUC_ACTIONS_ABI_H   +/**

• • DOC: HOST2GUC_REGISTER_CTB
• • This message is used as part of the `CTB based communication`_ setup.
• • This message must be sent as `MMIO HXG Message`_.
• *

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

• *

+===+=======+==============================================================+

• *  | 0 |    31 | ORIGIN =

GUC_HXG_ORIGIN_HOST_                                |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 30:28 | TYPE =

GUC_HXG_TYPE_REQUEST_                                 |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 27:16 | DATA0 =

MBZ                                                  |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  15:0 | ACTION = _`GUC_ACTION_HOST2GUC_REGISTER_CTB` =

0x5200        |

Specs says 4505

but draft was saying 5200 ;)

...

• *

+---+-------+--------------------------------------------------------------+

• *  | 1 | 31:12 | RESERVED =

MBZ                                               |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  11:8 | **TYPE** - type for the `CT

Buffer`_                         |

• *  |   |

|                                                              |

• *  |   |       |   - _`GUC_CTB_TYPE_HOST2GUC` =

0                             |

• *  |   |       |   - _`GUC_CTB_TYPE_GUC2HOST` =

1                             |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |   7:0 | **SIZE** - size of the `CT Buffer`_ in 4K units

minus 1      |

• *

+---+-------+--------------------------------------------------------------+

• *  | 2 |  31:0 | **DESC_ADDR** - GGTT address of the `CTB

Descriptor`_        |

• *

+---+-------+--------------------------------------------------------------+

• *  | 3 |  31:0 | **BUFF_ADDF** - GGTT address of the `CT

Buffer`_             |

• *

+---+-------+--------------------------------------------------------------+

+*

• *

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

• *

+===+=======+==============================================================+

• *  | 0 |    31 | ORIGIN =

GUC_HXG_ORIGIN_GUC_                                 |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 30:28 | TYPE =

GUC_HXG_TYPE_RESPONSE_SUCCESS_                        |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  27:0 | DATA0 =

MBZ                                                  |

• *

+---+-------+--------------------------------------------------------------+

• */

+#define GUC_ACTION_HOST2GUC_REGISTER_CTB        0x4505 // FIXME 0x5200

Why FIXME? AFAICS the specs still says 4505, even if we plan to update at some point I don;t think this deserves a FIXME since nothing is incorrect.

patch was prepared based on draft spec and this FIXME was added just as head-up since we were expecting GuC to make this change soon, but since we are going with GuC 62 that uses 4505, agree, we need drop this FIXME

...

+#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_LEN        (GUC_HXG_REQUEST_MSG_MIN_LEN + 3u) +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_0_MBZ        GUC_HXG_REQUEST_MSG_0_DATA0 +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_MBZ        (0xfffff << 12) +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_TYPE    (0xf << 8) +#define   GUC_CTB_TYPE_HOST2GUC                0u +#define   GUC_CTB_TYPE_GUC2HOST                1u +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_SIZE    (0xff << 0) +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_2_DESC_ADDR    GUC_HXG_REQUEST_MSG_n_DATAn +#define HOST2GUC_REGISTER_CTB_REQUEST_MSG_3_BUFF_ADDR    GUC_HXG_REQUEST_MSG_n_DATAn

The full mask still seems like overkill to me and I still think we should use BIT()/GENMASK() and a _MASK prefix, but not going to block on it.

...

+#define HOST2GUC_REGISTER_CTB_RESPONSE_MSG_LEN        GUC_HXG_RESPONSE_MSG_MIN_LEN +#define HOST2GUC_REGISTER_CTB_RESPONSE_MSG_0_MBZ    GUC_HXG_RESPONSE_MSG_0_DATA0

+/**

• • DOC: HOST2GUC_DEREGISTER_CTB
• • This message is used as part of the `CTB based communication`_

teardown.

• • This message must be sent as `MMIO HXG Message`_.
• *

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

• *

+===+=======+==============================================================+

• *  | 0 |    31 | ORIGIN =

GUC_HXG_ORIGIN_HOST_                                |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 30:28 | TYPE =

GUC_HXG_TYPE_REQUEST_                                 |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 27:16 | DATA0 =

MBZ                                                  |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  15:0 | ACTION = _`GUC_ACTION_HOST2GUC_DEREGISTER_CTB` =

0x5201      |

Specs says 4506

...

• *

+---+-------+--------------------------------------------------------------+

• *  | 1 | 31:12 | RESERVED =

MBZ                                               |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  11:8 | **TYPE** - type of the `CT

Buffer`_                          |

• *  |   |

|                                                              |

• *  |   |       | see

`GUC_ACTION_HOST2GUC_REGISTER_CTB`_                      |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |   7:0 | RESERVED =

MBZ                                               |

• *

+---+-------+--------------------------------------------------------------+

+*

• *

+---+-------+--------------------------------------------------------------+

• *  |   | Bits  |

Description                                                  |

• *

+===+=======+==============================================================+

• *  | 0 |    31 | ORIGIN =

GUC_HXG_ORIGIN_GUC_                                 |

• *  |

+-------+--------------------------------------------------------------+

• *  |   | 30:28 | TYPE =

GUC_HXG_TYPE_RESPONSE_SUCCESS_                        |

• *  |

+-------+--------------------------------------------------------------+

• *  |   |  27:0 | DATA0 =

MBZ                                                  |

• *

+---+-------+--------------------------------------------------------------+

• */

+#define GUC_ACTION_HOST2GUC_DEREGISTER_CTB        0x4506 // FIXME 0x5201

Same comment for the FIXME as above

...

+#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_LEN        (GUC_HXG_REQUEST_MSG_MIN_LEN + 1u) +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_0_MBZ    GUC_HXG_REQUEST_MSG_0_DATA0 +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_MBZ    (0xfffff << 12) +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_TYPE    (0xf << 8) +#define HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_MBZ2    (0xff << 0)

+#define HOST2GUC_DEREGISTER_CTB_RESPONSE_MSG_LEN    GUC_HXG_RESPONSE_MSG_MIN_LEN +#define HOST2GUC_DEREGISTER_CTB_RESPONSE_MSG_0_MBZ    GUC_HXG_RESPONSE_MSG_0_DATA0

+/* legacy definitions */

enum intel_guc_action {       INTEL_GUC_ACTION_DEFAULT = 0x0,       INTEL_GUC_ACTION_REQUEST_PREEMPTION = 0x2, diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h index c2a069a78e01..127b256a662c 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h @@ -112,10 +112,6 @@ static_assert(sizeof(struct guc_ct_buffer_desc) == 64);    * - **flags**, holds various bits to control message handling    */   -/* Type of command transport buffer */ -#define INTEL_GUC_CT_BUFFER_TYPE_SEND    0x0u -#define INTEL_GUC_CT_BUFFER_TYPE_RECV    0x1u

/*    * Definition of the command transport message header (DW0)    * diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 3241a477196f..6a29be779cc9 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -103,9 +103,9 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)   static inline const char *guc_ct_buffer_type_to_str(u32 type)   {       switch (type) { -    case INTEL_GUC_CT_BUFFER_TYPE_SEND: +    case GUC_CTB_TYPE_HOST2GUC:           return "SEND"; -    case INTEL_GUC_CT_BUFFER_TYPE_RECV: +    case GUC_CTB_TYPE_GUC2HOST:           return "RECV";       default:           return "<invalid>"; @@ -136,25 +136,33 @@ static void guc_ct_buffer_init(struct intel_guc_ct_buffer *ctb,       guc_ct_buffer_reset(ctb);   }   -static int guc_action_register_ct_buffer(struct intel_guc *guc, -                     u32 desc_addr, -                     u32 type) +static int guc_action_register_ct_buffer(struct intel_guc *guc, u32 type, +                     u32 desc_addr, u32 buff_addr, u32 size)   { -    u32 action[] = { -        INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER, -        desc_addr, -        sizeof(struct guc_ct_buffer_desc), -        type +    u32 request[HOST2GUC_REGISTER_CTB_REQUEST_MSG_LEN] = { +        FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) | +        FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) | +        FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, GUC_ACTION_HOST2GUC_REGISTER_CTB),

IMO we could use a macro or 2 for the HXG header, to avoid all these lines, which are hard to read. something like:

GUC_HXG_HEADER(origin, type, data, action) \     (FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, origin) | \      FIELD_PREP(GUC_HXG_MSG_0_TYPE, type) | \ FIELD_PREP(GUC_HXG_MSG_0_DATA0, data) | \      FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, action))

H2G_HEADER(type, data, action) \     GUC_HXG_HEADER(GUC_HXG_ORIGIN_HOST, type, data, action)

and then call

H2G_HEADER(GUC_HXG_TYPE_REQUEST, 0, GUC_ACTION_HOST2GUC_REGISTER_CTB)

note that each message type defines its own bits, so helpers macros will likely be per type, but still doable (as the future improvement)

#define H2G_REQUEST(action, data) ... \ GUC_ACTION_##action

H2G_REQUEST(HOST2GUC_REGISTER_CTB, 0)

Not a blocker.

Daniele

...

+        FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_SIZE, size / SZ_4K - 1) | +        FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_1_TYPE, type), +        FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_2_DESC_ADDR, desc_addr), +        FIELD_PREP(HOST2GUC_REGISTER_CTB_REQUEST_MSG_3_BUFF_ADDR, buff_addr),       };   -    /* Can't use generic send(), CT registration must go over MMIO */ -    return intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0); +    GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != GUC_CTB_TYPE_GUC2HOST); +    GEM_BUG_ON(size % SZ_4K);

+    /* CT registration must go over MMIO */ +    return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0);   }   -static int ct_register_buffer(struct intel_guc_ct *ct, u32 desc_addr, u32 type) +static int ct_register_buffer(struct intel_guc_ct *ct, u32 type, +                  u32 desc_addr, u32 buff_addr, u32 size)   { -    int err = guc_action_register_ct_buffer(ct_to_guc(ct), desc_addr, type); +    int err;   +    err = guc_action_register_ct_buffer(ct_to_guc(ct), type, +                        desc_addr, buff_addr, size);       if (unlikely(err))           CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n",                guc_ct_buffer_type_to_str(type), err); @@ -163,14 +171,17 @@ static int ct_register_buffer(struct intel_guc_ct *ct, u32 desc_addr, u32 type)     static int guc_action_deregister_ct_buffer(struct intel_guc *guc, u32 type)   { -    u32 action[] = { -        INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER, -        CTB_OWNER_HOST, -        type +    u32 request[HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_LEN] = { +        FIELD_PREP(GUC_HXG_MSG_0_ORIGIN, GUC_HXG_ORIGIN_HOST) | +        FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) | +        FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION, GUC_ACTION_HOST2GUC_DEREGISTER_CTB), +        FIELD_PREP(HOST2GUC_DEREGISTER_CTB_REQUEST_MSG_1_TYPE, type),       };   -    /* Can't use generic send(), CT deregistration must go over MMIO */ -    return intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0); +    GEM_BUG_ON(type != GUC_CTB_TYPE_HOST2GUC && type != GUC_CTB_TYPE_GUC2HOST);

+    /* CT deregistration must go over MMIO */ +    return intel_guc_send_mmio(guc, request, ARRAY_SIZE(request), NULL, 0);   }     static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type) @@ -258,7 +269,7 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct)   int intel_guc_ct_enable(struct intel_guc_ct *ct)   {       struct intel_guc *guc = ct_to_guc(ct); -    u32 base, cmds; +    u32 base, desc, cmds;       void *blob;       int err;   @@ -274,23 +285,26 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)       GEM_BUG_ON(blob != ct->ctbs.send.desc);         /* (re)initialize descriptors */ -    cmds = base + ptrdiff(ct->ctbs.send.cmds, blob);       guc_ct_buffer_reset(&ct->ctbs.send);

-    cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob);       guc_ct_buffer_reset(&ct->ctbs.recv);         /*        * Register both CT buffers starting with RECV buffer.        * Descriptors are in first half of the blob.        */ -    err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.recv.desc, blob), -                 INTEL_GUC_CT_BUFFER_TYPE_RECV); +    desc = base + ptrdiff(ct->ctbs.recv.desc, blob); +    cmds = base + ptrdiff(ct->ctbs.recv.cmds, blob); +    err = ct_register_buffer(ct, GUC_CTB_TYPE_GUC2HOST, +                 desc, cmds, ct->ctbs.recv.size * 4);

if (unlikely(err))           goto err_out;   -    err = ct_register_buffer(ct, base + ptrdiff(ct->ctbs.send.desc, blob), -                 INTEL_GUC_CT_BUFFER_TYPE_SEND); +    desc = base + ptrdiff(ct->ctbs.send.desc, blob); +    cmds = base + ptrdiff(ct->ctbs.send.cmds, blob); +    err = ct_register_buffer(ct, GUC_CTB_TYPE_HOST2GUC, +                 desc, cmds, ct->ctbs.send.size * 4);

if (unlikely(err))           goto err_deregister;   @@ -299,7 +313,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)       return 0;     err_deregister: -    ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV); +    ct_deregister_buffer(ct, GUC_CTB_TYPE_GUC2HOST);   err_out:       CT_PROBE_ERROR(ct, "Failed to enable CTB (%pe)\n", ERR_PTR(err));       return err; @@ -318,8 +332,8 @@ void intel_guc_ct_disable(struct intel_guc_ct *ct)       ct->enabled = false;         if (intel_guc_is_fw_running(guc)) { -        ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_SEND); -        ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV); +        ct_deregister_buffer(ct, GUC_CTB_TYPE_HOST2GUC); +        ct_deregister_buffer(ct, GUC_CTB_TYPE_GUC2HOST);       }   }

On 6/7/2021 11:03 AM, Matthew Brost wrote:

From: Michal Wajdeczko michal.wajdeczko@intel.com

Format of the CTB messages has changed:

• support for multiple formats
• message fence is now part of the header
• reuse of unified HXG message formats

Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: Piotr Piórkowski piotr.piorkowski@intel.com

---

.../gt/uc/abi/guc_communication_ctb_abi.h | 56 +++++ drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 194 +++++++----------- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 2 +- 3 files changed, 135 insertions(+), 117 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h index 127b256a662c..92660726c094 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_communication_ctb_abi.h @@ -60,6 +60,62 @@ struct guc_ct_buffer_desc { } __packed; static_assert(sizeof(struct guc_ct_buffer_desc) == 64);

+/**

• • DOC: CTB Message
• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31:16 | **FENCE** - message identifier |
• • | +-------+--------------------------------------------------------------+
• • | | 15:12 | **FORMAT** - format of the CTB message |
• • | | | - _`GUC_CTB_FORMAT_HXG` = 0 - see `CTB HXG Message`_ |
• • | +-------+--------------------------------------------------------------+
• • | | 11:8 | **RESERVED** |
• • | +-------+--------------------------------------------------------------+
• • | | 7:0 | **NUM_DWORDS** - length of the CTB message (w/o header) |
• • +---+-------+--------------------------------------------------------------+
• • | 1 | 31:0 | optional (depends on FORMAT) |
• • +---+-------+ |
• • |...| | |
• • +---+-------+ |
• • | n | 31:0 | |
• • +---+-------+--------------------------------------------------------------+
• */

+#define GUC_CTB_MSG_MIN_LEN 1u +#define GUC_CTB_MSG_MAX_LEN 256u +#define GUC_CTB_MSG_0_FENCE (0xffff << 16) +#define GUC_CTB_MSG_0_FORMAT (0xf << 12) +#define GUC_CTB_FORMAT_HXG 0u +#define GUC_CTB_MSG_0_RESERVED (0xf << 8) +#define GUC_CTB_MSG_0_NUM_DWORDS (0xff << 0)

+/**

• • DOC: CTB HXG Message
• • +---+-------+--------------------------------------------------------------+
• • | | Bits | Description |
• • +===+=======+==============================================================+
• • | 0 | 31:16 | FENCE |
• • | +-------+--------------------------------------------------------------+
• • | | 15:12 | FORMAT = GUC_CTB_FORMAT_HXG_ |
• • | +-------+--------------------------------------------------------------+
• • | | 11:8 | RESERVED = MBZ |
• • | +-------+--------------------------------------------------------------+
• • | | 7:0 | NUM_DWORDS = length (in dwords) of the embedded HXG message |
• • +---+-------+--------------------------------------------------------------+
• • | 1 | 31:0 | +--------------------------------------------------------+ |
• • +---+-------+ | | |
• • |...| | | Embedded `HXG Message`_ | |
• • +---+-------+ | | |
• • | n | 31:0 | +--------------------------------------------------------+ |
• • +---+-------+--------------------------------------------------------------+
• */

+#define GUC_CTB_HXG_MSG_MIN_LEN (GUC_CTB_MSG_MIN_LEN + GUC_HXG_MSG_MIN_LEN) +#define GUC_CTB_HXG_MSG_MAX_LEN GUC_CTB_MSG_MAX_LEN

• /**
  • DOC: CTB based communication

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 6a29be779cc9..729f29bc2a57 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -365,24 +365,6 @@ static void write_barrier(struct intel_guc_ct *ct) } }

-/**

• • DOC: CTB Host to GuC request
• • Format of the CTB Host to GuC request message is as follows::

• •  +------------+---------+---------+---------+---------+
    

• •  |   msg[0]   |   [1]   |   [2]   |   ...   |  [n-1]  |
    

• •  +------------+---------+---------+---------+---------+
    

• •  |   MESSAGE  |       MESSAGE PAYLOAD                 |
    

• •  +   HEADER   +---------+---------+---------+---------+
    

• •  |            |    0    |    1    |   ...   |    n    |
    

• •  +============+=========+=========+=========+=========+
    

• •  |  len >= 1  |  FENCE  |     request specific data   |
    

• •  +------+-----+---------+---------+---------+---------+
    

• •               ^-----------------len-------------------^
    

• */
• static int ct_write(struct intel_guc_ct *ct, const u32 *action, u32 len /* in dwords */,

@@ -395,6 +377,7 @@ static int ct_write(struct intel_guc_ct *ct, u32 size = ctb->size; u32 used; u32 header;

• u32 hxg; u32 *cmds = ctb->cmds; unsigned int i;

@@ -425,22 +408,24 @@ static int ct_write(struct intel_guc_ct *ct, return -ENOSPC;

Doesn't the free space math up here need updating, since now we have an extra header dword?

/*

• * Write the message. The format is the following:
  

• * DW0: header (including action code)
  

• * DW1: fence
  

• * DW2+: action data
  

• * dw0: CT header (including fence)
  

• * dw1: HXG header
  

maybe better as:

* dw1+: HXG message

*/

• header = (len << GUC_CT_MSG_LEN_SHIFT) |

•  GUC_CT_MSG_SEND_STATUS |
  

•  (action[0] << GUC_CT_MSG_ACTION_SHIFT);
  

• header = FIELD_PREP(GUC_CTB_MSG_0_FORMAT, GUC_CTB_FORMAT_HXG) |

•  FIELD_PREP(GUC_CTB_MSG_0_NUM_DWORDS, len) |
  

•  FIELD_PREP(GUC_CTB_MSG_0_FENCE, fence);
  

• hxg = FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |

Do we have a case where we might want to use a different type? e.g. a response to a request from GuC?

•      FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
  

• 	 GUC_HXG_REQUEST_MSG_0_DATA0, action[0]);
  

See macro suggestion for the hxg header in previous patch review.

• CT_DEBUG(ct, "writing %*ph %*ph %*ph\n",

•  4, &header, 4, &fence, 4 * (len - 1), &action[1]);
  

• CT_DEBUG(ct, "writing (tail %u) %*ph %*ph %*ph\n",

•  tail, 4, &header, 4, &hxg, 4 * (len - 1), &action[1]);
  
  

  cmds[tail] = header; tail = (tail + 1) % size;

• cmds[tail] = fence;

• cmds[tail] = hxg; tail = (tail + 1) % size;

  for (i = 1; i < len; i++) {

@@ -598,21 +583,6 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len, return ret; }

-static inline unsigned int ct_header_get_len(u32 header) -{

• return (header >> GUC_CT_MSG_LEN_SHIFT) & GUC_CT_MSG_LEN_MASK;

-}

-static inline unsigned int ct_header_get_action(u32 header) -{

• return (header >> GUC_CT_MSG_ACTION_SHIFT) & GUC_CT_MSG_ACTION_MASK;

-}

-static inline bool ct_header_is_response(u32 header) -{

• return !!(header & GUC_CT_MSG_IS_RESPONSE);

-}

• static struct ct_incoming_msg *ct_alloc_msg(u32 num_dwords) { struct ct_incoming_msg *msg;

@@ -675,7 +645,7 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg) head = (head + 1) % size;

/* message len with header */

• len = ct_header_get_len(header) + 1;

• len = FIELD_GET(GUC_CTB_MSG_0_NUM_DWORDS, header) + GUC_CTB_MSG_MIN_LEN; if (unlikely(len > (u32)available)) { CT_ERROR(ct, "Incomplete message %*ph %*ph %*ph\n", 4, &header,

@@ -718,55 +688,24 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg) return -EPIPE; }

-/**

• • DOC: CTB GuC to Host response
• • Format of the CTB GuC to Host response message is as follows::

• •  +------------+---------+---------+---------+---------+---------+
    

• •  |   msg[0]   |   [1]   |   [2]   |   [3]   |   ...   |  [n-1]  |
    

• •  +------------+---------+---------+---------+---------+---------+
    

• •  |   MESSAGE  |       MESSAGE PAYLOAD                           |
    

• •  +   HEADER   +---------+---------+---------+---------+---------+
    

• •  |            |    0    |    1    |    2    |   ...   |    n    |
    

• •  +============+=========+=========+=========+=========+=========+
    

• •  |  len >= 2  |  FENCE  |  STATUS |   response specific data    |
    

• •  +------+-----+---------+---------+---------+---------+---------+
    

• •               ^-----------------------len-----------------------^
    

• */
• static int ct_handle_response(struct intel_guc_ct *ct, struct ct_incoming_msg *response) {
• u32 header = response->msg[0];
• u32 len = ct_header_get_len(header);
• u32 fence;
• u32 status;
• u32 datalen;

• u32 len = FIELD_GET(GUC_CTB_MSG_0_NUM_DWORDS, response->msg[0]);
• u32 fence = FIELD_GET(GUC_CTB_MSG_0_FENCE, response->msg[0]);
• const u32 *hxg = &response->msg[GUC_CTB_MSG_MIN_LEN];

IMO it'd be better to just save the hxg in the msg field. We can save the fence as an extra field in the ct_incoming_msg. That way we won't have to convert from CTB to HXG in multiple places in the code (I count 4 total in this patch).

Daniele

• const u32 *data = &hxg[GUC_HXG_MSG_MIN_LEN];
• u32 datalen = len - GUC_HXG_MSG_MIN_LEN; struct ct_request *req; unsigned long flags; bool found = false; int err = 0;

• GEM_BUG_ON(!ct_header_is_response(header));

• GEM_BUG_ON(len < GUC_HXG_MSG_MIN_LEN);
• GEM_BUG_ON(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, hxg[0]) != GUC_HXG_ORIGIN_GUC);
• GEM_BUG_ON(FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg[0]) != GUC_HXG_TYPE_RESPONSE_SUCCESS &&

•    FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg[0]) != GUC_HXG_TYPE_RESPONSE_FAILURE);
  
  

• /* Response payload shall at least include fence and status */
• if (unlikely(len < 2)) {

• CT_ERROR(ct, "Corrupted response (len %u)\n", len);
  

• return -EPROTO;
  

• }
• fence = response->msg[1];
• status = response->msg[2];
• datalen = len - 2;
• /* Format of the status dword follows HXG header */
• if (unlikely(FIELD_GET(GUC_HXG_MSG_0_ORIGIN, status) != GUC_HXG_ORIGIN_GUC)) {

• CT_ERROR(ct, "Corrupted response (status %#x)\n", status);
  

• return -EPROTO;
  

• }
• CT_DEBUG(ct, "response fence %u status %#x\n", fence, status);

• CT_DEBUG(ct, "response fence %u status %#x\n", fence, hxg[0]);

  spin_lock_irqsave(&ct->requests.lock, flags); list_for_each_entry(req, &ct->requests.pending, link) {

@@ -782,9 +721,9 @@ static int ct_handle_response(struct intel_guc_ct *ct, struct ct_incoming_msg *r err = -EMSGSIZE; } if (datalen)

• 	memcpy(req->response_buf, response->msg + 3, 4 * datalen);
  

• 	memcpy(req->response_buf, data, 4 * datalen);
  

  req->response_len = datalen;

• WRITE_ONCE(req->status, status);
  

• WRITE_ONCE(req->status, hxg[0]);
  

  found = true; break; }

@@ -805,14 +744,16 @@ static int ct_handle_response(struct intel_guc_ct *ct, struct ct_incoming_msg *r static int ct_process_request(struct intel_guc_ct *ct, struct ct_incoming_msg *request) { struct intel_guc *guc = ct_to_guc(ct);

• u32 header, action, len;

• const u32 *hxg; const u32 *payload;
• u32 hxg_len, action, len; int ret;

• header = request->msg[0];
• payload = &request->msg[1];
• action = ct_header_get_action(header);
• len = ct_header_get_len(header);

• hxg = &request->msg[GUC_CTB_MSG_MIN_LEN];

• hxg_len = request->size - GUC_CTB_MSG_MIN_LEN;

• payload = &hxg[GUC_HXG_MSG_MIN_LEN];

• action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, hxg[0]);

• len = hxg_len - GUC_HXG_MSG_MIN_LEN;

  CT_DEBUG(ct, "request %x %*ph\n", action, 4 * len, payload);

@@ -874,29 +815,12 @@ static void ct_incoming_request_worker_func(struct work_struct *w) queue_work(system_unbound_wq, &ct->requests.worker); }

-/**

• • DOC: CTB GuC to Host request
• • Format of the CTB GuC to Host request message is as follows::

• •  +------------+---------+---------+---------+---------+---------+
    

• •  |   msg[0]   |   [1]   |   [2]   |   [3]   |   ...   |  [n-1]  |
    

• •  +------------+---------+---------+---------+---------+---------+
    

• •  |   MESSAGE  |       MESSAGE PAYLOAD                           |
    

• •  +   HEADER   +---------+---------+---------+---------+---------+
    

• •  |            |    0    |    1    |    2    |   ...   |    n    |
    

• •  +============+=========+=========+=========+=========+=========+
    

• •  |     len    |            request specific data                |
    

• •  +------+-----+---------+---------+---------+---------+---------+
    

• •               ^-----------------------len-----------------------^
    

• */

-static int ct_handle_request(struct intel_guc_ct *ct, struct ct_incoming_msg *request) +static int ct_handle_event(struct intel_guc_ct *ct, struct ct_incoming_msg *request) {

• const u32 *hxg = &request->msg[GUC_CTB_MSG_MIN_LEN]; unsigned long flags;

• GEM_BUG_ON(ct_header_is_response(request->msg[0]));

• GEM_BUG_ON(FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg[0]) != GUC_HXG_TYPE_EVENT);

  spin_lock_irqsave(&ct->requests.lock, flags); list_add_tail(&request->link, &ct->requests.incoming);

@@ -906,15 +830,53 @@ static int ct_handle_request(struct intel_guc_ct *ct, struct ct_incoming_msg *re return 0; }

-static void ct_handle_msg(struct intel_guc_ct *ct, struct ct_incoming_msg *msg) +static int ct_handle_hxg(struct intel_guc_ct *ct, struct ct_incoming_msg *msg) {

• u32 header = msg->msg[0];

• u32 origin, type;
• u32 *hxg; int err;

• if (ct_header_is_response(header))

• if (unlikely(msg->size < GUC_CTB_HXG_MSG_MIN_LEN))

• return -EBADMSG;
  

• hxg = &msg->msg[GUC_CTB_MSG_MIN_LEN];
• origin = FIELD_GET(GUC_HXG_MSG_0_ORIGIN, hxg[0]);
• if (unlikely(origin != GUC_HXG_ORIGIN_GUC)) {

• err = -EPROTO;
  

• goto failed;
  

• }
• type = FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg[0]);
• switch (type) {
• case GUC_HXG_TYPE_EVENT:

• err = ct_handle_event(ct, msg);
  

• break;
  

• case GUC_HXG_TYPE_RESPONSE_SUCCESS:
• case GUC_HXG_TYPE_RESPONSE_FAILURE: err = ct_handle_response(ct, msg);

• break;
  

• default:

• err = -EOPNOTSUPP;
  

• }
• if (unlikely(err)) {

+failed:

• CT_ERROR(ct, "Failed to handle HXG message (%pe) %*ph\n",
  

• 	 ERR_PTR(err), 4 * GUC_HXG_MSG_MIN_LEN, hxg);
  

• }
• return err;

+}

+static void ct_handle_msg(struct intel_guc_ct *ct, struct ct_incoming_msg *msg) +{

• u32 format = FIELD_GET(GUC_CTB_MSG_0_FORMAT, msg->msg[0]);
• int err;
• if (format == GUC_CTB_FORMAT_HXG)

• err = ct_handle_hxg(ct, msg);
  

  else

• err = ct_handle_request(ct, msg);
  

• err = -EOPNOTSUPP;
  
  

  if (unlikely(err)) { CT_ERROR(ct, "Failed to process CT message (%pe) %*ph\n",

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h index 905202caaad3..1ae2dde6db93 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h @@ -61,7 +61,7 @@ struct intel_guc_ct { struct tasklet_struct receive_tasklet;

struct {

• u32 last_fence; /* last fence used to send request */
  

• u16 last_fence; /* last fence used to send request */
  
  

  spinlock_t lock; /* protects pending requests list */ struct list_head pending; /* requests waiting for response */