On 24.06.2021 09:04, Matthew Brost wrote:

In upcoming patch we will allow more CTB requests to be sent in parallel to the GuC for processing, so we shouldn't assume any more that GuC will always reply without 10ms.

Use bigger value hardcoded value of 1s instead.

v2: Add CONFIG_DRM_I915_GUC_CTB_TIMEOUT config option v3: (Daniel Vetter)

• Use hardcoded value of 1s rather than config option

Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: Michal Wajdeczko michal.wajdeczko@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 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 43409044528e..a59e239497ee 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -474,14 +474,16 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) /* * Fast commands should complete in less than 10us, so sample quickly * up to that length of time, then switch to a slower sleep-wait loop.

• * No GuC command should ever take longer than 10ms.
  

• * No GuC command should ever take longer than 10ms but many GuC
  

• * commands can be inflight at time, so use a 1s timeout on the slower
  

• * sleep-wait loop.
  

  */

#define done \ (FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \ GUC_HXG_ORIGIN_GUC) err = wait_for_us(done, 10); if (err)

• err = wait_for(done, 10);
  

• err = wait_for(done, 1000);
  

can we add #defines for these 10/1000 values? with that

Reviewed-by: Michal Wajdeczko michal.wajdeczko@intel.com

#undef done

if (unlikely(err))

On 24.06.2021 09:04, Matthew Brost wrote:

Improve the error message when a unsolicited CT response is received by printing fence that couldn't be found, the last fence, and all requests with a response outstanding.

Signed-off-by: Matthew Brost matthew.brost@intel.com

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 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 a59e239497ee..07f080ddb9ae 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -730,12 +730,16 @@ static int ct_handle_response(struct intel_guc_ct *ct, struct ct_incoming_msg *r found = true; break; }

• spin_unlock_irqrestore(&ct->requests.lock, flags);
• if (!found) { CT_ERROR(ct, "Unsolicited response (fence %u)\n", fence);

• return -ENOKEY;
  

• CT_ERROR(ct, "Could not find fence=%u, last_fence=%u\n", fence,
  

• 	 ct->requests.last_fence);
  

• list_for_each_entry(req, &ct->requests.pending, link)
  

• 	CT_ERROR(ct, "request %u awaits response\n",
  

• 		 req->fence);
  

not quite sure how listing of awaiting requests could help here (if we suspect that this is a duplicated reply, then we should rather track short list of already processed messages to look there) but since it does not hurt too much, this is:

Reviewed-by: Michal Wajdeczko michal.wajdeczko@intel.com

• err = -ENOKEY;
  

  }

• spin_unlock_irqrestore(&ct->requests.lock, flags);

  if (unlikely(err)) return err;

On 24.06.2021 09:04, Matthew Brost wrote:

With the introduction of non-blocking CTBs more than one CTB can be in flight at a time. Increasing the size of the CTBs should reduce how often software hits the case where no space is available in the CTB buffer.

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 | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 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 07f080ddb9ae..a17215920e58 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -58,11 +58,16 @@ static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct)

•  +--------+-----------------------------------------------+------+
  

• Size of each `CT Buffer`_ must be multiple of 4K.

• • As we don't expect too many messages, for now use minimum sizes.

• • We don't expect too many messages in flight at any time, unless we are
• • using the GuC submission. In that case each request requires a minimum
• • 2 dwords which gives us a maximum 256 queue'd requests. Hopefully this
• • enough space to avoid backpressure on the driver. We increase the size
• • of the receive buffer (relative to the send) to ensure a G2H response
• • CTB has a landing spot.
  */

#define CTB_DESC_SIZE ALIGN(sizeof(struct guc_ct_buffer_desc), SZ_2K) #define CTB_H2G_BUFFER_SIZE (SZ_4K) -#define CTB_G2H_BUFFER_SIZE (SZ_4K) +#define CTB_G2H_BUFFER_SIZE (4 * CTB_H2G_BUFFER_SIZE)

struct ct_request { struct list_head link; @@ -641,7 +646,7 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg) /* beware of buffer wrap case */ if (unlikely(available < 0)) available += size;

• CT_DEBUG(ct, "available %d (%u:%u)\n", available, head, tail);

• CT_DEBUG(ct, "available %d (%u:%u:%u)\n", available, head, tail, size);

CTB size is already printed in intel_guc_ct_init() and is fixed so not sure if repeating it on every ct_read has any benefit

GEM_BUG_ON(available < 0);

header = cmds[head];

On 24.06.2021 17:41, Matthew Brost wrote:

On Thu, Jun 24, 2021 at 03:49:55PM +0200, Michal Wajdeczko wrote:

...

On 24.06.2021 09:04, Matthew Brost wrote:

...

With the introduction of non-blocking CTBs more than one CTB can be in flight at a time. Increasing the size of the CTBs should reduce how often software hits the case where no space is available in the CTB buffer.

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 | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 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 07f080ddb9ae..a17215920e58 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -58,11 +58,16 @@ static inline struct drm_device *ct_to_drm(struct intel_guc_ct *ct)

•  +--------+-----------------------------------------------+------+
  

• Size of each `CT Buffer`_ must be multiple of 4K.

• • As we don't expect too many messages, for now use minimum sizes.

• • We don't expect too many messages in flight at any time, unless we are
• • using the GuC submission. In that case each request requires a minimum
• • 2 dwords which gives us a maximum 256 queue'd requests. Hopefully this
• • enough space to avoid backpressure on the driver. We increase the size
• • of the receive buffer (relative to the send) to ensure a G2H response
• • CTB has a landing spot.
  */

#define CTB_DESC_SIZE ALIGN(sizeof(struct guc_ct_buffer_desc), SZ_2K) #define CTB_H2G_BUFFER_SIZE (SZ_4K) -#define CTB_G2H_BUFFER_SIZE (SZ_4K) +#define CTB_G2H_BUFFER_SIZE (4 * CTB_H2G_BUFFER_SIZE)

struct ct_request { struct list_head link; @@ -641,7 +646,7 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg) /* beware of buffer wrap case */ if (unlikely(available < 0)) available += size;

• CT_DEBUG(ct, "available %d (%u:%u)\n", available, head, tail);

• CT_DEBUG(ct, "available %d (%u:%u:%u)\n", available, head, tail, size);

CTB size is already printed in intel_guc_ct_init() and is fixed so not sure if repeating it on every ct_read has any benefit

I'd say more debug the better and if CT_DEBUG is enabled the logs are very verbose so an extra value doesn't really hurt.

fair, but this doesn't mean we should add little/no value item, anyway since DEBUG_GUC is if off by default, this is:

Reviewed-by: Michal Wajdeczko michal.wajdeczko@intel.com

Matt

...

...

GEM_BUG_ON(available < 0);

header = cmds[head];

On 24.06.2021 09:04, Matthew Brost wrote:

Add non blocking CTB send function, intel_guc_send_nb. GuC submission will send CTBs in the critical path and does not need to wait for these CTBs to complete before moving on, hence the need for this new function.

The non-blocking CTB now must have a flow control mechanism to ensure the buffer isn't overrun. A lazy spin wait is used as we believe the flow control condition should be rare with a properly sized buffer.

The function, intel_guc_send_nb, is exported in this patch but unused. Several patches later in the series make use of this function.

Signed-off-by: John Harrison John.C.Harrison@Intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc.h | 12 +++- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 77 +++++++++++++++++++++-- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 3 +- 3 files changed, 82 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 4abc59f6f3cd..24b1df6ad4ae 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -74,7 +74,15 @@ static inline struct intel_guc *log_to_guc(struct intel_guc_log *log) static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len) {

• return intel_guc_ct_send(&guc->ct, action, len, NULL, 0);

• return intel_guc_ct_send(&guc->ct, action, len, NULL, 0, 0);

+}

+#define INTEL_GUC_SEND_NB BIT(31)

hmm, this flag really belongs to intel_guc_ct_send() so it should be defined as CTB flag near that function declaration

+static +inline int intel_guc_send_nb(struct intel_guc *guc, const u32 *action, u32 len) +{

• return intel_guc_ct_send(&guc->ct, action, len, NULL, 0,

• 		 INTEL_GUC_SEND_NB);
  

}

static inline int @@ -82,7 +90,7 @@ intel_guc_send_and_receive(struct intel_guc *guc, const u32 *action, u32 len, u32 *response_buf, u32 response_buf_size) { return intel_guc_ct_send(&guc->ct, action, len,

• 		 response_buf, response_buf_size);
  

• 		 response_buf, response_buf_size, 0);
  

}

static inline void intel_guc_to_host_event_handler(struct intel_guc *guc) 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 a17215920e58..c9a65d05911f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -3,6 +3,11 @@

• Copyright © 2016-2019 Intel Corporation

*/

+#include <linux/circ_buf.h> +#include <linux/ktime.h> +#include <linux/time64.h> +#include <linux/timekeeping.h>

#include "i915_drv.h" #include "intel_guc_ct.h" #include "gt/intel_gt.h" @@ -373,7 +378,7 @@ static void write_barrier(struct intel_guc_ct *ct) static int ct_write(struct intel_guc_ct *ct, const u32 *action, u32 len /* in dwords */,

•     u32 fence)
  

•     u32 fence, u32 flags)
  

{ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send; struct guc_ct_buffer_desc *desc = ctb->desc; @@ -421,9 +426,13 @@ static int ct_write(struct intel_guc_ct *ct, FIELD_PREP(GUC_CTB_MSG_0_NUM_DWORDS, len) | FIELD_PREP(GUC_CTB_MSG_0_FENCE, fence);

• hxg = FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |

•      FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
  

• 	 GUC_HXG_REQUEST_MSG_0_DATA0, action[0]);
  

• hxg = (flags & INTEL_GUC_SEND_NB) ?

• (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_EVENT) |
  

•  FIELD_PREP(GUC_HXG_EVENT_MSG_0_ACTION |
  

• 	    GUC_HXG_EVENT_MSG_0_DATA0, action[0])) :
  

• (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
  

•  FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
  

• 	    GUC_HXG_REQUEST_MSG_0_DATA0, action[0]));
  

or as we already switched to accept and return whole HXG messages in guc_send_mmio() maybe we should do the same for CTB variant too and instead of using extra flag just let caller to prepare proper HXG header with HXG_EVENT type and then in CTB code just look at this type to make decision which code path to use

note that existing callers should not be impacted, as full HXG header for the REQUEST message looks exactly the same as "action" code alone.

CT_DEBUG(ct, "writing (tail %u) %*ph %*ph %*ph\n", tail, 4, &header, 4, &hxg, 4 * (len - 1), &action[1]); @@ -498,6 +507,46 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) return err; }

+static inline bool h2g_has_room(struct intel_guc_ct_buffer *ctb, u32 len_dw) +{

• struct guc_ct_buffer_desc *desc = ctb->desc;
• u32 head = READ_ONCE(desc->head);
• u32 space;
• space = CIRC_SPACE(desc->tail, head, ctb->size);
• return space >= len_dw;

here you are returning true(1) as has room

+}

+static int ct_send_nb(struct intel_guc_ct *ct,

•       const u32 *action,
  

•       u32 len,
  

•       u32 flags)
  

+{

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
• unsigned long spin_flags;
• u32 fence;
• int ret;
• spin_lock_irqsave(&ctb->lock, spin_flags);
• ret = h2g_has_room(ctb, len + 1);

but here you treat "1" it as en error

and this "1" is GUC_HXG_MSG_MIN_LEN, right ?

• if (unlikely(ret))

• goto out;
  

• fence = ct_get_next_fence(ct);
• ret = ct_write(ct, action, len, fence, flags);
• if (unlikely(ret))

• goto out;
  

• intel_guc_notify(ct_to_guc(ct));

+out:

• spin_unlock_irqrestore(&ctb->lock, spin_flags);
• return ret;

+}

static int ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len, @@ -505,6 +554,7 @@ static int ct_send(struct intel_guc_ct *ct, u32 response_buf_size, u32 *status) {

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.send; struct ct_request request; unsigned long flags; u32 fence;

@@ -514,8 +564,20 @@ static int ct_send(struct intel_guc_ct *ct, GEM_BUG_ON(!len); GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK); GEM_BUG_ON(!response_buf && response_buf_size);

• might_sleep();

• /*

• * We use a lazy spin wait loop here as we believe that if the CT
  

• * buffers are sized correctly the flow control condition should be
  

• * rare.
  

shouldn't we at least try to log such cases with RATE_LIMITED to find out how "rare" it is, or if really unlikely just return -EBUSY as in case of non-blocking send ?

• */
  

+retry: spin_lock_irqsave(&ct->ctbs.send.lock, flags);

• if (unlikely(!h2g_has_room(ctb, len + 1))) {

• spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
  

• cond_resched();
  

• goto retry;
  

• }

  fence = ct_get_next_fence(ct); request.fence = fence;

@@ -527,7 +589,7 @@ static int ct_send(struct intel_guc_ct *ct, list_add_tail(&request.link, &ct->requests.pending); spin_unlock(&ct->requests.lock);

• err = ct_write(ct, action, len, fence);

• err = ct_write(ct, action, len, fence, 0);

  spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);

@@ -569,7 +631,7 @@ static int ct_send(struct intel_guc_ct *ct,

• Command Transport (CT) buffer based GuC send function.

*/ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,

•       u32 *response_buf, u32 response_buf_size)
  

•       u32 *response_buf, u32 response_buf_size, u32 flags)
  

{ u32 status = ~0; /* undefined */ int ret; @@ -579,6 +641,9 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len, return -ENODEV; }

• if (flags & INTEL_GUC_SEND_NB)

• return ct_send_nb(ct, action, len, flags);
  

