drm/i915/guc: rework guc_add_workqueue_item()

Mostly little optimisations and future-proofing against code breakage.
For instance, if the driver is correctly following the submission
protocol, the "out of space" condition is impossible, so the previous
runtime WARN_ON() is promoted to a GEM_BUG_ON() for a more dramatic
effect in development and less impact in end-user systems.

Similarly we can make alignment checking more stringent and replace
other WARN_ON() conditions that don't relate to the runtime hardware
state with either BUILD_BUG_ON() for compile-time-detectable issues, or
GEM_BUG_ON() for logical "can't happen" errors.

With those changes, we can convert it to void, as suggested by Chris
Wilson, and update the calling code appropriately.

v2:
    Note that we're now putting the request seqno in the "fence_id"
    field of each GuC-work-item, in case it turns up somewhere useful
    (e.g. in a GuC log) [Tvrtko Ursulin].

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 85b2b89eeb3a67ca9aae518145faae1676ac4561..42a85088657b47bc77740a5f9fc029c4cb479ff6 100644 (file)
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -486,23 +486,28 @@ int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
        return -EAGAIN;
 }
 
-static int guc_add_workqueue_item(struct i915_guc_client *gc,
-                                 struct drm_i915_gem_request *rq)
+static void guc_add_workqueue_item(struct i915_guc_client *gc,
+                                  struct drm_i915_gem_request *rq)
 {
+       /* wqi_len is in DWords, and does not include the one-word header */
+       const size_t wqi_size = sizeof(struct guc_wq_item);
+       const u32 wqi_len = wqi_size/sizeof(u32) - 1;
        struct guc_process_desc *desc;
        struct guc_wq_item *wqi;
        void *base;
-       u32 tail, wq_len, wq_off, space;
+       u32 freespace, tail, wq_off, wq_page;
 
        desc = gc->client_base + gc->proc_desc_offset;
-       space = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
-       if (WARN_ON(space < sizeof(struct guc_wq_item)))
-               return -ENOSPC; /* shouldn't happen */
 
-       /* postincrement WQ tail for next time */
-       wq_off = gc->wq_tail;
-       gc->wq_tail += sizeof(struct guc_wq_item);
-       gc->wq_tail &= gc->wq_size - 1;
+       /* Free space is guaranteed, see i915_guc_wq_check_space() above */
+       freespace = CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size);
+       GEM_BUG_ON(freespace < wqi_size);
+
+       /* The GuC firmware wants the tail index in QWords, not bytes */
+       tail = rq->tail;
+       GEM_BUG_ON(tail & 7);
+       tail >>= 3;
+       GEM_BUG_ON(tail > WQ_RING_TAIL_MAX);
 
        /* For now workqueue item is 4 DWs; workqueue buffer is 2 pages. So we
         * should not have the case where structure wqi is across page, neither
@@ -511,19 +516,23 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
         * XXX: if not the case, we need save data to a temp wqi and copy it to
         * workqueue buffer dw by dw.
         */
-       WARN_ON(sizeof(struct guc_wq_item) != 16);
-       WARN_ON(wq_off & 3);
+       BUILD_BUG_ON(wqi_size != 16);
+
+       /* postincrement WQ tail for next time */
+       wq_off = gc->wq_tail;
+       gc->wq_tail += wqi_size;
+       gc->wq_tail &= gc->wq_size - 1;
+       GEM_BUG_ON(wq_off & (wqi_size - 1));
 
-       /* wq starts from the page after doorbell / process_desc */
-       base = kmap_atomic(i915_gem_object_get_page(gc->client_obj,
-                       (wq_off + GUC_DB_SIZE) >> PAGE_SHIFT));
+       /* WQ starts from the page after doorbell / process_desc */
+       wq_page = (wq_off + GUC_DB_SIZE) >> PAGE_SHIFT;
        wq_off &= PAGE_SIZE - 1;
+       base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, wq_page));
        wqi = (struct guc_wq_item *)((char *)base + wq_off);
 
-       /* len does not include the header */
-       wq_len = sizeof(struct guc_wq_item) / sizeof(u32) - 1;
+       /* Now fill in the 4-word work queue item */
        wqi->header = WQ_TYPE_INORDER |
-                       (wq_len << WQ_LEN_SHIFT) |
+                       (wqi_len << WQ_LEN_SHIFT) |
                        (rq->engine->guc_id << WQ_TARGET_SHIFT) |
                        WQ_NO_WCFLUSH_WAIT;
 
@@ -531,14 +540,10 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc,
        wqi->context_desc = (u32)intel_lr_context_descriptor(rq->ctx,
                                                             rq->engine);
 
-       /* The GuC firmware wants the tail index in QWords, not bytes */
-       tail = rq->ringbuf->tail >> 3;
        wqi->ring_tail = tail << WQ_RING_TAIL_SHIFT;
-       wqi->fence_id = 0; /*XXX: what fence to be here */
+       wqi->fence_id = rq->seqno;
 
        kunmap_atomic(base);
-
-       return 0;
 }
 
 /**
@@ -565,26 +570,20 @@ int i915_guc_submit(struct drm_i915_gem_request *rq)
        unsigned int engine_id = rq->engine->guc_id;
        struct intel_guc *guc = &rq->i915->guc;
        struct i915_guc_client *client = guc->execbuf_client;
-       int q_ret, b_ret;
+       int b_ret;
 
-       q_ret = guc_add_workqueue_item(client, rq);
-       if (q_ret == 0)
-               b_ret = guc_ring_doorbell(client);
+       guc_add_workqueue_item(client, rq);
+       b_ret = guc_ring_doorbell(client);
 
        client->submissions[engine_id] += 1;
-       if (q_ret) {
-               client->q_fail += 1;
-               client->retcode = q_ret;
-       } else if (b_ret) {
+       client->retcode = b_ret;
+       if (b_ret)
                client->b_fail += 1;
-               client->retcode = q_ret = b_ret;
-       } else {
-               client->retcode = 0;
-       }
+
        guc->submissions[engine_id] += 1;
        guc->last_seqno[engine_id] = rq->seqno;
 
-       return q_ret;
+       return b_ret;
 }
 
 /*

diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 380a743f830f459e44ca454a4f0c7dd08121c46e..7b1a5a3fb106adf05ad368be388d7ae465abdfd4 100644 (file)
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -81,7 +81,7 @@ struct i915_guc_client {
        uint32_t unused;                /* Was 'wq_head'                */
 
        uint32_t no_wq_space;
-       uint32_t q_fail;
+       uint32_t q_fail;                /* No longer used               */
        uint32_t b_fail;
        int retcode;
 

diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 2de57ffe5e18b8a20538ff33e70b8cc679c46ca0..944786d7075b7c73631093c3240f091dda065c1d 100644 (file)
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -71,7 +71,8 @@
 #define   WQ_WORKLOAD_TOUCH            (2 << WQ_WORKLOAD_SHIFT)
 
 #define WQ_RING_TAIL_SHIFT             20
-#define WQ_RING_TAIL_MASK              (0x7FF << WQ_RING_TAIL_SHIFT)
+#define WQ_RING_TAIL_MAX               0x7FF   /* 2^11 QWords */
+#define WQ_RING_TAIL_MASK              (WQ_RING_TAIL_MAX << WQ_RING_TAIL_SHIFT)
 
 #define GUC_DOORBELL_ENABLED           1
 #define GUC_DOORBELL_DISABLED          0