* [PATCH] drm/i915/guc: Redefine guc_log_level modparam values
@ 2018-01-10 16:05 Michal Wajdeczko
2018-01-10 16:26 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-01-11 5:52 ` [PATCH] " Sagar Arun Kamble
0 siblings, 2 replies; 5+ messages in thread
From: Michal Wajdeczko @ 2018-01-10 16:05 UTC (permalink / raw)
To: intel-gfx
We used value -1 to indicate "disabled" and values 0..3 to
indicate "enabled", but most of our other modparams are using
-1 for "auto" mode and 0 for "disable". For consistency let's
change our log level values to:
-1: auto (depends on USES_GUC and DRM_I915_DEBUG)
0: disabled
1: enabled (severity level 0 = min)
2: enabled (severity level 1)
3: enabled (severity level 2)
4: enabled (severity level 3 = max)
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_params.c | 3 ++-
drivers/gpu/drm/i915/intel_guc.c | 21 ++++++++++-----
drivers/gpu/drm/i915/intel_guc_log.c | 47 ++++++++++++++++-----------------
drivers/gpu/drm/i915/intel_uc.c | 51 ++++++++++++++++++++++++++++++++++--
4 files changed, 87 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b5f3eb4..0b553a8 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = {
"(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
i915_param_named(guc_log_level, int, 0400,
- "GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
+ "GuC firmware logging level. Requires GuC to be loaded. "
+ "(-1=auto [default], 0=disable, 1..4=enable with verbosity min..max)");
i915_param_named_unsafe(guc_firmware_path, charp, 0400,
"GuC firmware path to use instead of the default one");
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 50b4725..2227236 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -215,6 +215,18 @@ static u32 get_core_family(struct drm_i915_private *dev_priv)
}
}
+static u32 get_log_verbosity_flags(void)
+{
+ if (i915_modparams.guc_log_level > 0) {
+ u32 verbosity = i915_modparams.guc_log_level - 1;
+
+ GEM_BUG_ON(verbosity > GUC_LOG_VERBOSITY_MAX);
+ return verbosity << GUC_LOG_VERBOSITY_SHIFT;
+ }
+ GEM_BUG_ON(i915_modparams.enable_guc < 0);
+ return GUC_LOG_DISABLED;
+}
+
/*
* Initialise the GuC parameter block before starting the firmware
* transfer. These parameters are read by the firmware on startup
@@ -247,12 +259,7 @@ void intel_guc_init_params(struct intel_guc *guc)
params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
- if (i915_modparams.guc_log_level >= 0) {
- params[GUC_CTL_DEBUG] =
- i915_modparams.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
- } else {
- params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
- }
+ params[GUC_CTL_DEBUG] = get_log_verbosity_flags();
/* If GuC submission is enabled, set up additional parameters here */
if (USES_GUC_SUBMISSION(dev_priv)) {
@@ -445,7 +452,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
return 0;
- if (i915_modparams.guc_log_level >= 0)
+ if (i915_modparams.guc_log_level > 0)
gen9_enable_guc_interrupts(dev_priv);
data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index eaedd63..e6d59bf 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -33,11 +33,10 @@
/**
* DOC: GuC firmware log
*
- * Firmware log is enabled by setting i915.guc_log_level to non-negative level.
+ * Firmware log is enabled by setting i915.guc_log_level to the positive level.
* Log data is printed out via reading debugfs i915_guc_log_dump. Reading from
* i915_guc_load_status will print out firmware loading status and scratch
* registers value.
- *
*/
static int guc_log_flush_complete(struct intel_guc *guc)
@@ -147,7 +146,7 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
struct dentry *log_dir;
int ret;
- if (i915_modparams.guc_log_level < 0)
+ if (!i915_modparams.guc_log_level)
return 0;
/* For now create the log file in /sys/kernel/debug/dri/0 dir */
@@ -423,8 +422,8 @@ static void guc_log_runtime_destroy(struct intel_guc *guc)
{
/*
* It's possible that the runtime stuff was never allocated because
- * guc_log_level was < 0 at the time
- **/
+ * GuC log was disabled at the boot time.
+ */
if (!guc_log_has_runtime(guc))
return;
@@ -441,9 +440,10 @@ static int guc_log_late_setup(struct intel_guc *guc)
lockdep_assert_held(&dev_priv->drm.struct_mutex);
if (!guc_log_has_runtime(guc)) {
- /* If log_level was set as -1 at boot time, then setup needed to
- * handle log buffer flush interrupts would not have been done yet,
- * so do that now.
+ /*
+ * If log was disabled at boot time, then setup needed to handle
+ * log buffer flush interrupts would not have been done yet, so
+ * do that now.
*/
ret = guc_log_runtime_create(guc);
if (ret)
@@ -460,7 +460,7 @@ static int guc_log_late_setup(struct intel_guc *guc)
guc_log_runtime_destroy(guc);
err:
/* logging will remain off */
- i915_modparams.guc_log_level = -1;
+ i915_modparams.guc_log_level = 0;
return ret;
}
@@ -482,8 +482,7 @@ static void guc_flush_logs(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
- if (!USES_GUC_SUBMISSION(dev_priv) ||
- (i915_modparams.guc_log_level < 0))
+ if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level)
return;
/* First disable the interrupts, will be renabled afterwards */
@@ -511,9 +510,6 @@ int intel_guc_log_create(struct intel_guc *guc)
GEM_BUG_ON(guc->log.vma);
- if (i915_modparams.guc_log_level > GUC_LOG_VERBOSITY_MAX)
- i915_modparams.guc_log_level = GUC_LOG_VERBOSITY_MAX;
-
/* The first page is to save log buffer state. Allocate one
* extra page for others in case for overlap */
size = (1 + GUC_LOG_DPC_PAGES + 1 +
@@ -537,7 +533,7 @@ int intel_guc_log_create(struct intel_guc *guc)
guc->log.vma = vma;
- if (i915_modparams.guc_log_level >= 0) {
+ if (i915_modparams.guc_log_level) {
ret = guc_log_runtime_create(guc);
if (ret < 0)
goto err_vma;
@@ -558,7 +554,7 @@ int intel_guc_log_create(struct intel_guc *guc)
i915_vma_unpin_and_release(&guc->log.vma);
err:
/* logging will be off */
- i915_modparams.guc_log_level = -1;
+ i915_modparams.guc_log_level = 0;
return ret;
}
@@ -582,7 +578,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
return -EINVAL;
/* This combination doesn't make sense & won't have any effect */
- if (!log_param.logging_enabled && (i915_modparams.guc_log_level < 0))
+ if (!log_param.logging_enabled && !i915_modparams.guc_log_level)
return 0;
ret = guc_log_control(guc, log_param.value);
@@ -592,11 +588,12 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
}
if (log_param.logging_enabled) {
- i915_modparams.guc_log_level = log_param.verbosity;
+ i915_modparams.guc_log_level = 1 + log_param.verbosity;
- /* If log_level was set as -1 at boot time, then the relay channel file
- * wouldn't have been created by now and interrupts also would not have
- * been enabled. Try again now, just in case.
+ /*
+ * If log was disabled at boot time, then the relay channel file
+ * wouldn't have been created by now and interrupts also would
+ * not have been enabled. Try again now, just in case.
*/
ret = guc_log_late_setup(guc);
if (ret < 0) {
@@ -607,7 +604,8 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
/* GuC logging is currently the only user of Guc2Host interrupts */
gen9_enable_guc_interrupts(dev_priv);
} else {
- /* Once logging is disabled, GuC won't generate logs & send an
+ /*
+ * Once logging is disabled, GuC won't generate logs & send an
* interrupt. But there could be some data in the log buffer
* which is yet to be captured. So request GuC to update the log
* buffer state and then collect the left over logs.
@@ -615,7 +613,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
guc_flush_logs(guc);
/* As logging is disabled, update log level to reflect that */
- i915_modparams.guc_log_level = -1;
+ i915_modparams.guc_log_level = 0;
}
return ret;
@@ -623,8 +621,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
void i915_guc_log_register(struct drm_i915_private *dev_priv)
{
- if (!USES_GUC_SUBMISSION(dev_priv) ||
- (i915_modparams.guc_log_level < 0))
+ if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level)
return;
mutex_lock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index d82ca0f..fd39c06 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -65,6 +65,21 @@ static int __get_platform_enable_guc(struct drm_i915_private *dev_priv)
return enable_guc;
}
+static int __get_default_guc_log_level(struct drm_i915_private *dev_priv)
+{
+ int guc_log_level = 0; /* disabled */
+
+ /* 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 = 1 + GUC_LOG_VERBOSITY_MAX;
+
+ /* Any platform specific fine-tuning can be done here */
+
+ return guc_log_level;
+}
+
/**
* intel_uc_sanitize_options - sanitize uC related modparam options
* @dev_priv: device private
@@ -74,6 +89,13 @@ static int __get_platform_enable_guc(struct drm_i915_private *dev_priv)
* modparam varies between platforms and it is hardcoded in driver code.
* Any other modparam value is only monitored against availability of the
* related hardware or firmware definitions.
+ *
+ * In case of "guc_log_level" option this function will attempt to modify
+ * it only if it was initially set to "auto(-1)" or if initial value was
+ * "enable(1..4)" on platforms without the GuC. Default value for this
+ * modparam varies between platforms and is usually set to "disable(0)"
+ * unless GuC is enabled on given platform and the driver is compiled with
+ * debug config when this modparam will default to "enable(1..4)".
*/
void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
{
@@ -105,8 +127,33 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
"no HuC firmware");
}
+ /* A negative value means "use platform/config default" */
+ if (i915_modparams.guc_log_level < 0)
+ i915_modparams.guc_log_level =
+ __get_default_guc_log_level(dev_priv);
+
+ DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s verbosity:%d)\n",
+ i915_modparams.guc_log_level,
+ yesno(i915_modparams.guc_log_level > 0),
+ i915_modparams.guc_log_level - 1);
+
+ if (i915_modparams.guc_log_level > 0 && !intel_uc_is_using_guc()) {
+ DRM_WARN("Incompatible option ignored: guc_log_level=%d, %s!\n",
+ i915_modparams.guc_log_level,
+ !HAS_GUC(dev_priv) ? "no GuC hardware" :
+ "GuC not enabled");
+ i915_modparams.guc_log_level = 0;
+ }
+
+ if (i915_modparams.guc_log_level > 1 + GUC_LOG_VERBOSITY_MAX) {
+ DRM_WARN("Incompatible option ignored: guc_log_level=%d, %s!\n",
+ i915_modparams.guc_log_level, "verbosity too high");
+ i915_modparams.guc_log_level = 1 + GUC_LOG_VERBOSITY_MAX;
+ }
+
/* Make sure that sanitization was done */
GEM_BUG_ON(i915_modparams.enable_guc < 0);
+ GEM_BUG_ON(i915_modparams.guc_log_level < 0);
}
void intel_uc_init_early(struct drm_i915_private *dev_priv)
@@ -152,7 +199,7 @@ void intel_uc_init_mmio(struct drm_i915_private *dev_priv)
static void guc_capture_load_err_log(struct intel_guc *guc)
{
- if (!guc->log.vma || i915_modparams.guc_log_level < 0)
+ if (!guc->log.vma || !i915_modparams.guc_log_level)
return;
if (!guc->load_err_log)
@@ -322,7 +369,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
}
if (USES_GUC_SUBMISSION(dev_priv)) {
- if (i915_modparams.guc_log_level >= 0)
+ if (i915_modparams.guc_log_level)
gen9_enable_guc_interrupts(dev_priv);
ret = intel_guc_submission_enable(guc);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/guc: Redefine guc_log_level modparam values
2018-01-10 16:05 [PATCH] drm/i915/guc: Redefine guc_log_level modparam values Michal Wajdeczko
@ 2018-01-10 16:26 ` Patchwork
2018-01-11 5:52 ` [PATCH] " Sagar Arun Kamble
1 sibling, 0 replies; 5+ messages in thread
From: Patchwork @ 2018-01-10 16:26 UTC (permalink / raw)
To: Michal Wajdeczko; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/guc: Redefine guc_log_level modparam values
URL : https://patchwork.freedesktop.org/series/36281/
State : success
== Summary ==
Series 36281v1 drm/i915/guc: Redefine guc_log_level modparam values
https://patchwork.freedesktop.org/api/1.0/series/36281/revisions/1/mbox/
Test debugfs_test:
Subgroup read_all_entries:
dmesg-fail -> DMESG-WARN (fi-elk-e7500) fdo#103989
Test gem_exec_fence:
Subgroup await-hang-default:
dmesg-fail -> PASS (fi-pnv-d510) fdo#104572
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
fail -> PASS (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-a:
pass -> DMESG-WARN (fi-kbl-r) fdo#104172 +1
Subgroup suspend-read-crc-pipe-c:
pass -> INCOMPLETE (fi-bdw-5557u) fdo#104162
fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#104572 https://bugs.freedesktop.org/show_bug.cgi?id=104572
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#104172 https://bugs.freedesktop.org/show_bug.cgi?id=104172
fdo#104162 https://bugs.freedesktop.org/show_bug.cgi?id=104162
fi-bdw-5557u total:246 pass:228 dwarn:0 dfail:0 fail:0 skip:17
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:425s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:372s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:491s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:280s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:490s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:486s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:467s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:459s
fi-elk-e7500 total:224 pass:168 dwarn:10 dfail:0 fail:0 skip:45
fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:274s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:512s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:392s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:409s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:413s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:451s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:412s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:469s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:502s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:453s
fi-kbl-r total:288 pass:260 dwarn:1 dfail:0 fail:0 skip:27 time:505s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:578s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:425s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:508s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:529s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:494s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:494s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:436s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:531s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:399s
Blacklisted hosts:
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:563s
fi-glk-dsi total:21 pass:20 dwarn:0 dfail:0 fail:0 skip:0
4269b88bf52fc7a991d05ca75d1b5f86052b8eab drm-tip: 2018y-01m-10d-14h-03m-47s UTC integration manifest
780cb4f3bc13 drm/i915/guc: Redefine guc_log_level modparam values
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7644/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/guc: Redefine guc_log_level modparam values
2018-01-10 16:05 [PATCH] drm/i915/guc: Redefine guc_log_level modparam values Michal Wajdeczko
2018-01-10 16:26 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-01-11 5:52 ` Sagar Arun Kamble
2018-01-11 9:24 ` Michal Wajdeczko
1 sibling, 1 reply; 5+ messages in thread
From: Sagar Arun Kamble @ 2018-01-11 5:52 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 1/10/2018 9:35 PM, Michal Wajdeczko wrote:
> We used value -1 to indicate "disabled" and values 0..3 to
> indicate "enabled", but most of our other modparams are using
> -1 for "auto" mode and 0 for "disable". For consistency let's
> change our log level values to:
>
> -1: auto (depends on USES_GUC and DRM_I915_DEBUG)
"depends on HAS_GUC, USES_GUC, DRM_I915_DEBUG and DRM_I915_DEBUG_GEM"
s/intel_uc_is_using_guc()/USES_GUC
seeing some more intel_uc_is_using_* instead of macro usage USES_*
> 0: disabled
> 1: enabled (severity level 0 = min)
> 2: enabled (severity level 1)
> 3: enabled (severity level 2)
> 4: enabled (severity level 3 = max)
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_params.c | 3 ++-
> drivers/gpu/drm/i915/intel_guc.c | 21 ++++++++++-----
> drivers/gpu/drm/i915/intel_guc_log.c | 47 ++++++++++++++++-----------------
> drivers/gpu/drm/i915/intel_uc.c | 51 ++++++++++++++++++++++++++++++++++--
> 4 files changed, 87 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index b5f3eb4..0b553a8 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = {
> "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
>
> i915_param_named(guc_log_level, int, 0400,
> - "GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
> + "GuC firmware logging level. Requires GuC to be loaded. "
> + "(-1=auto [default], 0=disable, 1..4=enable with verbosity min..max)");
>
> i915_param_named_unsafe(guc_firmware_path, charp, 0400,
> "GuC firmware path to use instead of the default one");
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index 50b4725..2227236 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -215,6 +215,18 @@ static u32 get_core_family(struct drm_i915_private *dev_priv)
> }
> }
>
> +static u32 get_log_verbosity_flags(void)
> +{
making this inline would be good i guess.
> + if (i915_modparams.guc_log_level > 0) {
> + u32 verbosity = i915_modparams.guc_log_level - 1;
> +
> + GEM_BUG_ON(verbosity > GUC_LOG_VERBOSITY_MAX);
> + return verbosity << GUC_LOG_VERBOSITY_SHIFT;
> + }
> + GEM_BUG_ON(i915_modparams.enable_guc < 0);
> + return GUC_LOG_DISABLED;
> +}
> +
> /*
> * Initialise the GuC parameter block before starting the firmware
> * transfer. These parameters are read by the firmware on startup
> @@ -247,12 +259,7 @@ void intel_guc_init_params(struct intel_guc *guc)
>
> params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
>
> - if (i915_modparams.guc_log_level >= 0) {
> - params[GUC_CTL_DEBUG] =
> - i915_modparams.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
> - } else {
> - params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
> - }
> + params[GUC_CTL_DEBUG] = get_log_verbosity_flags();
>
> /* If GuC submission is enabled, set up additional parameters here */
> if (USES_GUC_SUBMISSION(dev_priv)) {
> @@ -445,7 +452,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
> if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> return 0;
>
> - if (i915_modparams.guc_log_level >= 0)
> + if (i915_modparams.guc_log_level > 0)
if (i915_modparams.guc_log_level)
> gen9_enable_guc_interrupts(dev_priv);
>
> data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index eaedd63..e6d59bf 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
<snip>
>
> @@ -582,7 +578,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
> return -EINVAL;
>
> /* This combination doesn't make sense & won't have any effect */
> - if (!log_param.logging_enabled && (i915_modparams.guc_log_level < 0))
> + if (!log_param.logging_enabled && !i915_modparams.guc_log_level)
> return 0;
>
> ret = guc_log_control(guc, log_param.value);
> @@ -592,11 +588,12 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
> }
>
> if (log_param.logging_enabled) {
> - i915_modparams.guc_log_level = log_param.verbosity;
> + i915_modparams.guc_log_level = 1 + log_param.verbosity;
reading the log level from i915_guc_log_control_get debugfs should
decrement guc_log_level by 1.
>
> - /* If log_level was set as -1 at boot time, then the relay channel file
> - * wouldn't have been created by now and interrupts also would not have
> - * been enabled. Try again now, just in case.
> + /*
> + * If log was disabled at boot time, then the relay channel file
> + * wouldn't have been created by now and interrupts also would
> + * not have been enabled. Try again now, just in case.
> */
> ret = guc_log_late_setup(guc);
> if (ret < 0) {
> @@ -607,7 +604,8 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
> /* GuC logging is currently the only user of Guc2Host interrupts */
> gen9_enable_guc_interrupts(dev_priv);
> } else {
> - /* Once logging is disabled, GuC won't generate logs & send an
> + /*
> + * Once logging is disabled, GuC won't generate logs & send an
> * interrupt. But there could be some data in the log buffer
> * which is yet to be captured. So request GuC to update the log
> * buffer state and then collect the left over logs.
> @@ -615,7 +613,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
> guc_flush_logs(guc);
>
> /* As logging is disabled, update log level to reflect that */
> - i915_modparams.guc_log_level = -1;
> + i915_modparams.guc_log_level = 0;
> }
>
> return ret;
> @@ -623,8 +621,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>
> void i915_guc_log_register(struct drm_i915_private *dev_priv)
> {
> - if (!USES_GUC_SUBMISSION(dev_priv) ||
> - (i915_modparams.guc_log_level < 0))
> + if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level)
> return;
>
> mutex_lock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index d82ca0f..fd39c06 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -65,6 +65,21 @@ static int __get_platform_enable_guc(struct drm_i915_private *dev_priv)
> return enable_guc;
> }
>
> +static int __get_default_guc_log_level(struct drm_i915_private *dev_priv)
> +{
> + int guc_log_level = 0; /* disabled */
> +
> + /* 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 = 1 + GUC_LOG_VERBOSITY_MAX;
> +
> + /* Any platform specific fine-tuning can be done here */
> +
> + return guc_log_level;
> +}
> +
> /**
> * intel_uc_sanitize_options - sanitize uC related modparam options
> * @dev_priv: device private
> @@ -74,6 +89,13 @@ static int __get_platform_enable_guc(struct drm_i915_private *dev_priv)
> * modparam varies between platforms and it is hardcoded in driver code.
> * Any other modparam value is only monitored against availability of the
> * related hardware or firmware definitions.
> + *
> + * In case of "guc_log_level" option this function will attempt to modify
> + * it only if it was initially set to "auto(-1)" or if initial value was
> + * "enable(1..4)" on platforms without the GuC. Default value for this
> + * modparam varies between platforms and is usually set to "disable(0)"
> + * unless GuC is enabled on given platform and the driver is compiled with
> + * debug config when this modparam will default to "enable(1..4)".
> */
> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
> {
> @@ -105,8 +127,33 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
> "no HuC firmware");
> }
>
> + /* A negative value means "use platform/config default" */
> + if (i915_modparams.guc_log_level < 0)
> + i915_modparams.guc_log_level =
> + __get_default_guc_log_level(dev_priv);
> +
> + DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s verbosity:%d)\n",
> + i915_modparams.guc_log_level,
> + yesno(i915_modparams.guc_log_level > 0),
> + i915_modparams.guc_log_level - 1);
> +
I think this DRM_DEBUG_DRIVER should be moved after all sanitization.
> + if (i915_modparams.guc_log_level > 0 && !intel_uc_is_using_guc()) {
> + DRM_WARN("Incompatible option ignored: guc_log_level=%d, %s!\n",
> + i915_modparams.guc_log_level,
> + !HAS_GUC(dev_priv) ? "no GuC hardware" :
> + "GuC not enabled");
> + i915_modparams.guc_log_level = 0;
> + }
> +
> + if (i915_modparams.guc_log_level > 1 + GUC_LOG_VERBOSITY_MAX) {
> + DRM_WARN("Incompatible option ignored: guc_log_level=%d, %s!\n",
"Incompatible option overridden"? as we are letting this through with
max verbosity which I think is not ignoring :)
> + i915_modparams.guc_log_level, "verbosity too high");
> + i915_modparams.guc_log_level = 1 + GUC_LOG_VERBOSITY_MAX;
> + }
> +
> /* Make sure that sanitization was done */
> GEM_BUG_ON(i915_modparams.enable_guc < 0);
> + GEM_BUG_ON(i915_modparams.guc_log_level < 0);
> }
>
> void intel_uc_init_early(struct drm_i915_private *dev_priv)
> @@ -152,7 +199,7 @@ void intel_uc_init_mmio(struct drm_i915_private *dev_priv)
>
> static void guc_capture_load_err_log(struct intel_guc *guc)
> {
> - if (!guc->log.vma || i915_modparams.guc_log_level < 0)
> + if (!guc->log.vma || !i915_modparams.guc_log_level)
> return;
>
> if (!guc->load_err_log)
> @@ -322,7 +369,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> }
>
> if (USES_GUC_SUBMISSION(dev_priv)) {
> - if (i915_modparams.guc_log_level >= 0)
> + if (i915_modparams.guc_log_level)
> gen9_enable_guc_interrupts(dev_priv);
>
> ret = intel_guc_submission_enable(guc);
Thanks
Sagar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/guc: Redefine guc_log_level modparam values
2018-01-11 5:52 ` [PATCH] " Sagar Arun Kamble
@ 2018-01-11 9:24 ` Michal Wajdeczko
2018-01-11 9:38 ` Sagar Arun Kamble
0 siblings, 1 reply; 5+ messages in thread
From: Michal Wajdeczko @ 2018-01-11 9:24 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Thu, 11 Jan 2018 06:52:18 +0100, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
>
>
> On 1/10/2018 9:35 PM, Michal Wajdeczko wrote:
>> We used value -1 to indicate "disabled" and values 0..3 to
>> indicate "enabled", but most of our other modparams are using
>> -1 for "auto" mode and 0 for "disable". For consistency let's
>> change our log level values to:
>>
>> -1: auto (depends on USES_GUC and DRM_I915_DEBUG)
> "depends on HAS_GUC, USES_GUC, DRM_I915_DEBUG and DRM_I915_DEBUG_GEM"
I should write "(depends on platform and Kconfig.debug settings)" as
actual condition may change in the near feature.
> s/intel_uc_is_using_guc()/USES_GUC
> seeing some more intel_uc_is_using_* instead of macro usage USES_*
It was by design, as in intel_uc_sanitize_options() function we are
using only HAS_xxx macros and instead of USES_xxx we call intel_uc_is_xxx
helpers directly.
>> 0: disabled
>> 1: enabled (severity level 0 = min)
>> 2: enabled (severity level 1)
>> 3: enabled (severity level 2)
>> 4: enabled (severity level 3 = max)
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>> drivers/gpu/drm/i915/i915_params.c | 3 ++-
>> drivers/gpu/drm/i915/intel_guc.c | 21 ++++++++++-----
>> drivers/gpu/drm/i915/intel_guc_log.c | 47
>> ++++++++++++++++-----------------
>> drivers/gpu/drm/i915/intel_uc.c | 51
>> ++++++++++++++++++++++++++++++++++--
>> 4 files changed, 87 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c
>> b/drivers/gpu/drm/i915/i915_params.c
>> index b5f3eb4..0b553a8 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = {
>> "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
>> i915_param_named(guc_log_level, int, 0400,
>> - "GuC firmware logging level (-1:disabled (default), 0-3:enabled)");
>> + "GuC firmware logging level. Requires GuC to be loaded. "
>> + "(-1=auto [default], 0=disable, 1..4=enable with verbosity
>> min..max)");
>> i915_param_named_unsafe(guc_firmware_path, charp, 0400,
>> "GuC firmware path to use instead of the default one");
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 50b4725..2227236 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -215,6 +215,18 @@ static u32 get_core_family(struct drm_i915_private
>> *dev_priv)
>> }
>> }
>> +static u32 get_log_verbosity_flags(void)
>> +{
> making this inline would be good i guess.
No need to do so. Compiler will take care of it as needed (as this
function is already marked as static)
>> + if (i915_modparams.guc_log_level > 0) {
>> + u32 verbosity = i915_modparams.guc_log_level - 1;
>> +
>> + GEM_BUG_ON(verbosity > GUC_LOG_VERBOSITY_MAX);
>> + return verbosity << GUC_LOG_VERBOSITY_SHIFT;
>> + }
>> + GEM_BUG_ON(i915_modparams.enable_guc < 0);
>> + return GUC_LOG_DISABLED;
>> +}
>> +
>> /*
>> * Initialise the GuC parameter block before starting the firmware
>> * transfer. These parameters are read by the firmware on startup
>> @@ -247,12 +259,7 @@ void intel_guc_init_params(struct intel_guc *guc)
>> params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
>> - if (i915_modparams.guc_log_level >= 0) {
>> - params[GUC_CTL_DEBUG] =
>> - i915_modparams.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
>> - } else {
>> - params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
>> - }
>> + params[GUC_CTL_DEBUG] = get_log_verbosity_flags();
>> /* If GuC submission is enabled, set up additional parameters here
>> */
>> if (USES_GUC_SUBMISSION(dev_priv)) {
>> @@ -445,7 +452,7 @@ int intel_guc_resume(struct drm_i915_private
>> *dev_priv)
>> if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> return 0;
>> - if (i915_modparams.guc_log_level >= 0)
>> + if (i915_modparams.guc_log_level > 0)
> if (i915_modparams.guc_log_level)
>> gen9_enable_guc_interrupts(dev_priv);
>> data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index eaedd63..e6d59bf 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> <snip>
>> @@ -582,7 +578,7 @@ int i915_guc_log_control(struct drm_i915_private
>> *dev_priv, u64 control_val)
>> return -EINVAL;
>> /* This combination doesn't make sense & won't have any effect */
>> - if (!log_param.logging_enabled && (i915_modparams.guc_log_level < 0))
>> + if (!log_param.logging_enabled && !i915_modparams.guc_log_level)
>> return 0;
>> ret = guc_log_control(guc, log_param.value);
>> @@ -592,11 +588,12 @@ int i915_guc_log_control(struct drm_i915_private
>> *dev_priv, u64 control_val)
>> }
>> if (log_param.logging_enabled) {
>> - i915_modparams.guc_log_level = log_param.verbosity;
>> + i915_modparams.guc_log_level = 1 + log_param.verbosity;
> reading the log level from i915_guc_log_control_get debugfs should
> decrement guc_log_level by 1.
That's the other story.
If I read our code right, we were using different format for get:
(0=level 0, 1=level 1, ... 3=level 3)
and set:
(0=disabled, 1=enabled level 0, 9=enabled level 1, 11=enabled level 2...
I think we can change that to use always (0, 1..4) and hide internal
layout of the GuC command from the user.
>> - /* If log_level was set as -1 at boot time, then the relay channel
>> file
>> - * wouldn't have been created by now and interrupts also would not
>> have
>> - * been enabled. Try again now, just in case.
>> + /*
>> + * If log was disabled at boot time, then the relay channel file
>> + * wouldn't have been created by now and interrupts also would
>> + * not have been enabled. Try again now, just in case.
>> */
>> ret = guc_log_late_setup(guc);
>> if (ret < 0) {
>> @@ -607,7 +604,8 @@ int i915_guc_log_control(struct drm_i915_private
>> *dev_priv, u64 control_val)
>> /* GuC logging is currently the only user of Guc2Host interrupts */
>> gen9_enable_guc_interrupts(dev_priv);
>> } else {
>> - /* Once logging is disabled, GuC won't generate logs & send an
>> + /*
>> + * Once logging is disabled, GuC won't generate logs & send an
>> * interrupt. But there could be some data in the log buffer
>> * which is yet to be captured. So request GuC to update the log
>> * buffer state and then collect the left over logs.
>> @@ -615,7 +613,7 @@ int i915_guc_log_control(struct drm_i915_private
>> *dev_priv, u64 control_val)
>> guc_flush_logs(guc);
>> /* As logging is disabled, update log level to reflect that */
>> - i915_modparams.guc_log_level = -1;
>> + i915_modparams.guc_log_level = 0;
>> }
>> return ret;
>> @@ -623,8 +621,7 @@ int i915_guc_log_control(struct drm_i915_private
>> *dev_priv, u64 control_val)
>> void i915_guc_log_register(struct drm_i915_private *dev_priv)
>> {
>> - if (!USES_GUC_SUBMISSION(dev_priv) ||
>> - (i915_modparams.guc_log_level < 0))
>> + if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level)
>> return;
>> mutex_lock(&dev_priv->drm.struct_mutex);
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index d82ca0f..fd39c06 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -65,6 +65,21 @@ static int __get_platform_enable_guc(struct
>> drm_i915_private *dev_priv)
>> return enable_guc;
>> }
>> +static int __get_default_guc_log_level(struct drm_i915_private
>> *dev_priv)
>> +{
>> + int guc_log_level = 0; /* disabled */
>> +
>> + /* 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 = 1 + GUC_LOG_VERBOSITY_MAX;
>> +
>> + /* Any platform specific fine-tuning can be done here */
>> +
>> + return guc_log_level;
>> +}
>> +
>> /**
>> * intel_uc_sanitize_options - sanitize uC related modparam options
>> * @dev_priv: device private
>> @@ -74,6 +89,13 @@ static int __get_platform_enable_guc(struct
>> drm_i915_private *dev_priv)
>> * modparam varies between platforms and it is hardcoded in driver
>> code.
>> * Any other modparam value is only monitored against availability of
>> the
>> * related hardware or firmware definitions.
>> + *
>> + * In case of "guc_log_level" option this function will attempt to
>> modify
>> + * it only if it was initially set to "auto(-1)" or if initial value
>> was
>> + * "enable(1..4)" on platforms without the GuC. Default value for this
>> + * modparam varies between platforms and is usually set to "disable(0)"
>> + * unless GuC is enabled on given platform and the driver is compiled
>> with
>> + * debug config when this modparam will default to "enable(1..4)".
>> */
>> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>> {
>> @@ -105,8 +127,33 @@ void intel_uc_sanitize_options(struct
>> drm_i915_private *dev_priv)
>> "no HuC firmware");
>> }
>> + /* A negative value means "use platform/config default" */
>> + if (i915_modparams.guc_log_level < 0)
>> + i915_modparams.guc_log_level =
>> + __get_default_guc_log_level(dev_priv);
>> +
>> + DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s verbosity:%d)\n",
>> + i915_modparams.guc_log_level,
>> + yesno(i915_modparams.guc_log_level > 0),
>> + i915_modparams.guc_log_level - 1);
>> +
> I think this DRM_DEBUG_DRIVER should be moved after all sanitization.
Yes. I copied that from above enable_guc sanitization but forget that
in above code there is no override :)
>> + if (i915_modparams.guc_log_level > 0 && !intel_uc_is_using_guc()) {
>> + DRM_WARN("Incompatible option ignored: guc_log_level=%d, %s!\n",
>> + i915_modparams.guc_log_level,
>> + !HAS_GUC(dev_priv) ? "no GuC hardware" :
>> + "GuC not enabled");
>> + i915_modparams.guc_log_level = 0;
>> + }
>> +
>> + if (i915_modparams.guc_log_level > 1 + GUC_LOG_VERBOSITY_MAX) {
>> + DRM_WARN("Incompatible option ignored: guc_log_level=%d, %s!\n",
> "Incompatible option overridden"? as we are letting this through with
> max verbosity which I think is not ignoring :)
sure
>> + i915_modparams.guc_log_level, "verbosity too high");
>> + i915_modparams.guc_log_level = 1 + GUC_LOG_VERBOSITY_MAX;
>> + }
>> +
>> /* Make sure that sanitization was done */
>> GEM_BUG_ON(i915_modparams.enable_guc < 0);
>> + GEM_BUG_ON(i915_modparams.guc_log_level < 0);
>> }
>> void intel_uc_init_early(struct drm_i915_private *dev_priv)
>> @@ -152,7 +199,7 @@ void intel_uc_init_mmio(struct drm_i915_private
>> *dev_priv)
>> static void guc_capture_load_err_log(struct intel_guc *guc)
>> {
>> - if (!guc->log.vma || i915_modparams.guc_log_level < 0)
>> + if (!guc->log.vma || !i915_modparams.guc_log_level)
>> return;
>> if (!guc->load_err_log)
>> @@ -322,7 +369,7 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> }
>> if (USES_GUC_SUBMISSION(dev_priv)) {
>> - if (i915_modparams.guc_log_level >= 0)
>> + if (i915_modparams.guc_log_level)
>> gen9_enable_guc_interrupts(dev_priv);
>> ret = intel_guc_submission_enable(guc);
>
> Thanks
> Sagar
Thanks,
Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/guc: Redefine guc_log_level modparam values
2018-01-11 9:24 ` Michal Wajdeczko
@ 2018-01-11 9:38 ` Sagar Arun Kamble
0 siblings, 0 replies; 5+ messages in thread
From: Sagar Arun Kamble @ 2018-01-11 9:38 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 1/11/2018 2:54 PM, Michal Wajdeczko wrote:
> On Thu, 11 Jan 2018 06:52:18 +0100, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>>
>>
>> On 1/10/2018 9:35 PM, Michal Wajdeczko wrote:
>>> We used value -1 to indicate "disabled" and values 0..3 to
>>> indicate "enabled", but most of our other modparams are using
>>> -1 for "auto" mode and 0 for "disable". For consistency let's
>>> change our log level values to:
>>>
>>> -1: auto (depends on USES_GUC and DRM_I915_DEBUG)
>> "depends on HAS_GUC, USES_GUC, DRM_I915_DEBUG and DRM_I915_DEBUG_GEM"
>
> I should write "(depends on platform and Kconfig.debug settings)" as
> actual condition may change in the near feature.
>
Yes
>> s/intel_uc_is_using_guc()/USES_GUC
>> seeing some more intel_uc_is_using_* instead of macro usage USES_*
>
> It was by design, as in intel_uc_sanitize_options() function we are
> using only HAS_xxx macros and instead of USES_xxx we call intel_uc_is_xxx
> helpers directly.
>
Got it
>>> 0: disabled
>>> 1: enabled (severity level 0 = min)
>>> 2: enabled (severity level 1)
>>> 3: enabled (severity level 2)
>>> 4: enabled (severity level 3 = max)
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>> drivers/gpu/drm/i915/i915_params.c | 3 ++-
>>> drivers/gpu/drm/i915/intel_guc.c | 21 ++++++++++-----
>>> drivers/gpu/drm/i915/intel_guc_log.c | 47
>>> ++++++++++++++++-----------------
>>> drivers/gpu/drm/i915/intel_uc.c | 51
>>> ++++++++++++++++++++++++++++++++++--
>>> 4 files changed, 87 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_params.c
>>> b/drivers/gpu/drm/i915/i915_params.c
>>> index b5f3eb4..0b553a8 100644
>>> --- a/drivers/gpu/drm/i915/i915_params.c
>>> +++ b/drivers/gpu/drm/i915/i915_params.c
>>> @@ -155,7 +155,8 @@ struct i915_params i915_modparams __read_mostly = {
>>> "(-1=auto, 0=disable [default], 1=GuC submission, 2=HuC load)");
>>> i915_param_named(guc_log_level, int, 0400,
>>> - "GuC firmware logging level (-1:disabled (default),
>>> 0-3:enabled)");
>>> + "GuC firmware logging level. Requires GuC to be loaded. "
>>> + "(-1=auto [default], 0=disable, 1..4=enable with verbosity
>>> min..max)");
>>> i915_param_named_unsafe(guc_firmware_path, charp, 0400,
>>> "GuC firmware path to use instead of the default one");
>>> diff --git a/drivers/gpu/drm/i915/intel_guc.c
>>> b/drivers/gpu/drm/i915/intel_guc.c
>>> index 50b4725..2227236 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>>> @@ -215,6 +215,18 @@ static u32 get_core_family(struct
>>> drm_i915_private *dev_priv)
>>> }
>>> }
>>> +static u32 get_log_verbosity_flags(void)
>>> +{
>> making this inline would be good i guess.
>
> No need to do so. Compiler will take care of it as needed (as this
> function is already marked as static)
>
>>> + if (i915_modparams.guc_log_level > 0) {
>>> + u32 verbosity = i915_modparams.guc_log_level - 1;
>>> +
>>> + GEM_BUG_ON(verbosity > GUC_LOG_VERBOSITY_MAX);
>>> + return verbosity << GUC_LOG_VERBOSITY_SHIFT;
>>> + }
>>> + GEM_BUG_ON(i915_modparams.enable_guc < 0);
>>> + return GUC_LOG_DISABLED;
>>> +}
>>> +
>>> /*
>>> * Initialise the GuC parameter block before starting the firmware
>>> * transfer. These parameters are read by the firmware on startup
>>> @@ -247,12 +259,7 @@ void intel_guc_init_params(struct intel_guc *guc)
>>> params[GUC_CTL_LOG_PARAMS] = guc->log.flags;
>>> - if (i915_modparams.guc_log_level >= 0) {
>>> - params[GUC_CTL_DEBUG] =
>>> - i915_modparams.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
>>> - } else {
>>> - params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
>>> - }
>>> + params[GUC_CTL_DEBUG] = get_log_verbosity_flags();
>>> /* If GuC submission is enabled, set up additional
>>> parameters here */
>>> if (USES_GUC_SUBMISSION(dev_priv)) {
>>> @@ -445,7 +452,7 @@ int intel_guc_resume(struct drm_i915_private
>>> *dev_priv)
>>> if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>> return 0;
>>> - if (i915_modparams.guc_log_level >= 0)
>>> + if (i915_modparams.guc_log_level > 0)
>> if (i915_modparams.guc_log_level)
>>> gen9_enable_guc_interrupts(dev_priv);
>>> data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
>>> b/drivers/gpu/drm/i915/intel_guc_log.c
>>> index eaedd63..e6d59bf 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> <snip>
>>> @@ -582,7 +578,7 @@ int i915_guc_log_control(struct
>>> drm_i915_private *dev_priv, u64 control_val)
>>> return -EINVAL;
>>> /* This combination doesn't make sense & won't have any
>>> effect */
>>> - if (!log_param.logging_enabled && (i915_modparams.guc_log_level
>>> < 0))
>>> + if (!log_param.logging_enabled && !i915_modparams.guc_log_level)
>>> return 0;
>>> ret = guc_log_control(guc, log_param.value);
>>> @@ -592,11 +588,12 @@ int i915_guc_log_control(struct
>>> drm_i915_private *dev_priv, u64 control_val)
>>> }
>>> if (log_param.logging_enabled) {
>>> - i915_modparams.guc_log_level = log_param.verbosity;
>>> + i915_modparams.guc_log_level = 1 + log_param.verbosity;
>> reading the log level from i915_guc_log_control_get debugfs should
>> decrement guc_log_level by 1.
>
> That's the other story.
> If I read our code right, we were using different format for get:
> (0=level 0, 1=level 1, ... 3=level 3)
> and set:
> (0=disabled, 1=enabled level 0, 9=enabled level 1, 11=enabled level 2...
>
> I think we can change that to use always (0, 1..4) and hide internal
> layout of the GuC command from the user.
>
Yes. makes sense to hide internals.
will then need to update intel_guc_logger as well.
>
>>> - /* If log_level was set as -1 at boot time, then the
>>> relay channel file
>>> - * wouldn't have been created by now and interrupts also
>>> would not have
>>> - * been enabled. Try again now, just in case.
>>> + /*
>>> + * If log was disabled at boot time, then the relay channel
>>> file
>>> + * wouldn't have been created by now and interrupts also would
>>> + * not have been enabled. Try again now, just in case.
>>> */
>>> ret = guc_log_late_setup(guc);
>>> if (ret < 0) {
>>> @@ -607,7 +604,8 @@ int i915_guc_log_control(struct drm_i915_private
>>> *dev_priv, u64 control_val)
>>> /* GuC logging is currently the only user of Guc2Host
>>> interrupts */
>>> gen9_enable_guc_interrupts(dev_priv);
>>> } else {
>>> - /* Once logging is disabled, GuC won't generate logs & send an
>>> + /*
>>> + * Once logging is disabled, GuC won't generate logs & send an
>>> * interrupt. But there could be some data in the log buffer
>>> * which is yet to be captured. So request GuC to update
>>> the log
>>> * buffer state and then collect the left over logs.
>>> @@ -615,7 +613,7 @@ int i915_guc_log_control(struct drm_i915_private
>>> *dev_priv, u64 control_val)
>>> guc_flush_logs(guc);
>>> /* As logging is disabled, update log level to reflect
>>> that */
>>> - i915_modparams.guc_log_level = -1;
>>> + i915_modparams.guc_log_level = 0;
>>> }
>>> return ret;
>>> @@ -623,8 +621,7 @@ int i915_guc_log_control(struct drm_i915_private
>>> *dev_priv, u64 control_val)
>>> void i915_guc_log_register(struct drm_i915_private *dev_priv)
>>> {
>>> - if (!USES_GUC_SUBMISSION(dev_priv) ||
>>> - (i915_modparams.guc_log_level < 0))
>>> + if (!USES_GUC_SUBMISSION(dev_priv) ||
>>> !i915_modparams.guc_log_level)
>>> return;
>>> mutex_lock(&dev_priv->drm.struct_mutex);
>>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>>> b/drivers/gpu/drm/i915/intel_uc.c
>>> index d82ca0f..fd39c06 100644
>>> --- a/drivers/gpu/drm/i915/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>>> @@ -65,6 +65,21 @@ static int __get_platform_enable_guc(struct
>>> drm_i915_private *dev_priv)
>>> return enable_guc;
>>> }
>>> +static int __get_default_guc_log_level(struct drm_i915_private
>>> *dev_priv)
>>> +{
>>> + int guc_log_level = 0; /* disabled */
>>> +
>>> + /* 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 = 1 + GUC_LOG_VERBOSITY_MAX;
>>> +
>>> + /* Any platform specific fine-tuning can be done here */
>>> +
>>> + return guc_log_level;
>>> +}
>>> +
>>> /**
>>> * intel_uc_sanitize_options - sanitize uC related modparam options
>>> * @dev_priv: device private
>>> @@ -74,6 +89,13 @@ static int __get_platform_enable_guc(struct
>>> drm_i915_private *dev_priv)
>>> * modparam varies between platforms and it is hardcoded in driver
>>> code.
>>> * Any other modparam value is only monitored against availability
>>> of the
>>> * related hardware or firmware definitions.
>>> + *
>>> + * In case of "guc_log_level" option this function will attempt to
>>> modify
>>> + * it only if it was initially set to "auto(-1)" or if initial
>>> value was
>>> + * "enable(1..4)" on platforms without the GuC. Default value for this
>>> + * modparam varies between platforms and is usually set to
>>> "disable(0)"
>>> + * unless GuC is enabled on given platform and the driver is
>>> compiled with
>>> + * debug config when this modparam will default to "enable(1..4)".
>>> */
>>> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>>> {
>>> @@ -105,8 +127,33 @@ void intel_uc_sanitize_options(struct
>>> drm_i915_private *dev_priv)
>>> "no HuC firmware");
>>> }
>>> + /* A negative value means "use platform/config default" */
>>> + if (i915_modparams.guc_log_level < 0)
>>> + i915_modparams.guc_log_level =
>>> + __get_default_guc_log_level(dev_priv);
>>> +
>>> + DRM_DEBUG_DRIVER("guc_log_level=%d (enabled:%s verbosity:%d)\n",
>>> + i915_modparams.guc_log_level,
>>> + yesno(i915_modparams.guc_log_level > 0),
>>> + i915_modparams.guc_log_level - 1);
>>> +
>> I think this DRM_DEBUG_DRIVER should be moved after all sanitization.
>
> Yes. I copied that from above enable_guc sanitization but forget that
> in above code there is no override :)
>
>>> + if (i915_modparams.guc_log_level > 0 &&
>>> !intel_uc_is_using_guc()) {
>>> + DRM_WARN("Incompatible option ignored: guc_log_level=%d,
>>> %s!\n",
>>> + i915_modparams.guc_log_level,
>>> + !HAS_GUC(dev_priv) ? "no GuC hardware" :
>>> + "GuC not enabled");
>>> + i915_modparams.guc_log_level = 0;
>>> + }
>>> +
>>> + if (i915_modparams.guc_log_level > 1 + GUC_LOG_VERBOSITY_MAX) {
>>> + DRM_WARN("Incompatible option ignored: guc_log_level=%d,
>>> %s!\n",
>> "Incompatible option overridden"? as we are letting this through with
>> max verbosity which I think is not ignoring :)
>
> sure
>
>>> + i915_modparams.guc_log_level, "verbosity too high");
>>> + i915_modparams.guc_log_level = 1 + GUC_LOG_VERBOSITY_MAX;
>>> + }
>>> +
>>> /* Make sure that sanitization was done */
>>> GEM_BUG_ON(i915_modparams.enable_guc < 0);
>>> + GEM_BUG_ON(i915_modparams.guc_log_level < 0);
>>> }
>>> void intel_uc_init_early(struct drm_i915_private *dev_priv)
>>> @@ -152,7 +199,7 @@ void intel_uc_init_mmio(struct drm_i915_private
>>> *dev_priv)
>>> static void guc_capture_load_err_log(struct intel_guc *guc)
>>> {
>>> - if (!guc->log.vma || i915_modparams.guc_log_level < 0)
>>> + if (!guc->log.vma || !i915_modparams.guc_log_level)
>>> return;
>>> if (!guc->load_err_log)
>>> @@ -322,7 +369,7 @@ int intel_uc_init_hw(struct drm_i915_private
>>> *dev_priv)
>>> }
>>> if (USES_GUC_SUBMISSION(dev_priv)) {
>>> - if (i915_modparams.guc_log_level >= 0)
>>> + if (i915_modparams.guc_log_level)
>>> gen9_enable_guc_interrupts(dev_priv);
>>> ret = intel_guc_submission_enable(guc);
>>
>> Thanks
>> Sagar
>
> Thanks,
> Michal
Thanks
Sagar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-01-11 9:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-10 16:05 [PATCH] drm/i915/guc: Redefine guc_log_level modparam values Michal Wajdeczko
2018-01-10 16:26 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-01-11 5:52 ` [PATCH] " Sagar Arun Kamble
2018-01-11 9:24 ` Michal Wajdeczko
2018-01-11 9:38 ` Sagar Arun Kamble
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.