• ret = ct_send(ct, action, len, response_buf, response_buf_size, &status); if (unlikely(ret < 0)) { CT_ERROR(ct, "Sending action %#x failed (err=%d status=%#X)\n",

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 1ae2dde6db93..eb69263324ba 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h @@ -42,7 +42,6 @@ struct intel_guc_ct_buffer { bool broken; };

/** Top-level structure for Command Transport related data

• Includes a pair of CT buffers for bi-directional communication and tracking

@@ -88,7 +87,7 @@ static inline bool intel_guc_ct_enabled(struct intel_guc_ct *ct) }

int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,

•       u32 *response_buf, u32 response_buf_size);
  

•       u32 *response_buf, u32 response_buf_size, u32 flags);
  

void intel_guc_ct_event_handler(struct intel_guc_ct *ct);

#endif /* _INTEL_GUC_CT_H_ */

On 24.06.2021 17:49, Matthew Brost wrote:

On Thu, Jun 24, 2021 at 04:48:32PM +0200, Michal Wajdeczko wrote:

...

On 24.06.2021 09:04, Matthew Brost wrote:

...

Add non blocking CTB send function, intel_guc_send_nb. GuC submission will send CTBs in the critical path and does not need to wait for these CTBs to complete before moving on, hence the need for this new function.

The non-blocking CTB now must have a flow control mechanism to ensure the buffer isn't overrun. A lazy spin wait is used as we believe the flow control condition should be rare with a properly sized buffer.

The function, intel_guc_send_nb, is exported in this patch but unused. Several patches later in the series make use of this function.

Signed-off-by: John Harrison John.C.Harrison@Intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc.h | 12 +++- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 77 +++++++++++++++++++++-- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 3 +- 3 files changed, 82 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 4abc59f6f3cd..24b1df6ad4ae 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -74,7 +74,15 @@ static inline struct intel_guc *log_to_guc(struct intel_guc_log *log) static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len) {

• return intel_guc_ct_send(&guc->ct, action, len, NULL, 0);

• return intel_guc_ct_send(&guc->ct, action, len, NULL, 0, 0);

+}

+#define INTEL_GUC_SEND_NB BIT(31)

hmm, this flag really belongs to intel_guc_ct_send() so it should be defined as CTB flag near that function declaration

I can move this up a few lines.

...

...

+static +inline int intel_guc_send_nb(struct intel_guc *guc, const u32 *action, u32 len) +{

• return intel_guc_ct_send(&guc->ct, action, len, NULL, 0,

• 		 INTEL_GUC_SEND_NB);
  

}

static inline int @@ -82,7 +90,7 @@ intel_guc_send_and_receive(struct intel_guc *guc, const u32 *action, u32 len, u32 *response_buf, u32 response_buf_size) { return intel_guc_ct_send(&guc->ct, action, len,

• 		 response_buf, response_buf_size);
  

• 		 response_buf, response_buf_size, 0);
  

}

static inline void intel_guc_to_host_event_handler(struct intel_guc *guc) 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 a17215920e58..c9a65d05911f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -3,6 +3,11 @@

• Copyright © 2016-2019 Intel Corporation

*/

+#include <linux/circ_buf.h> +#include <linux/ktime.h> +#include <linux/time64.h> +#include <linux/timekeeping.h>

#include "i915_drv.h" #include "intel_guc_ct.h" #include "gt/intel_gt.h" @@ -373,7 +378,7 @@ static void write_barrier(struct intel_guc_ct *ct) static int ct_write(struct intel_guc_ct *ct, const u32 *action, u32 len /* in dwords */,

•     u32 fence)
  

•     u32 fence, u32 flags)
  

{ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send; struct guc_ct_buffer_desc *desc = ctb->desc; @@ -421,9 +426,13 @@ static int ct_write(struct intel_guc_ct *ct, FIELD_PREP(GUC_CTB_MSG_0_NUM_DWORDS, len) | FIELD_PREP(GUC_CTB_MSG_0_FENCE, fence);

• hxg = FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |

•      FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
  

• 	 GUC_HXG_REQUEST_MSG_0_DATA0, action[0]);
  

• hxg = (flags & INTEL_GUC_SEND_NB) ?

• (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_EVENT) |
  

•  FIELD_PREP(GUC_HXG_EVENT_MSG_0_ACTION |
  

• 	    GUC_HXG_EVENT_MSG_0_DATA0, action[0])) :
  

• (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
  

•  FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
  

• 	    GUC_HXG_REQUEST_MSG_0_DATA0, action[0]));
  

or as we already switched to accept and return whole HXG messages in guc_send_mmio() maybe we should do the same for CTB variant too and instead of using extra flag just let caller to prepare proper HXG header with HXG_EVENT type and then in CTB code just look at this type to make decision which code path to use

Not sure I follow. Anyways could this be done in a follow up by you if want this change.

...

note that existing callers should not be impacted, as full HXG header for the REQUEST message looks exactly the same as "action" code alone.

...

CT_DEBUG(ct, "writing (tail %u) %*ph %*ph %*ph\n", tail, 4, &header, 4, &hxg, 4 * (len - 1), &action[1]); @@ -498,6 +507,46 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) return err; }

+static inline bool h2g_has_room(struct intel_guc_ct_buffer *ctb, u32 len_dw) +{

• struct guc_ct_buffer_desc *desc = ctb->desc;
• u32 head = READ_ONCE(desc->head);
• u32 space;
• space = CIRC_SPACE(desc->tail, head, ctb->size);
• return space >= len_dw;

here you are returning true(1) as has room

See below.

...

...

+}

+static int ct_send_nb(struct intel_guc_ct *ct,

•       const u32 *action,
  

•       u32 len,
  

•       u32 flags)
  

+{

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
• unsigned long spin_flags;
• u32 fence;
• int ret;
• spin_lock_irqsave(&ctb->lock, spin_flags);
• ret = h2g_has_room(ctb, len + 1);

but here you treat "1" it as en error

Yes, this patch is broken but fixed in a follow up one. Regardless I'll fix this patch in place.

...

and this "1" is GUC_HXG_MSG_MIN_LEN, right ?

Not exactly. This is following how ct_send() uses the action + len field. Action[0] field goes in the HXG header and extra + 1 is for the CT header.

well, "len" already counts "action" so by treating input as full HXG message (including HXG header) will make it cleaner

we can try do it later but by doing it right now we would avoid introducing this send_nb() function and deprecating them long term again

...

...

• if (unlikely(ret))

• goto out;
  

• fence = ct_get_next_fence(ct);
• ret = ct_write(ct, action, len, fence, flags);
• if (unlikely(ret))

• goto out;
  

• intel_guc_notify(ct_to_guc(ct));

+out:

• spin_unlock_irqrestore(&ctb->lock, spin_flags);
• return ret;

+}

static int ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len, @@ -505,6 +554,7 @@ static int ct_send(struct intel_guc_ct *ct, u32 response_buf_size, u32 *status) {

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.send; struct ct_request request; unsigned long flags; u32 fence;

@@ -514,8 +564,20 @@ static int ct_send(struct intel_guc_ct *ct, GEM_BUG_ON(!len); GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK); GEM_BUG_ON(!response_buf && response_buf_size);

• might_sleep();

• /*

• * We use a lazy spin wait loop here as we believe that if the CT
  

• * buffers are sized correctly the flow control condition should be
  

• * rare.
  

shouldn't we at least try to log such cases with RATE_LIMITED to find out how "rare" it is, or if really unlikely just return -EBUSY as in case of non-blocking send ?

Definitely not return -EBUSY as this a blocking call. Perhaps we can log

blocking calls still can fail for various reasons, full CTB is one of them, and if we return error (now broken) for non-blocking variant then we should do the same for blocking variant as well and let the caller decide about next steps

this, but IGTs likely can hit rather easily. It really is only interesting if real workloads hit this. Regardless that can be a follow up.

if we hide retry in a silent loop then we will not find it out if we hit this condition (IGT or real WL) or not

Matt

...

...

• */
  

+retry: spin_lock_irqsave(&ct->ctbs.send.lock, flags);

• if (unlikely(!h2g_has_room(ctb, len + 1))) {

• spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
  

• cond_resched();
  

• goto retry;
  

• }

  fence = ct_get_next_fence(ct); request.fence = fence;

@@ -527,7 +589,7 @@ static int ct_send(struct intel_guc_ct *ct, list_add_tail(&request.link, &ct->requests.pending); spin_unlock(&ct->requests.lock);

• err = ct_write(ct, action, len, fence);

• err = ct_write(ct, action, len, fence, 0);

  spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);

@@ -569,7 +631,7 @@ static int ct_send(struct intel_guc_ct *ct,

• Command Transport (CT) buffer based GuC send function.

*/ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,

•       u32 *response_buf, u32 response_buf_size)
  

•       u32 *response_buf, u32 response_buf_size, u32 flags)
  

{ u32 status = ~0; /* undefined */ int ret; @@ -579,6 +641,9 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len, return -ENODEV; }

• if (flags & INTEL_GUC_SEND_NB)

• return ct_send_nb(ct, action, len, flags);
  

• ret = ct_send(ct, action, len, response_buf, response_buf_size, &status); if (unlikely(ret < 0)) { CT_ERROR(ct, "Sending action %#x failed (err=%d status=%#X)\n",

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 1ae2dde6db93..eb69263324ba 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h @@ -42,7 +42,6 @@ struct intel_guc_ct_buffer { bool broken; };

/** Top-level structure for Command Transport related data

• Includes a pair of CT buffers for bi-directional communication and tracking

@@ -88,7 +87,7 @@ static inline bool intel_guc_ct_enabled(struct intel_guc_ct *ct) }

int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,

•       u32 *response_buf, u32 response_buf_size);
  

•       u32 *response_buf, u32 response_buf_size, u32 flags);
  

void intel_guc_ct_event_handler(struct intel_guc_ct *ct);

#endif /* _INTEL_GUC_CT_H_ */

On 25.06.2021 00:41, Matthew Brost wrote:

On Thu, Jun 24, 2021 at 07:02:18PM +0200, Michal Wajdeczko wrote:

...

On 24.06.2021 17:49, Matthew Brost wrote:

...

On Thu, Jun 24, 2021 at 04:48:32PM +0200, Michal Wajdeczko wrote:

...

On 24.06.2021 09:04, Matthew Brost wrote:

...

Add non blocking CTB send function, intel_guc_send_nb. GuC submission will send CTBs in the critical path and does not need to wait for these CTBs to complete before moving on, hence the need for this new function.

The non-blocking CTB now must have a flow control mechanism to ensure the buffer isn't overrun. A lazy spin wait is used as we believe the flow control condition should be rare with a properly sized buffer.

The function, intel_guc_send_nb, is exported in this patch but unused. Several patches later in the series make use of this function.

Signed-off-by: John Harrison John.C.Harrison@Intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc.h | 12 +++- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 77 +++++++++++++++++++++-- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 3 +- 3 files changed, 82 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 4abc59f6f3cd..24b1df6ad4ae 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -74,7 +74,15 @@ static inline struct intel_guc *log_to_guc(struct intel_guc_log *log) static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len) {

• return intel_guc_ct_send(&guc->ct, action, len, NULL, 0);

• return intel_guc_ct_send(&guc->ct, action, len, NULL, 0, 0);

+}

+#define INTEL_GUC_SEND_NB BIT(31)

hmm, this flag really belongs to intel_guc_ct_send() so it should be defined as CTB flag near that function declaration

I can move this up a few lines.

...

...

+static +inline int intel_guc_send_nb(struct intel_guc *guc, const u32 *action, u32 len) +{

• return intel_guc_ct_send(&guc->ct, action, len, NULL, 0,

• 		 INTEL_GUC_SEND_NB);
  

}

static inline int @@ -82,7 +90,7 @@ intel_guc_send_and_receive(struct intel_guc *guc, const u32 *action, u32 len, u32 *response_buf, u32 response_buf_size) { return intel_guc_ct_send(&guc->ct, action, len,

• 		 response_buf, response_buf_size);
  

• 		 response_buf, response_buf_size, 0);
  

}

static inline void intel_guc_to_host_event_handler(struct intel_guc *guc) 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 a17215920e58..c9a65d05911f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -3,6 +3,11 @@

• Copyright © 2016-2019 Intel Corporation

*/

+#include <linux/circ_buf.h> +#include <linux/ktime.h> +#include <linux/time64.h> +#include <linux/timekeeping.h>

#include "i915_drv.h" #include "intel_guc_ct.h" #include "gt/intel_gt.h" @@ -373,7 +378,7 @@ static void write_barrier(struct intel_guc_ct *ct) static int ct_write(struct intel_guc_ct *ct, const u32 *action, u32 len /* in dwords */,

•     u32 fence)
  

•     u32 fence, u32 flags)
  

{ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send; struct guc_ct_buffer_desc *desc = ctb->desc; @@ -421,9 +426,13 @@ static int ct_write(struct intel_guc_ct *ct, FIELD_PREP(GUC_CTB_MSG_0_NUM_DWORDS, len) | FIELD_PREP(GUC_CTB_MSG_0_FENCE, fence);

• hxg = FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |

•      FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
  

• 	 GUC_HXG_REQUEST_MSG_0_DATA0, action[0]);
  

• hxg = (flags & INTEL_GUC_SEND_NB) ?

• (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_EVENT) |
  

•  FIELD_PREP(GUC_HXG_EVENT_MSG_0_ACTION |
  

• 	    GUC_HXG_EVENT_MSG_0_DATA0, action[0])) :
  

• (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
  

•  FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
  

• 	    GUC_HXG_REQUEST_MSG_0_DATA0, action[0]));
  

or as we already switched to accept and return whole HXG messages in guc_send_mmio() maybe we should do the same for CTB variant too and instead of using extra flag just let caller to prepare proper HXG header with HXG_EVENT type and then in CTB code just look at this type to make decision which code path to use

Not sure I follow. Anyways could this be done in a follow up by you if want this change.

...

note that existing callers should not be impacted, as full HXG header for the REQUEST message looks exactly the same as "action" code alone.

...

CT_DEBUG(ct, "writing (tail %u) %*ph %*ph %*ph\n", tail, 4, &header, 4, &hxg, 4 * (len - 1), &action[1]); @@ -498,6 +507,46 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) return err; }

+static inline bool h2g_has_room(struct intel_guc_ct_buffer *ctb, u32 len_dw) +{

• struct guc_ct_buffer_desc *desc = ctb->desc;
• u32 head = READ_ONCE(desc->head);
• u32 space;
• space = CIRC_SPACE(desc->tail, head, ctb->size);
• return space >= len_dw;

here you are returning true(1) as has room

See below.

...

...

+}

+static int ct_send_nb(struct intel_guc_ct *ct,

•       const u32 *action,
  

•       u32 len,
  

•       u32 flags)
  

