* [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling
@ 2020-07-20 9:23 Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 02/10] drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs Chris Wilson
` (12 more replies)
0 siblings, 13 replies; 26+ messages in thread
From: Chris Wilson @ 2020-07-20 9:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson
I915_GEM_THROTTLE dates back to the time before contexts where there was
just a single engine, and therefore a single timeline and request list
globally. That request list was in execution/retirement order, and so
walking it to find a particular aged request made sense and could be
split per file.
That is no more. We now have many timelines with a file, as many as the
user wants to construct (essentially per-engine, per-context). Each of
those run independently and so make the single list futile. Remove the
disordered list, and iterate over all the timelines to find a request to
wait on in each to satisfy the criteria that the CPU is no more than 20ms
ahead of its oldest request.
It should go without saying that the I915_GEM_THROTTLE ioctl is no
longer used as the primary means of throttling, so it makes sense to push
the complication into the ioctl where it only impacts upon its few
irregular users, rather than the execbuf/retire where everybody has to
pay the cost. Fortunately, the few users do not create vast amount of
contexts, so the loops over contexts/engines should be concise.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 13 ----
drivers/gpu/drm/i915/gem/i915_gem_throttle.c | 67 +++++++++++++------
drivers/gpu/drm/i915/gt/selftest_lrc.c | 5 +-
drivers/gpu/drm/i915/i915_drv.c | 1 -
drivers/gpu/drm/i915/i915_drv.h | 6 --
drivers/gpu/drm/i915/i915_gem.c | 18 -----
drivers/gpu/drm/i915/i915_request.c | 21 ------
drivers/gpu/drm/i915/i915_request.h | 4 --
8 files changed, 50 insertions(+), 85 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 6b4ec66cb558..b7a86cdec9b5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1916,18 +1916,6 @@ static int eb_parse(struct i915_execbuffer *eb)
return err;
}
-static void
-add_to_client(struct i915_request *rq, struct drm_file *file)
-{
- struct drm_i915_file_private *file_priv = file->driver_priv;
-
- rq->file_priv = file_priv;
-
- spin_lock(&file_priv->mm.lock);
- list_add_tail(&rq->client_link, &file_priv->mm.request_list);
- spin_unlock(&file_priv->mm.lock);
-}
-
static int eb_submit(struct i915_execbuffer *eb, struct i915_vma *batch)
{
int err;
@@ -2567,7 +2555,6 @@ i915_gem_do_execbuffer(struct drm_device *dev,
trace_i915_request_queue(eb.request, eb.batch_flags);
err = eb_submit(&eb, batch);
err_request:
- add_to_client(eb.request, file);
i915_request_get(eb.request);
eb_request_add(&eb);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_throttle.c b/drivers/gpu/drm/i915/gem/i915_gem_throttle.c
index 540ef0551789..1929d6cf4150 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_throttle.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_throttle.c
@@ -9,6 +9,7 @@
#include <drm/drm_file.h>
#include "i915_drv.h"
+#include "i915_gem_context.h"
#include "i915_gem_ioctls.h"
#include "i915_gem_object.h"
@@ -35,9 +36,10 @@ int
i915_gem_throttle_ioctl(struct drm_device *dev, void *data,
struct drm_file *file)
{
+ const unsigned long recent_enough = jiffies - DRM_I915_THROTTLE_JIFFIES;
struct drm_i915_file_private *file_priv = file->driver_priv;
- unsigned long recent_enough = jiffies - DRM_I915_THROTTLE_JIFFIES;
- struct i915_request *request, *target = NULL;
+ struct i915_gem_context *ctx;
+ unsigned long idx;
long ret;
/* ABI: return -EIO if already wedged */
@@ -45,27 +47,54 @@ i915_gem_throttle_ioctl(struct drm_device *dev, void *data,
if (ret)
return ret;
- spin_lock(&file_priv->mm.lock);
- list_for_each_entry(request, &file_priv->mm.request_list, client_link) {
- if (time_after_eq(request->emitted_jiffies, recent_enough))
- break;
+ rcu_read_lock();
+ xa_for_each(&file_priv->context_xa, idx, ctx) {
+ struct i915_gem_engines_iter it;
+ struct intel_context *ce;
- if (target && xchg(&target->file_priv, NULL))
- list_del(&target->client_link);
+ if (!kref_get_unless_zero(&ctx->ref))
+ continue;
+ rcu_read_unlock();
- target = request;
- }
- if (target)
- i915_request_get(target);
- spin_unlock(&file_priv->mm.lock);
+ for_each_gem_engine(ce,
+ i915_gem_context_lock_engines(ctx),
+ it) {
+ struct i915_request *rq, *target = NULL;
+
+ if (!ce->timeline)
+ continue;
+
+ mutex_lock(&ce->timeline->mutex);
+ list_for_each_entry_reverse(rq,
+ &ce->timeline->requests,
+ link) {
+ if (i915_request_completed(rq))
+ break;
- if (!target)
- return 0;
+ if (time_after(rq->emitted_jiffies,
+ recent_enough))
+ continue;
- ret = i915_request_wait(target,
- I915_WAIT_INTERRUPTIBLE,
- MAX_SCHEDULE_TIMEOUT);
- i915_request_put(target);
+ target = i915_request_get(rq);
+ break;
+ }
+ mutex_unlock(&ce->timeline->mutex);
+ if (!target)
+ continue;
+
+ ret = i915_request_wait(target,
+ I915_WAIT_INTERRUPTIBLE,
+ MAX_SCHEDULE_TIMEOUT);
+ i915_request_put(target);
+ if (ret < 0)
+ break;
+ }
+ i915_gem_context_unlock_engines(ctx);
+ i915_gem_context_put(ctx);
+
+ rcu_read_lock();
+ }
+ rcu_read_unlock();
return ret < 0 ? ret : 0;
}
diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
index 3fc5de961280..f749071f54a7 100644
--- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
@@ -2729,7 +2729,7 @@ static int create_gang(struct intel_engine_cs *engine,
i915_gem_object_put(obj);
intel_context_put(ce);
- rq->client_link.next = &(*prev)->client_link;
+ rq->mock.link.next = &(*prev)->mock.link;
*prev = rq;
return 0;
@@ -2970,8 +2970,7 @@ static int live_preempt_gang(void *arg)
}
while (rq) { /* wait for each rq from highest to lowest prio */
- struct i915_request *n =
- list_next_entry(rq, client_link);
+ struct i915_request *n = list_next_entry(rq, mock.link);
if (err == 0 && i915_request_wait(rq, 0, HZ / 5) < 0) {
struct drm_printer p =
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5fd5af4bc855..a5f58ed219fe 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1119,7 +1119,6 @@ static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file)
struct drm_i915_file_private *file_priv = file->driver_priv;
i915_gem_context_close(file);
- i915_gem_release(dev, file);
kfree_rcu(file_priv, rcu);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 56dfc6d98caa..cb848ee6a19d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -203,11 +203,6 @@ struct drm_i915_file_private {
struct rcu_head rcu;
};
- struct {
- spinlock_t lock;
- struct list_head request_list;
- } mm;
-
struct xarray context_xa;
struct xarray vm_xa;
@@ -1839,7 +1834,6 @@ void i915_gem_suspend_late(struct drm_i915_private *dev_priv);
void i915_gem_resume(struct drm_i915_private *dev_priv);
int i915_gem_open(struct drm_i915_private *i915, struct drm_file *file);
-void i915_gem_release(struct drm_device *dev, struct drm_file *file);
int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
enum i915_cache_level cache_level);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9aa3066cb75d..e1de50780ed5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1301,21 +1301,6 @@ int i915_gem_freeze_late(struct drm_i915_private *i915)
return 0;
}
-void i915_gem_release(struct drm_device *dev, struct drm_file *file)
-{
- struct drm_i915_file_private *file_priv = file->driver_priv;
- struct i915_request *request;
-
- /* Clean up our request list when the client is going away, so that
- * later retire_requests won't dereference our soon-to-be-gone
- * file_priv.
- */
- spin_lock(&file_priv->mm.lock);
- list_for_each_entry(request, &file_priv->mm.request_list, client_link)
- request->file_priv = NULL;
- spin_unlock(&file_priv->mm.lock);
-}
-
int i915_gem_open(struct drm_i915_private *i915, struct drm_file *file)
{
struct drm_i915_file_private *file_priv;
@@ -1331,9 +1316,6 @@ int i915_gem_open(struct drm_i915_private *i915, struct drm_file *file)
file_priv->dev_priv = i915;
file_priv->file = file;
- spin_lock_init(&file_priv->mm.lock);
- INIT_LIST_HEAD(&file_priv->mm.request_list);
-
file_priv->bsd_engine = -1;
file_priv->hang_timestamp = jiffies;
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 679a915e9a63..050b55f0f5c0 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -216,24 +216,6 @@ static void __notify_execute_cb_imm(struct i915_request *rq)
__notify_execute_cb(rq, irq_work_imm);
}
-static inline void
-remove_from_client(struct i915_request *request)
-{
- struct drm_i915_file_private *file_priv;
-
- if (!READ_ONCE(request->file_priv))
- return;
-
- rcu_read_lock();
- file_priv = xchg(&request->file_priv, NULL);
- if (file_priv) {
- spin_lock(&file_priv->mm.lock);
- list_del(&request->client_link);
- spin_unlock(&file_priv->mm.lock);
- }
- rcu_read_unlock();
-}
-
static void free_capture_list(struct i915_request *request)
{
struct i915_capture_list *capture;
@@ -341,7 +323,6 @@ bool i915_request_retire(struct i915_request *rq)
remove_from_engine(rq);
GEM_BUG_ON(!llist_empty(&rq->execute_cb));
- remove_from_client(rq);
__list_del_entry(&rq->link); /* poison neither prev/next (RCU walks) */
intel_context_exit(rq->context);
@@ -799,7 +780,6 @@ static void __i915_request_ctor(void *arg)
dma_fence_init(&rq->fence, &i915_fence_ops, &rq->lock, 0, 0);
- rq->file_priv = NULL;
rq->capture_list = NULL;
init_llist_head(&rq->execute_cb);
@@ -889,7 +869,6 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
/* No zalloc, everything must be cleared after use */
rq->batch = NULL;
- GEM_BUG_ON(rq->file_priv);
GEM_BUG_ON(rq->capture_list);
GEM_BUG_ON(!llist_empty(&rq->execute_cb));
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index 590762820761..fc18378c685d 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -284,10 +284,6 @@ struct i915_request {
/** timeline->request entry for this request */
struct list_head link;
- struct drm_i915_file_private *file_priv;
- /** file_priv list entry for this request */
- struct list_head client_link;
-
I915_SELFTEST_DECLARE(struct {
struct list_head link;
unsigned long delay;
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Intel-gfx] [PATCH 02/10] drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
@ 2020-07-20 9:23 ` Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 03/10] drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs Chris Wilson
` (11 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Chris Wilson @ 2020-07-20 9:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson
Since the breadcrumb enabling/cancelling itself is serialised by the
breadcrumbs.irq_lock, with a bit of care we can remove the outer
serialisation with i915_request.lock for concurrent
dma_fence_enable_signaling(). This has the important side-effect of
eliminating the nested i915_request.lock within request submission.
The challenge in serialisation is around the unsubmission where we take
an active request that wants a breadcrumb on the signaling engine and
put it to sleep. We do not want a concurrent
dma_fence_enable_signaling() to attach a breadcrumb as we unsubmit, so
we must mark the request as no longer active before serialising with the
concurrent enable-signaling.
On retire, we serialise with the concurrent enable-signaling, but
instead of clearing ACTIVE, we mark it as SIGNALED.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 130 +++++++++++++-------
drivers/gpu/drm/i915/gt/intel_lrc.c | 14 ---
drivers/gpu/drm/i915/i915_request.c | 39 +++---
3 files changed, 100 insertions(+), 83 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index 91786310c114..3d211a0c2b5a 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -220,17 +220,17 @@ static void signal_irq_work(struct irq_work *work)
}
}
-static bool __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
+static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
{
struct intel_engine_cs *engine =
container_of(b, struct intel_engine_cs, breadcrumbs);
lockdep_assert_held(&b->irq_lock);
if (b->irq_armed)
- return true;
+ return;
if (!intel_gt_pm_get_if_awake(engine->gt))
- return false;
+ return;
/*
* The breadcrumb irq will be disarmed on the interrupt after the
@@ -250,8 +250,6 @@ static bool __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
if (!b->irq_enabled++)
irq_enable(engine);
-
- return true;
}
void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
@@ -310,57 +308,99 @@ void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
{
}
-bool i915_request_enable_breadcrumb(struct i915_request *rq)
+static void insert_breadcrumb(struct i915_request *rq,
+ struct intel_breadcrumbs *b)
{
- lockdep_assert_held(&rq->lock);
+ struct intel_context *ce = rq->context;
+ struct list_head *pos;
- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
- return true;
+ if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
+ return;
- if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags)) {
- struct intel_breadcrumbs *b = &rq->engine->breadcrumbs;
- struct intel_context *ce = rq->context;
- struct list_head *pos;
+ __intel_breadcrumbs_arm_irq(b);
- spin_lock(&b->irq_lock);
+ /*
+ * We keep the seqno in retirement order, so we can break
+ * inside intel_engine_signal_breadcrumbs as soon as we've
+ * passed the last completed request (or seen a request that
+ * hasn't event started). We could walk the timeline->requests,
+ * but keeping a separate signalers_list has the advantage of
+ * hopefully being much smaller than the full list and so
+ * provides faster iteration and detection when there are no
+ * more interrupts required for this context.
+ *
+ * We typically expect to add new signalers in order, so we
+ * start looking for our insertion point from the tail of
+ * the list.
+ */
+ list_for_each_prev(pos, &ce->signals) {
+ struct i915_request *it =
+ list_entry(pos, typeof(*it), signal_link);
- if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
- goto unlock;
+ if (i915_seqno_passed(rq->fence.seqno, it->fence.seqno))
+ break;
+ }
+ list_add(&rq->signal_link, pos);
+ if (pos == &ce->signals) /* catch transitions from empty list */
+ list_move_tail(&ce->signal_link, &b->signalers);
+ GEM_BUG_ON(!check_signal_order(ce, rq));
- if (!__intel_breadcrumbs_arm_irq(b))
- goto unlock;
+ set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
+}
- /*
- * We keep the seqno in retirement order, so we can break
- * inside intel_engine_signal_breadcrumbs as soon as we've
- * passed the last completed request (or seen a request that
- * hasn't event started). We could walk the timeline->requests,
- * but keeping a separate signalers_list has the advantage of
- * hopefully being much smaller than the full list and so
- * provides faster iteration and detection when there are no
- * more interrupts required for this context.
- *
- * We typically expect to add new signalers in order, so we
- * start looking for our insertion point from the tail of
- * the list.
- */
- list_for_each_prev(pos, &ce->signals) {
- struct i915_request *it =
- list_entry(pos, typeof(*it), signal_link);
+bool i915_request_enable_breadcrumb(struct i915_request *rq)
+{
+ struct intel_breadcrumbs *b;
- if (i915_seqno_passed(rq->fence.seqno, it->fence.seqno))
- break;
- }
- list_add(&rq->signal_link, pos);
- if (pos == &ce->signals) /* catch transitions from empty list */
- list_move_tail(&ce->signal_link, &b->signalers);
- GEM_BUG_ON(!check_signal_order(ce, rq));
+ /* Serialises with i915_request_retire() using rq->lock */
+ if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
+ return true;
- set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
-unlock:
+ /*
+ * Peek at i915_request_submit()/i915_request_unsubmit() status.
+ *
+ * If the request is not yet active (and not signaled), we will
+ * attach the breadcrumb later.
+ */
+ if (!test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
+ return true;
+
+ /*
+ * rq->engine is locked by rq->engine->active.lock. That however
+ * is not known until after rq->engine has been dereferenced and
+ * the lock acquired. Hence we acquire the lock and then validate
+ * that rq->engine still matches the lock we hold for it.
+ *
+ * Here, we are using the breadcrumb lock as a proxy for the
+ * rq->engine->active.lock, and we know that since the breadcrumb
+ * will be serialised within i915_request_submit/i915_request_unsubmit,
+ * the engine cannot change while active as long as we hold the
+ * breadcrumb lock on that engine.
+ *
+ * From the dma_fence_enable_signaling() path, we are outside of the
+ * request submit/unsubmit path, and so we must be more careful to
+ * acquire the right lock.
+ */
+ b = &READ_ONCE(rq->engine)->breadcrumbs;
+ spin_lock(&b->irq_lock);
+ while (unlikely(b != &READ_ONCE(rq->engine)->breadcrumbs)) {
spin_unlock(&b->irq_lock);
+ b = &READ_ONCE(rq->engine)->breadcrumbs;
+ spin_lock(&b->irq_lock);
}
+ /*
+ * Now that we are finally serialised with request submit/unsubmit,
+ * [with b->irq_lock] and with i915_request_retire() [via checking
+ * SIGNALED with rq->lock] confirm the request is indeed active. If
+ * it is no longer active, the breadcrumb will be attached upon
+ * i915_request_submit().
+ */
+ if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
+ insert_breadcrumb(rq, b);
+
+ spin_unlock(&b->irq_lock);
+
return !__request_completed(rq);
}
@@ -368,8 +408,6 @@ void i915_request_cancel_breadcrumb(struct i915_request *rq)
{
struct intel_breadcrumbs *b = &rq->engine->breadcrumbs;
- lockdep_assert_held(&rq->lock);
-
/*
* We must wait for b->irq_lock so that we know the interrupt handler
* has released its reference to the intel_context and has completed
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 29c0fde8b4df..21c16e31c4fe 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1148,20 +1148,6 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
} else {
struct intel_engine_cs *owner = rq->context->engine;
- /*
- * Decouple the virtual breadcrumb before moving it
- * back to the virtual engine -- we don't want the
- * request to complete in the background and try
- * and cancel the breadcrumb on the virtual engine
- * (instead of the old engine where it is linked)!
- */
- if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
- &rq->fence.flags)) {
- spin_lock_nested(&rq->lock,
- SINGLE_DEPTH_NESTING);
- i915_request_cancel_breadcrumb(rq);
- spin_unlock(&rq->lock);
- }
WRITE_ONCE(rq->engine, owner);
owner->submit_request(rq);
active = NULL;
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 050b55f0f5c0..58e37f96ae21 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -304,11 +304,12 @@ bool i915_request_retire(struct i915_request *rq)
dma_fence_signal_locked(&rq->fence);
if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &rq->fence.flags))
i915_request_cancel_breadcrumb(rq);
+ spin_unlock_irq(&rq->lock);
+
if (i915_request_has_waitboost(rq)) {
GEM_BUG_ON(!atomic_read(&rq->engine->gt->rps.num_waiters));
atomic_dec(&rq->engine->gt->rps.num_waiters);
}
- spin_unlock_irq(&rq->lock);
/*
* We only loosely track inflight requests across preemption,
@@ -591,17 +592,9 @@ bool __i915_request_submit(struct i915_request *request)
*/
__notify_execute_cb_irq(request);
- /* We may be recursing from the signal callback of another i915 fence */
- if (!i915_request_signaled(request)) {
- spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
-
- if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
- &request->fence.flags) &&
- !i915_request_enable_breadcrumb(request))
- intel_engine_signal_breadcrumbs(engine);
-
- spin_unlock(&request->lock);
- }
+ if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags) &&
+ !i915_request_enable_breadcrumb(request))
+ intel_engine_signal_breadcrumbs(engine);
return result;
}
@@ -623,27 +616,27 @@ void __i915_request_unsubmit(struct i915_request *request)
{
struct intel_engine_cs *engine = request->engine;
+ /*
+ * Only unwind in reverse order, required so that the per-context list
+ * is kept in seqno/ring order.
+ */
RQ_TRACE(request, "\n");
GEM_BUG_ON(!irqs_disabled());
lockdep_assert_held(&engine->active.lock);
/*
- * Only unwind in reverse order, required so that the per-context list
- * is kept in seqno/ring order.
+ * Before we remove this breadcrumb from the signal list, we have
+ * to ensure that a concurrent dma_fence_enable_signaling() does not
+ * attach itself. We first mark the request as no longer active and
+ * make sure that is visible to other cores, and then remove the
+ * breadcrumb if attached.
*/
-
- /* We may be recursing from the signal callback of another i915 fence */
- spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
-
+ GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags));
+ clear_bit_unlock(I915_FENCE_FLAG_ACTIVE, &request->fence.flags);
if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags))
i915_request_cancel_breadcrumb(request);
- GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags));
- clear_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags);
-
- spin_unlock(&request->lock);
-
/* We've already spun, don't charge on resubmitting. */
if (request->sched.semaphores && i915_request_started(request))
request->sched.semaphores = 0;
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Intel-gfx] [PATCH 03/10] drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 02/10] drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs Chris Wilson
@ 2020-07-20 9:23 ` Chris Wilson
2020-07-20 9:32 ` Tvrtko Ursulin
2020-07-20 9:23 ` [Intel-gfx] [PATCH 04/10] drm/i915/gt: Only transfer the virtual context to the new engine if active Chris Wilson
` (10 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Chris Wilson @ 2020-07-20 9:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson
After staring at the breadcrumb enabling/cancellation and coming to the
conclusion that the cause of the mysterious stale breadcrumbs must the
act of submitting a completed requests, we can then redirect those
completed requests onto a dedicated signaled_list at the time of
construction and so eliminate intel_engine_transfer_stale_breadcrumbs().
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 50 ++++++++-------------
drivers/gpu/drm/i915/gt/intel_engine.h | 3 --
drivers/gpu/drm/i915/gt/intel_lrc.c | 15 -------
drivers/gpu/drm/i915/i915_request.c | 5 +--
4 files changed, 21 insertions(+), 52 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index 3d211a0c2b5a..fbdc465a5870 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -142,16 +142,16 @@ static void add_retire(struct intel_breadcrumbs *b, struct intel_timeline *tl)
intel_engine_add_retire(engine, tl);
}
-static void __signal_request(struct i915_request *rq, struct list_head *signals)
+static bool __signal_request(struct i915_request *rq, struct list_head *signals)
{
- GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags));
clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
if (!__dma_fence_signal(&rq->fence))
- return;
+ return false;
i915_request_get(rq);
list_add_tail(&rq->signal_link, signals);
+ return true;
}
static void signal_irq_work(struct irq_work *work)
@@ -278,32 +278,6 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
spin_unlock_irqrestore(&b->irq_lock, flags);
}
-void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
- struct intel_context *ce)
-{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
- unsigned long flags;
-
- spin_lock_irqsave(&b->irq_lock, flags);
- if (!list_empty(&ce->signals)) {
- struct i915_request *rq, *next;
-
- /* Queue for executing the signal callbacks in the irq_work */
- list_for_each_entry_safe(rq, next, &ce->signals, signal_link) {
- GEM_BUG_ON(rq->engine != engine);
- GEM_BUG_ON(!__request_completed(rq));
-
- __signal_request(rq, &b->signaled_requests);
- }
-
- INIT_LIST_HEAD(&ce->signals);
- list_del_init(&ce->signal_link);
-
- irq_work_queue(&b->irq_work);
- }
- spin_unlock_irqrestore(&b->irq_lock, flags);
-}
-
void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
{
}
@@ -317,6 +291,17 @@ static void insert_breadcrumb(struct i915_request *rq,
if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
return;
+ /*
+ * If the request is already completed, we can transfer it
+ * straight onto a signaled list, and queue the irq worker for
+ * its signal completion.
+ */
+ if (__request_completed(rq)) {
+ if (__signal_request(rq, &b->signaled_requests))
+ irq_work_queue(&b->irq_work);
+ return;
+ }
+
__intel_breadcrumbs_arm_irq(b);
/*
@@ -344,8 +329,11 @@ static void insert_breadcrumb(struct i915_request *rq,
if (pos == &ce->signals) /* catch transitions from empty list */
list_move_tail(&ce->signal_link, &b->signalers);
GEM_BUG_ON(!check_signal_order(ce, rq));
-
set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
+
+ /* Check after attaching to irq, interrupt may have already fired. */
+ if (__request_completed(rq))
+ irq_work_queue(&b->irq_work);
}
bool i915_request_enable_breadcrumb(struct i915_request *rq)
@@ -401,7 +389,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
spin_unlock(&b->irq_lock);
- return !__request_completed(rq);
+ return true;
}
void i915_request_cancel_breadcrumb(struct i915_request *rq)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index a9249a23903a..faf00a353e25 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -237,9 +237,6 @@ intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
-void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
- struct intel_context *ce);
-
void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
struct drm_printer *p);
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 21c16e31c4fe..88a5c155154d 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1805,18 +1805,6 @@ static bool virtual_matches(const struct virtual_engine *ve,
return true;
}
-static void virtual_xfer_breadcrumbs(struct virtual_engine *ve)
-{
- /*
- * All the outstanding signals on ve->siblings[0] must have
- * been completed, just pending the interrupt handler. As those
- * signals still refer to the old sibling (via rq->engine), we must
- * transfer those to the old irq_worker to keep our locking
- * consistent.
- */
- intel_engine_transfer_stale_breadcrumbs(ve->siblings[0], &ve->context);
-}
-
#define for_each_waiter(p__, rq__) \
list_for_each_entry_lockless(p__, \
&(rq__)->sched.waiters_list, \
@@ -2275,9 +2263,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
virtual_update_register_offsets(regs,
engine);
- if (!list_empty(&ve->context.signals))
- virtual_xfer_breadcrumbs(ve);
-
/*
* Move the bound engine to the top of the list
* for future execution. We then kick this
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 58e37f96ae21..ada630677cf3 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -592,9 +592,8 @@ bool __i915_request_submit(struct i915_request *request)
*/
__notify_execute_cb_irq(request);
- if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags) &&
- !i915_request_enable_breadcrumb(request))
- intel_engine_signal_breadcrumbs(engine);
+ if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags))
+ i915_request_enable_breadcrumb(request);
return result;
}
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Intel-gfx] [PATCH 04/10] drm/i915/gt: Only transfer the virtual context to the new engine if active
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 02/10] drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 03/10] drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs Chris Wilson
@ 2020-07-20 9:23 ` Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 05/10] drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs Chris Wilson
` (9 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Chris Wilson @ 2020-07-20 9:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Nayana, Venkata Ramana, Chris Wilson
One more complication of preempt-to-busy with respect to the virtual
engine is that we may have retired the last request along the virtual
engine at the same time as preparing to submit the completed request to
a new engine. That submit will be shortcircuited, but not before we have
updated the context with the new register offsets and marked the virtual
engine as bound to the new engine (by calling swap on ve->siblings[]).
As we may have just retired the completed request, we may also be in the
middle of calling virtual_context_exit() to turn off the power management
associated with the virtual engine, and that in turn walks the
ve->siblings[]. If we happen to call swap() on the array as we walk, we
will call intel_engine_pm_put() twice on the same engine.
In this patch, we prevent this by only updating the bound engine after a
successful submission which weeds out the already completed requests.
Alternatively, we could walk a non-volatile array for the pm, such as
using the engine->mask. The small advantage to performing the update
after the submit is that we then only have to do a swap for active
requests.
Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
References: 6d06779e8672 ("drm/i915: Load balancing across a virtual engine"
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: "Nayana, Venkata Ramana" <venkata.ramana.nayana@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/gt/intel_lrc.c | 65 ++++++++++++++++++-----------
1 file changed, 40 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 88a5c155154d..5e8278e8ac79 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1805,6 +1805,33 @@ static bool virtual_matches(const struct virtual_engine *ve,
return true;
}
+static void virtual_xfer_context(struct virtual_engine *ve,
+ struct intel_engine_cs *engine)
+{
+ unsigned int n;
+
+ if (likely(engine == ve->siblings[0]))
+ return;
+
+ GEM_BUG_ON(READ_ONCE(ve->context.inflight));
+ if (!intel_engine_has_relative_mmio(engine))
+ virtual_update_register_offsets(ve->context.lrc_reg_state,
+ engine);
+
+ /*
+ * Move the bound engine to the top of the list for
+ * future execution. We then kick this tasklet first
+ * before checking others, so that we preferentially
+ * reuse this set of bound registers.
+ */
+ for (n = 1; n < ve->num_siblings; n++) {
+ if (ve->siblings[n] == engine) {
+ swap(ve->siblings[n], ve->siblings[0]);
+ break;
+ }
+ }
+}
+
#define for_each_waiter(p__, rq__) \
list_for_each_entry_lockless(p__, \
&(rq__)->sched.waiters_list, \
@@ -2253,35 +2280,23 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
GEM_BUG_ON(!(rq->execution_mask & engine->mask));
WRITE_ONCE(rq->engine, engine);
- if (engine != ve->siblings[0]) {
- u32 *regs = ve->context.lrc_reg_state;
- unsigned int n;
-
- GEM_BUG_ON(READ_ONCE(ve->context.inflight));
-
- if (!intel_engine_has_relative_mmio(engine))
- virtual_update_register_offsets(regs,
- engine);
-
+ if (__i915_request_submit(rq)) {
/*
- * Move the bound engine to the top of the list
- * for future execution. We then kick this
- * tasklet first before checking others, so that
- * we preferentially reuse this set of bound
- * registers.
+ * Only after we confirm that we will submit
+ * this request (i.e. it has not already
+ * completed), do we want to update the context.
+ *
+ * This serves two purposes. It avoids
+ * unnecessary work if we are resubmitting an
+ * already completed request after timeslicing.
+ * But more importantly, it prevents us altering
+ * ve->siblings[] on an idle context, where
+ * we may be using ve->siblings[] in
+ * virtual_context_enter / virtual_context_exit.
*/
- for (n = 1; n < ve->num_siblings; n++) {
- if (ve->siblings[n] == engine) {
- swap(ve->siblings[n],
- ve->siblings[0]);
- break;
- }
- }
-
+ virtual_xfer_context(ve, engine);
GEM_BUG_ON(ve->siblings[0] != engine);
- }
- if (__i915_request_submit(rq)) {
submit = true;
last = rq;
}
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Intel-gfx] [PATCH 05/10] drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
` (2 preceding siblings ...)
2020-07-20 9:23 ` [Intel-gfx] [PATCH 04/10] drm/i915/gt: Only transfer the virtual context to the new engine if active Chris Wilson
@ 2020-07-20 9:23 ` Chris Wilson
2020-07-20 11:23 ` Tvrtko Ursulin
2020-07-21 7:59 ` kernel test robot
2020-07-20 9:23 ` [Intel-gfx] [PATCH 06/10] drm/i915/gt: Move intel_breadcrumbs_arm_irq earlier Chris Wilson
` (8 subsequent siblings)
12 siblings, 2 replies; 26+ messages in thread
From: Chris Wilson @ 2020-07-20 9:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson
On the virtual engines, we only use the intel_breadcrumbs for tracking
signaling of stale breadcrumbs from the irq_workers. They do not have
any associated interrupt handling, active requests are passed to a
physical engine and associated breadcrumb interrupt handler. This causes
issues for us as we need to ensure that we do not actually try and
enable interrupts and the powermanagement required for them on the
virtual engine, as they will never be disabled. Instead, let's
specify the physical engine used for interrupt handler on a particular
breadcrumb.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 72 ++++++++++---------
drivers/gpu/drm/i915/gt/intel_breadcrumbs.h | 36 ++++++++++
.../gpu/drm/i915/gt/intel_breadcrumbs_types.h | 47 ++++++++++++
drivers/gpu/drm/i915/gt/intel_engine.h | 17 -----
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 14 +++-
drivers/gpu/drm/i915/gt/intel_engine_pm.c | 3 +-
drivers/gpu/drm/i915/gt/intel_engine_types.h | 31 +-------
drivers/gpu/drm/i915/gt/intel_gt_irq.c | 1 +
drivers/gpu/drm/i915/gt/intel_lrc.c | 11 ++-
drivers/gpu/drm/i915/gt/intel_reset.c | 1 +
.../gpu/drm/i915/gt/intel_ring_submission.c | 3 +-
drivers/gpu/drm/i915/gt/intel_rps.c | 1 +
drivers/gpu/drm/i915/gt/mock_engine.c | 10 ++-
drivers/gpu/drm/i915/i915_irq.c | 1 +
drivers/gpu/drm/i915/i915_request.c | 1 +
drivers/gpu/drm/i915/i915_request.h | 4 --
16 files changed, 162 insertions(+), 91 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gt/intel_breadcrumbs.h
create mode 100644 drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index fbdc465a5870..621cf9d1d7ad 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -28,6 +28,7 @@
#include "i915_drv.h"
#include "i915_trace.h"
+#include "intel_breadcrumbs.h"
#include "intel_gt_pm.h"
#include "intel_gt_requests.h"
@@ -55,22 +56,21 @@ static void irq_disable(struct intel_engine_cs *engine)
static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
{
- struct intel_engine_cs *engine =
- container_of(b, struct intel_engine_cs, breadcrumbs);
-
lockdep_assert_held(&b->irq_lock);
+ if (!b->irq_engine)
+ return;
+
GEM_BUG_ON(!b->irq_enabled);
if (!--b->irq_enabled)
- irq_disable(engine);
+ irq_disable(b->irq_engine);
WRITE_ONCE(b->irq_armed, false);
- intel_gt_pm_put_async(engine->gt);
+ intel_gt_pm_put_async(b->irq_engine->gt);
}
-void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
+void intel_breadcrumbs_park(struct intel_breadcrumbs *b)
{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
unsigned long flags;
if (!READ_ONCE(b->irq_armed))
@@ -133,13 +133,8 @@ __dma_fence_signal__notify(struct dma_fence *fence,
static void add_retire(struct intel_breadcrumbs *b, struct intel_timeline *tl)
{
- struct intel_engine_cs *engine =
- container_of(b, struct intel_engine_cs, breadcrumbs);
-
- if (unlikely(intel_engine_is_virtual(engine)))
- engine = intel_virtual_engine_get_sibling(engine, 0);
-
- intel_engine_add_retire(engine, tl);
+ if (b->irq_engine)
+ intel_engine_add_retire(b->irq_engine, tl);
}
static bool __signal_request(struct i915_request *rq, struct list_head *signals)
@@ -222,14 +217,13 @@ static void signal_irq_work(struct irq_work *work)
static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
{
- struct intel_engine_cs *engine =
- container_of(b, struct intel_engine_cs, breadcrumbs);
-
lockdep_assert_held(&b->irq_lock);
+
if (b->irq_armed)
return;
- if (!intel_gt_pm_get_if_awake(engine->gt))
+ GEM_BUG_ON(!b->irq_engine);
+ if (!intel_gt_pm_get_if_awake(b->irq_engine->gt))
return;
/*
@@ -249,37 +243,51 @@ static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
*/
if (!b->irq_enabled++)
- irq_enable(engine);
+ irq_enable(b->irq_engine);
}
-void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
+struct intel_breadcrumbs *
+intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
+ struct intel_breadcrumbs *b;
+
+ b = kzalloc(sizeof(*b), GFP_KERNEL);
+ if (!b)
+ return NULL;
spin_lock_init(&b->irq_lock);
INIT_LIST_HEAD(&b->signalers);
INIT_LIST_HEAD(&b->signaled_requests);
init_irq_work(&b->irq_work, signal_irq_work);
+
+ b->irq_engine = irq_engine;
+ if (!irq_engine)
+ b->irq_armed = true; /* fake HW, used for irq_work */
+
+ return b;
}
-void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
+void intel_breadcrumbs_reset(struct intel_breadcrumbs *b)
{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
unsigned long flags;
+ if (!b->irq_engine)
+ return;
+
spin_lock_irqsave(&b->irq_lock, flags);
if (b->irq_enabled)
- irq_enable(engine);
+ irq_enable(b->irq_engine);
else
- irq_disable(engine);
+ irq_disable(b->irq_engine);
spin_unlock_irqrestore(&b->irq_lock, flags);
}
-void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
+void intel_breadcrumbs_free(struct intel_breadcrumbs *b)
{
+ kfree(b);
}
static void insert_breadcrumb(struct i915_request *rq,
@@ -369,11 +377,11 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
* request submit/unsubmit path, and so we must be more careful to
* acquire the right lock.
*/
- b = &READ_ONCE(rq->engine)->breadcrumbs;
+ b = READ_ONCE(rq->engine)->breadcrumbs;
spin_lock(&b->irq_lock);
- while (unlikely(b != &READ_ONCE(rq->engine)->breadcrumbs)) {
+ while (unlikely(b != READ_ONCE(rq->engine)->breadcrumbs)) {
spin_unlock(&b->irq_lock);
- b = &READ_ONCE(rq->engine)->breadcrumbs;
+ b = READ_ONCE(rq->engine)->breadcrumbs;
spin_lock(&b->irq_lock);
}
@@ -394,7 +402,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
void i915_request_cancel_breadcrumb(struct i915_request *rq)
{
- struct intel_breadcrumbs *b = &rq->engine->breadcrumbs;
+ struct intel_breadcrumbs *b = rq->engine->breadcrumbs;
/*
* We must wait for b->irq_lock so that we know the interrupt handler
@@ -418,11 +426,11 @@ void i915_request_cancel_breadcrumb(struct i915_request *rq)
void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
struct drm_printer *p)
{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
+ struct intel_breadcrumbs *b = engine->breadcrumbs;
struct intel_context *ce;
struct i915_request *rq;
- if (list_empty(&b->signalers))
+ if (!b || list_empty(&b->signalers))
return;
drm_printf(p, "Signals:\n");
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.h
new file mode 100644
index 000000000000..0d25f793159e
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef __INTEL_BREADCRUMBS___
+#define __INTEL_BREADCRUMBS__
+
+#include <linux/irq_work.h>
+
+#include "intel_engine_types.h"
+
+struct drm_printer;
+struct i915_request;
+struct intel_breadcrumbs;
+
+struct intel_breadcrumbs *
+intel_breadcrumbs_create(struct intel_engine_cs *irq_engine);
+void intel_breadcrumbs_free(struct intel_breadcrumbs *b);
+
+void intel_breadcrumbs_reset(struct intel_breadcrumbs *b);
+void intel_breadcrumbs_park(struct intel_breadcrumbs *b);
+
+static inline void
+intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
+{
+ irq_work_queue(&engine->breadcrumbs->irq_work);
+}
+
+void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
+ struct drm_printer *p);
+
+bool i915_request_enable_breadcrumb(struct i915_request *request);
+void i915_request_cancel_breadcrumb(struct i915_request *request);
+
+#endif /* __INTEL_BREADCRUMBS__ */
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
new file mode 100644
index 000000000000..8e53b9942695
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+
+#ifndef __INTEL_BREADCRUMBS_TYPES__
+#define __INTEL_BREADCRUMBS_TYPES__
+
+#include <linux/irq_work.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+/*
+ * Rather than have every client wait upon all user interrupts,
+ * with the herd waking after every interrupt and each doing the
+ * heavyweight seqno dance, we delegate the task (of being the
+ * bottom-half of the user interrupt) to the first client. After
+ * every interrupt, we wake up one client, who does the heavyweight
+ * coherent seqno read and either goes back to sleep (if incomplete),
+ * or wakes up all the completed clients in parallel, before then
+ * transferring the bottom-half status to the next client in the queue.
+ *
+ * Compared to walking the entire list of waiters in a single dedicated
+ * bottom-half, we reduce the latency of the first waiter by avoiding
+ * a context switch, but incur additional coherent seqno reads when
+ * following the chain of request breadcrumbs. Since it is most likely
+ * that we have a single client waiting on each seqno, then reducing
+ * the overhead of waking that client is much preferred.
+ */
+struct intel_breadcrumbs {
+ spinlock_t irq_lock; /* protects the lists used in hardirq context */
+
+ /* Not all breadcrumbs are attached to physical HW */
+ struct intel_engine_cs *irq_engine;
+
+ struct list_head signalers;
+ struct list_head signaled_requests;
+
+ struct irq_work irq_work; /* for use from inside irq_lock */
+
+ unsigned int irq_enabled;
+
+ bool irq_armed;
+};
+
+#endif /* __INTEL_BREADCRUMBS_TYPES__ */
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index faf00a353e25..08e2c000dcc3 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -223,23 +223,6 @@ void intel_engine_get_instdone(const struct intel_engine_cs *engine,
void intel_engine_init_execlists(struct intel_engine_cs *engine);
-void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine);
-void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
-
-void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
-
-static inline void
-intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
-{
- irq_work_queue(&engine->breadcrumbs.irq_work);
-}
-
-void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
-void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
-
-void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
- struct drm_printer *p);
-
static inline u32 *__gen8_emit_pipe_control(u32 *batch, u32 flags0, u32 flags1, u32 offset)
{
memset(batch, 0, 6 * sizeof(u32));
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index dd1a42c4d344..c20a91c1318f 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -28,6 +28,7 @@
#include "i915_drv.h"
+#include "intel_breadcrumbs.h"
#include "intel_context.h"
#include "intel_engine.h"
#include "intel_engine_pm.h"
@@ -700,8 +701,13 @@ static int engine_setup_common(struct intel_engine_cs *engine)
if (err)
return err;
+ engine->breadcrumbs = intel_breadcrumbs_create(engine);
+ if (!engine->breadcrumbs) {
+ err = -ENOMEM;
+ goto err_status;
+ }
+
intel_engine_init_active(engine, ENGINE_PHYSICAL);
- intel_engine_init_breadcrumbs(engine);
intel_engine_init_execlists(engine);
intel_engine_init_cmd_parser(engine);
intel_engine_init__pm(engine);
@@ -716,6 +722,10 @@ static int engine_setup_common(struct intel_engine_cs *engine)
intel_engine_init_ctx_wa(engine);
return 0;
+
+err_status:
+ cleanup_status_page(engine);
+ return err;
}
struct measure_breadcrumb {
@@ -902,9 +912,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
tasklet_kill(&engine->execlists.tasklet); /* flush the callback */
cleanup_status_page(engine);
+ intel_breadcrumbs_free(engine->breadcrumbs);
intel_engine_fini_retire(engine);
- intel_engine_fini_breadcrumbs(engine);
intel_engine_cleanup_cmd_parser(engine);
if (engine->default_state)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
index 8ec3eecf3e39..f7b2e07e2229 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
@@ -6,6 +6,7 @@
#include "i915_drv.h"
+#include "intel_breadcrumbs.h"
#include "intel_context.h"
#include "intel_engine.h"
#include "intel_engine_heartbeat.h"
@@ -247,7 +248,7 @@ static int __engine_park(struct intel_wakeref *wf)
call_idle_barriers(engine); /* cleanup after wedging */
intel_engine_park_heartbeat(engine);
- intel_engine_disarm_breadcrumbs(engine);
+ intel_breadcrumbs_park(engine->breadcrumbs);
/* Must be reset upon idling, or we may miss the busy wakeup. */
GEM_BUG_ON(engine->execlists.queue_priority_hint != INT_MIN);
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 8de92fd7d392..c400aaa2287b 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -22,6 +22,7 @@
#include "i915_pmu.h"
#include "i915_priolist_types.h"
#include "i915_selftest.h"
+#include "intel_breadcrumbs_types.h"
#include "intel_sseu.h"
#include "intel_timeline_types.h"
#include "intel_uncore.h"
@@ -373,34 +374,8 @@ struct intel_engine_cs {
*/
struct ewma__engine_latency latency;
- /* Rather than have every client wait upon all user interrupts,
- * with the herd waking after every interrupt and each doing the
- * heavyweight seqno dance, we delegate the task (of being the
- * bottom-half of the user interrupt) to the first client. After
- * every interrupt, we wake up one client, who does the heavyweight
- * coherent seqno read and either goes back to sleep (if incomplete),
- * or wakes up all the completed clients in parallel, before then
- * transferring the bottom-half status to the next client in the queue.
- *
- * Compared to walking the entire list of waiters in a single dedicated
- * bottom-half, we reduce the latency of the first waiter by avoiding
- * a context switch, but incur additional coherent seqno reads when
- * following the chain of request breadcrumbs. Since it is most likely
- * that we have a single client waiting on each seqno, then reducing
- * the overhead of waking that client is much preferred.
- */
- struct intel_breadcrumbs {
- spinlock_t irq_lock;
- struct list_head signalers;
-
- struct list_head signaled_requests;
-
- struct irq_work irq_work; /* for use from inside irq_lock */
-
- unsigned int irq_enabled;
-
- bool irq_armed;
- } breadcrumbs;
+ /* Keep track of all the seqno used, a trail of breadcrumbs */
+ struct intel_breadcrumbs *breadcrumbs;
struct intel_engine_pmu {
/**
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
index b05da68e52f4..257063a57101 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
@@ -8,6 +8,7 @@
#include "i915_drv.h"
#include "i915_irq.h"
+#include "intel_breadcrumbs.h"
#include "intel_gt.h"
#include "intel_gt_irq.h"
#include "intel_uncore.h"
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 5e8278e8ac79..9112bb07a068 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -137,6 +137,7 @@
#include "i915_perf.h"
#include "i915_trace.h"
#include "i915_vgpu.h"
+#include "intel_breadcrumbs.h"
#include "intel_context.h"
#include "intel_engine_pm.h"
#include "intel_gt.h"
@@ -4119,7 +4120,7 @@ static int execlists_resume(struct intel_engine_cs *engine)
{
intel_mocs_init_engine(engine);
- intel_engine_reset_breadcrumbs(engine);
+ intel_breadcrumbs_reset(engine->breadcrumbs);
if (GEM_SHOW_DEBUG() && unexpected_starting_state(engine)) {
struct drm_printer p = drm_debug_printer(__func__);
@@ -5704,9 +5705,7 @@ intel_execlists_create_virtual(struct intel_engine_cs **siblings,
snprintf(ve->base.name, sizeof(ve->base.name), "virtual");
intel_engine_init_active(&ve->base, ENGINE_VIRTUAL);
- intel_engine_init_breadcrumbs(&ve->base);
intel_engine_init_execlists(&ve->base);
- ve->base.breadcrumbs.irq_armed = true; /* fake HW, used for irq_work */
ve->base.cops = &virtual_context_ops;
ve->base.request_alloc = execlists_request_alloc;
@@ -5723,6 +5722,12 @@ intel_execlists_create_virtual(struct intel_engine_cs **siblings,
intel_context_init(&ve->context, &ve->base);
+ ve->base.breadcrumbs = intel_breadcrumbs_create(NULL);
+ if (!ve->base.breadcrumbs) {
+ err = -ENOMEM;
+ goto err_put;
+ }
+
for (n = 0; n < count; n++) {
struct intel_engine_cs *sibling = siblings[n];
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
index 46a5ceffc22f..ac36b67fb46b 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -15,6 +15,7 @@
#include "i915_drv.h"
#include "i915_gpu_error.h"
#include "i915_irq.h"
+#include "intel_breadcrumbs.h"
#include "intel_engine_pm.h"
#include "intel_gt.h"
#include "intel_gt_pm.h"
diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
index 94915f668715..186aa2d3a83e 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
@@ -32,6 +32,7 @@
#include "gen6_ppgtt.h"
#include "gen7_renderclear.h"
#include "i915_drv.h"
+#include "intel_breadcrumbs.h"
#include "intel_context.h"
#include "intel_gt.h"
#include "intel_reset.h"
@@ -255,7 +256,7 @@ static int xcs_resume(struct intel_engine_cs *engine)
else
ring_setup_status_page(engine);
- intel_engine_reset_breadcrumbs(engine);
+ intel_breadcrumbs_reset(engine->breadcrumbs);
/* Enforce ordering by reading HEAD register back */
ENGINE_POSTING_READ(engine, RING_HEAD);
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index 97ba14ad52e4..e6a00eea0631 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -7,6 +7,7 @@
#include <drm/i915_drm.h>
#include "i915_drv.h"
+#include "intel_breadcrumbs.h"
#include "intel_gt.h"
#include "intel_gt_clock_utils.h"
#include "intel_gt_irq.h"
diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
index 06303ba98c19..d1971ffdca42 100644
--- a/drivers/gpu/drm/i915/gt/mock_engine.c
+++ b/drivers/gpu/drm/i915/gt/mock_engine.c
@@ -261,11 +261,12 @@ static void mock_engine_release(struct intel_engine_cs *engine)
GEM_BUG_ON(timer_pending(&mock->hw_delay));
+ intel_breadcrumbs_free(engine->breadcrumbs);
+
intel_context_unpin(engine->kernel_context);
intel_context_put(engine->kernel_context);
intel_engine_fini_retire(engine);
- intel_engine_fini_breadcrumbs(engine);
}
struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
@@ -323,11 +324,14 @@ int mock_engine_init(struct intel_engine_cs *engine)
struct intel_context *ce;
intel_engine_init_active(engine, ENGINE_MOCK);
- intel_engine_init_breadcrumbs(engine);
intel_engine_init_execlists(engine);
intel_engine_init__pm(engine);
intel_engine_init_retire(engine);
+ engine->breadcrumbs = intel_breadcrumbs_create(NULL);
+ if (!engine->breadcrumbs)
+ return -ENOMEM;
+
ce = create_kernel_context(engine);
if (IS_ERR(ce))
goto err_breadcrumbs;
@@ -339,7 +343,7 @@ int mock_engine_init(struct intel_engine_cs *engine)
return 0;
err_breadcrumbs:
- intel_engine_fini_breadcrumbs(engine);
+ intel_breadcrumbs_free(engine->breadcrumbs);
return -ENOMEM;
}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1fa67700d8f4..f113fe44572b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -41,6 +41,7 @@
#include "display/intel_lpe_audio.h"
#include "display/intel_psr.h"
+#include "gt/intel_breadcrumbs.h"
#include "gt/intel_gt.h"
#include "gt/intel_gt_irq.h"
#include "gt/intel_gt_pm_irq.h"
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index ada630677cf3..394587e70a2d 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -31,6 +31,7 @@
#include <linux/sched/signal.h>
#include "gem/i915_gem_context.h"
+#include "gt/intel_breadcrumbs.h"
#include "gt/intel_context.h"
#include "gt/intel_ring.h"
#include "gt/intel_rps.h"
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index fc18378c685d..16b721080195 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -361,10 +361,6 @@ void i915_request_submit(struct i915_request *request);
void __i915_request_unsubmit(struct i915_request *request);
void i915_request_unsubmit(struct i915_request *request);
-/* Note: part of the intel_breadcrumbs family */
-bool i915_request_enable_breadcrumb(struct i915_request *request);
-void i915_request_cancel_breadcrumb(struct i915_request *request);
-
long i915_request_wait(struct i915_request *rq,
unsigned int flags,
long timeout)
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Intel-gfx] [PATCH 06/10] drm/i915/gt: Move intel_breadcrumbs_arm_irq earlier
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
` (3 preceding siblings ...)
2020-07-20 9:23 ` [Intel-gfx] [PATCH 05/10] drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs Chris Wilson
@ 2020-07-20 9:23 ` Chris Wilson
2020-07-22 12:55 ` Tvrtko Ursulin
2020-07-20 9:23 ` [Intel-gfx] [PATCH 07/10] drm/i915/gt: Hold context/request reference while breadcrumbs are active Chris Wilson
` (7 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Chris Wilson @ 2020-07-20 9:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson
Move the __intel_breadcrumbs_arm_irq earlier, next to the disarm_irq, so
that we can make use of it in the following patch.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 88 ++++++++++-----------
1 file changed, 44 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index 621cf9d1d7ad..d6008034869f 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -54,6 +54,37 @@ static void irq_disable(struct intel_engine_cs *engine)
spin_unlock(&engine->gt->irq_lock);
}
+static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
+{
+ lockdep_assert_held(&b->irq_lock);
+
+ if (b->irq_armed)
+ return;
+
+ GEM_BUG_ON(!b->irq_engine);
+ if (!intel_gt_pm_get_if_awake(b->irq_engine->gt))
+ return;
+
+ /*
+ * The breadcrumb irq will be disarmed on the interrupt after the
+ * waiters are signaled. This gives us a single interrupt window in
+ * which we can add a new waiter and avoid the cost of re-enabling
+ * the irq.
+ */
+ WRITE_ONCE(b->irq_armed, true);
+
+ /*
+ * Since we are waiting on a request, the GPU should be busy
+ * and should have its own rpm reference. This is tracked
+ * by i915->gt.awake, we can forgo holding our own wakref
+ * for the interrupt as before i915->gt.awake is released (when
+ * the driver is idle) we disarm the breadcrumbs.
+ */
+
+ if (!b->irq_enabled++)
+ irq_enable(b->irq_engine);
+}
+
static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
{
lockdep_assert_held(&b->irq_lock);
@@ -69,19 +100,6 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
intel_gt_pm_put_async(b->irq_engine->gt);
}
-void intel_breadcrumbs_park(struct intel_breadcrumbs *b)
-{
- unsigned long flags;
-
- if (!READ_ONCE(b->irq_armed))
- return;
-
- spin_lock_irqsave(&b->irq_lock, flags);
- if (b->irq_armed)
- __intel_breadcrumbs_disarm_irq(b);
- spin_unlock_irqrestore(&b->irq_lock, flags);
-}
-
static inline bool __request_completed(const struct i915_request *rq)
{
return i915_seqno_passed(__hwsp_seqno(rq), rq->fence.seqno);
@@ -215,37 +233,6 @@ static void signal_irq_work(struct irq_work *work)
}
}
-static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
-{
- lockdep_assert_held(&b->irq_lock);
-
- if (b->irq_armed)
- return;
-
- GEM_BUG_ON(!b->irq_engine);
- if (!intel_gt_pm_get_if_awake(b->irq_engine->gt))
- return;
-
- /*
- * The breadcrumb irq will be disarmed on the interrupt after the
- * waiters are signaled. This gives us a single interrupt window in
- * which we can add a new waiter and avoid the cost of re-enabling
- * the irq.
- */
- WRITE_ONCE(b->irq_armed, true);
-
- /*
- * Since we are waiting on a request, the GPU should be busy
- * and should have its own rpm reference. This is tracked
- * by i915->gt.awake, we can forgo holding our own wakref
- * for the interrupt as before i915->gt.awake is released (when
- * the driver is idle) we disarm the breadcrumbs.
- */
-
- if (!b->irq_enabled++)
- irq_enable(b->irq_engine);
-}
-
struct intel_breadcrumbs *
intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
{
@@ -285,6 +272,19 @@ void intel_breadcrumbs_reset(struct intel_breadcrumbs *b)
spin_unlock_irqrestore(&b->irq_lock, flags);
}
+void intel_breadcrumbs_park(struct intel_breadcrumbs *b)
+{
+ unsigned long flags;
+
+ if (!READ_ONCE(b->irq_armed))
+ return;
+
+ spin_lock_irqsave(&b->irq_lock, flags);
+ if (b->irq_armed)
+ __intel_breadcrumbs_disarm_irq(b);
+ spin_unlock_irqrestore(&b->irq_lock, flags);
+}
+
void intel_breadcrumbs_free(struct intel_breadcrumbs *b)
{
kfree(b);
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Intel-gfx] [PATCH 07/10] drm/i915/gt: Hold context/request reference while breadcrumbs are active
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
` (4 preceding siblings ...)
2020-07-20 9:23 ` [Intel-gfx] [PATCH 06/10] drm/i915/gt: Move intel_breadcrumbs_arm_irq earlier Chris Wilson
@ 2020-07-20 9:23 ` Chris Wilson
2020-07-22 13:05 ` Tvrtko Ursulin
2020-07-20 9:23 ` [Intel-gfx] [PATCH 08/10] drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock Chris Wilson
` (6 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Chris Wilson @ 2020-07-20 9:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson
Currently we hold no actual reference to the request nor context while
they are attached to a breadcrumb. To avoid freeing the request/context
too early, we serialise with cancel-breadcrumbs by taking the irq
spinlock in i915_request_retire(). The alternative is to take a
reference for a new breadcrumb and release it upon signaling; removing
the more frequently hit contention point in i915_request_retire().
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 58 ++++++++++++++++-----
drivers/gpu/drm/i915/i915_request.c | 9 ++--
2 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index d6008034869f..59e8cd505569 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -29,6 +29,7 @@
#include "i915_drv.h"
#include "i915_trace.h"
#include "intel_breadcrumbs.h"
+#include "intel_context.h"
#include "intel_gt_pm.h"
#include "intel_gt_requests.h"
@@ -100,6 +101,22 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
intel_gt_pm_put_async(b->irq_engine->gt);
}
+static void add_signaling_context(struct intel_breadcrumbs *b,
+ struct intel_context *ce)
+{
+ intel_context_get(ce);
+ list_add_tail(&ce->signal_link, &b->signalers);
+ if (list_is_first(&ce->signal_link, &b->signalers))
+ __intel_breadcrumbs_arm_irq(b);
+}
+
+static void remove_signaling_context(struct intel_breadcrumbs *b,
+ struct intel_context *ce)
+{
+ list_del(&ce->signal_link);
+ intel_context_put(ce);
+}
+
static inline bool __request_completed(const struct i915_request *rq)
{
return i915_seqno_passed(__hwsp_seqno(rq), rq->fence.seqno);
@@ -108,6 +125,9 @@ static inline bool __request_completed(const struct i915_request *rq)
__maybe_unused static bool
check_signal_order(struct intel_context *ce, struct i915_request *rq)
{
+ if (rq->context != ce)
+ return false;
+
if (!list_is_last(&rq->signal_link, &ce->signals) &&
i915_seqno_passed(rq->fence.seqno,
list_next_entry(rq, signal_link)->fence.seqno))
@@ -162,7 +182,6 @@ static bool __signal_request(struct i915_request *rq, struct list_head *signals)
if (!__dma_fence_signal(&rq->fence))
return false;
- i915_request_get(rq);
list_add_tail(&rq->signal_link, signals);
return true;
}
@@ -198,7 +217,8 @@ static void signal_irq_work(struct irq_work *work)
* spinlock as the callback chain may end up adding
* more signalers to the same context or engine.
*/
- __signal_request(rq, &signal);
+ if (!__signal_request(rq, &signal))
+ i915_request_put(rq);
}
/*
@@ -210,7 +230,7 @@ static void signal_irq_work(struct irq_work *work)
/* Advance the list to the first incomplete request */
__list_del_many(&ce->signals, pos);
if (&ce->signals == pos) { /* now empty */
- list_del_init(&ce->signal_link);
+ remove_signaling_context(b, ce);
add_retire(b, ce->timeline);
}
}
@@ -282,6 +302,8 @@ void intel_breadcrumbs_park(struct intel_breadcrumbs *b)
spin_lock_irqsave(&b->irq_lock, flags);
if (b->irq_armed)
__intel_breadcrumbs_disarm_irq(b);
+ if (!list_empty(&b->signalers))
+ irq_work_queue(&b->irq_work);
spin_unlock_irqrestore(&b->irq_lock, flags);
}
@@ -299,6 +321,8 @@ static void insert_breadcrumb(struct i915_request *rq,
if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
return;
+ i915_request_get(rq);
+
/*
* If the request is already completed, we can transfer it
* straight onto a signaled list, and queue the irq worker for
@@ -307,11 +331,11 @@ static void insert_breadcrumb(struct i915_request *rq,
if (__request_completed(rq)) {
if (__signal_request(rq, &b->signaled_requests))
irq_work_queue(&b->irq_work);
+ else
+ i915_request_put(rq);
return;
}
- __intel_breadcrumbs_arm_irq(b);
-
/*
* We keep the seqno in retirement order, so we can break
* inside intel_engine_signal_breadcrumbs as soon as we've
@@ -326,16 +350,20 @@ static void insert_breadcrumb(struct i915_request *rq,
* start looking for our insertion point from the tail of
* the list.
*/
- list_for_each_prev(pos, &ce->signals) {
- struct i915_request *it =
- list_entry(pos, typeof(*it), signal_link);
-
- if (i915_seqno_passed(rq->fence.seqno, it->fence.seqno))
- break;
+ if (list_empty(&ce->signals)) {
+ add_signaling_context(b, ce);
+ GEM_BUG_ON(!b->irq_armed);
+ pos = &ce->signals;
+ } else {
+ list_for_each_prev(pos, &ce->signals) {
+ struct i915_request *it =
+ list_entry(pos, typeof(*it), signal_link);
+
+ if (i915_seqno_passed(rq->fence.seqno, it->fence.seqno))
+ break;
+ }
}
list_add(&rq->signal_link, pos);
- if (pos == &ce->signals) /* catch transitions from empty list */
- list_move_tail(&ce->signal_link, &b->signalers);
GEM_BUG_ON(!check_signal_order(ce, rq));
set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
@@ -416,9 +444,10 @@ void i915_request_cancel_breadcrumb(struct i915_request *rq)
list_del(&rq->signal_link);
if (list_empty(&ce->signals))
- list_del_init(&ce->signal_link);
+ remove_signaling_context(b, ce);
clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
+ i915_request_put(rq);
}
spin_unlock(&b->irq_lock);
}
@@ -433,6 +462,7 @@ void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
if (!b || list_empty(&b->signalers))
return;
+ drm_printf(p, "IRQ: %s\n", enableddisabled(b->irq_armed));
drm_printf(p, "Signals:\n");
spin_lock_irq(&b->irq_lock);
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 394587e70a2d..4ebb0f144ac4 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -300,12 +300,11 @@ bool i915_request_retire(struct i915_request *rq)
__i915_request_fill(rq, POISON_FREE);
rq->ring->head = rq->postfix;
- spin_lock_irq(&rq->lock);
- if (!i915_request_signaled(rq))
+ if (!i915_request_signaled(rq)) {
+ spin_lock_irq(&rq->lock);
dma_fence_signal_locked(&rq->fence);
- if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &rq->fence.flags))
- i915_request_cancel_breadcrumb(rq);
- spin_unlock_irq(&rq->lock);
+ spin_unlock_irq(&rq->lock);
+ }
if (i915_request_has_waitboost(rq)) {
GEM_BUG_ON(!atomic_read(&rq->engine->gt->rps.num_waiters));
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Intel-gfx] [PATCH 08/10] drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
` (5 preceding siblings ...)
2020-07-20 9:23 ` [Intel-gfx] [PATCH 07/10] drm/i915/gt: Hold context/request reference while breadcrumbs are active Chris Wilson
@ 2020-07-20 9:23 ` Chris Wilson
2020-07-22 13:26 ` Tvrtko Ursulin
2020-07-20 9:23 ` [Intel-gfx] [PATCH 09/10] drm/i915/gt: Split the breadcrumb spinlock between global and contexts Chris Wilson
` (5 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Chris Wilson @ 2020-07-20 9:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson
Make b->signaled_requests a lockless-list so that we can manipulate it
outside of the b->irq_lock.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 42 +++++++++----------
.../gpu/drm/i915/gt/intel_breadcrumbs_types.h | 2 +-
drivers/gpu/drm/i915/i915_request.h | 6 ++-
3 files changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index 59e8cd505569..2b3ad17c63b9 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -175,32 +175,23 @@ static void add_retire(struct intel_breadcrumbs *b, struct intel_timeline *tl)
intel_engine_add_retire(b->irq_engine, tl);
}
-static bool __signal_request(struct i915_request *rq, struct list_head *signals)
-{
- clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
-
- if (!__dma_fence_signal(&rq->fence))
- return false;
-
- list_add_tail(&rq->signal_link, signals);
- return true;
-}
-
static void signal_irq_work(struct irq_work *work)
{
struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
const ktime_t timestamp = ktime_get();
+ struct llist_node *signal, *sn;
struct intel_context *ce, *cn;
struct list_head *pos, *next;
- LIST_HEAD(signal);
+
+ signal = NULL;
+ if (!llist_empty(&b->signaled_requests))
+ signal = llist_del_all(&b->signaled_requests);
spin_lock(&b->irq_lock);
- if (b->irq_armed && list_empty(&b->signalers))
+ if (!signal && b->irq_armed && list_empty(&b->signalers))
__intel_breadcrumbs_disarm_irq(b);
- list_splice_init(&b->signaled_requests, &signal);
-
list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
GEM_BUG_ON(list_empty(&ce->signals));
@@ -217,8 +208,13 @@ static void signal_irq_work(struct irq_work *work)
* spinlock as the callback chain may end up adding
* more signalers to the same context or engine.
*/
- if (!__signal_request(rq, &signal))
+ clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
+ if (__dma_fence_signal(&rq->fence)) {
+ rq->signal_node.next = signal;
+ signal = &rq->signal_node;
+ } else {
i915_request_put(rq);
+ }
}
/*
@@ -238,9 +234,9 @@ static void signal_irq_work(struct irq_work *work)
spin_unlock(&b->irq_lock);
- list_for_each_safe(pos, next, &signal) {
+ llist_for_each_safe(signal, sn, signal) {
struct i915_request *rq =
- list_entry(pos, typeof(*rq), signal_link);
+ llist_entry(signal, typeof(*rq), signal_node);
struct list_head cb_list;
spin_lock(&rq->lock);
@@ -264,7 +260,7 @@ intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
spin_lock_init(&b->irq_lock);
INIT_LIST_HEAD(&b->signalers);
- INIT_LIST_HEAD(&b->signaled_requests);
+ init_llist_head(&b->signaled_requests);
init_irq_work(&b->irq_work, signal_irq_work);
@@ -329,10 +325,12 @@ static void insert_breadcrumb(struct i915_request *rq,
* its signal completion.
*/
if (__request_completed(rq)) {
- if (__signal_request(rq, &b->signaled_requests))
- irq_work_queue(&b->irq_work);
- else
+ if (__dma_fence_signal(&rq->fence)) {
+ if (llist_add(&rq->signal_node, &b->signaled_requests))
+ irq_work_queue(&b->irq_work);
+ } else {
i915_request_put(rq);
+ }
return;
}
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
index 8e53b9942695..3fa19820b37a 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
@@ -35,7 +35,7 @@ struct intel_breadcrumbs {
struct intel_engine_cs *irq_engine;
struct list_head signalers;
- struct list_head signaled_requests;
+ struct llist_head signaled_requests;
struct irq_work irq_work; /* for use from inside irq_lock */
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index 16b721080195..874af6db6103 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -176,7 +176,11 @@ struct i915_request {
struct intel_context *context;
struct intel_ring *ring;
struct intel_timeline __rcu *timeline;
- struct list_head signal_link;
+
+ union {
+ struct list_head signal_link;
+ struct llist_node signal_node;
+ };
/*
* The rcu epoch of when this request was allocated. Used to judiciously
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Intel-gfx] [PATCH 09/10] drm/i915/gt: Split the breadcrumb spinlock between global and contexts
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
` (6 preceding siblings ...)
2020-07-20 9:23 ` [Intel-gfx] [PATCH 08/10] drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock Chris Wilson
@ 2020-07-20 9:23 ` Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 10/10] drm/i915: Drop i915_request.lock serialisation around await_start Chris Wilson
` (4 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Chris Wilson @ 2020-07-20 9:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson
As we funnel more and more contexts into the breadcrumbs on an engine,
the hold time of b->irq_lock grows. As we may then contend with the
b->irq_lock during request submission, this increases the burden upon
the engine->active.lock and so directly impacts both our execution
latency and client latency. If we split the b->irq_lock by introducing a
per-context spinlock to manage the signalers within a context, we then
only need the b->irq_lock for enabling/disabling the interrupt and can
avoid taking the lock for walking the list of contexts within the signal
worker. Even with the current setup, this greatly reduces the number of
times we have to take and fight for b->irq_lock.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 128 ++++++++++--------
drivers/gpu/drm/i915/gt/intel_context.c | 1 +
drivers/gpu/drm/i915/gt/intel_context_types.h | 1 +
3 files changed, 72 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
index 2b3ad17c63b9..221e7801d760 100644
--- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
@@ -105,15 +105,15 @@ static void add_signaling_context(struct intel_breadcrumbs *b,
struct intel_context *ce)
{
intel_context_get(ce);
- list_add_tail(&ce->signal_link, &b->signalers);
- if (list_is_first(&ce->signal_link, &b->signalers))
+ list_add_rcu(&ce->signal_link, &b->signalers);
+ if (list_is_last(&ce->signal_link, &b->signalers))
__intel_breadcrumbs_arm_irq(b);
}
static void remove_signaling_context(struct intel_breadcrumbs *b,
struct intel_context *ce)
{
- list_del(&ce->signal_link);
+ list_del_rcu(&ce->signal_link);
intel_context_put(ce);
}
@@ -180,20 +180,27 @@ static void signal_irq_work(struct irq_work *work)
struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
const ktime_t timestamp = ktime_get();
struct llist_node *signal, *sn;
- struct intel_context *ce, *cn;
- struct list_head *pos, *next;
+ struct intel_context *ce;
signal = NULL;
if (!llist_empty(&b->signaled_requests))
signal = llist_del_all(&b->signaled_requests);
- spin_lock(&b->irq_lock);
+ if (!signal && READ_ONCE(b->irq_armed) && list_empty(&b->signalers)) {
+ spin_lock(&b->irq_lock);
+ if (b->irq_armed && list_empty(&b->signalers))
+ __intel_breadcrumbs_disarm_irq(b);
+ spin_unlock(&b->irq_lock);
+ }
- if (!signal && b->irq_armed && list_empty(&b->signalers))
- __intel_breadcrumbs_disarm_irq(b);
+ rcu_read_lock();
+ list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
+ struct list_head *pos, *next;
- list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
- GEM_BUG_ON(list_empty(&ce->signals));
+ spin_lock(&ce->signal_lock);
+
+ if (list_empty(&ce->signals))
+ goto unlock;
list_for_each_safe(pos, next, &ce->signals) {
struct i915_request *rq =
@@ -226,13 +233,18 @@ static void signal_irq_work(struct irq_work *work)
/* Advance the list to the first incomplete request */
__list_del_many(&ce->signals, pos);
if (&ce->signals == pos) { /* now empty */
+ spin_lock(&b->irq_lock);
remove_signaling_context(b, ce);
+ spin_unlock(&b->irq_lock);
+
add_retire(b, ce->timeline);
}
}
- }
- spin_unlock(&b->irq_lock);
+unlock:
+ spin_unlock(&ce->signal_lock);
+ }
+ rcu_read_unlock();
llist_for_each_safe(signal, sn, signal) {
struct i915_request *rq =
@@ -308,9 +320,9 @@ void intel_breadcrumbs_free(struct intel_breadcrumbs *b)
kfree(b);
}
-static void insert_breadcrumb(struct i915_request *rq,
- struct intel_breadcrumbs *b)
+static void insert_breadcrumb(struct i915_request *rq)
{
+ struct intel_breadcrumbs *b = READ_ONCE(rq->engine)->breadcrumbs;
struct intel_context *ce = rq->context;
struct list_head *pos;
@@ -349,8 +361,33 @@ static void insert_breadcrumb(struct i915_request *rq,
* the list.
*/
if (list_empty(&ce->signals)) {
+ /*
+ * rq->engine is locked by rq->engine->active.lock. That
+ * however is not known until after rq->engine has been
+ * dereferenced and the lock acquired. Hence we acquire the
+ * lock and then validate that rq->engine still matches the
+ * lock we hold for it.
+ *
+ * Here, we are using the breadcrumb lock as a proxy for the
+ * rq->engine->active.lock, and we know that since the
+ * breadcrumb will be serialised within i915_request_submit
+ * the engine cannot change while active as long as we hold
+ * the breadcrumb lock on that engine.
+ *
+ * From the dma_fence_enable_signaling() path, we are outside
+ * of the request submit/unsubmit path, and so we must be more
+ * careful to acquire the right lock.
+ */
+ spin_lock(&b->irq_lock);
+ while (unlikely(b != READ_ONCE(rq->engine)->breadcrumbs)) {
+ spin_unlock(&b->irq_lock);
+ b = READ_ONCE(rq->engine)->breadcrumbs;
+ spin_lock(&b->irq_lock);
+ }
add_signaling_context(b, ce);
GEM_BUG_ON(!b->irq_armed);
+ spin_unlock(&b->irq_lock);
+
pos = &ce->signals;
} else {
list_for_each_prev(pos, &ce->signals) {
@@ -372,7 +409,7 @@ static void insert_breadcrumb(struct i915_request *rq,
bool i915_request_enable_breadcrumb(struct i915_request *rq)
{
- struct intel_breadcrumbs *b;
+ struct intel_context *ce = rq->context;
/* Serialises with i915_request_retire() using rq->lock */
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags))
@@ -387,48 +424,17 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
if (!test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
return true;
- /*
- * rq->engine is locked by rq->engine->active.lock. That however
- * is not known until after rq->engine has been dereferenced and
- * the lock acquired. Hence we acquire the lock and then validate
- * that rq->engine still matches the lock we hold for it.
- *
- * Here, we are using the breadcrumb lock as a proxy for the
- * rq->engine->active.lock, and we know that since the breadcrumb
- * will be serialised within i915_request_submit/i915_request_unsubmit,
- * the engine cannot change while active as long as we hold the
- * breadcrumb lock on that engine.
- *
- * From the dma_fence_enable_signaling() path, we are outside of the
- * request submit/unsubmit path, and so we must be more careful to
- * acquire the right lock.
- */
- b = READ_ONCE(rq->engine)->breadcrumbs;
- spin_lock(&b->irq_lock);
- while (unlikely(b != READ_ONCE(rq->engine)->breadcrumbs)) {
- spin_unlock(&b->irq_lock);
- b = READ_ONCE(rq->engine)->breadcrumbs;
- spin_lock(&b->irq_lock);
- }
-
- /*
- * Now that we are finally serialised with request submit/unsubmit,
- * [with b->irq_lock] and with i915_request_retire() [via checking
- * SIGNALED with rq->lock] confirm the request is indeed active. If
- * it is no longer active, the breadcrumb will be attached upon
- * i915_request_submit().
- */
+ spin_lock(&ce->signal_lock);
if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
- insert_breadcrumb(rq, b);
-
- spin_unlock(&b->irq_lock);
+ insert_breadcrumb(rq);
+ spin_unlock(&ce->signal_lock);
return true;
}
void i915_request_cancel_breadcrumb(struct i915_request *rq)
{
- struct intel_breadcrumbs *b = rq->engine->breadcrumbs;
+ struct intel_context *ce = rq->context;
/*
* We must wait for b->irq_lock so that we know the interrupt handler
@@ -436,18 +442,22 @@ void i915_request_cancel_breadcrumb(struct i915_request *rq)
* the DMA_FENCE_FLAG_SIGNALED_BIT/I915_FENCE_FLAG_SIGNAL dance (if
* required).
*/
- spin_lock(&b->irq_lock);
+ spin_lock(&ce->signal_lock);
if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags)) {
- struct intel_context *ce = rq->context;
-
list_del(&rq->signal_link);
- if (list_empty(&ce->signals))
+
+ if (list_empty(&ce->signals)) {
+ struct intel_breadcrumbs *b = rq->engine->breadcrumbs;
+
+ spin_lock(&b->irq_lock);
remove_signaling_context(b, ce);
+ spin_unlock(&b->irq_lock);
+ }
clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
i915_request_put(rq);
}
- spin_unlock(&b->irq_lock);
+ spin_unlock(&ce->signal_lock);
}
void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
@@ -463,8 +473,9 @@ void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
drm_printf(p, "IRQ: %s\n", enableddisabled(b->irq_armed));
drm_printf(p, "Signals:\n");
- spin_lock_irq(&b->irq_lock);
- list_for_each_entry(ce, &b->signalers, signal_link) {
+ rcu_read_lock();
+ list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
+ spin_lock_irq(&ce->signal_lock);
list_for_each_entry(rq, &ce->signals, signal_link) {
drm_printf(p, "\t[%llx:%llx%s] @ %dms\n",
rq->fence.context, rq->fence.seqno,
@@ -473,6 +484,7 @@ void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
"",
jiffies_to_msecs(jiffies - rq->emitted_jiffies));
}
+ spin_unlock_irq(&ce->signal_lock);
}
- spin_unlock_irq(&b->irq_lock);
+ rcu_read_unlock();
}
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 52db2bde44a3..c3010c8eb966 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -297,6 +297,7 @@ intel_context_init(struct intel_context *ce,
ce->vm = i915_vm_get(engine->gt->vm);
+ spin_lock_init(&ce->signal_lock);
INIT_LIST_HEAD(&ce->signal_link);
INIT_LIST_HEAD(&ce->signals);
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 4954b0df4864..a78c1c225ce3 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -51,6 +51,7 @@ struct intel_context {
struct i915_address_space *vm;
struct i915_gem_context __rcu *gem_context;
+ spinlock_t signal_lock;
struct list_head signal_link;
struct list_head signals;
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Intel-gfx] [PATCH 10/10] drm/i915: Drop i915_request.lock serialisation around await_start
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
` (7 preceding siblings ...)
2020-07-20 9:23 ` [Intel-gfx] [PATCH 09/10] drm/i915/gt: Split the breadcrumb spinlock between global and contexts Chris Wilson
@ 2020-07-20 9:23 ` Chris Wilson
2020-07-20 10:33 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/10] drm/i915/gem: Remove disordered per-file request list for throttling Patchwork
` (3 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Chris Wilson @ 2020-07-20 9:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson
Originally, we used the signal->lock as a means of following the
previous link in its timeline and peeking at the previous fence.
However, we have replaced the explicit serialisation with a series of
very careful probes that anticipate the links being deleted and the
fences recycled before we are able to acquire a strong reference to it.
We do not need the signal->lock crutch anymore, nor want the contention.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_request.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 4ebb0f144ac4..9f369c33ca49 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -955,9 +955,16 @@ i915_request_await_start(struct i915_request *rq, struct i915_request *signal)
if (i915_request_started(signal))
return 0;
+ /*
+ * The caller holds a reference on @signal, but we do not serialise
+ * against it being retired and removed from the lists.
+ *
+ * We do not hold a reference to the request before @signal, and
+ * so must be very careful to ensure that it is not _recycled_ as
+ * we follow the link backwards.
+ */
fence = NULL;
rcu_read_lock();
- spin_lock_irq(&signal->lock);
do {
struct list_head *pos = READ_ONCE(signal->link.prev);
struct i915_request *prev;
@@ -988,7 +995,6 @@ i915_request_await_start(struct i915_request *rq, struct i915_request *signal)
fence = &prev->fence;
} while (0);
- spin_unlock_irq(&signal->lock);
rcu_read_unlock();
if (!fence)
return 0;
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH 03/10] drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs
2020-07-20 9:23 ` [Intel-gfx] [PATCH 03/10] drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs Chris Wilson
@ 2020-07-20 9:32 ` Tvrtko Ursulin
0 siblings, 0 replies; 26+ messages in thread
From: Tvrtko Ursulin @ 2020-07-20 9:32 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 20/07/2020 10:23, Chris Wilson wrote:
> After staring at the breadcrumb enabling/cancellation and coming to the
> conclusion that the cause of the mysterious stale breadcrumbs must the
> act of submitting a completed requests, we can then redirect those
> completed requests onto a dedicated signaled_list at the time of
> construction and so eliminate intel_engine_transfer_stale_breadcrumbs().
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
> ---
> drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 50 ++++++++-------------
> drivers/gpu/drm/i915/gt/intel_engine.h | 3 --
> drivers/gpu/drm/i915/gt/intel_lrc.c | 15 -------
> drivers/gpu/drm/i915/i915_request.c | 5 +--
> 4 files changed, 21 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index 3d211a0c2b5a..fbdc465a5870 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -142,16 +142,16 @@ static void add_retire(struct intel_breadcrumbs *b, struct intel_timeline *tl)
> intel_engine_add_retire(engine, tl);
> }
>
> -static void __signal_request(struct i915_request *rq, struct list_head *signals)
> +static bool __signal_request(struct i915_request *rq, struct list_head *signals)
> {
> - GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags));
> clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
>
> if (!__dma_fence_signal(&rq->fence))
> - return;
> + return false;
>
> i915_request_get(rq);
> list_add_tail(&rq->signal_link, signals);
> + return true;
> }
>
> static void signal_irq_work(struct irq_work *work)
> @@ -278,32 +278,6 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
> spin_unlock_irqrestore(&b->irq_lock, flags);
> }
>
> -void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
> - struct intel_context *ce)
> -{
> - struct intel_breadcrumbs *b = &engine->breadcrumbs;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&b->irq_lock, flags);
> - if (!list_empty(&ce->signals)) {
> - struct i915_request *rq, *next;
> -
> - /* Queue for executing the signal callbacks in the irq_work */
> - list_for_each_entry_safe(rq, next, &ce->signals, signal_link) {
> - GEM_BUG_ON(rq->engine != engine);
> - GEM_BUG_ON(!__request_completed(rq));
> -
> - __signal_request(rq, &b->signaled_requests);
> - }
> -
> - INIT_LIST_HEAD(&ce->signals);
> - list_del_init(&ce->signal_link);
> -
> - irq_work_queue(&b->irq_work);
> - }
> - spin_unlock_irqrestore(&b->irq_lock, flags);
> -}
> -
> void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
> {
> }
> @@ -317,6 +291,17 @@ static void insert_breadcrumb(struct i915_request *rq,
> if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
> return;
>
> + /*
> + * If the request is already completed, we can transfer it
> + * straight onto a signaled list, and queue the irq worker for
> + * its signal completion.
> + */
> + if (__request_completed(rq)) {
> + if (__signal_request(rq, &b->signaled_requests))
> + irq_work_queue(&b->irq_work);
> + return;
> + }
> +
> __intel_breadcrumbs_arm_irq(b);
>
> /*
> @@ -344,8 +329,11 @@ static void insert_breadcrumb(struct i915_request *rq,
> if (pos == &ce->signals) /* catch transitions from empty list */
> list_move_tail(&ce->signal_link, &b->signalers);
> GEM_BUG_ON(!check_signal_order(ce, rq));
> -
> set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> +
> + /* Check after attaching to irq, interrupt may have already fired. */
> + if (__request_completed(rq))
> + irq_work_queue(&b->irq_work);
> }
>
> bool i915_request_enable_breadcrumb(struct i915_request *rq)
> @@ -401,7 +389,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>
> spin_unlock(&b->irq_lock);
>
> - return !__request_completed(rq);
> + return true;
> }
>
> void i915_request_cancel_breadcrumb(struct i915_request *rq)
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index a9249a23903a..faf00a353e25 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -237,9 +237,6 @@ intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
> void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
> void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
>
> -void intel_engine_transfer_stale_breadcrumbs(struct intel_engine_cs *engine,
> - struct intel_context *ce);
> -
> void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
> struct drm_printer *p);
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 21c16e31c4fe..88a5c155154d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1805,18 +1805,6 @@ static bool virtual_matches(const struct virtual_engine *ve,
> return true;
> }
>
> -static void virtual_xfer_breadcrumbs(struct virtual_engine *ve)
> -{
> - /*
> - * All the outstanding signals on ve->siblings[0] must have
> - * been completed, just pending the interrupt handler. As those
> - * signals still refer to the old sibling (via rq->engine), we must
> - * transfer those to the old irq_worker to keep our locking
> - * consistent.
> - */
> - intel_engine_transfer_stale_breadcrumbs(ve->siblings[0], &ve->context);
> -}
> -
> #define for_each_waiter(p__, rq__) \
> list_for_each_entry_lockless(p__, \
> &(rq__)->sched.waiters_list, \
> @@ -2275,9 +2263,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> virtual_update_register_offsets(regs,
> engine);
>
> - if (!list_empty(&ve->context.signals))
> - virtual_xfer_breadcrumbs(ve);
> -
> /*
> * Move the bound engine to the top of the list
> * for future execution. We then kick this
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 58e37f96ae21..ada630677cf3 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -592,9 +592,8 @@ bool __i915_request_submit(struct i915_request *request)
> */
> __notify_execute_cb_irq(request);
>
> - if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags) &&
> - !i915_request_enable_breadcrumb(request))
> - intel_engine_signal_breadcrumbs(engine);
> + if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags))
> + i915_request_enable_breadcrumb(request);
>
> return result;
> }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/10] drm/i915/gem: Remove disordered per-file request list for throttling
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
` (8 preceding siblings ...)
2020-07-20 9:23 ` [Intel-gfx] [PATCH 10/10] drm/i915: Drop i915_request.lock serialisation around await_start Chris Wilson
@ 2020-07-20 10:33 ` Patchwork
2020-07-20 10:34 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
` (2 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Patchwork @ 2020-07-20 10:33 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [01/10] drm/i915/gem: Remove disordered per-file request list for throttling
URL : https://patchwork.freedesktop.org/series/79663/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
5658c6f4f5ec drm/i915/gem: Remove disordered per-file request list for throttling
9bbfe782827e drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs
b31adc8db68e drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs
c0249b2d18cb drm/i915/gt: Only transfer the virtual context to the new engine if active
-:28: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#28:
References: 6d06779e8672 ("drm/i915: Load balancing across a virtual engine"
-:28: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 6d06779e8672 ("drm/i915: Load balancing across a virtual engine")'
#28:
References: 6d06779e8672 ("drm/i915: Load balancing across a virtual engine"
total: 1 errors, 1 warnings, 0 checks, 81 lines checked
b94091b54c89 drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs
-:194: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#194:
new file mode 100644
total: 0 errors, 1 warnings, 0 checks, 498 lines checked
d3452c2885e6 drm/i915/gt: Move intel_breadcrumbs_arm_irq earlier
1ee68c9b0e38 drm/i915/gt: Hold context/request reference while breadcrumbs are active
6bbb5b5dc21a drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock
caef82f06367 drm/i915/gt: Split the breadcrumb spinlock between global and contexts
-:276: CHECK:UNCOMMENTED_DEFINITION: spinlock_t definition without comment
#276: FILE: drivers/gpu/drm/i915/gt/intel_context_types.h:54:
+ spinlock_t signal_lock;
total: 0 errors, 0 warnings, 1 checks, 236 lines checked
a9f67affbc82 drm/i915: Drop i915_request.lock serialisation around await_start
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [01/10] drm/i915/gem: Remove disordered per-file request list for throttling
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
` (9 preceding siblings ...)
2020-07-20 10:33 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/10] drm/i915/gem: Remove disordered per-file request list for throttling Patchwork
@ 2020-07-20 10:34 ` Patchwork
2020-07-20 10:55 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-07-20 13:44 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
12 siblings, 0 replies; 26+ messages in thread
From: Patchwork @ 2020-07-20 10:34 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [01/10] drm/i915/gem: Remove disordered per-file request list for throttling
URL : https://patchwork.freedesktop.org/series/79663/
State : warning
== Summary ==
$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.0
Fast mode used, each commit won't be checked separately.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [01/10] drm/i915/gem: Remove disordered per-file request list for throttling
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
` (10 preceding siblings ...)
2020-07-20 10:34 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
@ 2020-07-20 10:55 ` Patchwork
2020-07-20 13:44 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
12 siblings, 0 replies; 26+ messages in thread
From: Patchwork @ 2020-07-20 10:55 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 7096 bytes --]
== Series Details ==
Series: series starting with [01/10] drm/i915/gem: Remove disordered per-file request list for throttling
URL : https://patchwork.freedesktop.org/series/79663/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_8766 -> Patchwork_18210
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/index.html
Known issues
------------
Here are the changes found in Patchwork_18210 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_suspend@basic-s3:
- fi-tgl-u2: [PASS][1] -> [FAIL][2] ([i915#1888])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/fi-tgl-u2/igt@gem_exec_suspend@basic-s3.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/fi-tgl-u2/igt@gem_exec_suspend@basic-s3.html
* igt@gem_flink_basic@double-flink:
- fi-tgl-y: [PASS][3] -> [DMESG-WARN][4] ([i915#402]) +1 similar issue
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/fi-tgl-y/igt@gem_flink_basic@double-flink.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/fi-tgl-y/igt@gem_flink_basic@double-flink.html
* igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1:
- fi-icl-u2: [PASS][5] -> [DMESG-WARN][6] ([i915#1982])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1.html
#### Possible fixes ####
* igt@gem_exec_suspend@basic-s0:
- fi-tgl-u2: [FAIL][7] ([i915#1888]) -> [PASS][8]
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/fi-tgl-u2/igt@gem_exec_suspend@basic-s0.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/fi-tgl-u2/igt@gem_exec_suspend@basic-s0.html
* igt@i915_pm_rpm@basic-pci-d3-state:
- fi-bsw-kefka: [DMESG-WARN][9] ([i915#1982]) -> [PASS][10]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/fi-bsw-kefka/igt@i915_pm_rpm@basic-pci-d3-state.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/fi-bsw-kefka/igt@i915_pm_rpm@basic-pci-d3-state.html
* igt@i915_pm_rpm@module-reload:
- {fi-tgl-dsi}: [DMESG-WARN][11] ([i915#1982]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/fi-tgl-dsi/igt@i915_pm_rpm@module-reload.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/fi-tgl-dsi/igt@i915_pm_rpm@module-reload.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- fi-tgl-y: [DMESG-WARN][13] ([i915#1982]) -> [PASS][14] +1 similar issue
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/fi-tgl-y/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/fi-tgl-y/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
* igt@kms_flip@basic-flip-vs-wf_vblank@c-hdmi-a2:
- fi-skl-guc: [DMESG-WARN][15] -> [PASS][16]
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/fi-skl-guc/igt@kms_flip@basic-flip-vs-wf_vblank@c-hdmi-a2.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/fi-skl-guc/igt@kms_flip@basic-flip-vs-wf_vblank@c-hdmi-a2.html
* igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
- fi-tgl-u2: [DMESG-WARN][17] ([i915#402]) -> [PASS][18]
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/fi-tgl-u2/igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/fi-tgl-u2/igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence.html
* igt@vgem_basic@sysfs:
- fi-tgl-y: [DMESG-WARN][19] ([i915#402]) -> [PASS][20] +1 similar issue
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/fi-tgl-y/igt@vgem_basic@sysfs.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/fi-tgl-y/igt@vgem_basic@sysfs.html
#### Warnings ####
* igt@gem_exec_suspend@basic-s0:
- fi-kbl-x1275: [DMESG-WARN][21] ([i915#62] / [i915#92]) -> [DMESG-WARN][22] ([i915#62] / [i915#92] / [i915#95]) +2 similar issues
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/fi-kbl-x1275/igt@gem_exec_suspend@basic-s0.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/fi-kbl-x1275/igt@gem_exec_suspend@basic-s0.html
* igt@kms_flip@basic-flip-vs-modeset@a-dp1:
- fi-kbl-x1275: [DMESG-WARN][23] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][24] ([i915#62] / [i915#92]) +5 similar issues
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-modeset@a-dp1.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-modeset@a-dp1.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[i915#1888]: https://gitlab.freedesktop.org/drm/intel/issues/1888
[i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
[i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
[i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
[i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
[i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
Participating hosts (47 -> 40)
------------------------------
Missing (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus
Build changes
-------------
* Linux: CI_DRM_8766 -> Patchwork_18210
CI-20190529: 20190529
CI_DRM_8766: 947ce595ea05b4baaea060a7e018cc3f49eaf413 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5740: 6663e3ab5f77add7077711c2b649caf2bd7903c4 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_18210: a9f67affbc82ba919e3980d10bd2a447f4db4c07 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
a9f67affbc82 drm/i915: Drop i915_request.lock serialisation around await_start
caef82f06367 drm/i915/gt: Split the breadcrumb spinlock between global and contexts
6bbb5b5dc21a drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock
1ee68c9b0e38 drm/i915/gt: Hold context/request reference while breadcrumbs are active
d3452c2885e6 drm/i915/gt: Move intel_breadcrumbs_arm_irq earlier
b94091b54c89 drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs
c0249b2d18cb drm/i915/gt: Only transfer the virtual context to the new engine if active
b31adc8db68e drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs
9bbfe782827e drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs
5658c6f4f5ec drm/i915/gem: Remove disordered per-file request list for throttling
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/index.html
[-- Attachment #1.2: Type: text/html, Size: 8916 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH 05/10] drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs
2020-07-20 9:23 ` [Intel-gfx] [PATCH 05/10] drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs Chris Wilson
@ 2020-07-20 11:23 ` Tvrtko Ursulin
2020-07-20 15:02 ` Chris Wilson
2020-07-21 7:59 ` kernel test robot
1 sibling, 1 reply; 26+ messages in thread
From: Tvrtko Ursulin @ 2020-07-20 11:23 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 20/07/2020 10:23, Chris Wilson wrote:
> On the virtual engines, we only use the intel_breadcrumbs for tracking
> signaling of stale breadcrumbs from the irq_workers. They do not have
> any associated interrupt handling, active requests are passed to a
> physical engine and associated breadcrumb interrupt handler. This causes
> issues for us as we need to ensure that we do not actually try and
> enable interrupts and the powermanagement required for them on the
> virtual engine, as they will never be disabled. Instead, let's
> specify the physical engine used for interrupt handler on a particular
> breadcrumb.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 72 ++++++++++---------
> drivers/gpu/drm/i915/gt/intel_breadcrumbs.h | 36 ++++++++++
> .../gpu/drm/i915/gt/intel_breadcrumbs_types.h | 47 ++++++++++++
> drivers/gpu/drm/i915/gt/intel_engine.h | 17 -----
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 14 +++-
> drivers/gpu/drm/i915/gt/intel_engine_pm.c | 3 +-
> drivers/gpu/drm/i915/gt/intel_engine_types.h | 31 +-------
> drivers/gpu/drm/i915/gt/intel_gt_irq.c | 1 +
> drivers/gpu/drm/i915/gt/intel_lrc.c | 11 ++-
> drivers/gpu/drm/i915/gt/intel_reset.c | 1 +
> .../gpu/drm/i915/gt/intel_ring_submission.c | 3 +-
> drivers/gpu/drm/i915/gt/intel_rps.c | 1 +
> drivers/gpu/drm/i915/gt/mock_engine.c | 10 ++-
> drivers/gpu/drm/i915/i915_irq.c | 1 +
> drivers/gpu/drm/i915/i915_request.c | 1 +
> drivers/gpu/drm/i915/i915_request.h | 4 --
> 16 files changed, 162 insertions(+), 91 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/gt/intel_breadcrumbs.h
> create mode 100644 drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index fbdc465a5870..621cf9d1d7ad 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -28,6 +28,7 @@
>
> #include "i915_drv.h"
> #include "i915_trace.h"
> +#include "intel_breadcrumbs.h"
> #include "intel_gt_pm.h"
> #include "intel_gt_requests.h"
>
> @@ -55,22 +56,21 @@ static void irq_disable(struct intel_engine_cs *engine)
>
> static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
> {
> - struct intel_engine_cs *engine =
> - container_of(b, struct intel_engine_cs, breadcrumbs);
> -
> lockdep_assert_held(&b->irq_lock);
>
> + if (!b->irq_engine)
> + return;
> +
> GEM_BUG_ON(!b->irq_enabled);
> if (!--b->irq_enabled)
> - irq_disable(engine);
> + irq_disable(b->irq_engine);
>
> WRITE_ONCE(b->irq_armed, false);
> - intel_gt_pm_put_async(engine->gt);
> + intel_gt_pm_put_async(b->irq_engine->gt);
> }
>
> -void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
> +void intel_breadcrumbs_park(struct intel_breadcrumbs *b)
> {
> - struct intel_breadcrumbs *b = &engine->breadcrumbs;
> unsigned long flags;
>
> if (!READ_ONCE(b->irq_armed))
> @@ -133,13 +133,8 @@ __dma_fence_signal__notify(struct dma_fence *fence,
>
> static void add_retire(struct intel_breadcrumbs *b, struct intel_timeline *tl)
> {
> - struct intel_engine_cs *engine =
> - container_of(b, struct intel_engine_cs, breadcrumbs);
> -
> - if (unlikely(intel_engine_is_virtual(engine)))
> - engine = intel_virtual_engine_get_sibling(engine, 0);
> -
> - intel_engine_add_retire(engine, tl);
> + if (b->irq_engine)
> + intel_engine_add_retire(b->irq_engine, tl);
> }
>
> static bool __signal_request(struct i915_request *rq, struct list_head *signals)
> @@ -222,14 +217,13 @@ static void signal_irq_work(struct irq_work *work)
>
> static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
> {
> - struct intel_engine_cs *engine =
> - container_of(b, struct intel_engine_cs, breadcrumbs);
> -
> lockdep_assert_held(&b->irq_lock);
> +
> if (b->irq_armed)
> return;
>
> - if (!intel_gt_pm_get_if_awake(engine->gt))
> + GEM_BUG_ON(!b->irq_engine);
> + if (!intel_gt_pm_get_if_awake(b->irq_engine->gt))
> return;
>
> /*
> @@ -249,37 +243,51 @@ static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
> */
>
> if (!b->irq_enabled++)
> - irq_enable(engine);
> + irq_enable(b->irq_engine);
> }
>
> -void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
> +struct intel_breadcrumbs *
> +intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
> {
> - struct intel_breadcrumbs *b = &engine->breadcrumbs;
> + struct intel_breadcrumbs *b;
> +
> + b = kzalloc(sizeof(*b), GFP_KERNEL);
> + if (!b)
> + return NULL;
>
> spin_lock_init(&b->irq_lock);
> INIT_LIST_HEAD(&b->signalers);
> INIT_LIST_HEAD(&b->signaled_requests);
>
> init_irq_work(&b->irq_work, signal_irq_work);
> +
> + b->irq_engine = irq_engine;
> + if (!irq_engine)
> + b->irq_armed = true; /* fake HW, used for irq_work */
Disarm is checking for !b->irq_engine and arm asserts there must be when
arming. If instead arm would abort on !b->irq_engine would it all work
just as well without the need for this hack?
Regards,
Tvrtko
> +
> + return b;
> }
>
> -void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
> +void intel_breadcrumbs_reset(struct intel_breadcrumbs *b)
> {
> - struct intel_breadcrumbs *b = &engine->breadcrumbs;
> unsigned long flags;
>
> + if (!b->irq_engine)
> + return;
> +
> spin_lock_irqsave(&b->irq_lock, flags);
>
> if (b->irq_enabled)
> - irq_enable(engine);
> + irq_enable(b->irq_engine);
> else
> - irq_disable(engine);
> + irq_disable(b->irq_engine);
>
> spin_unlock_irqrestore(&b->irq_lock, flags);
> }
>
> -void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
> +void intel_breadcrumbs_free(struct intel_breadcrumbs *b)
> {
> + kfree(b);
> }
>
> static void insert_breadcrumb(struct i915_request *rq,
> @@ -369,11 +377,11 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
> * request submit/unsubmit path, and so we must be more careful to
> * acquire the right lock.
> */
> - b = &READ_ONCE(rq->engine)->breadcrumbs;
> + b = READ_ONCE(rq->engine)->breadcrumbs;
> spin_lock(&b->irq_lock);
> - while (unlikely(b != &READ_ONCE(rq->engine)->breadcrumbs)) {
> + while (unlikely(b != READ_ONCE(rq->engine)->breadcrumbs)) {
> spin_unlock(&b->irq_lock);
> - b = &READ_ONCE(rq->engine)->breadcrumbs;
> + b = READ_ONCE(rq->engine)->breadcrumbs;
> spin_lock(&b->irq_lock);
> }
>
> @@ -394,7 +402,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>
> void i915_request_cancel_breadcrumb(struct i915_request *rq)
> {
> - struct intel_breadcrumbs *b = &rq->engine->breadcrumbs;
> + struct intel_breadcrumbs *b = rq->engine->breadcrumbs;
>
> /*
> * We must wait for b->irq_lock so that we know the interrupt handler
> @@ -418,11 +426,11 @@ void i915_request_cancel_breadcrumb(struct i915_request *rq)
> void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
> struct drm_printer *p)
> {
> - struct intel_breadcrumbs *b = &engine->breadcrumbs;
> + struct intel_breadcrumbs *b = engine->breadcrumbs;
> struct intel_context *ce;
> struct i915_request *rq;
>
> - if (list_empty(&b->signalers))
> + if (!b || list_empty(&b->signalers))
> return;
>
> drm_printf(p, "Signals:\n");
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.h
> new file mode 100644
> index 000000000000..0d25f793159e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef __INTEL_BREADCRUMBS___
> +#define __INTEL_BREADCRUMBS__
> +
> +#include <linux/irq_work.h>
> +
> +#include "intel_engine_types.h"
> +
> +struct drm_printer;
> +struct i915_request;
> +struct intel_breadcrumbs;
> +
> +struct intel_breadcrumbs *
> +intel_breadcrumbs_create(struct intel_engine_cs *irq_engine);
> +void intel_breadcrumbs_free(struct intel_breadcrumbs *b);
> +
> +void intel_breadcrumbs_reset(struct intel_breadcrumbs *b);
> +void intel_breadcrumbs_park(struct intel_breadcrumbs *b);
> +
> +static inline void
> +intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
> +{
> + irq_work_queue(&engine->breadcrumbs->irq_work);
> +}
> +
> +void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
> + struct drm_printer *p);
> +
> +bool i915_request_enable_breadcrumb(struct i915_request *request);
> +void i915_request_cancel_breadcrumb(struct i915_request *request);
> +
> +#endif /* __INTEL_BREADCRUMBS__ */
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> new file mode 100644
> index 000000000000..8e53b9942695
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef __INTEL_BREADCRUMBS_TYPES__
> +#define __INTEL_BREADCRUMBS_TYPES__
> +
> +#include <linux/irq_work.h>
> +#include <linux/list.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +/*
> + * Rather than have every client wait upon all user interrupts,
> + * with the herd waking after every interrupt and each doing the
> + * heavyweight seqno dance, we delegate the task (of being the
> + * bottom-half of the user interrupt) to the first client. After
> + * every interrupt, we wake up one client, who does the heavyweight
> + * coherent seqno read and either goes back to sleep (if incomplete),
> + * or wakes up all the completed clients in parallel, before then
> + * transferring the bottom-half status to the next client in the queue.
> + *
> + * Compared to walking the entire list of waiters in a single dedicated
> + * bottom-half, we reduce the latency of the first waiter by avoiding
> + * a context switch, but incur additional coherent seqno reads when
> + * following the chain of request breadcrumbs. Since it is most likely
> + * that we have a single client waiting on each seqno, then reducing
> + * the overhead of waking that client is much preferred.
> + */
> +struct intel_breadcrumbs {
> + spinlock_t irq_lock; /* protects the lists used in hardirq context */
> +
> + /* Not all breadcrumbs are attached to physical HW */
> + struct intel_engine_cs *irq_engine;
> +
> + struct list_head signalers;
> + struct list_head signaled_requests;
> +
> + struct irq_work irq_work; /* for use from inside irq_lock */
> +
> + unsigned int irq_enabled;
> +
> + bool irq_armed;
> +};
> +
> +#endif /* __INTEL_BREADCRUMBS_TYPES__ */
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index faf00a353e25..08e2c000dcc3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -223,23 +223,6 @@ void intel_engine_get_instdone(const struct intel_engine_cs *engine,
>
> void intel_engine_init_execlists(struct intel_engine_cs *engine);
>
> -void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine);
> -void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
> -
> -void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
> -
> -static inline void
> -intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
> -{
> - irq_work_queue(&engine->breadcrumbs.irq_work);
> -}
> -
> -void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
> -void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
> -
> -void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
> - struct drm_printer *p);
> -
> static inline u32 *__gen8_emit_pipe_control(u32 *batch, u32 flags0, u32 flags1, u32 offset)
> {
> memset(batch, 0, 6 * sizeof(u32));
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index dd1a42c4d344..c20a91c1318f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -28,6 +28,7 @@
>
> #include "i915_drv.h"
>
> +#include "intel_breadcrumbs.h"
> #include "intel_context.h"
> #include "intel_engine.h"
> #include "intel_engine_pm.h"
> @@ -700,8 +701,13 @@ static int engine_setup_common(struct intel_engine_cs *engine)
> if (err)
> return err;
>
> + engine->breadcrumbs = intel_breadcrumbs_create(engine);
> + if (!engine->breadcrumbs) {
> + err = -ENOMEM;
> + goto err_status;
> + }
> +
> intel_engine_init_active(engine, ENGINE_PHYSICAL);
> - intel_engine_init_breadcrumbs(engine);
> intel_engine_init_execlists(engine);
> intel_engine_init_cmd_parser(engine);
> intel_engine_init__pm(engine);
> @@ -716,6 +722,10 @@ static int engine_setup_common(struct intel_engine_cs *engine)
> intel_engine_init_ctx_wa(engine);
>
> return 0;
> +
> +err_status:
> + cleanup_status_page(engine);
> + return err;
> }
>
> struct measure_breadcrumb {
> @@ -902,9 +912,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
> tasklet_kill(&engine->execlists.tasklet); /* flush the callback */
>
> cleanup_status_page(engine);
> + intel_breadcrumbs_free(engine->breadcrumbs);
>
> intel_engine_fini_retire(engine);
> - intel_engine_fini_breadcrumbs(engine);
> intel_engine_cleanup_cmd_parser(engine);
>
> if (engine->default_state)
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> index 8ec3eecf3e39..f7b2e07e2229 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> @@ -6,6 +6,7 @@
>
> #include "i915_drv.h"
>
> +#include "intel_breadcrumbs.h"
> #include "intel_context.h"
> #include "intel_engine.h"
> #include "intel_engine_heartbeat.h"
> @@ -247,7 +248,7 @@ static int __engine_park(struct intel_wakeref *wf)
> call_idle_barriers(engine); /* cleanup after wedging */
>
> intel_engine_park_heartbeat(engine);
> - intel_engine_disarm_breadcrumbs(engine);
> + intel_breadcrumbs_park(engine->breadcrumbs);
>
> /* Must be reset upon idling, or we may miss the busy wakeup. */
> GEM_BUG_ON(engine->execlists.queue_priority_hint != INT_MIN);
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 8de92fd7d392..c400aaa2287b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -22,6 +22,7 @@
> #include "i915_pmu.h"
> #include "i915_priolist_types.h"
> #include "i915_selftest.h"
> +#include "intel_breadcrumbs_types.h"
> #include "intel_sseu.h"
> #include "intel_timeline_types.h"
> #include "intel_uncore.h"
> @@ -373,34 +374,8 @@ struct intel_engine_cs {
> */
> struct ewma__engine_latency latency;
>
> - /* Rather than have every client wait upon all user interrupts,
> - * with the herd waking after every interrupt and each doing the
> - * heavyweight seqno dance, we delegate the task (of being the
> - * bottom-half of the user interrupt) to the first client. After
> - * every interrupt, we wake up one client, who does the heavyweight
> - * coherent seqno read and either goes back to sleep (if incomplete),
> - * or wakes up all the completed clients in parallel, before then
> - * transferring the bottom-half status to the next client in the queue.
> - *
> - * Compared to walking the entire list of waiters in a single dedicated
> - * bottom-half, we reduce the latency of the first waiter by avoiding
> - * a context switch, but incur additional coherent seqno reads when
> - * following the chain of request breadcrumbs. Since it is most likely
> - * that we have a single client waiting on each seqno, then reducing
> - * the overhead of waking that client is much preferred.
> - */
> - struct intel_breadcrumbs {
> - spinlock_t irq_lock;
> - struct list_head signalers;
> -
> - struct list_head signaled_requests;
> -
> - struct irq_work irq_work; /* for use from inside irq_lock */
> -
> - unsigned int irq_enabled;
> -
> - bool irq_armed;
> - } breadcrumbs;
> + /* Keep track of all the seqno used, a trail of breadcrumbs */
> + struct intel_breadcrumbs *breadcrumbs;
>
> struct intel_engine_pmu {
> /**
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> index b05da68e52f4..257063a57101 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> @@ -8,6 +8,7 @@
>
> #include "i915_drv.h"
> #include "i915_irq.h"
> +#include "intel_breadcrumbs.h"
> #include "intel_gt.h"
> #include "intel_gt_irq.h"
> #include "intel_uncore.h"
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 5e8278e8ac79..9112bb07a068 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -137,6 +137,7 @@
> #include "i915_perf.h"
> #include "i915_trace.h"
> #include "i915_vgpu.h"
> +#include "intel_breadcrumbs.h"
> #include "intel_context.h"
> #include "intel_engine_pm.h"
> #include "intel_gt.h"
> @@ -4119,7 +4120,7 @@ static int execlists_resume(struct intel_engine_cs *engine)
> {
> intel_mocs_init_engine(engine);
>
> - intel_engine_reset_breadcrumbs(engine);
> + intel_breadcrumbs_reset(engine->breadcrumbs);
>
> if (GEM_SHOW_DEBUG() && unexpected_starting_state(engine)) {
> struct drm_printer p = drm_debug_printer(__func__);
> @@ -5704,9 +5705,7 @@ intel_execlists_create_virtual(struct intel_engine_cs **siblings,
> snprintf(ve->base.name, sizeof(ve->base.name), "virtual");
>
> intel_engine_init_active(&ve->base, ENGINE_VIRTUAL);
> - intel_engine_init_breadcrumbs(&ve->base);
> intel_engine_init_execlists(&ve->base);
> - ve->base.breadcrumbs.irq_armed = true; /* fake HW, used for irq_work */
>
> ve->base.cops = &virtual_context_ops;
> ve->base.request_alloc = execlists_request_alloc;
> @@ -5723,6 +5722,12 @@ intel_execlists_create_virtual(struct intel_engine_cs **siblings,
>
> intel_context_init(&ve->context, &ve->base);
>
> + ve->base.breadcrumbs = intel_breadcrumbs_create(NULL);
> + if (!ve->base.breadcrumbs) {
> + err = -ENOMEM;
> + goto err_put;
> + }
> +
> for (n = 0; n < count; n++) {
> struct intel_engine_cs *sibling = siblings[n];
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 46a5ceffc22f..ac36b67fb46b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -15,6 +15,7 @@
> #include "i915_drv.h"
> #include "i915_gpu_error.h"
> #include "i915_irq.h"
> +#include "intel_breadcrumbs.h"
> #include "intel_engine_pm.h"
> #include "intel_gt.h"
> #include "intel_gt_pm.h"
> diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> index 94915f668715..186aa2d3a83e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> @@ -32,6 +32,7 @@
> #include "gen6_ppgtt.h"
> #include "gen7_renderclear.h"
> #include "i915_drv.h"
> +#include "intel_breadcrumbs.h"
> #include "intel_context.h"
> #include "intel_gt.h"
> #include "intel_reset.h"
> @@ -255,7 +256,7 @@ static int xcs_resume(struct intel_engine_cs *engine)
> else
> ring_setup_status_page(engine);
>
> - intel_engine_reset_breadcrumbs(engine);
> + intel_breadcrumbs_reset(engine->breadcrumbs);
>
> /* Enforce ordering by reading HEAD register back */
> ENGINE_POSTING_READ(engine, RING_HEAD);
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
> index 97ba14ad52e4..e6a00eea0631 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -7,6 +7,7 @@
> #include <drm/i915_drm.h>
>
> #include "i915_drv.h"
> +#include "intel_breadcrumbs.h"
> #include "intel_gt.h"
> #include "intel_gt_clock_utils.h"
> #include "intel_gt_irq.h"
> diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
> index 06303ba98c19..d1971ffdca42 100644
> --- a/drivers/gpu/drm/i915/gt/mock_engine.c
> +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
> @@ -261,11 +261,12 @@ static void mock_engine_release(struct intel_engine_cs *engine)
>
> GEM_BUG_ON(timer_pending(&mock->hw_delay));
>
> + intel_breadcrumbs_free(engine->breadcrumbs);
> +
> intel_context_unpin(engine->kernel_context);
> intel_context_put(engine->kernel_context);
>
> intel_engine_fini_retire(engine);
> - intel_engine_fini_breadcrumbs(engine);
> }
>
> struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
> @@ -323,11 +324,14 @@ int mock_engine_init(struct intel_engine_cs *engine)
> struct intel_context *ce;
>
> intel_engine_init_active(engine, ENGINE_MOCK);
> - intel_engine_init_breadcrumbs(engine);
> intel_engine_init_execlists(engine);
> intel_engine_init__pm(engine);
> intel_engine_init_retire(engine);
>
> + engine->breadcrumbs = intel_breadcrumbs_create(NULL);
> + if (!engine->breadcrumbs)
> + return -ENOMEM;
> +
> ce = create_kernel_context(engine);
> if (IS_ERR(ce))
> goto err_breadcrumbs;
> @@ -339,7 +343,7 @@ int mock_engine_init(struct intel_engine_cs *engine)
> return 0;
>
> err_breadcrumbs:
> - intel_engine_fini_breadcrumbs(engine);
> + intel_breadcrumbs_free(engine->breadcrumbs);
> return -ENOMEM;
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1fa67700d8f4..f113fe44572b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -41,6 +41,7 @@
> #include "display/intel_lpe_audio.h"
> #include "display/intel_psr.h"
>
> +#include "gt/intel_breadcrumbs.h"
> #include "gt/intel_gt.h"
> #include "gt/intel_gt_irq.h"
> #include "gt/intel_gt_pm_irq.h"
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index ada630677cf3..394587e70a2d 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -31,6 +31,7 @@
> #include <linux/sched/signal.h>
>
> #include "gem/i915_gem_context.h"
> +#include "gt/intel_breadcrumbs.h"
> #include "gt/intel_context.h"
> #include "gt/intel_ring.h"
> #include "gt/intel_rps.h"
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index fc18378c685d..16b721080195 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -361,10 +361,6 @@ void i915_request_submit(struct i915_request *request);
> void __i915_request_unsubmit(struct i915_request *request);
> void i915_request_unsubmit(struct i915_request *request);
>
> -/* Note: part of the intel_breadcrumbs family */
> -bool i915_request_enable_breadcrumb(struct i915_request *request);
> -void i915_request_cancel_breadcrumb(struct i915_request *request);
> -
> long i915_request_wait(struct i915_request *rq,
> unsigned int flags,
> long timeout)
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Intel-gfx] ✗ Fi.CI.IGT: failure for series starting with [01/10] drm/i915/gem: Remove disordered per-file request list for throttling
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
` (11 preceding siblings ...)
2020-07-20 10:55 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
@ 2020-07-20 13:44 ` Patchwork
12 siblings, 0 replies; 26+ messages in thread
From: Patchwork @ 2020-07-20 13:44 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 16595 bytes --]
== Series Details ==
Series: series starting with [01/10] drm/i915/gem: Remove disordered per-file request list for throttling
URL : https://patchwork.freedesktop.org/series/79663/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_8766_full -> Patchwork_18210_full
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_18210_full absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_18210_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_18210_full:
### IGT changes ###
#### Possible regressions ####
* igt@kms_cursor_legacy@cursor-vs-flip-toggle:
- shard-glk: [PASS][1] -> [INCOMPLETE][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-glk2/igt@kms_cursor_legacy@cursor-vs-flip-toggle.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-glk1/igt@kms_cursor_legacy@cursor-vs-flip-toggle.html
* igt@prime_busy@hang-wait@bcs0:
- shard-hsw: [PASS][3] -> [FAIL][4] +12 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-hsw1/igt@prime_busy@hang-wait@bcs0.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-hsw1/igt@prime_busy@hang-wait@bcs0.html
Known issues
------------
Here are the changes found in Patchwork_18210_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_pm_rpm@i2c:
- shard-skl: [PASS][5] -> [DMESG-WARN][6] ([i915#1982]) +8 similar issues
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-skl1/igt@i915_pm_rpm@i2c.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-skl10/igt@i915_pm_rpm@i2c.html
* igt@i915_selftest@mock@requests:
- shard-apl: [PASS][7] -> [INCOMPLETE][8] ([i915#1635] / [i915#2110])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-apl3/igt@i915_selftest@mock@requests.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-apl4/igt@i915_selftest@mock@requests.html
* igt@kms_big_fb@x-tiled-64bpp-rotate-0:
- shard-glk: [PASS][9] -> [DMESG-FAIL][10] ([i915#118] / [i915#95])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-glk2/igt@kms_big_fb@x-tiled-64bpp-rotate-0.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-glk8/igt@kms_big_fb@x-tiled-64bpp-rotate-0.html
* igt@kms_cursor_edge_walk@pipe-b-256x256-bottom-edge:
- shard-glk: [PASS][11] -> [DMESG-WARN][12] ([i915#1982])
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-glk7/igt@kms_cursor_edge_walk@pipe-b-256x256-bottom-edge.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-glk9/igt@kms_cursor_edge_walk@pipe-b-256x256-bottom-edge.html
* igt@kms_flip@flip-vs-suspend@b-dp1:
- shard-kbl: [PASS][13] -> [DMESG-WARN][14] ([i915#180]) +5 similar issues
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-kbl4/igt@kms_flip@flip-vs-suspend@b-dp1.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-kbl2/igt@kms_flip@flip-vs-suspend@b-dp1.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-rte:
- shard-tglb: [PASS][15] -> [DMESG-WARN][16] ([i915#1982])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-tglb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-rte.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-tglb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-rte.html
* igt@kms_hdr@bpc-switch-suspend:
- shard-skl: [PASS][17] -> [FAIL][18] ([i915#1188])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-skl6/igt@kms_hdr@bpc-switch-suspend.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-skl10/igt@kms_hdr@bpc-switch-suspend.html
* igt@kms_psr@no_drrs:
- shard-iclb: [PASS][19] -> [FAIL][20] ([i915#173])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-iclb7/igt@kms_psr@no_drrs.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-iclb1/igt@kms_psr@no_drrs.html
* igt@kms_psr@psr2_sprite_plane_move:
- shard-iclb: [PASS][21] -> [SKIP][22] ([fdo#109441]) +4 similar issues
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-iclb8/igt@kms_psr@psr2_sprite_plane_move.html
* igt@kms_setmode@basic:
- shard-kbl: [PASS][23] -> [FAIL][24] ([i915#31])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-kbl1/igt@kms_setmode@basic.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-kbl6/igt@kms_setmode@basic.html
* igt@perf_pmu@semaphore-busy@rcs0:
- shard-kbl: [PASS][25] -> [FAIL][26] ([i915#1820])
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-kbl4/igt@perf_pmu@semaphore-busy@rcs0.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-kbl2/igt@perf_pmu@semaphore-busy@rcs0.html
#### Possible fixes ####
* igt@i915_pm_dc@dc6-psr:
- shard-iclb: [FAIL][27] ([i915#454]) -> [PASS][28]
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-iclb8/igt@i915_pm_dc@dc6-psr.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-iclb7/igt@i915_pm_dc@dc6-psr.html
* igt@i915_selftest@mock@requests:
- shard-kbl: [INCOMPLETE][29] ([i915#2110]) -> [PASS][30]
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-kbl7/igt@i915_selftest@mock@requests.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-kbl3/igt@i915_selftest@mock@requests.html
- shard-tglb: [INCOMPLETE][31] ([i915#2110]) -> [PASS][32]
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-tglb5/igt@i915_selftest@mock@requests.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-tglb6/igt@i915_selftest@mock@requests.html
- shard-skl: [INCOMPLETE][33] ([i915#198] / [i915#2110]) -> [PASS][34]
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-skl1/igt@i915_selftest@mock@requests.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-skl10/igt@i915_selftest@mock@requests.html
- shard-iclb: [INCOMPLETE][35] ([i915#2110]) -> [PASS][36]
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-iclb5/igt@i915_selftest@mock@requests.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-iclb1/igt@i915_selftest@mock@requests.html
- shard-hsw: [INCOMPLETE][37] ([i915#2110]) -> [PASS][38]
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-hsw7/igt@i915_selftest@mock@requests.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-hsw4/igt@i915_selftest@mock@requests.html
* igt@kms_cursor_crc@pipe-a-cursor-suspend:
- shard-kbl: [DMESG-WARN][39] ([i915#180]) -> [PASS][40] +1 similar issue
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
* igt@kms_cursor_edge_walk@pipe-a-256x256-right-edge:
- shard-skl: [DMESG-WARN][41] ([i915#1982]) -> [PASS][42] +11 similar issues
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-skl2/igt@kms_cursor_edge_walk@pipe-a-256x256-right-edge.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-skl6/igt@kms_cursor_edge_walk@pipe-a-256x256-right-edge.html
* igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic:
- shard-glk: [FAIL][43] ([i915#72]) -> [PASS][44]
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-glk8/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-glk7/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic.html
* igt@kms_cursor_legacy@cursor-vs-flip-atomic-transitions-varying-size:
- shard-hsw: [FAIL][45] ([i915#57]) -> [PASS][46]
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-hsw6/igt@kms_cursor_legacy@cursor-vs-flip-atomic-transitions-varying-size.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-hsw8/igt@kms_cursor_legacy@cursor-vs-flip-atomic-transitions-varying-size.html
* igt@kms_frontbuffer_tracking@psr-farfromfence:
- shard-tglb: [DMESG-WARN][47] ([i915#1982]) -> [PASS][48] +1 similar issue
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-tglb1/igt@kms_frontbuffer_tracking@psr-farfromfence.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-tglb2/igt@kms_frontbuffer_tracking@psr-farfromfence.html
* igt@kms_frontbuffer_tracking@psr-rgb565-draw-mmap-gtt:
- shard-skl: [FAIL][49] ([i915#49]) -> [PASS][50]
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-skl9/igt@kms_frontbuffer_tracking@psr-rgb565-draw-mmap-gtt.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-skl1/igt@kms_frontbuffer_tracking@psr-rgb565-draw-mmap-gtt.html
* igt@kms_hdr@bpc-switch-dpms:
- shard-skl: [FAIL][51] ([i915#1188]) -> [PASS][52]
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-skl3/igt@kms_hdr@bpc-switch-dpms.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-skl9/igt@kms_hdr@bpc-switch-dpms.html
* igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min:
- shard-skl: [FAIL][53] ([fdo#108145] / [i915#265]) -> [PASS][54] +1 similar issue
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-skl2/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-skl4/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html
* igt@kms_psr@psr2_cursor_blt:
- shard-iclb: [SKIP][55] ([fdo#109441]) -> [PASS][56] +1 similar issue
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-iclb5/igt@kms_psr@psr2_cursor_blt.html
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-iclb2/igt@kms_psr@psr2_cursor_blt.html
* igt@perf@polling-parameterized:
- shard-tglb: [FAIL][57] ([i915#1542]) -> [PASS][58]
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-tglb7/igt@perf@polling-parameterized.html
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-tglb5/igt@perf@polling-parameterized.html
* igt@perf_pmu@module-unload:
- shard-apl: [DMESG-WARN][59] ([i915#1635] / [i915#1982]) -> [PASS][60] +1 similar issue
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-apl1/igt@perf_pmu@module-unload.html
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-apl6/igt@perf_pmu@module-unload.html
#### Warnings ####
* igt@gem_exec_reloc@basic-concurrent16:
- shard-glk: [INCOMPLETE][61] ([i915#1958]) -> [TIMEOUT][62] ([i915#1958] / [i915#2119])
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-glk1/igt@gem_exec_reloc@basic-concurrent16.html
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-glk3/igt@gem_exec_reloc@basic-concurrent16.html
* igt@i915_pm_dc@dc3co-vpb-simulation:
- shard-iclb: [SKIP][63] ([i915#658]) -> [SKIP][64] ([i915#588])
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-iclb6/igt@i915_pm_dc@dc3co-vpb-simulation.html
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-iclb2/igt@i915_pm_dc@dc3co-vpb-simulation.html
* igt@kms_color@pipe-a-ctm-max:
- shard-skl: [DMESG-FAIL][65] ([i915#1982]) -> [DMESG-WARN][66] ([i915#1982])
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-skl9/igt@kms_color@pipe-a-ctm-max.html
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-skl1/igt@kms_color@pipe-a-ctm-max.html
* igt@kms_content_protection@atomic-dpms:
- shard-kbl: [TIMEOUT][67] ([i915#1319] / [i915#2119]) -> [TIMEOUT][68] ([i915#1319] / [i915#1958] / [i915#2119])
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-kbl2/igt@kms_content_protection@atomic-dpms.html
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-kbl3/igt@kms_content_protection@atomic-dpms.html
* igt@kms_dp_dsc@basic-dsc-enable-edp:
- shard-iclb: [DMESG-WARN][69] ([i915#1226]) -> [SKIP][70] ([fdo#109349])
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-iclb8/igt@kms_dp_dsc@basic-dsc-enable-edp.html
* igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
- shard-skl: [DMESG-FAIL][71] ([fdo#108145] / [i915#1982]) -> [DMESG-WARN][72] ([i915#1982])
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-skl9/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-skl1/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
* igt@runner@aborted:
- shard-apl: [FAIL][73] ([i915#1610] / [i915#1635] / [i915#2110]) -> [FAIL][74] ([i915#1635] / [i915#2110])
[73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8766/shard-apl6/igt@runner@aborted.html
[74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/shard-apl4/igt@runner@aborted.html
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
[i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
[i915#1226]: https://gitlab.freedesktop.org/drm/intel/issues/1226
[i915#1319]: https://gitlab.freedesktop.org/drm/intel/issues/1319
[i915#1542]: https://gitlab.freedesktop.org/drm/intel/issues/1542
[i915#1610]: https://gitlab.freedesktop.org/drm/intel/issues/1610
[i915#1635]: https://gitlab.freedesktop.org/drm/intel/issues/1635
[i915#173]: https://gitlab.freedesktop.org/drm/intel/issues/173
[i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
[i915#1820]: https://gitlab.freedesktop.org/drm/intel/issues/1820
[i915#1958]: https://gitlab.freedesktop.org/drm/intel/issues/1958
[i915#198]: https://gitlab.freedesktop.org/drm/intel/issues/198
[i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
[i915#2110]: https://gitlab.freedesktop.org/drm/intel/issues/2110
[i915#2119]: https://gitlab.freedesktop.org/drm/intel/issues/2119
[i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
[i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
[i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
[i915#49]: https://gitlab.freedesktop.org/drm/intel/issues/49
[i915#57]: https://gitlab.freedesktop.org/drm/intel/issues/57
[i915#588]: https://gitlab.freedesktop.org/drm/intel/issues/588
[i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
[i915#72]: https://gitlab.freedesktop.org/drm/intel/issues/72
[i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
Participating hosts (10 -> 10)
------------------------------
No changes in participating hosts
Build changes
-------------
* Linux: CI_DRM_8766 -> Patchwork_18210
CI-20190529: 20190529
CI_DRM_8766: 947ce595ea05b4baaea060a7e018cc3f49eaf413 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5740: 6663e3ab5f77add7077711c2b649caf2bd7903c4 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_18210: a9f67affbc82ba919e3980d10bd2a447f4db4c07 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18210/index.html
[-- Attachment #1.2: Type: text/html, Size: 19938 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH 05/10] drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs
2020-07-20 11:23 ` Tvrtko Ursulin
@ 2020-07-20 15:02 ` Chris Wilson
2020-07-22 8:12 ` Tvrtko Ursulin
0 siblings, 1 reply; 26+ messages in thread
From: Chris Wilson @ 2020-07-20 15:02 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2020-07-20 12:23:35)
>
> On 20/07/2020 10:23, Chris Wilson wrote:
> > -void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
> > +struct intel_breadcrumbs *
> > +intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
> > {
> > - struct intel_breadcrumbs *b = &engine->breadcrumbs;
> > + struct intel_breadcrumbs *b;
> > +
> > + b = kzalloc(sizeof(*b), GFP_KERNEL);
> > + if (!b)
> > + return NULL;
> >
> > spin_lock_init(&b->irq_lock);
> > INIT_LIST_HEAD(&b->signalers);
> > INIT_LIST_HEAD(&b->signaled_requests);
> >
> > init_irq_work(&b->irq_work, signal_irq_work);
> > +
> > + b->irq_engine = irq_engine;
> > + if (!irq_engine)
> > + b->irq_armed = true; /* fake HW, used for irq_work */
>
> Disarm is checking for !b->irq_engine and arm asserts there must be when
> arming. If instead arm would abort on !b->irq_engine would it all work
> just as well without the need for this hack?
Yes, it is asymmetric. I thought keeping the asymmetry in place for the
conversion would be simpler, but didn't really make an attempt to make
irq_armed behave as one would expect.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH 05/10] drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs
2020-07-20 9:23 ` [Intel-gfx] [PATCH 05/10] drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs Chris Wilson
@ 2020-07-21 7:59 ` kernel test robot
2020-07-21 7:59 ` kernel test robot
1 sibling, 0 replies; 26+ messages in thread
From: kernel test robot @ 2020-07-21 7:59 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: clang-built-linux, kbuild-all, Chris Wilson
[-- Attachment #1: Type: text/plain, Size: 2194 bytes --]
Hi Chris,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-tip/drm-tip next-20200720]
[cannot apply to linus/master v5.8-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-gem-Remove-disordered-per-file-request-list-for-throttling/20200720-172543
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project cf1105069648446d58adfb7a6cc590013d6886ba)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/i915/i915_irq.c:44:
>> drivers/gpu/drm/i915/gt/intel_breadcrumbs.h:6:9: warning: '__INTEL_BREADCRUMBS___' is used as a header guard here, followed by #define of a different macro [-Wheader-guard]
#ifndef __INTEL_BREADCRUMBS___
^~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/gt/intel_breadcrumbs.h:7:9: note: '__INTEL_BREADCRUMBS__' is defined here; did you mean '__INTEL_BREADCRUMBS___'?
#define __INTEL_BREADCRUMBS__
^~~~~~~~~~~~~~~~~~~~~
__INTEL_BREADCRUMBS___
1 warning generated.
vim +/__INTEL_BREADCRUMBS___ +6 drivers/gpu/drm/i915/gt/intel_breadcrumbs.h
> 6 #ifndef __INTEL_BREADCRUMBS___
7 #define __INTEL_BREADCRUMBS__
8
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75351 bytes --]
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH 05/10] drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs
@ 2020-07-21 7:59 ` kernel test robot
0 siblings, 0 replies; 26+ messages in thread
From: kernel test robot @ 2020-07-21 7:59 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2243 bytes --]
Hi Chris,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-tip/drm-tip next-20200720]
[cannot apply to linus/master v5.8-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-gem-Remove-disordered-per-file-request-list-for-throttling/20200720-172543
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project cf1105069648446d58adfb7a6cc590013d6886ba)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/i915/i915_irq.c:44:
>> drivers/gpu/drm/i915/gt/intel_breadcrumbs.h:6:9: warning: '__INTEL_BREADCRUMBS___' is used as a header guard here, followed by #define of a different macro [-Wheader-guard]
#ifndef __INTEL_BREADCRUMBS___
^~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/gt/intel_breadcrumbs.h:7:9: note: '__INTEL_BREADCRUMBS__' is defined here; did you mean '__INTEL_BREADCRUMBS___'?
#define __INTEL_BREADCRUMBS__
^~~~~~~~~~~~~~~~~~~~~
__INTEL_BREADCRUMBS___
1 warning generated.
vim +/__INTEL_BREADCRUMBS___ +6 drivers/gpu/drm/i915/gt/intel_breadcrumbs.h
> 6 #ifndef __INTEL_BREADCRUMBS___
7 #define __INTEL_BREADCRUMBS__
8
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 75351 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH 05/10] drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs
2020-07-20 15:02 ` Chris Wilson
@ 2020-07-22 8:12 ` Tvrtko Ursulin
2020-07-22 8:30 ` Chris Wilson
0 siblings, 1 reply; 26+ messages in thread
From: Tvrtko Ursulin @ 2020-07-22 8:12 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 20/07/2020 16:02, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-07-20 12:23:35)
>>
>> On 20/07/2020 10:23, Chris Wilson wrote:
>>> -void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
>>> +struct intel_breadcrumbs *
>>> +intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
>>> {
>>> - struct intel_breadcrumbs *b = &engine->breadcrumbs;
>>> + struct intel_breadcrumbs *b;
>>> +
>>> + b = kzalloc(sizeof(*b), GFP_KERNEL);
>>> + if (!b)
>>> + return NULL;
>>>
>>> spin_lock_init(&b->irq_lock);
>>> INIT_LIST_HEAD(&b->signalers);
>>> INIT_LIST_HEAD(&b->signaled_requests);
>>>
>>> init_irq_work(&b->irq_work, signal_irq_work);
>>> +
>>> + b->irq_engine = irq_engine;
>>> + if (!irq_engine)
>>> + b->irq_armed = true; /* fake HW, used for irq_work */
>>
>> Disarm is checking for !b->irq_engine and arm asserts there must be when
>> arming. If instead arm would abort on !b->irq_engine would it all work
>> just as well without the need for this hack?
>
> Yes, it is asymmetric. I thought keeping the asymmetry in place for the
> conversion would be simpler, but didn't really make an attempt to make
> irq_armed behave as one would expect.
You think it's not as simple as early return in arm if on irq engine?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH 05/10] drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs
2020-07-22 8:12 ` Tvrtko Ursulin
@ 2020-07-22 8:30 ` Chris Wilson
0 siblings, 0 replies; 26+ messages in thread
From: Chris Wilson @ 2020-07-22 8:30 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2020-07-22 09:12:14)
>
> On 20/07/2020 16:02, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-07-20 12:23:35)
> >>
> >> On 20/07/2020 10:23, Chris Wilson wrote:
> >>> -void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
> >>> +struct intel_breadcrumbs *
> >>> +intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
> >>> {
> >>> - struct intel_breadcrumbs *b = &engine->breadcrumbs;
> >>> + struct intel_breadcrumbs *b;
> >>> +
> >>> + b = kzalloc(sizeof(*b), GFP_KERNEL);
> >>> + if (!b)
> >>> + return NULL;
> >>>
> >>> spin_lock_init(&b->irq_lock);
> >>> INIT_LIST_HEAD(&b->signalers);
> >>> INIT_LIST_HEAD(&b->signaled_requests);
> >>>
> >>> init_irq_work(&b->irq_work, signal_irq_work);
> >>> +
> >>> + b->irq_engine = irq_engine;
> >>> + if (!irq_engine)
> >>> + b->irq_armed = true; /* fake HW, used for irq_work */
> >>
> >> Disarm is checking for !b->irq_engine and arm asserts there must be when
> >> arming. If instead arm would abort on !b->irq_engine would it all work
> >> just as well without the need for this hack?
> >
> > Yes, it is asymmetric. I thought keeping the asymmetry in place for the
> > conversion would be simpler, but didn't really make an attempt to make
> > irq_armed behave as one would expect.
>
> You think it's not as simple as early return in arm if on irq engine?
And moving the early checks to disarm_irq for symmetry. I was just
worrying too much about the impact of changing irq_armed.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH 06/10] drm/i915/gt: Move intel_breadcrumbs_arm_irq earlier
2020-07-20 9:23 ` [Intel-gfx] [PATCH 06/10] drm/i915/gt: Move intel_breadcrumbs_arm_irq earlier Chris Wilson
@ 2020-07-22 12:55 ` Tvrtko Ursulin
0 siblings, 0 replies; 26+ messages in thread
From: Tvrtko Ursulin @ 2020-07-22 12:55 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 20/07/2020 10:23, Chris Wilson wrote:
> Move the __intel_breadcrumbs_arm_irq earlier, next to the disarm_irq, so
> that we can make use of it in the following patch.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 88 ++++++++++-----------
> 1 file changed, 44 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index 621cf9d1d7ad..d6008034869f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -54,6 +54,37 @@ static void irq_disable(struct intel_engine_cs *engine)
> spin_unlock(&engine->gt->irq_lock);
> }
>
> +static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
> +{
> + lockdep_assert_held(&b->irq_lock);
> +
> + if (b->irq_armed)
> + return;
> +
> + GEM_BUG_ON(!b->irq_engine);
> + if (!intel_gt_pm_get_if_awake(b->irq_engine->gt))
> + return;
> +
> + /*
> + * The breadcrumb irq will be disarmed on the interrupt after the
> + * waiters are signaled. This gives us a single interrupt window in
> + * which we can add a new waiter and avoid the cost of re-enabling
> + * the irq.
> + */
> + WRITE_ONCE(b->irq_armed, true);
> +
> + /*
> + * Since we are waiting on a request, the GPU should be busy
> + * and should have its own rpm reference. This is tracked
> + * by i915->gt.awake, we can forgo holding our own wakref
> + * for the interrupt as before i915->gt.awake is released (when
> + * the driver is idle) we disarm the breadcrumbs.
> + */
> +
> + if (!b->irq_enabled++)
> + irq_enable(b->irq_engine);
> +}
> +
> static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
> {
> lockdep_assert_held(&b->irq_lock);
> @@ -69,19 +100,6 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
> intel_gt_pm_put_async(b->irq_engine->gt);
> }
>
> -void intel_breadcrumbs_park(struct intel_breadcrumbs *b)
> -{
> - unsigned long flags;
> -
> - if (!READ_ONCE(b->irq_armed))
> - return;
> -
> - spin_lock_irqsave(&b->irq_lock, flags);
> - if (b->irq_armed)
> - __intel_breadcrumbs_disarm_irq(b);
> - spin_unlock_irqrestore(&b->irq_lock, flags);
> -}
> -
> static inline bool __request_completed(const struct i915_request *rq)
> {
> return i915_seqno_passed(__hwsp_seqno(rq), rq->fence.seqno);
> @@ -215,37 +233,6 @@ static void signal_irq_work(struct irq_work *work)
> }
> }
>
> -static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
> -{
> - lockdep_assert_held(&b->irq_lock);
> -
> - if (b->irq_armed)
> - return;
> -
> - GEM_BUG_ON(!b->irq_engine);
> - if (!intel_gt_pm_get_if_awake(b->irq_engine->gt))
> - return;
> -
> - /*
> - * The breadcrumb irq will be disarmed on the interrupt after the
> - * waiters are signaled. This gives us a single interrupt window in
> - * which we can add a new waiter and avoid the cost of re-enabling
> - * the irq.
> - */
> - WRITE_ONCE(b->irq_armed, true);
> -
> - /*
> - * Since we are waiting on a request, the GPU should be busy
> - * and should have its own rpm reference. This is tracked
> - * by i915->gt.awake, we can forgo holding our own wakref
> - * for the interrupt as before i915->gt.awake is released (when
> - * the driver is idle) we disarm the breadcrumbs.
> - */
> -
> - if (!b->irq_enabled++)
> - irq_enable(b->irq_engine);
> -}
> -
> struct intel_breadcrumbs *
> intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
> {
> @@ -285,6 +272,19 @@ void intel_breadcrumbs_reset(struct intel_breadcrumbs *b)
> spin_unlock_irqrestore(&b->irq_lock, flags);
> }
>
> +void intel_breadcrumbs_park(struct intel_breadcrumbs *b)
> +{
> + unsigned long flags;
> +
> + if (!READ_ONCE(b->irq_armed))
> + return;
> +
> + spin_lock_irqsave(&b->irq_lock, flags);
> + if (b->irq_armed)
> + __intel_breadcrumbs_disarm_irq(b);
> + spin_unlock_irqrestore(&b->irq_lock, flags);
> +}
> +
> void intel_breadcrumbs_free(struct intel_breadcrumbs *b)
> {
> kfree(b);
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH 07/10] drm/i915/gt: Hold context/request reference while breadcrumbs are active
2020-07-20 9:23 ` [Intel-gfx] [PATCH 07/10] drm/i915/gt: Hold context/request reference while breadcrumbs are active Chris Wilson
@ 2020-07-22 13:05 ` Tvrtko Ursulin
2020-07-22 13:11 ` Chris Wilson
0 siblings, 1 reply; 26+ messages in thread
From: Tvrtko Ursulin @ 2020-07-22 13:05 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 20/07/2020 10:23, Chris Wilson wrote:
> Currently we hold no actual reference to the request nor context while
> they are attached to a breadcrumb. To avoid freeing the request/context
> too early, we serialise with cancel-breadcrumbs by taking the irq
> spinlock in i915_request_retire(). The alternative is to take a
> reference for a new breadcrumb and release it upon signaling; removing
> the more frequently hit contention point in i915_request_retire().
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 58 ++++++++++++++++-----
> drivers/gpu/drm/i915/i915_request.c | 9 ++--
> 2 files changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index d6008034869f..59e8cd505569 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -29,6 +29,7 @@
> #include "i915_drv.h"
> #include "i915_trace.h"
> #include "intel_breadcrumbs.h"
> +#include "intel_context.h"
> #include "intel_gt_pm.h"
> #include "intel_gt_requests.h"
>
> @@ -100,6 +101,22 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
> intel_gt_pm_put_async(b->irq_engine->gt);
> }
>
> +static void add_signaling_context(struct intel_breadcrumbs *b,
> + struct intel_context *ce)
> +{
> + intel_context_get(ce);
> + list_add_tail(&ce->signal_link, &b->signalers);
> + if (list_is_first(&ce->signal_link, &b->signalers))
> + __intel_breadcrumbs_arm_irq(b);
> +}
> +
> +static void remove_signaling_context(struct intel_breadcrumbs *b,
> + struct intel_context *ce)
> +{
> + list_del(&ce->signal_link);
> + intel_context_put(ce);
> +}
> +
> static inline bool __request_completed(const struct i915_request *rq)
> {
> return i915_seqno_passed(__hwsp_seqno(rq), rq->fence.seqno);
> @@ -108,6 +125,9 @@ static inline bool __request_completed(const struct i915_request *rq)
> __maybe_unused static bool
> check_signal_order(struct intel_context *ce, struct i915_request *rq)
> {
> + if (rq->context != ce)
> + return false;
> +
> if (!list_is_last(&rq->signal_link, &ce->signals) &&
> i915_seqno_passed(rq->fence.seqno,
> list_next_entry(rq, signal_link)->fence.seqno))
> @@ -162,7 +182,6 @@ static bool __signal_request(struct i915_request *rq, struct list_head *signals)
> if (!__dma_fence_signal(&rq->fence))
> return false;
>
> - i915_request_get(rq);
> list_add_tail(&rq->signal_link, signals);
> return true;
> }
> @@ -198,7 +217,8 @@ static void signal_irq_work(struct irq_work *work)
> * spinlock as the callback chain may end up adding
> * more signalers to the same context or engine.
> */
> - __signal_request(rq, &signal);
> + if (!__signal_request(rq, &signal))
> + i915_request_put(rq);
Looks like __signal_request could do the request put but doesn't matter
hugely.
> }
>
> /*
> @@ -210,7 +230,7 @@ static void signal_irq_work(struct irq_work *work)
> /* Advance the list to the first incomplete request */
> __list_del_many(&ce->signals, pos);
> if (&ce->signals == pos) { /* now empty */
> - list_del_init(&ce->signal_link);
> + remove_signaling_context(b, ce);
> add_retire(b, ce->timeline);
> }
> }
> @@ -282,6 +302,8 @@ void intel_breadcrumbs_park(struct intel_breadcrumbs *b)
> spin_lock_irqsave(&b->irq_lock, flags);
> if (b->irq_armed)
> __intel_breadcrumbs_disarm_irq(b);
> + if (!list_empty(&b->signalers))
> + irq_work_queue(&b->irq_work);
> spin_unlock_irqrestore(&b->irq_lock, flags);
> }
>
> @@ -299,6 +321,8 @@ static void insert_breadcrumb(struct i915_request *rq,
> if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
> return;
>
> + i915_request_get(rq);
> +
> /*
> * If the request is already completed, we can transfer it
> * straight onto a signaled list, and queue the irq worker for
> @@ -307,11 +331,11 @@ static void insert_breadcrumb(struct i915_request *rq,
> if (__request_completed(rq)) {
> if (__signal_request(rq, &b->signaled_requests))
> irq_work_queue(&b->irq_work);
> + else
> + i915_request_put(rq);
> return;
> }
>
> - __intel_breadcrumbs_arm_irq(b);
> -
> /*
> * We keep the seqno in retirement order, so we can break
> * inside intel_engine_signal_breadcrumbs as soon as we've
> @@ -326,16 +350,20 @@ static void insert_breadcrumb(struct i915_request *rq,
> * start looking for our insertion point from the tail of
> * the list.
> */
> - list_for_each_prev(pos, &ce->signals) {
> - struct i915_request *it =
> - list_entry(pos, typeof(*it), signal_link);
> -
> - if (i915_seqno_passed(rq->fence.seqno, it->fence.seqno))
> - break;
> + if (list_empty(&ce->signals)) {
> + add_signaling_context(b, ce);
> + GEM_BUG_ON(!b->irq_armed);
> + pos = &ce->signals;
> + } else {
> + list_for_each_prev(pos, &ce->signals) {
> + struct i915_request *it =
> + list_entry(pos, typeof(*it), signal_link);
> +
> + if (i915_seqno_passed(rq->fence.seqno, it->fence.seqno))
> + break;
> + }
> }
> list_add(&rq->signal_link, pos);
> - if (pos == &ce->signals) /* catch transitions from empty list */
> - list_move_tail(&ce->signal_link, &b->signalers);
> GEM_BUG_ON(!check_signal_order(ce, rq));
> set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
>
> @@ -416,9 +444,10 @@ void i915_request_cancel_breadcrumb(struct i915_request *rq)
>
> list_del(&rq->signal_link);
> if (list_empty(&ce->signals))
> - list_del_init(&ce->signal_link);
> + remove_signaling_context(b, ce);
>
> clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> + i915_request_put(rq);
> }
> spin_unlock(&b->irq_lock);
> }
> @@ -433,6 +462,7 @@ void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
> if (!b || list_empty(&b->signalers))
> return;
>
> + drm_printf(p, "IRQ: %s\n", enableddisabled(b->irq_armed));
> drm_printf(p, "Signals:\n");
>
> spin_lock_irq(&b->irq_lock);
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 394587e70a2d..4ebb0f144ac4 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -300,12 +300,11 @@ bool i915_request_retire(struct i915_request *rq)
> __i915_request_fill(rq, POISON_FREE);
> rq->ring->head = rq->postfix;
>
> - spin_lock_irq(&rq->lock);
> - if (!i915_request_signaled(rq))
> + if (!i915_request_signaled(rq)) {
> + spin_lock_irq(&rq->lock);
> dma_fence_signal_locked(&rq->fence);
> - if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &rq->fence.flags))
> - i915_request_cancel_breadcrumb(rq);
> - spin_unlock_irq(&rq->lock);
> + spin_unlock_irq(&rq->lock);
> + }
>
> if (i915_request_has_waitboost(rq)) {
> GEM_BUG_ON(!atomic_read(&rq->engine->gt->rps.num_waiters));
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH 07/10] drm/i915/gt: Hold context/request reference while breadcrumbs are active
2020-07-22 13:05 ` Tvrtko Ursulin
@ 2020-07-22 13:11 ` Chris Wilson
0 siblings, 0 replies; 26+ messages in thread
From: Chris Wilson @ 2020-07-22 13:11 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2020-07-22 14:05:35)
>
> On 20/07/2020 10:23, Chris Wilson wrote:
> > @@ -198,7 +217,8 @@ static void signal_irq_work(struct irq_work *work)
> > * spinlock as the callback chain may end up adding
> > * more signalers to the same context or engine.
> > */
> > - __signal_request(rq, &signal);
> > + if (!__signal_request(rq, &signal))
> > + i915_request_put(rq);
>
> Looks like __signal_request could do the request put but doesn't matter
> hugely.
I ditch the __signal_request() wrapper as the two callers diverge a bit
more.
1:
clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
if (__dma_fence_signal(&rq->fence)) {
rq->signal_node.next = signal;
signal = &rq->signal_node;
} else {
i915_request_put(rq);
}
2:
if (__request_completed(rq)) {
if (__dma_fence_signal(&rq->fence)) {
if (llist_add(&rq->signal_node, &b->signaled_requests))
irq_work_queue(&b->irq_work);
} else {
i915_request_put(rq);
}
return;
}
Not completely sold on that though. Keeping the i915_request_put() as
part of __signal_request() would remove the duplicate line there.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH 08/10] drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock
2020-07-20 9:23 ` [Intel-gfx] [PATCH 08/10] drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock Chris Wilson
@ 2020-07-22 13:26 ` Tvrtko Ursulin
2020-07-22 13:42 ` Chris Wilson
0 siblings, 1 reply; 26+ messages in thread
From: Tvrtko Ursulin @ 2020-07-22 13:26 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 20/07/2020 10:23, Chris Wilson wrote:
> Make b->signaled_requests a lockless-list so that we can manipulate it
> outside of the b->irq_lock.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 42 +++++++++----------
> .../gpu/drm/i915/gt/intel_breadcrumbs_types.h | 2 +-
> drivers/gpu/drm/i915/i915_request.h | 6 ++-
> 3 files changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index 59e8cd505569..2b3ad17c63b9 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -175,32 +175,23 @@ static void add_retire(struct intel_breadcrumbs *b, struct intel_timeline *tl)
> intel_engine_add_retire(b->irq_engine, tl);
> }
>
> -static bool __signal_request(struct i915_request *rq, struct list_head *signals)
> -{
> - clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> -
> - if (!__dma_fence_signal(&rq->fence))
> - return false;
> -
> - list_add_tail(&rq->signal_link, signals);
> - return true;
> -}
> -
> static void signal_irq_work(struct irq_work *work)
> {
> struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
> const ktime_t timestamp = ktime_get();
> + struct llist_node *signal, *sn;
> struct intel_context *ce, *cn;
> struct list_head *pos, *next;
> - LIST_HEAD(signal);
> +
> + signal = NULL;
> + if (!llist_empty(&b->signaled_requests))
> + signal = llist_del_all(&b->signaled_requests);
Uncoditional llist_del_all? It's not likely list will be empty and if it
is llist_del_all will return NULL.
>
> spin_lock(&b->irq_lock);
>
> - if (b->irq_armed && list_empty(&b->signalers))
> + if (!signal && b->irq_armed && list_empty(&b->signalers))
Why !signal check in here? Couldn't figure out what changed to make this
needed.
> __intel_breadcrumbs_disarm_irq(b);
>
> - list_splice_init(&b->signaled_requests, &signal);
> -
> list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
> GEM_BUG_ON(list_empty(&ce->signals));
>
> @@ -217,8 +208,13 @@ static void signal_irq_work(struct irq_work *work)
> * spinlock as the callback chain may end up adding
> * more signalers to the same context or engine.
> */
> - if (!__signal_request(rq, &signal))
> + clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> + if (__dma_fence_signal(&rq->fence)) {
> + rq->signal_node.next = signal;
> + signal = &rq->signal_node;
Okay it creates a bit of a differently ordered list like this but I
think it doesn't matter.
> + } else {
> i915_request_put(rq);
> + }
> }
>
> /*
> @@ -238,9 +234,9 @@ static void signal_irq_work(struct irq_work *work)
>
> spin_unlock(&b->irq_lock);
>
> - list_for_each_safe(pos, next, &signal) {
> + llist_for_each_safe(signal, sn, signal) {
> struct i915_request *rq =
> - list_entry(pos, typeof(*rq), signal_link);
> + llist_entry(signal, typeof(*rq), signal_node);
> struct list_head cb_list;
>
> spin_lock(&rq->lock);
> @@ -264,7 +260,7 @@ intel_breadcrumbs_create(struct intel_engine_cs *irq_engine)
>
> spin_lock_init(&b->irq_lock);
> INIT_LIST_HEAD(&b->signalers);
> - INIT_LIST_HEAD(&b->signaled_requests);
> + init_llist_head(&b->signaled_requests);
>
> init_irq_work(&b->irq_work, signal_irq_work);
>
> @@ -329,10 +325,12 @@ static void insert_breadcrumb(struct i915_request *rq,
> * its signal completion.
> */
> if (__request_completed(rq)) {
> - if (__signal_request(rq, &b->signaled_requests))
> - irq_work_queue(&b->irq_work);
> - else
> + if (__dma_fence_signal(&rq->fence)) {
> + if (llist_add(&rq->signal_node, &b->signaled_requests))
> + irq_work_queue(&b->irq_work);
> + } else {
> i915_request_put(rq);
> + }
> return;
> }
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> index 8e53b9942695..3fa19820b37a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs_types.h
> @@ -35,7 +35,7 @@ struct intel_breadcrumbs {
> struct intel_engine_cs *irq_engine;
>
> struct list_head signalers;
> - struct list_head signaled_requests;
> + struct llist_head signaled_requests;
>
> struct irq_work irq_work; /* for use from inside irq_lock */
>
> diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> index 16b721080195..874af6db6103 100644
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -176,7 +176,11 @@ struct i915_request {
> struct intel_context *context;
> struct intel_ring *ring;
> struct intel_timeline __rcu *timeline;
> - struct list_head signal_link;
> +
> + union {
> + struct list_head signal_link;
> + struct llist_node signal_node;
Transition is only from signal_link to signal_node, which uses and
initializes only one field, so I think potential garbage in other ones
is safe.
> + };
>
> /*
> * The rcu epoch of when this request was allocated. Used to judiciously
>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Intel-gfx] [PATCH 08/10] drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock
2020-07-22 13:26 ` Tvrtko Ursulin
@ 2020-07-22 13:42 ` Chris Wilson
0 siblings, 0 replies; 26+ messages in thread
From: Chris Wilson @ 2020-07-22 13:42 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2020-07-22 14:26:36)
>
> On 20/07/2020 10:23, Chris Wilson wrote:
> > Make b->signaled_requests a lockless-list so that we can manipulate it
> > outside of the b->irq_lock.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 42 +++++++++----------
> > .../gpu/drm/i915/gt/intel_breadcrumbs_types.h | 2 +-
> > drivers/gpu/drm/i915/i915_request.h | 6 ++-
> > 3 files changed, 26 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > index 59e8cd505569..2b3ad17c63b9 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > @@ -175,32 +175,23 @@ static void add_retire(struct intel_breadcrumbs *b, struct intel_timeline *tl)
> > intel_engine_add_retire(b->irq_engine, tl);
> > }
> >
> > -static bool __signal_request(struct i915_request *rq, struct list_head *signals)
> > -{
> > - clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> > -
> > - if (!__dma_fence_signal(&rq->fence))
> > - return false;
> > -
> > - list_add_tail(&rq->signal_link, signals);
> > - return true;
> > -}
> > -
> > static void signal_irq_work(struct irq_work *work)
> > {
> > struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
> > const ktime_t timestamp = ktime_get();
> > + struct llist_node *signal, *sn;
> > struct intel_context *ce, *cn;
> > struct list_head *pos, *next;
> > - LIST_HEAD(signal);
> > +
> > + signal = NULL;
> > + if (!llist_empty(&b->signaled_requests))
> > + signal = llist_del_all(&b->signaled_requests);
>
> Uncoditional llist_del_all? It's not likely list will be empty and if it
> is llist_del_all will return NULL.
Nah, this is likely to be empty, since this will only be filled after we
resubmit an already completed request. So the conditional is cheaper
than the locked xchg.
> > spin_lock(&b->irq_lock);
> >
> > - if (b->irq_armed && list_empty(&b->signalers))
> > + if (!signal && b->irq_armed && list_empty(&b->signalers))
>
> Why !signal check in here? Couldn't figure out what changed to make this
> needed.
Because we invoke signal_irq_work() after adding to
b->signaled_requests, I thought it was sensible to attempt keep the
interrupt shadow in place until after the following interrupt.
[Sadly we need to avoid the frequent enable/disable of the interrupts,
they are expensive enough to perform that it shows up in throughput
measurement. The question has become our _user_ interrupts now cheap
enough to always enable? The problem with snb dying under an interrupt
storm has been fixed, afaict we no longer burn through an entire core
handling interrupts when the GPU is busy.]
> > __intel_breadcrumbs_disarm_irq(b);
> >
> > - list_splice_init(&b->signaled_requests, &signal);
> > -
> > list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
> > GEM_BUG_ON(list_empty(&ce->signals));
> >
> > @@ -217,8 +208,13 @@ static void signal_irq_work(struct irq_work *work)
> > * spinlock as the callback chain may end up adding
> > * more signalers to the same context or engine.
> > */
> > - if (!__signal_request(rq, &signal))
> > + clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> > + if (__dma_fence_signal(&rq->fence)) {
> > + rq->signal_node.next = signal;
> > + signal = &rq->signal_node;
>
> Okay it creates a bit of a differently ordered list like this but I
> think it doesn't matter.
Right. We change the order in which we signal, but I reasoned that we
are already signaling in a loose order as dma_fence_signal() is called
from many, many different paths without a care to timeline order.
Signaling the most recent first may improve latency in some cases, and
since the latency will be smaller for the most recent request that will
be a relatively large improvement vs the increase in latency for
handling the older request after the most recent. This is pure
speculation, I haven't measured the effect.
> > diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
> > index 16b721080195..874af6db6103 100644
> > --- a/drivers/gpu/drm/i915/i915_request.h
> > +++ b/drivers/gpu/drm/i915/i915_request.h
> > @@ -176,7 +176,11 @@ struct i915_request {
> > struct intel_context *context;
> > struct intel_ring *ring;
> > struct intel_timeline __rcu *timeline;
> > - struct list_head signal_link;
> > +
> > + union {
> > + struct list_head signal_link;
> > + struct llist_node signal_node;
>
> Transition is only from signal_link to signal_node, which uses and
> initializes only one field, so I think potential garbage in other ones
> is safe.
Yup, the signal_link was used to being garbage after signaling :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2020-07-22 13:42 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-20 9:23 [Intel-gfx] [PATCH 01/10] drm/i915/gem: Remove disordered per-file request list for throttling Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 02/10] drm/i915: Remove requirement for holding i915_request.lock for breadcrumbs Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 03/10] drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs Chris Wilson
2020-07-20 9:32 ` Tvrtko Ursulin
2020-07-20 9:23 ` [Intel-gfx] [PATCH 04/10] drm/i915/gt: Only transfer the virtual context to the new engine if active Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 05/10] drm/i915/gt: Distinguish the virtual breadcrumbs from the irq breadcrumbs Chris Wilson
2020-07-20 11:23 ` Tvrtko Ursulin
2020-07-20 15:02 ` Chris Wilson
2020-07-22 8:12 ` Tvrtko Ursulin
2020-07-22 8:30 ` Chris Wilson
2020-07-21 7:59 ` kernel test robot
2020-07-21 7:59 ` kernel test robot
2020-07-20 9:23 ` [Intel-gfx] [PATCH 06/10] drm/i915/gt: Move intel_breadcrumbs_arm_irq earlier Chris Wilson
2020-07-22 12:55 ` Tvrtko Ursulin
2020-07-20 9:23 ` [Intel-gfx] [PATCH 07/10] drm/i915/gt: Hold context/request reference while breadcrumbs are active Chris Wilson
2020-07-22 13:05 ` Tvrtko Ursulin
2020-07-22 13:11 ` Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 08/10] drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock Chris Wilson
2020-07-22 13:26 ` Tvrtko Ursulin
2020-07-22 13:42 ` Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 09/10] drm/i915/gt: Split the breadcrumb spinlock between global and contexts Chris Wilson
2020-07-20 9:23 ` [Intel-gfx] [PATCH 10/10] drm/i915: Drop i915_request.lock serialisation around await_start Chris Wilson
2020-07-20 10:33 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/10] drm/i915/gem: Remove disordered per-file request list for throttling Patchwork
2020-07-20 10:34 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2020-07-20 10:55 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-07-20 13:44 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.