[Intel-gfx] [PATCH v2 5/6] drm/i915/guc: refactor doorbell management code

[Tvrtko Ursulin]

On 10/06/16 17:51, Dave Gordon wrote:
> This patch refactors the driver's handling and tracking of doorbells, in
> preparation for a later one which will resolve a suspend-resume issue.
>
> There are three resources to be managed:
> 1. Cachelines: a single line within the client-object's page 0
>     is snooped by doorbell hardware for writes from the host.
> 2. Doorbell registers: each defines one cacheline to be snooped.
> 3. Bitmap: tracks which doorbell registers are in use.
>
> The doorbell setup/teardown protocol starts with:
> 1. Pick a cacheline: select_doorbell_cacheline()
> 2. Find an available doorbell register: assign_doorbell()
> (These values are passed to the GuC via the shared context
> descriptor; this part of the sequence remains unchanged).
>
> 3. Update the bitmap to reflect registers-in-use
> 4. Prepare the cacheline for use by setting its status to ENABLED
> 5. Ask the GuC to program the doorbell to snoop the cacheline
>
> and of course teardown is very similar:
> 6. Set the cacheline to DISABLED
> 7. Ask the GuC to reprogram the doorbell to stop snooping
> 8. Record that the doorbell is not in use.
>
> Operations 6-8 (guc_disable_doorbell(), host2guc_release_doorbell(), and
> release_doorbell()) were called in sequence from guc_client_free(), but
> are now moved into the teardown phase of the common function.
>
> Steps 4-5 (guc_init_doorbell() and host2guc_allocate_doorbell()) were
> similarly done as sequential steps in guc_client_alloc(), but since it
> turns out that we don't need to be able to do them separately they're
> now collected into the setup phase of the common function.
>
> The only new code (and new capability) is the block tagged
>      /* Update the GuC's idea of the doorbell ID */
> i.e. we can now *change* the doorbell register used by an existing
> client, whereas previously it was set once for the entire lifetime
> of the client. We will use this new feature in the next patch.
>
> v2: Trivial independent fixes pushed ahead as separate patches.
>      MUCH longer commit message :) [Tvrtko Ursulin]
>
> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 94 +++++++++++++++++-------------
>   1 file changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 45b33f8..1833bfd 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -174,31 +174,59 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
>    * client object which contains the page being used for the doorbell
>    */
>
> -static void guc_init_doorbell(struct intel_guc *guc,
> -			      struct i915_guc_client *client)
> +static int guc_update_doorbell_id(struct intel_guc *guc,
> +				  struct i915_guc_client *client,
> +				  u16 new_id)
>   {
> +	struct sg_table *sg = guc->ctx_pool_obj->pages;
> +	void *doorbell_bitmap = guc->doorbell_bitmap;
>   	struct guc_doorbell_info *doorbell;
> +	struct guc_context_desc desc;
> +	size_t len;
>
>   	doorbell = client->client_base + client->doorbell_offset;
>
> -	doorbell->db_status = GUC_DOORBELL_ENABLED;
> +	if (client->doorbell_id != GUC_INVALID_DOORBELL_ID &&
> +	    test_bit(client->doorbell_id, doorbell_bitmap)) {
> +		/* Deactivate the old doorbell */
> +		doorbell->db_status = GUC_DOORBELL_DISABLED;
> +		(void)host2guc_release_doorbell(guc, client);
> +		__clear_bit(client->doorbell_id, doorbell_bitmap);
> +	}
> +
> +	/* Update the GuC's idea of the doorbell ID */
> +	len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
> +			     sizeof(desc) * client->ctx_index);
> +	if (len != sizeof(desc))
> +		return -EFAULT;
> +	desc.db_id = new_id;
> +	len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
> +			     sizeof(desc) * client->ctx_index);
> +	if (len != sizeof(desc))
> +		return -EFAULT;
> +
> +	client->doorbell_id = new_id;
> +	if (new_id == GUC_INVALID_DOORBELL_ID)
> +		return 0;
> +
> +	/* Activate the new doorbell */
> +	__set_bit(new_id, doorbell_bitmap);

Is this the same bit as in assign_doorbell so redundant?

>   	doorbell->cookie = 0;
> +	doorbell->db_status = GUC_DOORBELL_ENABLED;
> +	return host2guc_allocate_doorbell(guc, client);
> +}
> +
> +static int guc_init_doorbell(struct intel_guc *guc,
> +			      struct i915_guc_client *client,
> +			      uint16_t db_id)
> +{
> +	return guc_update_doorbell_id(guc, client, db_id);
>   }
>
>   static void guc_disable_doorbell(struct intel_guc *guc,
>   				 struct i915_guc_client *client)
>   {
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	struct guc_doorbell_info *doorbell;
> -	i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
> -	int value;
> -
> -	doorbell = client->client_base + client->doorbell_offset;
> -
> -	doorbell->db_status = GUC_DOORBELL_DISABLED;
> -
> -	value = I915_READ(drbreg);
> -	WARN_ON((value & GEN8_DRB_VALID) != 0);
> +	(void)guc_update_doorbell_id(guc, client, GUC_INVALID_DOORBELL_ID);
>
>   	/* XXX: wait for any interrupts */
>   	/* XXX: wait for workqueue to drain */
> @@ -254,11 +282,6 @@ static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
>   	return id;
>   }
>
> -static void release_doorbell(struct intel_guc *guc, uint16_t id)
> -{
> -	__clear_bit(id, guc->doorbell_bitmap);
> -}
> -
>   /*
>    * Initialise the process descriptor shared with the GuC firmware.
>    */
> @@ -652,21 +675,11 @@ static void guc_client_free(struct drm_device *dev,
>   	 */
>
>   	if (client->client_base) {
> -		uint16_t db_id = client->doorbell_id;
> -
>   		/*
> -		 * If we got as far as setting up a doorbell, make sure
> -		 * we shut it down before unmapping & deallocating the
> -		 * memory. So first disable the doorbell, then tell the
> -		 * GuC that we've finished with it, finally deallocate
> -		 * it in our bitmap
> +		 * If we got as far as setting up a doorbell, make sure we
> +		 * shut it down before unmapping & deallocating the memory.
>   		 */
> -		if (db_id != GUC_INVALID_DOORBELL_ID) {
> -			guc_disable_doorbell(guc, client);
> -			if (test_bit(db_id, guc->doorbell_bitmap))
> -				host2guc_release_doorbell(guc, client);
> -			release_doorbell(guc, db_id);
> -		}
> +		guc_disable_doorbell(guc, client);
>
>   		kunmap(kmap_to_page(client->client_base));
>   	}
> @@ -701,6 +714,7 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_guc *guc = &dev_priv->guc;
>   	struct drm_i915_gem_object *obj;
> +	uint16_t db_id;
>
>   	client = kzalloc(sizeof(*client), GFP_KERNEL);
>   	if (!client)
> @@ -741,22 +755,20 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>   	else
>   		client->proc_desc_offset = (GUC_DB_SIZE / 2);
>
> -	client->doorbell_id = assign_doorbell(guc, client->priority);
> -	if (client->doorbell_id == GUC_INVALID_DOORBELL_ID)
> +	db_id = assign_doorbell(guc, client->priority);
> +	if (db_id == GUC_INVALID_DOORBELL_ID)
>   		/* XXX: evict a doorbell instead */
>   		goto err;
>
>   	guc_init_proc_desc(guc, client);
>   	guc_init_ctx_desc(guc, client);
> -	guc_init_doorbell(guc, client);
> -
> -	/* XXX: Any cache flushes needed? General domain mgmt calls? */
> -
> -	if (host2guc_allocate_doorbell(guc, client))
> +	if (guc_init_doorbell(guc, client, db_id))
>   		goto err;
>
> -	DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id %u\n",
> -		priority, client, client->ctx_index, client->doorbell_id);
> +	DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u\n",
> +		priority, client, client->ctx_index);
> +	DRM_DEBUG_DRIVER("doorbell id %u, cacheline offset 0x%x\n",
> +		client->doorbell_id, client->doorbell_offset);
>
>   	return client;
>
>

Otherwise looks good to me, much more easier to understand what is 
happening now.

Regards,

Tvrtko