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@xxxxxxxxx> >> >> 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@xxxxxxxxx> >> Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx> >> --- >> .../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 { >