[Intel-gfx] [PATCH 2/3] drm/i915/guc: disable GuC submission earlier during GuC (re)load

[Tvrtko Ursulin]

On 07/06/16 09:14, Dave Gordon wrote:
> When resetting and reloading the GuC, the GuC submission management code
> also needs to destroy and recreate the GuC client(s). Currently this is
> done by a separate call from the GuC loader, but really, it's just an
> internal detail of the submission code. So here we remove the call from
> the loader (which is too late, really, because the GuC has already been
> reloaded at this point) and put it into guc_submission_init() instead.
> This means that any preexisting client is destroyed *before* the GuC
> (re)load and then recreated after, iff the firmware was successfully
> loaded. If the GuC reload fails, we don't recreate the client, so
> fallback to execlists mode (if active) won't leak the client object
> (previously, the now-unusable client would have been left allocated,
> and leaked if the driver were unloaded).
>
> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 15 ++++++++++-----
>   drivers/gpu/drm/i915/intel_guc_loader.c    |  3 ---
>   2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index ac72451..2db1182 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -657,6 +657,8 @@ 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
> @@ -664,10 +666,11 @@ static void guc_client_free(struct drm_device *dev,
>   		 * GuC that we've finished with it, finally deallocate
>   		 * it in our bitmap
>   		 */
> -		if (client->doorbell_id != GUC_INVALID_DOORBELL_ID) {
> +		if (db_id != GUC_INVALID_DOORBELL_ID) {
>   			guc_disable_doorbell(guc, client);
> -			host2guc_release_doorbell(guc, client);
> -			release_doorbell(guc, client->doorbell_id);
> +			if (test_bit(db_id, guc->doorbell_bitmap))
> +				host2guc_release_doorbell(guc, client);
> +			release_doorbell(guc, db_id);
>   		}
>
>   		kunmap(kmap_to_page(client->client_base));
> @@ -912,6 +915,10 @@ int i915_guc_submission_init(struct drm_device *dev)
>   	const size_t gemsize = round_up(poolsize, PAGE_SIZE);
>   	struct intel_guc *guc = &dev_priv->guc;
>
> +	/* Wipe bitmap & delete client in case of reinitialisation */
> +	bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS);
> +	i915_guc_submission_disable(dev);
> +
>   	if (!i915.enable_guc_submission)
>   		return 0; /* not enabled  */
>
> @@ -923,9 +930,7 @@ int i915_guc_submission_init(struct drm_device *dev)
>   		return -ENOMEM;
>
>   	ida_init(&guc->ctx_ids);
> -
>   	guc_create_log(guc);
> -
>   	guc_create_ads(guc);
>
>   	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 4e34c2e..41f7c7d 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -492,9 +492,6 @@ int intel_guc_setup(struct drm_device *dev)
>   		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>
>   	if (i915.enable_guc_submission) {
> -		/* The execbuf_client will be recreated. Release it first. */
> -		i915_guc_submission_disable(dev);
> -
>   		err = i915_guc_submission_enable(dev);
>   		if (err)
>   			goto fail;
>

This fixes the errors on suspend/resume? It would be useful for the 
commit message to explain what was happening.

It is a bit strange since the first disable now comes before the init, 
but since the disable path already does handle that case I suppose it is OK.

Anyway,

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

Regards,

Tvrtko