[Intel-gfx] [PATCH v3 10/13] drm/i915/guc: Allow user to control default GuC logging
Sagar Arun Kamble
sagar.a.kamble at intel.com
Mon Mar 19 11:45:42 UTC 2018
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 at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble at 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 at 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);
--
Thanks,
Sagar
More information about the Intel-gfx
mailing list