[Intel-gfx] [PATCH 01/15] drm/i915/guc: Tidy guc_log_control

[Michal Wajdeczko]

On Tue, 27 Feb 2018 13:52:16 +0100, Michał Winiarski  
<michal.winiarski at intel.com> wrote:

> We plan to decouple log runtime (mapping + relay) from verbosity control.
> Let's tidy the code now to reduce the churn in the following patches.
>
> 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>
> ---

/snip/

> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -657,52 +657,55 @@ void intel_guc_log_destroy(struct intel_guc *guc)
>  	i915_vma_unpin_and_release(&guc->log.vma);
>  }
> -int intel_guc_log_control(struct intel_guc *guc, u64 control_val)
> +int intel_guc_log_control_get(struct intel_guc *guc)

Can we add kernel-doc for this new function?

> +{
> +	GEM_BUG_ON(!guc->log.vma);
> +	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
> +
> +	return i915_modparams.guc_log_level;
> +}
> +
> +#define GUC_LOG_IS_ENABLED(x)		(x > 0)

1) x must be wrapped into (x)
2) GUC_LOG_ prefix is misleading, maybe LOG_LEVEL_TO_ENABLED()

> +#define GUC_LOG_LEVEL_TO_VERBOSITY(x)	(GUC_LOG_IS_ENABLED(x) ? x - 1 :  
> 0)

1) x must be wrapped into (x)
2) try to avoid double evaluation of x

> +int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
>  {
>  	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -	bool enable_logging = control_val > 0;
> -	u32 verbosity;
>  	int ret;
> -	if (!guc->log.vma)
> -		return -ENODEV;
> +	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0);
> +	GEM_BUG_ON(!guc->log.vma);
> +	GEM_BUG_ON(i915_modparams.guc_log_level < 0);
> -	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN);
> -	if (control_val > 1 + GUC_LOG_VERBOSITY_MAX)
> +	/*
> +	 * GuC is recognizing log levels starting from 0 to max, we're using 0
> +	 * as indication that logging should be disablded.

typo

> +	 */
> +	if (GUC_LOG_LEVEL_TO_VERBOSITY(val) < GUC_LOG_VERBOSITY_MIN ||
> +	    GUC_LOG_LEVEL_TO_VERBOSITY(val) > GUC_LOG_VERBOSITY_MAX)

try to use helper variable easier read:

int verbosity = LOG_LEVEL_TO_VERBOSITY(val);
if (verbosity < GUC_LOG_VERBOSITY_MIN || verbosity > GUC_LOG_VERBOSITY_MAX)

>  		return -EINVAL;
> -	/* This combination doesn't make sense & won't have any effect */
> -	if (!enable_logging && !i915_modparams.guc_log_level)
> -		return 0;
> +	mutex_lock(&dev_priv->drm.struct_mutex);
> -	verbosity = enable_logging ? control_val - 1 : 0;
> +	if (i915_modparams.guc_log_level == val) {
> +		ret = 0;
> +		goto out_unlock;
> +	}
> -	ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
> -	if (ret)
> -		return ret;
>  	intel_runtime_pm_get(dev_priv);
> -	ret = guc_log_control(guc, enable_logging, verbosity);
> +	ret = guc_log_control(guc, GUC_LOG_IS_ENABLED(val),
> +			      GUC_LOG_LEVEL_TO_VERBOSITY(val));
>  	intel_runtime_pm_put(dev_priv);
> -	mutex_unlock(&dev_priv->drm.struct_mutex);
> +	if (ret)
> +		goto out_unlock;
> -	if (ret < 0) {
> -		DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
> -		return ret;
> -	}
> +	i915_modparams.guc_log_level = val;
> -	if (enable_logging) {
> -		i915_modparams.guc_log_level = 1 + verbosity;
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
> -		/*
> -		 * If log was disabled at boot time, then the relay channel file
> -		 * wouldn't have been created by now and interrupts also would
> -		 * not have been enabled. Try again now, just in case.
> -		 */
> +	if (GUC_LOG_IS_ENABLED(val) && !guc_log_has_runtime(guc)) {
>  		ret = guc_log_late_setup(guc);
> -		if (ret < 0) {
> -			DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);

why do you want to remove all these diagnostic messages?

> -			return ret;
> -		}
> +		if (ret)
> +			goto out;
> 		/* GuC logging is currently the only user of Guc2Host interrupts */
>  		mutex_lock(&dev_priv->drm.struct_mutex);
> @@ -710,7 +713,7 @@ int intel_guc_log_control(struct intel_guc *guc, u64  
> control_val)
>  		gen9_enable_guc_interrupts(dev_priv);
>  		intel_runtime_pm_put(dev_priv);
>  		mutex_unlock(&dev_priv->drm.struct_mutex);
> -	} else {
> +	} 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
> @@ -718,11 +721,13 @@ int intel_guc_log_control(struct intel_guc *guc,  
> u64 control_val)
>  		 * buffer state and then collect the left over logs.
>  		 */
>  		guc_flush_logs(guc);
> -
> -		/* As logging is disabled, update log level to reflect that */
> -		i915_modparams.guc_log_level = 0;
>  	}
> +	return 0;
> +
> +out_unlock:
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
> +out:
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h  
> b/drivers/gpu/drm/i915/intel_guc_log.h
> index dab0e949567a..141ce9ca22ce 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -64,7 +64,8 @@ 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(struct intel_guc *guc, u64 control_val);
> +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);