Re: [Intel-gfx] [PATCH v3 2/5] drm/i915/uc: Reserve upper range of GGTT
Quoting Fernando Pacheco (2019-04-20 00:00:12)
> GuC and HuC depend on struct_mutex for device
> reinitialization. Moving away from this dependency
> requires perma-pinning the firmware images in GGTT.
> The upper portion of the GuC address space has
> a sizeable hole (several MB) that is inaccessible
> by GuC. Reserve this range within GGTT as it can
> comfortably hold GuC/HuC firmware images.
>
> v2: Reserve node rather than insert (Chris)
> Simpler determination of node start/size (Daniele)
> Move reserve/release out to intel_guc.* files
>
> v3: Reserve starting at GUC_GGTT_TOP only and bail if this
> fails (Chris)
>
> Signed-off-by: Fernando Pacheco
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 25 -
> drivers/gpu/drm/i915/i915_gem_gtt.h | 1 +
> drivers/gpu/drm/i915/intel_guc.c| 28
> drivers/gpu/drm/i915/intel_guc.h| 2 ++
> 4 files changed, 43 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8f460cc4cc1f..0b4c22e68574 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2752,6 +2752,12 @@ 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(_priv->guc);
> + if (ret)
> + goto err_reserve;
> + }
> +
> /* Clear any non-preallocated blocks */
> drm_mm_for_each_hole(entry, >vm.mm, hole_start, hole_end) {
> DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
> @@ -2766,12 +2772,14 @@ int i915_gem_init_ggtt(struct drm_i915_private
> *dev_priv)
> if (INTEL_PPGTT(dev_priv) == INTEL_PPGTT_ALIASING) {
> ret = i915_gem_init_aliasing_ppgtt(dev_priv);
> if (ret)
> - goto err;
> + goto err_appgtt;
> }
>
> return 0;
>
> -err:
> +err_appgtt:
> + intel_guc_release_ggtt_top(_priv->guc);
> +err_reserve:
> drm_mm_remove_node(>error_capture);
> return ret;
> }
> @@ -2797,6 +2805,8 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private
> *dev_priv)
> if (drm_mm_node_allocated(>error_capture))
> drm_mm_remove_node(>error_capture);
>
> + intel_guc_release_ggtt_top(_priv->guc);
> +
> if (drm_mm_initialized(>vm.mm)) {
> intel_vgt_deballoon(dev_priv);
> i915_address_space_fini(>vm);
> @@ -3369,17 +3379,6 @@ int i915_ggtt_probe_hw(struct drm_i915_private
> *dev_priv)
> if (ret)
> return ret;
>
> - /* Trim the GGTT to fit the GuC mappable upper range (when enabled).
> -* This is easier than doing range restriction on the fly, as we
> -* currently don't have any bits spare to pass in this upper
> -* restriction!
> -*/
> - if (USES_GUC(dev_priv)) {
> - ggtt->vm.total = min_t(u64, ggtt->vm.total, GUC_GGTT_TOP);
> - ggtt->mappable_end =
> - min_t(u64, ggtt->mappable_end, ggtt->vm.total);
> - }
> -
> if ((ggtt->vm.total - 1) >> 32) {
> DRM_ERROR("We never expected a Global GTT with more than
> 32bits"
> " of address space! Found %lldM!\n",
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
> b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index f597f35b109b..b51e779732c3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -384,6 +384,7 @@ struct i915_ggtt {
> u32 pin_bias;
>
> struct drm_mm_node error_capture;
> + struct drm_mm_node uc_fw;
I thought this might have been a better fit in with intel_uc if there's
a common spot for it. Likewise, I should move error_capture over to
i915_gpu_error.
> };
>
> struct i915_hw_ppgtt {
> diff --git a/drivers/gpu/drm/i915/intel_guc.c
> b/drivers/gpu/drm/i915/intel_guc.c
> index d81a02b0f525..299b6aa4fe28 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -721,3 +721,31 @@ u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
> {
> return guc_to_i915(guc)->wopcm.guc.size;
> }
> +
> +int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
> +{
> + struct drm_i915_private *i915 = guc_to_i915(guc);
> + struct i915_ggtt *ggtt = >ggtt;
> + u64 size;
> + int ret;
> +
> + size = ggtt->vm.total - GUC_GGTT_TOP;
> +
> + ret = i915_gem_gtt_reserve(>vm, >uc_fw, size,
> + GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE,
> + PIN_NOEVICT);
> +
We don't tend to leave newlines before conditions.
> + if (ret)
> + DRM_DEBUG_DRIVER("GuC: failed to reserve top of ggtt\n");
> +
> +
[Intel-gfx] [PATCH v3 2/5] drm/i915/uc: Reserve upper range of GGTT
GuC and HuC depend on struct_mutex for device
reinitialization. Moving away from this dependency
requires perma-pinning the firmware images in GGTT.
The upper portion of the GuC address space has
a sizeable hole (several MB) that is inaccessible
by GuC. Reserve this range within GGTT as it can
comfortably hold GuC/HuC firmware images.
v2: Reserve node rather than insert (Chris)
Simpler determination of node start/size (Daniele)
Move reserve/release out to intel_guc.* files
v3: Reserve starting at GUC_GGTT_TOP only and bail if this
fails (Chris)
Signed-off-by: Fernando Pacheco
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 25 -
drivers/gpu/drm/i915/i915_gem_gtt.h | 1 +
drivers/gpu/drm/i915/intel_guc.c| 28
drivers/gpu/drm/i915/intel_guc.h| 2 ++
4 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 8f460cc4cc1f..0b4c22e68574 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2752,6 +2752,12 @@ 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(_priv->guc);
+ if (ret)
+ goto err_reserve;
+ }
+
/* Clear any non-preallocated blocks */
drm_mm_for_each_hole(entry, >vm.mm, hole_start, hole_end) {
DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
@@ -2766,12 +2772,14 @@ int i915_gem_init_ggtt(struct drm_i915_private
*dev_priv)
if (INTEL_PPGTT(dev_priv) == INTEL_PPGTT_ALIASING) {
ret = i915_gem_init_aliasing_ppgtt(dev_priv);
if (ret)
- goto err;
+ goto err_appgtt;
}
return 0;
-err:
+err_appgtt:
+ intel_guc_release_ggtt_top(_priv->guc);
+err_reserve:
drm_mm_remove_node(>error_capture);
return ret;
}
@@ -2797,6 +2805,8 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private
*dev_priv)
if (drm_mm_node_allocated(>error_capture))
drm_mm_remove_node(>error_capture);
+ intel_guc_release_ggtt_top(_priv->guc);
+
if (drm_mm_initialized(>vm.mm)) {
intel_vgt_deballoon(dev_priv);
i915_address_space_fini(>vm);
@@ -3369,17 +3379,6 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv)
if (ret)
return ret;
- /* Trim the GGTT to fit the GuC mappable upper range (when enabled).
-* This is easier than doing range restriction on the fly, as we
-* currently don't have any bits spare to pass in this upper
-* restriction!
-*/
- if (USES_GUC(dev_priv)) {
- ggtt->vm.total = min_t(u64, ggtt->vm.total, GUC_GGTT_TOP);
- ggtt->mappable_end =
- min_t(u64, ggtt->mappable_end, ggtt->vm.total);
- }
-
if ((ggtt->vm.total - 1) >> 32) {
DRM_ERROR("We never expected a Global GTT with more than 32bits"
" of address space! Found %lldM!\n",
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index f597f35b109b..b51e779732c3 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -384,6 +384,7 @@ struct i915_ggtt {
u32 pin_bias;
struct drm_mm_node error_capture;
+ struct drm_mm_node uc_fw;
};
struct i915_hw_ppgtt {
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index d81a02b0f525..299b6aa4fe28 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -721,3 +721,31 @@ u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
{
return guc_to_i915(guc)->wopcm.guc.size;
}
+
+int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
+{
+ struct drm_i915_private *i915 = guc_to_i915(guc);
+ struct i915_ggtt *ggtt = >ggtt;
+ u64 size;
+ int ret;
+
+ size = ggtt->vm.total - GUC_GGTT_TOP;
+
+ ret = i915_gem_gtt_reserve(>vm, >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 = >ggtt;
+
+ if (drm_mm_node_allocated(>uc_fw))
+ drm_mm_remove_node(>uc_fw);
+}
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 2c59ff8d9f39..2494e84831a2 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -173,6 +173,8 @@ int intel_guc_suspend(struct
