[Intel-gfx] [PATCH v2] drm/i915/guc: Don't try to enable GuC logging when we're not using GuC

[Chris Wilson]

Quoting Michał Winiarski (2018-03-20 08:49:29)
> When changing the default values for guc_log_level, we accidentally left
> the log enabled on non-guc platforms. Let's fix that.
> 
> v2: Define the levels used and remove (now obsolete) comments (Chris)
> 
> Fixes: 9605d1ce7c6b ("drm/i915/guc: Default to non-verbose GuC logging")
> Reported-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_log.c |  3 +--
>  drivers/gpu/drm/i915/intel_guc_log.h | 10 ++++++----
>  drivers/gpu/drm/i915/intel_uc.c      | 21 ++++++++++-----------
>  3 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 4cb422ceb283..16d206680edf 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -515,8 +515,7 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val)
>          * GuC is recognizing log levels starting from 0 to max, we're using 0
>          * as indication that logging should be disabled.
>          */
> -       if (val < GUC_LOG_LEVEL_DISABLED ||
> -           val > GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX))
> +       if (val < GUC_LOG_LEVEL_DISABLED || val > GUC_LOG_LEVEL_MAX)
>                 return -EINVAL;
>  
>         mutex_lock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> index af1532c0d3e4..5793db8f9c09 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -46,14 +46,16 @@ struct intel_guc;
>   * log enabling, and separate bit for default logging - which "conveniently"
>   * ignores the enable bit.
>   */
> -#define GUC_LOG_LEVEL_DISABLED                 0
> -#define GUC_LOG_LEVEL_TO_ENABLED(x)            ((x) > 0)
> -#define GUC_LOG_LEVEL_TO_VERBOSE(x)            ((x) > 1)
> +#define GUC_LOG_LEVEL_DISABLED         0
> +#define GUC_LOG_LEVEL_NON_VERBOSE      1
> +#define GUC_LOG_LEVEL_TO_ENABLED(x)    ((x) > GUC_LOG_LEVEL_DISABLED)
> +#define GUC_LOG_LEVEL_TO_VERBOSE(x)    ((x) > GUC_LOG_LEVEL_NON_VERBOSE)

Since this pair is boolean, may is suggest IS_ENABLED, IS_VERBOSE
respectively?

>  #define GUC_LOG_LEVEL_TO_VERBOSITY(x) ({               \
>         typeof(x) _x = (x);                             \
>         GUC_LOG_LEVEL_TO_VERBOSE(_x) ? _x - 2 : 0;      \
>  })
> -#define GUC_VERBOSITY_TO_LOG_LEVEL(x)          ((x) + 2)
> +#define GUC_VERBOSITY_TO_LOG_LEVEL(x)  ((x) + 2)
> +#define GUC_LOG_LEVEL_MAX GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX)

Then you have

bool: IS_ENABLED, IS_VERBOSE;
int: TO_VERBOSITY, TO_LOG_LEVEL;

>  
>  struct intel_guc_log {
>         u32 flags;
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 34e847d0ee4c..b525411ae631 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -69,14 +69,15 @@ static int __get_platform_enable_guc(struct drm_i915_private *dev_priv)
>  
>  static int __get_default_guc_log_level(struct drm_i915_private *dev_priv)
>  {
> -       int guc_log_level = 1; /* non-verbose */
> +       int guc_log_level;
>  
> -       /* Enable if we're running on platform with GuC and debug config */
> -       if (HAS_GUC(dev_priv) && intel_uc_is_using_guc() &&
> -           (IS_ENABLED(CONFIG_DRM_I915_DEBUG) ||
> -            IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)))
> -               guc_log_level =
> -                       GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX);
> +       if (!HAS_GUC(dev_priv) || !intel_uc_is_using_guc())
> +               guc_log_level = GUC_LOG_LEVEL_DISABLED;

Ok. That'll quieten down the surprise.

> +       else if (IS_ENABLED(CONFIG_DRM_I915_DEBUG) ||
> +                IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> +               guc_log_level = GUC_LOG_LEVEL_MAX;

Noisy for CI.

> +       else
> +               guc_log_level = GUC_LOG_LEVEL_NON_VERBOSE;

Otherwise silent until asked.

>  
>         /* Any platform specific fine-tuning can be done here */
>  
> @@ -143,13 +144,11 @@ static void sanitize_options_early(struct drm_i915_private *dev_priv)
>                 i915_modparams.guc_log_level = 0;
>         }
>  
> -       if (i915_modparams.guc_log_level >
> -           GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX)) {
> +       if (i915_modparams.guc_log_level > GUC_LOG_LEVEL_MAX) {
>                 DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
>                          "guc_log_level", i915_modparams.guc_log_level,
>                          "verbosity too high");
> -               i915_modparams.guc_log_level =
> -                       GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX);
> +               i915_modparams.guc_log_level = GUC_LOG_LEVEL_MAX;
>         }

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris