[Intel-gfx] [PATCH v2 2/5] drm/i915/uc: Reserve upper range of GGTT

[Chris Wilson]

Quoting Fernando Pacheco (2019-04-19 00:31:48)
> 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
> 
> Signed-off-by: Fernando Pacheco <fernando.pacheco at intel.com>
> ---
>  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    | 45 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_guc.h    |  2 ++
>  4 files changed, 60 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(&dev_priv->guc);
> +               if (ret)
> +                       goto err_reserve;
> +       }
> +
>         /* Clear any non-preallocated blocks */
>         drm_mm_for_each_hole(entry, &ggtt->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(&dev_priv->guc);
> +err_reserve:
>         drm_mm_remove_node(&ggtt->error_capture);
>         return ret;
>  }
> @@ -2797,6 +2805,8 @@ 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);
> +
>         if (drm_mm_initialized(&ggtt->vm.mm)) {
>                 intel_vgt_deballoon(dev_priv);
>                 i915_address_space_fini(&ggtt->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..ddd246dc3f14 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -721,3 +721,48 @@ u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
>  {
>         return guc_to_i915(guc)->wopcm.guc.size;
>  }
> +
> +static u32 __intel_guc_ggtt_top_offset(struct intel_guc *guc)
> +{
> +       struct drm_i915_private *i915 = guc_to_i915(guc);
> +       struct i915_ggtt *ggtt = &i915->ggtt;
> +       struct intel_huc *huc = &i915->huc;
> +       u32 guc_fw_size, huc_fw_size;
> +       u32 min_reserved_size;
> +
> +       guc_fw_size = round_up(guc->fw.size, I915_GTT_PAGE_SIZE);
> +       huc_fw_size = round_up(huc->fw.size, I915_GTT_PAGE_SIZE);
> +
> +       min_reserved_size = guc_fw_size + huc_fw_size;

In this patch, you should only be using GUC_GGTT_TOP so that the only
change is from excluding the zone to using a reserved node.


So why reserve room for both fw? You should only be binding them for the
xfer (so that you do not have to insert extraneous code outside of
reset).
-Chris