Re: [PATCH 2/3] drm/i915/guc: Optimized CTB writes and reads

[Matthew Brost <matthew.brost@intel.com>]

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@intel.com> wrote:
>
>>From: Matthew Brost <matthew.brost@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@intel.com>
>>Signed-off-by: Matthew Brost <matthew.brost@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
>_______________________________________________

Yes.

>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx