Re: [PATCH] drm/i915/guc: enable GuC GGTT invalidation from the start

[John Harrison <john.c.harrison@xxxxxxxxx>]

On 11/10/2022 09:58, Daniele Ceraolo Spurio wrote:
> Invalidating the GuC TLBs while GuC is not loaded does not have negative
> consequences, so if we're starting the driver with GuC enabled we can
> use the GGTT invalidation function from the get-go, iinstead of switching
> to it when we initialize the GuC objects.
>
> In MTL, this fixes and issue where we try to overwrite the invalidation
> function twice (once for each GuC), due to the GGTT being shared between
> the primary and media GTs
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
> Cc: Aravind Iddamsetty <aravind.iddamsetty@xxxxxxxxx>
Reviewed-by: John Harrison <John.C.Harrison@xxxxxxxxx>

> ---
>   drivers/gpu/drm/i915/gt/intel_ggtt.c   | 28 ++++----------------------
>   drivers/gpu/drm/i915/gt/intel_gtt.h    |  2 --
>   drivers/gpu/drm/i915/gt/uc/intel_guc.c |  7 -------
>   3 files changed, 4 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index 2518cebbf931..2dbe6ad5c900 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -979,7 +979,10 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
>   			I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND;
>   	}
>   
> -	ggtt->invalidate = gen8_ggtt_invalidate;
> +	if (intel_uc_wants_guc(&ggtt->vm.gt->uc))
> +		ggtt->invalidate = guc_ggtt_invalidate;
> +	else
> +		ggtt->invalidate = gen8_ggtt_invalidate;
>   
>   	ggtt->vm.vma_ops.bind_vma    = intel_ggtt_bind_vma;
>   	ggtt->vm.vma_ops.unbind_vma  = intel_ggtt_unbind_vma;
> @@ -1216,29 +1219,6 @@ int i915_ggtt_enable_hw(struct drm_i915_private *i915)
>   	return 0;
>   }
>   
> -void i915_ggtt_enable_guc(struct i915_ggtt *ggtt)
> -{
> -	GEM_BUG_ON(ggtt->invalidate != gen8_ggtt_invalidate);
> -
> -	ggtt->invalidate = guc_ggtt_invalidate;
> -
> -	ggtt->invalidate(ggtt);
> -}
> -
> -void i915_ggtt_disable_guc(struct i915_ggtt *ggtt)
> -{
> -	/* XXX Temporary pardon for error unload */
> -	if (ggtt->invalidate == gen8_ggtt_invalidate)
> -		return;
> -
> -	/* We should only be called after i915_ggtt_enable_guc() */
> -	GEM_BUG_ON(ggtt->invalidate != guc_ggtt_invalidate);
> -
> -	ggtt->invalidate = gen8_ggtt_invalidate;
> -
> -	ggtt->invalidate(ggtt);
> -}
> -
>   /**
>    * i915_ggtt_resume_vm - Restore the memory mappings for a GGTT or DPT VM
>    * @vm: The VM to restore the mappings for
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
> index 4d75ba4bb41d..fcbbfed79f15 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> @@ -579,8 +579,6 @@ void intel_ggtt_unbind_vma(struct i915_address_space *vm,
>   int i915_ggtt_probe_hw(struct drm_i915_private *i915);
>   int i915_ggtt_init_hw(struct drm_i915_private *i915);
>   int i915_ggtt_enable_hw(struct drm_i915_private *i915);
> -void i915_ggtt_enable_guc(struct i915_ggtt *ggtt);
> -void i915_ggtt_disable_guc(struct i915_ggtt *ggtt);
>   int i915_init_ggtt(struct drm_i915_private *i915);
>   void i915_ggtt_driver_release(struct drm_i915_private *i915);
>   void i915_ggtt_driver_late_release(struct drm_i915_private *i915);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index 1bcd61bb50f8..4ec954b6b4e8 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -421,9 +421,6 @@ int intel_guc_init(struct intel_guc *guc)
>   	/* now that everything is perma-pinned, initialize the parameters */
>   	guc_init_params(guc);
>   
> -	/* We need to notify the guc whenever we change the GGTT */
> -	i915_ggtt_enable_guc(gt->ggtt);
> -
>   	intel_uc_fw_change_status(&guc->fw, INTEL_UC_FIRMWARE_LOADABLE);
>   
>   	return 0;
> @@ -448,13 +445,9 @@ int intel_guc_init(struct intel_guc *guc)
>   
>   void intel_guc_fini(struct intel_guc *guc)
>   {
> -	struct intel_gt *gt = guc_to_gt(guc);
> -
>   	if (!intel_uc_fw_is_loadable(&guc->fw))
>   		return;
>   
> -	i915_ggtt_disable_guc(gt->ggtt);
> -
>   	if (intel_guc_slpc_is_used(guc))
>   		intel_guc_slpc_fini(&guc->slpc);
>