diff mbox series

[RFC,14/14] drm/i915: Make GuC GGTT reservation work on ggtt

Message ID 20190610155419.23723-15-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Implicit dev_priv removal | expand

Commit Message

Tvrtko Ursulin June 10, 2019, 3:54 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

These functions operate on ggtt so make them take that directly as
parameter.

At the same time move the USES_GUC conditional down to
intel_guc_reserve_ggtt_top for symmetry with
intel_guc_reserved_gtt_size.

v2:
 * Rename and move functions to be static in i915_gem_gtt.c (Michal)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 37 +++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_guc.c    | 27 ---------------------
 drivers/gpu/drm/i915/intel_guc.h    |  2 --
 3 files changed, 30 insertions(+), 36 deletions(-)

Comments

Michal Wajdeczko June 10, 2019, 6:43 p.m. UTC | #1 
On Mon, 10 Jun 2019 17:54:19 +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.
>
> At the same time move the USES_GUC conditional down to
> intel_guc_reserve_ggtt_top for symmetry with
> intel_guc_reserved_gtt_size.
>
> v2:
>  * Rename and move functions to be static in i915_gem_gtt.c (Michal)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>

with some nits below,

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 37 +++++++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_guc.c    | 27 ---------------------
>  drivers/gpu/drm/i915/intel_guc.h    |  2 --
>  3 files changed, 30 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c  
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index e62041eb10b8..394f347a90ee 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2830,6 +2830,31 @@ static void fini_aliasing_ppgtt(struct  
> drm_i915_private *i915)
>  	ggtt->vm.vma_ops.unbind_vma = ggtt_unbind_vma;
>  }
> +static int ggtt_reserve_guc_top(struct i915_ggtt *ggtt)

maybe we should copy some comment from commit 911800765ef6
directly to the code here, to explain why we do that ?

> +{
> +	u64 size;
> +	int ret;
> +
> +	if (!USES_GUC(ggtt->vm.i915))
> +		return 0;
> +
> +	size = ggtt->vm.total - GUC_GGTT_TOP;

what about making sure that ggtt size is large enough:

	GEM_BUG_ON(ggtt->vm.total > GUC_GGTT_TOP)

> +
> +	ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
> +				   GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE,
> +				   PIN_NOEVICT);
> +	if (ret)
> +		DRM_DEBUG_DRIVER("GuC: failed to reserve top of ggtt\n");

this is now ggtt code, so:

	DRM_DEBUG_DRIVER("Failed to reserve top of GGTT for GuC\n");

> +
> +	return ret;
> +}
> +
> +static void ggtt_release_guc_top(struct i915_ggtt *ggtt)
> +{
> +	if (drm_mm_node_allocated(&ggtt->uc_fw))
> +		drm_mm_remove_node(&ggtt->uc_fw);
> +}
> +
>  int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>  {
>  	/* Let GEM Manage all of the aperture.
> @@ -2867,11 +2892,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 = ggtt_reserve_guc_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) {
> @@ -2893,7 +2916,7 @@ int i915_gem_init_ggtt(struct drm_i915_private  
> *dev_priv)
>  	return 0;
> err_appgtt:
> -	intel_guc_release_ggtt_top(&dev_priv->guc);
> +	ggtt_release_guc_top(ggtt);
>  err_reserve:
>  	drm_mm_remove_node(&ggtt->error_capture);
>  	return ret;
> @@ -2920,7 +2943,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);
> +	ggtt_release_guc_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 d45d97624402..c40a6efdd33a 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -685,30 +685,3 @@ struct i915_vma *intel_guc_allocate_vma(struct  
> intel_guc *guc, u32 size)
>  	i915_gem_object_put(obj);
>  	return vma;
>  }
> -
> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
> -{
> -	struct drm_i915_private *i915 = guc_to_i915(guc);
> -	struct i915_ggtt *ggtt = &i915->ggtt;
> -	u64 size;
> -	int ret;
> -
> -	size = ggtt->vm.total - GUC_GGTT_TOP;
> -
> -	ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
> -				   GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE,
> -				   PIN_NOEVICT);
> -	if (ret)
> -		DRM_DEBUG_DRIVER("GuC: failed to reserve top of ggtt\n");
> -
> -	return ret;
> -}
> -
> -void intel_guc_release_ggtt_top(struct intel_guc *guc)
> -{
> -	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 85c3b02a0c08..08c906abdfa2 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -172,8 +172,6 @@ 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);
> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc);
> -void intel_guc_release_ggtt_top(struct intel_guc *guc);
> static inline bool intel_guc_is_loaded(struct intel_guc *guc)
>  {
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e62041eb10b8..394f347a90ee 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2830,6 +2830,31 @@  static void fini_aliasing_ppgtt(struct drm_i915_private *i915)
 	ggtt->vm.vma_ops.unbind_vma = ggtt_unbind_vma;
 }
 
+static int ggtt_reserve_guc_top(struct i915_ggtt *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,
+				   GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE,
+				   PIN_NOEVICT);
+	if (ret)
+		DRM_DEBUG_DRIVER("GuC: failed to reserve top of ggtt\n");
+
+	return ret;
+}
+
+static void ggtt_release_guc_top(struct i915_ggtt *ggtt)
+{
+	if (drm_mm_node_allocated(&ggtt->uc_fw))
+		drm_mm_remove_node(&ggtt->uc_fw);
+}
+
 int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
 {
 	/* Let GEM Manage all of the aperture.
@@ -2867,11 +2892,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 = ggtt_reserve_guc_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) {
@@ -2893,7 +2916,7 @@  int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
 	return 0;
 
 err_appgtt:
-	intel_guc_release_ggtt_top(&dev_priv->guc);
+	ggtt_release_guc_top(ggtt);
 err_reserve:
 	drm_mm_remove_node(&ggtt->error_capture);
 	return ret;
@@ -2920,7 +2943,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);
+	ggtt_release_guc_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 d45d97624402..c40a6efdd33a 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -685,30 +685,3 @@  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
 	i915_gem_object_put(obj);
 	return vma;
 }
-
-int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
-{
-	struct drm_i915_private *i915 = guc_to_i915(guc);
-	struct i915_ggtt *ggtt = &i915->ggtt;
-	u64 size;
-	int ret;
-
-	size = ggtt->vm.total - GUC_GGTT_TOP;
-
-	ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
-				   GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE,
-				   PIN_NOEVICT);
-	if (ret)
-		DRM_DEBUG_DRIVER("GuC: failed to reserve top of ggtt\n");
-
-	return ret;
-}
-
-void intel_guc_release_ggtt_top(struct intel_guc *guc)
-{
-	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 85c3b02a0c08..08c906abdfa2 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -172,8 +172,6 @@  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);
-int intel_guc_reserve_ggtt_top(struct intel_guc *guc);
-void intel_guc_release_ggtt_top(struct intel_guc *guc);
 
 static inline bool intel_guc_is_loaded(struct intel_guc *guc)
 {