| Message ID | 20180320084929.5038-1-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@chris-wilson.co.uk> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@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@chris-wilson.co.uk> -Chris
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) #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) 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; + else if (IS_ENABLED(CONFIG_DRM_I915_DEBUG) || + IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) + guc_log_level = GUC_LOG_LEVEL_MAX; + else + guc_log_level = GUC_LOG_LEVEL_NON_VERBOSE; /* 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; } DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s, verbose:%s, verbosity:%d)\n",
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@chris-wilson.co.uk> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@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(-)