| Message ID | 1506504639-25631-10-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 27 Sep 2017 11:30:39 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > We ensure that GuC is completely suspended and client is destroyed > in i915_gem_suspend during i915_driver_unload. So now intel_uc_fini_hw > should just take care of cleanup, > hence s/intel_uc_fini_hw/intel_uc_cleanup. Correspondingly > we also updated as s/i915_guc_submission_fini/i915_guc_submission_cleanup > Other functionality to disable communication, disable interrupts and > update of ggtt.invalidate is taken care by intel_uc_suspend. > > v2: Rebase w.r.t removal of GuC code restructuring. > > v3: Removed intel_guc_cleanup. (Michal Wajdeczko) > > v4: guc_free_load_err_log() needs to be called without checking > i915.enable_guc_loading as this param is cleared on GuC load failure. > (Michal Wajdeczko) > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 2 +- > drivers/gpu/drm/i915/i915_guc_submission.c | 2 +- > drivers/gpu/drm/i915/intel_uc.c | 14 ++++---------- > drivers/gpu/drm/i915/intel_uc.h | 4 ++-- > 4 files changed, 8 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c > index f79646b..4223cee 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -603,7 +603,7 @@ static void i915_gem_fini(struct drm_i915_private > *dev_priv) > i915_gem_drain_workqueue(dev_priv); > mutex_lock(&dev_priv->drm.struct_mutex); > - intel_uc_fini_hw(dev_priv); > + intel_uc_cleanup(dev_priv); > i915_gem_cleanup_engines(dev_priv); > i915_gem_contexts_fini(dev_priv); > i915_gem_cleanup_userptr(dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c > b/drivers/gpu/drm/i915/i915_guc_submission.c > index 45f2ee8..0ac9bd4 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -1053,7 +1053,7 @@ int i915_guc_submission_init(struct > drm_i915_private *dev_priv) > return ret; > } > -void i915_guc_submission_fini(struct drm_i915_private *dev_priv) > +void i915_guc_submission_cleanup(struct drm_i915_private *dev_priv) > { > struct intel_guc *guc = &dev_priv->guc; > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index 59e6995..0370265 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -439,7 +439,7 @@ int intel_uc_init_hw(struct drm_i915_private > *dev_priv) > guc_capture_load_err_log(guc); > err_submission: > if (i915_modparams.enable_guc_submission) > - i915_guc_submission_fini(dev_priv); > + i915_guc_submission_cleanup(dev_priv); > err_guc: > i915_ggtt_disable_guc(dev_priv); > @@ -461,21 +461,15 @@ int intel_uc_init_hw(struct drm_i915_private > *dev_priv) > return ret; > } > -void intel_uc_fini_hw(struct drm_i915_private *dev_priv) > +void intel_uc_cleanup(struct drm_i915_private *dev_priv) > { > guc_free_load_err_log(&dev_priv->guc); > if (!i915_modparams.enable_guc_loading) > return; > - guc_disable_communication(&dev_priv->guc); > - > - if (i915_modparams.enable_guc_submission) { > - gen9_disable_guc_interrupts(dev_priv); > - i915_guc_submission_fini(dev_priv); > - } > - > - i915_ggtt_disable_guc(dev_priv); > + if (i915_modparams.enable_guc_submission) In patch 6/9 you said we should avoid looking at user params but here you're still using it ... > + i915_guc_submission_cleanup(dev_priv); > } > /** > diff --git a/drivers/gpu/drm/i915/intel_uc.h > b/drivers/gpu/drm/i915/intel_uc.h > index 78ccbd9..838a364 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -207,7 +207,7 @@ struct intel_huc { > void intel_uc_init_fw(struct drm_i915_private *dev_priv); > void intel_uc_fini_fw(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); > +void intel_uc_cleanup(struct drm_i915_private *dev_priv); > int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv); > int intel_uc_runtime_resume(struct drm_i915_private *dev_priv); > int intel_uc_suspend(struct drm_i915_private *dev_priv); > @@ -239,7 +239,7 @@ static inline void intel_guc_notify(struct intel_guc > *guc) > int i915_guc_submission_init(struct drm_i915_private *dev_priv); > int i915_guc_submission_enable(struct drm_i915_private *dev_priv); > void i915_guc_submission_disable(struct drm_i915_private *dev_priv); > -void i915_guc_submission_fini(struct drm_i915_private *dev_priv); > +void i915_guc_submission_cleanup(struct drm_i915_private *dev_priv); > struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 > size); > /* intel_guc_log.c */
On 9/27/2017 10:41 PM, Michal Wajdeczko wrote: > On Wed, 27 Sep 2017 11:30:39 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> We ensure that GuC is completely suspended and client is destroyed >> in i915_gem_suspend during i915_driver_unload. So now intel_uc_fini_hw >> should just take care of cleanup, >> hence s/intel_uc_fini_hw/intel_uc_cleanup. Correspondingly >> we also updated as >> s/i915_guc_submission_fini/i915_guc_submission_cleanup >> Other functionality to disable communication, disable interrupts and >> update of ggtt.invalidate is taken care by intel_uc_suspend. >> >> v2: Rebase w.r.t removal of GuC code restructuring. >> >> v3: Removed intel_guc_cleanup. (Michal Wajdeczko) >> >> v4: guc_free_load_err_log() needs to be called without checking >> i915.enable_guc_loading as this param is cleared on GuC load failure. >> (Michal Wajdeczko) >> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Michał Winiarski <michal.winiarski@intel.com> >> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 2 +- >> drivers/gpu/drm/i915/i915_guc_submission.c | 2 +- >> drivers/gpu/drm/i915/intel_uc.c | 14 ++++---------- >> drivers/gpu/drm/i915/intel_uc.h | 4 ++-- >> 4 files changed, 8 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c >> b/drivers/gpu/drm/i915/i915_drv.c >> index f79646b..4223cee 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -603,7 +603,7 @@ static void i915_gem_fini(struct drm_i915_private >> *dev_priv) >> i915_gem_drain_workqueue(dev_priv); >> mutex_lock(&dev_priv->drm.struct_mutex); >> - intel_uc_fini_hw(dev_priv); >> + intel_uc_cleanup(dev_priv); >> i915_gem_cleanup_engines(dev_priv); >> i915_gem_contexts_fini(dev_priv); >> i915_gem_cleanup_userptr(dev_priv); >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >> b/drivers/gpu/drm/i915/i915_guc_submission.c >> index 45f2ee8..0ac9bd4 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -1053,7 +1053,7 @@ int i915_guc_submission_init(struct >> drm_i915_private *dev_priv) >> return ret; >> } >> -void i915_guc_submission_fini(struct drm_i915_private *dev_priv) >> +void i915_guc_submission_cleanup(struct drm_i915_private *dev_priv) >> { >> struct intel_guc *guc = &dev_priv->guc; >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c >> index 59e6995..0370265 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -439,7 +439,7 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> guc_capture_load_err_log(guc); >> err_submission: >> if (i915_modparams.enable_guc_submission) >> - i915_guc_submission_fini(dev_priv); >> + i915_guc_submission_cleanup(dev_priv); >> err_guc: >> i915_ggtt_disable_guc(dev_priv); >> @@ -461,21 +461,15 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> return ret; >> } >> -void intel_uc_fini_hw(struct drm_i915_private *dev_priv) >> +void intel_uc_cleanup(struct drm_i915_private *dev_priv) >> { >> guc_free_load_err_log(&dev_priv->guc); >> if (!i915_modparams.enable_guc_loading) >> return; >> - guc_disable_communication(&dev_priv->guc); >> - >> - if (i915_modparams.enable_guc_submission) { >> - gen9_disable_guc_interrupts(dev_priv); >> - i915_guc_submission_fini(dev_priv); >> - } >> - >> - i915_ggtt_disable_guc(dev_priv); >> + if (i915_modparams.enable_guc_submission) > > In patch 6/9 you said we should avoid looking at user params > but here you're still using it ... > Yes. I thought of taking that up in new series. Should I add another patch in this series itself to update such snippets? >> + i915_guc_submission_cleanup(dev_priv); >> } >> /** >> diff --git a/drivers/gpu/drm/i915/intel_uc.h >> b/drivers/gpu/drm/i915/intel_uc.h >> index 78ccbd9..838a364 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.h >> +++ b/drivers/gpu/drm/i915/intel_uc.h >> @@ -207,7 +207,7 @@ struct intel_huc { >> void intel_uc_init_fw(struct drm_i915_private *dev_priv); >> void intel_uc_fini_fw(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); >> +void intel_uc_cleanup(struct drm_i915_private *dev_priv); >> int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv); >> int intel_uc_runtime_resume(struct drm_i915_private *dev_priv); >> int intel_uc_suspend(struct drm_i915_private *dev_priv); >> @@ -239,7 +239,7 @@ static inline void intel_guc_notify(struct >> intel_guc *guc) >> int i915_guc_submission_init(struct drm_i915_private *dev_priv); >> int i915_guc_submission_enable(struct drm_i915_private *dev_priv); >> void i915_guc_submission_disable(struct drm_i915_private *dev_priv); >> -void i915_guc_submission_fini(struct drm_i915_private *dev_priv); >> +void i915_guc_submission_cleanup(struct drm_i915_private *dev_priv); >> struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 >> size); >> /* intel_guc_log.c */
Quoting Sagar Arun Kamble (2017-09-27 10:30:39) > -void intel_uc_fini_hw(struct drm_i915_private *dev_priv) > +void intel_uc_cleanup(struct drm_i915_private *dev_priv) > { > guc_free_load_err_log(&dev_priv->guc); > > if (!i915_modparams.enable_guc_loading) > return; > > - guc_disable_communication(&dev_priv->guc); > - > - if (i915_modparams.enable_guc_submission) { > - gen9_disable_guc_interrupts(dev_priv); > - i915_guc_submission_fini(dev_priv); > - } > - > - i915_ggtt_disable_guc(dev_priv); > + if (i915_modparams.enable_guc_submission) > + i915_guc_submission_cleanup(dev_priv); My preference would be for if (!guc->stage_desc_pool) return; inside i915_guc_submission_cleanup(). -Chris
On 9/28/2017 5:25 PM, Chris Wilson wrote: > Quoting Sagar Arun Kamble (2017-09-27 10:30:39) >> -void intel_uc_fini_hw(struct drm_i915_private *dev_priv) >> +void intel_uc_cleanup(struct drm_i915_private *dev_priv) >> { >> guc_free_load_err_log(&dev_priv->guc); >> >> if (!i915_modparams.enable_guc_loading) >> return; >> >> - guc_disable_communication(&dev_priv->guc); >> - >> - if (i915_modparams.enable_guc_submission) { >> - gen9_disable_guc_interrupts(dev_priv); >> - i915_guc_submission_fini(dev_priv); >> - } >> - >> - i915_ggtt_disable_guc(dev_priv); >> + if (i915_modparams.enable_guc_submission) >> + i915_guc_submission_cleanup(dev_priv); > My preference would be for if (!guc->stage_desc_pool) return; inside > i915_guc_submission_cleanup(). > -Chris Yes. I have taken similar input in the latest patch - https://patchwork.freedesktop.org/patch/179405/ In i915_guc_submission_cleanup we can cover destroy of stage_ids and stage_desc_pool based on check you have suggested. guc_ads_destroy is always required data so should we link with stage_desc_pool check? intel_guc_log is optional so destroy need to be made dependent on guc->log.vma
On 9/28/2017 6:45 PM, Sagar Arun Kamble wrote: > > > On 9/28/2017 5:25 PM, Chris Wilson wrote: >> Quoting Sagar Arun Kamble (2017-09-27 10:30:39) >>> -void intel_uc_fini_hw(struct drm_i915_private *dev_priv) >>> +void intel_uc_cleanup(struct drm_i915_private *dev_priv) >>> { >>> guc_free_load_err_log(&dev_priv->guc); >>> if (!i915_modparams.enable_guc_loading) >>> return; >>> - guc_disable_communication(&dev_priv->guc); >>> - >>> - if (i915_modparams.enable_guc_submission) { >>> - gen9_disable_guc_interrupts(dev_priv); >>> - i915_guc_submission_fini(dev_priv); >>> - } >>> - >>> - i915_ggtt_disable_guc(dev_priv); >>> + if (i915_modparams.enable_guc_submission) >>> + i915_guc_submission_cleanup(dev_priv); >> My preference would be for if (!guc->stage_desc_pool) return; inside >> i915_guc_submission_cleanup(). >> -Chris > Yes. I have taken similar input in the latest patch - > https://patchwork.freedesktop.org/patch/179405/ > In i915_guc_submission_cleanup we can cover destroy of stage_ids and > stage_desc_pool based on check you have suggested. > guc_ads_destroy is always required data so should we link with > stage_desc_pool check? > intel_guc_log is optional so destroy need to be made dependent on > guc->log.vma Realized that vma need not be checked so the check as you suggested looks more subtle here.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index f79646b..4223cee 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -603,7 +603,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv) i915_gem_drain_workqueue(dev_priv); mutex_lock(&dev_priv->drm.struct_mutex); - intel_uc_fini_hw(dev_priv); + intel_uc_cleanup(dev_priv); i915_gem_cleanup_engines(dev_priv); i915_gem_contexts_fini(dev_priv); i915_gem_cleanup_userptr(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 45f2ee8..0ac9bd4 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -1053,7 +1053,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv) return ret; } -void i915_guc_submission_fini(struct drm_i915_private *dev_priv) +void i915_guc_submission_cleanup(struct drm_i915_private *dev_priv) { struct intel_guc *guc = &dev_priv->guc; diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 59e6995..0370265 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -439,7 +439,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) guc_capture_load_err_log(guc); err_submission: if (i915_modparams.enable_guc_submission) - i915_guc_submission_fini(dev_priv); + i915_guc_submission_cleanup(dev_priv); err_guc: i915_ggtt_disable_guc(dev_priv); @@ -461,21 +461,15 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) return ret; } -void intel_uc_fini_hw(struct drm_i915_private *dev_priv) +void intel_uc_cleanup(struct drm_i915_private *dev_priv) { guc_free_load_err_log(&dev_priv->guc); if (!i915_modparams.enable_guc_loading) return; - guc_disable_communication(&dev_priv->guc); - - if (i915_modparams.enable_guc_submission) { - gen9_disable_guc_interrupts(dev_priv); - i915_guc_submission_fini(dev_priv); - } - - i915_ggtt_disable_guc(dev_priv); + if (i915_modparams.enable_guc_submission) + i915_guc_submission_cleanup(dev_priv); } /** diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index 78ccbd9..838a364 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -207,7 +207,7 @@ struct intel_huc { void intel_uc_init_fw(struct drm_i915_private *dev_priv); void intel_uc_fini_fw(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); +void intel_uc_cleanup(struct drm_i915_private *dev_priv); int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv); int intel_uc_runtime_resume(struct drm_i915_private *dev_priv); int intel_uc_suspend(struct drm_i915_private *dev_priv); @@ -239,7 +239,7 @@ static inline void intel_guc_notify(struct intel_guc *guc) int i915_guc_submission_init(struct drm_i915_private *dev_priv); int i915_guc_submission_enable(struct drm_i915_private *dev_priv); void i915_guc_submission_disable(struct drm_i915_private *dev_priv); -void i915_guc_submission_fini(struct drm_i915_private *dev_priv); +void i915_guc_submission_cleanup(struct drm_i915_private *dev_priv); struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size); /* intel_guc_log.c */