Re: [PATCH 3/3] drm/i915/uc: replace uc init/fini misc

["Michal Wajdeczko" <michal.wajdeczko@xxxxxxxxx>]

On Tue, 02 Jul 2019 22:09:47 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@xxxxxxxxx> 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@xxxxxxxxx>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> ---
>  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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx