Re: [PATCH 20/21] drm/i915: Make GuC GGTT reservation work on ggtt

From: "Michal Wajdeczko" <michal.wajdeczko@intel.com>
To: Intel-gfx@lists.freedesktop.org,
	Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Subject: Re: [PATCH 20/21] drm/i915: Make GuC GGTT reservation work on ggtt
Date: Thu, 06 Jun 2019 13:58:08 +0200	[thread overview]
Message-ID: <op.z2you6mfxaggs7@mwajdecz-mobl1.ger.corp.intel.com> (raw)
In-Reply-To: <20190606093639.9372-21-tvrtko.ursulin@linux.intel.com>

On Thu, 06 Jun 2019 11:36:38 +0200, Tvrtko Ursulin  
<tvrtko.ursulin@linux.intel.com> wrote:

> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> These functions operate on ggtt so make them take that directly as
> parameter.

Not quite.

Function intel_guc_reserved_gtt_size() operates on struct intel_guc
and is defined in intel_guc.c for proper layering, so NAK here.

For other two intel_guc_reserve|release_ggtt_top() I would rather:
- rename them to i915_ggtt_reserve|release_guc_top(),
- move to i915_gem_ggtt.c
- make them static

Michal

>
> At the same time move the USES_GUC conditional down to
> intel_guc_reserve_ggtt_top for symmetry with
> intel_guc_reserved_gtt_size.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++++++--------
>  drivers/gpu/drm/i915/intel_guc.c    | 18 ++++++++----------
>  drivers/gpu/drm/i915/intel_guc.h    |  6 +++---
>  3 files changed, 17 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c  
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index d3b3676d10f3..d967a4e9ceb0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2912,7 +2912,7 @@ int i915_gem_init_ggtt(struct drm_i915_private  
> *dev_priv)
>  	 * why.
>  	 */
>  	ggtt->pin_bias = max_t(u32, I915_GTT_PAGE_SIZE,
> -			       intel_guc_reserved_gtt_size(&dev_priv->guc));
> +			       intel_guc_reserved_gtt_size(ggtt));
> 	ret = intel_vgt_balloon(ggtt);
>  	if (ret)
> @@ -2926,11 +2926,9 @@ int i915_gem_init_ggtt(struct drm_i915_private  
> *dev_priv)
>  	if (ret)
>  		return ret;
> -	if (USES_GUC(dev_priv)) {
> -		ret = intel_guc_reserve_ggtt_top(&dev_priv->guc);
> -		if (ret)
> -			goto err_reserve;
> -	}
> +	ret = intel_guc_reserve_ggtt_top(ggtt);
> +	if (ret)
> +		goto err_reserve;
> 	/* Clear any non-preallocated blocks */
>  	drm_mm_for_each_hole(entry, &ggtt->vm.mm, hole_start, hole_end) {
> @@ -2952,7 +2950,7 @@ int i915_gem_init_ggtt(struct drm_i915_private  
> *dev_priv)
>  	return 0;
> err_appgtt:
> -	intel_guc_release_ggtt_top(&dev_priv->guc);
> +	intel_guc_release_ggtt_top(ggtt);
>  err_reserve:
>  	drm_mm_remove_node(&ggtt->error_capture);
>  	return ret;
> @@ -2979,7 +2977,7 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private  
> *dev_priv)
>  	if (drm_mm_node_allocated(&ggtt->error_capture))
>  		drm_mm_remove_node(&ggtt->error_capture);
> -	intel_guc_release_ggtt_top(&dev_priv->guc);
> +	intel_guc_release_ggtt_top(ggtt);
> 	if (drm_mm_initialized(&ggtt->vm.mm)) {
>  		intel_vgt_deballoon(ggtt);
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index b88c349c4fa6..633248b7da25 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -719,7 +719,7 @@ struct i915_vma *intel_guc_allocate_vma(struct  
> intel_guc *guc, u32 size)
> /**
>   * intel_guc_reserved_gtt_size()
> - * @guc:	intel_guc structure
> + * @ggtt:	Pointer to struct i915_ggtt
>   *
>   * The GuC WOPCM mapping shadows the lower part of the GGTT, so if we  
> are using
>   * GuC we can't have any objects pinned in that region. This function  
> returns
> @@ -729,18 +729,19 @@ struct i915_vma *intel_guc_allocate_vma(struct  
> intel_guc *guc, u32 size)
>   * 0 if GuC is not present or not in use.
>   * Otherwise, the GuC WOPCM size.
>   */
> -u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
> +u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt)
>  {
> -	return guc_to_i915(guc)->wopcm.guc.size;
> +	return ggtt->vm.i915->wopcm.guc.size;
>  }
> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
> +int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt)
>  {
> -	struct drm_i915_private *i915 = guc_to_i915(guc);
> -	struct i915_ggtt *ggtt = &i915->ggtt;
>  	u64 size;
>  	int ret;
> +	if (!USES_GUC(ggtt->vm.i915))
> +		return 0;
> +
>  	size = ggtt->vm.total - GUC_GGTT_TOP;
> 	ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
> @@ -752,11 +753,8 @@ int intel_guc_reserve_ggtt_top(struct intel_guc  
> *guc)
>  	return ret;
>  }
> -void intel_guc_release_ggtt_top(struct intel_guc *guc)
> +void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt)
>  {
> -	struct drm_i915_private *i915 = guc_to_i915(guc);
> -	struct i915_ggtt *ggtt = &i915->ggtt;
> -
>  	if (drm_mm_node_allocated(&ggtt->uc_fw))
>  		drm_mm_remove_node(&ggtt->uc_fw);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
> b/drivers/gpu/drm/i915/intel_guc.h
> index cbfed7a77c8b..55ea14176c5e 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -173,9 +173,9 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32  
> rsa_offset);
>  int intel_guc_suspend(struct intel_guc *guc);
>  int intel_guc_resume(struct intel_guc *guc);
>  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32  
> size);
> -u32 intel_guc_reserved_gtt_size(struct intel_guc *guc);
> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc);
> -void intel_guc_release_ggtt_top(struct intel_guc *guc);
> +u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt);
> +int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt);
> +void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt);
> static inline bool intel_guc_is_loaded(struct intel_guc *guc)
>  {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx