| Message ID | 20180319095348.9716-10-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 3/19/2018 3:23 PM, Michał Winiarski wrote: > While both naming and actual log enable logic in GuC interface are > confusing, we can simply expose the default log as yet another log > level. > GuC logic aside, from i915 point of view we now have the following GuC > log levels: > 0 Log disabled > 1 Non-verbose log > 2-5 Verbose log > > v2: Adjust naming after rebase. > v3: Fixed the log_level logic error introduced on rebase. > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v2) > --- > drivers/gpu/drm/i915/intel_guc.c | 24 +++++++++++++++--------- > drivers/gpu/drm/i915/intel_guc_fwif.h | 5 +++-- > drivers/gpu/drm/i915/intel_guc_log.c | 18 +++++++----------- > drivers/gpu/drm/i915/intel_guc_log.h | 15 +++++++++++++++ > drivers/gpu/drm/i915/intel_uc.c | 14 +++++++++----- > 5 files changed, 49 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c > index d4c2524012fa..05c3484d02a3 100644 > --- a/drivers/gpu/drm/i915/intel_guc.c > +++ b/drivers/gpu/drm/i915/intel_guc.c > @@ -221,17 +221,23 @@ static u32 get_core_family(struct drm_i915_private *dev_priv) > } > } > > -static u32 get_log_verbosity_flags(void) > +static u32 get_log_control_flags(void) > { > - if (i915_modparams.guc_log_level > 0) { > - u32 verbosity = i915_modparams.guc_log_level - 1; > + u32 level = i915_modparams.guc_log_level; > + u32 flags = 0; > > - GEM_BUG_ON(verbosity > GUC_LOG_VERBOSITY_MAX); > - return verbosity << GUC_LOG_VERBOSITY_SHIFT; > - } > + GEM_BUG_ON(level < 0); > + > + if (!GUC_LOG_LEVEL_TO_ENABLED(level)) > + flags |= GUC_LOG_DEFAULT_DISABLED; > + > + if (!GUC_LOG_LEVEL_TO_VERBOSE(level)) > + flags |= GUC_LOG_DISABLED; > + else > + flags |= GUC_LOG_LEVEL_TO_VERBOSITY(level) << > + GUC_LOG_VERBOSITY_SHIFT; > > - GEM_BUG_ON(i915_modparams.enable_guc < 0); > - return GUC_LOG_DISABLED; > + return flags; > } > > /* > @@ -266,7 +272,7 @@ void intel_guc_init_params(struct intel_guc *guc) > > params[GUC_CTL_LOG_PARAMS] = guc->log.flags; > > - params[GUC_CTL_DEBUG] = get_log_verbosity_flags(); > + params[GUC_CTL_DEBUG] = get_log_control_flags(); > > /* If GuC submission is enabled, set up additional parameters here */ > if (USES_GUC_SUBMISSION(dev_priv)) { > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h > index 6a10aa6f04d3..4971685a2ea8 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > @@ -127,7 +127,7 @@ > #define GUC_PROFILE_ENABLED (1 << 7) > #define GUC_WQ_TRACK_ENABLED (1 << 8) > #define GUC_ADS_ENABLED (1 << 9) > -#define GUC_DEBUG_RESERVED (1 << 10) > +#define GUC_LOG_DEFAULT_DISABLED (1 << 10) > #define GUC_ADS_ADDR_SHIFT 11 > #define GUC_ADS_ADDR_MASK 0xfffff800 > > @@ -539,7 +539,8 @@ union guc_log_control { > u32 logging_enabled:1; > u32 reserved1:3; > u32 verbosity:4; > - u32 reserved2:24; > + u32 default_logging:1; > + u32 reserved2:23; > }; > u32 value; > } __packed; > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c > index 1e671f2b2f64..068f5e7f7594 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -57,12 +57,14 @@ static int guc_log_flush(struct intel_guc *guc) > return intel_guc_send(guc, action, ARRAY_SIZE(action)); > } > > -static int guc_log_control(struct intel_guc *guc, bool enable, u32 verbosity) > +static int guc_log_control(struct intel_guc *guc, bool enable, > + bool default_logging, u32 verbosity) Can we change the order of parameters as: struct intel_guc *guc, bool default_logging, bool enable, u32 verbosity That way I can see that param 3 and param 4 are related. But this change can be done at your discretion. Updated logic to handle default logging looks fine. Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > { > union guc_log_control control_val = { > { > .logging_enabled = enable, > .verbosity = verbosity, > + .default_logging = default_logging, > }, > }; > u32 action[] = { > @@ -499,13 +501,6 @@ int intel_guc_log_level_get(struct intel_guc_log *log) > return i915_modparams.guc_log_level; > } > > -#define GUC_LOG_LEVEL_DISABLED 0 > -#define LOG_LEVEL_TO_ENABLED(x) ((x) > 0) > -#define LOG_LEVEL_TO_VERBOSITY(x) ({ \ > - typeof(x) _x = (x); \ > - LOG_LEVEL_TO_ENABLED(_x) ? _x - 1 : 0; \ > -}) > -#define VERBOSITY_TO_LOG_LEVEL(x) ((x) + 1) > int intel_guc_log_level_set(struct intel_guc_log *log, u64 val) > { > struct intel_guc *guc = log_to_guc(log); > @@ -521,7 +516,7 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val) > * as indication that logging should be disabled. > */ > if (val < GUC_LOG_LEVEL_DISABLED || > - val > VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX)) > + val > GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX)) > return -EINVAL; > > mutex_lock(&dev_priv->drm.struct_mutex); > @@ -532,8 +527,9 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val) > } > > intel_runtime_pm_get(dev_priv); > - ret = guc_log_control(guc, LOG_LEVEL_TO_ENABLED(val), > - LOG_LEVEL_TO_VERBOSITY(val)); > + ret = guc_log_control(guc, GUC_LOG_LEVEL_TO_VERBOSE(val), > + GUC_LOG_LEVEL_TO_ENABLED(val), > + GUC_LOG_LEVEL_TO_VERBOSITY(val)); > intel_runtime_pm_put(dev_priv); > if (ret) { > DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret); > diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h > index cc86587a0543..dab27785231b 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.h > +++ b/drivers/gpu/drm/i915/intel_guc_log.h > @@ -40,6 +40,21 @@ struct intel_guc; > #define GUC_LOG_SIZE ((1 + GUC_LOG_DPC_PAGES + 1 + GUC_LOG_ISR_PAGES + \ > 1 + GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT) > > +/* > + * While we're using plain log level in i915, GuC controls are much more... > + * "elaborate"? We have a couple of bits for verbosity, separate bit for actual > + * 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_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) > + > struct intel_guc_log { > u32 flags; > struct i915_vma *vma; > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 9bb40cd047a0..ad1785522497 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -75,7 +75,8 @@ static int __get_default_guc_log_level(struct drm_i915_private *dev_priv) > 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 = 1 + GUC_LOG_VERBOSITY_MAX; > + guc_log_level = > + GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX); > > /* Any platform specific fine-tuning can be done here */ > > @@ -142,17 +143,20 @@ static void sanitize_options_early(struct drm_i915_private *dev_priv) > i915_modparams.guc_log_level = 0; > } > > - if (i915_modparams.guc_log_level > 1 + GUC_LOG_VERBOSITY_MAX) { > + if (i915_modparams.guc_log_level > > + GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_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 = 1 + GUC_LOG_VERBOSITY_MAX; > + i915_modparams.guc_log_level = > + GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX); > } > > - DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s verbosity:%d)\n", > + DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s, verbose:%s, verbosity:%d)\n", > i915_modparams.guc_log_level, > yesno(i915_modparams.guc_log_level), > - i915_modparams.guc_log_level - 1); > + yesno(GUC_LOG_LEVEL_TO_VERBOSE(i915_modparams.guc_log_level)), > + GUC_LOG_LEVEL_TO_VERBOSITY(i915_modparams.guc_log_level)); > > /* Make sure that sanitization was done */ > GEM_BUG_ON(i915_modparams.enable_guc < 0);
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index d4c2524012fa..05c3484d02a3 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -221,17 +221,23 @@ static u32 get_core_family(struct drm_i915_private *dev_priv) } } -static u32 get_log_verbosity_flags(void) +static u32 get_log_control_flags(void) { - if (i915_modparams.guc_log_level > 0) { - u32 verbosity = i915_modparams.guc_log_level - 1; + u32 level = i915_modparams.guc_log_level; + u32 flags = 0; - GEM_BUG_ON(verbosity > GUC_LOG_VERBOSITY_MAX); - return verbosity << GUC_LOG_VERBOSITY_SHIFT; - } + GEM_BUG_ON(level < 0); + + if (!GUC_LOG_LEVEL_TO_ENABLED(level)) + flags |= GUC_LOG_DEFAULT_DISABLED; + + if (!GUC_LOG_LEVEL_TO_VERBOSE(level)) + flags |= GUC_LOG_DISABLED; + else + flags |= GUC_LOG_LEVEL_TO_VERBOSITY(level) << + GUC_LOG_VERBOSITY_SHIFT; - GEM_BUG_ON(i915_modparams.enable_guc < 0); - return GUC_LOG_DISABLED; + return flags; } /* @@ -266,7 +272,7 @@ void intel_guc_init_params(struct intel_guc *guc) params[GUC_CTL_LOG_PARAMS] = guc->log.flags; - params[GUC_CTL_DEBUG] = get_log_verbosity_flags(); + params[GUC_CTL_DEBUG] = get_log_control_flags(); /* If GuC submission is enabled, set up additional parameters here */ if (USES_GUC_SUBMISSION(dev_priv)) { diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index 6a10aa6f04d3..4971685a2ea8 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -127,7 +127,7 @@ #define GUC_PROFILE_ENABLED (1 << 7) #define GUC_WQ_TRACK_ENABLED (1 << 8) #define GUC_ADS_ENABLED (1 << 9) -#define GUC_DEBUG_RESERVED (1 << 10) +#define GUC_LOG_DEFAULT_DISABLED (1 << 10) #define GUC_ADS_ADDR_SHIFT 11 #define GUC_ADS_ADDR_MASK 0xfffff800 @@ -539,7 +539,8 @@ union guc_log_control { u32 logging_enabled:1; u32 reserved1:3; u32 verbosity:4; - u32 reserved2:24; + u32 default_logging:1; + u32 reserved2:23; }; u32 value; } __packed; diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 1e671f2b2f64..068f5e7f7594 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -57,12 +57,14 @@ static int guc_log_flush(struct intel_guc *guc) return intel_guc_send(guc, action, ARRAY_SIZE(action)); } -static int guc_log_control(struct intel_guc *guc, bool enable, u32 verbosity) +static int guc_log_control(struct intel_guc *guc, bool enable, + bool default_logging, u32 verbosity) { union guc_log_control control_val = { { .logging_enabled = enable, .verbosity = verbosity, + .default_logging = default_logging, }, }; u32 action[] = { @@ -499,13 +501,6 @@ int intel_guc_log_level_get(struct intel_guc_log *log) return i915_modparams.guc_log_level; } -#define GUC_LOG_LEVEL_DISABLED 0 -#define LOG_LEVEL_TO_ENABLED(x) ((x) > 0) -#define LOG_LEVEL_TO_VERBOSITY(x) ({ \ - typeof(x) _x = (x); \ - LOG_LEVEL_TO_ENABLED(_x) ? _x - 1 : 0; \ -}) -#define VERBOSITY_TO_LOG_LEVEL(x) ((x) + 1) int intel_guc_log_level_set(struct intel_guc_log *log, u64 val) { struct intel_guc *guc = log_to_guc(log); @@ -521,7 +516,7 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val) * as indication that logging should be disabled. */ if (val < GUC_LOG_LEVEL_DISABLED || - val > VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX)) + val > GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX)) return -EINVAL; mutex_lock(&dev_priv->drm.struct_mutex); @@ -532,8 +527,9 @@ int intel_guc_log_level_set(struct intel_guc_log *log, u64 val) } intel_runtime_pm_get(dev_priv); - ret = guc_log_control(guc, LOG_LEVEL_TO_ENABLED(val), - LOG_LEVEL_TO_VERBOSITY(val)); + ret = guc_log_control(guc, GUC_LOG_LEVEL_TO_VERBOSE(val), + GUC_LOG_LEVEL_TO_ENABLED(val), + GUC_LOG_LEVEL_TO_VERBOSITY(val)); intel_runtime_pm_put(dev_priv); if (ret) { DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret); diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h index cc86587a0543..dab27785231b 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.h +++ b/drivers/gpu/drm/i915/intel_guc_log.h @@ -40,6 +40,21 @@ struct intel_guc; #define GUC_LOG_SIZE ((1 + GUC_LOG_DPC_PAGES + 1 + GUC_LOG_ISR_PAGES + \ 1 + GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT) +/* + * While we're using plain log level in i915, GuC controls are much more... + * "elaborate"? We have a couple of bits for verbosity, separate bit for actual + * 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_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) + struct intel_guc_log { u32 flags; struct i915_vma *vma; diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 9bb40cd047a0..ad1785522497 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -75,7 +75,8 @@ static int __get_default_guc_log_level(struct drm_i915_private *dev_priv) 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 = 1 + GUC_LOG_VERBOSITY_MAX; + guc_log_level = + GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX); /* Any platform specific fine-tuning can be done here */ @@ -142,17 +143,20 @@ static void sanitize_options_early(struct drm_i915_private *dev_priv) i915_modparams.guc_log_level = 0; } - if (i915_modparams.guc_log_level > 1 + GUC_LOG_VERBOSITY_MAX) { + if (i915_modparams.guc_log_level > + GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_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 = 1 + GUC_LOG_VERBOSITY_MAX; + i915_modparams.guc_log_level = + GUC_VERBOSITY_TO_LOG_LEVEL(GUC_LOG_VERBOSITY_MAX); } - DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s verbosity:%d)\n", + DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s, verbose:%s, verbosity:%d)\n", i915_modparams.guc_log_level, yesno(i915_modparams.guc_log_level), - i915_modparams.guc_log_level - 1); + yesno(GUC_LOG_LEVEL_TO_VERBOSE(i915_modparams.guc_log_level)), + GUC_LOG_LEVEL_TO_VERBOSITY(i915_modparams.guc_log_level)); /* Make sure that sanitization was done */ GEM_BUG_ON(i915_modparams.enable_guc < 0);