Re: [PATCH v2 2/6] drm/i915/guc: refactor guc_init_doorbell_hw()

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

On 21/07/16 19:15, Dave Gordon wrote:
> We have essentially the same code in each of two different loops, so we
> can refactor it into a little helper function.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 54 +++++++++++++++++-------------
>   1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 9c5b81b..816bdca 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -698,32 +698,47 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj)
>   	kfree(client);
>   }
>
> +/* Check that a doorbell register is in the expected state */
> +static bool guc_doorbell_check(struct intel_guc *guc, uint16_t db_id)
> +{
> +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +	i915_reg_t drbreg = GEN8_DRBREGL(db_id);
> +	uint32_t value = I915_READ(drbreg);
> +	bool enabled = (value & GUC_DOORBELL_ENABLED) != 0;
> +	bool expected = test_bit(db_id, guc->doorbell_bitmap);
> +
> +	if (enabled == expected)
> +		return true;
> +
> +	DRM_DEBUG_DRIVER("Doorbell %d (reg 0x%x) 0x%x, should be %s\n",
> +			 db_id, drbreg.reg, value,
> +			 expected ? "active" : "inactive");
> +
> +	return false;
> +}
> +
>   /*
>    * Borrow the first client to set up & tear down each unused doorbell
>    * in turn, to ensure that all doorbell h/w is (re)initialised.
>    */
>   static void guc_init_doorbell_hw(struct intel_guc *guc)
>   {
> -	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   	struct i915_guc_client *client = guc->execbuf_client;
> -	uint16_t db_id, i;
> -	int err;
> +	uint16_t db_id;
> +	int i, err;
>
> +	/* Save client's original doorbell selection */
>   	db_id = client->doorbell_id;
>
>   	for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
> -		i915_reg_t drbreg = GEN8_DRBREGL(i);
> -		u32 value = I915_READ(drbreg);
> -
> -		if (test_bit(i, guc->doorbell_bitmap))
> +		/* Skip if doorbell is OK */
> +		if (guc_doorbell_check(guc, i))
>   			continue;
>
>   		err = guc_update_doorbell_id(guc, client, i);
> -
> -		/* Report update failure or unexpectedly active doorbell */
> -		if (err || (i != db_id && (value & GUC_DOORBELL_ENABLED)))
> -			DRM_DEBUG_DRIVER("Doorbell %d (reg 0x%x) was 0x%x, err %d\n",
> -					  i, drbreg.reg, value, err);
> +		if (err)
> +			DRM_DEBUG_DRIVER("Doorbell %d update failed, err %d\n",
> +					i, err);
>   	}
>
>   	/* Restore to original value */
> @@ -732,18 +747,9 @@ static void guc_init_doorbell_hw(struct intel_guc *guc)
>   		DRM_ERROR("Failed to restore doorbell to %d, err %d\n",
>   			db_id, err);
>
> -	for (i = 0; i < GUC_MAX_DOORBELLS; ++i) {
> -		i915_reg_t drbreg = GEN8_DRBREGL(i);
> -		u32 value = I915_READ(drbreg);
> -
> -		if (test_bit(i, guc->doorbell_bitmap))
> -			continue;
> -
> -		if (i != db_id && (value & GUC_DOORBELL_ENABLED))
> -			DRM_DEBUG_DRIVER("Doorbell %d (reg 0x%x) finally 0x%x\n",
> -					  i, drbreg.reg, value);
> -
> -	}
> +	/* Read back & verify all doorbell registers */
> +	for (i = 0; i < GUC_MAX_DOORBELLS; ++i)
> +		(void)guc_doorbell_check(guc, i);
>   }
>
>   /**
>

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