Re: [PATCH v2 6/6] drm/i915/guc: (re)initialise doorbell h/w when enabling GuC submission

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

On 10/06/16 17:51, Dave Gordon wrote:
> During a hibernate/resume cycle, the whole system is reset, including
> the GuC and the doorbell hardware. Then the system is booted up, drivers
> are loaded, etc -- the GuC firmware may be loaded and set running at
> this point. But then, the booted kernel is replaced by the hibernated
> image, and this resumed kernel will also try to reload the GuC firmware
> (which will fail). To recover, we reset the GuC and try again (which
> should work). But this GuC reset doesn't also reset the doorbell
> hardware, so it can be left in a state inconsistent with that assumed
> by the driver and/or the newly-loaded GuC firmware.
>
> It would be better if the GuC reset also cleared all doorbell state,
> but that's not how the hardware currently works; also, the driver cannot
> directly reprogram the doorbell hardware (only the GuC can do that).
>
> So this patch cycles through all doorbells, assigning and releasing each
> in turn, so that all the doorbell hardware is left in a consistent
> state, no matter how it was programmed by the previously-running kernel
> and/or GuC firmware.
>
> v2: don't use kmap_atomic() now that client page 0 is kept mapped.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 44 +++++++++++++++++++++++++++++-
>   1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 1833bfd..120d2e8 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -694,6 +694,48 @@ static void guc_client_free(struct drm_device *dev,
>   	kfree(client);
>   }
>
> +/*
> + * Borrow the first client to set up & tear down every 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;
> +
> +	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);
> +
> +		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);
> +	}
> +
> +	/* Restore to original value */
> +	err = guc_update_doorbell_id(guc, client, db_id);
> +	if (err)
> +		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 (i != db_id && (value & GUC_DOORBELL_ENABLED))
> +			DRM_DEBUG_DRIVER("Doorbell %d (reg 0x%x) finally 0x%x\n",
> +					  i, drbreg.reg, value);
> +
> +	}
> +}
> +
>   /**
>    * guc_client_alloc() - Allocate an i915_guc_client
>    * @dev:	drm device
> @@ -959,8 +1001,8 @@ int i915_guc_submission_enable(struct drm_device *dev)
>   	}
>
>   	guc->execbuf_client = client;
> -
>   	host2guc_sample_forcewake(guc, client);
> +	guc_init_doorbell_hw(guc);
>
>   	return 0;
>   }
>

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