| Message ID | 20180308210036.33424-3-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Michal Wajdeczko (2018-03-08 21:00:35) > Instead of dancing around uC on reset/suspend/resume scenarios, > explicitly sanitize uC when we sanitize GEM to force uC reload > and start from known beginning. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michel Thierry <michel.thierry@intel.com> Lgtm, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Second opinions are welcome. > --- > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > drivers/gpu/drm/i915/intel_guc.h | 6 ++++++ > drivers/gpu/drm/i915/intel_huc.h | 6 ++++++ > drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++++++ > drivers/gpu/drm/i915/intel_uc.h | 1 + > drivers/gpu/drm/i915/intel_uc_fw.h | 6 ++++++ > 6 files changed, 39 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index ab88ca5..49c81ae 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4892,6 +4892,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915) > */ > if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915)) > WARN_ON(intel_gpu_reset(i915, ALL_ENGINES)); I think we can say that our reset works for gen3+ well enough to use everywhere. (Certainly gen3/gen4 are no worse than gen5!) Future task for the brave. -Chris
On 08/03/18 13:00, Michal Wajdeczko wrote: > Instead of dancing around uC on reset/suspend/resume scenarios, > explicitly sanitize uC when we sanitize GEM to force uC reload > and start from known beginning. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michel Thierry <michel.thierry@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > drivers/gpu/drm/i915/intel_guc.h | 6 ++++++ > drivers/gpu/drm/i915/intel_huc.h | 6 ++++++ > drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++++++ > drivers/gpu/drm/i915/intel_uc.h | 1 + > drivers/gpu/drm/i915/intel_uc_fw.h | 6 ++++++ > 6 files changed, 39 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index ab88ca5..49c81ae 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4892,6 +4892,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915) > */ > if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915)) > WARN_ON(intel_gpu_reset(i915, ALL_ENGINES)); > + > + intel_uc_sanitize(i915); > } > > int i915_gem_suspend(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > index b9424ac..ec8569f 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -132,4 +132,10 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma) > struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size); > u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv); > > +static inline int intel_guc_sanitize(struct intel_guc *guc) > +{ > + intel_uc_fw_sanitize(&guc->fw); > + return 0; > +} > + > #endif > diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h > index 5d6e804..b185850 100644 > --- a/drivers/gpu/drm/i915/intel_huc.h > +++ b/drivers/gpu/drm/i915/intel_huc.h > @@ -38,4 +38,10 @@ struct intel_huc { > void intel_huc_init_early(struct intel_huc *huc); > int intel_huc_auth(struct intel_huc *huc); > > +static inline int intel_huc_sanitize(struct intel_huc *huc) > +{ > + intel_uc_fw_sanitize(&huc->fw); > + return 0; > +} > + > #endif > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index a45171c..abd1f7b 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -327,6 +327,24 @@ void intel_uc_fini(struct drm_i915_private *dev_priv) > intel_guc_fini(guc); > } > > +void intel_uc_sanitize(struct drm_i915_private *i915) > +{ > + struct intel_guc *guc = &i915->guc; > + struct intel_huc *huc = &i915->huc; > + > + if (!USES_GUC(i915)) > + return; > + > + GEM_BUG_ON(!HAS_GUC(i915)); > + > + guc_disable_communication(guc); If you want to do this here for CT buffers you need to call this before resetting the GPU otherwise GuC will already be reset and won't be able to unregister the buffers. We can also just wipe the shared memory where the CT buffer info is stored but that doesn't sound too clean, although we might have to add the code for it anyway if we want to support the (unlikely) case where GuC dies on some error. Might also be interesting to release doorbells here to sync that state as well, but that's a task for a another time ;) BTW, are you planning to call this in the i915_reset flow as well or is the plan to add another dedicated function? Thanks, Daniele > + > + intel_huc_sanitize(huc); > + intel_guc_sanitize(guc); > + > + __intel_uc_reset_hw(i915); > +} > + > int intel_uc_init_hw(struct drm_i915_private *dev_priv) > { > struct intel_guc *guc = &dev_priv->guc; > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index dce4813..937e611 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -34,6 +34,7 @@ > void intel_uc_fini_fw(struct drm_i915_private *dev_priv); > int intel_uc_init_misc(struct drm_i915_private *dev_priv); > void intel_uc_fini_misc(struct drm_i915_private *dev_priv); > +void intel_uc_sanitize(struct drm_i915_private *dev_priv); > int intel_uc_init_hw(struct drm_i915_private *dev_priv); > void intel_uc_fini_hw(struct drm_i915_private *dev_priv); > int intel_uc_init(struct drm_i915_private *dev_priv); > diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h > index d5fd460..2601521 100644 > --- a/drivers/gpu/drm/i915/intel_uc_fw.h > +++ b/drivers/gpu/drm/i915/intel_uc_fw.h > @@ -115,6 +115,12 @@ static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw) > return uc_fw->path != NULL; > } > > +static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) > +{ > + if (uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS) > + uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING; > +} > + > void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, > struct intel_uc_fw *uc_fw); > int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, >
On 3/9/2018 2:30 AM, Michal Wajdeczko wrote: > Instead of dancing around uC on reset/suspend/resume scenarios, > explicitly sanitize uC when we sanitize GEM to force uC reload > and start from known beginning. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michel Thierry <michel.thierry@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > drivers/gpu/drm/i915/intel_guc.h | 6 ++++++ > drivers/gpu/drm/i915/intel_huc.h | 6 ++++++ > drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++++++ > drivers/gpu/drm/i915/intel_uc.h | 1 + > drivers/gpu/drm/i915/intel_uc_fw.h | 6 ++++++ > 6 files changed, 39 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index ab88ca5..49c81ae 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4892,6 +4892,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915) > */ > if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915)) > WARN_ON(intel_gpu_reset(i915, ALL_ENGINES)); above intel_gpu_reset also resets uC. Should we just let it reset only real engines with this change then? > + > + intel_uc_sanitize(i915); > } > > int i915_gem_suspend(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > index b9424ac..ec8569f 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -132,4 +132,10 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma) > struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size); > u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv); > > +static inline int intel_guc_sanitize(struct intel_guc *guc) > +{ > + intel_uc_fw_sanitize(&guc->fw); > + return 0; > +} > + > #endif > diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h > index 5d6e804..b185850 100644 > --- a/drivers/gpu/drm/i915/intel_huc.h > +++ b/drivers/gpu/drm/i915/intel_huc.h > @@ -38,4 +38,10 @@ struct intel_huc { > void intel_huc_init_early(struct intel_huc *huc); > int intel_huc_auth(struct intel_huc *huc); > > +static inline int intel_huc_sanitize(struct intel_huc *huc) > +{ > + intel_uc_fw_sanitize(&huc->fw); > + return 0; > +} > + > #endif > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index a45171c..abd1f7b 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -327,6 +327,24 @@ void intel_uc_fini(struct drm_i915_private *dev_priv) > intel_guc_fini(guc); > } > > +void intel_uc_sanitize(struct drm_i915_private *i915) > +{ > + struct intel_guc *guc = &i915->guc; > + struct intel_huc *huc = &i915->huc; > + > + if (!USES_GUC(i915)) > + return; > + > + GEM_BUG_ON(!HAS_GUC(i915)); > + > + guc_disable_communication(guc); > + > + intel_huc_sanitize(huc); > + intel_guc_sanitize(guc); > + > + __intel_uc_reset_hw(i915); > +} > + > int intel_uc_init_hw(struct drm_i915_private *dev_priv) > { > struct intel_guc *guc = &dev_priv->guc; > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index dce4813..937e611 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -34,6 +34,7 @@ > void intel_uc_fini_fw(struct drm_i915_private *dev_priv); > int intel_uc_init_misc(struct drm_i915_private *dev_priv); > void intel_uc_fini_misc(struct drm_i915_private *dev_priv); > +void intel_uc_sanitize(struct drm_i915_private *dev_priv); > int intel_uc_init_hw(struct drm_i915_private *dev_priv); > void intel_uc_fini_hw(struct drm_i915_private *dev_priv); > int intel_uc_init(struct drm_i915_private *dev_priv); > diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h > index d5fd460..2601521 100644 > --- a/drivers/gpu/drm/i915/intel_uc_fw.h > +++ b/drivers/gpu/drm/i915/intel_uc_fw.h > @@ -115,6 +115,12 @@ static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw) > return uc_fw->path != NULL; > } > > +static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) > +{ > + if (uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS) > + uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING; > +} > + > void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, > struct intel_uc_fw *uc_fw); > int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ab88ca5..49c81ae 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4892,6 +4892,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915) */ if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915)) WARN_ON(intel_gpu_reset(i915, ALL_ENGINES)); + + intel_uc_sanitize(i915); } int i915_gem_suspend(struct drm_i915_private *dev_priv) diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index b9424ac..ec8569f 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -132,4 +132,10 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma) struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size); u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv); +static inline int intel_guc_sanitize(struct intel_guc *guc) +{ + intel_uc_fw_sanitize(&guc->fw); + return 0; +} + #endif diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h index 5d6e804..b185850 100644 --- a/drivers/gpu/drm/i915/intel_huc.h +++ b/drivers/gpu/drm/i915/intel_huc.h @@ -38,4 +38,10 @@ struct intel_huc { void intel_huc_init_early(struct intel_huc *huc); int intel_huc_auth(struct intel_huc *huc); +static inline int intel_huc_sanitize(struct intel_huc *huc) +{ + intel_uc_fw_sanitize(&huc->fw); + return 0; +} + #endif diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index a45171c..abd1f7b 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -327,6 +327,24 @@ void intel_uc_fini(struct drm_i915_private *dev_priv) intel_guc_fini(guc); } +void intel_uc_sanitize(struct drm_i915_private *i915) +{ + struct intel_guc *guc = &i915->guc; + struct intel_huc *huc = &i915->huc; + + if (!USES_GUC(i915)) + return; + + GEM_BUG_ON(!HAS_GUC(i915)); + + guc_disable_communication(guc); + + intel_huc_sanitize(huc); + intel_guc_sanitize(guc); + + __intel_uc_reset_hw(i915); +} + int intel_uc_init_hw(struct drm_i915_private *dev_priv) { struct intel_guc *guc = &dev_priv->guc; diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index dce4813..937e611 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -34,6 +34,7 @@ void intel_uc_fini_fw(struct drm_i915_private *dev_priv); int intel_uc_init_misc(struct drm_i915_private *dev_priv); void intel_uc_fini_misc(struct drm_i915_private *dev_priv); +void intel_uc_sanitize(struct drm_i915_private *dev_priv); int intel_uc_init_hw(struct drm_i915_private *dev_priv); void intel_uc_fini_hw(struct drm_i915_private *dev_priv); int intel_uc_init(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h index d5fd460..2601521 100644 --- a/drivers/gpu/drm/i915/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/intel_uc_fw.h @@ -115,6 +115,12 @@ static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw) return uc_fw->path != NULL; } +static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) +{ + if (uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS) + uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING; +} + void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, struct intel_uc_fw *uc_fw); int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
Instead of dancing around uC on reset/suspend/resume scenarios, explicitly sanitize uC when we sanitize GEM to force uC reload and start from known beginning. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michel Thierry <michel.thierry@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 2 ++ drivers/gpu/drm/i915/intel_guc.h | 6 ++++++ drivers/gpu/drm/i915/intel_huc.h | 6 ++++++ drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++++++ drivers/gpu/drm/i915/intel_uc.h | 1 + drivers/gpu/drm/i915/intel_uc_fw.h | 6 ++++++ 6 files changed, 39 insertions(+)