| Message ID | 20210607180356.165785-7-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update firmware to v62.0.0 | expand |
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 {
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 { >
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 + */ + +/** + * 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 | + * +---+-------+--------------------------------------------------------------+ + * |...| | 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) + 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) { 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 {