+{

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
• unsigned long spin_flags;
• u32 fence;
• int ret;
• spin_lock_irqsave(&ctb->lock, spin_flags);
• ret = h2g_has_room(ctb, len + 1);

but here you treat "1" it as en error

Yes, this patch is broken but fixed in a follow up one. Regardless I'll fix this patch in place.

...

and this "1" is GUC_HXG_MSG_MIN_LEN, right ?

Not exactly. This is following how ct_send() uses the action + len field. Action[0] field goes in the HXG header and extra + 1 is for the CT header.

well, "len" already counts "action" so by treating input as full HXG message (including HXG header) will make it cleaner

Yes, I know. See above. To me GUC_HXG_MSG_MIN_LEN makes zero sense and it is worse than adding + 1. This + 1 accounts for the CT header not the HXG header. If any we add a new define, GUC_CT_HDR_LEN, and add that.

you mean GUC_CTB_MSG_MIN_LEN ? it's already there [1]

[1] https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/i915/gt/uc/abi...

Matt

...

we can try do it later but by doing it right now we would avoid introducing this send_nb() function and deprecating them long term again

...

...

...

• if (unlikely(ret))

• goto out;
  

• fence = ct_get_next_fence(ct);
• ret = ct_write(ct, action, len, fence, flags);
• if (unlikely(ret))

• goto out;
  

• intel_guc_notify(ct_to_guc(ct));

+out:

• spin_unlock_irqrestore(&ctb->lock, spin_flags);
• return ret;

+}

static int ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len, @@ -505,6 +554,7 @@ static int ct_send(struct intel_guc_ct *ct, u32 response_buf_size, u32 *status) {

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.send; struct ct_request request; unsigned long flags; u32 fence;

@@ -514,8 +564,20 @@ static int ct_send(struct intel_guc_ct *ct, GEM_BUG_ON(!len); GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK); GEM_BUG_ON(!response_buf && response_buf_size);

• might_sleep();

• /*

• * We use a lazy spin wait loop here as we believe that if the CT
  

• * buffers are sized correctly the flow control condition should be
  

• * rare.
  

shouldn't we at least try to log such cases with RATE_LIMITED to find out how "rare" it is, or if really unlikely just return -EBUSY as in case of non-blocking send ?

Definitely not return -EBUSY as this a blocking call. Perhaps we can log

blocking calls still can fail for various reasons, full CTB is one of them, and if we return error (now broken) for non-blocking variant then we should do the same for blocking variant as well and let the caller decide about next steps

...

this, but IGTs likely can hit rather easily. It really is only interesting if real workloads hit this. Regardless that can be a follow up.

if we hide retry in a silent loop then we will not find it out if we hit this condition (IGT or real WL) or not

...

Matt

...

...

• */
  

+retry: spin_lock_irqsave(&ct->ctbs.send.lock, flags);

• if (unlikely(!h2g_has_room(ctb, len + 1))) {

• spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
  

• cond_resched();
  

• goto retry;
  

• }

  fence = ct_get_next_fence(ct); request.fence = fence;

@@ -527,7 +589,7 @@ static int ct_send(struct intel_guc_ct *ct, list_add_tail(&request.link, &ct->requests.pending); spin_unlock(&ct->requests.lock);

• err = ct_write(ct, action, len, fence);

• err = ct_write(ct, action, len, fence, 0);

  spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);

@@ -569,7 +631,7 @@ static int ct_send(struct intel_guc_ct *ct,

• Command Transport (CT) buffer based GuC send function.

*/ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,

•       u32 *response_buf, u32 response_buf_size)
  

•       u32 *response_buf, u32 response_buf_size, u32 flags)
  

{ u32 status = ~0; /* undefined */ int ret; @@ -579,6 +641,9 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len, return -ENODEV; }

• if (flags & INTEL_GUC_SEND_NB)

• return ct_send_nb(ct, action, len, flags);
  

• ret = ct_send(ct, action, len, response_buf, response_buf_size, &status); if (unlikely(ret < 0)) { CT_ERROR(ct, "Sending action %#x failed (err=%d status=%#X)\n",

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 1ae2dde6db93..eb69263324ba 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h @@ -42,7 +42,6 @@ struct intel_guc_ct_buffer { bool broken; };

/** Top-level structure for Command Transport related data

• Includes a pair of CT buffers for bi-directional communication and tracking

@@ -88,7 +87,7 @@ static inline bool intel_guc_ct_enabled(struct intel_guc_ct *ct) }

int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,

•       u32 *response_buf, u32 response_buf_size);
  

•       u32 *response_buf, u32 response_buf_size, u32 flags);
  

void intel_guc_ct_event_handler(struct intel_guc_ct *ct);

#endif /* _INTEL_GUC_CT_H_ */

On Thu, Jun 24, 2021 at 07:02:18PM +0200, Michal Wajdeczko wrote:

On 24.06.2021 17:49, Matthew Brost wrote:

...

On Thu, Jun 24, 2021 at 04:48:32PM +0200, Michal Wajdeczko wrote:

...

On 24.06.2021 09:04, Matthew Brost wrote:

...

Add non blocking CTB send function, intel_guc_send_nb. GuC submission will send CTBs in the critical path and does not need to wait for these CTBs to complete before moving on, hence the need for this new function.

The non-blocking CTB now must have a flow control mechanism to ensure the buffer isn't overrun. A lazy spin wait is used as we believe the flow control condition should be rare with a properly sized buffer.

The function, intel_guc_send_nb, is exported in this patch but unused. Several patches later in the series make use of this function.

Signed-off-by: John Harrison John.C.Harrison@Intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc.h | 12 +++- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 77 +++++++++++++++++++++-- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 3 +- 3 files changed, 82 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 4abc59f6f3cd..24b1df6ad4ae 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -74,7 +74,15 @@ static inline struct intel_guc *log_to_guc(struct intel_guc_log *log) static inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len) {

• return intel_guc_ct_send(&guc->ct, action, len, NULL, 0);

• return intel_guc_ct_send(&guc->ct, action, len, NULL, 0, 0);

+}

+#define INTEL_GUC_SEND_NB BIT(31)

hmm, this flag really belongs to intel_guc_ct_send() so it should be defined as CTB flag near that function declaration

I can move this up a few lines.

...

...

+static +inline int intel_guc_send_nb(struct intel_guc *guc, const u32 *action, u32 len) +{

• return intel_guc_ct_send(&guc->ct, action, len, NULL, 0,

• 		 INTEL_GUC_SEND_NB);
  

}

static inline int @@ -82,7 +90,7 @@ intel_guc_send_and_receive(struct intel_guc *guc, const u32 *action, u32 len, u32 *response_buf, u32 response_buf_size) { return intel_guc_ct_send(&guc->ct, action, len,

• 		 response_buf, response_buf_size);
  

• 		 response_buf, response_buf_size, 0);
  

}

static inline void intel_guc_to_host_event_handler(struct intel_guc *guc) 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 a17215920e58..c9a65d05911f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -3,6 +3,11 @@

• Copyright © 2016-2019 Intel Corporation

*/

+#include <linux/circ_buf.h> +#include <linux/ktime.h> +#include <linux/time64.h> +#include <linux/timekeeping.h>

#include "i915_drv.h" #include "intel_guc_ct.h" #include "gt/intel_gt.h" @@ -373,7 +378,7 @@ static void write_barrier(struct intel_guc_ct *ct) static int ct_write(struct intel_guc_ct *ct, const u32 *action, u32 len /* in dwords */,

•     u32 fence)
  

•     u32 fence, u32 flags)
  

{ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send; struct guc_ct_buffer_desc *desc = ctb->desc; @@ -421,9 +426,13 @@ static int ct_write(struct intel_guc_ct *ct, FIELD_PREP(GUC_CTB_MSG_0_NUM_DWORDS, len) | FIELD_PREP(GUC_CTB_MSG_0_FENCE, fence);

• hxg = FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |

•      FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
  

• 	 GUC_HXG_REQUEST_MSG_0_DATA0, action[0]);
  

• hxg = (flags & INTEL_GUC_SEND_NB) ?

• (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_EVENT) |
  

•  FIELD_PREP(GUC_HXG_EVENT_MSG_0_ACTION |
  

• 	    GUC_HXG_EVENT_MSG_0_DATA0, action[0])) :
  

• (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
  

•  FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
  

• 	    GUC_HXG_REQUEST_MSG_0_DATA0, action[0]));
  

or as we already switched to accept and return whole HXG messages in guc_send_mmio() maybe we should do the same for CTB variant too and instead of using extra flag just let caller to prepare proper HXG header with HXG_EVENT type and then in CTB code just look at this type to make decision which code path to use

Not sure I follow. Anyways could this be done in a follow up by you if want this change.

...

note that existing callers should not be impacted, as full HXG header for the REQUEST message looks exactly the same as "action" code alone.

...

CT_DEBUG(ct, "writing (tail %u) %*ph %*ph %*ph\n", tail, 4, &header, 4, &hxg, 4 * (len - 1), &action[1]); @@ -498,6 +507,46 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) return err; }

+static inline bool h2g_has_room(struct intel_guc_ct_buffer *ctb, u32 len_dw) +{

• struct guc_ct_buffer_desc *desc = ctb->desc;
• u32 head = READ_ONCE(desc->head);
• u32 space;
• space = CIRC_SPACE(desc->tail, head, ctb->size);
• return space >= len_dw;

here you are returning true(1) as has room

See below.

...

...

+}

+static int ct_send_nb(struct intel_guc_ct *ct,

•       const u32 *action,
  

•       u32 len,
  

•       u32 flags)
  

+{

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
• unsigned long spin_flags;
• u32 fence;
• int ret;
• spin_lock_irqsave(&ctb->lock, spin_flags);
• ret = h2g_has_room(ctb, len + 1);

but here you treat "1" it as en error

Yes, this patch is broken but fixed in a follow up one. Regardless I'll fix this patch in place.

...

and this "1" is GUC_HXG_MSG_MIN_LEN, right ?

Not exactly. This is following how ct_send() uses the action + len field. Action[0] field goes in the HXG header and extra + 1 is for the CT header.

well, "len" already counts "action" so by treating input as full HXG message (including HXG header) will make it cleaner

we can try do it later but by doing it right now we would avoid introducing this send_nb() function and deprecating them long term again

...

...

...

• if (unlikely(ret))

• goto out;
  

• fence = ct_get_next_fence(ct);
• ret = ct_write(ct, action, len, fence, flags);
• if (unlikely(ret))

• goto out;
  

• intel_guc_notify(ct_to_guc(ct));

+out:

• spin_unlock_irqrestore(&ctb->lock, spin_flags);
• return ret;

+}

static int ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len, @@ -505,6 +554,7 @@ static int ct_send(struct intel_guc_ct *ct, u32 response_buf_size, u32 *status) {

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.send; struct ct_request request; unsigned long flags; u32 fence;

@@ -514,8 +564,20 @@ static int ct_send(struct intel_guc_ct *ct, GEM_BUG_ON(!len); GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK); GEM_BUG_ON(!response_buf && response_buf_size);

• might_sleep();

• /*

• * We use a lazy spin wait loop here as we believe that if the CT
  

• * buffers are sized correctly the flow control condition should be
  

• * rare.
  

shouldn't we at least try to log such cases with RATE_LIMITED to find out how "rare" it is, or if really unlikely just return -EBUSY as in case of non-blocking send ?

Definitely not return -EBUSY as this a blocking call. Perhaps we can log

blocking calls still can fail for various reasons, full CTB is one of them, and if we return error (now broken) for non-blocking variant then we should do the same for blocking variant as well and let the caller decide about next steps

And have to rewrite reset of the stack with the new behavior, that seems wrong. This function is allowed to block, so let it.

If you want to do this but all means go ahead but I'll likely NACK it as over engineered.

...

this, but IGTs likely can hit rather easily. It really is only interesting if real workloads hit this. Regardless that can be a follow up.

if we hide retry in a silent loop then we will not find it out if we hit this condition (IGT or real WL) or not

We don't care if this is hit in IGTs as this isn't a real world use case and will spam the dmesg if we hit this. I can make a note of this as an open and we can revisit later.

Matt

...

Matt

...

...

• */
  

+retry: spin_lock_irqsave(&ct->ctbs.send.lock, flags);

• if (unlikely(!h2g_has_room(ctb, len + 1))) {

• spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
  

• cond_resched();
  

• goto retry;
  

• }

  fence = ct_get_next_fence(ct); request.fence = fence;

@@ -527,7 +589,7 @@ static int ct_send(struct intel_guc_ct *ct, list_add_tail(&request.link, &ct->requests.pending); spin_unlock(&ct->requests.lock);

• err = ct_write(ct, action, len, fence);

• err = ct_write(ct, action, len, fence, 0);

  spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);

@@ -569,7 +631,7 @@ static int ct_send(struct intel_guc_ct *ct,

• Command Transport (CT) buffer based GuC send function.

*/ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,

•       u32 *response_buf, u32 response_buf_size)
  

•       u32 *response_buf, u32 response_buf_size, u32 flags)
  

{ u32 status = ~0; /* undefined */ int ret; @@ -579,6 +641,9 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len, return -ENODEV; }

• if (flags & INTEL_GUC_SEND_NB)

• return ct_send_nb(ct, action, len, flags);
  

• ret = ct_send(ct, action, len, response_buf, response_buf_size, &status); if (unlikely(ret < 0)) { CT_ERROR(ct, "Sending action %#x failed (err=%d status=%#X)\n",

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 1ae2dde6db93..eb69263324ba 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h @@ -42,7 +42,6 @@ struct intel_guc_ct_buffer { bool broken; };

/** Top-level structure for Command Transport related data

• Includes a pair of CT buffers for bi-directional communication and tracking

@@ -88,7 +87,7 @@ static inline bool intel_guc_ct_enabled(struct intel_guc_ct *ct) }

int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,

•       u32 *response_buf, u32 response_buf_size);
  

•       u32 *response_buf, u32 response_buf_size, u32 flags);
  

void intel_guc_ct_event_handler(struct intel_guc_ct *ct);

#endif /* _INTEL_GUC_CT_H_ */

On 24.06.2021 09:04, Matthew Brost wrote:

Implement a stall timer which fails H2G CTBs once a period of time with no forward progress is reached to prevent deadlock.

Also update to ct_write to return -EIO rather than -EPIPE on a corrupted descriptor.

by doing so you will have the same error code for two different problems:

a) corrupted CTB descriptor (definitely unrecoverable) b) long stall in CTB processing (still recoverable)

