| Message ID | 1508309222-26406-8-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/10/2017 07:46, Sagar Arun Kamble wrote: > Disabling GuC interrupts involves access to GuC IRQ control registers > hence ensure device is RPM awake. > > v2: Add comment about need to synchronize flush work and log runtime > destroy > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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 | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c > index f87e9f5..ed239cb 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -656,8 +656,17 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv) > { > mutex_lock(&dev_priv->drm.struct_mutex); > /* GuC logging is currently the only user of Guc2Host interrupts */ > - if (i915_modparams.guc_log_level >= 0) > + if (i915_modparams.guc_log_level >= 0) { > + intel_runtime_pm_get(dev_priv); > intel_disable_guc_interrupts(&dev_priv->guc); > + intel_runtime_pm_put(dev_priv); Is it possible to trigger the assert from I915_WRITE today and if so which test case? > + } > + /* > + * TODO: Need to synchronize access to relay channel from flush work > + * and release here if interrupt stays enabled from hereon. > + * Possibly with GuC CT recv. interrupts will stay enabled until GEM > + * suspend/unload. > + */ I think we normally don't put such reminders in code. Regardless if it is going away in this patch series or not it looks equally pointless to me. > guc_log_runtime_destroy(&dev_priv->guc); Ha right, this is how the cleanup happens what I was wondering in the previous patch. So intel_guc_log_destroy is pretty pointless now since it effectively does only this. Or I missed something? > mutex_unlock(&dev_priv->drm.struct_mutex); > } > Regards, Tvrtko
On 10/19/2017 3:39 PM, Tvrtko Ursulin wrote: > > On 18/10/2017 07:46, Sagar Arun Kamble wrote: >> Disabling GuC interrupts involves access to GuC IRQ control registers >> hence ensure device is RPM awake. >> >> v2: Add comment about need to synchronize flush work and log runtime >> destroy >> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@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 | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c >> b/drivers/gpu/drm/i915/intel_guc_log.c >> index f87e9f5..ed239cb 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_log.c >> +++ b/drivers/gpu/drm/i915/intel_guc_log.c >> @@ -656,8 +656,17 @@ void i915_guc_log_unregister(struct >> drm_i915_private *dev_priv) >> { >> mutex_lock(&dev_priv->drm.struct_mutex); >> /* GuC logging is currently the only user of Guc2Host >> interrupts */ >> - if (i915_modparams.guc_log_level >= 0) >> + if (i915_modparams.guc_log_level >= 0) { >> + intel_runtime_pm_get(dev_priv); >> intel_disable_guc_interrupts(&dev_priv->guc); >> + intel_runtime_pm_put(dev_priv); > > Is it possible to trigger the assert from I915_WRITE today and if so > which test case? drv_module_reload/basic-reload > >> + } >> + /* >> + * TODO: Need to synchronize access to relay channel from flush >> work >> + * and release here if interrupt stays enabled from hereon. >> + * Possibly with GuC CT recv. interrupts will stay enabled until >> GEM >> + * suspend/unload. >> + */ > > I think we normally don't put such reminders in code. Regardless if it > is going away in this patch series or not it looks equally pointless > to me. Ok. Will remove this. Will need to be noted when we add CT buf recv support. > >> guc_log_runtime_destroy(&dev_priv->guc); > > Ha right, this is how the cleanup happens what I was wondering in the > previous patch. So intel_guc_log_destroy is pretty pointless now since > it effectively does only this. Or I missed something? Yes. Part of intel_guc_log_destroy to unpin and release log vma is needed during uc_fini_hw. Will remove this function then as it is only used during init failure and we can open code there. > >> mutex_unlock(&dev_priv->drm.struct_mutex); >> } >> > > Regards, > > Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index f87e9f5..ed239cb 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -656,8 +656,17 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv) { mutex_lock(&dev_priv->drm.struct_mutex); /* GuC logging is currently the only user of Guc2Host interrupts */ - if (i915_modparams.guc_log_level >= 0) + if (i915_modparams.guc_log_level >= 0) { + intel_runtime_pm_get(dev_priv); intel_disable_guc_interrupts(&dev_priv->guc); + intel_runtime_pm_put(dev_priv); + } + /* + * TODO: Need to synchronize access to relay channel from flush work + * and release here if interrupt stays enabled from hereon. + * Possibly with GuC CT recv. interrupts will stay enabled until GEM + * suspend/unload. + */ guc_log_runtime_destroy(&dev_priv->guc); mutex_unlock(&dev_priv->drm.struct_mutex); }
Disabling GuC interrupts involves access to GuC IRQ control registers hence ensure device is RPM awake. v2: Add comment about need to synchronize flush work and log runtime destroy Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@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 | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)