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

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

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@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

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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx