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

[Fernando Pacheco]

On 4/9/19 2:41 PM, Chris Wilson wrote:
> Quoting Fernando Pacheco (2019-04-09 22:31:00)
>> 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.
>>
>> 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.h    | 11 +++++++++++
>>  3 files changed, 34 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 736c845eb77f..30f294a07e6d 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2747,6 +2747,18 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>>         if (ret)
>>                 return ret;
>>  
>> +       /* Reserve a mappable slot for our lockless uC firmware load */
>> +       if (USES_GUC(dev_priv)) {
>> +               ret = drm_mm_insert_node_in_range(&ggtt->vm.mm, &ggtt->uc_fw,
>> +                                                 GUC_GGTT_FW_SIZE, 0,
>> +                                                 I915_COLOR_UNEVICTABLE,
>> +                                                 GUC_GGTT_FW_START,
>> +                                                 GUC_GGTT_FW_END,
>> +                                                 DRM_MM_INSERT_LOW);
> Use drm_mm_reserve_node() as you want an explicit address.
>
> We should be able to push this to guc init, with appropriate bailing if
> something already took the high range.

I tried pushing this to guc init, but this bailed immediately as there are
pinnings between the calls to ggtt init and guc init. Did I misinterpret what
you had in mind?

Fernando

>
>> +               if (ret)
>> +                       goto err_node;
>> +       }
>> +
>>         /* 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",
>> @@ -2761,12 +2773,15 @@ 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:
>> +       if (USES_GUC(dev_priv))
>> +               drm_mm_remove_node(&ggtt->uc_fw);
>> +err_node:
>>         drm_mm_remove_node(&ggtt->error_capture);
>>         return ret;
>>  }
>> @@ -2792,6 +2807,9 @@ 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);
>>  
>> +       if (drm_mm_node_allocated(&ggtt->uc_fw))
>> +               drm_mm_remove_node(&ggtt->uc_fw);
>> +
>>         if (drm_mm_initialized(&ggtt->vm.mm)) {
>>                 intel_vgt_deballoon(dev_priv);
>>                 i915_address_space_fini(&ggtt->vm);
>> @@ -3370,7 +3388,8 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv)
>>          * restriction!
>>          */
>>         if (USES_GUC(dev_priv)) {
>> -               ggtt->vm.total = min_t(u64, ggtt->vm.total, GUC_GGTT_TOP);
>> +               ggtt->vm.total = min_t(u64, ggtt->vm.total,
>> +                                      GUC_GGTT_RESERVED_END);
> Should not be required now as you should have completely reserved the range
> for the guc, by the guc.