Commit e2300560 authored by Chris Wilson's avatar Chris Wilson Committed by Joonas Lahtinen

drm/i915/gt: Hold context/request reference while breadcrumbs are active

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: default avatarChris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: default avatarTvrtko Ursulin <tvrtko.ursulin@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20200801160225.6814-2-chris@chris-wilson.co.ukSigned-off-by: default avatarRodrigo Vivi <rodrigo.vivi@intel.com>
[Joonas: Rebased and reordered into drm-intel-gt-next branch]
Signed-off-by: default avatarJoonas Lahtinen <joonas.lahtinen@linux.intel.com>
parent 3f7dc107
...@@ -29,6 +29,7 @@ ...@@ -29,6 +29,7 @@
#include "i915_drv.h" #include "i915_drv.h"
#include "i915_trace.h" #include "i915_trace.h"
#include "intel_breadcrumbs.h" #include "intel_breadcrumbs.h"
#include "intel_context.h"
#include "intel_gt_pm.h" #include "intel_gt_pm.h"
#include "intel_gt_requests.h" #include "intel_gt_requests.h"
...@@ -99,6 +100,22 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b) ...@@ -99,6 +100,22 @@ static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
intel_gt_pm_put_async(b->irq_engine->gt); 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) static inline bool __request_completed(const struct i915_request *rq)
{ {
return i915_seqno_passed(__hwsp_seqno(rq), rq->fence.seqno); return i915_seqno_passed(__hwsp_seqno(rq), rq->fence.seqno);
...@@ -107,6 +124,9 @@ static inline bool __request_completed(const struct i915_request *rq) ...@@ -107,6 +124,9 @@ static inline bool __request_completed(const struct i915_request *rq)
__maybe_unused static bool __maybe_unused static bool
check_signal_order(struct intel_context *ce, struct i915_request *rq) 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) && if (!list_is_last(&rq->signal_link, &ce->signals) &&
i915_seqno_passed(rq->fence.seqno, i915_seqno_passed(rq->fence.seqno,
list_next_entry(rq, signal_link)->fence.seqno)) list_next_entry(rq, signal_link)->fence.seqno))
...@@ -158,10 +178,11 @@ static bool __signal_request(struct i915_request *rq, struct list_head *signals) ...@@ -158,10 +178,11 @@ static bool __signal_request(struct i915_request *rq, struct list_head *signals)
{ {
clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags); clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
if (!__dma_fence_signal(&rq->fence)) if (!__dma_fence_signal(&rq->fence)) {
i915_request_put(rq);
return false; return false;
}
i915_request_get(rq);
list_add_tail(&rq->signal_link, signals); list_add_tail(&rq->signal_link, signals);
return true; return true;
} }
...@@ -209,8 +230,8 @@ static void signal_irq_work(struct irq_work *work) ...@@ -209,8 +230,8 @@ static void signal_irq_work(struct irq_work *work)
/* Advance the list to the first incomplete request */ /* Advance the list to the first incomplete request */
__list_del_many(&ce->signals, pos); __list_del_many(&ce->signals, pos);
if (&ce->signals == pos) { /* now empty */ if (&ce->signals == pos) { /* now empty */
list_del_init(&ce->signal_link);
add_retire(b, ce->timeline); add_retire(b, ce->timeline);
remove_signaling_context(b, ce);
} }
} }
} }
...@@ -279,6 +300,9 @@ void intel_breadcrumbs_park(struct intel_breadcrumbs *b) ...@@ -279,6 +300,9 @@ void intel_breadcrumbs_park(struct intel_breadcrumbs *b)
spin_lock_irqsave(&b->irq_lock, flags); spin_lock_irqsave(&b->irq_lock, flags);
__intel_breadcrumbs_disarm_irq(b); __intel_breadcrumbs_disarm_irq(b);
spin_unlock_irqrestore(&b->irq_lock, flags); spin_unlock_irqrestore(&b->irq_lock, flags);
if (!list_empty(&b->signalers))
irq_work_queue(&b->irq_work);
} }
void intel_breadcrumbs_free(struct intel_breadcrumbs *b) void intel_breadcrumbs_free(struct intel_breadcrumbs *b)
...@@ -295,6 +319,8 @@ static void insert_breadcrumb(struct i915_request *rq, ...@@ -295,6 +319,8 @@ static void insert_breadcrumb(struct i915_request *rq,
if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags)) if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
return; return;
i915_request_get(rq);
/* /*
* If the request is already completed, we can transfer it * If the request is already completed, we can transfer it
* straight onto a signaled list, and queue the irq worker for * straight onto a signaled list, and queue the irq worker for
...@@ -306,32 +332,33 @@ static void insert_breadcrumb(struct i915_request *rq, ...@@ -306,32 +332,33 @@ static void insert_breadcrumb(struct i915_request *rq,
return; return;
} }
__intel_breadcrumbs_arm_irq(b); if (list_empty(&ce->signals)) {
add_signaling_context(b, ce);
/* pos = &ce->signals;
* We keep the seqno in retirement order, so we can break } else {
* inside intel_engine_signal_breadcrumbs as soon as we've /*
* passed the last completed request (or seen a request that * We keep the seqno in retirement order, so we can break
* hasn't event started). We could walk the timeline->requests, * inside intel_engine_signal_breadcrumbs as soon as we've
* but keeping a separate signalers_list has the advantage of * passed the last completed request (or seen a request that
* hopefully being much smaller than the full list and so * hasn't event started). We could walk the timeline->requests,
* provides faster iteration and detection when there are no * but keeping a separate signalers_list has the advantage of
* more interrupts required for this context. * hopefully being much smaller than the full list and so
* * provides faster iteration and detection when there are no
* We typically expect to add new signalers in order, so we * more interrupts required for this context.
* start looking for our insertion point from the tail of *
* the list. * We typically expect to add new signalers in order, so we
*/ * start looking for our insertion point from the tail of
list_for_each_prev(pos, &ce->signals) { * the list.
struct i915_request *it = */
list_entry(pos, typeof(*it), signal_link); 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)) if (i915_seqno_passed(rq->fence.seqno, it->fence.seqno))
break; break;
}
} }
list_add(&rq->signal_link, pos); 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)); GEM_BUG_ON(!check_signal_order(ce, rq));
set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags); set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
...@@ -412,23 +439,19 @@ void i915_request_cancel_breadcrumb(struct i915_request *rq) ...@@ -412,23 +439,19 @@ void i915_request_cancel_breadcrumb(struct i915_request *rq)
list_del(&rq->signal_link); list_del(&rq->signal_link);
if (list_empty(&ce->signals)) 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); clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
i915_request_put(rq);
} }
spin_unlock(&b->irq_lock); spin_unlock(&b->irq_lock);
} }
void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine, static void print_signals(struct intel_breadcrumbs *b, struct drm_printer *p)
struct drm_printer *p)
{ {
struct intel_breadcrumbs *b = engine->breadcrumbs;
struct intel_context *ce; struct intel_context *ce;
struct i915_request *rq; struct i915_request *rq;
if (!b || list_empty(&b->signalers))
return;
drm_printf(p, "Signals:\n"); drm_printf(p, "Signals:\n");
spin_lock_irq(&b->irq_lock); spin_lock_irq(&b->irq_lock);
...@@ -444,3 +467,17 @@ void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine, ...@@ -444,3 +467,17 @@ void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
} }
spin_unlock_irq(&b->irq_lock); spin_unlock_irq(&b->irq_lock);
} }
void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
struct drm_printer *p)
{
struct intel_breadcrumbs *b;
b = engine->breadcrumbs;
if (!b)
return;
drm_printf(p, "IRQ: %s\n", enableddisabled(b->irq_armed));
if (!list_empty(&b->signalers))
print_signals(b, p);
}
...@@ -296,13 +296,12 @@ bool i915_request_retire(struct i915_request *rq) ...@@ -296,13 +296,12 @@ bool i915_request_retire(struct i915_request *rq)
*/ */
remove_from_engine(rq); remove_from_engine(rq);
spin_lock_irq(&rq->lock);
i915_request_mark_complete(rq); i915_request_mark_complete(rq);
if (!i915_request_signaled(rq)) if (!i915_request_signaled(rq)) {
spin_lock_irq(&rq->lock);
dma_fence_signal_locked(&rq->fence); dma_fence_signal_locked(&rq->fence);
if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &rq->fence.flags)) spin_unlock_irq(&rq->lock);
i915_request_cancel_breadcrumb(rq); }
spin_unlock_irq(&rq->lock);
if (i915_request_has_waitboost(rq)) { if (i915_request_has_waitboost(rq)) {
GEM_BUG_ON(!atomic_read(&rq->engine->gt->rps.num_waiters)); GEM_BUG_ON(!atomic_read(&rq->engine->gt->rps.num_waiters));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment