[Intel-gfx] [PATCH 2/3] drm/i915/guc: Optimized CTB writes and reads
Matthew Brost
matthew.brost at intel.com
Fri Nov 22 01:29:59 UTC 2019
On Thu, Nov 21, 2019 at 07:56:07AM -0800, Matthew Brost wrote:
>On Thu, Nov 21, 2019 at 12:58:50PM +0100, Michal Wajdeczko wrote:
>>On Thu, 21 Nov 2019 00:56:03 +0100, <John.C.Harrison at intel.com> wrote:
>>
>>>From: Matthew Brost <matthew.brost at intel.com>
>>>
>>>CTB writes are now in the path of command submission and should be
>>>optimized for performance. Rather than reading CTB descriptor values
>>>(e.g. head, tail, size) which could result in accesses across the PCIe
>>>bus, store shadow local copies and only read/write the descriptor
>>>values when absolutely necessary.
>>>
>>>Cc: John Harrison <john.c.harrison at intel.com>
>>>Signed-off-by: Matthew Brost <matthew.brost at intel.com>
>>>---
>>>drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 79 +++++++++++------------
>>>drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 8 +++
>>>2 files changed, 45 insertions(+), 42 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 e50d968b15d5..4d8a4c6afd71 100644
>>>--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>>>+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>>>@@ -68,23 +68,29 @@ 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,
>>>+static void guc_ct_buffer_desc_init(struct intel_guc_ct_buffer *ctb,
>>> u32 cmds_addr, u32 size, u32 owner)
>>
>>as now this function takes ctb instead of desc, it should be renamed
>>or make it separate from guc_ct_buffer_desc_init
>>
>
>Yes, makes sense.
>
>>>{
>>>+ struct guc_ct_buffer_desc *desc = ctb->desc;
>>> CT_DEBUG_DRIVER("CT: desc %p init addr=%#x size=%u owner=%u\n",
>>> desc, cmds_addr, size, owner);
>>> memset(desc, 0, sizeof(*desc));
>>> desc->addr = cmds_addr;
>>>- desc->size = size;
>>>+ ctb->size = desc->size = size;
>>> desc->owner = owner;
>>>+ ctb->tail = 0;
>>>+ ctb->head = 0;
>>>+ ctb->space = CIRC_SPACE(ctb->tail, ctb->head, ctb->size);
>>>}
>>>-static void guc_ct_buffer_desc_reset(struct guc_ct_buffer_desc *desc)
>>>+static void guc_ct_buffer_desc_reset(struct intel_guc_ct_buffer *ctb)
>>
>>same here
>>
>
>Same.
>
>>>{
>>>+ struct guc_ct_buffer_desc *desc = ctb->desc;
>>> CT_DEBUG_DRIVER("CT: desc %p reset head=%u tail=%u\n",
>>> desc, desc->head, desc->tail);
>>>- desc->head = 0;
>>>- desc->tail = 0;
>>>+ ctb->head = desc->head = 0;
>>>+ ctb->tail = desc->tail = 0;
>>>+ ctb->space = CIRC_SPACE(ctb->tail, ctb->head, ctb->size);
>>> desc->is_in_error = 0;
>>>}
>>>@@ -220,7 +226,7 @@ static int ctch_enable(struct intel_guc *guc,
>>> */
>>> for (i = 0; i < ARRAY_SIZE(ctch->ctbs); i++) {
>>> GEM_BUG_ON((i != CTB_SEND) && (i != CTB_RECV));
>>>- guc_ct_buffer_desc_init(ctch->ctbs[i].desc,
>>>+ guc_ct_buffer_desc_init(&ctch->ctbs[i],
>>> base + PAGE_SIZE/4 * i + PAGE_SIZE/2,
>>> PAGE_SIZE/4,
>>> ctch->owner);
>>>@@ -301,32 +307,16 @@ static int ctb_write(struct
>>>intel_guc_ct_buffer *ctb,
>>> bool want_response)
>>>{
>>> struct guc_ct_buffer_desc *desc = ctb->desc;
>>>- u32 head = desc->head / 4; /* in dwords */
>>>- u32 tail = desc->tail / 4; /* in dwords */
>>>- u32 size = desc->size / 4; /* in dwords */
>>>- u32 used; /* in dwords */
>>>+ u32 tail = ctb->tail / 4; /* in dwords */
>>>+ u32 size = ctb->size / 4; /* in dwords */
>>> u32 header;
>>> u32 *cmds = ctb->cmds;
>>> unsigned int i;
>>>- GEM_BUG_ON(desc->size % 4);
>>>- GEM_BUG_ON(desc->head % 4);
>>>- GEM_BUG_ON(desc->tail % 4);
>>>+ GEM_BUG_ON(ctb->size % 4);
>>>+ GEM_BUG_ON(ctb->tail % 4);
>>> GEM_BUG_ON(tail >= size);
>>>- /*
>>>- * tail == head condition indicates empty. GuC FW does not support
>>>- * using up the entire buffer to get tail == head meaning full.
>>>- */
>>>- if (tail < head)
>>>- used = (size - head) + tail;
>>>- else
>>>- used = tail - head;
>>>-
>>>- /* make sure there is a space including extra dw for the fence */
>>>- if (unlikely(used + len + 1 >= size))
>>>- return -ENOSPC;
>>>-
>>> /*
>>> * Write the message. The format is the following:
>>> * DW0: header (including action code)
>>>@@ -354,15 +344,16 @@ static int ctb_write(struct
>>>intel_guc_ct_buffer *ctb,
>>> }
>>> /* now update desc tail (back in bytes) */
>>>- desc->tail = tail * 4;
>>>- GEM_BUG_ON(desc->tail > desc->size);
>>>+ ctb->tail = desc->tail = tail * 4;
>>>+ ctb->space -= (len + 1) * 4;
>>>+ GEM_BUG_ON(ctb->tail > ctb->size);
>>> return 0;
>>>}
>>>/**
>>> * wait_for_ctb_desc_update - Wait for the CT buffer descriptor update.
>>>- * @desc: buffer descriptor
>>>+ * @ctb: ctb buffer
>>> * @fence: response fence
>>> * @status: placeholder for status
>>> *
>>>@@ -376,11 +367,12 @@ static int ctb_write(struct
>>>intel_guc_ct_buffer *ctb,
>>> * * -ETIMEDOUT no response within hardcoded timeout
>>> * * -EPROTO no response, CT buffer is in error
>>> */
>>>-static int wait_for_ctb_desc_update(struct guc_ct_buffer_desc *desc,
>>>+static int wait_for_ctb_desc_update(struct intel_guc_ct_buffer *ctb,
>>> u32 fence,
>>> u32 *status)
>>>{
>>> int err;
>>>+ struct guc_ct_buffer_desc *desc = ctb->desc;
>>> /*
>>> * Fast commands should complete in less than 10us, so sample quickly
>>>@@ -401,7 +393,7 @@ static int wait_for_ctb_desc_update(struct
>>>guc_ct_buffer_desc *desc,
>>> /* Something went wrong with the messaging, try to reset
>>> * the buffer and hope for the best
>>> */
>>>- guc_ct_buffer_desc_reset(desc);
>>>+ guc_ct_buffer_desc_reset(ctb);
>>> err = -EPROTO;
>>> }
>>> }
>>>@@ -446,12 +438,17 @@ static int wait_for_ct_request_update(struct
>>>ct_request *req, u32 *status)
>>> return err;
>>>}
>>>-static inline bool ctb_has_room(struct guc_ct_buffer_desc *desc,
>>>u32 len)
>>>+static inline bool ctb_has_room(struct intel_guc_ct_buffer *ctb,
>>>u32 len)
>>>{
>>>- u32 head = READ_ONCE(desc->head);
>>>+ u32 head;
>>> u32 space;
>>>- space = CIRC_SPACE(desc->tail, head, desc->size);
>>>+ if (ctb->space >= len)
>>>+ return true;
>>>+
>>>+ head = READ_ONCE(ctb->desc->head);
>>>+ space = CIRC_SPACE(ctb->tail, head, ctb->size);
>>>+ ctb->space = space;
>>> return space >= len;
>>>}
>>>@@ -462,7 +459,6 @@ int intel_guc_send_nb(struct intel_guc_ct *ct,
>>>{
>>> struct intel_guc_ct_channel *ctch = &ct->host_channel;
>>> struct intel_guc_ct_buffer *ctb = &ctch->ctbs[CTB_SEND];
>>>- struct guc_ct_buffer_desc *desc = ctb->desc;
>>> int err;
>>> GEM_BUG_ON(!ctch->enabled);
>>>@@ -470,7 +466,7 @@ int intel_guc_send_nb(struct intel_guc_ct *ct,
>>> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
>>> lockdep_assert_held(&ct->send_lock);
>>>- if (unlikely(!ctb_has_room(desc, (len + 1) * 4))) {
>>>+ if (unlikely(!ctb_has_room(ctb, (len + 1) * 4))) {
>>> ct->retry++;
>>> if (ct->retry >= MAX_RETRY)
>>> return -EDEADLK;
>>>@@ -496,7 +492,6 @@ static int ctch_send(struct intel_guc_ct *ct,
>>> u32 *status)
>>>{
>>> struct intel_guc_ct_buffer *ctb = &ctch->ctbs[CTB_SEND];
>>>- struct guc_ct_buffer_desc *desc = ctb->desc;
>>> struct ct_request request;
>>> unsigned long flags;
>>> u32 fence;
>>>@@ -514,7 +509,7 @@ static int ctch_send(struct intel_guc_ct *ct,
>>> */
>>>retry:
>>> spin_lock_irqsave(&ct->send_lock, flags);
>>>- if (unlikely(!ctb_has_room(desc, (len + 1) * 4))) {
>>>+ if (unlikely(!ctb_has_room(ctb, (len + 1) * 4))) {
>>> spin_unlock_irqrestore(&ct->send_lock, flags);
>>> ct->retry++;
>>> if (ct->retry >= MAX_RETRY)
>>>@@ -544,7 +539,7 @@ static int ctch_send(struct intel_guc_ct *ct,
>>> if (response_buf)
>>> err = wait_for_ct_request_update(&request, status);
>>> else
>>>- err = wait_for_ctb_desc_update(desc, fence, status);
>>>+ err = wait_for_ctb_desc_update(ctb, fence, status);
>>> if (unlikely(err))
>>> goto unlink;
>>>@@ -618,9 +613,9 @@ static inline bool ct_header_is_response(u32 header)
>>>static int ctb_read(struct intel_guc_ct_buffer *ctb, u32 *data)
>>>{
>>> struct guc_ct_buffer_desc *desc = ctb->desc;
>>>- u32 head = desc->head / 4; /* in dwords */
>>>+ u32 head = ctb->head / 4; /* in dwords */
>>> u32 tail = desc->tail / 4; /* in dwords */
>>>- u32 size = desc->size / 4; /* in dwords */
>>>+ u32 size = ctb->size / 4; /* in dwords */
>>> u32 *cmds = ctb->cmds;
>>> s32 available; /* in dwords */
>>> unsigned int len;
>>>@@ -664,7 +659,7 @@ static int ctb_read(struct intel_guc_ct_buffer
>>>*ctb, u32 *data)
>>> }
>>> CT_DEBUG_DRIVER("CT: received %*ph\n", 4 * len, data);
>>>- desc->head = head * 4;
>>>+ ctb->head = desc->head = head * 4;
>>> return 0;
>>>}
>>>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 bc670a796bd8..1bff4f0b91f7 100644
>>>--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>>>+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>>>@@ -29,10 +29,18 @@ struct intel_guc;
>>> *
>>> * @desc: pointer to the buffer descriptor
>>> * @cmds: pointer to the commands buffer
>>>+ * @size: local shadow copy of size
>>
>>I would rather expect this as real fixed size,
>>note that size is not expected to change
>>
>
>Yes, it is fixed over the life of the CTB but not all CTBs need to be the same
>size. e.g. The H2G & G2H may and likely will be different sizes with the new Guc
>interface.
>
>>>+ * @head: local shadow copy of head
>>>+ * @tail: local shadow copy of tail
>>>+ * @space: local shadow copy of space
>>> */
>>>struct intel_guc_ct_buffer {
>>> struct guc_ct_buffer_desc *desc;
>>> u32 *cmds;
>>>+ u32 size;
>>>+ u32 tail;
>>>+ u32 head;
>>>+ u32 space;
>>>};
>>>/** Represents pair of command transport buffers.
>>
>>Can we reorder this patch to be first in the series ?
>>
>>Michal
Tried this and I think it makes more sense the way it is. The 'space' value
doesn't have a meaning before the non blocking call is introduced. Also it ends
up changing a bunch of code that is then deleted in the non blocking call patch.
Better to leave it as is.
Matt
>>_______________________________________________
>
>Yes.
>
>>Intel-gfx mailing list
>>Intel-gfx at lists.freedesktop.org
>>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list