Re: [PATCH 06/13] drm/i915/guc: New definition of the CTB descriptor

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?

• */

+/**

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

• • +---+-------+--------------------------------------------------------------+
• • |...| | 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() ?

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

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 {