| Message ID | 20181001201754.31716-1-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/guc: init GuC descriptors after GuC load | expand |
On Mon, 01 Oct 2018 22:17:53 +0200, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote: > GuC stores some data in there, which might be stale after a reset. > We already reset the WQ head and tail, but more things are being moved > to the descriptor with the interface updates. Instead of trying to track > them one by one, always memset and init the descriptors from scratch > after GuC is loaded. > The code is also reorganized so that the above operations and the > doorbell creation are grouped as "client enabling" > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > drivers/gpu/drm/i915/intel_guc_submission.c | 127 ++++++++++---------- > drivers/gpu/drm/i915/intel_uc_fw.h | 7 +- > drivers/gpu/drm/i915/selftests/intel_guc.c | 17 ++- > 3 files changed, 78 insertions(+), 73 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c > b/drivers/gpu/drm/i915/intel_guc_submission.c > index ac862b42f6a1..0806adbaa864 100644 > --- a/drivers/gpu/drm/i915/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/intel_guc_submission.c > @@ -282,8 +282,7 @@ __get_process_desc(struct intel_guc_client *client) > /* > * Initialise the process descriptor shared with the GuC firmware. > */ > -static void guc_proc_desc_init(struct intel_guc *guc, > - struct intel_guc_client *client) > +static void guc_proc_desc_init(struct intel_guc_client *client) > { > struct guc_process_desc *desc; > @@ -341,9 +340,9 @@ static void guc_stage_desc_pool_destroy(struct > intel_guc *guc) > * data structures relating to this client (doorbell, process > descriptor, > * write queue, etc). > */ > -static void guc_stage_desc_init(struct intel_guc *guc, > - struct intel_guc_client *client) > +static void guc_stage_desc_init(struct intel_guc_client *client) > { > + struct intel_guc *guc = client->guc; > struct drm_i915_private *dev_priv = guc_to_i915(guc); > struct intel_engine_cs *engine; > struct i915_gem_context *ctx = client->owner; > @@ -424,8 +423,7 @@ static void guc_stage_desc_init(struct intel_guc > *guc, > desc->desc_private = ptr_to_u64(client); > } > -static void guc_stage_desc_fini(struct intel_guc *guc, > - struct intel_guc_client *client) > +static void guc_stage_desc_fini(struct intel_guc_client *client) > { > struct guc_stage_desc *desc; > @@ -486,14 +484,6 @@ static void guc_wq_item_append(struct > intel_guc_client *client, > WRITE_ONCE(desc->tail, (wq_off + wqi_size) & (GUC_WQ_SIZE - 1)); > } > -static void guc_reset_wq(struct intel_guc_client *client) > -{ > - struct guc_process_desc *desc = __get_process_desc(client); > - > - desc->head = 0; > - desc->tail = 0; > -} > - > static void guc_ring_doorbell(struct intel_guc_client *client) > { > struct guc_doorbell_info *db; > @@ -898,45 +888,6 @@ static bool guc_verify_doorbells(struct intel_guc > *guc) > return true; > } > -static int guc_clients_doorbell_init(struct intel_guc *guc) > -{ > - int ret; > - > - ret = create_doorbell(guc->execbuf_client); > - if (ret) > - return ret; > - > - if (guc->preempt_client) { > - ret = create_doorbell(guc->preempt_client); > - if (ret) { > - destroy_doorbell(guc->execbuf_client); > - return ret; > - } > - } > - > - return 0; > -} > - > -static void guc_clients_doorbell_fini(struct intel_guc *guc) > -{ > - /* > - * By the time we're here, GuC has already been reset. > - * Instead of trying (in vain) to communicate with it, let's just > - * cleanup the doorbell HW and our internal state. > - */ > - if (guc->preempt_client) { > - __destroy_doorbell(guc->preempt_client); > - __update_doorbell_desc(guc->preempt_client, > - GUC_DOORBELL_INVALID); > - } > - > - if (guc->execbuf_client) { > - __destroy_doorbell(guc->execbuf_client); > - __update_doorbell_desc(guc->execbuf_client, > - GUC_DOORBELL_INVALID); > - } > -} > - > /** > * guc_client_alloc() - Allocate an intel_guc_client > * @dev_priv: driver private data structure > @@ -1009,9 +960,6 @@ guc_client_alloc(struct drm_i915_private *dev_priv, > else > client->proc_desc_offset = (GUC_DB_SIZE / 2); > - guc_proc_desc_init(guc, client); > - guc_stage_desc_init(guc, client); > - > ret = reserve_doorbell(client); > if (ret) > goto err_vaddr; > @@ -1037,7 +985,6 @@ guc_client_alloc(struct drm_i915_private *dev_priv, > static void guc_client_free(struct intel_guc_client *client) > { > unreserve_doorbell(client); > - guc_stage_desc_fini(client->guc, client); > i915_vma_unpin_and_release(&client->vma, I915_VMA_RELEASE_MAP); > ida_simple_remove(&client->guc->stage_ids, client->stage_id); > kfree(client); > @@ -1104,6 +1051,64 @@ static void guc_clients_destroy(struct intel_guc > *guc) > guc_client_free(client); > } > +static int __guc_client_enable(struct intel_guc_client *client) > +{ > + int ret = 0; initialization is not needed here > + > + guc_proc_desc_init(client); > + guc_stage_desc_init(client); > + > + ret = create_doorbell(client); > + if (ret) > + guc_stage_desc_fini(client); > + > + return ret; > +} > + > +static void __guc_client_disable(struct intel_guc_client *client) > +{ > + /* > + * By the time we're here, GuC may have already been reset. if that is > + * the case, instead of trying (in vain) to communicate with it, let's > + * just cleanup the doorbell HW and our internal state. > + */ > + extra new line > + if (intel_uc_fw_is_loaded(&client->guc->fw)) what about adding one more helper in intel_guc.h: static inline bool intel_guc_is_alive(struct intel_guc *guc) { return intel_uc_fw_is_loaded(&guc->fw); } and use it here as: if (intel_guc_is_alive(client->guc)) > + destroy_doorbell(client); > + else > + __destroy_doorbell(client); > + > + guc_stage_desc_fini(client); > +} > + > +static int guc_clients_enable(struct intel_guc *guc) > +{ > + int ret = 0; initialization is not needed here > + > + 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; > +} > + > +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); > +} > + > /* > * Set up the memory resources to be shared with the GuC (via the GGTT) > * at firmware loading time. > @@ -1287,15 +1292,11 @@ int intel_guc_submission_enable(struct intel_guc > *guc) > GEM_BUG_ON(!guc->execbuf_client); > - guc_reset_wq(guc->execbuf_client); > - if (guc->preempt_client) > - guc_reset_wq(guc->preempt_client); > - > err = intel_guc_sample_forcewake(guc); > if (err) > return err; > - err = guc_clients_doorbell_init(guc); > + err = guc_clients_enable(guc); > if (err) > return err; > @@ -1317,7 +1318,7 @@ void intel_guc_submission_disable(struct intel_guc > *guc) > GEM_BUG_ON(dev_priv->gt.awake); /* GT should be parked first */ > guc_interrupts_release(dev_priv); > - guc_clients_doorbell_fini(guc); > + guc_clients_disable(guc); > } > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h > b/drivers/gpu/drm/i915/intel_uc_fw.h > index 87910aa83267..0e3bd580e267 100644 > --- a/drivers/gpu/drm/i915/intel_uc_fw.h > +++ b/drivers/gpu/drm/i915/intel_uc_fw.h > @@ -115,9 +115,14 @@ static inline bool intel_uc_fw_is_selected(struct > intel_uc_fw *uc_fw) > return uc_fw->path != NULL; > } > +static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw) > +{ > + return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS; > +} > + > static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) > { > - if (uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS) > + if (intel_uc_fw_is_loaded(uc_fw)) > uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING; > } > diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c > b/drivers/gpu/drm/i915/selftests/intel_guc.c > index 0c0ab82b6228..bf27162fb327 100644 > --- a/drivers/gpu/drm/i915/selftests/intel_guc.c > +++ b/drivers/gpu/drm/i915/selftests/intel_guc.c > @@ -159,6 +159,7 @@ static int igt_guc_clients(void *args) > * Get rid of clients created during driver load because the test will > * recreate them. > */ > + guc_clients_disable(guc); > guc_clients_destroy(guc); > if (guc->execbuf_client || guc->preempt_client) { > pr_err("guc_clients_destroy lied!\n"); > @@ -197,8 +198,8 @@ static int igt_guc_clients(void *args) > goto out; > } > - /* Now create the doorbells */ > - guc_clients_doorbell_init(guc); > + /* Now enable the clients */ > + guc_clients_enable(guc); > /* each client should now have received a doorbell */ > if (!client_doorbell_in_sync(guc->execbuf_client) || > @@ -212,7 +213,7 @@ static int igt_guc_clients(void *args) > * Basic test - an attempt to reallocate a valid doorbell to the > * client it is currently assigned should not cause a failure. > */ > - err = guc_clients_doorbell_init(guc); > + err = create_doorbell(guc->execbuf_client); > if (err) > goto out; > @@ -263,12 +264,10 @@ static int igt_guc_clients(void *args) > * Leave clean state for other test, plus the driver always destroy the > * clients during unload. > */ > - destroy_doorbell(guc->execbuf_client); > - if (guc->preempt_client) > - destroy_doorbell(guc->preempt_client); > + guc_clients_disable(guc); > guc_clients_destroy(guc); > guc_clients_create(guc); > - guc_clients_doorbell_init(guc); > + guc_clients_enable(guc); > unlock: > intel_runtime_pm_put(dev_priv); > mutex_unlock(&dev_priv->drm.struct_mutex); > @@ -352,7 +351,7 @@ static int igt_guc_doorbells(void *arg) > db_id = clients[i]->doorbell_id; > - err = create_doorbell(clients[i]); > + err = __guc_client_enable(clients[i]); > if (err) { > pr_err("[%d] Failed to create a doorbell\n", i); > goto out; > @@ -378,7 +377,7 @@ static int igt_guc_doorbells(void *arg) > out: > for (i = 0; i < ATTEMPTS; i++) > if (!IS_ERR_OR_NULL(clients[i])) { > - destroy_doorbell(clients[i]); > + __guc_client_disable(clients[i]); > guc_client_free(clients[i]); > } > unlock: with above small updates, Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Michal
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c index ac862b42f6a1..0806adbaa864 100644 --- a/drivers/gpu/drm/i915/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/intel_guc_submission.c @@ -282,8 +282,7 @@ __get_process_desc(struct intel_guc_client *client) /* * Initialise the process descriptor shared with the GuC firmware. */ -static void guc_proc_desc_init(struct intel_guc *guc, - struct intel_guc_client *client) +static void guc_proc_desc_init(struct intel_guc_client *client) { struct guc_process_desc *desc; @@ -341,9 +340,9 @@ static void guc_stage_desc_pool_destroy(struct intel_guc *guc) * data structures relating to this client (doorbell, process descriptor, * write queue, etc). */ -static void guc_stage_desc_init(struct intel_guc *guc, - struct intel_guc_client *client) +static void guc_stage_desc_init(struct intel_guc_client *client) { + struct intel_guc *guc = client->guc; struct drm_i915_private *dev_priv = guc_to_i915(guc); struct intel_engine_cs *engine; struct i915_gem_context *ctx = client->owner; @@ -424,8 +423,7 @@ static void guc_stage_desc_init(struct intel_guc *guc, desc->desc_private = ptr_to_u64(client); } -static void guc_stage_desc_fini(struct intel_guc *guc, - struct intel_guc_client *client) +static void guc_stage_desc_fini(struct intel_guc_client *client) { struct guc_stage_desc *desc; @@ -486,14 +484,6 @@ static void guc_wq_item_append(struct intel_guc_client *client, WRITE_ONCE(desc->tail, (wq_off + wqi_size) & (GUC_WQ_SIZE - 1)); } -static void guc_reset_wq(struct intel_guc_client *client) -{ - struct guc_process_desc *desc = __get_process_desc(client); - - desc->head = 0; - desc->tail = 0; -} - static void guc_ring_doorbell(struct intel_guc_client *client) { struct guc_doorbell_info *db; @@ -898,45 +888,6 @@ static bool guc_verify_doorbells(struct intel_guc *guc) return true; } -static int guc_clients_doorbell_init(struct intel_guc *guc) -{ - int ret; - - ret = create_doorbell(guc->execbuf_client); - if (ret) - return ret; - - if (guc->preempt_client) { - ret = create_doorbell(guc->preempt_client); - if (ret) { - destroy_doorbell(guc->execbuf_client); - return ret; - } - } - - return 0; -} - -static void guc_clients_doorbell_fini(struct intel_guc *guc) -{ - /* - * By the time we're here, GuC has already been reset. - * Instead of trying (in vain) to communicate with it, let's just - * cleanup the doorbell HW and our internal state. - */ - if (guc->preempt_client) { - __destroy_doorbell(guc->preempt_client); - __update_doorbell_desc(guc->preempt_client, - GUC_DOORBELL_INVALID); - } - - if (guc->execbuf_client) { - __destroy_doorbell(guc->execbuf_client); - __update_doorbell_desc(guc->execbuf_client, - GUC_DOORBELL_INVALID); - } -} - /** * guc_client_alloc() - Allocate an intel_guc_client * @dev_priv: driver private data structure @@ -1009,9 +960,6 @@ guc_client_alloc(struct drm_i915_private *dev_priv, else client->proc_desc_offset = (GUC_DB_SIZE / 2); - guc_proc_desc_init(guc, client); - guc_stage_desc_init(guc, client); - ret = reserve_doorbell(client); if (ret) goto err_vaddr; @@ -1037,7 +985,6 @@ guc_client_alloc(struct drm_i915_private *dev_priv, static void guc_client_free(struct intel_guc_client *client) { unreserve_doorbell(client); - guc_stage_desc_fini(client->guc, client); i915_vma_unpin_and_release(&client->vma, I915_VMA_RELEASE_MAP); ida_simple_remove(&client->guc->stage_ids, client->stage_id); kfree(client); @@ -1104,6 +1051,64 @@ static void guc_clients_destroy(struct intel_guc *guc) guc_client_free(client); } +static int __guc_client_enable(struct intel_guc_client *client) +{ + int ret = 0; + + guc_proc_desc_init(client); + guc_stage_desc_init(client); + + ret = create_doorbell(client); + if (ret) + guc_stage_desc_fini(client); + + return ret; +} + +static void __guc_client_disable(struct intel_guc_client *client) +{ + /* + * By the time we're here, GuC may have already been reset. if that is + * the case, instead of trying (in vain) to communicate with it, let's + * just cleanup the doorbell HW and our internal state. + */ + + if (intel_uc_fw_is_loaded(&client->guc->fw)) + destroy_doorbell(client); + else + __destroy_doorbell(client); + + guc_stage_desc_fini(client); +} + +static int guc_clients_enable(struct intel_guc *guc) +{ + int ret = 0; + + 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; +} + +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); +} + /* * Set up the memory resources to be shared with the GuC (via the GGTT) * at firmware loading time. @@ -1287,15 +1292,11 @@ int intel_guc_submission_enable(struct intel_guc *guc) GEM_BUG_ON(!guc->execbuf_client); - guc_reset_wq(guc->execbuf_client); - if (guc->preempt_client) - guc_reset_wq(guc->preempt_client); - err = intel_guc_sample_forcewake(guc); if (err) return err; - err = guc_clients_doorbell_init(guc); + err = guc_clients_enable(guc); if (err) return err; @@ -1317,7 +1318,7 @@ void intel_guc_submission_disable(struct intel_guc *guc) GEM_BUG_ON(dev_priv->gt.awake); /* GT should be parked first */ guc_interrupts_release(dev_priv); - guc_clients_doorbell_fini(guc); + guc_clients_disable(guc); } #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h index 87910aa83267..0e3bd580e267 100644 --- a/drivers/gpu/drm/i915/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/intel_uc_fw.h @@ -115,9 +115,14 @@ static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw) return uc_fw->path != NULL; } +static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw) +{ + return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS; +} + static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) { - if (uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS) + if (intel_uc_fw_is_loaded(uc_fw)) uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING; } diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c index 0c0ab82b6228..bf27162fb327 100644 --- a/drivers/gpu/drm/i915/selftests/intel_guc.c +++ b/drivers/gpu/drm/i915/selftests/intel_guc.c @@ -159,6 +159,7 @@ static int igt_guc_clients(void *args) * Get rid of clients created during driver load because the test will * recreate them. */ + guc_clients_disable(guc); guc_clients_destroy(guc); if (guc->execbuf_client || guc->preempt_client) { pr_err("guc_clients_destroy lied!\n"); @@ -197,8 +198,8 @@ static int igt_guc_clients(void *args) goto out; } - /* Now create the doorbells */ - guc_clients_doorbell_init(guc); + /* Now enable the clients */ + guc_clients_enable(guc); /* each client should now have received a doorbell */ if (!client_doorbell_in_sync(guc->execbuf_client) || @@ -212,7 +213,7 @@ static int igt_guc_clients(void *args) * Basic test - an attempt to reallocate a valid doorbell to the * client it is currently assigned should not cause a failure. */ - err = guc_clients_doorbell_init(guc); + err = create_doorbell(guc->execbuf_client); if (err) goto out; @@ -263,12 +264,10 @@ static int igt_guc_clients(void *args) * Leave clean state for other test, plus the driver always destroy the * clients during unload. */ - destroy_doorbell(guc->execbuf_client); - if (guc->preempt_client) - destroy_doorbell(guc->preempt_client); + guc_clients_disable(guc); guc_clients_destroy(guc); guc_clients_create(guc); - guc_clients_doorbell_init(guc); + guc_clients_enable(guc); unlock: intel_runtime_pm_put(dev_priv); mutex_unlock(&dev_priv->drm.struct_mutex); @@ -352,7 +351,7 @@ static int igt_guc_doorbells(void *arg) db_id = clients[i]->doorbell_id; - err = create_doorbell(clients[i]); + err = __guc_client_enable(clients[i]); if (err) { pr_err("[%d] Failed to create a doorbell\n", i); goto out; @@ -378,7 +377,7 @@ static int igt_guc_doorbells(void *arg) out: for (i = 0; i < ATTEMPTS; i++) if (!IS_ERR_OR_NULL(clients[i])) { - destroy_doorbell(clients[i]); + __guc_client_disable(clients[i]); guc_client_free(clients[i]); } unlock:
GuC stores some data in there, which might be stale after a reset. We already reset the WQ head and tail, but more things are being moved to the descriptor with the interface updates. Instead of trying to track them one by one, always memset and init the descriptors from scratch after GuC is loaded. The code is also reorganized so that the above operations and the doorbell creation are grouped as "client enabling" Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/intel_guc_submission.c | 127 ++++++++++---------- drivers/gpu/drm/i915/intel_uc_fw.h | 7 +- drivers/gpu/drm/i915/selftests/intel_guc.c | 17 ++- 3 files changed, 78 insertions(+), 73 deletions(-)