[Intel-gfx] [PATCH 1/2] drm/i915/guc: init GuC descriptors after GuC load
Michal Wajdeczko
michal.wajdeczko at intel.com
Tue Oct 2 11:02:25 UTC 2018
On Mon, 01 Oct 2018 22:17:53 +0200, Daniele Ceraolo Spurio
<daniele.ceraolospurio at 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 at intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at 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 at intel.com>
Michal
More information about the Intel-gfx
mailing list