| Message ID | 1506581329-29720-7-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote: > During GuC load/enable, state is setup by driver that can be looked at > while disabling. So remove the check for i915.enable_guc_submission > parameter in those functions. > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > 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> <SNIP> > @@ -1002,7 +1002,8 @@ static int guc_ads_create(struct intel_guc *guc) > > static void guc_ads_destroy(struct intel_guc *guc) > { > - i915_vma_unpin_and_release(&guc->ads_vma); > + if (guc->ads_vma) GEM_BUG_ON(!guc->ads_vma); > + i915_vma_unpin_and_release(&guc->ads_vma); > } > > /* > @@ -1060,11 +1061,14 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv) > { > struct intel_guc *guc = &dev_priv->guc; > > + WARN_ON_ONCE(!ida_is_empty(&guc->stage_ids)); > ida_destroy(&guc->stage_ids); > guc_ads_destroy(guc); > intel_guc_log_destroy(guc); > - i915_gem_object_unpin_map(guc->stage_desc_pool->obj); > - i915_vma_unpin_and_release(&guc->stage_desc_pool); > + if (guc->stage_desc_pool) { GEM_BUG_ON(!guc->stage_desc_pol) is the right thing. > + i915_gem_object_unpin_map(guc->stage_desc_pool->obj); > + i915_vma_unpin_and_release(&guc->stage_desc_pool); > + } I'm generally against conditional teardown. If the _init did not fully succeed, the _fini is never supposed to be called. > static void guc_interrupts_capture(struct drm_i915_private *dev_priv) > @@ -1204,6 +1208,9 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv) > { > struct intel_guc *guc = &dev_priv->guc; > We may want document pre-requirements assert_lockdep_held() in enable/disable submission funcs, for a good measure. Then it'll be easier to convert away from struct_mutex when the time comes. > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c > index 6571d96..73333b6 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -584,7 +584,8 @@ int intel_guc_log_create(struct intel_guc *guc) > void intel_guc_log_destroy(struct intel_guc *guc) > { > guc_log_runtime_destroy(guc); > - i915_vma_unpin_and_release(&guc->log.vma); > + if (guc->log.vma) > + i915_vma_unpin_and_release(&guc->log.vma); Again, GEM_BUG_ON(!guc->log.vma); > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -445,8 +445,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > err_log_capture: > guc_capture_load_err_log(guc); > err_submission: > - if (i915_modparams.enable_guc_submission) > - i915_guc_submission_fini(dev_priv); > + i915_guc_submission_fini(dev_priv); No, no unconditional calling of _fini if the _init is not uncoditional too. You can drop both checks down to the submission_init/_fini funcs if you want to. For me it's more clear if they're here. Inside the funcs or right before calling them, when called just from one place (like I'd prefer here), but most importantly it has to be symmetric. Regards, Joonas
On 9/29/2017 5:57 PM, Joonas Lahtinen wrote: > On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote: >> During GuC load/enable, state is setup by driver that can be looked at >> while disabling. So remove the check for i915.enable_guc_submission >> parameter in those functions. >> >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> >> 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> > <SNIP> > >> @@ -1002,7 +1002,8 @@ static int guc_ads_create(struct intel_guc *guc) >> >> static void guc_ads_destroy(struct intel_guc *guc) >> { >> - i915_vma_unpin_and_release(&guc->ads_vma); >> + if (guc->ads_vma) > GEM_BUG_ON(!guc->ads_vma); This check was unnecessary. Suggestion from Chris was to make these destroys be self-check based instead of invoking based on module parameters like enable_guc_submission/loading. In my new patch I have removed these checks. https://patchwork.freedesktop.org/patch/179683/ > >> + i915_vma_unpin_and_release(&guc->ads_vma); >> } >> >> /* >> @@ -1060,11 +1061,14 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv) >> { >> struct intel_guc *guc = &dev_priv->guc; >> >> + WARN_ON_ONCE(!ida_is_empty(&guc->stage_ids)); >> ida_destroy(&guc->stage_ids); >> guc_ads_destroy(guc); >> intel_guc_log_destroy(guc); >> - i915_gem_object_unpin_map(guc->stage_desc_pool->obj); >> - i915_vma_unpin_and_release(&guc->stage_desc_pool); >> + if (guc->stage_desc_pool) { > GEM_BUG_ON(!guc->stage_desc_pol) is the right thing. stage_desc_pool check is used to enter submission_fini in my latest patch. Other than that there are no more checks needed. https://patchwork.freedesktop.org/patch/179683/ > >> + i915_gem_object_unpin_map(guc->stage_desc_pool->obj); >> + i915_vma_unpin_and_release(&guc->stage_desc_pool); >> + } > I'm generally against conditional teardown. If the _init did not fully > succeed, the _fini is never supposed to be called. Plan is to replace the module parameter based condition to driver state based condition. >> static void guc_interrupts_capture(struct drm_i915_private *dev_priv) >> @@ -1204,6 +1208,9 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv) >> { >> struct intel_guc *guc = &dev_priv->guc; >> > We may want document pre-requirements assert_lockdep_held() in > enable/disable submission funcs, for a good measure. Then it'll be > easier to convert away from struct_mutex when the time comes. We removed lockdep assert as mutex is needed by internal functions which already have the asserts. > >> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c >> index 6571d96..73333b6 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_log.c >> +++ b/drivers/gpu/drm/i915/intel_guc_log.c >> @@ -584,7 +584,8 @@ int intel_guc_log_create(struct intel_guc *guc) >> void intel_guc_log_destroy(struct intel_guc *guc) >> { >> guc_log_runtime_destroy(guc); >> - i915_vma_unpin_and_release(&guc->log.vma); >> + if (guc->log.vma) >> + i915_vma_unpin_and_release(&guc->log.vma); > Again, GEM_BUG_ON(!guc->log.vma); This is unnecessary check. > >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -445,8 +445,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) >> err_log_capture: >> guc_capture_load_err_log(guc); >> err_submission: >> - if (i915_modparams.enable_guc_submission) >> - i915_guc_submission_fini(dev_priv); >> + i915_guc_submission_fini(dev_priv); > No, no unconditional calling of _fini if the _init is not uncoditional > too. You can drop both checks down to the submission_init/_fini funcs > if you want to. For me it's more clear if they're here. > > Inside the funcs or right before calling them, when called just from > one place (like I'd prefer here), but most importantly it has to be > symmetric. > > Regards, Joonas _fini is still conditional, but inside the function based on various states set.
On Sat, 2017-09-30 at 13:52 +0530, Sagar Arun Kamble wrote: > > On 9/29/2017 5:57 PM, Joonas Lahtinen wrote: > > On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote: > > > During GuC load/enable, state is setup by driver that can be looked at > > > while disabling. So remove the check for i915.enable_guc_submission > > > parameter in those functions. > > > > > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > > > 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> > > > > <SNIP> > > > > > @@ -1002,7 +1002,8 @@ static int guc_ads_create(struct intel_guc *guc) > > > > > > static void guc_ads_destroy(struct intel_guc *guc) > > > { > > > - i915_vma_unpin_and_release(&guc->ads_vma); > > > + if (guc->ads_vma) > > > > GEM_BUG_ON(!guc->ads_vma); > > This check was unnecessary. Suggestion from Chris was to make these > destroys be self-check based instead of invoking > based on module parameters like enable_guc_submission/loading. In my new > patch I have removed these checks. > https://patchwork.freedesktop.org/patch/179683/ I pinged Chris about this. I do prefer the symmetry, and I think we could have guc->enable_submission variable based on which i915_guc_submission_init/fini() would be executed on. No conditionals in the _init/_fini themselves. You can of course do the enable_submission check at the beginning of i915_guc_submission_init and _fini too, if that feels clearer for the upper level code flow. If _init is called and succeeds, _fini needs to be called. If _init fails, _fini is to never be called. The check outside or inside will both adhere to that. Regards, Joonas
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 04f1281..a2f67ca 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -1002,7 +1002,8 @@ static int guc_ads_create(struct intel_guc *guc) static void guc_ads_destroy(struct intel_guc *guc) { - i915_vma_unpin_and_release(&guc->ads_vma); + if (guc->ads_vma) + i915_vma_unpin_and_release(&guc->ads_vma); } /* @@ -1060,11 +1061,14 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv) { struct intel_guc *guc = &dev_priv->guc; + WARN_ON_ONCE(!ida_is_empty(&guc->stage_ids)); ida_destroy(&guc->stage_ids); guc_ads_destroy(guc); intel_guc_log_destroy(guc); - i915_gem_object_unpin_map(guc->stage_desc_pool->obj); - i915_vma_unpin_and_release(&guc->stage_desc_pool); + if (guc->stage_desc_pool) { + i915_gem_object_unpin_map(guc->stage_desc_pool->obj); + i915_vma_unpin_and_release(&guc->stage_desc_pool); + } } static void guc_interrupts_capture(struct drm_i915_private *dev_priv) @@ -1204,6 +1208,9 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv) { struct intel_guc *guc = &dev_priv->guc; + if (!guc->execbuf_client) + return; + guc_interrupts_release(dev_priv); /* Revert back to manual ELSP submission */ diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 0b756213..75406ee 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -455,6 +455,9 @@ void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv) void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv) { + if (READ_ONCE(dev_priv->guc.interrupts_enabled)) + return; + spin_lock_irq(&dev_priv->irq_lock); if (!dev_priv->guc.interrupts_enabled) { WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & @@ -467,6 +470,9 @@ void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv) void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv) { + if (!READ_ONCE(dev_priv->guc.interrupts_enabled)) + return; + spin_lock_irq(&dev_priv->irq_lock); dev_priv->guc.interrupts_enabled = false; diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 6571d96..73333b6 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -584,7 +584,8 @@ int intel_guc_log_create(struct intel_guc *guc) void intel_guc_log_destroy(struct intel_guc *guc) { guc_log_runtime_destroy(guc); - i915_vma_unpin_and_release(&guc->log.vma); + if (guc->log.vma) + i915_vma_unpin_and_release(&guc->log.vma); } int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) @@ -653,9 +654,6 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv) void i915_guc_log_unregister(struct drm_i915_private *dev_priv) { - if (!i915_modparams.enable_guc_submission) - return; - mutex_lock(&dev_priv->drm.struct_mutex); /* GuC logging is currently the only user of Guc2Host interrupts */ gen9_disable_guc_interrupts(dev_priv); diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index ab26232..ea7c39c 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -445,8 +445,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) err_log_capture: guc_capture_load_err_log(guc); err_submission: - if (i915_modparams.enable_guc_submission) - i915_guc_submission_fini(dev_priv); + i915_guc_submission_fini(dev_priv); err_guc: i915_ggtt_disable_guc(dev_priv); @@ -475,15 +474,12 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) if (!i915_modparams.enable_guc_loading) return; - if (i915_modparams.enable_guc_submission) - i915_guc_submission_disable(dev_priv); + i915_guc_submission_disable(dev_priv); guc_disable_communication(&dev_priv->guc); - if (i915_modparams.enable_guc_submission) { - gen9_disable_guc_interrupts(dev_priv); - i915_guc_submission_fini(dev_priv); - } + gen9_disable_guc_interrupts(dev_priv); + i915_guc_submission_fini(dev_priv); i915_ggtt_disable_guc(dev_priv); }
During GuC load/enable, state is setup by driver that can be looked at while disabling. So remove the check for i915.enable_guc_submission parameter in those functions. Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> 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> --- drivers/gpu/drm/i915/i915_guc_submission.c | 13 ++++++++++--- drivers/gpu/drm/i915/i915_irq.c | 6 ++++++ drivers/gpu/drm/i915/intel_guc_log.c | 6 ++---- drivers/gpu/drm/i915/intel_uc.c | 12 ++++-------- 4 files changed, 22 insertions(+), 15 deletions(-)