[Intel-gfx] [PATCH 2/3] drm/i915/guc: Optimized CTB writes and reads
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Nov 21 11:58:50 UTC 2019
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
> {
> + 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
> {
> + 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
> + * @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
More information about the Intel-gfx
mailing list