[Intel-gfx] [PATCH 4/8] drm/i915/guc: Disable critical logging in GuC by default from GuC v9

[Kamble, Sagar A]

-----Original Message-----
From: Chris Wilson [mailto:chris at chris-wilson.co.uk] 
Sent: Wednesday, August 9, 2017 4:20 PM
To: Kamble, Sagar A <sagar.a.kamble at intel.com>; intel-gfx at lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915/guc: Disable critical logging in GuC by default from GuC v9

Quoting Sagar Arun Kamble (2017-08-09 11:23:48)
> From GuC v9 firmware (for KBL v9.39+), separate control is added to 
> enable critical logging in GuC to enable capturing minimal important 
> logs in production systems.
> i915.guc_log_level controls the verbosity and logging in GuC for logs 
> other than critical logs. By default, logging in GuC is disabled 
> through i915.guc_log_level.
> This patch introduces new kernel param i915.enable_guc_critical_logging.
> For Linux release builds, if needed critical GuC logs can be enabled 
> separately through this parameter. GuC log snapshot captured in error 
> state will have these minimal critical events logged.
> Default value for this parameter is currently set to false.
> This patch updates the initialization parameter sent during GuC load 
> to disable critical logging unless i915.guc_log_level is set to enable 
> logging and ensures it is enabled/disabling while enabling/disabling 
> through debugfs based on i915.enable_guc_critical_logging.
> 
> Cc: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> Cc: Spotswood John A <john.a.spotswood at intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> Signed-off-by: Jeff McGee <jeff.mcgee at intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_params.c      |  5 +++++
>  drivers/gpu/drm/i915/i915_params.h      |  3 ++-
>  drivers/gpu/drm/i915/intel_guc_fwif.h   | 14 +++++++++++++-
>  drivers/gpu/drm/i915/intel_guc_loader.c | 21 ++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_guc_log.c    |  9 ++++++++-
>  5 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c 
> b/drivers/gpu/drm/i915/i915_params.c
> index 14e2c2e..902bf2c 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -59,6 +59,7 @@ struct i915_params i915 __read_mostly = {
>         .enable_guc_loading = 0,
>         .enable_guc_submission = 0,
>         .guc_log_level = -1,
> +       .enable_guc_critical_logging = false,

What's the point in having a log level if LOG_LEVEL_CRITICAL is not part of it? Please do explain why the current parameter does not cover this.
-Chris

<Sagar> Agree that this could have been enumerated as another log level. Having it as log level simplifies other capture code too which
I am seeing needs fix with current change. Will check if this can be changed in upcoming releases.