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

[Fernando Pacheco <fernando.pacheco@intel.com>]

On 4/19/19 12:14 AM, Chris Wilson wrote:
> 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@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).

Sorry. It took a while to understand the larger point you were making
in the first version of these patches. You're right. If I only bind them for the
transfer then there is no need to reserve room for both.

It is unlikely, but should there be a check in case GUC_GGTT_TOP exceeds vm.total and
adjust the start accordingly? Or should we let this bail? GEM_BUG_ON?

Thanks,
Fernando

> -Chris

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx