[Intel-gfx] [PATCH 3/3] drm/i915/uc: replace uc init/fini misc
Michal Wajdeczko
michal.wajdeczko at intel.com
Tue Jul 2 20:52:51 UTC 2019
On Tue, 02 Jul 2019 22:09:47 +0200, Daniele Ceraolo Spurio
<daniele.ceraolospurio at intel.com> wrote:
> The "misc" terminology doesn't clearly explain what we intend to cover
> in this phase. The only thing we do in there apart from FW fetch is
> initializing the log workqueue. We can move the latter to the
> init_early phase and rename the function to clarify that they only
> fetch/release the blobs.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 7 +++-
> drivers/gpu/drm/i915/i915_gem.c | 12 +++---
> drivers/gpu/drm/i915/intel_guc.c | 57 ++++------------------------
> drivers/gpu/drm/i915/intel_guc.h | 5 +--
> drivers/gpu/drm/i915/intel_guc_log.c | 32 +++++++++++++++-
> drivers/gpu/drm/i915/intel_guc_log.h | 3 +-
> drivers/gpu/drm/i915/intel_huc.c | 8 ----
> drivers/gpu/drm/i915/intel_huc.h | 6 ---
> drivers/gpu/drm/i915/intel_uc.c | 50 ++++++++++--------------
> drivers/gpu/drm/i915/intel_uc.h | 6 +--
> 10 files changed, 75 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 794c6814a6d0..20ca0208dd98 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -934,7 +934,11 @@ static int i915_driver_init_early(struct
> drm_i915_private *dev_priv)
> intel_detect_pch(dev_priv);
> intel_wopcm_init_early(&dev_priv->wopcm);
> - intel_uc_init_early(dev_priv);
> +
> + ret = intel_uc_init_early(dev_priv);
> + if (ret)
> + goto err_gem;
> +
> intel_pm_setup(dev_priv);
> intel_init_dpio(dev_priv);
> ret = intel_power_domains_init(dev_priv);
> @@ -953,6 +957,7 @@ static int i915_driver_init_early(struct
> drm_i915_private *dev_priv)
> err_uc:
> intel_uc_cleanup_early(dev_priv);
> +err_gem:
> i915_gem_cleanup_early(dev_priv);
> err_workqueues:
> i915_workqueues_cleanup(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index b7f290b77f8f..46a75844f303 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1429,13 +1429,11 @@ int i915_gem_init(struct drm_i915_private
> *dev_priv)
> if (ret)
> return ret;
> - ret = intel_uc_init_misc(dev_priv);
> - if (ret)
> - return ret;
> + intel_uc_fetch_firmwares(dev_priv);
> ret = intel_wopcm_init(&dev_priv->wopcm);
> if (ret)
> - goto err_uc_misc;
> + goto err_uc_fw;
> /* This is just a security blanket to placate dragons.
> * On some systems, we very sporadically observe that the first TLBs
> @@ -1561,8 +1559,8 @@ int i915_gem_init(struct drm_i915_private
> *dev_priv)
> intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL);
> mutex_unlock(&dev_priv->drm.struct_mutex);
> -err_uc_misc:
> - intel_uc_fini_misc(dev_priv);
> +err_uc_fw:
> + intel_uc_cleanup_firmwares(dev_priv);
> if (ret != -EIO) {
> i915_gem_cleanup_userptr(dev_priv);
> @@ -1628,7 +1626,7 @@ void i915_gem_fini(struct drm_i915_private
> *dev_priv)
> intel_cleanup_gt_powersave(dev_priv);
> - intel_uc_fini_misc(dev_priv);
> + intel_uc_cleanup_firmwares(dev_priv);
> i915_gem_cleanup_userptr(dev_priv);
> intel_timelines_fini(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_guc.c
> b/drivers/gpu/drm/i915/intel_guc.c
> index 501b74f44374..03fad4fe3d9b 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -74,13 +74,16 @@ void intel_guc_init_send_regs(struct intel_guc *guc)
> guc->send_regs.fw_domains = fw_domains;
> }
> -void intel_guc_init_early(struct intel_guc *guc)
> +int intel_guc_init_early(struct intel_guc *guc)
> {
> struct drm_i915_private *i915 = guc_to_i915(guc);
> + int ret;
> intel_guc_fw_init_early(guc);
> intel_guc_ct_init_early(&guc->ct);
> - intel_guc_log_init_early(&guc->log);
> + ret = intel_guc_log_init_early(&guc->log);
> + if (ret)
> + return ret;
> mutex_init(&guc->send_mutex);
> spin_lock_init(&guc->irq_lock);
> @@ -97,59 +100,13 @@ void intel_guc_init_early(struct intel_guc *guc)
> guc->interrupts.enable = gen9_enable_guc_interrupts;
> guc->interrupts.disable = gen9_disable_guc_interrupts;
> }
> -}
> -
> -static int guc_init_wq(struct intel_guc *guc)
> -{
> - /*
> - * GuC log buffer flush work item has to do register access to
> - * send the ack to GuC and this work item, if not synced before
> - * suspend, can potentially get executed after the GFX device is
> - * suspended.
> - * By marking the WQ as freezable, we don't have to bother about
> - * flushing of this work item from the suspend hooks, the pending
> - * work item if any will be either executed before the suspend
> - * or scheduled later on resume. This way the handling of work
> - * item can be kept same between system suspend & rpm suspend.
> - */
> - guc->log.relay.flush_wq =
> - alloc_ordered_workqueue("i915-guc_log",
> - WQ_HIGHPRI | WQ_FREEZABLE);
> - if (!guc->log.relay.flush_wq) {
> - DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
> - return -ENOMEM;
> - }
> return 0;
> }
> -static void guc_fini_wq(struct intel_guc *guc)
> -{
> - struct workqueue_struct *wq;
> -
> - wq = fetch_and_zero(&guc->log.relay.flush_wq);
> - if (wq)
> - destroy_workqueue(wq);
> -}
> -
> -int intel_guc_init_misc(struct intel_guc *guc)
> +void intel_guc_fini_early(struct intel_guc *guc)
looks like this function can be defined as inline in .h
> {
> - struct drm_i915_private *i915 = guc_to_i915(guc);
> - int ret;
> -
> - ret = guc_init_wq(guc);
> - if (ret)
> - return ret;
> -
> - intel_uc_fw_fetch(i915, &guc->fw);
> -
> - return 0;
> -}
> -
> -void intel_guc_fini_misc(struct intel_guc *guc)
> -{
> - intel_uc_fw_cleanup_fetch(&guc->fw);
> - guc_fini_wq(guc);
> + intel_guc_log_fini_early(&guc->log);
> }
> static int guc_shared_data_create(struct intel_guc *guc)
> diff --git a/drivers/gpu/drm/i915/intel_guc.h
> b/drivers/gpu/drm/i915/intel_guc.h
> index ec1038c1f50e..045a3608bade 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -150,13 +150,12 @@ static inline u32 intel_guc_ggtt_offset(struct
> intel_guc *guc,
> return offset;
> }
> -void intel_guc_init_early(struct intel_guc *guc);
> +int intel_guc_init_early(struct intel_guc *guc);
> +void intel_guc_fini_early(struct intel_guc *guc);
> void intel_guc_init_send_regs(struct intel_guc *guc);
> void intel_guc_init_params(struct intel_guc *guc);
> -int intel_guc_init_misc(struct intel_guc *guc);
> int intel_guc_init(struct intel_guc *guc);
> void intel_guc_fini(struct intel_guc *guc);
> -void intel_guc_fini_misc(struct intel_guc *guc);
> int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32
> len,
> u32 *response_buf, u32 response_buf_size);
> int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32
> len,
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index e3b83ecb90b5..4b1c7a9f23e7 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -374,10 +374,40 @@ static void guc_log_unmap(struct intel_guc_log
> *log)
> log->relay.buf_addr = NULL;
> }
> -void intel_guc_log_init_early(struct intel_guc_log *log)
> +int intel_guc_log_init_early(struct intel_guc_log *log)
> {
> mutex_init(&log->relay.lock);
> INIT_WORK(&log->relay.flush_work, capture_logs_work);
> +
> + /*
> + * GuC log buffer flush work item has to do register access to
> + * send the ack to GuC and this work item, if not synced before
> + * suspend, can potentially get executed after the GFX device is
> + * suspended.
> + * By marking the WQ as freezable, we don't have to bother about
> + * flushing of this work item from the suspend hooks, the pending
> + * work item if any will be either executed before the suspend
> + * or scheduled later on resume. This way the handling of work
> + * item can be kept same between system suspend & rpm suspend.
> + */
> + log->relay.flush_wq =
> + alloc_ordered_workqueue("i915-guc_log",
> + WQ_HIGHPRI | WQ_FREEZABLE);
I'm wondering if this is ok that now we always create wq regardless GuC
is used or not ... in now removed init_misc we checked for USES_GUC
maybe we can use other already existing wq for flush work instead?
> + if (!log->relay.flush_wq) {
> + DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +void intel_guc_log_fini_early(struct intel_guc_log *log)
> +{
> + struct workqueue_struct *wq;
> +
> + wq = fetch_and_zero(&log->relay.flush_wq);
> + if (wq)
do we need to check this? if we failed to allocate wq in init_early
we should not be here
> + destroy_workqueue(wq);
> }
> static int guc_log_relay_create(struct intel_guc_log *log)
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h
> b/drivers/gpu/drm/i915/intel_guc_log.h
> index 7bc763f10c03..8b5e2fc7df24 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -80,7 +80,8 @@ struct intel_guc_log {
> } stats[GUC_MAX_LOG_BUFFER];
> };
> -void intel_guc_log_init_early(struct intel_guc_log *log);
> +int intel_guc_log_init_early(struct intel_guc_log *log);
> +void intel_guc_log_fini_early(struct intel_guc_log *log);
> int intel_guc_log_create(struct intel_guc_log *log);
> void intel_guc_log_destroy(struct intel_guc_log *log);
> diff --git a/drivers/gpu/drm/i915/intel_huc.c
> b/drivers/gpu/drm/i915/intel_huc.c
> index fb6f693d3cac..2a41ee89a16d 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -44,14 +44,6 @@ void intel_huc_init_early(struct intel_huc *huc)
> }
> }
> -int intel_huc_init_misc(struct intel_huc *huc)
> -{
> - struct drm_i915_private *i915 = huc_to_i915(huc);
> -
> - intel_uc_fw_fetch(i915, &huc->fw);
> - return 0;
> -}
> -
> static int intel_huc_rsa_data_create(struct intel_huc *huc)
> {
> struct drm_i915_private *i915 = huc_to_i915(huc);
> diff --git a/drivers/gpu/drm/i915/intel_huc.h
> b/drivers/gpu/drm/i915/intel_huc.h
> index 2a6c94e79f17..9fa3d4629f2e 100644
> --- a/drivers/gpu/drm/i915/intel_huc.h
> +++ b/drivers/gpu/drm/i915/intel_huc.h
> @@ -45,17 +45,11 @@ struct intel_huc {
> };
> void intel_huc_init_early(struct intel_huc *huc);
> -int intel_huc_init_misc(struct intel_huc *huc);
> int intel_huc_init(struct intel_huc *huc);
> void intel_huc_fini(struct intel_huc *huc);
> int intel_huc_auth(struct intel_huc *huc);
> int intel_huc_check_status(struct intel_huc *huc);
> -static inline void intel_huc_fini_misc(struct intel_huc *huc)
> -{
> - intel_uc_fw_cleanup_fetch(&huc->fw);
> -}
> -
> static inline int intel_huc_sanitize(struct intel_huc *huc)
> {
> intel_uc_fw_sanitize(&huc->fw);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index fdf00f1ebb57..c3e65236cfba 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -171,15 +171,19 @@ static void sanitize_options_early(struct
> drm_i915_private *i915)
> GEM_BUG_ON(i915_modparams.guc_log_level < 0);
> }
> -void intel_uc_init_early(struct drm_i915_private *i915)
> +int intel_uc_init_early(struct drm_i915_private *i915)
> {
> - struct intel_guc *guc = &i915->guc;
> - struct intel_huc *huc = &i915->huc;
> + int ret;
> +
> + ret = intel_guc_init_early(&i915->guc);
> + if (ret)
> + return ret;
> - intel_guc_init_early(guc);
> - intel_huc_init_early(huc);
> + intel_huc_init_early(&i915->huc);
> sanitize_options_early(i915);
> +
> + return 0;
> }
> void intel_uc_cleanup_early(struct drm_i915_private *i915)
> @@ -187,6 +191,8 @@ void intel_uc_cleanup_early(struct drm_i915_private
> *i915)
> struct intel_guc *guc = &i915->guc;
> guc_free_load_err_log(guc);
> +
> + intel_guc_fini_early(guc);
> }
> /**
> @@ -345,44 +351,26 @@ static void guc_disable_communication(struct
> intel_guc *guc)
> DRM_INFO("GuC communication disabled\n");
> }
> -int intel_uc_init_misc(struct drm_i915_private *i915)
> +void intel_uc_fetch_firmwares(struct drm_i915_private *i915)
> {
> - struct intel_guc *guc = &i915->guc;
> - struct intel_huc *huc = &i915->huc;
> - int ret;
> -
> if (!USES_GUC(i915))
> - return 0;
> -
> - ret = intel_guc_init_misc(guc);
> - if (ret)
> - return ret;
> + return;
> - if (USES_HUC(i915)) {
> - ret = intel_huc_init_misc(huc);
> - if (ret)
> - goto err_guc;
> - }
> + intel_uc_fw_fetch(i915, &i915->guc.fw);
> - return 0;
> -
> -err_guc:
> - intel_guc_fini_misc(guc);
> - return ret;
> + if (USES_HUC(i915))
> + intel_uc_fw_fetch(i915, &i915->huc.fw);
> }
> -void intel_uc_fini_misc(struct drm_i915_private *i915)
> +void intel_uc_cleanup_firmwares(struct drm_i915_private *i915)
> {
> - struct intel_guc *guc = &i915->guc;
> - struct intel_huc *huc = &i915->huc;
> -
> if (!USES_GUC(i915))
> return;
> if (USES_HUC(i915))
> - intel_huc_fini_misc(huc);
> + intel_uc_fw_cleanup_fetch(&i915->huc.fw);
> - intel_guc_fini_misc(guc);
> + intel_uc_fw_cleanup_fetch(&i915->guc.fw);
> }
> int intel_uc_init(struct drm_i915_private *i915)
> diff --git a/drivers/gpu/drm/i915/intel_uc.h
> b/drivers/gpu/drm/i915/intel_uc.h
> index 3ea06c87dfcd..c3022890e604 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -28,11 +28,11 @@
> #include "intel_huc.h"
> #include "i915_params.h"
> -void intel_uc_init_early(struct drm_i915_private *dev_priv);
> +int intel_uc_init_early(struct drm_i915_private *dev_priv);
> void intel_uc_cleanup_early(struct drm_i915_private *dev_priv);
> void intel_uc_init_mmio(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);
> +void intel_uc_fetch_firmwares(struct drm_i915_private *dev_priv);
> +void intel_uc_cleanup_firmwares(struct drm_i915_private *dev_priv);
> void intel_uc_sanitize(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);
More information about the Intel-gfx
mailing list