Re: [PATCH 08/12] drm/i915/guc: Wait for doorbell to be inactive before deallocating

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

On 3/21/2017 2:02 AM, Oscar Mateo wrote:
> Doorbell release flow requires that we wait for GEN8_DRB_VALID bit to go
> to zero after updating db_status before we call the GuC to release the
> doorbell.
>
> Kudos to Daniele for finding this out.
>
> v2: WARN instead of DRM_ERROR (Joonas)
>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

I'd like Joonas to give his ack on not returning after the wait since he 
commented on it in the previous version. I personally prefer it this way 
because we gain a bit more time for the doorbell to disable itself 
(there is no spec in regards to how long this should take, but it should 
be quick) while the GuC processes the H2G. If we ever see a case where 
the warn fires but the H2G is successful it would mean that we have to 
tune our wait time.

Thanks,
Daniele

> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 855112d..143ef26 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -206,12 +206,22 @@ static int __create_doorbell(struct i915_guc_client *client)
>
>  static int __destroy_doorbell(struct i915_guc_client *client)
>  {
> +	struct drm_i915_private *dev_priv = guc_to_i915(client->guc);
>  	struct guc_doorbell_info *doorbell;
> +	u16 db_id = client->doorbell_id;
> +
> +	GEM_BUG_ON(db_id >= GUC_DOORBELL_INVALID);
>
>  	doorbell = __get_doorbell(client);
>  	doorbell->db_status = GUC_DOORBELL_DISABLED;
>  	doorbell->cookie = 0;
>
> +	/* Doorbell release flow requires that we wait for GEN8_DRB_VALID bit
> +	 * to go to zero after updating db_status before we call the GuC to
> +	 * release the doorbell */
> +	if (wait_for_us(!(I915_READ(GEN8_DRBREGL(db_id)) & GEN8_DRB_VALID), 10))
> +		WARN_ONCE(true, "Doorbell never became invalid after disable\n");
> +




>  	return __guc_deallocate_doorbell(client->guc, client->ctx_index);
>  }
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx