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@xxxxxxxxx> 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@xxxxxxxxx> Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> --- 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.cindex 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.cindex 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.cindex 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)reading the log level from i915_guc_log_control_get debugfs should decrement guc_log_level by 1.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;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.cindex 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","Incompatible option overridden"? as we are letting this through with max verbosity which I think is not ignoring :)+ 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",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 SagarThanks, Michal
Thanks Sagar _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
