[Intel-gfx] [PATCH] drm/i915/guc: Remove preemption support for current fw
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Sat Feb 16 01:19:00 UTC 2019
On 2/15/19 2:48 PM, Chris Wilson wrote:
> Preemption via GuC submission quickly degrades into confused firmware
> and misordered requests. It is not being supported with no intention of
> being fixed in its current incarnation, so remove the unstable code. By
> removing the workqueue from the GuC submission path, we can now do
> direct engine-resets and direct submission from atomic context.
>
Nothing against ripping out the current code, but the new code will
still require an H2G, so the workqueue is most likely going to come
back. I guess it's still fine to remove it now as long as we don't build
on the fact that it isn't there for now.
Tomasz is looking at the new logic so he's probably the best person to
comment/ack this.
Daniele
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108622
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Jon Ewins <jon.ewins at intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 5 -
> drivers/gpu/drm/i915/intel_guc.c | 31 ---
> drivers/gpu/drm/i915/intel_guc.h | 9 -
> drivers/gpu/drm/i915/intel_guc_submission.c | 203 +-----------------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 -
> drivers/gpu/drm/i915/selftests/intel_guc.c | 17 +-
> .../gpu/drm/i915/selftests/intel_hangcheck.c | 3 -
> 7 files changed, 5 insertions(+), 265 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 2aeea977283f..636e28367980 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2233,11 +2233,6 @@ static int i915_guc_info(struct seq_file *m, void *data)
>
> seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client);
> i915_guc_client_info(m, dev_priv, guc->execbuf_client);
> - if (guc->preempt_client) {
> - seq_printf(m, "\nGuC preempt client @ %p:\n",
> - guc->preempt_client);
> - i915_guc_client_info(m, dev_priv, guc->preempt_client);
> - }
>
> /* Add more as required ... */
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 8660af3fd755..8fa2de84d82e 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -76,8 +76,6 @@ void intel_guc_init_early(struct intel_guc *guc)
>
> static int guc_init_wq(struct intel_guc *guc)
> {
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -
> /*
> * GuC log buffer flush work item has to do register access to
> * send the ack to GuC and this work item, if not synced before
> @@ -97,31 +95,6 @@ static int guc_init_wq(struct intel_guc *guc)
> return -ENOMEM;
> }
>
> - /*
> - * Even though both sending GuC action, and adding a new workitem to
> - * GuC workqueue are serialized (each with its own locking), since
> - * we're using mutliple engines, it's possible that we're going to
> - * issue a preempt request with two (or more - each for different
> - * engine) workitems in GuC queue. In this situation, GuC may submit
> - * all of them, which will make us very confused.
> - * Our preemption contexts may even already be complete - before we
> - * even had the chance to sent the preempt action to GuC!. Rather
> - * than introducing yet another lock, we can just use ordered workqueue
> - * to make sure we're always sending a single preemption request with a
> - * single workitem.
> - */
> - if (HAS_LOGICAL_RING_PREEMPTION(dev_priv) &&
> - USES_GUC_SUBMISSION(dev_priv)) {
> - guc->preempt_wq = alloc_ordered_workqueue("i915-guc_preempt",
> - WQ_HIGHPRI);
> - if (!guc->preempt_wq) {
> - destroy_workqueue(guc->log.relay.flush_wq);
> - DRM_ERROR("Couldn't allocate workqueue for GuC "
> - "preemption\n");
> - return -ENOMEM;
> - }
> - }
> -
> return 0;
> }
>
> @@ -129,10 +102,6 @@ static void guc_fini_wq(struct intel_guc *guc)
> {
> struct workqueue_struct *wq;
>
> - wq = fetch_and_zero(&guc->preempt_wq);
> - if (wq)
> - destroy_workqueue(wq);
> -
> wq = fetch_and_zero(&guc->log.relay.flush_wq);
> if (wq)
> destroy_workqueue(wq);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 744220296653..b8c4615715c0 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -34,11 +34,6 @@
> #include "intel_uc_fw.h"
> #include "i915_vma.h"
>
> -struct guc_preempt_work {
> - struct work_struct work;
> - struct intel_engine_cs *engine;
> -};
> -
> /*
> * Top level structure of GuC. It handles firmware loading and manages client
> * pool and doorbells. intel_guc owns a intel_guc_client to replace the legacy
> @@ -65,10 +60,6 @@ struct intel_guc {
> void *shared_data_vaddr;
>
> struct intel_guc_client *execbuf_client;
> - struct intel_guc_client *preempt_client;
> -
> - struct guc_preempt_work preempt_work[I915_NUM_ENGINES];
> - struct workqueue_struct *preempt_wq;
>
> DECLARE_BITMAP(doorbell_bitmap, GUC_NUM_DOORBELLS);
> /* Cyclic counter mod pagesize */
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 8bc8aa54aa35..d5677dd8cc15 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -81,12 +81,6 @@
> *
> */
>
> -static inline u32 intel_hws_preempt_done_address(struct intel_engine_cs *engine)
> -{
> - return (i915_ggtt_offset(engine->status_page.vma) +
> - I915_GEM_HWS_PREEMPT_ADDR);
> -}
> -
> static inline struct i915_priolist *to_priolist(struct rb_node *rb)
> {
> return rb_entry(rb, struct i915_priolist, node);
> @@ -558,125 +552,6 @@ static void flush_ggtt_writes(struct i915_vma *vma)
> POSTING_READ_FW(GUC_STATUS);
> }
>
> -static void inject_preempt_context(struct work_struct *work)
> -{
> - struct guc_preempt_work *preempt_work =
> - container_of(work, typeof(*preempt_work), work);
> - struct intel_engine_cs *engine = preempt_work->engine;
> - struct intel_guc *guc = container_of(preempt_work, typeof(*guc),
> - preempt_work[engine->id]);
> - struct intel_guc_client *client = guc->preempt_client;
> - struct guc_stage_desc *stage_desc = __get_stage_desc(client);
> - struct intel_context *ce = to_intel_context(client->owner, engine);
> - u32 data[7];
> -
> - if (!ce->ring->emit) { /* recreate upon load/resume */
> - u32 addr = intel_hws_preempt_done_address(engine);
> - u32 *cs;
> -
> - cs = ce->ring->vaddr;
> - if (engine->id == RCS) {
> - cs = gen8_emit_ggtt_write_rcs(cs,
> - GUC_PREEMPT_FINISHED,
> - addr,
> - PIPE_CONTROL_CS_STALL);
> - } else {
> - cs = gen8_emit_ggtt_write(cs,
> - GUC_PREEMPT_FINISHED,
> - addr);
> - *cs++ = MI_NOOP;
> - *cs++ = MI_NOOP;
> - }
> - *cs++ = MI_USER_INTERRUPT;
> - *cs++ = MI_NOOP;
> -
> - ce->ring->emit = GUC_PREEMPT_BREADCRUMB_BYTES;
> - GEM_BUG_ON((void *)cs - ce->ring->vaddr != ce->ring->emit);
> -
> - flush_ggtt_writes(ce->ring->vma);
> - }
> -
> - spin_lock_irq(&client->wq_lock);
> - guc_wq_item_append(client, engine->guc_id, lower_32_bits(ce->lrc_desc),
> - GUC_PREEMPT_BREADCRUMB_BYTES / sizeof(u64), 0);
> - spin_unlock_irq(&client->wq_lock);
> -
> - /*
> - * If GuC firmware performs an engine reset while that engine had
> - * a preemption pending, it will set the terminated attribute bit
> - * on our preemption stage descriptor. GuC firmware retains all
> - * pending work items for a high-priority GuC client, unlike the
> - * normal-priority GuC client where work items are dropped. It
> - * wants to make sure the preempt-to-idle work doesn't run when
> - * scheduling resumes, and uses this bit to inform its scheduler
> - * and presumably us as well. Our job is to clear it for the next
> - * preemption after reset, otherwise that and future preemptions
> - * will never complete. We'll just clear it every time.
> - */
> - stage_desc->attribute &= ~GUC_STAGE_DESC_ATTR_TERMINATED;
> -
> - data[0] = INTEL_GUC_ACTION_REQUEST_PREEMPTION;
> - data[1] = client->stage_id;
> - data[2] = INTEL_GUC_PREEMPT_OPTION_DROP_WORK_Q |
> - INTEL_GUC_PREEMPT_OPTION_DROP_SUBMIT_Q;
> - data[3] = engine->guc_id;
> - data[4] = guc->execbuf_client->priority;
> - data[5] = guc->execbuf_client->stage_id;
> - data[6] = intel_guc_ggtt_offset(guc, guc->shared_data);
> -
> - if (WARN_ON(intel_guc_send(guc, data, ARRAY_SIZE(data)))) {
> - execlists_clear_active(&engine->execlists,
> - EXECLISTS_ACTIVE_PREEMPT);
> - tasklet_schedule(&engine->execlists.tasklet);
> - }
> -
> - (void)I915_SELFTEST_ONLY(engine->execlists.preempt_hang.count++);
> -}
> -
> -/*
> - * We're using user interrupt and HWSP value to mark that preemption has
> - * finished and GPU is idle. Normally, we could unwind and continue similar to
> - * execlists submission path. Unfortunately, with GuC we also need to wait for
> - * it to finish its own postprocessing, before attempting to submit. Otherwise
> - * GuC may silently ignore our submissions, and thus we risk losing request at
> - * best, executing out-of-order and causing kernel panic at worst.
> - */
> -#define GUC_PREEMPT_POSTPROCESS_DELAY_MS 10
> -static void wait_for_guc_preempt_report(struct intel_engine_cs *engine)
> -{
> - struct intel_guc *guc = &engine->i915->guc;
> - struct guc_shared_ctx_data *data = guc->shared_data_vaddr;
> - struct guc_ctx_report *report =
> - &data->preempt_ctx_report[engine->guc_id];
> -
> - WARN_ON(wait_for_atomic(report->report_return_status ==
> - INTEL_GUC_REPORT_STATUS_COMPLETE,
> - GUC_PREEMPT_POSTPROCESS_DELAY_MS));
> - /*
> - * GuC is expecting that we're also going to clear the affected context
> - * counter, let's also reset the return status to not depend on GuC
> - * resetting it after recieving another preempt action
> - */
> - report->affected_count = 0;
> - report->report_return_status = INTEL_GUC_REPORT_STATUS_UNKNOWN;
> -}
> -
> -static void complete_preempt_context(struct intel_engine_cs *engine)
> -{
> - struct intel_engine_execlists *execlists = &engine->execlists;
> -
> - GEM_BUG_ON(!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT));
> -
> - if (inject_preempt_hang(execlists))
> - return;
> -
> - execlists_cancel_port_requests(execlists);
> - execlists_unwind_incomplete_requests(execlists);
> -
> - wait_for_guc_preempt_report(engine);
> - intel_write_status_page(engine, I915_GEM_HWS_PREEMPT, 0);
> -}
> -
> /**
> * guc_submit() - Submit commands through GuC
> * @engine: engine associated with the commands
> @@ -736,20 +611,6 @@ static bool __guc_dequeue(struct intel_engine_cs *engine)
> lockdep_assert_held(&engine->timeline.lock);
>
> if (port_isset(port)) {
> - if (intel_engine_has_preemption(engine)) {
> - struct guc_preempt_work *preempt_work =
> - &engine->i915->guc.preempt_work[engine->id];
> - int prio = execlists->queue_priority_hint;
> -
> - if (__execlists_need_preempt(prio, port_prio(port))) {
> - execlists_set_active(execlists,
> - EXECLISTS_ACTIVE_PREEMPT);
> - queue_work(engine->i915->guc.preempt_wq,
> - &preempt_work->work);
> - return false;
> - }
> - }
> -
> port++;
> if (port_isset(port))
> return false;
> @@ -832,13 +693,7 @@ static void guc_submission_tasklet(unsigned long data)
> }
> }
>
> - if (execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT) &&
> - intel_read_status_page(engine, I915_GEM_HWS_PREEMPT) ==
> - GUC_PREEMPT_FINISHED)
> - complete_preempt_context(engine);
> -
> - if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_PREEMPT))
> - guc_dequeue(engine);
> + guc_dequeue(engine);
>
> spin_unlock_irqrestore(&engine->timeline.lock, flags);
> }
> @@ -859,16 +714,6 @@ static void guc_reset_prepare(struct intel_engine_cs *engine)
> * prevents the race.
> */
> __tasklet_disable_sync_once(&execlists->tasklet);
> -
> - /*
> - * We're using worker to queue preemption requests from the tasklet in
> - * GuC submission mode.
> - * Even though tasklet was disabled, we may still have a worker queued.
> - * Let's make sure that all workers scheduled before disabling the
> - * tasklet are completed before continuing with the reset.
> - */
> - if (engine->i915->guc.preempt_wq)
> - flush_workqueue(engine->i915->guc.preempt_wq);
> }
>
> /*
> @@ -1028,7 +873,6 @@ static int guc_clients_create(struct intel_guc *guc)
> struct intel_guc_client *client;
>
> GEM_BUG_ON(guc->execbuf_client);
> - GEM_BUG_ON(guc->preempt_client);
>
> client = guc_client_alloc(dev_priv,
> INTEL_INFO(dev_priv)->ring_mask,
> @@ -1040,20 +884,6 @@ static int guc_clients_create(struct intel_guc *guc)
> }
> guc->execbuf_client = client;
>
> - if (dev_priv->preempt_context) {
> - client = guc_client_alloc(dev_priv,
> - INTEL_INFO(dev_priv)->ring_mask,
> - GUC_CLIENT_PRIORITY_KMD_HIGH,
> - dev_priv->preempt_context);
> - if (IS_ERR(client)) {
> - DRM_ERROR("Failed to create GuC client for preemption!\n");
> - guc_client_free(guc->execbuf_client);
> - guc->execbuf_client = NULL;
> - return PTR_ERR(client);
> - }
> - guc->preempt_client = client;
> - }
> -
> return 0;
> }
>
> @@ -1061,10 +891,6 @@ static void guc_clients_destroy(struct intel_guc *guc)
> {
> struct intel_guc_client *client;
>
> - client = fetch_and_zero(&guc->preempt_client);
> - if (client)
> - guc_client_free(client);
> -
> client = fetch_and_zero(&guc->execbuf_client);
> if (client)
> guc_client_free(client);
> @@ -1113,22 +939,11 @@ static int guc_clients_enable(struct intel_guc *guc)
> if (ret)
> return ret;
>
> - if (guc->preempt_client) {
> - ret = __guc_client_enable(guc->preempt_client);
> - if (ret) {
> - __guc_client_disable(guc->execbuf_client);
> - return ret;
> - }
> - }
> -
> return 0;
> }
>
> static void guc_clients_disable(struct intel_guc *guc)
> {
> - if (guc->preempt_client)
> - __guc_client_disable(guc->preempt_client);
> -
> if (guc->execbuf_client)
> __guc_client_disable(guc->execbuf_client);
> }
> @@ -1139,9 +954,6 @@ static void guc_clients_disable(struct intel_guc *guc)
> */
> int intel_guc_submission_init(struct intel_guc *guc)
> {
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> - struct intel_engine_cs *engine;
> - enum intel_engine_id id;
> int ret;
>
> if (guc->stage_desc_pool)
> @@ -1161,11 +973,6 @@ int intel_guc_submission_init(struct intel_guc *guc)
> if (ret)
> goto err_pool;
>
> - for_each_engine(engine, dev_priv, id) {
> - guc->preempt_work[id].engine = engine;
> - INIT_WORK(&guc->preempt_work[id].work, inject_preempt_context);
> - }
> -
> return 0;
>
> err_pool:
> @@ -1175,13 +982,6 @@ int intel_guc_submission_init(struct intel_guc *guc)
>
> void intel_guc_submission_fini(struct intel_guc *guc)
> {
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> - struct intel_engine_cs *engine;
> - enum intel_engine_id id;
> -
> - for_each_engine(engine, dev_priv, id)
> - cancel_work_sync(&guc->preempt_work[id].work);
> -
> guc_clients_destroy(guc);
> WARN_ON(!guc_verify_doorbells(guc));
>
> @@ -1292,6 +1092,7 @@ static void guc_set_default_submission(struct intel_engine_cs *engine)
> engine->reset.prepare = guc_reset_prepare;
>
> engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
> + engine->flags &= ~I915_ENGINE_HAS_PREEMPTION;
> }
>
> int intel_guc_submission_enable(struct intel_guc *guc)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 710ffb221775..9bd3f8aad6ac 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -724,8 +724,6 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
> */
> #define I915_GEM_HWS_INDEX 0x30
> #define I915_GEM_HWS_INDEX_ADDR (I915_GEM_HWS_INDEX * sizeof(u32))
> -#define I915_GEM_HWS_PREEMPT 0x32
> -#define I915_GEM_HWS_PREEMPT_ADDR (I915_GEM_HWS_PREEMPT * sizeof(u32))
> #define I915_GEM_HWS_SEQNO 0x40
> #define I915_GEM_HWS_SEQNO_ADDR (I915_GEM_HWS_SEQNO * sizeof(u32))
> #define I915_GEM_HWS_SCRATCH 0x80
> diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c
> index c5e0a0e98fcb..28ce5ce01043 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_guc.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
> @@ -162,7 +162,7 @@ static int igt_guc_clients(void *args)
> */
> guc_clients_disable(guc);
> guc_clients_destroy(guc);
> - if (guc->execbuf_client || guc->preempt_client) {
> + if (guc->execbuf_client) {
> pr_err("guc_clients_destroy lied!\n");
> err = -EINVAL;
> goto unlock;
> @@ -182,18 +182,8 @@ static int igt_guc_clients(void *args)
> goto out;
> }
>
> - if (guc->preempt_client) {
> - err = validate_client(guc->preempt_client,
> - GUC_CLIENT_PRIORITY_KMD_HIGH, true);
> - if (err) {
> - pr_err("preempt client validation failed\n");
> - goto out;
> - }
> - }
> -
> /* each client should now have reserved a doorbell */
> - if (!has_doorbell(guc->execbuf_client) ||
> - (guc->preempt_client && !has_doorbell(guc->preempt_client))) {
> + if (!has_doorbell(guc->execbuf_client)) {
> pr_err("guc_clients_create didn't reserve doorbells\n");
> err = -EINVAL;
> goto out;
> @@ -203,8 +193,7 @@ static int igt_guc_clients(void *args)
> guc_clients_enable(guc);
>
> /* each client should now have received a doorbell */
> - if (!client_doorbell_in_sync(guc->execbuf_client) ||
> - !client_doorbell_in_sync(guc->preempt_client)) {
> + if (!client_doorbell_in_sync(guc->execbuf_client)) {
> pr_err("failed to initialize the doorbells\n");
> err = -EINVAL;
> goto out;
> diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> index 74e743b101d9..28af146f28f3 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> @@ -1580,9 +1580,6 @@ static int igt_atomic_reset(void *arg)
>
> /* Check that the resets are usable from atomic context */
>
> - if (USES_GUC_SUBMISSION(i915))
> - return 0; /* guc is dead; long live the guc */
> -
> igt_global_reset_lock(i915);
> mutex_lock(&i915->drm.struct_mutex);
> wakeref = intel_runtime_pm_get(i915);
>
More information about the Intel-gfx
mailing list