[Intel-gfx] [PATCH 04/11] drm/i915/guc: Sanitize module parameter guc_log_level

[Tvrtko Ursulin]

On 18/10/2017 07:46, Sagar Arun Kamble wrote:
> Parameter guc_log_level needs to be sanitized based on GuC support and
> enable_guc_loading parameter since it depends on them like
> enable_guc_submission. This will make GuC logging paths independent of
> enable_guc_submission parameter in further patches.
> 
> v2: Added documentation to intel_guc_log.c and param description about
> GuC loading dependency. (Michal Wajdeczko)
> 
> 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/i915_params.c   |  4 +++-
>   drivers/gpu/drm/i915/intel_guc_log.c |  1 +
>   drivers/gpu/drm/i915/intel_uc.c      | 10 +++++++---
>   3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index b4faeb6..774c56e 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -171,7 +171,9 @@ struct i915_params i915_modparams __read_mostly = {
>   	"(-1=auto, 0=never [default], 1=if available, 2=required)");
>   
>   i915_param_named(guc_log_level, int, 0400,
> -	"GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
> +	"GuC firmware logging level. This takes effect only if GuC is to be "
> +	"loaded (depends on enable_guc_loading) (-1:disabled (default), "
> +	"0-3:enabled)");
>   
>   i915_param_named_unsafe(guc_firmware_path, charp, 0400,
>   	"GuC firmware path to use instead of the default one");
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index f53c663..200f0a1 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -34,6 +34,7 @@
>    * DOC: GuC firmware log
>    *
>    * Firmware log is enabled by setting i915.guc_log_level to non-negative level.
> + * This takes effect only if GuC is to be loaded based on enable_guc_loading.
>    * Log data is printed out via reading debugfs i915_guc_log_dump. Reading from
>    * i915_guc_load_status will print out firmware loading status and scratch
>    * registers value.
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 62738ad..8feefcd 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -51,11 +51,13 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>   {
>   	if (!HAS_GUC(dev_priv)) {
>   		if (i915_modparams.enable_guc_loading > 0 ||
> -		    i915_modparams.enable_guc_submission > 0)
> +		    i915_modparams.enable_guc_submission > 0 ||
> +		    i915_modparams.guc_log_level > 0)
>   			DRM_INFO("Ignoring GuC options, no hardware\n");

Hm, this won't fire on <=gen8 once enable_guc_submission starts to 
default to one? Or we can worry about that when we get there...

>   
>   		i915_modparams.enable_guc_loading = 0;
>   		i915_modparams.enable_guc_submission = 0;
> +		i915_modparams.guc_log_level = -1;
>   		return;
>   	}
>   
> @@ -72,9 +74,11 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>   			i915_modparams.enable_guc_loading = 0;
>   	}
>   
> -	/* Can't enable guc submission without guc loaded */
> -	if (!i915_modparams.enable_guc_loading)
> +	/* Can't enable guc submission and logging without guc loaded */
> +	if (!i915_modparams.enable_guc_loading) {
>   		i915_modparams.enable_guc_submission = 0;
> +		i915_modparams.guc_log_level = -1;

Hm2, how does this interact with the changes which are removing 
enable_guc_loading? Or it is a race?

I was just thinking that we should probably let the user know if they 
asked for logging, but it cannot happen due guc loading turned off, to 
have analogy with the same when GuC is not available in the first place.

Does that make sense?

> +	}
>   
>   	/* A negative value means "use platform default" */
>   	if (i915_modparams.enable_guc_submission < 0)
> 

But since it looks like a cleanup even without the above:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko