Re: [PATCH v2] drm/i915: Sanitize GuC client initialization

[Oscar Mateo <oscar.mateo@intel.com>]

On 02/14/2017 05:53 AM, Joonas Lahtinen wrote:
> Started adding proper teardown to guc_client_alloc, ended up removing
> quite a few dead ends where errors communicating with the GuC were
> silently ignored. There also seemed to be quite a few erronous
> teardown actions performed in case of an error (ordering wrong).
>
> v2:
> 	- Increase function symmetry/proximity (Michal/Daniele)
> 	- Fix __reserve_doorbell accounting for high priority (Daniele)
> 	- Call __update_doorbell_desc! (Daniele)
> 	- Isolate __guc_{,de}allocate_doorbell (Michal/Daniele)
>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c        |   4 +-
>   drivers/gpu/drm/i915/i915_guc_submission.c | 371 ++++++++++++++++-------------
>   drivers/gpu/drm/i915/intel_guc_fwif.h      |   4 +-
>   drivers/gpu/drm/i915/intel_uc.h            |  11 +-
>   4 files changed, 215 insertions(+), 175 deletions(-)
<SNIP>
>   
>   	/*
>   	 * Since the doorbell only requires a single cacheline, we can save
> @@ -753,27 +781,35 @@ guc_client_alloc(struct drm_i915_private *dev_priv,
>   	guc_proc_desc_init(guc, client);
>   	guc_ctx_desc_init(guc, client);
> -	/* For runtime client allocation we need to enable the doorbell. Not
> -	 * required yet for the static execbuf_client as this special kernel
> -	 * client is enabled from i915_guc_submission_enable().
> -	 *
> -	 * guc_update_doorbell_id(guc, client, db_id);
> -	 */
> +	/* For runtime client allocation we need to enable the doorbell. */
> +	ret = __update_doorbell_desc(client, client->doorbell_id);
> +	if (ret)
> +		goto err_vaddr;
> +
> +	ret = __create_doorbell(client);
> +	if (ret)
> +		goto err_db;
At this point, client->doorbell_id is still invalid (__reserve_doorbell 
is not called until later from guc_init_doorbell_hw), so the 
__create_doorbell fails (and from there, the whole thing falls over: see 
my next comment below). CI.BAT didn't catch it because GuC is disabled 
by default.

>   	DRM_DEBUG_DRIVER("new priority %u client %p for engine(s) 0x%x: ctx_index %u\n",
> -		priority, client, client->engines, client->ctx_index);
> -	DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n",
> -		client->doorbell_id, client->doorbell_offset);
> +			 priority, client, client->engines, client->ctx_index);
> +	DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%lx\n",
> +			 client->doorbell_id, client->doorbell_offset);
>   
>   	return client;
> +err_db:
> +	__update_doorbell_desc(client, GUC_DOORBELL_INVALID);
> +err_vaddr:
> +	i915_gem_object_unpin_map(client->vma->obj);
> +err_vma:
> +	i915_vma_unpin_and_release(&client->vma);
> +err_id:
> +	ida_simple_remove(&guc->ctx_ids, client->ctx_index);
> +err_client:
> +	kfree(client);
>   
> -err:
> -	guc_client_free(dev_priv, client);
> -	return NULL;
> +	return ERR_PTR(ret);
>   }
>
I know you are leaving i915_guc_submission_init to me, but this patch 
should as a minimum check the return code from guc_client_alloc, 
otherwise we might end up with an invalid guc->execbuf_client without 
noticing.

Something like this should suffice, and I can take it from there:

@@ -939,7 +939,7 @@ int i915_guc_submission_init(struct drm_i915_private 
*dev_priv)
INTEL_INFO(dev_priv)->ring_mask,
GUC_CTX_PRIORITY_KMD_NORMAL,
dev_priv->kernel_context);
-       if (!guc->execbuf_client) {
+       if (IS_ERR(guc->execbuf_client)) {
                 DRM_ERROR("Failed to create GuC client for execbuf!\n");
                 goto err;
         }
@@ -1016,10 +1016,8 @@ void i915_guc_submission_fini(struct 
drm_i915_private *dev_priv)
         struct i915_guc_client *client;

         client = fetch_and_zero(&guc->execbuf_client);
-       if (!client)
-               return;
-
-       guc_client_free(client);
+       if (client && !IS_ERR(client))
+               guc_client_free(client);

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx