Re: [PATCH v3 6/7] drm/i915/guc: replace assign_doorbell() with select_doorbell_register()

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 13/06/16 17:57, Dave Gordon wrote:
> This version doesn't update the doorbell bitmap, as that will
> be done when the selected doorbell is associated with a client.
>
> The call is now slightly earlier, just on the general principle
> that potentially-failing operations should be done as early as
> possible, to eliminate late failures and simplify recovery.
>
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 62 +++++++++++++++---------------
>   1 file changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 62bf4bd..a252505 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -232,6 +232,32 @@ static void guc_disable_doorbell(struct intel_guc *guc,
>   	/* XXX: wait for workqueue to drain */
>   }
>
> +static uint16_t
> +select_doorbell_register(struct intel_guc *guc, uint32_t priority)
> +{
> +	/*
> +	 * The bitmap tracks which doorbell registers are currently in use.
> +	 * It is split into two halves; the first half is used for normal
> +	 * priority contexts, the second half for high-priority ones.
> +	 * Note that logically higher priorities are numerically less than
> +	 * normal ones, so the test below means "is it high-priority?"
> +	 */
> +	const bool hi_pri = (priority <= GUC_CTX_PRIORITY_HIGH);
> +	const uint16_t half = GUC_MAX_DOORBELLS / 2;
> +	const uint16_t start = hi_pri ? half : 0;
> +	const uint16_t end = start + half;
> +	uint16_t id;
> +
> +	id = find_next_zero_bit(guc->doorbell_bitmap, end, start);
> +	if (id == end)
> +		id = GUC_INVALID_DOORBELL_ID;
> +
> +	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
> +			hi_pri ? "high" : "normal", id);
> +
> +	return id;
> +}
> +
>   /*
>    * Select, assign and relase doorbell cachelines
>    *
> @@ -256,32 +282,6 @@ static uint32_t select_doorbell_cacheline(struct intel_guc *guc)
>   	return offset;
>   }
>
> -static uint16_t assign_doorbell(struct intel_guc *guc, uint32_t priority)
> -{
> -	/*
> -	 * The bitmap is split into two halves; the first half is used for
> -	 * normal priority contexts, the second half for high-priority ones.
> -	 * Note that logically higher priorities are numerically less than
> -	 * normal ones, so the test below means "is it high-priority?"
> -	 */
> -	const bool hi_pri = (priority <= GUC_CTX_PRIORITY_HIGH);
> -	const uint16_t half = GUC_MAX_DOORBELLS / 2;
> -	const uint16_t start = hi_pri ? half : 0;
> -	const uint16_t end = start + half;
> -	uint16_t id;
> -
> -	id = find_next_zero_bit(guc->doorbell_bitmap, end, start);
> -	if (id == end)
> -		id = GUC_INVALID_DOORBELL_ID;
> -	else
> -		__set_bit(id, guc->doorbell_bitmap);
> -
> -	DRM_DEBUG_DRIVER("assigned %s priority doorbell id 0x%x\n",
> -			hi_pri ? "high" : "normal", id);
> -
> -	return id;
> -}
> -
>   /*
>    * Initialise the process descriptor shared with the GuC firmware.
>    */
> @@ -742,6 +742,11 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj)
>   	client->wq_offset = GUC_DB_SIZE;
>   	client->wq_size = GUC_WQ_SIZE;
>
> +	db_id = select_doorbell_register(guc, client->priority);
> +	if (db_id == GUC_INVALID_DOORBELL_ID)
> +		/* XXX: evict a doorbell instead? */
> +		goto err;
> +
>   	client->doorbell_offset = select_doorbell_cacheline(guc);
>
>   	/*
> @@ -754,11 +759,6 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj)
>   	else
>   		client->proc_desc_offset = (GUC_DB_SIZE / 2);
>
> -	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);
>   	if (guc_init_doorbell(guc, client, db_id))
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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