| Message ID | 20190418233151.17510-3-fernando.pacheco@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Perma-pin uC firmware and re-enable global reset | expand |
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). -Chris
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
Quoting Fernando Pacheco (2019-04-19 18:14:21) > > On 4/19/19 12:14 AM, Chris Wilson wrote: > > Quoting Fernando Pacheco (2019-04-19 00:31:48) > >> - /* 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); > >> - } [snip > 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? If GUC_GGTT_TOP is outside the range of vm.total, that should give us a bogus address for the drm_mm_reserve_node, and that should fail. That seems reasonable, if we can't support the guc with the current GGTT setup we probably do want to bail and get it fixed (as something would be very wrong with the GGTT probe, one presumes). -Chris
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; + + return min_t(u32, GUC_GGTT_TOP, ggtt->vm.total - min_reserved_size); +} + +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 start, size; + int ret; + + start = __intel_guc_ggtt_top_offset(guc); + size = ggtt->vm.total - start; + + ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size, + start, 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 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 intel_guc *guc); int intel_guc_resume(struct intel_guc *guc); struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size); u32 intel_guc_reserved_gtt_size(struct intel_guc *guc); +int intel_guc_reserve_ggtt_top(struct intel_guc *guc); +void intel_guc_release_ggtt_top(struct intel_guc *guc); static inline int intel_guc_sanitize(struct intel_guc *guc) {
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(-)