while caller is explicitly instructed to retry only on:

c) temporary stall in CTB processing (likely recoverable)

so why do we want to limit our diagnostics?

Signed-off-by: John Harrison John.C.Harrison@Intel.com Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 47 +++++++++++++++++++++-- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 4 ++ 2 files changed, 48 insertions(+), 3 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 c9a65d05911f..27ec30b5ef47 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -319,6 +319,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct) goto err_deregister;

ct->enabled = true;

• ct->stall_time = KTIME_MAX;

  return 0;

@@ -392,7 +393,7 @@ static int ct_write(struct intel_guc_ct *ct, unsigned int i;

if (unlikely(ctb->broken))

• return -EPIPE;
  

• return -EIO;
  
  

  if (unlikely(desc->status)) goto corrupted;

@@ -464,7 +465,7 @@ static int ct_write(struct intel_guc_ct *ct, CT_ERROR(ct, "Corrupted descriptor head=%u tail=%u status=%#x\n", desc->head, desc->tail, desc->status); ctb->broken = true;

• return -EPIPE;

• return -EIO;

}

/** @@ -507,6 +508,18 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status) return err; }

+#define GUC_CTB_TIMEOUT_MS 1500

it's 150% of core CTB timeout, maybe we should correlate them ?

+static inline bool ct_deadlocked(struct intel_guc_ct *ct) +{

• long timeout = GUC_CTB_TIMEOUT_MS;
• bool ret = ktime_ms_delta(ktime_get(), ct->stall_time) > timeout;
• if (unlikely(ret))

• CT_ERROR(ct, "CT deadlocked\n");
  

nit: in commit message you said all these changes are to "prevent deadlock" so maybe this message should rather be:

int delta = ktime_ms_delta(ktime_get(), ct->stall_time);

CT_ERROR(ct, "Communication stalled for %dms\n", delta);

(note that CT_ERROR already adds "CT" prefix)

• return ret;

+}

static inline bool h2g_has_room(struct intel_guc_ct_buffer *ctb, u32 len_dw) { struct guc_ct_buffer_desc *desc = ctb->desc; @@ -518,6 +531,26 @@ static inline bool h2g_has_room(struct intel_guc_ct_buffer *ctb, u32 len_dw) return space >= len_dw; }

+static int has_room_nb(struct intel_guc_ct *ct, u32 len_dw) +{

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
• lockdep_assert_held(&ct->ctbs.send.lock);
• if (unlikely(!h2g_has_room(ctb, len_dw))) {

• if (ct->stall_time == KTIME_MAX)
  

• 	ct->stall_time = ktime_get();
  

• if (unlikely(ct_deadlocked(ct)))
  

and maybe above message should be printed somewhere around here when we detect "deadlock" for the first time?

• 	return -EIO;
  

• else
  

• 	return -EBUSY;
  

• }
• ct->stall_time = KTIME_MAX;
• return 0;

+}

static int ct_send_nb(struct intel_guc_ct *ct, const u32 *action, u32 len, @@ -530,7 +563,7 @@ static int ct_send_nb(struct intel_guc_ct *ct,

spin_lock_irqsave(&ctb->lock, spin_flags);

• ret = h2g_has_room(ctb, len + 1);

• ret = has_room_nb(ct, len + 1); if (unlikely(ret)) goto out;

@@ -574,11 +607,19 @@ static int ct_send(struct intel_guc_ct *ct, retry: spin_lock_irqsave(&ct->ctbs.send.lock, flags); if (unlikely(!h2g_has_room(ctb, len + 1))) {

• if (ct->stall_time == KTIME_MAX)
  

• 	ct->stall_time = ktime_get();
  

as this is a repeated pattern, maybe it should be moved to h2g_has_room or other wrapper ?

spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);

• if (unlikely(ct_deadlocked(ct)))
  

• 	return -EIO;
  

• cond_resched(); goto retry; }

• ct->stall_time = KTIME_MAX;

this one too

• fence = ct_get_next_fence(ct); request.fence = fence; request.status = 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 eb69263324ba..55ef7c52472f 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h @@ -9,6 +9,7 @@ #include <linux/interrupt.h> #include <linux/spinlock.h> #include <linux/workqueue.h> +#include <linux/ktime.h>

#include "intel_guc_fwif.h"

@@ -68,6 +69,9 @@ struct intel_guc_ct { struct list_head incoming; /* incoming requests */ struct work_struct worker; /* handler for incoming requests */ } requests;

• /** @stall_time: time of first time a CTB submission is stalled */
• ktime_t stall_time;

};

void intel_guc_ct_init_early(struct intel_guc_ct *ct);

On Fri, Jun 25, 2021 at 03:09:29PM +0200, Michal Wajdeczko wrote:

On 24.06.2021 09:04, Matthew Brost wrote:

...

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) which could result in accesses across the PCIe bus, store shadow local copies and only read/write the descriptor values when absolutely necessary. Also store the current space in the each channel locally.

Signed-off-by: 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 | 76 ++++++++++++++--------- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 6 ++ 2 files changed, 51 insertions(+), 31 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 27ec30b5ef47..1fd5c69358ef 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -130,6 +130,10 @@ static void guc_ct_buffer_desc_init(struct guc_ct_buffer_desc *desc) static void guc_ct_buffer_reset(struct intel_guc_ct_buffer *ctb) { ctb->broken = false;

• ctb->tail = 0;
• ctb->head = 0;
• ctb->space = CIRC_SPACE(ctb->tail, ctb->head, ctb->size);
• guc_ct_buffer_desc_init(ctb->desc);

}

