On 09/05/2018 15:27, Chris Wilson wrote:
In the next few patches, we want to abuse tasklet to avoid ksoftirqd
latency along critical paths. To make that abuse easily to swallow,
first coat the tasklet in a little syntactic sugar.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem.c | 4 +-
drivers/gpu/drm/i915/i915_irq.c | 2 +-
drivers/gpu/drm/i915/i915_tasklet.h | 78 +++++++++++++++++++++
drivers/gpu/drm/i915/intel_engine_cs.c | 11 ++-
drivers/gpu/drm/i915/intel_guc_submission.c | 8 +--
drivers/gpu/drm/i915/intel_lrc.c | 18 ++---
drivers/gpu/drm/i915/intel_ringbuffer.h | 3 +-
7 files changed, 102 insertions(+), 22 deletions(-)
create mode 100644 drivers/gpu/drm/i915/i915_tasklet.h
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 63c96c2b8fcf..59e04387a27c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3036,7 +3036,7 @@ i915_gem_reset_prepare_engine(struct intel_engine_cs *engine)
* Turning off the execlists->tasklet until the reset is over
* prevents the race.
*/
- tasklet_disable(&engine->execlists.tasklet);
+ i915_tasklet_lock(&engine->execlists.tasklet);
/*
* We're using worker to queue preemption requests from the tasklet in
@@ -3256,7 +3256,7 @@ void i915_gem_reset(struct drm_i915_private *dev_priv,
void i915_gem_reset_finish_engine(struct intel_engine_cs *engine)
{
- tasklet_enable(&engine->execlists.tasklet);
+ i915_tasklet_unlock(&engine->execlists.tasklet);
kthread_unpark(engine->breadcrumbs.signaler);
intel_uncore_forcewake_put(engine->i915, FORCEWAKE_ALL);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f9bc3aaa90d0..f8aff5a5aa83 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1477,7 +1477,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
}
if (tasklet)
- tasklet_hi_schedule(&execlists->tasklet);
+ i915_tasklet_schedule(&execlists->tasklet);
}
static void gen8_gt_irq_ack(struct drm_i915_private *i915,
diff --git a/drivers/gpu/drm/i915/i915_tasklet.h b/drivers/gpu/drm/i915/i915_tasklet.h
new file mode 100644
index 000000000000..42b002b88edb
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_tasklet.h
@@ -0,0 +1,78 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright © 2018 Intel Corporation
+ */
+
+#ifndef _I915_TASKLET_H_
+#define _I915_TASKLET_H_
+
+#include <linux/atomic.h>
+#include <linux/interrupt.h>
+
+/**
+ * struct i915_tasklet - wrapper around tasklet_struct
+ *
+ * We want to abuse tasklets slightly, such as calling them directly. In some
+ * cases, this requires some auxiliary tracking so subclass the tasklet_struct
+ * so that we have a central place and helpers.
+ */
+struct i915_tasklet {
+ struct tasklet_struct base;
+};
+
+static inline void i915_tasklet_init(struct i915_tasklet *t,
+ void (*func)(unsigned long),
+ unsigned long data)
+{
+ tasklet_init(&t->base, func, data);
+}
+
+static inline bool i915_tasklet_is_scheduled(const struct i915_tasklet *t)
+{
+ return test_bit(TASKLET_STATE_SCHED, &t->base.state);
+}
+
+static inline bool i915_tasklet_is_running(const struct i915_tasklet *t)
+{
+ return test_bit(TASKLET_STATE_RUN, &t->base.state);
+}
+
+static inline bool i915_tasklet_is_enabled(const struct i915_tasklet *t)
+{
+ return likely(!atomic_read(&t->base.count));
+}
+
+static inline void i915_tasklet_schedule(struct i915_tasklet *t)
+{
+ tasklet_hi_schedule(&t->base);
+}
+
+static inline void i915_tasklet_flush(struct i915_tasklet *t)
+{
+ tasklet_kill(&t->base);
+}
+
+static inline void i915_tasklet_lock(struct i915_tasklet *t)
+{
+ tasklet_disable(&t->base);
+}
+
+static inline void i915_tasklet_unlock(struct i915_tasklet *t)
+{
+ tasklet_enable(&t->base);
+}
I agree keeping the original naming for kill/enable/disable would be better.
+
+static inline void i915_tasklet_set_func(struct i915_tasklet *t,
+ void (*func)(unsigned long data),
+ unsigned long data)
+{
+ i915_tasklet_lock(t);
+
+ t->base.func = func;
+ t->base.data = data;
+
+ i915_tasklet_unlock(t);
I kind of remember something about issues we had with placing
tasklet_disable placed in some contexts. Hm.. if only I could recall the
details. It's probably fine. I cannot come up with ideas on how to
protect against that. GEM_BUG_ON(in_irq() || in_softirq())? Too far
fetched probably.
+}
+
+#endif /* _I915_TASKLET_H_ */
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 70325e0824e3..3c8a0c3245f3 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1032,7 +1032,7 @@ void intel_engines_park(struct drm_i915_private *i915)
for_each_engine(engine, i915, id) {
/* Flush the residual irq tasklets first. */
intel_engine_disarm_breadcrumbs(engine);
- tasklet_kill(&engine->execlists.tasklet);
+ i915_tasklet_flush(&engine->execlists.tasklet);
/*
* We are committed now to parking the engines, make sure there
@@ -1249,9 +1249,8 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine,
intel_read_status_page(engine, intel_hws_csb_write_index(engine->i915)),
yesno(test_bit(ENGINE_IRQ_EXECLIST,
&engine->irq_posted)),
- yesno(test_bit(TASKLET_STATE_SCHED,
- &engine->execlists.tasklet.state)),
- enableddisabled(!atomic_read(&engine->execlists.tasklet.count)));
+ yesno(i915_tasklet_is_scheduled(&engine->execlists.tasklet)),
+ enableddisabled(i915_tasklet_is_enabled(&engine->execlists.tasklet)));
if (read >= GEN8_CSB_ENTRIES)
read = 0;
if (write >= GEN8_CSB_ENTRIES)
@@ -1479,7 +1478,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
if (!intel_engine_supports_stats(engine))
return -ENODEV;
- tasklet_disable(&execlists->tasklet);
+ i915_tasklet_lock(&execlists->tasklet);
write_seqlock_irqsave(&engine->stats.lock, flags);
if (unlikely(engine->stats.enabled == ~0)) {
@@ -1505,7 +1504,7 @@ int intel_enable_engine_stats(struct intel_engine_cs *engine)
unlock:
write_sequnlock_irqrestore(&engine->stats.lock, flags);
- tasklet_enable(&execlists->tasklet);
+ i915_tasklet_unlock(&execlists->tasklet);
return err;
}
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 2feb65096966..a7afc976c3b9 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -591,7 +591,7 @@ static void inject_preempt_context(struct work_struct *work)
if (WARN_ON(intel_guc_send(guc, data, ARRAY_SIZE(data)))) {
execlists_clear_active(&engine->execlists,
EXECLISTS_ACTIVE_PREEMPT);
- tasklet_schedule(&engine->execlists.tasklet);
+ i915_tasklet_schedule(&engine->execlists.tasklet);
}
}
@@ -1263,10 +1263,10 @@ int intel_guc_submission_enable(struct intel_guc *guc)
guc_interrupts_capture(dev_priv);
for_each_engine(engine, dev_priv, id) {
- struct intel_engine_execlists * const execlists =
- &engine->execlists;
+ i915_tasklet_set_func(&engine->execlists.tasklet,
+ guc_submission_tasklet,
+ (unsigned long)engine);
- execlists->tasklet.func = guc_submission_tasklet;
engine->park = guc_submission_park;
engine->unpark = guc_submission_unpark;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 7f98dda3c929..539fa03d7600 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1165,7 +1165,7 @@ static void queue_request(struct intel_engine_cs *engine,
static void __submit_queue(struct intel_engine_cs *engine, int prio)
{
engine->execlists.queue_priority = prio;
- tasklet_hi_schedule(&engine->execlists.tasklet);
+ i915_tasklet_schedule(&engine->execlists.tasklet);
}
static void submit_queue(struct intel_engine_cs *engine, int prio)
@@ -1778,7 +1778,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
/* After a GPU reset, we may have requests to replay */
if (execlists->first)
- tasklet_schedule(&execlists->tasklet);
+ i915_tasklet_schedule(&execlists->tasklet);
return 0;
}
@@ -2182,9 +2182,8 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
* Tasklet cannot be active at this point due intel_mark_active/idle
* so this is just for documentation.
*/
- if (WARN_ON(test_bit(TASKLET_STATE_SCHED,
- &engine->execlists.tasklet.state)))
- tasklet_kill(&engine->execlists.tasklet);
+ if (WARN_ON(i915_tasklet_is_scheduled(&engine->execlists.tasklet)))
+ i915_tasklet_flush(&engine->execlists.tasklet);
dev_priv = engine->i915;
@@ -2209,7 +2208,10 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
engine->submit_request = execlists_submit_request;
engine->cancel_requests = execlists_cancel_requests;
engine->schedule = execlists_schedule;
- engine->execlists.tasklet.func = execlists_submission_tasklet;
+
+ i915_tasklet_set_func(&engine->execlists.tasklet,
+ execlists_submission_tasklet,
+ (unsigned long)engine);
Or eliminate the above dilemma by removing the data parameter from
i915_tasklet_set_func since it looks it is not needed at the moment?
Regards,
Tvrtko
engine->park = NULL;
engine->unpark = NULL;
@@ -2303,8 +2305,8 @@ logical_ring_setup(struct intel_engine_cs *engine)
engine->execlists.fw_domains = fw_domains;
- tasklet_init(&engine->execlists.tasklet,
- execlists_submission_tasklet, (unsigned long)engine);
+ i915_tasklet_init(&engine->execlists.tasklet,
+ execlists_submission_tasklet, (unsigned long)engine);
logical_ring_default_vfuncs(engine);
logical_ring_default_irqs(engine);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 010750e8ee44..f6ba354faf89 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -11,6 +11,7 @@
#include "i915_pmu.h"
#include "i915_request.h"
#include "i915_selftest.h"
+#include "i915_tasklet.h"
#include "i915_timeline.h"
#include "intel_gpu_commands.h"
@@ -202,7 +203,7 @@ struct intel_engine_execlists {
/**
* @tasklet: softirq tasklet for bottom handler
*/
- struct tasklet_struct tasklet;
+ struct i915_tasklet tasklet;
/**
* @default_priolist: priority list for I915_PRIORITY_NORMAL
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
