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