@@ -383,10 +387,8 @@ static int ct_write(struct intel_guc_ct *ct, { struct intel_guc_ct_buffer *ctb = &ct->ctbs.send; struct guc_ct_buffer_desc *desc = ctb->desc;

• u32 head = desc->head;
• u32 tail = desc->tail;

• u32 tail = ctb->tail; u32 size = ctb->size;

• u32 used; u32 header; u32 hxg; u32 *cmds = ctb->cmds;

@@ -398,25 +400,14 @@ static int ct_write(struct intel_guc_ct *ct, if (unlikely(desc->status)) goto corrupted;

• if (unlikely((tail | head) >= size)) {

+#ifdef CONFIG_DRM_I915_DEBUG_GUC

since we are caching tail, we may want to check if it's sill correct:

tail = READ_ONCE(desc->tail); if (unlikely(tail != ctb->tail)) { CT_ERROR(ct, "Tail was modified %u != %u\n", tail, ctb->tail); desc->status |= GUC_CTB_STATUS_MISMATCH; goto corrupted; }

and since we own the tail then we can be more strict:

GEM_BUG_ON(tail > size);

and then finally just check GuC head:

head = READ_ONCE(desc->head); if (unlikely(head >= size)) { ...

Sure, but still hidden behind CONFIG_DRM_I915_DEBUG_GUC, right?

...

• if (unlikely((desc->tail | desc->head) >= size)) { CT_ERROR(ct, "Invalid offsets head=%u tail=%u (size=%u)\n",

• 	 head, tail, size);
  

• 	 desc->head, desc->tail, size);
  

  desc->status |= GUC_CTB_STATUS_OVERFLOW; goto corrupted; }

• /*

• * 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;
  

+#endif

/* * dw0: CT header (including fence) @@ -457,7 +448,9 @@ static int ct_write(struct intel_guc_ct *ct, write_barrier(ct);

/* now update descriptor */

• ctb->tail = tail; WRITE_ONCE(desc->tail, tail);
• ctb->space -= len + 1;

this magic "1" is likely GUC_CTB_MSG_MIN_LEN, right ?

Yes.

...

return 0;

@@ -473,7 +466,7 @@ static int ct_write(struct intel_guc_ct *ct,

• @req: pointer to pending request
• @status: placeholder for status

• • For each sent request, Guc shall send bac CT response message.

• • For each sent request, GuC shall send back CT response message.
  • Our message handler will update status of tracked request once
  • response message with given fence is received. Wait here and
  • check for valid response status value.

@@ -520,24 +513,35 @@ static inline bool ct_deadlocked(struct intel_guc_ct *ct) return ret; }

-static inline bool h2g_has_room(struct intel_guc_ct_buffer *ctb, u32 len_dw) +static inline bool h2g_has_room(struct intel_guc_ct *ct, u32 len_dw) {

• struct guc_ct_buffer_desc *desc = ctb->desc;
• u32 head = READ_ONCE(desc->head);

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
• u32 head; u32 space;

• space = CIRC_SPACE(desc->tail, head, ctb->size);

• if (ctb->space >= len_dw)

• return true;
  

• head = READ_ONCE(ctb->desc->head);
• if (unlikely(head > ctb->size)) {

• CT_ERROR(ct, "Corrupted descriptor head=%u tail=%u size=%u\n",
  

• 	  ctb->desc->head, ctb->desc->tail, ctb->size);
  

• ctb->desc->status |= GUC_CTB_STATUS_OVERFLOW;
  

• ctb->broken = true;
  

• return false;
  

• }
• space = CIRC_SPACE(ctb->tail, head, ctb->size);
• ctb->space = space;

maybe here we could mark stall_time ?

if (space >= len_dw) return true;

if (ct->stall_time == KTIME_MAX) ct->stall_time = ktime_get(); return false;

No. See my eariler comment [1] about why I'd rather leave this to the caller.

[1] https://patchwork.freedesktop.org/patch/440703/?series=91840&rev=1

...

return space >= len_dw;

btw, maybe to avoid filling CTB to the last dword, this should be

space > len_dw

CIRC_SPACE leaves an extra DW already.

note the earlier comment:

/*

• tail == head condition indicates empty. GuC FW does not support
• using up the entire buffer to get tail == head meaning full.

*/

Yes, again CIRC_SPACE uses this same algorithm.

Matt

...

}

static int has_room_nb(struct intel_guc_ct *ct, u32 len_dw) {

• struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;

• lockdep_assert_held(&ct->ctbs.send.lock);

• if (unlikely(!h2g_has_room(ctb, len_dw))) {

• if (unlikely(!h2g_has_room(ct, len_dw))) { if (ct->stall_time == KTIME_MAX) ct->stall_time = ktime_get();

@@ -606,10 +610,10 @@ static int ct_send(struct intel_guc_ct *ct, */ retry: spin_lock_irqsave(&ct->ctbs.send.lock, flags);

• if (unlikely(!h2g_has_room(ctb, len + 1))) {

• if (unlikely(!h2g_has_room(ct, len + 1))) { if (ct->stall_time == KTIME_MAX) ct->stall_time = ktime_get();

• spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
  

• spin_unlock_irqrestore(&ctb->lock, flags);
  
  

  if (unlikely(ct_deadlocked(ct))) return -EIO;

@@ -632,7 +636,7 @@ static int ct_send(struct intel_guc_ct *ct,

err = ct_write(ct, action, len, fence, 0);

• spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);

• spin_unlock_irqrestore(&ctb->lock, flags);

  if (unlikely(err)) goto unlink;

@@ -720,7 +724,7 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg) { struct intel_guc_ct_buffer *ctb = &ct->ctbs.recv; struct guc_ct_buffer_desc *desc = ctb->desc;

• u32 head = desc->head;

• u32 head = ctb->head; u32 tail = desc->tail; u32 size = ctb->size; u32 *cmds = ctb->cmds;

@@ -735,12 +739,21 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg) if (unlikely(desc->status)) goto corrupted;

• if (unlikely((tail | head) >= size)) {

+#ifdef CONFIG_DRM_I915_DEBUG_GUC

as above we may want to check if our cached head was not modified

...

• if (unlikely((desc->tail | desc->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; }

+#else

• if (unlikely((tail | ctb->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;
  

• }

+#endif

/* tail == head condition indicates empty */ available = tail - head; @@ -790,6 +803,7 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg) } CT_DEBUG(ct, "received %*ph\n", 4 * len, (*msg)->msg);

• ctb->head = head; /* now update descriptor */ WRITE_ONCE(desc->head, head);

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 55ef7c52472f..9924335e2ee6 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h @@ -33,6 +33,9 @@ struct intel_guc;

• @desc: pointer to the buffer descriptor
• @cmds: pointer to the commands buffer
• @size: size of the commands buffer in dwords

• • @head: local shadow copy of head in dwords
• • @tail: local shadow copy of tail in dwords
• • @space: local shadow copy of space in dwords
  • @broken: flag to indicate if descriptor data is broken
  */

struct intel_guc_ct_buffer { @@ -40,6 +43,9 @@ struct intel_guc_ct_buffer { struct guc_ct_buffer_desc *desc; u32 *cmds; u32 size;

• u32 tail;
• u32 head;
• u32 space;

in later patch this is changing to atomic_t maybe we can start with it ?

...

bool broken; };

On 6/24/2021 00:04, Matthew Brost wrote:

Add new GuC interface defines and structures while maintaining old ones in parallel.

Cc: John Harrison john.c.harrison@intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

I think there was some difference of opinion over whether these additions should be squashed in to the specific patches that first use them. However, on the grounds that such is basically a patch-only style comment and doesn't change the final product plus, we need to get this stuff merged efficiently and not spend forever rebasing and refactoring...

Reviewed-by: John Harrison John.C.Harrison@Intel.com

---

.../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 14 +++++++ drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 41 +++++++++++++++++++ 2 files changed, 55 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h index 2d6198e63ebe..57e18babdf4b 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h @@ -124,10 +124,24 @@ enum intel_guc_action { INTEL_GUC_ACTION_FORCE_LOG_BUFFER_FLUSH = 0x302, INTEL_GUC_ACTION_ENTER_S_STATE = 0x501, INTEL_GUC_ACTION_EXIT_S_STATE = 0x502,

• INTEL_GUC_ACTION_GLOBAL_SCHED_POLICY_CHANGE = 0x506,
• INTEL_GUC_ACTION_SCHED_CONTEXT = 0x1000,
• INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_SET = 0x1001,
• INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_DONE = 0x1002,
• INTEL_GUC_ACTION_SCHED_ENGINE_MODE_SET = 0x1003,
• INTEL_GUC_ACTION_SCHED_ENGINE_MODE_DONE = 0x1004,
• INTEL_GUC_ACTION_SET_CONTEXT_PRIORITY = 0x1005,
• INTEL_GUC_ACTION_SET_CONTEXT_EXECUTION_QUANTUM = 0x1006,
• INTEL_GUC_ACTION_SET_CONTEXT_PREEMPTION_TIMEOUT = 0x1007,
• INTEL_GUC_ACTION_CONTEXT_RESET_NOTIFICATION = 0x1008,
• INTEL_GUC_ACTION_ENGINE_FAILURE_NOTIFICATION = 0x1009, INTEL_GUC_ACTION_SLPC_REQUEST = 0x3003, INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000,
• INTEL_GUC_ACTION_REGISTER_CONTEXT = 0x4502,
• INTEL_GUC_ACTION_DEREGISTER_CONTEXT = 0x4503, INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505, INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER = 0x4506,
• INTEL_GUC_ACTION_DEREGISTER_CONTEXT_DONE = 0x4600, INTEL_GUC_ACTION_LIMIT };

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h index 617ec601648d..28245a217a39 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h @@ -17,6 +17,9 @@ #include "abi/guc_communication_ctb_abi.h" #include "abi/guc_messages_abi.h"

+#define GUC_CONTEXT_DISABLE 0 +#define GUC_CONTEXT_ENABLE 1

• #define GUC_CLIENT_PRIORITY_KMD_HIGH 0 #define GUC_CLIENT_PRIORITY_HIGH 1 #define GUC_CLIENT_PRIORITY_KMD_NORMAL 2

@@ -26,6 +29,9 @@ #define GUC_MAX_STAGE_DESCRIPTORS 1024 #define GUC_INVALID_STAGE_ID GUC_MAX_STAGE_DESCRIPTORS

+#define GUC_MAX_LRC_DESCRIPTORS 65535 +#define GUC_INVALID_LRC_ID GUC_MAX_LRC_DESCRIPTORS

• #define GUC_RENDER_ENGINE 0 #define GUC_VIDEO_ENGINE 1 #define GUC_BLITTER_ENGINE 2

@@ -237,6 +243,41 @@ struct guc_stage_desc { u64 desc_private; } __packed;

+#define CONTEXT_REGISTRATION_FLAG_KMD BIT(0)

+#define CONTEXT_POLICY_DEFAULT_EXECUTION_QUANTUM_US 1000000 +#define CONTEXT_POLICY_DEFAULT_PREEMPTION_TIME_US 500000

+/* Preempt to idle on quantum expiry */ +#define CONTEXT_POLICY_FLAG_PREEMPT_TO_IDLE BIT(0)

+/*

• • GuC Context registration descriptor.
• • FIXME: This is only required to exist during context registration.
• • The current 1:1 between guc_lrc_desc and LRCs for the lifetime of the LRC
• • is not required.
• */

+struct guc_lrc_desc {

• u32 hw_context_desc;
• u32 slpm_perf_mode_hint; /* SPLC v1 only */
• u32 slpm_freq_hint;
• u32 engine_submit_mask; /* In logical space */
• u8 engine_class;
• u8 reserved0[3];
• u32 priority;
• u32 process_desc;
• u32 wq_addr;
• u32 wq_size;
• u32 context_flags; /* CONTEXT_REGISTRATION_* */
• /* Time for one workload to execute. (in micro seconds) */
• u32 execution_quantum;
• /* Time to wait for a preemption request to complete before issuing a

• * reset. (in micro seconds). */
  

• u32 preemption_timeout;
• u32 policy_flags; /* CONTEXT_POLICY_* */
• u32 reserved1[19];

+} __packed;

• #define GUC_POWER_UNSPECIFIED 0 #define GUC_POWER_D0 1 #define GUC_POWER_D1 2

On Fri, Jun 25, 2021 at 03:17:51PM +0200, Michal Wajdeczko wrote:

On 24.06.2021 09:04, Matthew Brost wrote:

...

Add lrc descriptor context lookup array which can resolve the intel_context from the lrc descriptor index. In addition to lookup, it can determine in the lrc descriptor context is currently registered with the GuC by checking if an entry for a descriptor index is present. Future patches in the series will make use of this array.

s/lrc/LRC

I guess? lrc and LRC are used interchangeably throughout the current code base.

...

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.h | 5 +++ .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 32 +++++++++++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index b28fa54214f2..2313d9fc087b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -6,6 +6,8 @@ #ifndef _INTEL_GUC_H_ #define _INTEL_GUC_H_

+#include "linux/xarray.h"

#include <linux/xarray.h>

Yep.

...

#include "intel_uncore.h" #include "intel_guc_fw.h" #include "intel_guc_fwif.h" @@ -46,6 +48,9 @@ struct intel_guc { struct i915_vma *lrc_desc_pool; void *lrc_desc_pool_vaddr;

• /* guc_id to intel_context lookup */
• struct xarray context_lookup;
• /* Control params for fw initialization */ u32 params[GUC_CTL_MAX_DWORDS];

btw, IIRC there was idea to move most struct definitions to intel_guc_types.h, is this still a plan ?

I don't ever recall discussing this but we can certainly do this. For what it is worth we do introduce intel_guc_submission_types.h a bit later. I'll make a note about intel_guc_types.h though.

Matt

...

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index a366890fb840..23a94a896a0b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -65,8 +65,6 @@ static inline struct i915_priolist *to_priolist(struct rb_node *rb) return rb_entry(rb, struct i915_priolist, node); }

-/* Future patches will use this function */ -__attribute__ ((unused)) static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index) { struct guc_lrc_desc *base = guc->lrc_desc_pool_vaddr; @@ -76,6 +74,15 @@ static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index) return &base[index]; }

+static inline struct intel_context *__get_context(struct intel_guc *guc, u32 id) +{

• struct intel_context *ce = xa_load(&guc->context_lookup, id);
• GEM_BUG_ON(id >= GUC_MAX_LRC_DESCRIPTORS);
• return ce;

+}

static int guc_lrc_desc_pool_create(struct intel_guc *guc) { u32 size; @@ -96,6 +103,25 @@ static void guc_lrc_desc_pool_destroy(struct intel_guc *guc) i915_vma_unpin_and_release(&guc->lrc_desc_pool, I915_VMA_RELEASE_MAP); }

+static inline void reset_lrc_desc(struct intel_guc *guc, u32 id) +{

• struct guc_lrc_desc *desc = __get_lrc_desc(guc, id);
• memset(desc, 0, sizeof(*desc));
• xa_erase_irq(&guc->context_lookup, id);

+}

+static inline bool lrc_desc_registered(struct intel_guc *guc, u32 id) +{

• return __get_context(guc, id);

+}

+static inline void set_lrc_desc_registered(struct intel_guc *guc, u32 id,

• 			   struct intel_context *ce)
  

+{

• xa_store_irq(&guc->context_lookup, id, ce, GFP_ATOMIC);

+}

static void guc_add_request(struct intel_guc *guc, struct i915_request *rq) { /* Leaving stub as this function will be used in future patches */ @@ -400,6 +426,8 @@ int intel_guc_submission_init(struct intel_guc *guc) */ GEM_BUG_ON(!guc->lrc_desc_pool);

• xa_init_flags(&guc->context_lookup, XA_FLAGS_LOCK_IRQ);
• return 0;

}

On Tue, Jun 29, 2021 at 03:04:56PM -0700, John Harrison wrote:

On 6/24/2021 00:04, Matthew Brost wrote:

...

Implement GuC submission tasklet for new interface. The new GuC interface uses H2G to submit contexts to the GuC. Since H2G use a single channel, a single tasklet submits is used for the submission path.

Re-word? 'a single tasklet submits is used...' doesn't make sense.

Will do.

...

Also the per engine interrupt handler has been updated to disable the rescheduling of the physical engine tasklet, when using GuC scheduling, as the physical engine tasklet is no longer used.

In this patch the field, guc_id, has been added to intel_context and is not assigned. Patches later in the series will assign this value.

Cc: John Harrisonjohn.c.harrison@intel.com Signed-off-by: Matthew Brostmatthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/intel_context_types.h | 9 + drivers/gpu/drm/i915/gt/uc/intel_guc.h | 4 + .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 231 +++++++++--------- 3 files changed, 127 insertions(+), 117 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index ed8c447a7346..bb6fef7eae52 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -136,6 +136,15 @@ struct intel_context { struct intel_sseu sseu; u8 wa_bb_page; /* if set, page num reserved for context workarounds */

• /* GuC scheduling state that does not require a lock. */

Maybe 'GuC scheduling state flags that do not require a lock'? Otherwise it just looks like a counter or something.

Sure.

...

• atomic_t guc_sched_state_no_lock;
• /*

• * GuC lrc descriptor ID - Not assigned in this patch but future patches
  

Not a blocker but s/lrc/LRC/ would keep Michal happy ;). Although presumably this comment is at least being amended by later patches in the series.

Will fix.

...

• * in the series will.
  

• */
  

• u16 guc_id; }; #endif /* __INTEL_CONTEXT_TYPES__ */

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 2313d9fc087b..9ba8219475b2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -30,6 +30,10 @@ struct intel_guc { struct intel_guc_log log; struct intel_guc_ct ct;

• /* Global engine used to submit requests to GuC */
• struct i915_sched_engine *sched_engine;
• struct i915_request *stalled_request;
• /* intel_guc_recv interrupt related state */ spinlock_t irq_lock; unsigned int msg_enabled_mask;

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 23a94a896a0b..ee933efbf0ff 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -60,6 +60,31 @@ #define GUC_REQUEST_SIZE 64 /* bytes */ +/*

• • Below is a set of functions which control the GuC scheduling state which do
• • not require a lock as all state transitions are mutually exclusive. i.e. It
• • is not possible for the context pinning code and submission, for the same
• • context, to be executing simultaneously. We still need an atomic as it is
• • possible for some of the bits to changing at the same time though.
• */

+#define SCHED_STATE_NO_LOCK_ENABLED BIT(0) +static inline bool context_enabled(struct intel_context *ce) +{

• return (atomic_read(&ce->guc_sched_state_no_lock) &

• SCHED_STATE_NO_LOCK_ENABLED);
  

+}

+static inline void set_context_enabled(struct intel_context *ce) +{

• atomic_or(SCHED_STATE_NO_LOCK_ENABLED, &ce->guc_sched_state_no_lock);

+}

+static inline void clr_context_enabled(struct intel_context *ce) +{

• atomic_and((u32)~SCHED_STATE_NO_LOCK_ENABLED,

•    &ce->guc_sched_state_no_lock);
  

+}

• static inline struct i915_priolist *to_priolist(struct rb_node *rb) { return rb_entry(rb, struct i915_priolist, node);

@@ -122,37 +147,29 @@ static inline void set_lrc_desc_registered(struct intel_guc *guc, u32 id, xa_store_irq(&guc->context_lookup, id, ce, GFP_ATOMIC); } -static void guc_add_request(struct intel_guc *guc, struct i915_request *rq) +static int guc_add_request(struct intel_guc *guc, struct i915_request *rq) {

• /* Leaving stub as this function will be used in future patches */

-}

• int err;
• struct intel_context *ce = rq->context;
• u32 action[3];
• int len = 0;
• bool enabled = context_enabled(ce);

-/*

• • When we're doing submissions using regular execlists backend, writing to
• • ELSP from CPU side is enough to make sure that writes to ringbuffer pages
• • pinned in mappable aperture portion of GGTT are visible to command streamer.
• • Writes done by GuC on our behalf are not guaranteeing such ordering,
• • therefore, to ensure the flush, we're issuing a POSTING READ.
• */

-static void flush_ggtt_writes(struct i915_vma *vma) -{

• if (i915_vma_is_map_and_fenceable(vma))

• intel_uncore_posting_read_fw(vma->vm->gt->uncore,
  

• 			     GUC_STATUS);
  

-}

• if (!enabled) {

• action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_SET;
  

• action[len++] = ce->guc_id;
  

• action[len++] = GUC_CONTEXT_ENABLE;
  

• } else {

• action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT;
  

• action[len++] = ce->guc_id;
  

• }

-static void guc_submit(struct intel_engine_cs *engine,

•        struct i915_request **out,
  

•        struct i915_request **end)
  

-{

• struct intel_guc *guc = &engine->gt->uc.guc;

• err = intel_guc_send_nb(guc, action, len);

• do {

• struct i915_request *rq = *out++;
  

• if (!enabled && !err)

• set_context_enabled(ce);
  

• flush_ggtt_writes(rq->ring->vma);
  

• guc_add_request(guc, rq);
  

• } while (out != end);

• return err; } static inline int rq_prio(const struct i915_request *rq)

@@ -160,125 +177,88 @@ static inline int rq_prio(const struct i915_request *rq) return rq->sched.attr.priority; } -static struct i915_request *schedule_in(struct i915_request *rq, int idx) +static int guc_dequeue_one_context(struct intel_guc *guc) {

• trace_i915_request_in(rq, idx);
• /*

• * Currently we are not tracking the rq->context being inflight
  

• * (ce->inflight = rq->engine). It is only used by the execlists
  

• * backend at the moment, a similar counting strategy would be
  

• * required if we generalise the inflight tracking.
  

• */
  

• __intel_gt_pm_get(rq->engine->gt);
• return i915_request_get(rq);

-}

-static void schedule_out(struct i915_request *rq) -{

• trace_i915_request_out(rq);
• intel_gt_pm_put_async(rq->engine->gt);
• i915_request_put(rq);

-}

-static void __guc_dequeue(struct intel_engine_cs *engine) -{

• struct intel_engine_execlists * const execlists = &engine->execlists;
• struct i915_sched_engine * const sched_engine = engine->sched_engine;
• struct i915_request **first = execlists->inflight;
• struct i915_request ** const last_port = first + execlists->port_mask;
• struct i915_request *last = first[0];
• struct i915_request **port;

• struct i915_sched_engine * const sched_engine = guc->sched_engine;
• struct i915_request *last = NULL; bool submit = false; struct rb_node *rb;
• int ret; lockdep_assert_held(&sched_engine->lock);

• if (last) {

• if (*++first)
  

• 	return;
  

• last = NULL;
  

• if (guc->stalled_request) {

• submit = true;
  

• last = guc->stalled_request;
  

• goto resubmit;
  

  }

• /*

• * We write directly into the execlists->inflight queue and don't use
  

• * the execlists->pending queue, as we don't have a distinct switch
  

• * event.
  

• */
  

• port = first; while ((rb = rb_first_cached(&sched_engine->queue))) { struct i915_priolist *p = to_priolist(rb); struct i915_request *rq, *rn; priolist_for_each_request_consume(rq, rn, p) {

• 	if (last && rq->context != last->context) {
  

• 		if (port == last_port)
  

• 			goto done;
  

• 		*port = schedule_in(last,
  

• 				    port - execlists->inflight);
  

• 		port++;
  

• 	}
  

• 	if (last && rq->context != last->context)
  

• 		goto done;
  list_del_init(&rq->sched.link);
  

• __i915_request_submit(rq);
  

• 	submit = true;
  

• 	trace_i915_request_in(rq, 0);
  last = rq;
  

• 	submit = true;
  

  } rb_erase_cached(&p->node, &sched_engine->queue); i915_priolist_free(p); } done:

• sched_engine->queue_priority_hint =

• rb ? to_priolist(rb)->priority : INT_MIN;
  

  if (submit) {

• *port = schedule_in(last, port - execlists->inflight);
  

• *++port = NULL;
  

• guc_submit(engine, first, port);
  

• last->context->lrc_reg_state[CTX_RING_TAIL] =
  

• 	intel_ring_set_tail(last->ring, last->tail);
  

+resubmit:

• /*
  

•  * We only check for -EBUSY here even though it is possible for
  

•  * -EDEADLK to be returned. If -EDEADLK is returned, the GuC has
  

•  * died and a full GPU needs to be done. The hangcheck will
  

'full GPU reset'. Although I believe strictly speaking, it is a 'full GT reset'. There are other bits of the GPU beyond the GT.

Yep, will fix.

...

•  * eventually detect that the GuC has died and trigger this
  

•  * reset so no need to handle -EDEADLK here.
  

•  */
  

• ret = guc_add_request(guc, last);
  

• if (ret == -EBUSY) {
  

• 	tasklet_schedule(&sched_engine->tasklet);
  

• 	guc->stalled_request = last;
  

• 	return false;
  

• }
  

  }

• execlists->active = execlists->inflight;

• guc->stalled_request = NULL;
• return submit; } static void guc_submission_tasklet(struct tasklet_struct *t) { struct i915_sched_engine *sched_engine = from_tasklet(sched_engine, t, tasklet);

• struct intel_engine_cs * const engine = sched_engine->private_data;
• struct intel_engine_execlists * const execlists = &engine->execlists;
• struct i915_request **port, *rq; unsigned long flags;

• bool loop;

• spin_lock_irqsave(&engine->sched_engine->lock, flags);
• for (port = execlists->inflight; (rq = *port); port++) {

• if (!i915_request_completed(rq))
  

• 	break;
  

• schedule_out(rq);
  

• }
• if (port != execlists->inflight) {

• int idx = port - execlists->inflight;
  

• int rem = ARRAY_SIZE(execlists->inflight) - idx;
  

• memmove(execlists->inflight, port, rem * sizeof(*port));
  

• }

• spin_lock_irqsave(&sched_engine->lock, flags);

• __guc_dequeue(engine);

• do {

• loop = guc_dequeue_one_context(sched_engine->private_data);
  

• } while (loop);

• i915_sched_engine_reset_on_empty(engine->sched_engine);

• i915_sched_engine_reset_on_empty(sched_engine);

• spin_unlock_irqrestore(&engine->sched_engine->lock, flags);

• spin_unlock_irqrestore(&sched_engine->lock, flags); }

Not a blocker but it has to be said that it would be much easier to remove all of the above if the delete was split into a separate patch. Having two completely disparate threads of code interwoven in the diff makes it much harder to see what the new version is doing!

Yes, it would be easier to read if this code was deleted in a seperate patch. I'll keep that in mind going forward. No promises but perhaps I'll do this in the next rev.

...

static void cs_irq_handler(struct intel_engine_cs *engine, u16 iir) {

• if (iir & GT_RENDER_USER_INTERRUPT) {

• if (iir & GT_RENDER_USER_INTERRUPT) intel_engine_signal_breadcrumbs(engine);

• tasklet_hi_schedule(&engine->sched_engine->tasklet);
  

• } } static void guc_reset_prepare(struct intel_engine_cs *engine)

@@ -349,6 +329,10 @@ static void guc_reset_cancel(struct intel_engine_cs *engine) struct rb_node *rb; unsigned long flags;

• /* Can be called during boot if GuC fails to load */
• if (!engine->gt)

• return;
  

• ENGINE_TRACE(engine, "\n"); /*

@@ -433,8 +417,11 @@ int intel_guc_submission_init(struct intel_guc *guc) void intel_guc_submission_fini(struct intel_guc *guc) {

• if (guc->lrc_desc_pool)

• guc_lrc_desc_pool_destroy(guc);
  

• if (!guc->lrc_desc_pool)

• return;
  

• guc_lrc_desc_pool_destroy(guc);
• i915_sched_engine_put(guc->sched_engine); } static int guc_context_alloc(struct intel_context *ce)

@@ -499,32 +486,32 @@ static int guc_request_alloc(struct i915_request *request) return 0; } -static inline void queue_request(struct intel_engine_cs *engine, +static inline void queue_request(struct i915_sched_engine *sched_engine, struct i915_request *rq, int prio) { GEM_BUG_ON(!list_empty(&rq->sched.link)); list_add_tail(&rq->sched.link,

•       i915_sched_lookup_priolist(engine->sched_engine, prio));
  

•       i915_sched_lookup_priolist(sched_engine, prio));
  

  set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags); } static void guc_submit_request(struct i915_request *rq) {

• struct intel_engine_cs *engine = rq->engine;

• struct i915_sched_engine *sched_engine = rq->engine->sched_engine; unsigned long flags; /* Will be called from irq-context when using foreign fences. */

• spin_lock_irqsave(&engine->sched_engine->lock, flags);

• spin_lock_irqsave(&sched_engine->lock, flags);

• queue_request(engine, rq, rq_prio(rq));

• queue_request(sched_engine, rq, rq_prio(rq));

• GEM_BUG_ON(i915_sched_engine_is_empty(engine->sched_engine));

• GEM_BUG_ON(i915_sched_engine_is_empty(sched_engine)); GEM_BUG_ON(list_empty(&rq->sched.link));

• tasklet_hi_schedule(&engine->sched_engine->tasklet);

• tasklet_hi_schedule(&sched_engine->tasklet);

• spin_unlock_irqrestore(&engine->sched_engine->lock, flags);

• spin_unlock_irqrestore(&sched_engine->lock, flags); } static void sanitize_hwsp(struct intel_engine_cs *engine)

@@ -602,8 +589,6 @@ static void guc_release(struct intel_engine_cs *engine) { engine->sanitize = NULL; /* no longer in control, nothing to sanitize */

• tasklet_kill(&engine->sched_engine->tasklet);
• intel_engine_cleanup_common(engine); lrc_fini_wa_ctx(engine); }

@@ -674,6 +659,7 @@ static inline void guc_default_irqs(struct intel_engine_cs *engine) int intel_guc_submission_setup(struct intel_engine_cs *engine) { struct drm_i915_private *i915 = engine->i915;

• struct intel_guc *guc = &engine->gt->uc.guc; /*
  • The setup relies on several assumptions (e.g. irqs always enabled)

@@ -681,7 +667,18 @@ int intel_guc_submission_setup(struct intel_engine_cs *engine) */ GEM_BUG_ON(GRAPHICS_VER(i915) < 11);

• tasklet_setup(&engine->sched_engine->tasklet, guc_submission_tasklet);

• if (!guc->sched_engine) {

• guc->sched_engine = i915_sched_engine_create(ENGINE_VIRTUAL);
  

Does the re-work of the sched_engine create/destroy happen later in this patch series? Wasn't there issues with the wrong destroy function being called in certain situations? Or do those issues (and fixes) only come in with the virtual engine support?

We didn't need the destroy until we introduce the guc_submit_engine, but that is changing after the kasan bug fix I sent out today for the internal version of this code. I've already reworked my upstream branch to add a destroy vfunc for sched_engine in seperate patch a bit later in the series.

Matt

John.

...

• if (!guc->sched_engine)
  

• 	return -ENOMEM;
  

• guc->sched_engine->schedule = i915_schedule;
  

• guc->sched_engine->private_data = guc;
  

• tasklet_setup(&guc->sched_engine->tasklet,
  

• 	      guc_submission_tasklet);
  

• }
• i915_sched_engine_put(engine->sched_engine);
• engine->sched_engine = i915_sched_engine_get(guc->sched_engine); guc_default_vfuncs(engine); guc_default_irqs(engine);

On 6/24/2021 00:04, Matthew Brost wrote:

Add bypass tasklet submission path to GuC. The tasklet is only used if H2G channel has backpresure.

Signed-off-by: Matthew Brost matthew.brost@intel.com

Reviewed-by: John Harrison John.C.Harrison@Intel.com

---

.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 37 +++++++++++++++---- 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index ee933efbf0ff..38aff83ee9fa 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -172,6 +172,12 @@ static int guc_add_request(struct intel_guc *guc, struct i915_request *rq) return err; }

+static inline void guc_set_lrc_tail(struct i915_request *rq) +{

• rq->context->lrc_reg_state[CTX_RING_TAIL] =

• intel_ring_set_tail(rq->ring, rq->tail);
  

+}

• static inline int rq_prio(const struct i915_request *rq) { return rq->sched.attr.priority;

@@ -215,8 +221,7 @@ static int guc_dequeue_one_context(struct intel_guc *guc) } done: if (submit) {

• last->context->lrc_reg_state[CTX_RING_TAIL] =
  

• 	intel_ring_set_tail(last->ring, last->tail);
  

• guc_set_lrc_tail(last);
  

  resubmit: /*
  • We only check for -EBUSY here even though it is possible for

@@ -496,20 +501,36 @@ static inline void queue_request(struct i915_sched_engine *sched_engine, set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags); }

+static int guc_bypass_tasklet_submit(struct intel_guc *guc,

• 		     struct i915_request *rq)
  

+{

• int ret;
• __i915_request_submit(rq);
• trace_i915_request_in(rq, 0);
• guc_set_lrc_tail(rq);
• ret = guc_add_request(guc, rq);
• if (ret == -EBUSY)

• guc->stalled_request = rq;
  

• return ret;

+}

• static void guc_submit_request(struct i915_request *rq) { struct i915_sched_engine *sched_engine = rq->engine->sched_engine;

• struct intel_guc *guc = &rq->engine->gt->uc.guc; unsigned long flags;

  /* Will be called from irq-context when using foreign fences. */ spin_lock_irqsave(&sched_engine->lock, flags);

• queue_request(sched_engine, rq, rq_prio(rq));
• GEM_BUG_ON(i915_sched_engine_is_empty(sched_engine));
• GEM_BUG_ON(list_empty(&rq->sched.link));
• tasklet_hi_schedule(&sched_engine->tasklet);

• if (guc->stalled_request || !i915_sched_engine_is_empty(sched_engine))

• queue_request(sched_engine, rq, rq_prio(rq));
  

• else if (guc_bypass_tasklet_submit(guc, rq) == -EBUSY)

• tasklet_hi_schedule(&sched_engine->tasklet);
  
  

  spin_unlock_irqrestore(&sched_engine->lock, flags); }

On 24.06.2021 09:04, Matthew Brost wrote:

Implement GuC context operations which includes GuC specific operations alloc, pin, unpin, and destroy.

Signed-off-by: John Harrison John.C.Harrison@Intel.com Signed-off-by: Matthew Brost matthew.brost@intel.com

---

drivers/gpu/drm/i915/gt/intel_context.c | 5 + drivers/gpu/drm/i915/gt/intel_context_types.h | 22 +- drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 1 - drivers/gpu/drm/i915/gt/uc/intel_guc.h | 34 + drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 + .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 664 ++++++++++++++++-- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/i915_request.c | 1 + 8 files changed, 677 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index 4033184f13b9..2b68af16222c 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -383,6 +383,11 @@ intel_context_init(struct intel_context *ce, struct intel_engine_cs *engine)

mutex_init(&ce->pin_mutex);

• spin_lock_init(&ce->guc_state.lock);
• ce->guc_id = GUC_INVALID_LRC_ID;
• INIT_LIST_HEAD(&ce->guc_id_link);
• i915_active_init(&ce->active, __intel_context_active, __intel_context_retire, 0);

} diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index bb6fef7eae52..ce7c69b34cd1 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -95,6 +95,7 @@ struct intel_context { #define CONTEXT_BANNED 6 #define CONTEXT_FORCE_SINGLE_SUBMISSION 7 #define CONTEXT_NOPREEMPT 8 +#define CONTEXT_LRCA_DIRTY 9

struct { u64 timeout_us; @@ -137,14 +138,29 @@ struct intel_context {

u8 wa_bb_page; /* if set, page num reserved for context workarounds */

• struct {

• /** lock: protects everything in guc_state */
  

• spinlock_t lock;
  

• /**
  

•  * sched_state: scheduling state of this context using GuC
  

•  * submission
  

•  */
  

• u8 sched_state;
  

• } guc_state;

• /* GuC scheduling state that does not require a lock. */ atomic_t guc_sched_state_no_lock;

• /* GuC lrc descriptor ID */

• u16 guc_id;

• /* GuC lrc descriptor reference count */

• atomic_t guc_id_ref;

• /*

• * GuC lrc descriptor ID - Not assigned in this patch but future patches
  

• * in the series will.
  

• * GuC ID link - in list when unpinned but guc_id still valid in GuC
  

  */

• u16 guc_id;

• struct list_head guc_id_link;

some fields are being added with kerneldoc, some without what's the rule ?

};

#endif /* __INTEL_CONTEXT_TYPES__ */ diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h index 41e5350a7a05..49d4857ad9b7 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h +++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h @@ -87,7 +87,6 @@ #define GEN11_CSB_WRITE_PTR_MASK (GEN11_CSB_PTR_MASK << 0)

#define MAX_CONTEXT_HW_ID (1 << 21) /* exclusive */ -#define MAX_GUC_CONTEXT_HW_ID (1 << 20) /* exclusive */ #define GEN11_MAX_CONTEXT_HW_ID (1 << 11) /* exclusive */ /* in Gen12 ID 0x7FF is reserved to indicate idle */ #define GEN12_MAX_CONTEXT_HW_ID (GEN11_MAX_CONTEXT_HW_ID - 1) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 9ba8219475b2..d44316dc914b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -44,6 +44,14 @@ struct intel_guc { void (*disable)(struct intel_guc *guc); } interrupts;

• /*

• * contexts_lock protects the pool of free guc ids and a linked list of
  

• * guc ids available to be stolen
  

• */
  

• spinlock_t contexts_lock;

• struct ida guc_ids;

• struct list_head guc_id_list;

• bool submission_selected;

  struct i915_vma *ads_vma;

@@ -102,6 +110,29 @@ intel_guc_send_and_receive(struct intel_guc *guc, const u32 *action, u32 len, response_buf, response_buf_size, 0); }

+static inline int intel_guc_send_busy_loop(struct intel_guc* guc,

• 			   const u32 *action,
  

• 			   u32 len,
  

• 			   bool loop)
  

+{

• int err;
• /* No sleeping with spin locks, just busy loop */
• might_sleep_if(loop && (!in_atomic() && !irqs_disabled()));

+retry:

• err = intel_guc_send_nb(guc, action, len);
• if (unlikely(err == -EBUSY && loop)) {

• if (likely(!in_atomic() && !irqs_disabled()))
  

• 	cond_resched();
  

• else
  

• 	cpu_relax();
  

• goto retry;
  

• }
• return err;

+}

static inline void intel_guc_to_host_event_handler(struct intel_guc *guc) { intel_guc_ct_event_handler(&guc->ct); @@ -203,6 +234,9 @@ static inline void intel_guc_disable_msg(struct intel_guc *guc, u32 mask) int intel_guc_reset_engine(struct intel_guc *guc, struct intel_engine_cs *engine);

+int intel_guc_deregister_done_process_msg(struct intel_guc *guc,

• 			  const u32 *msg, u32 len);
  

void intel_guc_load_status(struct intel_guc *guc, struct drm_printer *p);

#endif 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 8e0ed7d8feb3..42a7daef2ff6 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -901,6 +901,10 @@ static int ct_process_request(struct intel_guc_ct *ct, struct ct_incoming_msg *r case INTEL_GUC_ACTION_DEFAULT: ret = intel_guc_to_host_process_recv_msg(guc, payload, len); break;

• case INTEL_GUC_ACTION_DEREGISTER_CONTEXT_DONE:

• ret = intel_guc_deregister_done_process_msg(guc, payload,
  

• 					    len);
  

• break;
  

  default: ret = -EOPNOTSUPP; break;

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 38aff83ee9fa..d39579ac2faa 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -13,7 +13,9 @@ #include "gt/intel_gt.h" #include "gt/intel_gt_irq.h" #include "gt/intel_gt_pm.h" +#include "gt/intel_gt_requests.h" #include "gt/intel_lrc.h" +#include "gt/intel_lrc_reg.h" #include "gt/intel_mocs.h" #include "gt/intel_ring.h"

@@ -85,6 +87,73 @@ static inline void clr_context_enabled(struct intel_context *ce) &ce->guc_sched_state_no_lock); }

+/*

• • Below is a set of functions which control the GuC scheduling state which
• • require a lock, aside from the special case where the functions are called
• • from guc_lrc_desc_pin(). In that case it isn't possible for any other code
• • path to be executing on the context.
• */

+#define SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER BIT(0) +#define SCHED_STATE_DESTROYED BIT(1) +static inline void init_sched_state(struct intel_context *ce) +{

• /* Only should be called from guc_lrc_desc_pin() */
• atomic_set(&ce->guc_sched_state_no_lock, 0);
• ce->guc_state.sched_state = 0;

+}

+static inline bool +context_wait_for_deregister_to_register(struct intel_context *ce) +{

• return (ce->guc_state.sched_state &

• SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER);
  

+}

+static inline void +set_context_wait_for_deregister_to_register(struct intel_context *ce) +{

• /* Only should be called from guc_lrc_desc_pin() */
• ce->guc_state.sched_state |=

• SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER;
  

+}

+static inline void +clr_context_wait_for_deregister_to_register(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ce->guc_state.sched_state =

• (ce->guc_state.sched_state &
  

•  ~SCHED_STATE_WAIT_FOR_DEREGISTER_TO_REGISTER);
  

+}

+static inline bool +context_destroyed(struct intel_context *ce) +{

• return (ce->guc_state.sched_state & SCHED_STATE_DESTROYED);

+}

+static inline void +set_context_destroyed(struct intel_context *ce) +{

• lockdep_assert_held(&ce->guc_state.lock);
• ce->guc_state.sched_state |= SCHED_STATE_DESTROYED;

+}

+static inline bool context_guc_id_invalid(struct intel_context *ce) +{

• return (ce->guc_id == GUC_INVALID_LRC_ID);

+}

+static inline void set_context_guc_id_invalid(struct intel_context *ce) +{

• ce->guc_id = GUC_INVALID_LRC_ID;

+}

+static inline struct intel_guc *ce_to_guc(struct intel_context *ce) +{

• return &ce->engine->gt->uc.guc;

+}

static inline struct i915_priolist *to_priolist(struct rb_node *rb) { return rb_entry(rb, struct i915_priolist, node); @@ -155,6 +224,9 @@ static int guc_add_request(struct intel_guc *guc, struct i915_request *rq) int len = 0; bool enabled = context_enabled(ce);

• GEM_BUG_ON(!atomic_read(&ce->guc_id_ref));
• GEM_BUG_ON(context_guc_id_invalid(ce));
• if (!enabled) { action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_SET; action[len++] = ce->guc_id;

@@ -417,6 +489,10 @@ int intel_guc_submission_init(struct intel_guc *guc)

xa_init_flags(&guc->context_lookup, XA_FLAGS_LOCK_IRQ);

• spin_lock_init(&guc->contexts_lock);
• INIT_LIST_HEAD(&guc->guc_id_list);
• ida_init(&guc->guc_ids);
• return 0;

}

@@ -429,9 +505,303 @@ void intel_guc_submission_fini(struct intel_guc *guc) i915_sched_engine_put(guc->sched_engine); }

-static int guc_context_alloc(struct intel_context *ce) +static inline void queue_request(struct i915_sched_engine *sched_engine,

• 		 struct i915_request *rq,
  

• 		 int prio)
  

{

• return lrc_alloc(ce, ce->engine);

• GEM_BUG_ON(!list_empty(&rq->sched.link));
• list_add_tail(&rq->sched.link,

•       i915_sched_lookup_priolist(sched_engine, prio));
  

• set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);

+}

+static int guc_bypass_tasklet_submit(struct intel_guc *guc,

• 		     struct i915_request *rq)
  

+{

• int ret;
• __i915_request_submit(rq);
• trace_i915_request_in(rq, 0);
• guc_set_lrc_tail(rq);
• ret = guc_add_request(guc, rq);
• if (ret == -EBUSY)

• guc->stalled_request = rq;
  

• return ret;

+}

+static void guc_submit_request(struct i915_request *rq) +{

• struct i915_sched_engine *sched_engine = rq->engine->sched_engine;
• struct intel_guc *guc = &rq->engine->gt->uc.guc;
• unsigned long flags;
• /* Will be called from irq-context when using foreign fences. */
• spin_lock_irqsave(&sched_engine->lock, flags);
• if (guc->stalled_request || !i915_sched_engine_is_empty(sched_engine))

• queue_request(sched_engine, rq, rq_prio(rq));
  

• else if (guc_bypass_tasklet_submit(guc, rq) == -EBUSY)

• tasklet_hi_schedule(&sched_engine->tasklet);
  

• spin_unlock_irqrestore(&sched_engine->lock, flags);

+}

+#define GUC_ID_START 64 /* First 64 guc_ids reserved */ +static int new_guc_id(struct intel_guc *guc) +{

• return ida_simple_get(&guc->guc_ids, GUC_ID_START,

• 	      GUC_MAX_LRC_DESCRIPTORS, GFP_KERNEL |
  

• 	      __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
  

+}

+static void __release_guc_id(struct intel_guc *guc, struct intel_context *ce) +{

• if (!context_guc_id_invalid(ce)) {

• ida_simple_remove(&guc->guc_ids, ce->guc_id);
  

• reset_lrc_desc(guc, ce->guc_id);
  

• set_context_guc_id_invalid(ce);
  

• }
• if (!list_empty(&ce->guc_id_link))

• list_del_init(&ce->guc_id_link);
  

+}

+static void release_guc_id(struct intel_guc *guc, struct intel_context *ce) +{

• unsigned long flags;
• spin_lock_irqsave(&guc->contexts_lock, flags);
• __release_guc_id(guc, ce);
• spin_unlock_irqrestore(&guc->contexts_lock, flags);

+}

+static int steal_guc_id(struct intel_guc *guc) +{

• struct intel_context *ce;
• int guc_id;
• if (!list_empty(&guc->guc_id_list)) {

• ce = list_first_entry(&guc->guc_id_list,
  

• 		      struct intel_context,
  

• 		      guc_id_link);
  

• GEM_BUG_ON(atomic_read(&ce->guc_id_ref));
  

• GEM_BUG_ON(context_guc_id_invalid(ce));
  

• list_del_init(&ce->guc_id_link);
  

• guc_id = ce->guc_id;
  

• set_context_guc_id_invalid(ce);
  

• return guc_id;
  

• } else {

• return -EAGAIN;
  

• }

+}

+static int assign_guc_id(struct intel_guc *guc, u16 *out) +{

• int ret;
• ret = new_guc_id(guc);
• if (unlikely(ret < 0)) {

• ret = steal_guc_id(guc);
  

• if (ret < 0)
  

• 	return ret;
  

• }
• *out = ret;
• return 0;

+}

+#define PIN_GUC_ID_TRIES 4 +static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce) +{

• int ret = 0;
• unsigned long flags, tries = PIN_GUC_ID_TRIES;
• GEM_BUG_ON(atomic_read(&ce->guc_id_ref));

+try_again:

• spin_lock_irqsave(&guc->contexts_lock, flags);
• if (context_guc_id_invalid(ce)) {

• ret = assign_guc_id(guc, &ce->guc_id);
  

• if (ret)
  

• 	goto out_unlock;
  

• ret = 1;	// Indidcates newly assigned HW context
  

C++ style comment

• }
• if (!list_empty(&ce->guc_id_link))

• list_del_init(&ce->guc_id_link);
  

• atomic_inc(&ce->guc_id_ref);

+out_unlock:

• spin_unlock_irqrestore(&guc->contexts_lock, flags);
• /*

• * -EAGAIN indicates no guc_ids are available, let's retire any
  

• * outstanding requests to see if that frees up a guc_id. If the first
  

• * retire didn't help, insert a sleep with the timeslice duration before
  

• * attempting to retire more requests. Double the sleep period each
  

• * subsequent pass before finally giving up. The sleep period has max of
  

• * 100ms and minimum of 1ms.
  

• */
  

• if (ret == -EAGAIN && --tries) {

• if (PIN_GUC_ID_TRIES - tries > 1) {
  

• 	unsigned int timeslice_shifted =
  

• 		ce->engine->props.timeslice_duration_ms <<
  

• 		(PIN_GUC_ID_TRIES - tries - 2);
  

• 	unsigned int max = min_t(unsigned int, 100,
  

• 				 timeslice_shifted);
  

• 	msleep(max_t(unsigned int, max, 1));
  

• }
  

• intel_gt_retire_requests(guc_to_gt(guc));
  

• goto try_again;
  

• }
• return ret;

+}

+static void unpin_guc_id(struct intel_guc *guc, struct intel_context *ce) +{

• unsigned long flags;
• GEM_BUG_ON(atomic_read(&ce->guc_id_ref) < 0);
• spin_lock_irqsave(&guc->contexts_lock, flags);
• if (!context_guc_id_invalid(ce) && list_empty(&ce->guc_id_link) &&

•    !atomic_read(&ce->guc_id_ref))
  

• list_add_tail(&ce->guc_id_link, &guc->guc_id_list);
  

• spin_unlock_irqrestore(&guc->contexts_lock, flags);

+}

+static int __guc_action_register_context(struct intel_guc *guc,

• 			 u32 guc_id,
  

• 			 u32 offset)
  

+{

• u32 action[] = {

• INTEL_GUC_ACTION_REGISTER_CONTEXT,
  

• guc_id,
  

• offset,
  

• };
• return intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), true);

+}

+static int register_context(struct intel_context *ce) +{

• struct intel_guc *guc = ce_to_guc(ce);
• u32 offset = intel_guc_ggtt_offset(guc, guc->lrc_desc_pool) +

• ce->guc_id * sizeof(struct guc_lrc_desc);
  

• return __guc_action_register_context(guc, ce->guc_id, offset);

+}

+static int __guc_action_deregister_context(struct intel_guc *guc,

• 			   u32 guc_id)
  

+{

• u32 action[] = {

• INTEL_GUC_ACTION_DEREGISTER_CONTEXT,
  

• guc_id,
  

• };
• return intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), true);

+}

+static int deregister_context(struct intel_context *ce, u32 guc_id) +{

• struct intel_guc *guc = ce_to_guc(ce);
• return __guc_action_deregister_context(guc, guc_id);

+}

+static intel_engine_mask_t adjust_engine_mask(u8 class, intel_engine_mask_t mask) +{

• switch (class) {
• case RENDER_CLASS:

• return mask >> RCS0;
  

• case VIDEO_ENHANCEMENT_CLASS:

• return mask >> VECS0;
  

• case VIDEO_DECODE_CLASS:

• return mask >> VCS0;
  

• case COPY_ENGINE_CLASS:

• return mask >> BCS0;
  

• default:

• GEM_BUG_ON("Invalid Class");
  

• return 0;
  

• }

+}

+static void guc_context_policy_init(struct intel_engine_cs *engine,

• 		    struct guc_lrc_desc *desc)
  

+{

• desc->policy_flags = 0;
• desc->execution_quantum = CONTEXT_POLICY_DEFAULT_EXECUTION_QUANTUM_US;
• desc->preemption_timeout = CONTEXT_POLICY_DEFAULT_PREEMPTION_TIME_US;

+}

+static int guc_lrc_desc_pin(struct intel_context *ce) +{

• struct intel_runtime_pm *runtime_pm =

• &ce->engine->gt->i915->runtime_pm;
  

• struct intel_engine_cs *engine = ce->engine;
• struct intel_guc *guc = &engine->gt->uc.guc;
• u32 desc_idx = ce->guc_id;
• struct guc_lrc_desc *desc;
• bool context_registered;
• intel_wakeref_t wakeref;
• int ret = 0;
• GEM_BUG_ON(!engine->mask);
• /*

• * Ensure LRC + CT vmas are is same region as write barrier is done
  

• * based on CT vma region.
  

• */
  

• GEM_BUG_ON(i915_gem_object_is_lmem(guc->ct.vma->obj) !=

•    i915_gem_object_is_lmem(ce->ring->vma->obj));
  

• context_registered = lrc_desc_registered(guc, desc_idx);
• reset_lrc_desc(guc, desc_idx);
• set_lrc_desc_registered(guc, desc_idx, ce);
• desc = __get_lrc_desc(guc, desc_idx);
• desc->engine_class = engine_class_to_guc_class(engine->class);
• desc->engine_submit_mask = adjust_engine_mask(engine->class,

• 				      engine->mask);
  

• desc->hw_context_desc = ce->lrc.lrca;
• desc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;
• desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
• guc_context_policy_init(engine, desc);
• init_sched_state(ce);
• /*

• * The context_lookup xarray is used to determine if the hardware
  

• * context is currently registered. There are two cases in which it
  

• * could be regisgered either the guc_id has been stole from from
  

• * another context or the lrc descriptor address of this context has
  

• * changed. In either case the context needs to be deregistered with the
  

• * GuC before registering this context.
  

• */
  

• if (context_registered) {

• set_context_wait_for_deregister_to_register(ce);
  

• intel_context_get(ce);
  

• /*
  

•  * If stealing the guc_id, this ce has the same guc_id as the
  

•  * context whos guc_id was stole.
  

•  */
  

• with_intel_runtime_pm(runtime_pm, wakeref)
  

• 	ret = deregister_context(ce, ce->guc_id);
  

• } else {

• with_intel_runtime_pm(runtime_pm, wakeref)
  

• 	ret = register_context(ce);
  

• }
• return ret;

}

static int guc_context_pre_pin(struct intel_context *ce, @@ -443,36 +813,137 @@ static int guc_context_pre_pin(struct intel_context *ce,

static int guc_context_pin(struct intel_context *ce, void *vaddr) {

• if (i915_ggtt_offset(ce->state) !=

•    (ce->lrc.lrca & CTX_GTT_ADDRESS_MASK))
  

• set_bit(CONTEXT_LRCA_DIRTY, &ce->flags);
  

• return lrc_pin(ce, ce->engine, vaddr);

}

+static void guc_context_unpin(struct intel_context *ce) +{

• unpin_guc_id(ce_to_guc(ce), ce);
• lrc_unpin(ce);

+}

+static void guc_context_post_unpin(struct intel_context *ce) +{

• lrc_post_unpin(ce);

+}

+static inline void guc_lrc_desc_unpin(struct intel_context *ce) +{

• struct intel_engine_cs *engine = ce->engine;
• struct intel_guc *guc = &engine->gt->uc.guc;
• unsigned long flags;
• GEM_BUG_ON(!lrc_desc_registered(guc, ce->guc_id));
• GEM_BUG_ON(ce != __get_context(guc, ce->guc_id));
• spin_lock_irqsave(&ce->guc_state.lock, flags);
• set_context_destroyed(ce);
• spin_unlock_irqrestore(&ce->guc_state.lock, flags);
• deregister_context(ce, ce->guc_id);

+}

+static void guc_context_destroy(struct kref *kref) +{

• struct intel_context *ce = container_of(kref, typeof(*ce), ref);
• struct intel_runtime_pm *runtime_pm = &ce->engine->gt->i915->runtime_pm;
• struct intel_guc *guc = &ce->engine->gt->uc.guc;
• intel_wakeref_t wakeref;
• unsigned long flags;
• /*

• * If the guc_id is invalid this context has been stolen and we can free
  

• * it immediately. Also can be freed immediately if the context is not
  

• * registered with the GuC.
  

• */
  

• if (context_guc_id_invalid(ce) ||

•    !lrc_desc_registered(guc, ce->guc_id)) {
  

• release_guc_id(guc, ce);
  

• lrc_destroy(kref);
  

• return;
  

• }
• /*

• * We have to acquire the context spinlock and check guc_id again, if it
  

• * is valid it hasn't been stolen and needs to be deregistered. We
  

• * delete this context from the list of unpinned guc_ids available to
  

• * stole to seal a race with guc_lrc_desc_pin(). When the G2H CTB
  

• * returns indicating this context has been deregistered the guc_id is
  

• * returned to the pool of available guc_ids.
  

• */
  

• spin_lock_irqsave(&guc->contexts_lock, flags);
• if (context_guc_id_invalid(ce)) {

• __release_guc_id(guc, ce);
  

• spin_unlock_irqrestore(&guc->contexts_lock, flags);
  

• lrc_destroy(kref);
  

• return;
  

• }
• if (!list_empty(&ce->guc_id_link))

• list_del_init(&ce->guc_id_link);
  

• spin_unlock_irqrestore(&guc->contexts_lock, flags);
• /*

• * We defer GuC context deregistration until the context is destroyed
  

• * in order to save on CTBs. With this optimization ideally we only need
  

• * 1 CTB to register the context during the first pin and 1 CTB to
  

• * deregister the context when the context is destroyed. Without this
  

• * optimization, a CTB would be needed every pin & unpin.
  

• *
  

• * XXX: Need to acqiure the runtime wakeref as this can be triggered
  

• * from context_free_worker when not runtime wakeref is held.
  

• * guc_lrc_desc_unpin requires the runtime as a GuC register is written
  

• * in H2G CTB to deregister the context. A future patch may defer this
  

• * H2G CTB if the runtime wakeref is zero.
  

• */
  

• with_intel_runtime_pm(runtime_pm, wakeref)

• guc_lrc_desc_unpin(ce);
  

+}

+static int guc_context_alloc(struct intel_context *ce) +{

• return lrc_alloc(ce, ce->engine);

+}

static const struct intel_context_ops guc_context_ops = { .alloc = guc_context_alloc,

.pre_pin = guc_context_pre_pin, .pin = guc_context_pin,

• .unpin = lrc_unpin,
• .post_unpin = lrc_post_unpin,

• .unpin = guc_context_unpin,

• .post_unpin = guc_context_post_unpin,

  .enter = intel_context_enter_engine, .exit = intel_context_exit_engine,

  .reset = lrc_reset,

• .destroy = lrc_destroy,

• .destroy = guc_context_destroy,

};

-static int guc_request_alloc(struct i915_request *request) +static bool context_needs_register(struct intel_context *ce, bool new_guc_id) {

• return new_guc_id || test_bit(CONTEXT_LRCA_DIRTY, &ce->flags) ||

• !lrc_desc_registered(ce_to_guc(ce), ce->guc_id);
  

+}

+static int guc_request_alloc(struct i915_request *rq) +{

• struct intel_context *ce = rq->context;
• struct intel_guc *guc = ce_to_guc(ce); int ret;

• GEM_BUG_ON(!intel_context_is_pinned(request->context));

• GEM_BUG_ON(!intel_context_is_pinned(rq->context));

  /*

  • Flush enough space to reduce the likelihood of waiting after
  • we start building the request - in which case we will just
  • have to repeat work.

  */

• request->reserved_space += GUC_REQUEST_SIZE;

• rq->reserved_space += GUC_REQUEST_SIZE;

  /*

  • Note that after this point, we have committed to using

@@ -483,56 +954,47 @@ static int guc_request_alloc(struct i915_request *request) */

/* Unconditionally invalidate GPU caches and TLBs. */

• ret = request->engine->emit_flush(request, EMIT_INVALIDATE);

• ret = rq->engine->emit_flush(rq, EMIT_INVALIDATE); if (ret) return ret;

• request->reserved_space -= GUC_REQUEST_SIZE;
• return 0;

-}

-static inline void queue_request(struct i915_sched_engine *sched_engine,

• 		 struct i915_request *rq,
  

• 		 int prio)
  

-{

• GEM_BUG_ON(!list_empty(&rq->sched.link));
• list_add_tail(&rq->sched.link,

•       i915_sched_lookup_priolist(sched_engine, prio));
  

• set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);

-}

-static int guc_bypass_tasklet_submit(struct intel_guc *guc,

• 		     struct i915_request *rq)
  

-{

• int ret;
• __i915_request_submit(rq);

• rq->reserved_space -= GUC_REQUEST_SIZE;

• trace_i915_request_in(rq, 0);
• guc_set_lrc_tail(rq);
• ret = guc_add_request(guc, rq);
• if (ret == -EBUSY)

• guc->stalled_request = rq;
  

• return ret;

-}

-static void guc_submit_request(struct i915_request *rq) -{

• struct i915_sched_engine *sched_engine = rq->engine->sched_engine;
• struct intel_guc *guc = &rq->engine->gt->uc.guc;
• unsigned long flags;

• /*

• * Call pin_guc_id here rather than in the pinning step as with
  

• * dma_resv, contexts can be repeatedly pinned / unpinned trashing the
  

• * guc_ids and creating horrible race conditions. This is especially bad
  

• * when guc_ids are being stolen due to over subscription. By the time
  

• * this function is reached, it is guaranteed that the guc_id will be
  

• * persistent until the generated request is retired. Thus, sealing these
  

• * race conditions. It is still safe to fail here if guc_ids are
  

• * exhausted and return -EAGAIN to the user indicating that they can try
  

• * again in the future.
  

• *
  

• * There is no need for a lock here as the timeline mutex ensures at
  

• * most one context can be executing this code path at once. The
  

• * guc_id_ref is incremented once for every request in flight and
  

• * decremented on each retire. When it is zero, a lock around the
  

• * increment (in pin_guc_id) is needed to seal a race with unpin_guc_id.
  

• */
  

• if (atomic_add_unless(&ce->guc_id_ref, 1, 0))

• return 0;
  
  

• /* Will be called from irq-context when using foreign fences. */
• spin_lock_irqsave(&sched_engine->lock, flags);

• ret = pin_guc_id(guc, ce); /* returns 1 if new guc_id assigned */
• if (unlikely(ret < 0))

• return ret;;
  

• if (context_needs_register(ce, !!ret)) {

• ret = guc_lrc_desc_pin(ce);
  

• if (unlikely(ret)) {	/* unwind */
  

• 	atomic_dec(&ce->guc_id_ref);
  

• 	unpin_guc_id(guc, ce);
  

• 	return ret;
  

• }
  

• }

• if (guc->stalled_request || !i915_sched_engine_is_empty(sched_engine))

• queue_request(sched_engine, rq, rq_prio(rq));
  

• else if (guc_bypass_tasklet_submit(guc, rq) == -EBUSY)

• tasklet_hi_schedule(&sched_engine->tasklet);
  

• clear_bit(CONTEXT_LRCA_DIRTY, &ce->flags);

• spin_unlock_irqrestore(&sched_engine->lock, flags);

• return 0;

}

static void sanitize_hwsp(struct intel_engine_cs *engine) @@ -606,6 +1068,46 @@ static void guc_set_default_submission(struct intel_engine_cs *engine) engine->submit_request = guc_submit_request; }

+static inline void guc_kernel_context_pin(struct intel_guc *guc,

• 			  struct intel_context *ce)
  

+{

• if (context_guc_id_invalid(ce))

• pin_guc_id(guc, ce);
  

• guc_lrc_desc_pin(ce);

+}

+static inline void guc_init_lrc_mapping(struct intel_guc *guc) +{

• struct intel_gt *gt = guc_to_gt(guc);
• struct intel_engine_cs *engine;
• enum intel_engine_id id;
• /* make sure all descriptors are clean... */
• xa_destroy(&guc->context_lookup);
• /*

• * Some contexts might have been pinned before we enabled GuC
  

• * submission, so we need to add them to the GuC bookeeping.
  

• * Also, after a reset the GuC we want to make sure that the information
  

• * shared with GuC is properly reset. The kernel lrcs are not attached
  

• * to the gem_context, so they need to be added separately.
  

• *
  

• * Note: we purposely do not check the error return of
  

• * guc_lrc_desc_pin, because that function can only fail in two cases.
  

• * One, if there aren't enough free IDs, but we're guaranteed to have
  

• * enough here (we're either only pinning a handful of lrc on first boot
  

• * or we're re-pinning lrcs that were already pinned before the reset).
  

• * Two, if the GuC has died and CTBs can't make forward progress.
  

• * Presumably, the GuC should be alive as this function is called on
  

• * driver load or after a reset. Even if it is dead, another full GPU
  

• * reset will be triggered and this function would be called again.
  

• */
  

• for_each_engine(engine, gt, id)

• if (engine->kernel_context)
  

• 	guc_kernel_context_pin(guc, engine->kernel_context);
  

+}

static void guc_release(struct intel_engine_cs *engine) { engine->sanitize = NULL; /* no longer in control, nothing to sanitize */ @@ -718,6 +1220,7 @@ int intel_guc_submission_setup(struct intel_engine_cs *engine)

void intel_guc_submission_enable(struct intel_guc *guc) {

• guc_init_lrc_mapping(guc);

}

void intel_guc_submission_disable(struct intel_guc *guc) @@ -743,3 +1246,62 @@ void intel_guc_submission_init_early(struct intel_guc *guc) { guc->submission_selected = __guc_submission_selected(guc); }

+static inline struct intel_context * +g2h_context_lookup(struct intel_guc *guc, u32 desc_idx) +{

• struct intel_context *ce;
• if (unlikely(desc_idx >= GUC_MAX_LRC_DESCRIPTORS)) {

• drm_dbg(&guc_to_gt(guc)->i915->drm,
  

• 	"Invalid desc_idx %u", desc_idx);