[Intel-gfx] [PATCH v3 09/12] drm/i915/guc: Make GuC log related functions depend only on log level

[Michal Wajdeczko]

On Thu, 04 Jan 2018 17:21:51 +0100, Sagar Arun Kamble  
<sagar.a.kamble at intel.com> wrote:

> With GuC log level set properly only for cases where GuC is loaded we can
> remove the GuC submission checks from flush_guc_logs and  
> guc_log_register,
> unregister and uc_fini_hw functions. It is important to note that GuC log
> runtime data has to be freed during driver unregister.
> Freeing of that data can't be gated by guc_log_level check because if we
> free GuC log runtime only when log level >=0 then it will not be  
> destroyed
> when logging is disabled after enabling before driver unload.
>
> Also, with this patch GuC interrupts are enabled first after GuC load if
> logging is enabled. GuC to Host interrupts will be needed for GuC CT
> buffer recv mechanism and hence we will be adding support to control that
> interrupt based on ref. taken by Log or CT recv feature in next patch. To
> prepare for that all interrupt updates are now gated by GuC log level
> checks.
>
> v2: Rebase. Updated check in i915_guc_log_unregister to be based on
> guc_log_level. (Michal Wajdeczko)
>
> v3: Rebase. Made all GuC log related functions depend only log level.
> Updated uC init w.r.t enabling of GuC interrupts. Commit message update.
> Rebase w.r.t guc_log_level immutable changes. (Tvrtko)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc.c     |  3 ++-
>  drivers/gpu/drm/i915/intel_guc_log.c | 17 +++++++----------
>  drivers/gpu/drm/i915/intel_uc.c      | 15 ++++++++-------
>  3 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index d351642..36d1bca 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -406,7 +406,8 @@ int intel_guc_suspend(struct drm_i915_private  
> *dev_priv)
>  	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>  		return 0;
> -	intel_disable_guc_interrupts(guc);
> +	if (guc->log.level >= 0)
> +		intel_disable_guc_interrupts(guc);
> 	data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>  	/* any value greater than GUC_POWER_D0 */
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c  
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index d979830..7bc0065 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -484,8 +484,7 @@ static void guc_log_capture_logs(struct intel_guc  
> *guc)
> static void guc_flush_logs(struct intel_guc *guc)
>  {
> -	if (!USES_GUC_SUBMISSION(dev_priv) ||
> -	    guc->log.level < 0)
> +	if (guc->log.level < 0)
>  		return;
> 	/* First disable the interrupts, will be renabled afterwards */
> @@ -613,8 +612,7 @@ int i915_guc_log_control(struct drm_i915_private  
> *dev_priv, u64 control_val)
> void i915_guc_log_register(struct drm_i915_private *dev_priv)
>  {
> -	if (!USES_GUC_SUBMISSION(dev_priv) ||
> -	    dev_priv->guc.log.level < 0)
> +	if (dev_priv->guc.log.level < 0)
>  		return;
> 	mutex_lock(&dev_priv->drm.struct_mutex);
> @@ -624,14 +622,13 @@ void i915_guc_log_register(struct drm_i915_private  
> *dev_priv)
> void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>  {
> -	if (!USES_GUC_SUBMISSION(dev_priv))
> -		return;
> -
>  	mutex_lock(&dev_priv->drm.struct_mutex);
>  	/* GuC logging is currently the only user of Guc2Host interrupts */
> -	intel_runtime_pm_get(dev_priv);
> -	intel_disable_guc_interrupts(&dev_priv->guc);
> -	intel_runtime_pm_put(dev_priv);
> +	if (dev_priv->guc.log.level >= 0) {

Can we move "if (guc->log.level >= 0)" condition check to
intel_guc_[disable|enable]_interrupts functions? Then we
should be able to avoid repeating this check over and over
and it will be also easier for use once we add new CTB check.

> +		intel_runtime_pm_get(dev_priv);
> +		intel_disable_guc_interrupts(&dev_priv->guc);
> +		intel_runtime_pm_put(dev_priv);
> +	}
> 	intel_guc_log_runtime_destroy(&dev_priv->guc);
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index e2e2020..fb5edcc 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -318,9 +318,12 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	if (ret)
>  		goto err_log_capture;
> +	if (guc->log.level >= 0)
> +		intel_enable_guc_interrupts(guc);
> +
>  	ret = guc_enable_communication(guc);
>  	if (ret)
> -		goto err_log_capture;
> +		goto err_interrupts;
> 	if (USES_HUC(dev_priv)) {
>  		ret = intel_huc_auth(huc);
> @@ -329,12 +332,9 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	}
> 	if (USES_GUC_SUBMISSION(dev_priv)) {
> -		if (guc->log.level >= 0)
> -			intel_enable_guc_interrupts(guc);
> -
>  		ret = intel_guc_submission_enable(guc);
>  		if (ret)
> -			goto err_interrupts;
> +			goto err_communication;
>  	}
> 	dev_info(dev_priv->drm.dev, "GuC firmware version %u.%u\n",
> @@ -349,10 +349,11 @@ int intel_uc_init_hw(struct drm_i915_private  
> *dev_priv)
>  	/*
>  	 * We've failed to load the firmware :(
>  	 */
> -err_interrupts:
> -	intel_disable_guc_interrupts(guc);
>  err_communication:
>  	guc_disable_communication(guc);
> +err_interrupts:
> +	if (guc->log.level >= 0)
> +		intel_disable_guc_interrupts(guc);
>  err_log_capture:
>  	guc_capture_load_err_log(guc);
>  err_out: