From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
To: "Michał Winiarski" <michal.winiarski@intel.com>,
intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 13/15] drm/i915/guc: Allow user to control default GuC logging
Date: Tue, 6 Mar 2018 17:28:39 +0530 [thread overview]
Message-ID: <f996685e-9f84-d1eb-d819-1761dd3c79c2@intel.com> (raw)
In-Reply-To: <20180227125230.13000-13-michal.winiarski@intel.com>
On 2/27/2018 6:22 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
>
> 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>
Change looks good.
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Firmware should have enabled default logging internally without user
controls or should have created new verbosity level.
This was supposed to be fixed in the firmware based on the discussion
here https://patchwork.freedesktop.org/patch/178281/
Until this gets fixed in firmware, this patch will be needed for
multiple log captures.
> ---
> drivers/gpu/drm/i915/intel_guc.c | 21 ++++++++++++---------
> drivers/gpu/drm/i915/intel_guc_fwif.h | 5 +++--
> drivers/gpu/drm/i915/intel_guc_log.c | 9 +++++----
> drivers/gpu/drm/i915/intel_guc_log.h | 11 +++++++++++
> drivers/gpu/drm/i915/intel_uc.c | 14 +++++++++-----
> 5 files changed, 40 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 0500b4164254..83d813a6ff92 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -221,17 +221,20 @@ 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_IS_ENABLED(level))
> + flags = GUC_LOG_DEFAULT_DISABLED | GUC_LOG_DISABLED;
> + else if (GUC_LOG_IS_VERBOSE(level))
> + 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 +269,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 bdf6b3178488..ade7dadc34b8 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -58,11 +58,13 @@ 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[] = {
> INTEL_GUC_ACTION_UK_LOG_ENABLE_LOGGING,
> @@ -506,8 +508,6 @@ int intel_guc_log_level_get(struct intel_guc *guc)
> return i915_modparams.guc_log_level;
> }
>
> -#define GUC_LOG_IS_ENABLED(x) (x > 0)
> -#define GUC_LOG_LEVEL_TO_VERBOSITY(x) (GUC_LOG_IS_ENABLED(x) ? x - 1 : 0)
> int intel_guc_log_level_set(struct intel_guc *guc, u64 val)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> @@ -533,7 +533,8 @@ int intel_guc_log_level_set(struct intel_guc *guc, u64 val)
> }
>
> intel_runtime_pm_get(dev_priv);
> - ret = guc_log_control(guc, GUC_LOG_IS_ENABLED(val),
> + ret = guc_log_control(guc, GUC_LOG_IS_VERBOSE(val),
> + GUC_LOG_IS_ENABLED(val),
> GUC_LOG_LEVEL_TO_VERBOSITY(val));
> intel_runtime_pm_put(dev_priv);
> if (ret)
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> index a8ccb04e2b0a..a91d94da5543 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -39,6 +39,17 @@ 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_IS_ENABLED(x) (x > 0)
> +#define GUC_LOG_IS_VERBOSE(x) (x > 1)
> +#define GUC_LOG_LEVEL_TO_VERBOSITY(x) (GUC_LOG_IS_VERBOSE(x) ? x - 2 : 0)
> +#define GUC_LOG_VERBOSITY_TO_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 27e1f4c43b7b..9884a79c77bd 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_LOG_VERBOSITY_TO_LEVEL(GUC_LOG_VERBOSITY_MAX);
>
> /* Any platform specific fine-tuning can be done here */
>
> @@ -142,17 +143,20 @@ void intel_uc_sanitize_options(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_LOG_VERBOSITY_TO_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_LOG_VERBOSITY_TO_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_IS_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-06 11:58 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-27 12:52 [PATCH 01/15] drm/i915/guc: Tidy guc_log_control Michał Winiarski
2018-02-27 12:52 ` [PATCH 02/15] drm/i915/guc: Create common entry points for log register/unregister Michał Winiarski
2018-02-27 14:11 ` Michal Wajdeczko
2018-03-05 7:09 ` Sagar Arun Kamble
2018-03-05 13:38 ` Michał Winiarski
2018-03-06 5:24 ` Sagar Arun Kamble
2018-02-27 12:52 ` [PATCH 03/15] drm/i915/guc: Move GuC notification handling to separate function Michał Winiarski
2018-03-05 7:40 ` Sagar Arun Kamble
2018-02-27 12:52 ` [PATCH 04/15] drm/i915/guc: Keep GuC interrupts enabled when using GuC Michał Winiarski
2018-03-05 9:27 ` Sagar Arun Kamble
2018-02-27 12:52 ` [PATCH 05/15] drm/i915/guc: Log runtime should consist of both mapping and relay Michał Winiarski
2018-02-27 14:21 ` Michal Wajdeczko
2018-03-05 10:31 ` Sagar Arun Kamble
2018-03-05 14:14 ` Michał Winiarski
2018-03-06 5:54 ` Sagar Arun Kamble
2018-03-06 13:50 ` Chris Wilson
2018-02-27 12:52 ` [PATCH 06/15] drm/i915/guc: Merge log relay file and channel creation Michał Winiarski
2018-03-05 11:44 ` Sagar Arun Kamble
2018-02-27 12:52 ` [PATCH 07/15] drm/i915/guc: Flush directly in log unregister Michał Winiarski
2018-03-05 11:58 ` Sagar Arun Kamble
2018-03-05 12:10 ` Michał Winiarski
2018-03-06 4:59 ` Sagar Arun Kamble
2018-02-27 12:52 ` [PATCH 08/15] drm/i915/guc: Split relay control and GuC log level Michał Winiarski
2018-03-06 7:16 ` Sagar Arun Kamble
2018-02-27 12:52 ` [PATCH 09/15] drm/i915/guc: Move check for fast memcpy_wc to relay creation Michał Winiarski
2018-02-27 14:37 ` Michal Wajdeczko
2018-02-27 15:37 ` Michał Winiarski
2018-03-06 14:00 ` Chris Wilson
2018-03-06 7:23 ` Sagar Arun Kamble
2018-02-27 12:52 ` [PATCH 10/15] drm/i915/guc: Get rid of GuC log runtime Michał Winiarski
2018-03-06 9:30 ` Sagar Arun Kamble
2018-03-06 14:02 ` Chris Wilson
2018-02-27 12:52 ` [PATCH 11/15] drm/i915/guc: Always print log stats in i915_guc_info when using GuC Michał Winiarski
2018-03-06 7:41 ` Sagar Arun Kamble
2018-02-27 12:52 ` [PATCH 12/15] drm/i915/guc: Don't print out relay statistics when relay is disabled Michał Winiarski
2018-03-06 11:21 ` Sagar Arun Kamble
2018-03-06 12:00 ` Michał Winiarski
2018-03-06 13:16 ` Sagar Arun Kamble
2018-02-27 12:52 ` [PATCH 13/15] drm/i915/guc: Allow user to control default GuC logging Michał Winiarski
2018-03-06 11:58 ` Sagar Arun Kamble [this message]
2018-02-27 12:52 ` [PATCH 14/15] drm/i915/guc: Default to non-verbose " Michał Winiarski
2018-03-06 13:44 ` Sagar Arun Kamble
2018-02-27 12:52 ` [PATCH 15/15] HAX enable guc and guc_log for CI Michał Winiarski
2018-02-27 13:38 ` ✗ Fi.CI.SPARSE: warning for series starting with [01/15] drm/i915/guc: Tidy guc_log_control Patchwork
2018-02-27 13:47 ` ✓ Fi.CI.BAT: success " Patchwork
2018-02-27 13:49 ` [PATCH 01/15] " Michal Wajdeczko
2018-02-27 20:10 ` ✗ Fi.CI.IGT: failure for series starting with [01/15] " Patchwork
2018-03-02 11:09 ` [PATCH 01/15] " Sagar Arun Kamble
2018-03-02 11:52 ` Michał Winiarski
2018-03-05 5:29 ` Sagar Arun Kamble
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=f996685e-9f84-d1eb-d819-1761dd3c79c2@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).