On 27/04/16 19:03, Dave Gordon wrote:
Mostly little optimisations; 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 replace other WARN_ON() conditions that don't relate to
the 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.
Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 69 +++++++++++++++---------------
drivers/gpu/drm/i915/intel_guc.h | 2 +-
drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +-
3 files changed, 37 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 6626eff..4d2ea84 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -470,23 +470,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);
Again, u32 is correct I think.
+ 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 space, tail, wq_off, wq_page;
desc = gc->client_base + gc->proc_desc_offset;
+
+ /* Space was checked earlier, in i915_guc_wq_check_space() above */
It may be above in the file, but the two do not call one another so I
recommend saying exactly who called it.
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 */
+ GEM_BUG_ON(space < wqi_size);
It is impossible to hit this only because of the struct_mutex guarding
the whole time window from request creation to submission. If in the
future, near or far, that gets fixed, then this will need reworking.
I don't have any better ideas though.
But a WARN_ON and return would be almost as good. Everything is better
than a dead machine one can't ssh into...
So I appeal to make this a WARN_ON and return. Nothing bad would happen
apart from software thinking GPU has hung.
- /* 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;
+ /* The GuC firmware wants the tail index in QWords, not bytes */
+ tail = rq->tail;
Used to be sampled from rq->ringbuf->tail - are those the same?
+ 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
@@ -495,19 +500,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);
- /* 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));
+ /* 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));
Use to be wq_off & 3, now is wq_off & 15, which one is correct?
+
+ /* 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;
@@ -515,14 +524,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;
Not mentioned in the commit?
kunmap_atomic(base);
-
- return 0;
}
/**
@@ -537,26 +542,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;
What is b_ret out of interest, door*B*ell return code ?
- 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;
I wanted to ask for what is this for but then found it is for debugfs.
+ 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;
Could also kill the return value from this one, the only caller,
intel_logical_ring_advance_and_submit does not use it. That function
itself does not look like it needs a return value at the moment. :)
}
/*
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 436f2d6..10e1d5e 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -74,7 +74,7 @@ struct i915_guc_client {
/* GuC submission statistics & status */
uint64_t submissions[GUC_MAX_ENGINES_NUM];
uint32_t no_wq_space; /* Space pre-check failed */
- uint32_t q_fail; /* Failed to queue (MBZ) */
+ uint32_t q_fail; /* No longer used */
Get rid of it then?
uint32_t b_fail; /* Doorbell failure (MBZ) */
int retcode; /* Result of last guc_submit() */
};
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 2de57ff..944786d 100644
--- 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
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
