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@xxxxxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
