Re: [Intel-gfx] [PATCH v10 9/9] drm/i915/guc: Fix GuC cleanup in unload path
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. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v10 9/9] drm/i915/guc: Fix GuC cleanup in unload path
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v10 9/9] drm/i915/guc: Fix GuC cleanup in unload path
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v10 9/9] drm/i915/guc: Fix GuC cleanup in unload path
On 9/27/2017 10:41 PM, Michal Wajdeczko wrote: On Wed, 27 Sep 2017 11:30:39 +0200, Sagar Arun Kamble 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 Cc: Michal Wajdeczko Cc: Michał Winiarski Reviewed-by: Michal Wajdeczko --- 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 */ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lis
Re: [Intel-gfx] [PATCH v10 9/9] drm/i915/guc: Fix GuC cleanup in unload path
On Wed, 27 Sep 2017 11:30:39 +0200, Sagar Arun Kamble 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 Cc: Michal Wajdeczko Cc: Michał Winiarski Reviewed-by: Michal Wajdeczko --- 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 */ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v10 9/9] drm/i915/guc: Fix GuC cleanup in unload path
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 Cc: Michal Wajdeczko Cc: Michał Winiarski Reviewed-by: Michal Wajdeczko --- 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) + 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 */ -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx