[Intel-gfx] [PATCH v2 02/15] drm/i915/guc: Create common entry points for log register/unregister
Sagar Arun Kamble
sagar.a.kamble at intel.com
Fri Mar 9 06:55:53 UTC 2018
On 3/8/2018 9:16 PM, Michał Winiarski wrote:
> We have many functions responsible for allocating different parts of
> GuC log runtime called from multiple places. Let's stick with keeping
> everything in guc_log_register instead.
>
> v2: Use more generic intel_uc_register name, keep using "misc" suffix (Michał)
> s/dev_priv/i915 (Sagar)
> Make guc_log_relay_* static (sparse)
>
> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 6 +-
> drivers/gpu/drm/i915/intel_guc_log.c | 156 ++++++++++++++---------------------
> drivers/gpu/drm/i915/intel_guc_log.h | 6 +-
> drivers/gpu/drm/i915/intel_uc.c | 41 +++++----
> drivers/gpu/drm/i915/intel_uc.h | 2 +
> 5 files changed, 95 insertions(+), 116 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index d7c4de45644d..987c6770d1a6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1238,9 +1238,11 @@ 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);
> i915_setup_sysfs(dev_priv);
>
> + /* Depends on debugfs having been initialized */
> + intel_uc_register(dev_priv);
> +
> /* Depends on sysfs having been initialized */
> i915_perf_register(dev_priv);
> } else
> @@ -1298,7 +1300,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_unregister(dev_priv);
> drm_dev_unregister(&dev_priv->drm);
>
> i915_gem_shrinker_unregister(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 7e59fb07b06b..90b395f34808 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -443,7 +443,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)
> +static 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;
> @@ -496,7 +496,7 @@ int intel_guc_log_relay_create(struct intel_guc *guc)
> return ret;
> }
>
> -void intel_guc_log_relay_destroy(struct intel_guc *guc)
> +static void guc_log_relay_destroy(struct intel_guc *guc)
> {
> mutex_lock(&guc->log.runtime.relay_lock);
>
> @@ -514,49 +514,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);
> @@ -576,16 +533,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.
> @@ -628,12 +575,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) |
> @@ -645,8 +586,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;
> @@ -712,26 +651,14 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
> mutex_unlock(&dev_priv->drm.struct_mutex);
>
> if (enabled && !guc_log_has_runtime(guc)) {
> - ret = guc_log_late_setup(guc);
> + ret = intel_guc_log_register(guc);
> if (ret) {
> - DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", 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 (!enabled && 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;
> @@ -742,29 +669,72 @@ 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 *i915 = 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(&i915->drm.struct_mutex);
> + ret = guc_log_runtime_create(guc);
> + mutex_unlock(&i915->drm.struct_mutex);
> +
> + if (ret)
> + goto err_relay;
> +
> + ret = guc_log_relay_file_create(guc);
> + if (ret)
> + goto err_runtime;
> +
> + /* GuC logging is currently the only user of Guc2Host interrupts */
> + mutex_lock(&i915->drm.struct_mutex);
> + intel_runtime_pm_get(i915);
> + gen9_enable_guc_interrupts(i915);
> + intel_runtime_pm_put(i915);
> + mutex_unlock(&i915->drm.struct_mutex);
> +
> + return 0;
>
> - guc_log_late_setup(&dev_priv->guc);
> +err_runtime:
> + mutex_lock(&i915->drm.struct_mutex);
> + guc_log_runtime_destroy(guc);
> + mutex_unlock(&i915->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 *i915 = 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);
> + mutex_lock(&i915->drm.struct_mutex);
> /* GuC logging is currently the only user of Guc2Host interrupts */
> - intel_runtime_pm_get(dev_priv);
> - gen9_disable_guc_interrupts(dev_priv);
> - intel_runtime_pm_put(dev_priv);
> + intel_runtime_pm_get(i915);
> + gen9_disable_guc_interrupts(i915);
> + intel_runtime_pm_put(i915);
>
> guc_log_runtime_destroy(guc);
> - mutex_unlock(&dev_priv->drm.struct_mutex);
> + mutex_unlock(&i915->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 e5bf0d37bf43..1c1a00df010b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -219,6 +219,28 @@ static void guc_free_load_err_log(struct intel_guc *guc)
> i915_gem_object_put(guc->load_err_log);
> }
>
> +int intel_uc_register(struct drm_i915_private *i915)
> +{
> + int ret = 0;
> +
> + if (!USES_GUC(i915))
> + return 0;
> +
> + if (i915_modparams.guc_log_level)
> + ret = intel_guc_log_register(&i915->guc);
> +
> + return ret;
> +}
> +
> +void intel_uc_unregister(struct drm_i915_private *i915)
> +{
> + if (!USES_GUC(i915))
> + return;
> +
> + if (i915_modparams.guc_log_level)
> + intel_guc_log_unregister(&i915->guc);
> +}
> +
> static int guc_enable_communication(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> @@ -249,23 +271,10 @@ 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)
> @@ -276,8 +285,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 f76d51d1ce70..d6af984cd789 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -31,6 +31,8 @@
> 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_register(struct drm_i915_private *dev_priv);
> +void intel_uc_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);
--
Thanks,
Sagar
More information about the Intel-gfx
mailing list