From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
To: "Michał Winiarski" <michal.winiarski@intel.com>,
intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 10/13] drm/i915/guc: Allow user to control default GuC logging
Date: Mon, 19 Mar 2018 17:15:42 +0530 [thread overview]
Message-ID: <a887657b-cfed-5c2e-e75d-b7833ac99189@intel.com> (raw)
In-Reply-To: <20180319095348.9716-10-michal.winiarski@intel.com>
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);
--
Thanks,
Sagar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2018-03-19 11:45 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-19 9:53 [PATCH v3 01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC Michał Winiarski
2018-03-19 9:53 ` [PATCH v3 02/13] drm/i915/guc: Log runtime should consist of both mapping and relay Michał Winiarski
2018-03-19 9:53 ` [PATCH v3 03/13] drm/i915/guc: Merge log relay file and channel creation Michał Winiarski
2018-03-19 9:53 ` [PATCH v2 04/13] drm/i915/guc: Flush directly in log unregister Michał Winiarski
2018-03-19 9:53 ` [PATCH v3 05/13] drm/i915/guc: Split relay control and GuC log level Michał Winiarski
2018-03-19 9:53 ` [PATCH v2 06/13] drm/i915/guc: Move check for fast memcpy_wc to relay creation Michał Winiarski
2018-03-19 9:53 ` [PATCH v2 07/13] drm/i915/guc: Get rid of GuC log runtime Michał Winiarski
2018-03-19 9:53 ` [PATCH 08/13] drm/i915/guc: Always print log stats in i915_guc_info when using GuC Michał Winiarski
2018-03-19 9:53 ` [PATCH v3 09/13] drm/i915/guc: Don't print out relay statistics when relay is disabled Michał Winiarski
2018-03-19 9:53 ` [PATCH v3 10/13] drm/i915/guc: Allow user to control default GuC logging Michał Winiarski
2018-03-19 11:45 ` Sagar Arun Kamble [this message]
2018-03-19 9:53 ` [PATCH v2 11/13] drm/i915/guc: Default to non-verbose " Michał Winiarski
2018-03-19 9:53 ` [PATCH 12/13] drm/i915/guc: Demote GuC error messages Michał Winiarski
2018-03-19 11:49 ` Chris Wilson
2018-03-19 9:53 ` [PATCH 13/13] HAX enable guc for CI Michał Winiarski
2018-03-19 10:04 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [v3,01/13] drm/i915/guc: Keep GuC interrupts enabled when using GuC Patchwork
2018-03-19 10:20 ` ✓ Fi.CI.BAT: success " Patchwork
2018-03-19 11:08 ` ✗ Fi.CI.IGT: failure " Patchwork
2018-03-19 12:25 ` Chris Wilson
2018-03-19 11:17 ` [PATCH v3 01/13] " Sagar Arun Kamble
2018-03-19 12:24 ` Michal Wajdeczko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a887657b-cfed-5c2e-e75d-b7833ac99189@intel.com \
--to=sagar.a.kamble@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=michal.winiarski@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).