* [PATCH 1/3] drm/i915/guc: Remove preemption support for current fw
@ 2019-07-02 20:09 Daniele Ceraolo Spurio
2019-07-02 20:09 ` [PATCH 2/3] drm/i915/guc: simplify guc client Daniele Ceraolo Spurio
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Daniele Ceraolo Spurio @ 2019-07-02 20:09 UTC (permalink / raw)
To: intel-gfx
From: Chris Wilson <chris@chris-wilson.co.uk>
Preemption via GuC submission is not being supported with its current
legacy incarnation. The current FW does support a similar pre-emption
flow via H2G, but it is class-based instead of being instance-based,
which doesn't fit well with the i915 tracking. To fix this, the
firmware is being updated to better support our needs with a new flow,
so we can safely remove the old code.
v2 (Daniele): resurrect & rebase, reword commit message, remove
preempt_context as well
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 17 --
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 13 --
drivers/gpu/drm/i915/gt/intel_engine_types.h | 1 -
drivers/gpu/drm/i915/gt/intel_gt_pm.c | 4 -
drivers/gpu/drm/i915/i915_debugfs.c | 5 -
drivers/gpu/drm/i915/i915_drv.h | 2 -
drivers/gpu/drm/i915/intel_guc.c | 31 ---
drivers/gpu/drm/i915/intel_guc.h | 9 -
drivers/gpu/drm/i915/intel_guc_submission.c | 220 +------------------
drivers/gpu/drm/i915/selftests/intel_guc.c | 31 +--
10 files changed, 14 insertions(+), 319 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 8a9787cf0cd0..9c695910bc43 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -644,18 +644,12 @@ static void init_contexts(struct drm_i915_private *i915)
init_llist_head(&i915->contexts.free_list);
}
-static bool needs_preempt_context(struct drm_i915_private *i915)
-{
- return USES_GUC_SUBMISSION(i915);
-}
-
int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
{
struct i915_gem_context *ctx;
/* Reassure ourselves we are only called once */
GEM_BUG_ON(dev_priv->kernel_context);
- GEM_BUG_ON(dev_priv->preempt_context);
intel_engine_init_ctx_wa(dev_priv->engine[RCS0]);
init_contexts(dev_priv);
@@ -677,15 +671,6 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
GEM_BUG_ON(!atomic_read(&ctx->hw_id_pin_count));
dev_priv->kernel_context = ctx;
- /* highest priority; preempting task */
- if (needs_preempt_context(dev_priv)) {
- ctx = i915_gem_context_create_kernel(dev_priv, INT_MAX);
- if (!IS_ERR(ctx))
- dev_priv->preempt_context = ctx;
- else
- DRM_ERROR("Failed to create preempt context; disabling preemption\n");
- }
-
DRM_DEBUG_DRIVER("%s context support initialized\n",
DRIVER_CAPS(dev_priv)->has_logical_contexts ?
"logical" : "fake");
@@ -696,8 +681,6 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
{
lockdep_assert_held(&i915->drm.struct_mutex);
- if (i915->preempt_context)
- destroy_kernel_context(&i915->preempt_context);
destroy_kernel_context(&i915->kernel_context);
/* Must free all deferred contexts (via flush_workqueue) first */
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index d1508f0b4c84..55b11409c1f0 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -842,15 +842,6 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
if (ret)
return ret;
- /*
- * Similarly the preempt context must always be available so that
- * we can interrupt the engine at any time. However, as preemption
- * is optional, we allow it to fail.
- */
- if (i915->preempt_context)
- pin_context(i915->preempt_context, engine,
- &engine->preempt_context);
-
ret = measure_breadcrumb_dw(engine);
if (ret < 0)
goto err_unpin;
@@ -862,8 +853,6 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
return 0;
err_unpin:
- if (engine->preempt_context)
- intel_context_unpin(engine->preempt_context);
intel_context_unpin(engine->kernel_context);
return ret;
}
@@ -888,8 +877,6 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
if (engine->default_state)
i915_gem_object_put(engine->default_state);
- if (engine->preempt_context)
- intel_context_unpin(engine->preempt_context);
intel_context_unpin(engine->kernel_context);
GEM_BUG_ON(!llist_empty(&engine->barrier_tasks));
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 7e056114344e..8be63019d707 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -288,7 +288,6 @@ struct intel_engine_cs {
struct llist_head barrier_tasks;
struct intel_context *kernel_context; /* pinned */
- struct intel_context *preempt_context; /* pinned; optional */
intel_engine_mask_t saturated; /* submitting semaphores too late? */
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index 36ba80e6a0b7..da81b3a92d16 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -145,10 +145,6 @@ int intel_gt_resume(struct intel_gt *gt)
if (ce)
ce->ops->reset(ce);
- ce = engine->preempt_context;
- if (ce)
- ce->ops->reset(ce);
-
engine->serial++; /* kernel context lost */
err = engine->resume(engine);
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index eeecdad0e3ca..02eaa15d47c0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2017,11 +2017,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/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 02dd9f9f3a89..1cdd06539dc5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1378,8 +1378,6 @@ struct drm_i915_private {
struct intel_engine_cs *engine[I915_NUM_ENGINES];
/* Context used internally to idle the GPU and setup initial state */
struct i915_gem_context *kernel_context;
- /* Context only to be used for injecting preemption commands */
- struct i915_gem_context *preempt_context;
struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS + 1]
[MAX_ENGINE_INSTANCE + 1];
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index c40a6efdd33a..501b74f44374 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -101,8 +101,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
@@ -122,31 +120,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;
}
@@ -154,10 +127,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 d91c96679dbb..ec1038c1f50e 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -37,11 +37,6 @@
struct __guc_ads_blob;
-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
@@ -76,10 +71,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 12c22359fdac..8520bb224175 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -46,11 +46,10 @@ enum {
*
* GuC client:
* A intel_guc_client refers to a submission path through GuC. Currently, there
- * are two clients. One of them (the execbuf_client) is charged with all
- * submissions to the GuC, the other one (preempt_client) is responsible for
- * preempting the execbuf_client. This struct is the owner of a doorbell, a
- * process descriptor and a workqueue (all of them inside a single gem object
- * that contains all required pages for these elements).
+ * is only one client, which is charged with all submissions to the GuC. This
+ * struct is the owner of a doorbell, a process descriptor and a workqueue (all
+ * of them inside a single gem object that contains all required pages for these
+ * elements).
*
* GuC stage descriptor:
* During initialization, the driver allocates a static pool of 1024 such
@@ -88,12 +87,6 @@ enum {
*
*/
-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);
@@ -563,126 +556,6 @@ static void flush_ggtt_writes(struct i915_vma *vma)
intel_uncore_posting_read_fw(&i915->uncore, 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 = engine->preempt_context;
- 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->class == RENDER_CLASS) {
- 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,
- 0);
- *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)))) {
- intel_write_status_page(engine,
- I915_GEM_HWS_PREEMPT,
- GUC_PREEMPT_NONE);
- 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];
-
- if (wait_for_atomic(report->report_return_status ==
- INTEL_GUC_REPORT_STATUS_COMPLETE,
- GUC_PREEMPT_POSTPROCESS_DELAY_MS))
- DRM_ERROR("Timed out waiting for GuC preemption report\n");
- /*
- * 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;
-
- 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, GUC_PREEMPT_NONE);
-}
-
static void guc_submit(struct intel_engine_cs *engine,
struct i915_request **out,
struct i915_request **end)
@@ -742,21 +615,6 @@ static void __guc_dequeue(struct intel_engine_cs *engine)
lockdep_assert_held(&engine->active.lock);
if (last) {
- 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 (i915_scheduler_need_preempt(prio, rq_prio(last))) {
- intel_write_status_page(engine,
- I915_GEM_HWS_PREEMPT,
- GUC_PREEMPT_INPROGRESS);
- queue_work(engine->i915->guc.preempt_wq,
- &preempt_work->work);
- return;
- }
- }
-
if (*++first)
return;
@@ -820,12 +678,7 @@ static void guc_submission_tasklet(unsigned long data)
memmove(execlists->inflight, port, rem * sizeof(*port));
}
- if (intel_read_status_page(engine, I915_GEM_HWS_PREEMPT) ==
- GUC_PREEMPT_FINISHED)
- complete_preempt_context(engine);
-
- if (!intel_read_status_page(engine, I915_GEM_HWS_PREEMPT))
- __guc_dequeue(engine);
+ __guc_dequeue(engine);
spin_unlock_irqrestore(&engine->active.lock, flags);
}
@@ -846,16 +699,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);
}
static void guc_reset(struct intel_engine_cs *engine, bool stalled)
@@ -1112,7 +955,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)->engine_mask,
@@ -1124,20 +966,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)->engine_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;
}
@@ -1145,10 +973,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);
@@ -1191,28 +1015,11 @@ static void __guc_client_disable(struct intel_guc_client *client)
static int guc_clients_enable(struct intel_guc *guc)
{
- int ret;
-
- ret = __guc_client_enable(guc->execbuf_client);
- 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;
+ return __guc_client_enable(guc->execbuf_client);
}
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);
}
@@ -1223,9 +1030,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)
@@ -1245,11 +1049,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:
@@ -1259,13 +1058,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));
diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c
index 6ca8584cd64c..1a1915e44f6b 100644
--- a/drivers/gpu/drm/i915/selftests/intel_guc.c
+++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
@@ -103,13 +103,10 @@ static int ring_doorbell_nop(struct intel_guc_client *client)
/*
* Basic client sanity check, handy to validate create_clients.
*/
-static int validate_client(struct intel_guc_client *client,
- int client_priority,
- bool is_preempt_client)
+static int validate_client(struct intel_guc_client *client, int client_priority)
{
struct drm_i915_private *dev_priv = guc_to_i915(client->guc);
- struct i915_gem_context *ctx_owner = is_preempt_client ?
- dev_priv->preempt_context : dev_priv->kernel_context;
+ struct i915_gem_context *ctx_owner = dev_priv->kernel_context;
if (client->owner != ctx_owner ||
client->engines != INTEL_INFO(dev_priv)->engine_mask ||
@@ -163,7 +160,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;
@@ -177,24 +174,14 @@ static int igt_guc_clients(void *args)
GEM_BUG_ON(!guc->execbuf_client);
err = validate_client(guc->execbuf_client,
- GUC_CLIENT_PRIORITY_KMD_NORMAL, false);
+ GUC_CLIENT_PRIORITY_KMD_NORMAL);
if (err) {
pr_err("execbug client validation failed\n");
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))) {
+ /* the client should now have reserved a doorbell */
+ if (!has_doorbell(guc->execbuf_client)) {
pr_err("guc_clients_create didn't reserve doorbells\n");
err = -EINVAL;
goto out;
@@ -204,8 +191,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;
@@ -300,8 +286,7 @@ static int igt_guc_doorbells(void *arg)
goto out;
}
- err = validate_client(clients[i],
- i % GUC_CLIENT_PRIORITY_NUM, false);
+ err = validate_client(clients[i], i % GUC_CLIENT_PRIORITY_NUM);
if (err) {
pr_err("[%d] client_alloc sanity check failed!\n", i);
err = -EINVAL;
--
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] 10+ messages in thread
* [PATCH 2/3] drm/i915/guc: simplify guc client
2019-07-02 20:09 [PATCH 1/3] drm/i915/guc: Remove preemption support for current fw Daniele Ceraolo Spurio
@ 2019-07-02 20:09 ` Daniele Ceraolo Spurio
2019-07-09 0:53 ` Matthew Brost
2019-07-02 20:09 ` [PATCH 3/3] drm/i915/uc: replace uc init/fini misc Daniele Ceraolo Spurio
` (4 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Daniele Ceraolo Spurio @ 2019-07-02 20:09 UTC (permalink / raw)
To: intel-gfx
We originally added support, in some cases partial, for different modes
of operations via guc clients:
- proxy vs direct submission;
- variable engine mask per-client.
We only ever used one flow (all submissions via a single proxy), so the
other code paths haven't been exercised and are most likely
non-functional. The guc firmware interface is also in the process of
being updated to better fit the i915 flow and our client abstraction
will need to change accordingly (or possibly go away entirely), so these
old unused paths can be considered dead and removed.
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 3 +-
drivers/gpu/drm/i915/intel_guc_submission.c | 73 ++-------------------
drivers/gpu/drm/i915/intel_guc_submission.h | 2 -
drivers/gpu/drm/i915/selftests/intel_guc.c | 12 +---
4 files changed, 8 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 02eaa15d47c0..65ddb24a0f4b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2028,7 +2028,6 @@ static int i915_guc_stage_pool(struct seq_file *m, void *data)
struct drm_i915_private *dev_priv = node_to_i915(m->private);
const struct intel_guc *guc = &dev_priv->guc;
struct guc_stage_desc *desc = guc->stage_desc_pool_vaddr;
- struct intel_guc_client *client = guc->execbuf_client;
intel_engine_mask_t tmp;
int index;
@@ -2058,7 +2057,7 @@ static int i915_guc_stage_pool(struct seq_file *m, void *data)
desc->wq_addr, desc->wq_size);
seq_putc(m, '\n');
- for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
+ for_each_engine(engine, dev_priv, tmp) {
u32 guc_engine_id = engine->guc_id;
struct guc_execlist_context *lrc =
&desc->lrc[guc_engine_id];
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 8520bb224175..30692f8289bd 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -363,10 +363,7 @@ static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
static void guc_stage_desc_init(struct intel_guc_client *client)
{
struct intel_guc *guc = client->guc;
- struct i915_gem_context *ctx = client->owner;
- struct i915_gem_engines_iter it;
struct guc_stage_desc *desc;
- struct intel_context *ce;
u32 gfx_addr;
desc = __get_stage_desc(client);
@@ -380,55 +377,6 @@ static void guc_stage_desc_init(struct intel_guc_client *client)
desc->priority = client->priority;
desc->db_id = client->doorbell_id;
- for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
- struct guc_execlist_context *lrc;
-
- if (!(ce->engine->mask & client->engines))
- continue;
-
- /* TODO: We have a design issue to be solved here. Only when we
- * receive the first batch, we know which engine is used by the
- * user. But here GuC expects the lrc and ring to be pinned. It
- * is not an issue for default context, which is the only one
- * for now who owns a GuC client. But for future owner of GuC
- * client, need to make sure lrc is pinned prior to enter here.
- */
- if (!ce->state)
- break; /* XXX: continue? */
-
- /*
- * XXX: When this is a GUC_STAGE_DESC_ATTR_KERNEL client (proxy
- * submission or, in other words, not using a direct submission
- * model) the KMD's LRCA is not used for any work submission.
- * Instead, the GuC uses the LRCA of the user mode context (see
- * guc_add_request below).
- */
- lrc = &desc->lrc[ce->engine->guc_id];
- lrc->context_desc = lower_32_bits(ce->lrc_desc);
-
- /* The state page is after PPHWSP */
- lrc->ring_lrca = intel_guc_ggtt_offset(guc, ce->state) +
- LRC_STATE_PN * PAGE_SIZE;
-
- /* XXX: In direct submission, the GuC wants the HW context id
- * here. In proxy submission, it wants the stage id
- */
- lrc->context_id = (client->stage_id << GUC_ELC_CTXID_OFFSET) |
- (ce->engine->guc_id << GUC_ELC_ENGINE_OFFSET);
-
- lrc->ring_begin = intel_guc_ggtt_offset(guc, ce->ring->vma);
- lrc->ring_end = lrc->ring_begin + ce->ring->size - 1;
- lrc->ring_next_free_location = lrc->ring_begin;
- lrc->ring_current_tail_pointer_value = 0;
-
- desc->engines_used |= BIT(ce->engine->guc_id);
- }
- i915_gem_context_unlock_engines(ctx);
-
- DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
- client->engines, desc->engines_used);
- WARN_ON(desc->engines_used == 0);
-
/*
* The doorbell, process descriptor, and workqueue are all parts
* of the client object, which the GuC will reference via the GGTT
@@ -836,8 +784,7 @@ static bool guc_verify_doorbells(struct intel_guc *guc)
/**
* guc_client_alloc() - Allocate an intel_guc_client
- * @dev_priv: driver private data structure
- * @engines: The set of engines to enable for this client
+ * @guc: the intel_guc structure
* @priority: four levels priority _CRITICAL, _HIGH, _NORMAL and _LOW
* The kernel client to replace ExecList submission is created with
* NORMAL priority. Priority of a client for scheduler can be HIGH,
@@ -848,13 +795,9 @@ static bool guc_verify_doorbells(struct intel_guc *guc)
* Return: An intel_guc_client object if success, else NULL.
*/
static struct intel_guc_client *
-guc_client_alloc(struct drm_i915_private *dev_priv,
- u32 engines,
- u32 priority,
- struct i915_gem_context *ctx)
+guc_client_alloc(struct intel_guc *guc, u32 priority)
{
struct intel_guc_client *client;
- struct intel_guc *guc = &dev_priv->guc;
struct i915_vma *vma;
void *vaddr;
int ret;
@@ -864,8 +807,6 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
return ERR_PTR(-ENOMEM);
client->guc = guc;
- client->owner = ctx;
- client->engines = engines;
client->priority = priority;
client->doorbell_id = GUC_DOORBELL_INVALID;
spin_lock_init(&client->wq_lock);
@@ -910,8 +851,8 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
else
client->proc_desc_offset = (GUC_DB_SIZE / 2);
- DRM_DEBUG_DRIVER("new priority %u client %p for engine(s) 0x%x: stage_id %u\n",
- priority, client, client->engines, client->stage_id);
+ DRM_DEBUG_DRIVER("new priority %u client %p: stage_id %u\n",
+ priority, client, client->stage_id);
DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%lx\n",
client->doorbell_id, client->doorbell_offset);
@@ -951,15 +892,11 @@ static inline bool ctx_save_restore_disabled(struct intel_context *ce)
static int guc_clients_create(struct intel_guc *guc)
{
- struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct intel_guc_client *client;
GEM_BUG_ON(guc->execbuf_client);
- client = guc_client_alloc(dev_priv,
- INTEL_INFO(dev_priv)->engine_mask,
- GUC_CLIENT_PRIORITY_KMD_NORMAL,
- dev_priv->kernel_context);
+ client = guc_client_alloc(guc, GUC_CLIENT_PRIORITY_KMD_NORMAL);
if (IS_ERR(client)) {
DRM_ERROR("Failed to create GuC client for submission!\n");
return PTR_ERR(client);
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.h b/drivers/gpu/drm/i915/intel_guc_submission.h
index 7d823a513b9c..87a38cb6faf3 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.h
+++ b/drivers/gpu/drm/i915/intel_guc_submission.h
@@ -58,11 +58,9 @@ struct drm_i915_private;
struct intel_guc_client {
struct i915_vma *vma;
void *vaddr;
- struct i915_gem_context *owner;
struct intel_guc *guc;
/* bitmap of (host) engine ids */
- u32 engines;
u32 priority;
u32 stage_id;
u32 proc_desc_offset;
diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c
index 1a1915e44f6b..6ca76f5a98d4 100644
--- a/drivers/gpu/drm/i915/selftests/intel_guc.c
+++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
@@ -105,12 +105,7 @@ static int ring_doorbell_nop(struct intel_guc_client *client)
*/
static int validate_client(struct intel_guc_client *client, int client_priority)
{
- struct drm_i915_private *dev_priv = guc_to_i915(client->guc);
- struct i915_gem_context *ctx_owner = dev_priv->kernel_context;
-
- if (client->owner != ctx_owner ||
- client->engines != INTEL_INFO(dev_priv)->engine_mask ||
- client->priority != client_priority ||
+ if (client->priority != client_priority ||
client->doorbell_id == GUC_DOORBELL_INVALID)
return -EINVAL;
else
@@ -247,10 +242,7 @@ static int igt_guc_doorbells(void *arg)
goto unlock;
for (i = 0; i < ATTEMPTS; i++) {
- clients[i] = guc_client_alloc(dev_priv,
- INTEL_INFO(dev_priv)->engine_mask,
- i % GUC_CLIENT_PRIORITY_NUM,
- dev_priv->kernel_context);
+ clients[i] = guc_client_alloc(guc, i % GUC_CLIENT_PRIORITY_NUM);
if (!clients[i]) {
pr_err("[%d] No guc client\n", i);
--
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] 10+ messages in thread
* [PATCH 3/3] drm/i915/uc: replace uc init/fini misc
2019-07-02 20:09 [PATCH 1/3] drm/i915/guc: Remove preemption support for current fw Daniele Ceraolo Spurio
2019-07-02 20:09 ` [PATCH 2/3] drm/i915/guc: simplify guc client Daniele Ceraolo Spurio
@ 2019-07-02 20:09 ` Daniele Ceraolo Spurio
2019-07-02 20:52 ` Michal Wajdeczko
2019-07-02 22:04 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915/guc: Remove preemption support for current fw Patchwork
` (3 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Daniele Ceraolo Spurio @ 2019-07-02 20:09 UTC (permalink / raw)
To: intel-gfx
The "misc" terminology doesn't clearly explain what we intend to cover
in this phase. The only thing we do in there apart from FW fetch is
initializing the log workqueue. We can move the latter to the
init_early phase and rename the function to clarify that they only
fetch/release the blobs.
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 7 +++-
drivers/gpu/drm/i915/i915_gem.c | 12 +++---
drivers/gpu/drm/i915/intel_guc.c | 57 ++++------------------------
drivers/gpu/drm/i915/intel_guc.h | 5 +--
drivers/gpu/drm/i915/intel_guc_log.c | 32 +++++++++++++++-
drivers/gpu/drm/i915/intel_guc_log.h | 3 +-
drivers/gpu/drm/i915/intel_huc.c | 8 ----
drivers/gpu/drm/i915/intel_huc.h | 6 ---
drivers/gpu/drm/i915/intel_uc.c | 50 ++++++++++--------------
drivers/gpu/drm/i915/intel_uc.h | 6 +--
10 files changed, 75 insertions(+), 111 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 794c6814a6d0..20ca0208dd98 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -934,7 +934,11 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
intel_detect_pch(dev_priv);
intel_wopcm_init_early(&dev_priv->wopcm);
- intel_uc_init_early(dev_priv);
+
+ ret = intel_uc_init_early(dev_priv);
+ if (ret)
+ goto err_gem;
+
intel_pm_setup(dev_priv);
intel_init_dpio(dev_priv);
ret = intel_power_domains_init(dev_priv);
@@ -953,6 +957,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
err_uc:
intel_uc_cleanup_early(dev_priv);
+err_gem:
i915_gem_cleanup_early(dev_priv);
err_workqueues:
i915_workqueues_cleanup(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b7f290b77f8f..46a75844f303 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1429,13 +1429,11 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
if (ret)
return ret;
- ret = intel_uc_init_misc(dev_priv);
- if (ret)
- return ret;
+ intel_uc_fetch_firmwares(dev_priv);
ret = intel_wopcm_init(&dev_priv->wopcm);
if (ret)
- goto err_uc_misc;
+ goto err_uc_fw;
/* This is just a security blanket to placate dragons.
* On some systems, we very sporadically observe that the first TLBs
@@ -1561,8 +1559,8 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL);
mutex_unlock(&dev_priv->drm.struct_mutex);
-err_uc_misc:
- intel_uc_fini_misc(dev_priv);
+err_uc_fw:
+ intel_uc_cleanup_firmwares(dev_priv);
if (ret != -EIO) {
i915_gem_cleanup_userptr(dev_priv);
@@ -1628,7 +1626,7 @@ void i915_gem_fini(struct drm_i915_private *dev_priv)
intel_cleanup_gt_powersave(dev_priv);
- intel_uc_fini_misc(dev_priv);
+ intel_uc_cleanup_firmwares(dev_priv);
i915_gem_cleanup_userptr(dev_priv);
intel_timelines_fini(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 501b74f44374..03fad4fe3d9b 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -74,13 +74,16 @@ void intel_guc_init_send_regs(struct intel_guc *guc)
guc->send_regs.fw_domains = fw_domains;
}
-void intel_guc_init_early(struct intel_guc *guc)
+int intel_guc_init_early(struct intel_guc *guc)
{
struct drm_i915_private *i915 = guc_to_i915(guc);
+ int ret;
intel_guc_fw_init_early(guc);
intel_guc_ct_init_early(&guc->ct);
- intel_guc_log_init_early(&guc->log);
+ ret = intel_guc_log_init_early(&guc->log);
+ if (ret)
+ return ret;
mutex_init(&guc->send_mutex);
spin_lock_init(&guc->irq_lock);
@@ -97,59 +100,13 @@ void intel_guc_init_early(struct intel_guc *guc)
guc->interrupts.enable = gen9_enable_guc_interrupts;
guc->interrupts.disable = gen9_disable_guc_interrupts;
}
-}
-
-static int guc_init_wq(struct intel_guc *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
- * suspend, can potentially get executed after the GFX device is
- * suspended.
- * By marking the WQ as freezable, we don't have to bother about
- * flushing of this work item from the suspend hooks, the pending
- * work item if any will be either executed before the suspend
- * or scheduled later on resume. This way the handling of work
- * item can be kept same between system suspend & rpm suspend.
- */
- guc->log.relay.flush_wq =
- alloc_ordered_workqueue("i915-guc_log",
- WQ_HIGHPRI | WQ_FREEZABLE);
- if (!guc->log.relay.flush_wq) {
- DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
- return -ENOMEM;
- }
return 0;
}
-static void guc_fini_wq(struct intel_guc *guc)
-{
- struct workqueue_struct *wq;
-
- wq = fetch_and_zero(&guc->log.relay.flush_wq);
- if (wq)
- destroy_workqueue(wq);
-}
-
-int intel_guc_init_misc(struct intel_guc *guc)
+void intel_guc_fini_early(struct intel_guc *guc)
{
- struct drm_i915_private *i915 = guc_to_i915(guc);
- int ret;
-
- ret = guc_init_wq(guc);
- if (ret)
- return ret;
-
- intel_uc_fw_fetch(i915, &guc->fw);
-
- return 0;
-}
-
-void intel_guc_fini_misc(struct intel_guc *guc)
-{
- intel_uc_fw_cleanup_fetch(&guc->fw);
- guc_fini_wq(guc);
+ intel_guc_log_fini_early(&guc->log);
}
static int guc_shared_data_create(struct intel_guc *guc)
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index ec1038c1f50e..045a3608bade 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -150,13 +150,12 @@ static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc,
return offset;
}
-void intel_guc_init_early(struct intel_guc *guc);
+int intel_guc_init_early(struct intel_guc *guc);
+void intel_guc_fini_early(struct intel_guc *guc);
void intel_guc_init_send_regs(struct intel_guc *guc);
void intel_guc_init_params(struct intel_guc *guc);
-int intel_guc_init_misc(struct intel_guc *guc);
int intel_guc_init(struct intel_guc *guc);
void intel_guc_fini(struct intel_guc *guc);
-void intel_guc_fini_misc(struct intel_guc *guc);
int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len,
u32 *response_buf, u32 response_buf_size);
int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len,
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index e3b83ecb90b5..4b1c7a9f23e7 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -374,10 +374,40 @@ static void guc_log_unmap(struct intel_guc_log *log)
log->relay.buf_addr = NULL;
}
-void intel_guc_log_init_early(struct intel_guc_log *log)
+int intel_guc_log_init_early(struct intel_guc_log *log)
{
mutex_init(&log->relay.lock);
INIT_WORK(&log->relay.flush_work, capture_logs_work);
+
+ /*
+ * 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
+ * suspend, can potentially get executed after the GFX device is
+ * suspended.
+ * By marking the WQ as freezable, we don't have to bother about
+ * flushing of this work item from the suspend hooks, the pending
+ * work item if any will be either executed before the suspend
+ * or scheduled later on resume. This way the handling of work
+ * item can be kept same between system suspend & rpm suspend.
+ */
+ log->relay.flush_wq =
+ alloc_ordered_workqueue("i915-guc_log",
+ WQ_HIGHPRI | WQ_FREEZABLE);
+ if (!log->relay.flush_wq) {
+ DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+void intel_guc_log_fini_early(struct intel_guc_log *log)
+{
+ struct workqueue_struct *wq;
+
+ wq = fetch_and_zero(&log->relay.flush_wq);
+ if (wq)
+ destroy_workqueue(wq);
}
static int guc_log_relay_create(struct intel_guc_log *log)
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index 7bc763f10c03..8b5e2fc7df24 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -80,7 +80,8 @@ struct intel_guc_log {
} stats[GUC_MAX_LOG_BUFFER];
};
-void intel_guc_log_init_early(struct intel_guc_log *log);
+int intel_guc_log_init_early(struct intel_guc_log *log);
+void intel_guc_log_fini_early(struct intel_guc_log *log);
int intel_guc_log_create(struct intel_guc_log *log);
void intel_guc_log_destroy(struct intel_guc_log *log);
diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index fb6f693d3cac..2a41ee89a16d 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -44,14 +44,6 @@ void intel_huc_init_early(struct intel_huc *huc)
}
}
-int intel_huc_init_misc(struct intel_huc *huc)
-{
- struct drm_i915_private *i915 = huc_to_i915(huc);
-
- intel_uc_fw_fetch(i915, &huc->fw);
- return 0;
-}
-
static int intel_huc_rsa_data_create(struct intel_huc *huc)
{
struct drm_i915_private *i915 = huc_to_i915(huc);
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
index 2a6c94e79f17..9fa3d4629f2e 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -45,17 +45,11 @@ struct intel_huc {
};
void intel_huc_init_early(struct intel_huc *huc);
-int intel_huc_init_misc(struct intel_huc *huc);
int intel_huc_init(struct intel_huc *huc);
void intel_huc_fini(struct intel_huc *huc);
int intel_huc_auth(struct intel_huc *huc);
int intel_huc_check_status(struct intel_huc *huc);
-static inline void intel_huc_fini_misc(struct intel_huc *huc)
-{
- intel_uc_fw_cleanup_fetch(&huc->fw);
-}
-
static inline int intel_huc_sanitize(struct intel_huc *huc)
{
intel_uc_fw_sanitize(&huc->fw);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index fdf00f1ebb57..c3e65236cfba 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -171,15 +171,19 @@ static void sanitize_options_early(struct drm_i915_private *i915)
GEM_BUG_ON(i915_modparams.guc_log_level < 0);
}
-void intel_uc_init_early(struct drm_i915_private *i915)
+int intel_uc_init_early(struct drm_i915_private *i915)
{
- struct intel_guc *guc = &i915->guc;
- struct intel_huc *huc = &i915->huc;
+ int ret;
+
+ ret = intel_guc_init_early(&i915->guc);
+ if (ret)
+ return ret;
- intel_guc_init_early(guc);
- intel_huc_init_early(huc);
+ intel_huc_init_early(&i915->huc);
sanitize_options_early(i915);
+
+ return 0;
}
void intel_uc_cleanup_early(struct drm_i915_private *i915)
@@ -187,6 +191,8 @@ void intel_uc_cleanup_early(struct drm_i915_private *i915)
struct intel_guc *guc = &i915->guc;
guc_free_load_err_log(guc);
+
+ intel_guc_fini_early(guc);
}
/**
@@ -345,44 +351,26 @@ static void guc_disable_communication(struct intel_guc *guc)
DRM_INFO("GuC communication disabled\n");
}
-int intel_uc_init_misc(struct drm_i915_private *i915)
+void intel_uc_fetch_firmwares(struct drm_i915_private *i915)
{
- struct intel_guc *guc = &i915->guc;
- struct intel_huc *huc = &i915->huc;
- int ret;
-
if (!USES_GUC(i915))
- return 0;
-
- ret = intel_guc_init_misc(guc);
- if (ret)
- return ret;
+ return;
- if (USES_HUC(i915)) {
- ret = intel_huc_init_misc(huc);
- if (ret)
- goto err_guc;
- }
+ intel_uc_fw_fetch(i915, &i915->guc.fw);
- return 0;
-
-err_guc:
- intel_guc_fini_misc(guc);
- return ret;
+ if (USES_HUC(i915))
+ intel_uc_fw_fetch(i915, &i915->huc.fw);
}
-void intel_uc_fini_misc(struct drm_i915_private *i915)
+void intel_uc_cleanup_firmwares(struct drm_i915_private *i915)
{
- struct intel_guc *guc = &i915->guc;
- struct intel_huc *huc = &i915->huc;
-
if (!USES_GUC(i915))
return;
if (USES_HUC(i915))
- intel_huc_fini_misc(huc);
+ intel_uc_fw_cleanup_fetch(&i915->huc.fw);
- intel_guc_fini_misc(guc);
+ intel_uc_fw_cleanup_fetch(&i915->guc.fw);
}
int intel_uc_init(struct drm_i915_private *i915)
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 3ea06c87dfcd..c3022890e604 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -28,11 +28,11 @@
#include "intel_huc.h"
#include "i915_params.h"
-void intel_uc_init_early(struct drm_i915_private *dev_priv);
+int intel_uc_init_early(struct drm_i915_private *dev_priv);
void intel_uc_cleanup_early(struct drm_i915_private *dev_priv);
void intel_uc_init_mmio(struct drm_i915_private *dev_priv);
-int intel_uc_init_misc(struct drm_i915_private *dev_priv);
-void intel_uc_fini_misc(struct drm_i915_private *dev_priv);
+void intel_uc_fetch_firmwares(struct drm_i915_private *dev_priv);
+void intel_uc_cleanup_firmwares(struct drm_i915_private *dev_priv);
void intel_uc_sanitize(struct drm_i915_private *dev_priv);
int intel_uc_init_hw(struct drm_i915_private *dev_priv);
void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
--
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] 10+ messages in thread
* Re: [PATCH 3/3] drm/i915/uc: replace uc init/fini misc
2019-07-02 20:09 ` [PATCH 3/3] drm/i915/uc: replace uc init/fini misc Daniele Ceraolo Spurio
@ 2019-07-02 20:52 ` Michal Wajdeczko
2019-07-02 21:22 ` Daniele Ceraolo Spurio
0 siblings, 1 reply; 10+ messages in thread
From: Michal Wajdeczko @ 2019-07-02 20:52 UTC (permalink / raw)
To: intel-gfx, Daniele Ceraolo Spurio
On Tue, 02 Jul 2019 22:09:47 +0200, Daniele Ceraolo Spurio
<daniele.ceraolospurio@intel.com> wrote:
> The "misc" terminology doesn't clearly explain what we intend to cover
> in this phase. The only thing we do in there apart from FW fetch is
> initializing the log workqueue. We can move the latter to the
> init_early phase and rename the function to clarify that they only
> fetch/release the blobs.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 7 +++-
> drivers/gpu/drm/i915/i915_gem.c | 12 +++---
> drivers/gpu/drm/i915/intel_guc.c | 57 ++++------------------------
> drivers/gpu/drm/i915/intel_guc.h | 5 +--
> drivers/gpu/drm/i915/intel_guc_log.c | 32 +++++++++++++++-
> drivers/gpu/drm/i915/intel_guc_log.h | 3 +-
> drivers/gpu/drm/i915/intel_huc.c | 8 ----
> drivers/gpu/drm/i915/intel_huc.h | 6 ---
> drivers/gpu/drm/i915/intel_uc.c | 50 ++++++++++--------------
> drivers/gpu/drm/i915/intel_uc.h | 6 +--
> 10 files changed, 75 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 794c6814a6d0..20ca0208dd98 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -934,7 +934,11 @@ static int i915_driver_init_early(struct
> drm_i915_private *dev_priv)
> intel_detect_pch(dev_priv);
> intel_wopcm_init_early(&dev_priv->wopcm);
> - intel_uc_init_early(dev_priv);
> +
> + ret = intel_uc_init_early(dev_priv);
> + if (ret)
> + goto err_gem;
> +
> intel_pm_setup(dev_priv);
> intel_init_dpio(dev_priv);
> ret = intel_power_domains_init(dev_priv);
> @@ -953,6 +957,7 @@ static int i915_driver_init_early(struct
> drm_i915_private *dev_priv)
> err_uc:
> intel_uc_cleanup_early(dev_priv);
> +err_gem:
> i915_gem_cleanup_early(dev_priv);
> err_workqueues:
> i915_workqueues_cleanup(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index b7f290b77f8f..46a75844f303 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1429,13 +1429,11 @@ int i915_gem_init(struct drm_i915_private
> *dev_priv)
> if (ret)
> return ret;
> - ret = intel_uc_init_misc(dev_priv);
> - if (ret)
> - return ret;
> + intel_uc_fetch_firmwares(dev_priv);
> ret = intel_wopcm_init(&dev_priv->wopcm);
> if (ret)
> - goto err_uc_misc;
> + goto err_uc_fw;
> /* This is just a security blanket to placate dragons.
> * On some systems, we very sporadically observe that the first TLBs
> @@ -1561,8 +1559,8 @@ int i915_gem_init(struct drm_i915_private
> *dev_priv)
> intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL);
> mutex_unlock(&dev_priv->drm.struct_mutex);
> -err_uc_misc:
> - intel_uc_fini_misc(dev_priv);
> +err_uc_fw:
> + intel_uc_cleanup_firmwares(dev_priv);
> if (ret != -EIO) {
> i915_gem_cleanup_userptr(dev_priv);
> @@ -1628,7 +1626,7 @@ void i915_gem_fini(struct drm_i915_private
> *dev_priv)
> intel_cleanup_gt_powersave(dev_priv);
> - intel_uc_fini_misc(dev_priv);
> + intel_uc_cleanup_firmwares(dev_priv);
> i915_gem_cleanup_userptr(dev_priv);
> intel_timelines_fini(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_guc.c
> b/drivers/gpu/drm/i915/intel_guc.c
> index 501b74f44374..03fad4fe3d9b 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -74,13 +74,16 @@ void intel_guc_init_send_regs(struct intel_guc *guc)
> guc->send_regs.fw_domains = fw_domains;
> }
> -void intel_guc_init_early(struct intel_guc *guc)
> +int intel_guc_init_early(struct intel_guc *guc)
> {
> struct drm_i915_private *i915 = guc_to_i915(guc);
> + int ret;
> intel_guc_fw_init_early(guc);
> intel_guc_ct_init_early(&guc->ct);
> - intel_guc_log_init_early(&guc->log);
> + ret = intel_guc_log_init_early(&guc->log);
> + if (ret)
> + return ret;
> mutex_init(&guc->send_mutex);
> spin_lock_init(&guc->irq_lock);
> @@ -97,59 +100,13 @@ void intel_guc_init_early(struct intel_guc *guc)
> guc->interrupts.enable = gen9_enable_guc_interrupts;
> guc->interrupts.disable = gen9_disable_guc_interrupts;
> }
> -}
> -
> -static int guc_init_wq(struct intel_guc *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
> - * suspend, can potentially get executed after the GFX device is
> - * suspended.
> - * By marking the WQ as freezable, we don't have to bother about
> - * flushing of this work item from the suspend hooks, the pending
> - * work item if any will be either executed before the suspend
> - * or scheduled later on resume. This way the handling of work
> - * item can be kept same between system suspend & rpm suspend.
> - */
> - guc->log.relay.flush_wq =
> - alloc_ordered_workqueue("i915-guc_log",
> - WQ_HIGHPRI | WQ_FREEZABLE);
> - if (!guc->log.relay.flush_wq) {
> - DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
> - return -ENOMEM;
> - }
> return 0;
> }
> -static void guc_fini_wq(struct intel_guc *guc)
> -{
> - struct workqueue_struct *wq;
> -
> - wq = fetch_and_zero(&guc->log.relay.flush_wq);
> - if (wq)
> - destroy_workqueue(wq);
> -}
> -
> -int intel_guc_init_misc(struct intel_guc *guc)
> +void intel_guc_fini_early(struct intel_guc *guc)
looks like this function can be defined as inline in .h
> {
> - struct drm_i915_private *i915 = guc_to_i915(guc);
> - int ret;
> -
> - ret = guc_init_wq(guc);
> - if (ret)
> - return ret;
> -
> - intel_uc_fw_fetch(i915, &guc->fw);
> -
> - return 0;
> -}
> -
> -void intel_guc_fini_misc(struct intel_guc *guc)
> -{
> - intel_uc_fw_cleanup_fetch(&guc->fw);
> - guc_fini_wq(guc);
> + intel_guc_log_fini_early(&guc->log);
> }
> static int guc_shared_data_create(struct intel_guc *guc)
> diff --git a/drivers/gpu/drm/i915/intel_guc.h
> b/drivers/gpu/drm/i915/intel_guc.h
> index ec1038c1f50e..045a3608bade 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -150,13 +150,12 @@ static inline u32 intel_guc_ggtt_offset(struct
> intel_guc *guc,
> return offset;
> }
> -void intel_guc_init_early(struct intel_guc *guc);
> +int intel_guc_init_early(struct intel_guc *guc);
> +void intel_guc_fini_early(struct intel_guc *guc);
> void intel_guc_init_send_regs(struct intel_guc *guc);
> void intel_guc_init_params(struct intel_guc *guc);
> -int intel_guc_init_misc(struct intel_guc *guc);
> int intel_guc_init(struct intel_guc *guc);
> void intel_guc_fini(struct intel_guc *guc);
> -void intel_guc_fini_misc(struct intel_guc *guc);
> int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32
> len,
> u32 *response_buf, u32 response_buf_size);
> int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32
> len,
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index e3b83ecb90b5..4b1c7a9f23e7 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -374,10 +374,40 @@ static void guc_log_unmap(struct intel_guc_log
> *log)
> log->relay.buf_addr = NULL;
> }
> -void intel_guc_log_init_early(struct intel_guc_log *log)
> +int intel_guc_log_init_early(struct intel_guc_log *log)
> {
> mutex_init(&log->relay.lock);
> INIT_WORK(&log->relay.flush_work, capture_logs_work);
> +
> + /*
> + * 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
> + * suspend, can potentially get executed after the GFX device is
> + * suspended.
> + * By marking the WQ as freezable, we don't have to bother about
> + * flushing of this work item from the suspend hooks, the pending
> + * work item if any will be either executed before the suspend
> + * or scheduled later on resume. This way the handling of work
> + * item can be kept same between system suspend & rpm suspend.
> + */
> + log->relay.flush_wq =
> + alloc_ordered_workqueue("i915-guc_log",
> + WQ_HIGHPRI | WQ_FREEZABLE);
I'm wondering if this is ok that now we always create wq regardless GuC
is used or not ... in now removed init_misc we checked for USES_GUC
maybe we can use other already existing wq for flush work instead?
> + if (!log->relay.flush_wq) {
> + DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +void intel_guc_log_fini_early(struct intel_guc_log *log)
> +{
> + struct workqueue_struct *wq;
> +
> + wq = fetch_and_zero(&log->relay.flush_wq);
> + if (wq)
do we need to check this? if we failed to allocate wq in init_early
we should not be here
> + destroy_workqueue(wq);
> }
> static int guc_log_relay_create(struct intel_guc_log *log)
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h
> b/drivers/gpu/drm/i915/intel_guc_log.h
> index 7bc763f10c03..8b5e2fc7df24 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -80,7 +80,8 @@ struct intel_guc_log {
> } stats[GUC_MAX_LOG_BUFFER];
> };
> -void intel_guc_log_init_early(struct intel_guc_log *log);
> +int intel_guc_log_init_early(struct intel_guc_log *log);
> +void intel_guc_log_fini_early(struct intel_guc_log *log);
> int intel_guc_log_create(struct intel_guc_log *log);
> void intel_guc_log_destroy(struct intel_guc_log *log);
> diff --git a/drivers/gpu/drm/i915/intel_huc.c
> b/drivers/gpu/drm/i915/intel_huc.c
> index fb6f693d3cac..2a41ee89a16d 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -44,14 +44,6 @@ void intel_huc_init_early(struct intel_huc *huc)
> }
> }
> -int intel_huc_init_misc(struct intel_huc *huc)
> -{
> - struct drm_i915_private *i915 = huc_to_i915(huc);
> -
> - intel_uc_fw_fetch(i915, &huc->fw);
> - return 0;
> -}
> -
> static int intel_huc_rsa_data_create(struct intel_huc *huc)
> {
> struct drm_i915_private *i915 = huc_to_i915(huc);
> diff --git a/drivers/gpu/drm/i915/intel_huc.h
> b/drivers/gpu/drm/i915/intel_huc.h
> index 2a6c94e79f17..9fa3d4629f2e 100644
> --- a/drivers/gpu/drm/i915/intel_huc.h
> +++ b/drivers/gpu/drm/i915/intel_huc.h
> @@ -45,17 +45,11 @@ struct intel_huc {
> };
> void intel_huc_init_early(struct intel_huc *huc);
> -int intel_huc_init_misc(struct intel_huc *huc);
> int intel_huc_init(struct intel_huc *huc);
> void intel_huc_fini(struct intel_huc *huc);
> int intel_huc_auth(struct intel_huc *huc);
> int intel_huc_check_status(struct intel_huc *huc);
> -static inline void intel_huc_fini_misc(struct intel_huc *huc)
> -{
> - intel_uc_fw_cleanup_fetch(&huc->fw);
> -}
> -
> static inline int intel_huc_sanitize(struct intel_huc *huc)
> {
> intel_uc_fw_sanitize(&huc->fw);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index fdf00f1ebb57..c3e65236cfba 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -171,15 +171,19 @@ static void sanitize_options_early(struct
> drm_i915_private *i915)
> GEM_BUG_ON(i915_modparams.guc_log_level < 0);
> }
> -void intel_uc_init_early(struct drm_i915_private *i915)
> +int intel_uc_init_early(struct drm_i915_private *i915)
> {
> - struct intel_guc *guc = &i915->guc;
> - struct intel_huc *huc = &i915->huc;
> + int ret;
> +
> + ret = intel_guc_init_early(&i915->guc);
> + if (ret)
> + return ret;
> - intel_guc_init_early(guc);
> - intel_huc_init_early(huc);
> + intel_huc_init_early(&i915->huc);
> sanitize_options_early(i915);
> +
> + return 0;
> }
> void intel_uc_cleanup_early(struct drm_i915_private *i915)
> @@ -187,6 +191,8 @@ void intel_uc_cleanup_early(struct drm_i915_private
> *i915)
> struct intel_guc *guc = &i915->guc;
> guc_free_load_err_log(guc);
> +
> + intel_guc_fini_early(guc);
> }
> /**
> @@ -345,44 +351,26 @@ static void guc_disable_communication(struct
> intel_guc *guc)
> DRM_INFO("GuC communication disabled\n");
> }
> -int intel_uc_init_misc(struct drm_i915_private *i915)
> +void intel_uc_fetch_firmwares(struct drm_i915_private *i915)
> {
> - struct intel_guc *guc = &i915->guc;
> - struct intel_huc *huc = &i915->huc;
> - int ret;
> -
> if (!USES_GUC(i915))
> - return 0;
> -
> - ret = intel_guc_init_misc(guc);
> - if (ret)
> - return ret;
> + return;
> - if (USES_HUC(i915)) {
> - ret = intel_huc_init_misc(huc);
> - if (ret)
> - goto err_guc;
> - }
> + intel_uc_fw_fetch(i915, &i915->guc.fw);
> - return 0;
> -
> -err_guc:
> - intel_guc_fini_misc(guc);
> - return ret;
> + if (USES_HUC(i915))
> + intel_uc_fw_fetch(i915, &i915->huc.fw);
> }
> -void intel_uc_fini_misc(struct drm_i915_private *i915)
> +void intel_uc_cleanup_firmwares(struct drm_i915_private *i915)
> {
> - struct intel_guc *guc = &i915->guc;
> - struct intel_huc *huc = &i915->huc;
> -
> if (!USES_GUC(i915))
> return;
> if (USES_HUC(i915))
> - intel_huc_fini_misc(huc);
> + intel_uc_fw_cleanup_fetch(&i915->huc.fw);
> - intel_guc_fini_misc(guc);
> + intel_uc_fw_cleanup_fetch(&i915->guc.fw);
> }
> int intel_uc_init(struct drm_i915_private *i915)
> diff --git a/drivers/gpu/drm/i915/intel_uc.h
> b/drivers/gpu/drm/i915/intel_uc.h
> index 3ea06c87dfcd..c3022890e604 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -28,11 +28,11 @@
> #include "intel_huc.h"
> #include "i915_params.h"
> -void intel_uc_init_early(struct drm_i915_private *dev_priv);
> +int intel_uc_init_early(struct drm_i915_private *dev_priv);
> void intel_uc_cleanup_early(struct drm_i915_private *dev_priv);
> void intel_uc_init_mmio(struct drm_i915_private *dev_priv);
> -int intel_uc_init_misc(struct drm_i915_private *dev_priv);
> -void intel_uc_fini_misc(struct drm_i915_private *dev_priv);
> +void intel_uc_fetch_firmwares(struct drm_i915_private *dev_priv);
> +void intel_uc_cleanup_firmwares(struct drm_i915_private *dev_priv);
> void intel_uc_sanitize(struct drm_i915_private *dev_priv);
> int intel_uc_init_hw(struct drm_i915_private *dev_priv);
> void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/i915/uc: replace uc init/fini misc
2019-07-02 20:52 ` Michal Wajdeczko
@ 2019-07-02 21:22 ` Daniele Ceraolo Spurio
0 siblings, 0 replies; 10+ messages in thread
From: Daniele Ceraolo Spurio @ 2019-07-02 21:22 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 7/2/19 1:52 PM, Michal Wajdeczko wrote:
> On Tue, 02 Jul 2019 22:09:47 +0200, Daniele Ceraolo Spurio
> <daniele.ceraolospurio@intel.com> wrote:
>
>> The "misc" terminology doesn't clearly explain what we intend to cover
>> in this phase. The only thing we do in there apart from FW fetch is
>> initializing the log workqueue. We can move the latter to the
>> init_early phase and rename the function to clarify that they only
>> fetch/release the blobs.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 7 +++-
>> drivers/gpu/drm/i915/i915_gem.c | 12 +++---
>> drivers/gpu/drm/i915/intel_guc.c | 57 ++++------------------------
>> drivers/gpu/drm/i915/intel_guc.h | 5 +--
>> drivers/gpu/drm/i915/intel_guc_log.c | 32 +++++++++++++++-
>> drivers/gpu/drm/i915/intel_guc_log.h | 3 +-
>> drivers/gpu/drm/i915/intel_huc.c | 8 ----
>> drivers/gpu/drm/i915/intel_huc.h | 6 ---
>> drivers/gpu/drm/i915/intel_uc.c | 50 ++++++++++--------------
>> drivers/gpu/drm/i915/intel_uc.h | 6 +--
>> 10 files changed, 75 insertions(+), 111 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 794c6814a6d0..20ca0208dd98 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -934,7 +934,11 @@ static int i915_driver_init_early(struct
>> drm_i915_private *dev_priv)
>> intel_detect_pch(dev_priv);
>> intel_wopcm_init_early(&dev_priv->wopcm);
>> - intel_uc_init_early(dev_priv);
>> +
>> + ret = intel_uc_init_early(dev_priv);
>> + if (ret)
>> + goto err_gem;
>> +
>> intel_pm_setup(dev_priv);
>> intel_init_dpio(dev_priv);
>> ret = intel_power_domains_init(dev_priv);
>> @@ -953,6 +957,7 @@ static int i915_driver_init_early(struct
>> drm_i915_private *dev_priv)
>> err_uc:
>> intel_uc_cleanup_early(dev_priv);
>> +err_gem:
>> i915_gem_cleanup_early(dev_priv);
>> err_workqueues:
>> i915_workqueues_cleanup(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index b7f290b77f8f..46a75844f303 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1429,13 +1429,11 @@ int i915_gem_init(struct drm_i915_private
>> *dev_priv)
>> if (ret)
>> return ret;
>> - ret = intel_uc_init_misc(dev_priv);
>> - if (ret)
>> - return ret;
>> + intel_uc_fetch_firmwares(dev_priv);
>> ret = intel_wopcm_init(&dev_priv->wopcm);
>> if (ret)
>> - goto err_uc_misc;
>> + goto err_uc_fw;
>> /* This is just a security blanket to placate dragons.
>> * On some systems, we very sporadically observe that the first TLBs
>> @@ -1561,8 +1559,8 @@ int i915_gem_init(struct drm_i915_private
>> *dev_priv)
>> intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL);
>> mutex_unlock(&dev_priv->drm.struct_mutex);
>> -err_uc_misc:
>> - intel_uc_fini_misc(dev_priv);
>> +err_uc_fw:
>> + intel_uc_cleanup_firmwares(dev_priv);
>> if (ret != -EIO) {
>> i915_gem_cleanup_userptr(dev_priv);
>> @@ -1628,7 +1626,7 @@ void i915_gem_fini(struct drm_i915_private
>> *dev_priv)
>> intel_cleanup_gt_powersave(dev_priv);
>> - intel_uc_fini_misc(dev_priv);
>> + intel_uc_cleanup_firmwares(dev_priv);
>> i915_gem_cleanup_userptr(dev_priv);
>> intel_timelines_fini(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 501b74f44374..03fad4fe3d9b 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -74,13 +74,16 @@ void intel_guc_init_send_regs(struct intel_guc *guc)
>> guc->send_regs.fw_domains = fw_domains;
>> }
>> -void intel_guc_init_early(struct intel_guc *guc)
>> +int intel_guc_init_early(struct intel_guc *guc)
>> {
>> struct drm_i915_private *i915 = guc_to_i915(guc);
>> + int ret;
>> intel_guc_fw_init_early(guc);
>> intel_guc_ct_init_early(&guc->ct);
>> - intel_guc_log_init_early(&guc->log);
>> + ret = intel_guc_log_init_early(&guc->log);
>> + if (ret)
>> + return ret;
>> mutex_init(&guc->send_mutex);
>> spin_lock_init(&guc->irq_lock);
>> @@ -97,59 +100,13 @@ void intel_guc_init_early(struct intel_guc *guc)
>> guc->interrupts.enable = gen9_enable_guc_interrupts;
>> guc->interrupts.disable = gen9_disable_guc_interrupts;
>> }
>> -}
>> -
>> -static int guc_init_wq(struct intel_guc *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
>> - * suspend, can potentially get executed after the GFX device is
>> - * suspended.
>> - * By marking the WQ as freezable, we don't have to bother about
>> - * flushing of this work item from the suspend hooks, the pending
>> - * work item if any will be either executed before the suspend
>> - * or scheduled later on resume. This way the handling of work
>> - * item can be kept same between system suspend & rpm suspend.
>> - */
>> - guc->log.relay.flush_wq =
>> - alloc_ordered_workqueue("i915-guc_log",
>> - WQ_HIGHPRI | WQ_FREEZABLE);
>> - if (!guc->log.relay.flush_wq) {
>> - DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
>> - return -ENOMEM;
>> - }
>> return 0;
>> }
>> -static void guc_fini_wq(struct intel_guc *guc)
>> -{
>> - struct workqueue_struct *wq;
>> -
>> - wq = fetch_and_zero(&guc->log.relay.flush_wq);
>> - if (wq)
>> - destroy_workqueue(wq);
>> -}
>> -
>> -int intel_guc_init_misc(struct intel_guc *guc)
>> +void intel_guc_fini_early(struct intel_guc *guc)
>
> looks like this function can be defined as inline in .h
>
I thought we were moving away from inlines in .h files when it is not
required for performance.
>> {
>> - struct drm_i915_private *i915 = guc_to_i915(guc);
>> - int ret;
>> -
>> - ret = guc_init_wq(guc);
>> - if (ret)
>> - return ret;
>> -
>> - intel_uc_fw_fetch(i915, &guc->fw);
>> -
>> - return 0;
>> -}
>> -
>> -void intel_guc_fini_misc(struct intel_guc *guc)
>> -{
>> - intel_uc_fw_cleanup_fetch(&guc->fw);
>> - guc_fini_wq(guc);
>> + intel_guc_log_fini_early(&guc->log);
>> }
>> static int guc_shared_data_create(struct intel_guc *guc)
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index ec1038c1f50e..045a3608bade 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -150,13 +150,12 @@ static inline u32 intel_guc_ggtt_offset(struct
>> intel_guc *guc,
>> return offset;
>> }
>> -void intel_guc_init_early(struct intel_guc *guc);
>> +int intel_guc_init_early(struct intel_guc *guc);
>> +void intel_guc_fini_early(struct intel_guc *guc);
>> void intel_guc_init_send_regs(struct intel_guc *guc);
>> void intel_guc_init_params(struct intel_guc *guc);
>> -int intel_guc_init_misc(struct intel_guc *guc);
>> int intel_guc_init(struct intel_guc *guc);
>> void intel_guc_fini(struct intel_guc *guc);
>> -void intel_guc_fini_misc(struct intel_guc *guc);
>> int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32
>> len,
>> u32 *response_buf, u32 response_buf_size);
>> int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32
>> len,
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index e3b83ecb90b5..4b1c7a9f23e7 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -374,10 +374,40 @@ static void guc_log_unmap(struct intel_guc_log
>> *log)
>> log->relay.buf_addr = NULL;
>> }
>> -void intel_guc_log_init_early(struct intel_guc_log *log)
>> +int intel_guc_log_init_early(struct intel_guc_log *log)
>> {
>> mutex_init(&log->relay.lock);
>> INIT_WORK(&log->relay.flush_work, capture_logs_work);
>> +
>> + /*
>> + * 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
>> + * suspend, can potentially get executed after the GFX device is
>> + * suspended.
>> + * By marking the WQ as freezable, we don't have to bother about
>> + * flushing of this work item from the suspend hooks, the pending
>> + * work item if any will be either executed before the suspend
>> + * or scheduled later on resume. This way the handling of work
>> + * item can be kept same between system suspend & rpm suspend.
>> + */
>> + log->relay.flush_wq =
>> + alloc_ordered_workqueue("i915-guc_log",
>> + WQ_HIGHPRI | WQ_FREEZABLE);
>
> I'm wondering if this is ok that now we always create wq regardless GuC
> is used or not ... in now removed init_misc we checked for USES_GUC
>
> maybe we can use other already existing wq for flush work instead?
>
I don't think we have another suitable wq. I could reorder uc_init_early
instead to only allocate the wq if USES_GUC is true, e.g.:
int intel_uc_init_early(struct drm_i915_private *i915)
{
struct intel_guc *guc = &i915->guc;
struct intel_huc *huc = &i915->huc;
int ret;
intel_guc_fw_init_early(guc);
intel_huc_fw_init_early(huc);
/* option sanitizing requires fw selection */
sanitize_options_early(i915);
if (!USES_GUC(i915))
return 0;
ret = intel_guc_init_early(guc);
if (ret)
return ret;
intel_huc_init_early(huc);
return 0;
}
What do you think?
>> + if (!log->relay.flush_wq) {
>> + DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
>> + return -ENOMEM;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +void intel_guc_log_fini_early(struct intel_guc_log *log)
>> +{
>> + struct workqueue_struct *wq;
>> +
>> + wq = fetch_and_zero(&log->relay.flush_wq);
>> + if (wq)
>
> do we need to check this? if we failed to allocate wq in init_early
> we should not be here
>
True, will update.
Daniele
>> + destroy_workqueue(wq);
>> }
>> static int guc_log_relay_create(struct intel_guc_log *log)
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h
>> b/drivers/gpu/drm/i915/intel_guc_log.h
>> index 7bc763f10c03..8b5e2fc7df24 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
>> @@ -80,7 +80,8 @@ struct intel_guc_log {
>> } stats[GUC_MAX_LOG_BUFFER];
>> };
>> -void intel_guc_log_init_early(struct intel_guc_log *log);
>> +int intel_guc_log_init_early(struct intel_guc_log *log);
>> +void intel_guc_log_fini_early(struct intel_guc_log *log);
>> int intel_guc_log_create(struct intel_guc_log *log);
>> void intel_guc_log_destroy(struct intel_guc_log *log);
>> diff --git a/drivers/gpu/drm/i915/intel_huc.c
>> b/drivers/gpu/drm/i915/intel_huc.c
>> index fb6f693d3cac..2a41ee89a16d 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -44,14 +44,6 @@ void intel_huc_init_early(struct intel_huc *huc)
>> }
>> }
>> -int intel_huc_init_misc(struct intel_huc *huc)
>> -{
>> - struct drm_i915_private *i915 = huc_to_i915(huc);
>> -
>> - intel_uc_fw_fetch(i915, &huc->fw);
>> - return 0;
>> -}
>> -
>> static int intel_huc_rsa_data_create(struct intel_huc *huc)
>> {
>> struct drm_i915_private *i915 = huc_to_i915(huc);
>> diff --git a/drivers/gpu/drm/i915/intel_huc.h
>> b/drivers/gpu/drm/i915/intel_huc.h
>> index 2a6c94e79f17..9fa3d4629f2e 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.h
>> +++ b/drivers/gpu/drm/i915/intel_huc.h
>> @@ -45,17 +45,11 @@ struct intel_huc {
>> };
>> void intel_huc_init_early(struct intel_huc *huc);
>> -int intel_huc_init_misc(struct intel_huc *huc);
>> int intel_huc_init(struct intel_huc *huc);
>> void intel_huc_fini(struct intel_huc *huc);
>> int intel_huc_auth(struct intel_huc *huc);
>> int intel_huc_check_status(struct intel_huc *huc);
>> -static inline void intel_huc_fini_misc(struct intel_huc *huc)
>> -{
>> - intel_uc_fw_cleanup_fetch(&huc->fw);
>> -}
>> -
>> static inline int intel_huc_sanitize(struct intel_huc *huc)
>> {
>> intel_uc_fw_sanitize(&huc->fw);
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index fdf00f1ebb57..c3e65236cfba 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -171,15 +171,19 @@ static void sanitize_options_early(struct
>> drm_i915_private *i915)
>> GEM_BUG_ON(i915_modparams.guc_log_level < 0);
>> }
>> -void intel_uc_init_early(struct drm_i915_private *i915)
>> +int intel_uc_init_early(struct drm_i915_private *i915)
>> {
>> - struct intel_guc *guc = &i915->guc;
>> - struct intel_huc *huc = &i915->huc;
>> + int ret;
>> +
>> + ret = intel_guc_init_early(&i915->guc);
>> + if (ret)
>> + return ret;
>> - intel_guc_init_early(guc);
>> - intel_huc_init_early(huc);
>> + intel_huc_init_early(&i915->huc);
>> sanitize_options_early(i915);
>> +
>> + return 0;
>> }
>> void intel_uc_cleanup_early(struct drm_i915_private *i915)
>> @@ -187,6 +191,8 @@ void intel_uc_cleanup_early(struct
>> drm_i915_private *i915)
>> struct intel_guc *guc = &i915->guc;
>> guc_free_load_err_log(guc);
>> +
>> + intel_guc_fini_early(guc);
>> }
>> /**
>> @@ -345,44 +351,26 @@ static void guc_disable_communication(struct
>> intel_guc *guc)
>> DRM_INFO("GuC communication disabled\n");
>> }
>> -int intel_uc_init_misc(struct drm_i915_private *i915)
>> +void intel_uc_fetch_firmwares(struct drm_i915_private *i915)
>> {
>> - struct intel_guc *guc = &i915->guc;
>> - struct intel_huc *huc = &i915->huc;
>> - int ret;
>> -
>> if (!USES_GUC(i915))
>> - return 0;
>> -
>> - ret = intel_guc_init_misc(guc);
>> - if (ret)
>> - return ret;
>> + return;
>> - if (USES_HUC(i915)) {
>> - ret = intel_huc_init_misc(huc);
>> - if (ret)
>> - goto err_guc;
>> - }
>> + intel_uc_fw_fetch(i915, &i915->guc.fw);
>> - return 0;
>> -
>> -err_guc:
>> - intel_guc_fini_misc(guc);
>> - return ret;
>> + if (USES_HUC(i915))
>> + intel_uc_fw_fetch(i915, &i915->huc.fw);
>> }
>> -void intel_uc_fini_misc(struct drm_i915_private *i915)
>> +void intel_uc_cleanup_firmwares(struct drm_i915_private *i915)
>> {
>> - struct intel_guc *guc = &i915->guc;
>> - struct intel_huc *huc = &i915->huc;
>> -
>> if (!USES_GUC(i915))
>> return;
>> if (USES_HUC(i915))
>> - intel_huc_fini_misc(huc);
>> + intel_uc_fw_cleanup_fetch(&i915->huc.fw);
>> - intel_guc_fini_misc(guc);
>> + intel_uc_fw_cleanup_fetch(&i915->guc.fw);
>> }
>> int intel_uc_init(struct drm_i915_private *i915)
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h
>> b/drivers/gpu/drm/i915/intel_uc.h
>> index 3ea06c87dfcd..c3022890e604 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -28,11 +28,11 @@
>> #include "intel_huc.h"
>> #include "i915_params.h"
>> -void intel_uc_init_early(struct drm_i915_private *dev_priv);
>> +int intel_uc_init_early(struct drm_i915_private *dev_priv);
>> void intel_uc_cleanup_early(struct drm_i915_private *dev_priv);
>> void intel_uc_init_mmio(struct drm_i915_private *dev_priv);
>> -int intel_uc_init_misc(struct drm_i915_private *dev_priv);
>> -void intel_uc_fini_misc(struct drm_i915_private *dev_priv);
>> +void intel_uc_fetch_firmwares(struct drm_i915_private *dev_priv);
>> +void intel_uc_cleanup_firmwares(struct drm_i915_private *dev_priv);
>> void intel_uc_sanitize(struct drm_i915_private *dev_priv);
>> int intel_uc_init_hw(struct drm_i915_private *dev_priv);
>> void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915/guc: Remove preemption support for current fw
2019-07-02 20:09 [PATCH 1/3] drm/i915/guc: Remove preemption support for current fw Daniele Ceraolo Spurio
2019-07-02 20:09 ` [PATCH 2/3] drm/i915/guc: simplify guc client Daniele Ceraolo Spurio
2019-07-02 20:09 ` [PATCH 3/3] drm/i915/uc: replace uc init/fini misc Daniele Ceraolo Spurio
@ 2019-07-02 22:04 ` Patchwork
2019-07-02 22:37 ` ✓ Fi.CI.BAT: success " Patchwork
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-07-02 22:04 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/guc: Remove preemption support for current fw
URL : https://patchwork.freedesktop.org/series/63095/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/guc: Remove preemption support for current fw
-./drivers/gpu/drm/i915/i915_scheduler.h:70:23: warning: expression using sizeof(void)
Commit: drm/i915/guc: simplify guc client
Okay!
Commit: drm/i915/uc: replace uc init/fini misc
Okay!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/guc: Remove preemption support for current fw
2019-07-02 20:09 [PATCH 1/3] drm/i915/guc: Remove preemption support for current fw Daniele Ceraolo Spurio
` (2 preceding siblings ...)
2019-07-02 22:04 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915/guc: Remove preemption support for current fw Patchwork
@ 2019-07-02 22:37 ` Patchwork
2019-07-03 22:59 ` ✓ Fi.CI.IGT: " Patchwork
2019-07-09 0:45 ` [PATCH 1/3] " Matthew Brost
5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-07-02 22:37 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/guc: Remove preemption support for current fw
URL : https://patchwork.freedesktop.org/series/63095/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_6398 -> Patchwork_13497
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/
Known issues
------------
Here are the changes found in Patchwork_13497 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live_evict:
- fi-snb-2600: [PASS][1] -> [INCOMPLETE][2] ([fdo#105411])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/fi-snb-2600/igt@i915_selftest@live_evict.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/fi-snb-2600/igt@i915_selftest@live_evict.html
* igt@i915_selftest@live_hangcheck:
- fi-icl-u2: [PASS][3] -> [INCOMPLETE][4] ([fdo#107713] / [fdo#108569])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/fi-icl-u2/igt@i915_selftest@live_hangcheck.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/fi-icl-u2/igt@i915_selftest@live_hangcheck.html
* igt@kms_frontbuffer_tracking@basic:
- fi-icl-u2: [PASS][5] -> [FAIL][6] ([fdo#103167])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html
#### Possible fixes ####
* igt@gem_ctx_create@basic-files:
- fi-icl-dsi: [INCOMPLETE][7] ([fdo#107713] / [fdo#109100]) -> [PASS][8]
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/fi-icl-dsi/igt@gem_ctx_create@basic-files.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/fi-icl-dsi/igt@gem_ctx_create@basic-files.html
* igt@i915_pm_rpm@module-reload:
- fi-kbl-r: [DMESG-WARN][9] ([fdo#111012]) -> [PASS][10]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/fi-kbl-r/igt@i915_pm_rpm@module-reload.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/fi-kbl-r/igt@i915_pm_rpm@module-reload.html
- fi-hsw-peppy: [DMESG-WARN][11] ([fdo#111012]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/fi-hsw-peppy/igt@i915_pm_rpm@module-reload.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/fi-hsw-peppy/igt@i915_pm_rpm@module-reload.html
* igt@prime_vgem@basic-read:
- fi-icl-u3: [DMESG-WARN][13] ([fdo#107724]) -> [PASS][14]
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/fi-icl-u3/igt@prime_vgem@basic-read.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/fi-icl-u3/igt@prime_vgem@basic-read.html
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
[fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
[fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
[fdo#111012]: https://bugs.freedesktop.org/show_bug.cgi?id=111012
Participating hosts (55 -> 46)
------------------------------
Missing (9): fi-kbl-soraka fi-ilk-m540 fi-skl-gvtdvm fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper fi-bdw-samus
Build changes
-------------
* Linux: CI_DRM_6398 -> Patchwork_13497
CI_DRM_6398: 9b9df28dc0ec04a7fb1a020d869ef0ea14be4d14 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5079: 873df2fa9e8f5fd02d4532b30ef2579f4fe4f27f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_13497: 547860d7ee4f3d1bc1460defcc932cf08755fee6 @ git://anongit.freedesktop.org/gfx-ci/linux
== Kernel 32bit build ==
Warning: Kernel 32bit buildtest failed:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/build_32bit.log
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
CHK include/generated/compile.h
Kernel: arch/x86/boot/bzImage is ready (#1)
Building modules, stage 2.
MODPOST 112 modules
ERROR: "__udivdi3" [drivers/gpu/drm/amd/amdgpu/amdgpu.ko] undefined!
ERROR: "__divdi3" [drivers/gpu/drm/amd/amdgpu/amdgpu.ko] undefined!
scripts/Makefile.modpost:91: recipe for target '__modpost' failed
make[1]: *** [__modpost] Error 1
Makefile:1287: recipe for target 'modules' failed
make: *** [modules] Error 2
== Linux commits ==
547860d7ee4f drm/i915/uc: replace uc init/fini misc
877471f86b10 drm/i915/guc: simplify guc client
627e2de437dc drm/i915/guc: Remove preemption support for current fw
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915/guc: Remove preemption support for current fw
2019-07-02 20:09 [PATCH 1/3] drm/i915/guc: Remove preemption support for current fw Daniele Ceraolo Spurio
` (3 preceding siblings ...)
2019-07-02 22:37 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2019-07-03 22:59 ` Patchwork
2019-07-09 0:45 ` [PATCH 1/3] " Matthew Brost
5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-07-03 22:59 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915/guc: Remove preemption support for current fw
URL : https://patchwork.freedesktop.org/series/63095/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_6398_full -> Patchwork_13497_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_13497_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_workarounds@suspend-resume-context:
- shard-apl: [PASS][1] -> [DMESG-WARN][2] ([fdo#108566]) +8 similar issues
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-apl2/igt@gem_workarounds@suspend-resume-context.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-apl2/igt@gem_workarounds@suspend-resume-context.html
* igt@i915_selftest@mock_fence:
- shard-iclb: [PASS][3] -> [INCOMPLETE][4] ([fdo#107713]) +1 similar issue
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-iclb2/igt@i915_selftest@mock_fence.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-iclb1/igt@i915_selftest@mock_fence.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-render:
- shard-iclb: [PASS][5] -> [FAIL][6] ([fdo#103167]) +3 similar issues
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-iclb7/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-render.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-iclb5/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-render.html
* igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
- shard-skl: [PASS][7] -> [FAIL][8] ([fdo#108145] / [fdo#110403])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-skl5/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-skl9/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
* igt@kms_plane_lowres@pipe-a-tiling-x:
- shard-iclb: [PASS][9] -> [FAIL][10] ([fdo#103166])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-iclb5/igt@kms_plane_lowres@pipe-a-tiling-x.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-x.html
* igt@kms_psr@psr2_suspend:
- shard-iclb: [PASS][11] -> [SKIP][12] ([fdo#109441]) +2 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-iclb2/igt@kms_psr@psr2_suspend.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-iclb6/igt@kms_psr@psr2_suspend.html
* igt@kms_setmode@basic:
- shard-apl: [PASS][13] -> [FAIL][14] ([fdo#99912])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-apl1/igt@kms_setmode@basic.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-apl5/igt@kms_setmode@basic.html
- shard-kbl: [PASS][15] -> [FAIL][16] ([fdo#99912])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-kbl3/igt@kms_setmode@basic.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-kbl7/igt@kms_setmode@basic.html
#### Possible fixes ####
* igt@gem_workarounds@suspend-resume:
- shard-apl: [DMESG-WARN][17] ([fdo#108566]) -> [PASS][18] +1 similar issue
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-apl5/igt@gem_workarounds@suspend-resume.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-apl3/igt@gem_workarounds@suspend-resume.html
* igt@i915_pm_rps@min-max-config-idle:
- shard-iclb: [INCOMPLETE][19] ([fdo#107713]) -> [PASS][20]
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-iclb1/igt@i915_pm_rps@min-max-config-idle.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-iclb3/igt@i915_pm_rps@min-max-config-idle.html
* igt@kms_cursor_legacy@cursor-vs-flip-atomic:
- shard-hsw: [FAIL][21] ([fdo#103355]) -> [PASS][22]
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-hsw6/igt@kms_cursor_legacy@cursor-vs-flip-atomic.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-hsw1/igt@kms_cursor_legacy@cursor-vs-flip-atomic.html
* igt@kms_fbcon_fbt@psr-suspend:
- shard-skl: [INCOMPLETE][23] ([fdo#104108]) -> [PASS][24]
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-skl6/igt@kms_fbcon_fbt@psr-suspend.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-skl1/igt@kms_fbcon_fbt@psr-suspend.html
* igt@kms_flip@flip-vs-expired-vblank-interruptible:
- shard-skl: [FAIL][25] ([fdo#105363]) -> [PASS][26]
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-skl4/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-skl1/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
- shard-glk: [FAIL][27] ([fdo#105363]) -> [PASS][28]
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-glk3/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-glk5/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
* igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
- shard-iclb: [FAIL][29] ([fdo#103167]) -> [PASS][30]
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
* igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
- shard-skl: [FAIL][31] ([fdo#108145] / [fdo#110403]) -> [PASS][32]
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-skl7/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-skl10/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
* igt@kms_plane_lowres@pipe-a-tiling-y:
- shard-iclb: [FAIL][33] ([fdo#103166]) -> [PASS][34]
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-y.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-iclb3/igt@kms_plane_lowres@pipe-a-tiling-y.html
* igt@kms_psr2_su@frontbuffer:
- shard-iclb: [SKIP][35] ([fdo#109642]) -> [PASS][36]
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-iclb4/igt@kms_psr2_su@frontbuffer.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
* igt@kms_psr@psr2_cursor_mmap_cpu:
- shard-iclb: [SKIP][37] ([fdo#109441]) -> [PASS][38] +3 similar issues
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-iclb7/igt@kms_psr@psr2_cursor_mmap_cpu.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-iclb2/igt@kms_psr@psr2_cursor_mmap_cpu.html
* igt@perf@oa-exponents:
- shard-glk: [FAIL][39] ([fdo#105483]) -> [PASS][40]
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6398/shard-glk6/igt@perf@oa-exponents.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13497/shard-glk8/igt@perf@oa-exponents.html
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103355]: https://bugs.freedesktop.org/show_bug.cgi?id=103355
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#105483]: https://bugs.freedesktop.org/show_bug.cgi?id=105483
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
[fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
[fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
Participating hosts (10 -> 10)
------------------------------
No changes in participating hosts
Build changes
-------------
* Linux: CI_DRM_6398 -> Patchwork_13497
CI_DRM_6398: 9b9df28dc0ec04a7fb1a020d869ef0ea14be4d14 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5079: 873df2fa9e8f5fd02d4532b30ef2579f4fe4f27f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_13497: 547860d7ee4f3d1bc1460defcc932cf08755fee6 @ 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_13497/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drm/i915/guc: Remove preemption support for current fw
2019-07-02 20:09 [PATCH 1/3] drm/i915/guc: Remove preemption support for current fw Daniele Ceraolo Spurio
` (4 preceding siblings ...)
2019-07-03 22:59 ` ✓ Fi.CI.IGT: " Patchwork
@ 2019-07-09 0:45 ` Matthew Brost
5 siblings, 0 replies; 10+ messages in thread
From: Matthew Brost @ 2019-07-09 0:45 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx
On Tue, Jul 02, 2019 at 01:09:45PM -0700, Daniele Ceraolo Spurio wrote:
>From: Chris Wilson <chris@chris-wilson.co.uk>
>
>Preemption via GuC submission is not being supported with its current
>legacy incarnation. The current FW does support a similar pre-emption
>flow via H2G, but it is class-based instead of being instance-based,
>which doesn't fit well with the i915 tracking. To fix this, the
>firmware is being updated to better support our needs with a new flow,
>so we can safely remove the old code.
>
>v2 (Daniele): resurrect & rebase, reword commit message, remove
>preempt_context as well
>
>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>Cc: Matthew Brost <matthew.brost@intel.com>
>Cc: John Harrison <John.C.Harrison@Intel.com>
>---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 17 --
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 13 --
> drivers/gpu/drm/i915/gt/intel_engine_types.h | 1 -
> drivers/gpu/drm/i915/gt/intel_gt_pm.c | 4 -
> drivers/gpu/drm/i915/i915_debugfs.c | 5 -
> drivers/gpu/drm/i915/i915_drv.h | 2 -
> drivers/gpu/drm/i915/intel_guc.c | 31 ---
> drivers/gpu/drm/i915/intel_guc.h | 9 -
> drivers/gpu/drm/i915/intel_guc_submission.c | 220 +------------------
> drivers/gpu/drm/i915/selftests/intel_guc.c | 31 +--
> 10 files changed, 14 insertions(+), 319 deletions(-)
>
Nothing in this patch conflicts with the updates to the firmware/driver I'm
working on to better support our needs.
Acked-by: Matthew Brost <matthew.brost@intel.com>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>index 8a9787cf0cd0..9c695910bc43 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>@@ -644,18 +644,12 @@ static void init_contexts(struct drm_i915_private *i915)
> init_llist_head(&i915->contexts.free_list);
> }
>
>-static bool needs_preempt_context(struct drm_i915_private *i915)
>-{
>- return USES_GUC_SUBMISSION(i915);
>-}
>-
> int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> {
> struct i915_gem_context *ctx;
>
> /* Reassure ourselves we are only called once */
> GEM_BUG_ON(dev_priv->kernel_context);
>- GEM_BUG_ON(dev_priv->preempt_context);
>
> intel_engine_init_ctx_wa(dev_priv->engine[RCS0]);
> init_contexts(dev_priv);
>@@ -677,15 +671,6 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> GEM_BUG_ON(!atomic_read(&ctx->hw_id_pin_count));
> dev_priv->kernel_context = ctx;
>
>- /* highest priority; preempting task */
>- if (needs_preempt_context(dev_priv)) {
>- ctx = i915_gem_context_create_kernel(dev_priv, INT_MAX);
>- if (!IS_ERR(ctx))
>- dev_priv->preempt_context = ctx;
>- else
>- DRM_ERROR("Failed to create preempt context; disabling preemption\n");
>- }
>-
> DRM_DEBUG_DRIVER("%s context support initialized\n",
> DRIVER_CAPS(dev_priv)->has_logical_contexts ?
> "logical" : "fake");
>@@ -696,8 +681,6 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
> {
> lockdep_assert_held(&i915->drm.struct_mutex);
>
>- if (i915->preempt_context)
>- destroy_kernel_context(&i915->preempt_context);
> destroy_kernel_context(&i915->kernel_context);
>
> /* Must free all deferred contexts (via flush_workqueue) first */
>diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>index d1508f0b4c84..55b11409c1f0 100644
>--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>@@ -842,15 +842,6 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
> if (ret)
> return ret;
>
>- /*
>- * Similarly the preempt context must always be available so that
>- * we can interrupt the engine at any time. However, as preemption
>- * is optional, we allow it to fail.
>- */
>- if (i915->preempt_context)
>- pin_context(i915->preempt_context, engine,
>- &engine->preempt_context);
>-
> ret = measure_breadcrumb_dw(engine);
> if (ret < 0)
> goto err_unpin;
>@@ -862,8 +853,6 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
> return 0;
>
> err_unpin:
>- if (engine->preempt_context)
>- intel_context_unpin(engine->preempt_context);
> intel_context_unpin(engine->kernel_context);
> return ret;
> }
>@@ -888,8 +877,6 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
> if (engine->default_state)
> i915_gem_object_put(engine->default_state);
>
>- if (engine->preempt_context)
>- intel_context_unpin(engine->preempt_context);
> intel_context_unpin(engine->kernel_context);
> GEM_BUG_ON(!llist_empty(&engine->barrier_tasks));
>
>diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>index 7e056114344e..8be63019d707 100644
>--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
>+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>@@ -288,7 +288,6 @@ struct intel_engine_cs {
> struct llist_head barrier_tasks;
>
> struct intel_context *kernel_context; /* pinned */
>- struct intel_context *preempt_context; /* pinned; optional */
>
> intel_engine_mask_t saturated; /* submitting semaphores too late? */
>
>diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>index 36ba80e6a0b7..da81b3a92d16 100644
>--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>@@ -145,10 +145,6 @@ int intel_gt_resume(struct intel_gt *gt)
> if (ce)
> ce->ops->reset(ce);
>
>- ce = engine->preempt_context;
>- if (ce)
>- ce->ops->reset(ce);
>-
> engine->serial++; /* kernel context lost */
> err = engine->resume(engine);
>
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>index eeecdad0e3ca..02eaa15d47c0 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -2017,11 +2017,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/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index 02dd9f9f3a89..1cdd06539dc5 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -1378,8 +1378,6 @@ struct drm_i915_private {
> struct intel_engine_cs *engine[I915_NUM_ENGINES];
> /* Context used internally to idle the GPU and setup initial state */
> struct i915_gem_context *kernel_context;
>- /* Context only to be used for injecting preemption commands */
>- struct i915_gem_context *preempt_context;
> struct intel_engine_cs *engine_class[MAX_ENGINE_CLASS + 1]
> [MAX_ENGINE_INSTANCE + 1];
>
>diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
>index c40a6efdd33a..501b74f44374 100644
>--- a/drivers/gpu/drm/i915/intel_guc.c
>+++ b/drivers/gpu/drm/i915/intel_guc.c
>@@ -101,8 +101,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
>@@ -122,31 +120,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;
> }
>
>@@ -154,10 +127,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 d91c96679dbb..ec1038c1f50e 100644
>--- a/drivers/gpu/drm/i915/intel_guc.h
>+++ b/drivers/gpu/drm/i915/intel_guc.h
>@@ -37,11 +37,6 @@
>
> struct __guc_ads_blob;
>
>-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
>@@ -76,10 +71,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 12c22359fdac..8520bb224175 100644
>--- a/drivers/gpu/drm/i915/intel_guc_submission.c
>+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
>@@ -46,11 +46,10 @@ enum {
> *
> * GuC client:
> * A intel_guc_client refers to a submission path through GuC. Currently, there
>- * are two clients. One of them (the execbuf_client) is charged with all
>- * submissions to the GuC, the other one (preempt_client) is responsible for
>- * preempting the execbuf_client. This struct is the owner of a doorbell, a
>- * process descriptor and a workqueue (all of them inside a single gem object
>- * that contains all required pages for these elements).
>+ * is only one client, which is charged with all submissions to the GuC. This
>+ * struct is the owner of a doorbell, a process descriptor and a workqueue (all
>+ * of them inside a single gem object that contains all required pages for these
>+ * elements).
> *
> * GuC stage descriptor:
> * During initialization, the driver allocates a static pool of 1024 such
>@@ -88,12 +87,6 @@ enum {
> *
> */
>
>-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);
>@@ -563,126 +556,6 @@ static void flush_ggtt_writes(struct i915_vma *vma)
> intel_uncore_posting_read_fw(&i915->uncore, 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 = engine->preempt_context;
>- 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->class == RENDER_CLASS) {
>- 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,
>- 0);
>- *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)))) {
>- intel_write_status_page(engine,
>- I915_GEM_HWS_PREEMPT,
>- GUC_PREEMPT_NONE);
>- 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];
>-
>- if (wait_for_atomic(report->report_return_status ==
>- INTEL_GUC_REPORT_STATUS_COMPLETE,
>- GUC_PREEMPT_POSTPROCESS_DELAY_MS))
>- DRM_ERROR("Timed out waiting for GuC preemption report\n");
>- /*
>- * 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;
>-
>- 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, GUC_PREEMPT_NONE);
>-}
>-
> static void guc_submit(struct intel_engine_cs *engine,
> struct i915_request **out,
> struct i915_request **end)
>@@ -742,21 +615,6 @@ static void __guc_dequeue(struct intel_engine_cs *engine)
> lockdep_assert_held(&engine->active.lock);
>
> if (last) {
>- 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 (i915_scheduler_need_preempt(prio, rq_prio(last))) {
>- intel_write_status_page(engine,
>- I915_GEM_HWS_PREEMPT,
>- GUC_PREEMPT_INPROGRESS);
>- queue_work(engine->i915->guc.preempt_wq,
>- &preempt_work->work);
>- return;
>- }
>- }
>-
> if (*++first)
> return;
>
>@@ -820,12 +678,7 @@ static void guc_submission_tasklet(unsigned long data)
> memmove(execlists->inflight, port, rem * sizeof(*port));
> }
>
>- if (intel_read_status_page(engine, I915_GEM_HWS_PREEMPT) ==
>- GUC_PREEMPT_FINISHED)
>- complete_preempt_context(engine);
>-
>- if (!intel_read_status_page(engine, I915_GEM_HWS_PREEMPT))
>- __guc_dequeue(engine);
>+ __guc_dequeue(engine);
>
> spin_unlock_irqrestore(&engine->active.lock, flags);
> }
>@@ -846,16 +699,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);
> }
>
> static void guc_reset(struct intel_engine_cs *engine, bool stalled)
>@@ -1112,7 +955,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)->engine_mask,
>@@ -1124,20 +966,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)->engine_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;
> }
>
>@@ -1145,10 +973,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);
>@@ -1191,28 +1015,11 @@ static void __guc_client_disable(struct intel_guc_client *client)
>
> static int guc_clients_enable(struct intel_guc *guc)
> {
>- int ret;
>-
>- ret = __guc_client_enable(guc->execbuf_client);
>- 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;
>+ return __guc_client_enable(guc->execbuf_client);
> }
>
> 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);
> }
>@@ -1223,9 +1030,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)
>@@ -1245,11 +1049,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:
>@@ -1259,13 +1058,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));
>
>diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c
>index 6ca8584cd64c..1a1915e44f6b 100644
>--- a/drivers/gpu/drm/i915/selftests/intel_guc.c
>+++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
>@@ -103,13 +103,10 @@ static int ring_doorbell_nop(struct intel_guc_client *client)
> /*
> * Basic client sanity check, handy to validate create_clients.
> */
>-static int validate_client(struct intel_guc_client *client,
>- int client_priority,
>- bool is_preempt_client)
>+static int validate_client(struct intel_guc_client *client, int client_priority)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(client->guc);
>- struct i915_gem_context *ctx_owner = is_preempt_client ?
>- dev_priv->preempt_context : dev_priv->kernel_context;
>+ struct i915_gem_context *ctx_owner = dev_priv->kernel_context;
>
> if (client->owner != ctx_owner ||
> client->engines != INTEL_INFO(dev_priv)->engine_mask ||
>@@ -163,7 +160,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;
>@@ -177,24 +174,14 @@ static int igt_guc_clients(void *args)
> GEM_BUG_ON(!guc->execbuf_client);
>
> err = validate_client(guc->execbuf_client,
>- GUC_CLIENT_PRIORITY_KMD_NORMAL, false);
>+ GUC_CLIENT_PRIORITY_KMD_NORMAL);
> if (err) {
> pr_err("execbug client validation failed\n");
> 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))) {
>+ /* the client should now have reserved a doorbell */
>+ if (!has_doorbell(guc->execbuf_client)) {
> pr_err("guc_clients_create didn't reserve doorbells\n");
> err = -EINVAL;
> goto out;
>@@ -204,8 +191,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;
>@@ -300,8 +286,7 @@ static int igt_guc_doorbells(void *arg)
> goto out;
> }
>
>- err = validate_client(clients[i],
>- i % GUC_CLIENT_PRIORITY_NUM, false);
>+ err = validate_client(clients[i], i % GUC_CLIENT_PRIORITY_NUM);
> if (err) {
> pr_err("[%d] client_alloc sanity check failed!\n", i);
> err = -EINVAL;
>--
>2.20.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drm/i915/guc: simplify guc client
2019-07-02 20:09 ` [PATCH 2/3] drm/i915/guc: simplify guc client Daniele Ceraolo Spurio
@ 2019-07-09 0:53 ` Matthew Brost
0 siblings, 0 replies; 10+ messages in thread
From: Matthew Brost @ 2019-07-09 0:53 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx
On Tue, Jul 02, 2019 at 01:09:46PM -0700, Daniele Ceraolo Spurio wrote:
>We originally added support, in some cases partial, for different modes
>of operations via guc clients:
>
>- proxy vs direct submission;
>- variable engine mask per-client.
>
>We only ever used one flow (all submissions via a single proxy), so the
>other code paths haven't been exercised and are most likely
>non-functional. The guc firmware interface is also in the process of
>being updated to better fit the i915 flow and our client abstraction
>will need to change accordingly (or possibly go away entirely), so these
>old unused paths can be considered dead and removed.
>
>Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>Cc: Matthew Brost <matthew.brost@intel.com>
>Cc: John Harrison <John.C.Harrison@Intel.com>
>---
> drivers/gpu/drm/i915/i915_debugfs.c | 3 +-
> drivers/gpu/drm/i915/intel_guc_submission.c | 73 ++-------------------
> drivers/gpu/drm/i915/intel_guc_submission.h | 2 -
> drivers/gpu/drm/i915/selftests/intel_guc.c | 12 +---
> 4 files changed, 8 insertions(+), 82 deletions(-)
>
The client abstraction is likely going away in when the firmware interface is
reworked so this patch shouldn't interface with any of those changes.
Acked-by: Matthew Brost <Matthew Brost <matthew.brost@intel.com>
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>index 02eaa15d47c0..65ddb24a0f4b 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -2028,7 +2028,6 @@ static int i915_guc_stage_pool(struct seq_file *m, void *data)
> struct drm_i915_private *dev_priv = node_to_i915(m->private);
> const struct intel_guc *guc = &dev_priv->guc;
> struct guc_stage_desc *desc = guc->stage_desc_pool_vaddr;
>- struct intel_guc_client *client = guc->execbuf_client;
> intel_engine_mask_t tmp;
> int index;
>
>@@ -2058,7 +2057,7 @@ static int i915_guc_stage_pool(struct seq_file *m, void *data)
> desc->wq_addr, desc->wq_size);
> seq_putc(m, '\n');
>
>- for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
>+ for_each_engine(engine, dev_priv, tmp) {
> u32 guc_engine_id = engine->guc_id;
> struct guc_execlist_context *lrc =
> &desc->lrc[guc_engine_id];
>diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
>index 8520bb224175..30692f8289bd 100644
>--- a/drivers/gpu/drm/i915/intel_guc_submission.c
>+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
>@@ -363,10 +363,7 @@ static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
> static void guc_stage_desc_init(struct intel_guc_client *client)
> {
> struct intel_guc *guc = client->guc;
>- struct i915_gem_context *ctx = client->owner;
>- struct i915_gem_engines_iter it;
> struct guc_stage_desc *desc;
>- struct intel_context *ce;
> u32 gfx_addr;
>
> desc = __get_stage_desc(client);
>@@ -380,55 +377,6 @@ static void guc_stage_desc_init(struct intel_guc_client *client)
> desc->priority = client->priority;
> desc->db_id = client->doorbell_id;
>
>- for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
>- struct guc_execlist_context *lrc;
>-
>- if (!(ce->engine->mask & client->engines))
>- continue;
>-
>- /* TODO: We have a design issue to be solved here. Only when we
>- * receive the first batch, we know which engine is used by the
>- * user. But here GuC expects the lrc and ring to be pinned. It
>- * is not an issue for default context, which is the only one
>- * for now who owns a GuC client. But for future owner of GuC
>- * client, need to make sure lrc is pinned prior to enter here.
>- */
>- if (!ce->state)
>- break; /* XXX: continue? */
>-
>- /*
>- * XXX: When this is a GUC_STAGE_DESC_ATTR_KERNEL client (proxy
>- * submission or, in other words, not using a direct submission
>- * model) the KMD's LRCA is not used for any work submission.
>- * Instead, the GuC uses the LRCA of the user mode context (see
>- * guc_add_request below).
>- */
>- lrc = &desc->lrc[ce->engine->guc_id];
>- lrc->context_desc = lower_32_bits(ce->lrc_desc);
>-
>- /* The state page is after PPHWSP */
>- lrc->ring_lrca = intel_guc_ggtt_offset(guc, ce->state) +
>- LRC_STATE_PN * PAGE_SIZE;
>-
>- /* XXX: In direct submission, the GuC wants the HW context id
>- * here. In proxy submission, it wants the stage id
>- */
>- lrc->context_id = (client->stage_id << GUC_ELC_CTXID_OFFSET) |
>- (ce->engine->guc_id << GUC_ELC_ENGINE_OFFSET);
>-
>- lrc->ring_begin = intel_guc_ggtt_offset(guc, ce->ring->vma);
>- lrc->ring_end = lrc->ring_begin + ce->ring->size - 1;
>- lrc->ring_next_free_location = lrc->ring_begin;
>- lrc->ring_current_tail_pointer_value = 0;
>-
>- desc->engines_used |= BIT(ce->engine->guc_id);
>- }
>- i915_gem_context_unlock_engines(ctx);
>-
>- DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
>- client->engines, desc->engines_used);
>- WARN_ON(desc->engines_used == 0);
>-
> /*
> * The doorbell, process descriptor, and workqueue are all parts
> * of the client object, which the GuC will reference via the GGTT
>@@ -836,8 +784,7 @@ static bool guc_verify_doorbells(struct intel_guc *guc)
>
> /**
> * guc_client_alloc() - Allocate an intel_guc_client
>- * @dev_priv: driver private data structure
>- * @engines: The set of engines to enable for this client
>+ * @guc: the intel_guc structure
> * @priority: four levels priority _CRITICAL, _HIGH, _NORMAL and _LOW
> * The kernel client to replace ExecList submission is created with
> * NORMAL priority. Priority of a client for scheduler can be HIGH,
>@@ -848,13 +795,9 @@ static bool guc_verify_doorbells(struct intel_guc *guc)
> * Return: An intel_guc_client object if success, else NULL.
> */
> static struct intel_guc_client *
>-guc_client_alloc(struct drm_i915_private *dev_priv,
>- u32 engines,
>- u32 priority,
>- struct i915_gem_context *ctx)
>+guc_client_alloc(struct intel_guc *guc, u32 priority)
> {
> struct intel_guc_client *client;
>- struct intel_guc *guc = &dev_priv->guc;
> struct i915_vma *vma;
> void *vaddr;
> int ret;
>@@ -864,8 +807,6 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
> return ERR_PTR(-ENOMEM);
>
> client->guc = guc;
>- client->owner = ctx;
>- client->engines = engines;
> client->priority = priority;
> client->doorbell_id = GUC_DOORBELL_INVALID;
> spin_lock_init(&client->wq_lock);
>@@ -910,8 +851,8 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
> else
> client->proc_desc_offset = (GUC_DB_SIZE / 2);
>
>- DRM_DEBUG_DRIVER("new priority %u client %p for engine(s) 0x%x: stage_id %u\n",
>- priority, client, client->engines, client->stage_id);
>+ DRM_DEBUG_DRIVER("new priority %u client %p: stage_id %u\n",
>+ priority, client, client->stage_id);
> DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%lx\n",
> client->doorbell_id, client->doorbell_offset);
>
>@@ -951,15 +892,11 @@ static inline bool ctx_save_restore_disabled(struct intel_context *ce)
>
> static int guc_clients_create(struct intel_guc *guc)
> {
>- struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct intel_guc_client *client;
>
> GEM_BUG_ON(guc->execbuf_client);
>
>- client = guc_client_alloc(dev_priv,
>- INTEL_INFO(dev_priv)->engine_mask,
>- GUC_CLIENT_PRIORITY_KMD_NORMAL,
>- dev_priv->kernel_context);
>+ client = guc_client_alloc(guc, GUC_CLIENT_PRIORITY_KMD_NORMAL);
> if (IS_ERR(client)) {
> DRM_ERROR("Failed to create GuC client for submission!\n");
> return PTR_ERR(client);
>diff --git a/drivers/gpu/drm/i915/intel_guc_submission.h b/drivers/gpu/drm/i915/intel_guc_submission.h
>index 7d823a513b9c..87a38cb6faf3 100644
>--- a/drivers/gpu/drm/i915/intel_guc_submission.h
>+++ b/drivers/gpu/drm/i915/intel_guc_submission.h
>@@ -58,11 +58,9 @@ struct drm_i915_private;
> struct intel_guc_client {
> struct i915_vma *vma;
> void *vaddr;
>- struct i915_gem_context *owner;
> struct intel_guc *guc;
>
> /* bitmap of (host) engine ids */
>- u32 engines;
> u32 priority;
> u32 stage_id;
> u32 proc_desc_offset;
>diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c
>index 1a1915e44f6b..6ca76f5a98d4 100644
>--- a/drivers/gpu/drm/i915/selftests/intel_guc.c
>+++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
>@@ -105,12 +105,7 @@ static int ring_doorbell_nop(struct intel_guc_client *client)
> */
> static int validate_client(struct intel_guc_client *client, int client_priority)
> {
>- struct drm_i915_private *dev_priv = guc_to_i915(client->guc);
>- struct i915_gem_context *ctx_owner = dev_priv->kernel_context;
>-
>- if (client->owner != ctx_owner ||
>- client->engines != INTEL_INFO(dev_priv)->engine_mask ||
>- client->priority != client_priority ||
>+ if (client->priority != client_priority ||
> client->doorbell_id == GUC_DOORBELL_INVALID)
> return -EINVAL;
> else
>@@ -247,10 +242,7 @@ static int igt_guc_doorbells(void *arg)
> goto unlock;
>
> for (i = 0; i < ATTEMPTS; i++) {
>- clients[i] = guc_client_alloc(dev_priv,
>- INTEL_INFO(dev_priv)->engine_mask,
>- i % GUC_CLIENT_PRIORITY_NUM,
>- dev_priv->kernel_context);
>+ clients[i] = guc_client_alloc(guc, i % GUC_CLIENT_PRIORITY_NUM);
>
> if (!clients[i]) {
> pr_err("[%d] No guc client\n", i);
>--
>2.20.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-07-09 0:59 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-02 20:09 [PATCH 1/3] drm/i915/guc: Remove preemption support for current fw Daniele Ceraolo Spurio
2019-07-02 20:09 ` [PATCH 2/3] drm/i915/guc: simplify guc client Daniele Ceraolo Spurio
2019-07-09 0:53 ` Matthew Brost
2019-07-02 20:09 ` [PATCH 3/3] drm/i915/uc: replace uc init/fini misc Daniele Ceraolo Spurio
2019-07-02 20:52 ` Michal Wajdeczko
2019-07-02 21:22 ` Daniele Ceraolo Spurio
2019-07-02 22:04 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915/guc: Remove preemption support for current fw Patchwork
2019-07-02 22:37 ` ✓ Fi.CI.BAT: success " Patchwork
2019-07-03 22:59 ` ✓ Fi.CI.IGT: " Patchwork
2019-07-09 0:45 ` [PATCH 1/3] " Matthew Brost
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.