[Intel-gfx] [PATCH 1/3] drm/i915/guc: Add non blocking CTB send function
Matthew Brost
matthew.brost at intel.com
Fri Nov 22 01:34:22 UTC 2019
On Thu, Nov 21, 2019 at 04:13:25PM -0800, Matthew Brost wrote:
>On Thu, Nov 21, 2019 at 12:43:26PM +0100, Michal Wajdeczko wrote:
>>On Thu, 21 Nov 2019 00:56:02 +0100, <John.C.Harrison at intel.com> wrote:
>>
>>>From: Matthew Brost <matthew.brost at intel.com>
>>>
>>>Add non blocking CTB send fuction, intel_guc_send_nb. In order to
>>>support a non blocking CTB send fuction a spin lock is needed to
>>
>>2x typos
>>
>>>protect the CTB descriptors fields. Also the non blocking call must not
>>>update the fence value as this value is owned by the blocking call
>>>(intel_guc_send).
>>
>>you probably mean "intel_guc_send_ct", as intel_guc_send is just a wrapper
>>around guc->send
>>
>
>Ah, yes.
>
>>>
>>>The 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 properly sized buffer. A retry
>>>counter is also implemented which fails H2G CTBs once a limit is
>>>reached to prevent deadlock.
>>>
>>>The function, intel_guc_send_nb, is exported in this patch but unused.
>>>Several patches later in the series make use of this function.
>>
>>It's likely in yet another series
>>
>
>Yes, it is.
>
>>>
>>>Cc: John Harrison <john.c.harrison at intel.com>
>>>Signed-off-by: Matthew Brost <matthew.brost at intel.com>
>>>---
>>>drivers/gpu/drm/i915/gt/uc/intel_guc.h | 2 +
>>>drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 97 +++++++++++++++++++----
>>>drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 10 ++-
>>>3 files changed, 91 insertions(+), 18 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>>b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>>index e6400204a2bd..77c5af919ace 100644
>>>--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>>+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>>@@ -94,6 +94,8 @@ intel_guc_send_and_receive(struct intel_guc
>>>*guc, const u32 *action, u32 len,
>>> return guc->send(guc, action, len, response_buf, response_buf_size);
>>>}
>>>+int intel_guc_send_nb(struct intel_guc_ct *ct, const u32 *action,
>>>u32 len);
>>>+
>>
>>Hmm, this mismatch of guc/ct parameter breaks the our layering.
>>But we can keep this layering intact by introducing some flags to
>>the existing guc_send() function. These flags could be passed as
>>high bits in action[0], like this:
>
>This seems reasonable.
>
Prototyped this and I don't like it all. First 'action' is a const so what you
are suggesting doesn't work unless that is changed. Also what if all bits in DW
eventually mean something, to me overloading a field isn't a good idea if
anything we should add another argument to guc->send(). But I'd honestly prefer
we just leave it as is. Non-blocking only applies to CTs (not MMIO) and we have
GEM_BUG_ON to protect us if this function is called incorrectly. Doing what you
suggest just makes everything more complicated IMO.
Matt
>>
>>#define GUC_ACTION_FLAG_DONT_WAIT 0x80000000
>>
>>int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset)
>>{
>> u32 action[] = {
>> INTEL_GUC_ACTION_AUTHENTICATE_HUC | GUC_ACTION_FLAG_DONT_WAIT,
>> rsa_offset
>> };
>>
>> return intel_guc_send(guc, action, ARRAY_SIZE(action));
>>}
>>
>>then actual back-end of guc->send can take proper steps based on this flag:
>>
>>@@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action,
>>u32 len,
>> GEM_BUG_ON(!len);
>> GEM_BUG_ON(len > guc->send_regs.count);
>>
>>+ if (*action & GUC_ACTION_FLAG_DONT_WAIT)
>>+ return -EINVAL;
>>+ *action &= ~GUC_ACTION_FLAG_DONT_WAIT;
>>+
>> /* We expect only action code */
>> GEM_BUG_ON(*action & ~INTEL_GUC_MSG_CODE_MASK);
>>
>>@@ @@ int intel_guc_send_ct(struct intel_guc *guc, const u32
>>*action, u32 len,
>> u32 status = ~0; /* undefined */
>> int ret;
>>
>>+ if (*action & GUC_ACTION_FLAG_DONT_WAIT) {
>>+ GEM_BUG_ON(response_buf);
>>+ GEM_BUG_ON(response_buf_size);
>>+ return ctch_send_nb(ct, ctch, action, len);
>>+ }
>>+
>> mutex_lock(&guc->send_mutex);
>>
>> ret = ctch_send(ct, ctch, action, len, response_buf,
>>response_buf_size,
>>
>>
>>>static inline void intel_guc_notify(struct intel_guc *guc)
>>>{
>>> guc->notify(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 b49115517510..e50d968b15d5 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,8 @@
>>> * Copyright © 2016-2019 Intel Corporation
>>> */
>>>+#include <linux/circ_buf.h>
>>>+
>>>#include "i915_drv.h"
>>>#include "intel_guc_ct.h"
>>>@@ -12,6 +14,8 @@
>>>#define CT_DEBUG_DRIVER(...) do { } while (0)
>>>#endif
>>>+#define MAX_RETRY 0x1000000
>>>+
>>>struct ct_request {
>>> struct list_head link;
>>> u32 fence;
>>>@@ -40,7 +44,8 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)
>>> /* we're using static channel owners */
>>> ct->host_channel.owner = CTB_OWNER_HOST;
>>>- spin_lock_init(&ct->lock);
>>>+ spin_lock_init(&ct->request_lock);
>>>+ spin_lock_init(&ct->send_lock);
>>> INIT_LIST_HEAD(&ct->pending_requests);
>>> INIT_LIST_HEAD(&ct->incoming_requests);
>>> INIT_WORK(&ct->worker, ct_incoming_request_worker_func);
>>>@@ -291,7 +296,8 @@ static u32 ctch_get_next_fence(struct
>>>intel_guc_ct_channel *ctch)
>>>static int ctb_write(struct intel_guc_ct_buffer *ctb,
>>> const u32 *action,
>>> u32 len /* in dwords */,
>>>- u32 fence,
>>>+ u32 fence_value,
>>>+ bool enable_fence,
>>
>>maybe we can just guarantee that fence=0 will never be used as a valid
>>fence id, then this flag could be replaced with (fence != 0) check.
>>
>
>Yes, again seems reasonable. Initialize next_fence = 1, then increment by 2 each
>time and this works.
>
>>> bool want_response)
>>>{
>>> struct guc_ct_buffer_desc *desc = ctb->desc;
>>>@@ -328,18 +334,18 @@ static int ctb_write(struct
>>>intel_guc_ct_buffer *ctb,
>>> * DW2+: action data
>>> */
>>> header = (len << GUC_CT_MSG_LEN_SHIFT) |
>>>- (GUC_CT_MSG_WRITE_FENCE_TO_DESC) |
>>>+ (enable_fence ? GUC_CT_MSG_WRITE_FENCE_TO_DESC : 0) |
>>
>>Hmm, even if we ask fw to do not write back fence to the descriptor,
>>IIRC current firmware will unconditionally write back return status
>>of this non-blocking call, possibly overwriting status of the blocked
>>call.
>>
>
>Yes, known problem with the interface that needs to be fixed.
>
>>> (want_response ? GUC_CT_MSG_SEND_STATUS : 0) |
>>
>>btw, if we switch all requests to expect reply send back over CTB,
>>then we can possibly drop the send_mutex in CTB paths, and block
>>only when there is no DONT_WAIT flag and we have to wait for response.
>>
>
>Rather just wait for the GuC to fix this.
>
>>> (action[0] << GUC_CT_MSG_ACTION_SHIFT);
>>> CT_DEBUG_DRIVER("CT: writing %*ph %*ph %*ph\n",
>>>- 4, &header, 4, &fence,
>>>+ 4, &header, 4, &fence_value,
>>> 4 * (len - 1), &action[1]);
>>> cmds[tail] = header;
>>> tail = (tail + 1) % size;
>>>- cmds[tail] = fence;
>>>+ cmds[tail] = fence_value;
>>> tail = (tail + 1) % size;
>>> for (i = 1; i < len; i++) {
>>>@@ -440,6 +446,47 @@ static int wait_for_ct_request_update(struct
>>>ct_request *req, u32 *status)
>>> return err;
>>>}
>>>+static inline bool ctb_has_room(struct guc_ct_buffer_desc *desc,
>>>u32 len)
>>>+{
>>>+ u32 head = READ_ONCE(desc->head);
>>>+ u32 space;
>>>+
>>>+ space = CIRC_SPACE(desc->tail, head, desc->size);
>>>+
>>>+ return space >= len;
>>>+}
>>>+
>>>+int intel_guc_send_nb(struct intel_guc_ct *ct,
>>>+ const u32 *action,
>>>+ u32 len)
>>>+{
>>>+ struct intel_guc_ct_channel *ctch = &ct->host_channel;
>>>+ struct intel_guc_ct_buffer *ctb = &ctch->ctbs[CTB_SEND];
>>>+ struct guc_ct_buffer_desc *desc = ctb->desc;
>>>+ int err;
>>>+
>>>+ GEM_BUG_ON(!ctch->enabled);
>>>+ GEM_BUG_ON(!len);
>>>+ GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
>>>+ lockdep_assert_held(&ct->send_lock);
>>
>>hmm, does it mean that now it's caller responsibility to spinlock
>>on CT private lock ? That is not how other guc_send() functions work.
>>
>
>Yes, that how I would like this work as I feel like it gives more flexability to
>the caller on the -EBUSY case. The caller can call intel_guc_send_nb again while
>still holding the lock or it release lock the and use a different form of flow
>control. Perhaps locking / unlocking should be exposed via static inlines rather
>than the caller directly manipulating the lock?
>
>>>+
>>>+ if (unlikely(!ctb_has_room(desc, (len + 1) * 4))) {
>>>+ ct->retry++;
>>>+ if (ct->retry >= MAX_RETRY)
>>>+ return -EDEADLK;
>>>+ else
>>>+ return -EBUSY;
>>>+ }
>>>+
>>>+ ct->retry = 0;
>>>+ err = ctb_write(ctb, action, len, 0, false, false);
>>>+ if (unlikely(err))
>>>+ return err;
>>>+
>>>+ intel_guc_notify(ct_to_guc(ct));
>>>+ return 0;
>>>+}
>>>+
>>>static int ctch_send(struct intel_guc_ct *ct,
>>> struct intel_guc_ct_channel *ctch,
>>> const u32 *action,
>>>@@ -460,17 +507,35 @@ static int ctch_send(struct intel_guc_ct *ct,
>>> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
>>> GEM_BUG_ON(!response_buf && response_buf_size);
>>>+ /*
>>>+ * 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.
>>>+ */
>>>+retry:
>>>+ spin_lock_irqsave(&ct->send_lock, flags);
>>>+ if (unlikely(!ctb_has_room(desc, (len + 1) * 4))) {
>>>+ spin_unlock_irqrestore(&ct->send_lock, flags);
>>>+ ct->retry++;
>>>+ if (ct->retry >= MAX_RETRY)
>>>+ return -EDEADLK;
>>
>>I'm not sure what's better: have secret deadlock hard to reproduce,
>>or deadlocks easier to catch that helps improve to be deadlock-clean
>>
>
>This is covering the case where the has died and to avoid deadlock. Eventually
>we will have some GuC health check code that will trigger a full GPU reset if
>the GuC has died. We need a way for code spinning on the CTBs to exit.
>
>I've already tweaked this code locally a bit to use an atomic too with the idea
>being the GuC health check code can set this value to have all code spinning on
>CTBs immediately return --EDEADLK when the GuC has died.
>
>>>+ cpu_relax();
>>>+ goto retry;
>>>+ }
>>>+
>>>+ ct->retry = 0;
>>> fence = ctch_get_next_fence(ctch);
>>> request.fence = fence;
>>> request.status = 0;
>>> request.response_len = response_buf_size;
>>> request.response_buf = response_buf;
>>>- spin_lock_irqsave(&ct->lock, flags);
>>>+ spin_lock(&ct->request_lock);
>>> list_add_tail(&request.link, &ct->pending_requests);
>>>- spin_unlock_irqrestore(&ct->lock, flags);
>>>+ spin_unlock(&ct->request_lock);
>>>- err = ctb_write(ctb, action, len, fence, !!response_buf);
>>>+ err = ctb_write(ctb, action, len, fence, true, !!response_buf);
>>>+ spin_unlock_irqrestore(&ct->send_lock, flags);
>>> if (unlikely(err))
>>> goto unlink;
>>>@@ -501,9 +566,9 @@ static int ctch_send(struct intel_guc_ct *ct,
>>> }
>>>unlink:
>>>- spin_lock_irqsave(&ct->lock, flags);
>>>+ spin_lock_irqsave(&ct->request_lock, flags);
>>> list_del(&request.link);
>>>- spin_unlock_irqrestore(&ct->lock, flags);
>>>+ spin_unlock_irqrestore(&ct->request_lock, flags);
>>> return err;
>>>}
>>>@@ -653,7 +718,7 @@ static int ct_handle_response(struct
>>>intel_guc_ct *ct, const u32 *msg)
>>> CT_DEBUG_DRIVER("CT: response fence %u status %#x\n", fence, status);
>>>- spin_lock(&ct->lock);
>>>+ spin_lock(&ct->request_lock);
>>> list_for_each_entry(req, &ct->pending_requests, link) {
>>> if (unlikely(fence != req->fence)) {
>>> CT_DEBUG_DRIVER("CT: request %u awaits response\n",
>>>@@ -672,7 +737,7 @@ static int ct_handle_response(struct
>>>intel_guc_ct *ct, const u32 *msg)
>>> found = true;
>>> break;
>>> }
>>>- spin_unlock(&ct->lock);
>>>+ spin_unlock(&ct->request_lock);
>>> if (!found)
>>> DRM_ERROR("CT: unsolicited response %*ph\n", 4 * msglen, msg);
>>>@@ -710,13 +775,13 @@ static bool
>>>ct_process_incoming_requests(struct intel_guc_ct *ct)
>>> u32 *payload;
>>> bool done;
>>>- spin_lock_irqsave(&ct->lock, flags);
>>>+ spin_lock_irqsave(&ct->request_lock, flags);
>>> request = list_first_entry_or_null(&ct->incoming_requests,
>>> struct ct_incoming_request, link);
>>> if (request)
>>> list_del(&request->link);
>>> done = !!list_empty(&ct->incoming_requests);
>>>- spin_unlock_irqrestore(&ct->lock, flags);
>>>+ spin_unlock_irqrestore(&ct->request_lock, flags);
>>> if (!request)
>>> return true;
>>>@@ -777,9 +842,9 @@ static int ct_handle_request(struct
>>>intel_guc_ct *ct, const u32 *msg)
>>> }
>>> memcpy(request->msg, msg, 4 * msglen);
>>>- spin_lock_irqsave(&ct->lock, flags);
>>>+ spin_lock_irqsave(&ct->request_lock, flags);
>>> list_add_tail(&request->link, &ct->incoming_requests);
>>>- spin_unlock_irqrestore(&ct->lock, flags);
>>>+ spin_unlock_irqrestore(&ct->request_lock, flags);
>>> queue_work(system_unbound_wq, &ct->worker);
>>> return 0;
>>>diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>>>b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>>>index 7c24d83f5c24..bc670a796bd8 100644
>>>--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>>>+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>>>@@ -62,8 +62,11 @@ struct intel_guc_ct {
>>> struct intel_guc_ct_channel host_channel;
>>> /* other channels are tbd */
>>>- /** @lock: protects pending requests list */
>>>- spinlock_t lock;
>>>+ /** @request_lock: protects pending requests list */
>>>+ spinlock_t request_lock;
>>>+
>>>+ /** @send_lock: protects h2g channel */
>>>+ spinlock_t send_lock;
>>> /** @pending_requests: list of requests waiting for response */
>>> struct list_head pending_requests;
>>>@@ -73,6 +76,9 @@ struct intel_guc_ct {
>>> /** @worker: worker for handling incoming requests */
>>> struct work_struct worker;
>>>+
>>>+ /** @retry: the number of times a H2G CTB has been retried */
>>>+ u32 retry;
>>>};
>>>void intel_guc_ct_init_early(struct intel_guc_ct *ct);
>>_______________________________________________
>>Intel-gfx mailing list
>>Intel-gfx at lists.freedesktop.org
>>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list