| Message ID | 1507712056-25030-5-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 11 Oct 2017 10:53:59 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > With guc_log_level parameter sanitized and GuC interrupts control > functions made self sufficient w.r.t interrupts state, we can remove > the enable_guc_submission checks from flush_guc_logs and > i915_guc_log_register/unregister and intel_uc_fini_hw. > > 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> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_guc_log.c | 9 ++------- > drivers/gpu/drm/i915/intel_uc.c | 3 +-- > 2 files changed, 3 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c > b/drivers/gpu/drm/i915/intel_guc_log.c > index 0f201c0..fb5eb2b9 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -505,8 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > - if (!i915_guc_submission_enabled(guc) || > - i915_modparams.guc_log_level < 0) > + if (i915_modparams.guc_log_level < 0) > return; > /* First disable the interrupts, will be renabled afterwards */ > @@ -646,8 +645,7 @@ int i915_guc_log_control(struct drm_i915_private > *dev_priv, u64 control_val) > void i915_guc_log_register(struct drm_i915_private *dev_priv) > { > - if (!i915_guc_submission_enabled(&dev_priv->guc) || > - i915_modparams.guc_log_level < 0) > + if (i915_modparams.guc_log_level < 0) I would expect above two fixes in patch 2/21 > return; > mutex_lock(&dev_priv->drm.struct_mutex); > @@ -657,9 +655,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_guc_submission_enabled(&dev_priv->guc)) > - return; Hmm, as i915_guc_log_unregister() is called unconditionally by i915_driver_unregister so maybe here we should have at least if (i915_modparams.guc_log_level < 0) return; > - > mutex_lock(&dev_priv->drm.struct_mutex); > /* GuC logging is currently the only user of Guc2Host interrupts */ > gen9_disable_guc_interrupts(dev_priv); What about dropping this gen9_disable_guc_interrupts from here and rely on the intel_uc_fini_hw() that will do the same? > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index 3cf3cbd..974434e 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -280,8 +280,7 @@ void intel_uc_fini_hw(struct drm_i915_private > *dev_priv) > guc_disable_communication(guc); > - if (i915_guc_submission_enabled(guc)) > - gen9_disable_guc_interrupts(dev_priv); > + gen9_disable_guc_interrupts(dev_priv); > i915_guc_submission_fini(dev_priv); > i915_ggtt_disable_guc(dev_priv);
On 10/11/2017 9:10 PM, Michal Wajdeczko wrote: > On Wed, 11 Oct 2017 10:53:59 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> With guc_log_level parameter sanitized and GuC interrupts control >> functions made self sufficient w.r.t interrupts state, we can remove >> the enable_guc_submission checks from flush_guc_logs and >> i915_guc_log_register/unregister and intel_uc_fini_hw. >> >> 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> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_guc_log.c | 9 ++------- >> drivers/gpu/drm/i915/intel_uc.c | 3 +-- >> 2 files changed, 3 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c >> b/drivers/gpu/drm/i915/intel_guc_log.c >> index 0f201c0..fb5eb2b9 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_log.c >> +++ b/drivers/gpu/drm/i915/intel_guc_log.c >> @@ -505,8 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc) >> { >> struct drm_i915_private *dev_priv = guc_to_i915(guc); >> - if (!i915_guc_submission_enabled(guc) || >> - i915_modparams.guc_log_level < 0) >> + if (i915_modparams.guc_log_level < 0) >> return; >> /* First disable the interrupts, will be renabled afterwards */ >> @@ -646,8 +645,7 @@ int i915_guc_log_control(struct drm_i915_private >> *dev_priv, u64 control_val) >> void i915_guc_log_register(struct drm_i915_private *dev_priv) >> { >> - if (!i915_guc_submission_enabled(&dev_priv->guc) || >> - i915_modparams.guc_log_level < 0) >> + if (i915_modparams.guc_log_level < 0) > > I would expect above two fixes in patch 2/21 Since there are multiple places needing the update and for better division I had created separate patches. Will keep same for now. > >> return; >> mutex_lock(&dev_priv->drm.struct_mutex); >> @@ -657,9 +655,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_guc_submission_enabled(&dev_priv->guc)) >> - return; > > Hmm, as i915_guc_log_unregister() is called unconditionally by > i915_driver_unregister so maybe here we should have at least > > if (i915_modparams.guc_log_level < 0) > return; Yes. Will add this. > >> - >> mutex_lock(&dev_priv->drm.struct_mutex); >> /* GuC logging is currently the only user of Guc2Host interrupts */ >> gen9_disable_guc_interrupts(dev_priv); > > What about dropping this gen9_disable_guc_interrupts from here > and rely on the intel_uc_fini_hw() that will do the same? > This needs to be handled separately as if we remove it from here we will have to synchronize the logging irq with relay runtime destruction. Will defer this for now. >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c >> index 3cf3cbd..974434e 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -280,8 +280,7 @@ void intel_uc_fini_hw(struct drm_i915_private >> *dev_priv) >> guc_disable_communication(guc); >> - if (i915_guc_submission_enabled(guc)) >> - gen9_disable_guc_interrupts(dev_priv); >> + gen9_disable_guc_interrupts(dev_priv); >> i915_guc_submission_fini(dev_priv); >> i915_ggtt_disable_guc(dev_priv); >
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 0f201c0..fb5eb2b9 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -505,8 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); - if (!i915_guc_submission_enabled(guc) || - i915_modparams.guc_log_level < 0) + if (i915_modparams.guc_log_level < 0) return; /* First disable the interrupts, will be renabled afterwards */ @@ -646,8 +645,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) void i915_guc_log_register(struct drm_i915_private *dev_priv) { - if (!i915_guc_submission_enabled(&dev_priv->guc) || - i915_modparams.guc_log_level < 0) + if (i915_modparams.guc_log_level < 0) return; mutex_lock(&dev_priv->drm.struct_mutex); @@ -657,9 +655,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_guc_submission_enabled(&dev_priv->guc)) - 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 3cf3cbd..974434e 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -280,8 +280,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) guc_disable_communication(guc); - if (i915_guc_submission_enabled(guc)) - gen9_disable_guc_interrupts(dev_priv); + gen9_disable_guc_interrupts(dev_priv); i915_guc_submission_fini(dev_priv); i915_ggtt_disable_guc(dev_priv);
With guc_log_level parameter sanitized and GuC interrupts control functions made self sufficient w.r.t interrupts state, we can remove the enable_guc_submission checks from flush_guc_logs and i915_guc_log_register/unregister and intel_uc_fini_hw. 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> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/intel_guc_log.c | 9 ++------- drivers/gpu/drm/i915/intel_uc.c | 3 +-- 2 files changed, 3 insertions(+), 9 deletions(-)