| Message ID | 20180227125230.13000-2-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 27 Feb 2018 13:52:17 +0100, Michał Winiarski <michal.winiarski@intel.com> wrote: > We have many functions responsible for allocating different parts of > runtime called from multiple places. Let's stick with keeping s/runtime/GuC log runtime ? > everything in guc_log_register instead. > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 6 +- > drivers/gpu/drm/i915/i915_gem.c | 4 +- > drivers/gpu/drm/i915/intel_guc_log.c | 148 > ++++++++++++++--------------------- > drivers/gpu/drm/i915/intel_guc_log.h | 6 +- > drivers/gpu/drm/i915/intel_uc.c | 39 ++++----- > drivers/gpu/drm/i915/intel_uc.h | 6 +- > 6 files changed, 91 insertions(+), 118 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c > index aaa861b51024..719b2be73292 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -636,7 +636,7 @@ static void i915_gem_fini(struct drm_i915_private > *dev_priv) > i915_gem_contexts_fini(dev_priv); > mutex_unlock(&dev_priv->drm.struct_mutex); > - intel_uc_fini_misc(dev_priv); > + intel_uc_fini_wq(dev_priv); > i915_gem_cleanup_userptr(dev_priv); > i915_gem_drain_freed_objects(dev_priv); > @@ -1237,7 +1237,7 @@ static void i915_driver_register(struct > drm_i915_private *dev_priv) > /* Reveal our presence to userspace */ > if (drm_dev_register(dev, 0) == 0) { > i915_debugfs_register(dev_priv); > - i915_guc_log_register(dev_priv); > + intel_uc_log_register(dev_priv); btw, maybe we should place this function little later after /* Depends on debugfs having been initialized */ > i915_setup_sysfs(dev_priv); > /* Depends on sysfs having been initialized */ > @@ -1297,7 +1297,7 @@ static void i915_driver_unregister(struct > drm_i915_private *dev_priv) > i915_pmu_unregister(dev_priv); > i915_teardown_sysfs(dev_priv); > - i915_guc_log_unregister(dev_priv); > + intel_uc_log_unregister(dev_priv); > drm_dev_unregister(&dev_priv->drm); > i915_gem_shrinker_unregister(dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_gem.c > b/drivers/gpu/drm/i915/i915_gem.c > index 14c855b1a3a4..4bf5f25b29e2 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -5279,7 +5279,7 @@ int i915_gem_init(struct drm_i915_private > *dev_priv) > if (ret) > return ret; > - ret = intel_uc_init_misc(dev_priv); > + ret = intel_uc_init_wq(dev_priv); > if (ret) > return ret; > @@ -5375,7 +5375,7 @@ int i915_gem_init(struct drm_i915_private > *dev_priv) > intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); > mutex_unlock(&dev_priv->drm.struct_mutex); > - intel_uc_fini_misc(dev_priv); > + intel_uc_fini_wq(dev_priv); > if (ret != -EIO) > i915_gem_cleanup_userptr(dev_priv); > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c > b/drivers/gpu/drm/i915/intel_guc_log.c > index 22a05320817b..f1cab43d334e 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -441,7 +441,7 @@ void intel_guc_log_init_early(struct intel_guc *guc) > INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work); > } > -int intel_guc_log_relay_create(struct intel_guc *guc) > +int guc_log_relay_create(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > struct rchan *guc_log_relay_chan; > @@ -494,7 +494,7 @@ int intel_guc_log_relay_create(struct intel_guc *guc) > return ret; > } > -void intel_guc_log_relay_destroy(struct intel_guc *guc) > +void guc_log_relay_destroy(struct intel_guc *guc) > { > mutex_lock(&guc->log.runtime.relay_lock); > @@ -512,49 +512,6 @@ void intel_guc_log_relay_destroy(struct intel_guc > *guc) > mutex_unlock(&guc->log.runtime.relay_lock); > } > -static int guc_log_late_setup(struct intel_guc *guc) > -{ > - struct drm_i915_private *dev_priv = guc_to_i915(guc); > - int ret; > - > - if (!guc_log_has_runtime(guc)) { > - /* > - * If log was disabled at boot time, then setup needed to handle > - * log buffer flush interrupts would not have been done yet, so > - * do that now. > - */ > - ret = intel_guc_log_relay_create(guc); > - if (ret) > - goto err; > - > - mutex_lock(&dev_priv->drm.struct_mutex); > - intel_runtime_pm_get(dev_priv); > - ret = guc_log_runtime_create(guc); > - intel_runtime_pm_put(dev_priv); > - mutex_unlock(&dev_priv->drm.struct_mutex); > - > - if (ret) > - goto err_relay; > - } > - > - ret = guc_log_relay_file_create(guc); > - if (ret) > - goto err_runtime; > - > - return 0; > - > -err_runtime: > - mutex_lock(&dev_priv->drm.struct_mutex); > - guc_log_runtime_destroy(guc); > - mutex_unlock(&dev_priv->drm.struct_mutex); > -err_relay: > - intel_guc_log_relay_destroy(guc); > -err: > - /* logging will remain off */ > - i915_modparams.guc_log_level = 0; > - return ret; > -} > - > static void guc_log_capture_logs(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > @@ -574,16 +531,6 @@ static void guc_flush_logs(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > - if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level) > - return; > - > - /* First disable the interrupts, will be renabled afterwards */ > - mutex_lock(&dev_priv->drm.struct_mutex); > - intel_runtime_pm_get(dev_priv); > - gen9_disable_guc_interrupts(dev_priv); > - intel_runtime_pm_put(dev_priv); > - mutex_unlock(&dev_priv->drm.struct_mutex); > - > /* > * Before initiating the forceful flush, wait for any pending/ongoing > * flush to complete otherwise forceful flush may not actually happen. > @@ -626,12 +573,6 @@ int intel_guc_log_create(struct intel_guc *guc) > guc->log.vma = vma; > - if (i915_modparams.guc_log_level) { > - ret = guc_log_runtime_create(guc); > - if (ret < 0) > - goto err_vma; > - } > - > /* each allocated unit is a page */ > flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL | > (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) | > @@ -643,8 +584,6 @@ int intel_guc_log_create(struct intel_guc *guc) > return 0; > -err_vma: > - i915_vma_unpin_and_release(&guc->log.vma); > err: > /* logging will be off */ > i915_modparams.guc_log_level = 0; > @@ -703,24 +642,14 @@ int intel_guc_log_control_set(struct intel_guc > *guc, u64 val) > mutex_unlock(&dev_priv->drm.struct_mutex); > if (GUC_LOG_IS_ENABLED(val) && !guc_log_has_runtime(guc)) { > - ret = guc_log_late_setup(guc); > - if (ret) > + ret = intel_guc_log_register(guc); > + if (ret) { > + /* logging will remain off */ > + i915_modparams.guc_log_level = 0; > goto out; > - > - /* GuC logging is currently the only user of Guc2Host interrupts */ > - mutex_lock(&dev_priv->drm.struct_mutex); > - intel_runtime_pm_get(dev_priv); > - gen9_enable_guc_interrupts(dev_priv); > - intel_runtime_pm_put(dev_priv); > - mutex_unlock(&dev_priv->drm.struct_mutex); > + } > } else if (!GUC_LOG_IS_ENABLED(val) && guc_log_has_runtime(guc)) { > - /* > - * Once logging is disabled, GuC won't generate logs & send an > - * interrupt. But there could be some data in the log buffer > - * which is yet to be captured. So request GuC to update the log > - * buffer state and then collect the left over logs. > - */ > - guc_flush_logs(guc); > + intel_guc_log_unregister(guc); > } > return 0; > @@ -731,20 +660,63 @@ int intel_guc_log_control_set(struct intel_guc > *guc, u64 val) > return ret; > } > -void i915_guc_log_register(struct drm_i915_private *dev_priv) > +int intel_guc_log_register(struct intel_guc *guc) > { > - if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level) > - return; > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > + int ret; > + > + GEM_BUG_ON(guc_log_has_runtime(guc)); > + > + /* > + * If log was disabled at boot time, then setup needed to handle > + * log buffer flush interrupts would not have been done yet, so > + * do that now. > + */ > + ret = guc_log_relay_create(guc); > + if (ret) > + goto err; > + > + mutex_lock(&dev_priv->drm.struct_mutex); > + ret = guc_log_runtime_create(guc); > + mutex_unlock(&dev_priv->drm.struct_mutex); > + > + if (ret) > + goto err_relay; > + > + ret = guc_log_relay_file_create(guc); > + if (ret) > + goto err_runtime; > - guc_log_late_setup(&dev_priv->guc); > + /* GuC logging is currently the only user of Guc2Host interrupts */ > + mutex_lock(&dev_priv->drm.struct_mutex); > + intel_runtime_pm_get(dev_priv); > + gen9_enable_guc_interrupts(dev_priv); > + intel_runtime_pm_put(dev_priv); > + mutex_unlock(&dev_priv->drm.struct_mutex); > + > + return 0; > + > +err_runtime: > + mutex_lock(&dev_priv->drm.struct_mutex); > + guc_log_runtime_destroy(guc); > + mutex_unlock(&dev_priv->drm.struct_mutex); > +err_relay: > + guc_log_relay_destroy(guc); > +err: > + return ret; > } > -void i915_guc_log_unregister(struct drm_i915_private *dev_priv) > +void intel_guc_log_unregister(struct intel_guc *guc) > { > - struct intel_guc *guc = &dev_priv->guc; > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > - if (!USES_GUC_SUBMISSION(dev_priv)) > - return; > + /* > + * Once logging is disabled, GuC won't generate logs & send an > + * interrupt. But there could be some data in the log buffer > + * which is yet to be captured. So request GuC to update the log > + * buffer state and then collect the left over logs. > + */ > + guc_flush_logs(guc); > mutex_lock(&dev_priv->drm.struct_mutex); > /* GuC logging is currently the only user of Guc2Host interrupts */ > @@ -755,5 +727,5 @@ void i915_guc_log_unregister(struct drm_i915_private > *dev_priv) > guc_log_runtime_destroy(guc); > mutex_unlock(&dev_priv->drm.struct_mutex); > - intel_guc_log_relay_destroy(guc); > + guc_log_relay_destroy(guc); > } > diff --git a/drivers/gpu/drm/i915/intel_guc_log.h > b/drivers/gpu/drm/i915/intel_guc_log.h > index 141ce9ca22ce..09dd2ef1933d 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.h > +++ b/drivers/gpu/drm/i915/intel_guc_log.h > @@ -62,11 +62,9 @@ struct intel_guc_log { > int intel_guc_log_create(struct intel_guc *guc); > void intel_guc_log_destroy(struct intel_guc *guc); > void intel_guc_log_init_early(struct intel_guc *guc); > -int intel_guc_log_relay_create(struct intel_guc *guc); > -void intel_guc_log_relay_destroy(struct intel_guc *guc); > int intel_guc_log_control_get(struct intel_guc *guc); > int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val); > -void i915_guc_log_register(struct drm_i915_private *dev_priv); > -void i915_guc_log_unregister(struct drm_i915_private *dev_priv); > +int intel_guc_log_register(struct intel_guc *guc); > +void intel_guc_log_unregister(struct intel_guc *guc); > #endif > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index 9f1bac6398fb..3f41ef525e26 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -219,6 +219,22 @@ static void guc_free_load_err_log(struct intel_guc > *guc) > i915_gem_object_put(guc->load_err_log); > } > +int intel_uc_log_register(struct drm_i915_private *dev_priv) What if we'll add more entries that need to be registered? s/intel_uc_log_register/intel_uc_register ? > +{ > + if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level) > + return 0; > + > + return intel_guc_log_register(&dev_priv->guc); > +} > + > +void intel_uc_log_unregister(struct drm_i915_private *dev_priv) > +{ > + if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level) > + return; > + > + intel_guc_log_unregister(&dev_priv->guc); > +} > + > static int guc_enable_communication(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > @@ -240,7 +256,7 @@ static void guc_disable_communication(struct > intel_guc *guc) > guc->send = intel_guc_send_nop; > } > -int intel_uc_init_misc(struct drm_i915_private *dev_priv) > +int intel_uc_init_wq(struct drm_i915_private *dev_priv) I would leave old name here, as wq will expose our internals ;) > { > struct intel_guc *guc = &dev_priv->guc; > int ret; > @@ -249,26 +265,13 @@ int intel_uc_init_misc(struct drm_i915_private > *dev_priv) > return 0; > ret = intel_guc_init_wq(guc); > - if (ret) { > - DRM_ERROR("Couldn't allocate workqueues for GuC\n"); > - goto err; > - } > - > - ret = intel_guc_log_relay_create(guc); > - if (ret) { > - DRM_ERROR("Couldn't allocate relay for GuC log\n"); > - goto err_relay; > - } > + if (ret) > + return ret; > return 0; > - > -err_relay: > - intel_guc_fini_wq(guc); > -err: > - return ret; > } > -void intel_uc_fini_misc(struct drm_i915_private *dev_priv) > +void intel_uc_fini_wq(struct drm_i915_private *dev_priv) > { > struct intel_guc *guc = &dev_priv->guc; > @@ -276,8 +279,6 @@ void intel_uc_fini_misc(struct drm_i915_private > *dev_priv) > return; > intel_guc_fini_wq(guc); > - > - intel_guc_log_relay_destroy(guc); > } > int intel_uc_init(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/intel_uc.h > b/drivers/gpu/drm/i915/intel_uc.h > index f2984e01e257..9bc22103396d 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -31,10 +31,12 @@ > void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); > void intel_uc_init_early(struct drm_i915_private *dev_priv); > void intel_uc_init_mmio(struct drm_i915_private *dev_priv); > +int intel_uc_log_register(struct drm_i915_private *dev_priv); > +void intel_uc_log_unregister(struct drm_i915_private *dev_priv); > 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_misc(struct drm_i915_private *dev_priv); > -void intel_uc_fini_misc(struct drm_i915_private *dev_priv); > +int intel_uc_init_wq(struct drm_i915_private *dev_priv); > +void intel_uc_fini_wq(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); > int intel_uc_init(struct drm_i915_private *dev_priv);
Overall change looks good. Could you please clarify on below: intel_uc_log_register|unregister are removed in patch later in the series. Should we just stay with inner functions then to minimize changes? Thanks Sagar On 2/27/2018 6:22 PM, Michał Winiarski wrote: > We have many functions responsible for allocating different parts of > runtime called from multiple places. Let's stick with keeping > everything in guc_log_register instead. > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 6 +- > drivers/gpu/drm/i915/i915_gem.c | 4 +- > drivers/gpu/drm/i915/intel_guc_log.c | 148 ++++++++++++++--------------------- > drivers/gpu/drm/i915/intel_guc_log.h | 6 +- > drivers/gpu/drm/i915/intel_uc.c | 39 ++++----- > drivers/gpu/drm/i915/intel_uc.h | 6 +- > 6 files changed, 91 insertions(+), 118 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index aaa861b51024..719b2be73292 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -636,7 +636,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv) > i915_gem_contexts_fini(dev_priv); > mutex_unlock(&dev_priv->drm.struct_mutex); > > - intel_uc_fini_misc(dev_priv); > + intel_uc_fini_wq(dev_priv); > i915_gem_cleanup_userptr(dev_priv); > > i915_gem_drain_freed_objects(dev_priv); > @@ -1237,7 +1237,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) > /* Reveal our presence to userspace */ > if (drm_dev_register(dev, 0) == 0) { > i915_debugfs_register(dev_priv); > - i915_guc_log_register(dev_priv); > + intel_uc_log_register(dev_priv); > i915_setup_sysfs(dev_priv); > > /* Depends on sysfs having been initialized */ > @@ -1297,7 +1297,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) > i915_pmu_unregister(dev_priv); > > i915_teardown_sysfs(dev_priv); > - i915_guc_log_unregister(dev_priv); > + intel_uc_log_unregister(dev_priv); > drm_dev_unregister(&dev_priv->drm); > > i915_gem_shrinker_unregister(dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 14c855b1a3a4..4bf5f25b29e2 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -5279,7 +5279,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv) > if (ret) > return ret; > > - ret = intel_uc_init_misc(dev_priv); > + ret = intel_uc_init_wq(dev_priv); > if (ret) > return ret; > > @@ -5375,7 +5375,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv) > intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); > mutex_unlock(&dev_priv->drm.struct_mutex); > > - intel_uc_fini_misc(dev_priv); > + intel_uc_fini_wq(dev_priv); > > if (ret != -EIO) > i915_gem_cleanup_userptr(dev_priv); > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c > index 22a05320817b..f1cab43d334e 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -441,7 +441,7 @@ void intel_guc_log_init_early(struct intel_guc *guc) > INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work); > } > > -int intel_guc_log_relay_create(struct intel_guc *guc) > +int guc_log_relay_create(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > struct rchan *guc_log_relay_chan; > @@ -494,7 +494,7 @@ int intel_guc_log_relay_create(struct intel_guc *guc) > return ret; > } > > -void intel_guc_log_relay_destroy(struct intel_guc *guc) > +void guc_log_relay_destroy(struct intel_guc *guc) > { > mutex_lock(&guc->log.runtime.relay_lock); > > @@ -512,49 +512,6 @@ void intel_guc_log_relay_destroy(struct intel_guc *guc) > mutex_unlock(&guc->log.runtime.relay_lock); > } > > -static int guc_log_late_setup(struct intel_guc *guc) > -{ > - struct drm_i915_private *dev_priv = guc_to_i915(guc); > - int ret; > - > - if (!guc_log_has_runtime(guc)) { > - /* > - * If log was disabled at boot time, then setup needed to handle > - * log buffer flush interrupts would not have been done yet, so > - * do that now. > - */ > - ret = intel_guc_log_relay_create(guc); > - if (ret) > - goto err; > - > - mutex_lock(&dev_priv->drm.struct_mutex); > - intel_runtime_pm_get(dev_priv); > - ret = guc_log_runtime_create(guc); > - intel_runtime_pm_put(dev_priv); > - mutex_unlock(&dev_priv->drm.struct_mutex); > - > - if (ret) > - goto err_relay; > - } > - > - ret = guc_log_relay_file_create(guc); > - if (ret) > - goto err_runtime; > - > - return 0; > - > -err_runtime: > - mutex_lock(&dev_priv->drm.struct_mutex); > - guc_log_runtime_destroy(guc); > - mutex_unlock(&dev_priv->drm.struct_mutex); > -err_relay: > - intel_guc_log_relay_destroy(guc); > -err: > - /* logging will remain off */ > - i915_modparams.guc_log_level = 0; > - return ret; > -} > - > static void guc_log_capture_logs(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > @@ -574,16 +531,6 @@ static void guc_flush_logs(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > > - if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level) > - return; > - > - /* First disable the interrupts, will be renabled afterwards */ > - mutex_lock(&dev_priv->drm.struct_mutex); > - intel_runtime_pm_get(dev_priv); > - gen9_disable_guc_interrupts(dev_priv); > - intel_runtime_pm_put(dev_priv); > - mutex_unlock(&dev_priv->drm.struct_mutex); > - > /* > * Before initiating the forceful flush, wait for any pending/ongoing > * flush to complete otherwise forceful flush may not actually happen. > @@ -626,12 +573,6 @@ int intel_guc_log_create(struct intel_guc *guc) > > guc->log.vma = vma; > > - if (i915_modparams.guc_log_level) { > - ret = guc_log_runtime_create(guc); > - if (ret < 0) > - goto err_vma; > - } > - > /* each allocated unit is a page */ > flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL | > (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) | > @@ -643,8 +584,6 @@ int intel_guc_log_create(struct intel_guc *guc) > > return 0; > > -err_vma: > - i915_vma_unpin_and_release(&guc->log.vma); > err: > /* logging will be off */ > i915_modparams.guc_log_level = 0; > @@ -703,24 +642,14 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) > mutex_unlock(&dev_priv->drm.struct_mutex); > > if (GUC_LOG_IS_ENABLED(val) && !guc_log_has_runtime(guc)) { > - ret = guc_log_late_setup(guc); > - if (ret) > + ret = intel_guc_log_register(guc); > + if (ret) { > + /* logging will remain off */ > + i915_modparams.guc_log_level = 0; > goto out; > - > - /* GuC logging is currently the only user of Guc2Host interrupts */ > - mutex_lock(&dev_priv->drm.struct_mutex); > - intel_runtime_pm_get(dev_priv); > - gen9_enable_guc_interrupts(dev_priv); > - intel_runtime_pm_put(dev_priv); > - mutex_unlock(&dev_priv->drm.struct_mutex); > + } > } else if (!GUC_LOG_IS_ENABLED(val) && guc_log_has_runtime(guc)) { > - /* > - * Once logging is disabled, GuC won't generate logs & send an > - * interrupt. But there could be some data in the log buffer > - * which is yet to be captured. So request GuC to update the log > - * buffer state and then collect the left over logs. > - */ > - guc_flush_logs(guc); > + intel_guc_log_unregister(guc); > } > > return 0; > @@ -731,20 +660,63 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) > return ret; > } > > -void i915_guc_log_register(struct drm_i915_private *dev_priv) > +int intel_guc_log_register(struct intel_guc *guc) > { > - if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level) > - return; > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > + int ret; > + > + GEM_BUG_ON(guc_log_has_runtime(guc)); > + > + /* > + * If log was disabled at boot time, then setup needed to handle > + * log buffer flush interrupts would not have been done yet, so > + * do that now. > + */ > + ret = guc_log_relay_create(guc); > + if (ret) > + goto err; > + > + mutex_lock(&dev_priv->drm.struct_mutex); > + ret = guc_log_runtime_create(guc); > + mutex_unlock(&dev_priv->drm.struct_mutex); > + > + if (ret) > + goto err_relay; > + > + ret = guc_log_relay_file_create(guc); > + if (ret) > + goto err_runtime; > > - guc_log_late_setup(&dev_priv->guc); > + /* GuC logging is currently the only user of Guc2Host interrupts */ > + mutex_lock(&dev_priv->drm.struct_mutex); > + intel_runtime_pm_get(dev_priv); > + gen9_enable_guc_interrupts(dev_priv); > + intel_runtime_pm_put(dev_priv); > + mutex_unlock(&dev_priv->drm.struct_mutex); > + > + return 0; > + > +err_runtime: > + mutex_lock(&dev_priv->drm.struct_mutex); > + guc_log_runtime_destroy(guc); > + mutex_unlock(&dev_priv->drm.struct_mutex); > +err_relay: > + guc_log_relay_destroy(guc); > +err: > + return ret; > } > > -void i915_guc_log_unregister(struct drm_i915_private *dev_priv) > +void intel_guc_log_unregister(struct intel_guc *guc) > { > - struct intel_guc *guc = &dev_priv->guc; > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > > - if (!USES_GUC_SUBMISSION(dev_priv)) > - return; > + /* > + * Once logging is disabled, GuC won't generate logs & send an > + * interrupt. But there could be some data in the log buffer > + * which is yet to be captured. So request GuC to update the log > + * buffer state and then collect the left over logs. > + */ > + guc_flush_logs(guc); > > mutex_lock(&dev_priv->drm.struct_mutex); > /* GuC logging is currently the only user of Guc2Host interrupts */ > @@ -755,5 +727,5 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv) > guc_log_runtime_destroy(guc); > mutex_unlock(&dev_priv->drm.struct_mutex); > > - intel_guc_log_relay_destroy(guc); > + guc_log_relay_destroy(guc); > } > diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h > index 141ce9ca22ce..09dd2ef1933d 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.h > +++ b/drivers/gpu/drm/i915/intel_guc_log.h > @@ -62,11 +62,9 @@ struct intel_guc_log { > int intel_guc_log_create(struct intel_guc *guc); > void intel_guc_log_destroy(struct intel_guc *guc); > void intel_guc_log_init_early(struct intel_guc *guc); > -int intel_guc_log_relay_create(struct intel_guc *guc); > -void intel_guc_log_relay_destroy(struct intel_guc *guc); > int intel_guc_log_control_get(struct intel_guc *guc); > int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val); > -void i915_guc_log_register(struct drm_i915_private *dev_priv); > -void i915_guc_log_unregister(struct drm_i915_private *dev_priv); > +int intel_guc_log_register(struct intel_guc *guc); > +void intel_guc_log_unregister(struct intel_guc *guc); > > #endif > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 9f1bac6398fb..3f41ef525e26 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -219,6 +219,22 @@ static void guc_free_load_err_log(struct intel_guc *guc) > i915_gem_object_put(guc->load_err_log); > } > > +int intel_uc_log_register(struct drm_i915_private *dev_priv) > +{ > + if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level) > + return 0; > + > + return intel_guc_log_register(&dev_priv->guc); > +} > + > +void intel_uc_log_unregister(struct drm_i915_private *dev_priv) > +{ > + if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level) > + return; > + > + intel_guc_log_unregister(&dev_priv->guc); > +} > + > static int guc_enable_communication(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > @@ -240,7 +256,7 @@ static void guc_disable_communication(struct intel_guc *guc) > guc->send = intel_guc_send_nop; > } > > -int intel_uc_init_misc(struct drm_i915_private *dev_priv) > +int intel_uc_init_wq(struct drm_i915_private *dev_priv) > { > struct intel_guc *guc = &dev_priv->guc; > int ret; > @@ -249,26 +265,13 @@ int intel_uc_init_misc(struct drm_i915_private *dev_priv) > return 0; > > ret = intel_guc_init_wq(guc); > - if (ret) { > - DRM_ERROR("Couldn't allocate workqueues for GuC\n"); > - goto err; > - } > - > - ret = intel_guc_log_relay_create(guc); > - if (ret) { > - DRM_ERROR("Couldn't allocate relay for GuC log\n"); > - goto err_relay; > - } > + if (ret) > + return ret; > > return 0; > - > -err_relay: > - intel_guc_fini_wq(guc); > -err: > - return ret; > } > > -void intel_uc_fini_misc(struct drm_i915_private *dev_priv) > +void intel_uc_fini_wq(struct drm_i915_private *dev_priv) > { > struct intel_guc *guc = &dev_priv->guc; > > @@ -276,8 +279,6 @@ void intel_uc_fini_misc(struct drm_i915_private *dev_priv) > return; > > intel_guc_fini_wq(guc); > - > - intel_guc_log_relay_destroy(guc); > } > > int intel_uc_init(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index f2984e01e257..9bc22103396d 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -31,10 +31,12 @@ > void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); > void intel_uc_init_early(struct drm_i915_private *dev_priv); > void intel_uc_init_mmio(struct drm_i915_private *dev_priv); > +int intel_uc_log_register(struct drm_i915_private *dev_priv); > +void intel_uc_log_unregister(struct drm_i915_private *dev_priv); > 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_misc(struct drm_i915_private *dev_priv); > -void intel_uc_fini_misc(struct drm_i915_private *dev_priv); > +int intel_uc_init_wq(struct drm_i915_private *dev_priv); > +void intel_uc_fini_wq(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); > int intel_uc_init(struct drm_i915_private *dev_priv);
On Mon, Mar 05, 2018 at 12:39:58PM +0530, Sagar Arun Kamble wrote: > Overall change looks good. Could you please clarify on below: > > intel_uc_log_register|unregister are removed in patch later in the series. > Should we just stay with inner functions then to minimize changes? I've done this to move (USES_GUC/guc_log_level) checks to the callers. Otherwise it would need to stay in intel_guc_log_register, which would cause us to do more changes in intel_guc_log_control_set later in the series (when we're actually doing the decoupling). -Michał > > > Thanks > Sagar > > On 2/27/2018 6:22 PM, Michał Winiarski wrote: > > We have many functions responsible for allocating different parts of > > runtime called from multiple places. Let's stick with keeping > > everything in guc_log_register instead. > > > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 6 +- > > drivers/gpu/drm/i915/i915_gem.c | 4 +- > > drivers/gpu/drm/i915/intel_guc_log.c | 148 ++++++++++++++--------------------- > > drivers/gpu/drm/i915/intel_guc_log.h | 6 +- > > drivers/gpu/drm/i915/intel_uc.c | 39 ++++----- > > drivers/gpu/drm/i915/intel_uc.h | 6 +- > > 6 files changed, 91 insertions(+), 118 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index aaa861b51024..719b2be73292 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -636,7 +636,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv) > > i915_gem_contexts_fini(dev_priv); > > mutex_unlock(&dev_priv->drm.struct_mutex); > > - intel_uc_fini_misc(dev_priv); > > + intel_uc_fini_wq(dev_priv); > > i915_gem_cleanup_userptr(dev_priv); > > i915_gem_drain_freed_objects(dev_priv); > > @@ -1237,7 +1237,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) > > /* Reveal our presence to userspace */ > > if (drm_dev_register(dev, 0) == 0) { > > i915_debugfs_register(dev_priv); > > - i915_guc_log_register(dev_priv); > > + intel_uc_log_register(dev_priv); > > i915_setup_sysfs(dev_priv); > > /* Depends on sysfs having been initialized */ > > @@ -1297,7 +1297,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) > > i915_pmu_unregister(dev_priv); > > i915_teardown_sysfs(dev_priv); > > - i915_guc_log_unregister(dev_priv); > > + intel_uc_log_unregister(dev_priv); > > drm_dev_unregister(&dev_priv->drm); > > i915_gem_shrinker_unregister(dev_priv); > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 14c855b1a3a4..4bf5f25b29e2 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -5279,7 +5279,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv) > > if (ret) > > return ret; > > - ret = intel_uc_init_misc(dev_priv); > > + ret = intel_uc_init_wq(dev_priv); > > if (ret) > > return ret; > > @@ -5375,7 +5375,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv) > > intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); > > mutex_unlock(&dev_priv->drm.struct_mutex); > > - intel_uc_fini_misc(dev_priv); > > + intel_uc_fini_wq(dev_priv); > > if (ret != -EIO) > > i915_gem_cleanup_userptr(dev_priv); > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c > > index 22a05320817b..f1cab43d334e 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_log.c > > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > > @@ -441,7 +441,7 @@ void intel_guc_log_init_early(struct intel_guc *guc) > > INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work); > > } > > -int intel_guc_log_relay_create(struct intel_guc *guc) > > +int guc_log_relay_create(struct intel_guc *guc) > > { > > struct drm_i915_private *dev_priv = guc_to_i915(guc); > > struct rchan *guc_log_relay_chan; > > @@ -494,7 +494,7 @@ int intel_guc_log_relay_create(struct intel_guc *guc) > > return ret; > > } > > -void intel_guc_log_relay_destroy(struct intel_guc *guc) > > +void guc_log_relay_destroy(struct intel_guc *guc) > > { > > mutex_lock(&guc->log.runtime.relay_lock); > > @@ -512,49 +512,6 @@ void intel_guc_log_relay_destroy(struct intel_guc *guc) > > mutex_unlock(&guc->log.runtime.relay_lock); > > } > > -static int guc_log_late_setup(struct intel_guc *guc) > > -{ > > - struct drm_i915_private *dev_priv = guc_to_i915(guc); > > - int ret; > > - > > - if (!guc_log_has_runtime(guc)) { > > - /* > > - * If log was disabled at boot time, then setup needed to handle > > - * log buffer flush interrupts would not have been done yet, so > > - * do that now. > > - */ > > - ret = intel_guc_log_relay_create(guc); > > - if (ret) > > - goto err; > > - > > - mutex_lock(&dev_priv->drm.struct_mutex); > > - intel_runtime_pm_get(dev_priv); > > - ret = guc_log_runtime_create(guc); > > - intel_runtime_pm_put(dev_priv); > > - mutex_unlock(&dev_priv->drm.struct_mutex); > > - > > - if (ret) > > - goto err_relay; > > - } > > - > > - ret = guc_log_relay_file_create(guc); > > - if (ret) > > - goto err_runtime; > > - > > - return 0; > > - > > -err_runtime: > > - mutex_lock(&dev_priv->drm.struct_mutex); > > - guc_log_runtime_destroy(guc); > > - mutex_unlock(&dev_priv->drm.struct_mutex); > > -err_relay: > > - intel_guc_log_relay_destroy(guc); > > -err: > > - /* logging will remain off */ > > - i915_modparams.guc_log_level = 0; > > - return ret; > > -} > > - > > static void guc_log_capture_logs(struct intel_guc *guc) > > { > > struct drm_i915_private *dev_priv = guc_to_i915(guc); > > @@ -574,16 +531,6 @@ static void guc_flush_logs(struct intel_guc *guc) > > { > > struct drm_i915_private *dev_priv = guc_to_i915(guc); > > - if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level) > > - return; > > - > > - /* First disable the interrupts, will be renabled afterwards */ > > - mutex_lock(&dev_priv->drm.struct_mutex); > > - intel_runtime_pm_get(dev_priv); > > - gen9_disable_guc_interrupts(dev_priv); > > - intel_runtime_pm_put(dev_priv); > > - mutex_unlock(&dev_priv->drm.struct_mutex); > > - > > /* > > * Before initiating the forceful flush, wait for any pending/ongoing > > * flush to complete otherwise forceful flush may not actually happen. > > @@ -626,12 +573,6 @@ int intel_guc_log_create(struct intel_guc *guc) > > guc->log.vma = vma; > > - if (i915_modparams.guc_log_level) { > > - ret = guc_log_runtime_create(guc); > > - if (ret < 0) > > - goto err_vma; > > - } > > - > > /* each allocated unit is a page */ > > flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL | > > (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) | > > @@ -643,8 +584,6 @@ int intel_guc_log_create(struct intel_guc *guc) > > return 0; > > -err_vma: > > - i915_vma_unpin_and_release(&guc->log.vma); > > err: > > /* logging will be off */ > > i915_modparams.guc_log_level = 0; > > @@ -703,24 +642,14 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) > > mutex_unlock(&dev_priv->drm.struct_mutex); > > if (GUC_LOG_IS_ENABLED(val) && !guc_log_has_runtime(guc)) { > > - ret = guc_log_late_setup(guc); > > - if (ret) > > + ret = intel_guc_log_register(guc); > > + if (ret) { > > + /* logging will remain off */ > > + i915_modparams.guc_log_level = 0; > > goto out; > > - > > - /* GuC logging is currently the only user of Guc2Host interrupts */ > > - mutex_lock(&dev_priv->drm.struct_mutex); > > - intel_runtime_pm_get(dev_priv); > > - gen9_enable_guc_interrupts(dev_priv); > > - intel_runtime_pm_put(dev_priv); > > - mutex_unlock(&dev_priv->drm.struct_mutex); > > + } > > } else if (!GUC_LOG_IS_ENABLED(val) && guc_log_has_runtime(guc)) { > > - /* > > - * Once logging is disabled, GuC won't generate logs & send an > > - * interrupt. But there could be some data in the log buffer > > - * which is yet to be captured. So request GuC to update the log > > - * buffer state and then collect the left over logs. > > - */ > > - guc_flush_logs(guc); > > + intel_guc_log_unregister(guc); > > } > > return 0; > > @@ -731,20 +660,63 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) > > return ret; > > } > > -void i915_guc_log_register(struct drm_i915_private *dev_priv) > > +int intel_guc_log_register(struct intel_guc *guc) > > { > > - if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level) > > - return; > > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > > + int ret; > > + > > + GEM_BUG_ON(guc_log_has_runtime(guc)); > > + > > + /* > > + * If log was disabled at boot time, then setup needed to handle > > + * log buffer flush interrupts would not have been done yet, so > > + * do that now. > > + */ > > + ret = guc_log_relay_create(guc); > > + if (ret) > > + goto err; > > + > > + mutex_lock(&dev_priv->drm.struct_mutex); > > + ret = guc_log_runtime_create(guc); > > + mutex_unlock(&dev_priv->drm.struct_mutex); > > + > > + if (ret) > > + goto err_relay; > > + > > + ret = guc_log_relay_file_create(guc); > > + if (ret) > > + goto err_runtime; > > - guc_log_late_setup(&dev_priv->guc); > > + /* GuC logging is currently the only user of Guc2Host interrupts */ > > + mutex_lock(&dev_priv->drm.struct_mutex); > > + intel_runtime_pm_get(dev_priv); > > + gen9_enable_guc_interrupts(dev_priv); > > + intel_runtime_pm_put(dev_priv); > > + mutex_unlock(&dev_priv->drm.struct_mutex); > > + > > + return 0; > > + > > +err_runtime: > > + mutex_lock(&dev_priv->drm.struct_mutex); > > + guc_log_runtime_destroy(guc); > > + mutex_unlock(&dev_priv->drm.struct_mutex); > > +err_relay: > > + guc_log_relay_destroy(guc); > > +err: > > + return ret; > > } > > -void i915_guc_log_unregister(struct drm_i915_private *dev_priv) > > +void intel_guc_log_unregister(struct intel_guc *guc) > > { > > - struct intel_guc *guc = &dev_priv->guc; > > + struct drm_i915_private *dev_priv = guc_to_i915(guc); > > - if (!USES_GUC_SUBMISSION(dev_priv)) > > - return; > > + /* > > + * Once logging is disabled, GuC won't generate logs & send an > > + * interrupt. But there could be some data in the log buffer > > + * which is yet to be captured. So request GuC to update the log > > + * buffer state and then collect the left over logs. > > + */ > > + guc_flush_logs(guc); > > mutex_lock(&dev_priv->drm.struct_mutex); > > /* GuC logging is currently the only user of Guc2Host interrupts */ > > @@ -755,5 +727,5 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv) > > guc_log_runtime_destroy(guc); > > mutex_unlock(&dev_priv->drm.struct_mutex); > > - intel_guc_log_relay_destroy(guc); > > + guc_log_relay_destroy(guc); > > } > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h > > index 141ce9ca22ce..09dd2ef1933d 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_log.h > > +++ b/drivers/gpu/drm/i915/intel_guc_log.h > > @@ -62,11 +62,9 @@ struct intel_guc_log { > > int intel_guc_log_create(struct intel_guc *guc); > > void intel_guc_log_destroy(struct intel_guc *guc); > > void intel_guc_log_init_early(struct intel_guc *guc); > > -int intel_guc_log_relay_create(struct intel_guc *guc); > > -void intel_guc_log_relay_destroy(struct intel_guc *guc); > > int intel_guc_log_control_get(struct intel_guc *guc); > > int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val); > > -void i915_guc_log_register(struct drm_i915_private *dev_priv); > > -void i915_guc_log_unregister(struct drm_i915_private *dev_priv); > > +int intel_guc_log_register(struct intel_guc *guc); > > +void intel_guc_log_unregister(struct intel_guc *guc); > > #endif > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > > index 9f1bac6398fb..3f41ef525e26 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.c > > +++ b/drivers/gpu/drm/i915/intel_uc.c > > @@ -219,6 +219,22 @@ static void guc_free_load_err_log(struct intel_guc *guc) > > i915_gem_object_put(guc->load_err_log); > > } > > +int intel_uc_log_register(struct drm_i915_private *dev_priv) > > +{ > > + if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level) > > + return 0; > > + > > + return intel_guc_log_register(&dev_priv->guc); > > +} > > + > > +void intel_uc_log_unregister(struct drm_i915_private *dev_priv) > > +{ > > + if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level) > > + return; > > + > > + intel_guc_log_unregister(&dev_priv->guc); > > +} > > + > > static int guc_enable_communication(struct intel_guc *guc) > > { > > struct drm_i915_private *dev_priv = guc_to_i915(guc); > > @@ -240,7 +256,7 @@ static void guc_disable_communication(struct intel_guc *guc) > > guc->send = intel_guc_send_nop; > > } > > -int intel_uc_init_misc(struct drm_i915_private *dev_priv) > > +int intel_uc_init_wq(struct drm_i915_private *dev_priv) > > { > > struct intel_guc *guc = &dev_priv->guc; > > int ret; > > @@ -249,26 +265,13 @@ int intel_uc_init_misc(struct drm_i915_private *dev_priv) > > return 0; > > ret = intel_guc_init_wq(guc); > > - if (ret) { > > - DRM_ERROR("Couldn't allocate workqueues for GuC\n"); > > - goto err; > > - } > > - > > - ret = intel_guc_log_relay_create(guc); > > - if (ret) { > > - DRM_ERROR("Couldn't allocate relay for GuC log\n"); > > - goto err_relay; > > - } > > + if (ret) > > + return ret; > > return 0; > > - > > -err_relay: > > - intel_guc_fini_wq(guc); > > -err: > > - return ret; > > } > > -void intel_uc_fini_misc(struct drm_i915_private *dev_priv) > > +void intel_uc_fini_wq(struct drm_i915_private *dev_priv) > > { > > struct intel_guc *guc = &dev_priv->guc; > > @@ -276,8 +279,6 @@ void intel_uc_fini_misc(struct drm_i915_private *dev_priv) > > return; > > intel_guc_fini_wq(guc); > > - > > - intel_guc_log_relay_destroy(guc); > > } > > int intel_uc_init(struct drm_i915_private *dev_priv) > > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > > index f2984e01e257..9bc22103396d 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.h > > +++ b/drivers/gpu/drm/i915/intel_uc.h > > @@ -31,10 +31,12 @@ > > void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); > > void intel_uc_init_early(struct drm_i915_private *dev_priv); > > void intel_uc_init_mmio(struct drm_i915_private *dev_priv); > > +int intel_uc_log_register(struct drm_i915_private *dev_priv); > > +void intel_uc_log_unregister(struct drm_i915_private *dev_priv); > > 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_misc(struct drm_i915_private *dev_priv); > > -void intel_uc_fini_misc(struct drm_i915_private *dev_priv); > > +int intel_uc_init_wq(struct drm_i915_private *dev_priv); > > +void intel_uc_fini_wq(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); > > int intel_uc_init(struct drm_i915_private *dev_priv); > > -- > Thanks, > Sagar >
On 3/5/2018 7:08 PM, Michał Winiarski wrote: > On Mon, Mar 05, 2018 at 12:39:58PM +0530, Sagar Arun Kamble wrote: >> Overall change looks good. Could you please clarify on below: >> >> intel_uc_log_register|unregister are removed in patch later in the series. >> Should we just stay with inner functions then to minimize changes? > I've done this to move (USES_GUC/guc_log_level) checks to the callers. Otherwise > it would need to stay in intel_guc_log_register, which would cause us to do more > changes in intel_guc_log_control_set later in the series (when we're actually > doing the decoupling). But AFAIU change in that patch (8/15) will be decoupling guc_log_register from USES_GUC/guc_log_level which would be fine I guess. Your call :) > -Michał > >> >> Thanks >> Sagar >> >> On 2/27/2018 6:22 PM, Michał Winiarski wrote: >>> We have many functions responsible for allocating different parts of >>> runtime called from multiple places. Let's stick with keeping >>> everything in guc_log_register instead. >>> >>> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_drv.c | 6 +- >>> drivers/gpu/drm/i915/i915_gem.c | 4 +- >>> drivers/gpu/drm/i915/intel_guc_log.c | 148 ++++++++++++++--------------------- >>> drivers/gpu/drm/i915/intel_guc_log.h | 6 +- >>> drivers/gpu/drm/i915/intel_uc.c | 39 ++++----- >>> drivers/gpu/drm/i915/intel_uc.h | 6 +- >>> 6 files changed, 91 insertions(+), 118 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >>> index aaa861b51024..719b2be73292 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.c >>> +++ b/drivers/gpu/drm/i915/i915_drv.c >>> @@ -636,7 +636,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv) >>> i915_gem_contexts_fini(dev_priv); >>> mutex_unlock(&dev_priv->drm.struct_mutex); >>> - intel_uc_fini_misc(dev_priv); >>> + intel_uc_fini_wq(dev_priv); >>> i915_gem_cleanup_userptr(dev_priv); >>> i915_gem_drain_freed_objects(dev_priv); >>> @@ -1237,7 +1237,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) >>> /* Reveal our presence to userspace */ >>> if (drm_dev_register(dev, 0) == 0) { >>> i915_debugfs_register(dev_priv); >>> - i915_guc_log_register(dev_priv); >>> + intel_uc_log_register(dev_priv); >>> i915_setup_sysfs(dev_priv); >>> /* Depends on sysfs having been initialized */ >>> @@ -1297,7 +1297,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) >>> i915_pmu_unregister(dev_priv); >>> i915_teardown_sysfs(dev_priv); >>> - i915_guc_log_unregister(dev_priv); >>> + intel_uc_log_unregister(dev_priv); >>> drm_dev_unregister(&dev_priv->drm); >>> i915_gem_shrinker_unregister(dev_priv); >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >>> index 14c855b1a3a4..4bf5f25b29e2 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem.c >>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>> @@ -5279,7 +5279,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv) >>> if (ret) >>> return ret; >>> - ret = intel_uc_init_misc(dev_priv); >>> + ret = intel_uc_init_wq(dev_priv); >>> if (ret) >>> return ret; >>> @@ -5375,7 +5375,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv) >>> intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); >>> mutex_unlock(&dev_priv->drm.struct_mutex); >>> - intel_uc_fini_misc(dev_priv); >>> + intel_uc_fini_wq(dev_priv); >>> if (ret != -EIO) >>> i915_gem_cleanup_userptr(dev_priv); >>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c >>> index 22a05320817b..f1cab43d334e 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc_log.c >>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c >>> @@ -441,7 +441,7 @@ void intel_guc_log_init_early(struct intel_guc *guc) >>> INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work); >>> } >>> -int intel_guc_log_relay_create(struct intel_guc *guc) >>> +int guc_log_relay_create(struct intel_guc *guc) >>> { >>> struct drm_i915_private *dev_priv = guc_to_i915(guc); >>> struct rchan *guc_log_relay_chan; >>> @@ -494,7 +494,7 @@ int intel_guc_log_relay_create(struct intel_guc *guc) >>> return ret; >>> } >>> -void intel_guc_log_relay_destroy(struct intel_guc *guc) >>> +void guc_log_relay_destroy(struct intel_guc *guc) >>> { >>> mutex_lock(&guc->log.runtime.relay_lock); >>> @@ -512,49 +512,6 @@ void intel_guc_log_relay_destroy(struct intel_guc *guc) >>> mutex_unlock(&guc->log.runtime.relay_lock); >>> } >>> -static int guc_log_late_setup(struct intel_guc *guc) >>> -{ >>> - struct drm_i915_private *dev_priv = guc_to_i915(guc); >>> - int ret; >>> - >>> - if (!guc_log_has_runtime(guc)) { >>> - /* >>> - * If log was disabled at boot time, then setup needed to handle >>> - * log buffer flush interrupts would not have been done yet, so >>> - * do that now. >>> - */ >>> - ret = intel_guc_log_relay_create(guc); >>> - if (ret) >>> - goto err; >>> - >>> - mutex_lock(&dev_priv->drm.struct_mutex); >>> - intel_runtime_pm_get(dev_priv); >>> - ret = guc_log_runtime_create(guc); >>> - intel_runtime_pm_put(dev_priv); >>> - mutex_unlock(&dev_priv->drm.struct_mutex); >>> - >>> - if (ret) >>> - goto err_relay; >>> - } >>> - >>> - ret = guc_log_relay_file_create(guc); >>> - if (ret) >>> - goto err_runtime; >>> - >>> - return 0; >>> - >>> -err_runtime: >>> - mutex_lock(&dev_priv->drm.struct_mutex); >>> - guc_log_runtime_destroy(guc); >>> - mutex_unlock(&dev_priv->drm.struct_mutex); >>> -err_relay: >>> - intel_guc_log_relay_destroy(guc); >>> -err: >>> - /* logging will remain off */ >>> - i915_modparams.guc_log_level = 0; >>> - return ret; >>> -} >>> - >>> static void guc_log_capture_logs(struct intel_guc *guc) >>> { >>> struct drm_i915_private *dev_priv = guc_to_i915(guc); >>> @@ -574,16 +531,6 @@ static void guc_flush_logs(struct intel_guc *guc) >>> { >>> struct drm_i915_private *dev_priv = guc_to_i915(guc); >>> - if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level) >>> - return; >>> - >>> - /* First disable the interrupts, will be renabled afterwards */ >>> - mutex_lock(&dev_priv->drm.struct_mutex); >>> - intel_runtime_pm_get(dev_priv); >>> - gen9_disable_guc_interrupts(dev_priv); >>> - intel_runtime_pm_put(dev_priv); >>> - mutex_unlock(&dev_priv->drm.struct_mutex); >>> - >>> /* >>> * Before initiating the forceful flush, wait for any pending/ongoing >>> * flush to complete otherwise forceful flush may not actually happen. >>> @@ -626,12 +573,6 @@ int intel_guc_log_create(struct intel_guc *guc) >>> guc->log.vma = vma; >>> - if (i915_modparams.guc_log_level) { >>> - ret = guc_log_runtime_create(guc); >>> - if (ret < 0) >>> - goto err_vma; >>> - } >>> - >>> /* each allocated unit is a page */ >>> flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL | >>> (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) | >>> @@ -643,8 +584,6 @@ int intel_guc_log_create(struct intel_guc *guc) >>> return 0; >>> -err_vma: >>> - i915_vma_unpin_and_release(&guc->log.vma); >>> err: >>> /* logging will be off */ >>> i915_modparams.guc_log_level = 0; >>> @@ -703,24 +642,14 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) >>> mutex_unlock(&dev_priv->drm.struct_mutex); >>> if (GUC_LOG_IS_ENABLED(val) && !guc_log_has_runtime(guc)) { >>> - ret = guc_log_late_setup(guc); >>> - if (ret) >>> + ret = intel_guc_log_register(guc); >>> + if (ret) { >>> + /* logging will remain off */ >>> + i915_modparams.guc_log_level = 0; >>> goto out; >>> - >>> - /* GuC logging is currently the only user of Guc2Host interrupts */ >>> - mutex_lock(&dev_priv->drm.struct_mutex); >>> - intel_runtime_pm_get(dev_priv); >>> - gen9_enable_guc_interrupts(dev_priv); >>> - intel_runtime_pm_put(dev_priv); >>> - mutex_unlock(&dev_priv->drm.struct_mutex); >>> + } >>> } else if (!GUC_LOG_IS_ENABLED(val) && guc_log_has_runtime(guc)) { >>> - /* >>> - * Once logging is disabled, GuC won't generate logs & send an >>> - * interrupt. But there could be some data in the log buffer >>> - * which is yet to be captured. So request GuC to update the log >>> - * buffer state and then collect the left over logs. >>> - */ >>> - guc_flush_logs(guc); >>> + intel_guc_log_unregister(guc); >>> } >>> return 0; >>> @@ -731,20 +660,63 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) >>> return ret; >>> } >>> -void i915_guc_log_register(struct drm_i915_private *dev_priv) >>> +int intel_guc_log_register(struct intel_guc *guc) >>> { >>> - if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level) >>> - return; >>> + struct drm_i915_private *dev_priv = guc_to_i915(guc); >>> + int ret; >>> + >>> + GEM_BUG_ON(guc_log_has_runtime(guc)); >>> + >>> + /* >>> + * If log was disabled at boot time, then setup needed to handle >>> + * log buffer flush interrupts would not have been done yet, so >>> + * do that now. >>> + */ >>> + ret = guc_log_relay_create(guc); >>> + if (ret) >>> + goto err; >>> + >>> + mutex_lock(&dev_priv->drm.struct_mutex); >>> + ret = guc_log_runtime_create(guc); >>> + mutex_unlock(&dev_priv->drm.struct_mutex); >>> + >>> + if (ret) >>> + goto err_relay; >>> + >>> + ret = guc_log_relay_file_create(guc); >>> + if (ret) >>> + goto err_runtime; >>> - guc_log_late_setup(&dev_priv->guc); >>> + /* GuC logging is currently the only user of Guc2Host interrupts */ >>> + mutex_lock(&dev_priv->drm.struct_mutex); >>> + intel_runtime_pm_get(dev_priv); >>> + gen9_enable_guc_interrupts(dev_priv); >>> + intel_runtime_pm_put(dev_priv); >>> + mutex_unlock(&dev_priv->drm.struct_mutex); >>> + >>> + return 0; >>> + >>> +err_runtime: >>> + mutex_lock(&dev_priv->drm.struct_mutex); >>> + guc_log_runtime_destroy(guc); >>> + mutex_unlock(&dev_priv->drm.struct_mutex); >>> +err_relay: >>> + guc_log_relay_destroy(guc); >>> +err: >>> + return ret; >>> } >>> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv) >>> +void intel_guc_log_unregister(struct intel_guc *guc) >>> { >>> - struct intel_guc *guc = &dev_priv->guc; >>> + struct drm_i915_private *dev_priv = guc_to_i915(guc); >>> - if (!USES_GUC_SUBMISSION(dev_priv)) >>> - return; >>> + /* >>> + * Once logging is disabled, GuC won't generate logs & send an >>> + * interrupt. But there could be some data in the log buffer >>> + * which is yet to be captured. So request GuC to update the log >>> + * buffer state and then collect the left over logs. >>> + */ >>> + guc_flush_logs(guc); >>> mutex_lock(&dev_priv->drm.struct_mutex); >>> /* GuC logging is currently the only user of Guc2Host interrupts */ >>> @@ -755,5 +727,5 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv) >>> guc_log_runtime_destroy(guc); >>> mutex_unlock(&dev_priv->drm.struct_mutex); >>> - intel_guc_log_relay_destroy(guc); >>> + guc_log_relay_destroy(guc); >>> } >>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h >>> index 141ce9ca22ce..09dd2ef1933d 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc_log.h >>> +++ b/drivers/gpu/drm/i915/intel_guc_log.h >>> @@ -62,11 +62,9 @@ struct intel_guc_log { >>> int intel_guc_log_create(struct intel_guc *guc); >>> void intel_guc_log_destroy(struct intel_guc *guc); >>> void intel_guc_log_init_early(struct intel_guc *guc); >>> -int intel_guc_log_relay_create(struct intel_guc *guc); >>> -void intel_guc_log_relay_destroy(struct intel_guc *guc); >>> int intel_guc_log_control_get(struct intel_guc *guc); >>> int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val); >>> -void i915_guc_log_register(struct drm_i915_private *dev_priv); >>> -void i915_guc_log_unregister(struct drm_i915_private *dev_priv); >>> +int intel_guc_log_register(struct intel_guc *guc); >>> +void intel_guc_log_unregister(struct intel_guc *guc); >>> #endif >>> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c >>> index 9f1bac6398fb..3f41ef525e26 100644 >>> --- a/drivers/gpu/drm/i915/intel_uc.c >>> +++ b/drivers/gpu/drm/i915/intel_uc.c >>> @@ -219,6 +219,22 @@ static void guc_free_load_err_log(struct intel_guc *guc) >>> i915_gem_object_put(guc->load_err_log); >>> } >>> +int intel_uc_log_register(struct drm_i915_private *dev_priv) >>> +{ >>> + if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level) >>> + return 0; >>> + >>> + return intel_guc_log_register(&dev_priv->guc); >>> +} >>> + >>> +void intel_uc_log_unregister(struct drm_i915_private *dev_priv) >>> +{ >>> + if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level) >>> + return; >>> + >>> + intel_guc_log_unregister(&dev_priv->guc); >>> +} >>> + >>> static int guc_enable_communication(struct intel_guc *guc) >>> { >>> struct drm_i915_private *dev_priv = guc_to_i915(guc); >>> @@ -240,7 +256,7 @@ static void guc_disable_communication(struct intel_guc *guc) >>> guc->send = intel_guc_send_nop; >>> } >>> -int intel_uc_init_misc(struct drm_i915_private *dev_priv) >>> +int intel_uc_init_wq(struct drm_i915_private *dev_priv) >>> { >>> struct intel_guc *guc = &dev_priv->guc; >>> int ret; >>> @@ -249,26 +265,13 @@ int intel_uc_init_misc(struct drm_i915_private *dev_priv) >>> return 0; >>> ret = intel_guc_init_wq(guc); >>> - if (ret) { >>> - DRM_ERROR("Couldn't allocate workqueues for GuC\n"); >>> - goto err; >>> - } >>> - >>> - ret = intel_guc_log_relay_create(guc); >>> - if (ret) { >>> - DRM_ERROR("Couldn't allocate relay for GuC log\n"); >>> - goto err_relay; >>> - } >>> + if (ret) >>> + return ret; >>> return 0; >>> - >>> -err_relay: >>> - intel_guc_fini_wq(guc); >>> -err: >>> - return ret; >>> } >>> -void intel_uc_fini_misc(struct drm_i915_private *dev_priv) >>> +void intel_uc_fini_wq(struct drm_i915_private *dev_priv) >>> { >>> struct intel_guc *guc = &dev_priv->guc; >>> @@ -276,8 +279,6 @@ void intel_uc_fini_misc(struct drm_i915_private *dev_priv) >>> return; >>> intel_guc_fini_wq(guc); >>> - >>> - intel_guc_log_relay_destroy(guc); >>> } >>> int intel_uc_init(struct drm_i915_private *dev_priv) >>> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h >>> index f2984e01e257..9bc22103396d 100644 >>> --- a/drivers/gpu/drm/i915/intel_uc.h >>> +++ b/drivers/gpu/drm/i915/intel_uc.h >>> @@ -31,10 +31,12 @@ >>> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); >>> void intel_uc_init_early(struct drm_i915_private *dev_priv); >>> void intel_uc_init_mmio(struct drm_i915_private *dev_priv); >>> +int intel_uc_log_register(struct drm_i915_private *dev_priv); >>> +void intel_uc_log_unregister(struct drm_i915_private *dev_priv); >>> 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_misc(struct drm_i915_private *dev_priv); >>> -void intel_uc_fini_misc(struct drm_i915_private *dev_priv); >>> +int intel_uc_init_wq(struct drm_i915_private *dev_priv); >>> +void intel_uc_fini_wq(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); >>> int intel_uc_init(struct drm_i915_private *dev_priv); >> -- >> Thanks, >> Sagar >>
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index aaa861b51024..719b2be73292 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -636,7 +636,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv) i915_gem_contexts_fini(dev_priv); mutex_unlock(&dev_priv->drm.struct_mutex); - intel_uc_fini_misc(dev_priv); + intel_uc_fini_wq(dev_priv); i915_gem_cleanup_userptr(dev_priv); i915_gem_drain_freed_objects(dev_priv); @@ -1237,7 +1237,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) /* Reveal our presence to userspace */ if (drm_dev_register(dev, 0) == 0) { i915_debugfs_register(dev_priv); - i915_guc_log_register(dev_priv); + intel_uc_log_register(dev_priv); i915_setup_sysfs(dev_priv); /* Depends on sysfs having been initialized */ @@ -1297,7 +1297,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) i915_pmu_unregister(dev_priv); i915_teardown_sysfs(dev_priv); - i915_guc_log_unregister(dev_priv); + intel_uc_log_unregister(dev_priv); drm_dev_unregister(&dev_priv->drm); i915_gem_shrinker_unregister(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 14c855b1a3a4..4bf5f25b29e2 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -5279,7 +5279,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv) if (ret) return ret; - ret = intel_uc_init_misc(dev_priv); + ret = intel_uc_init_wq(dev_priv); if (ret) return ret; @@ -5375,7 +5375,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv) intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); mutex_unlock(&dev_priv->drm.struct_mutex); - intel_uc_fini_misc(dev_priv); + intel_uc_fini_wq(dev_priv); if (ret != -EIO) i915_gem_cleanup_userptr(dev_priv); diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 22a05320817b..f1cab43d334e 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -441,7 +441,7 @@ void intel_guc_log_init_early(struct intel_guc *guc) INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work); } -int intel_guc_log_relay_create(struct intel_guc *guc) +int guc_log_relay_create(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); struct rchan *guc_log_relay_chan; @@ -494,7 +494,7 @@ int intel_guc_log_relay_create(struct intel_guc *guc) return ret; } -void intel_guc_log_relay_destroy(struct intel_guc *guc) +void guc_log_relay_destroy(struct intel_guc *guc) { mutex_lock(&guc->log.runtime.relay_lock); @@ -512,49 +512,6 @@ void intel_guc_log_relay_destroy(struct intel_guc *guc) mutex_unlock(&guc->log.runtime.relay_lock); } -static int guc_log_late_setup(struct intel_guc *guc) -{ - struct drm_i915_private *dev_priv = guc_to_i915(guc); - int ret; - - if (!guc_log_has_runtime(guc)) { - /* - * If log was disabled at boot time, then setup needed to handle - * log buffer flush interrupts would not have been done yet, so - * do that now. - */ - ret = intel_guc_log_relay_create(guc); - if (ret) - goto err; - - mutex_lock(&dev_priv->drm.struct_mutex); - intel_runtime_pm_get(dev_priv); - ret = guc_log_runtime_create(guc); - intel_runtime_pm_put(dev_priv); - mutex_unlock(&dev_priv->drm.struct_mutex); - - if (ret) - goto err_relay; - } - - ret = guc_log_relay_file_create(guc); - if (ret) - goto err_runtime; - - return 0; - -err_runtime: - mutex_lock(&dev_priv->drm.struct_mutex); - guc_log_runtime_destroy(guc); - mutex_unlock(&dev_priv->drm.struct_mutex); -err_relay: - intel_guc_log_relay_destroy(guc); -err: - /* logging will remain off */ - i915_modparams.guc_log_level = 0; - return ret; -} - static void guc_log_capture_logs(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); @@ -574,16 +531,6 @@ static void guc_flush_logs(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); - if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level) - return; - - /* First disable the interrupts, will be renabled afterwards */ - mutex_lock(&dev_priv->drm.struct_mutex); - intel_runtime_pm_get(dev_priv); - gen9_disable_guc_interrupts(dev_priv); - intel_runtime_pm_put(dev_priv); - mutex_unlock(&dev_priv->drm.struct_mutex); - /* * Before initiating the forceful flush, wait for any pending/ongoing * flush to complete otherwise forceful flush may not actually happen. @@ -626,12 +573,6 @@ int intel_guc_log_create(struct intel_guc *guc) guc->log.vma = vma; - if (i915_modparams.guc_log_level) { - ret = guc_log_runtime_create(guc); - if (ret < 0) - goto err_vma; - } - /* each allocated unit is a page */ flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL | (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) | @@ -643,8 +584,6 @@ int intel_guc_log_create(struct intel_guc *guc) return 0; -err_vma: - i915_vma_unpin_and_release(&guc->log.vma); err: /* logging will be off */ i915_modparams.guc_log_level = 0; @@ -703,24 +642,14 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) mutex_unlock(&dev_priv->drm.struct_mutex); if (GUC_LOG_IS_ENABLED(val) && !guc_log_has_runtime(guc)) { - ret = guc_log_late_setup(guc); - if (ret) + ret = intel_guc_log_register(guc); + if (ret) { + /* logging will remain off */ + i915_modparams.guc_log_level = 0; goto out; - - /* GuC logging is currently the only user of Guc2Host interrupts */ - mutex_lock(&dev_priv->drm.struct_mutex); - intel_runtime_pm_get(dev_priv); - gen9_enable_guc_interrupts(dev_priv); - intel_runtime_pm_put(dev_priv); - mutex_unlock(&dev_priv->drm.struct_mutex); + } } else if (!GUC_LOG_IS_ENABLED(val) && guc_log_has_runtime(guc)) { - /* - * Once logging is disabled, GuC won't generate logs & send an - * interrupt. But there could be some data in the log buffer - * which is yet to be captured. So request GuC to update the log - * buffer state and then collect the left over logs. - */ - guc_flush_logs(guc); + intel_guc_log_unregister(guc); } return 0; @@ -731,20 +660,63 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) return ret; } -void i915_guc_log_register(struct drm_i915_private *dev_priv) +int intel_guc_log_register(struct intel_guc *guc) { - if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level) - return; + struct drm_i915_private *dev_priv = guc_to_i915(guc); + int ret; + + GEM_BUG_ON(guc_log_has_runtime(guc)); + + /* + * If log was disabled at boot time, then setup needed to handle + * log buffer flush interrupts would not have been done yet, so + * do that now. + */ + ret = guc_log_relay_create(guc); + if (ret) + goto err; + + mutex_lock(&dev_priv->drm.struct_mutex); + ret = guc_log_runtime_create(guc); + mutex_unlock(&dev_priv->drm.struct_mutex); + + if (ret) + goto err_relay; + + ret = guc_log_relay_file_create(guc); + if (ret) + goto err_runtime; - guc_log_late_setup(&dev_priv->guc); + /* GuC logging is currently the only user of Guc2Host interrupts */ + mutex_lock(&dev_priv->drm.struct_mutex); + intel_runtime_pm_get(dev_priv); + gen9_enable_guc_interrupts(dev_priv); + intel_runtime_pm_put(dev_priv); + mutex_unlock(&dev_priv->drm.struct_mutex); + + return 0; + +err_runtime: + mutex_lock(&dev_priv->drm.struct_mutex); + guc_log_runtime_destroy(guc); + mutex_unlock(&dev_priv->drm.struct_mutex); +err_relay: + guc_log_relay_destroy(guc); +err: + return ret; } -void i915_guc_log_unregister(struct drm_i915_private *dev_priv) +void intel_guc_log_unregister(struct intel_guc *guc) { - struct intel_guc *guc = &dev_priv->guc; + struct drm_i915_private *dev_priv = guc_to_i915(guc); - if (!USES_GUC_SUBMISSION(dev_priv)) - return; + /* + * Once logging is disabled, GuC won't generate logs & send an + * interrupt. But there could be some data in the log buffer + * which is yet to be captured. So request GuC to update the log + * buffer state and then collect the left over logs. + */ + guc_flush_logs(guc); mutex_lock(&dev_priv->drm.struct_mutex); /* GuC logging is currently the only user of Guc2Host interrupts */ @@ -755,5 +727,5 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv) guc_log_runtime_destroy(guc); mutex_unlock(&dev_priv->drm.struct_mutex); - intel_guc_log_relay_destroy(guc); + guc_log_relay_destroy(guc); } diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h index 141ce9ca22ce..09dd2ef1933d 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.h +++ b/drivers/gpu/drm/i915/intel_guc_log.h @@ -62,11 +62,9 @@ struct intel_guc_log { int intel_guc_log_create(struct intel_guc *guc); void intel_guc_log_destroy(struct intel_guc *guc); void intel_guc_log_init_early(struct intel_guc *guc); -int intel_guc_log_relay_create(struct intel_guc *guc); -void intel_guc_log_relay_destroy(struct intel_guc *guc); int intel_guc_log_control_get(struct intel_guc *guc); int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val); -void i915_guc_log_register(struct drm_i915_private *dev_priv); -void i915_guc_log_unregister(struct drm_i915_private *dev_priv); +int intel_guc_log_register(struct intel_guc *guc); +void intel_guc_log_unregister(struct intel_guc *guc); #endif diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 9f1bac6398fb..3f41ef525e26 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -219,6 +219,22 @@ static void guc_free_load_err_log(struct intel_guc *guc) i915_gem_object_put(guc->load_err_log); } +int intel_uc_log_register(struct drm_i915_private *dev_priv) +{ + if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level) + return 0; + + return intel_guc_log_register(&dev_priv->guc); +} + +void intel_uc_log_unregister(struct drm_i915_private *dev_priv) +{ + if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level) + return; + + intel_guc_log_unregister(&dev_priv->guc); +} + static int guc_enable_communication(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); @@ -240,7 +256,7 @@ static void guc_disable_communication(struct intel_guc *guc) guc->send = intel_guc_send_nop; } -int intel_uc_init_misc(struct drm_i915_private *dev_priv) +int intel_uc_init_wq(struct drm_i915_private *dev_priv) { struct intel_guc *guc = &dev_priv->guc; int ret; @@ -249,26 +265,13 @@ int intel_uc_init_misc(struct drm_i915_private *dev_priv) return 0; ret = intel_guc_init_wq(guc); - if (ret) { - DRM_ERROR("Couldn't allocate workqueues for GuC\n"); - goto err; - } - - ret = intel_guc_log_relay_create(guc); - if (ret) { - DRM_ERROR("Couldn't allocate relay for GuC log\n"); - goto err_relay; - } + if (ret) + return ret; return 0; - -err_relay: - intel_guc_fini_wq(guc); -err: - return ret; } -void intel_uc_fini_misc(struct drm_i915_private *dev_priv) +void intel_uc_fini_wq(struct drm_i915_private *dev_priv) { struct intel_guc *guc = &dev_priv->guc; @@ -276,8 +279,6 @@ void intel_uc_fini_misc(struct drm_i915_private *dev_priv) return; intel_guc_fini_wq(guc); - - intel_guc_log_relay_destroy(guc); } int intel_uc_init(struct drm_i915_private *dev_priv) diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index f2984e01e257..9bc22103396d 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -31,10 +31,12 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); void intel_uc_init_early(struct drm_i915_private *dev_priv); void intel_uc_init_mmio(struct drm_i915_private *dev_priv); +int intel_uc_log_register(struct drm_i915_private *dev_priv); +void intel_uc_log_unregister(struct drm_i915_private *dev_priv); 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_misc(struct drm_i915_private *dev_priv); -void intel_uc_fini_misc(struct drm_i915_private *dev_priv); +int intel_uc_init_wq(struct drm_i915_private *dev_priv); +void intel_uc_fini_wq(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); int intel_uc_init(struct drm_i915_private *dev_priv);
We have many functions responsible for allocating different parts of runtime called from multiple places. Let's stick with keeping everything in guc_log_register instead. Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 6 +- drivers/gpu/drm/i915/i915_gem.c | 4 +- drivers/gpu/drm/i915/intel_guc_log.c | 148 ++++++++++++++--------------------- drivers/gpu/drm/i915/intel_guc_log.h | 6 +- drivers/gpu/drm/i915/intel_uc.c | 39 ++++----- drivers/gpu/drm/i915/intel_uc.h | 6 +- 6 files changed, 91 insertions(+), 118 deletions(-)