| Message ID | 1502274231-30714-4-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Sagar Arun Kamble (2017-08-09 11:23:48) > From GuC v9 firmware (for KBL v9.39+), separate control is added to enable > critical logging in GuC to enable capturing minimal important logs in > production systems. > i915.guc_log_level controls the verbosity and logging in GuC for logs other > than critical logs. By default, logging in GuC is disabled through > i915.guc_log_level. > This patch introduces new kernel param i915.enable_guc_critical_logging. > For Linux release builds, if needed critical GuC logs can be enabled > separately through this parameter. GuC log snapshot captured in error state > will have these minimal critical events logged. > Default value for this parameter is currently set to false. > This patch updates the initialization parameter sent during GuC load to > disable critical logging unless i915.guc_log_level is set to enable logging > and ensures it is enabled/disabling while enabling/disabling through > debugfs based on i915.enable_guc_critical_logging. > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Spotswood John A <john.a.spotswood@intel.com> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_params.c | 5 +++++ > drivers/gpu/drm/i915/i915_params.h | 3 ++- > drivers/gpu/drm/i915/intel_guc_fwif.h | 14 +++++++++++++- > drivers/gpu/drm/i915/intel_guc_loader.c | 21 ++++++++++++++++++++- > drivers/gpu/drm/i915/intel_guc_log.c | 9 ++++++++- > 5 files changed, 48 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index 14e2c2e..902bf2c 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -59,6 +59,7 @@ struct i915_params i915 __read_mostly = { > .enable_guc_loading = 0, > .enable_guc_submission = 0, > .guc_log_level = -1, > + .enable_guc_critical_logging = false, What's the point in having a log level if LOG_LEVEL_CRITICAL is not part of it? Please do explain why the current parameter does not cover this. -Chris
-----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Wednesday, August 9, 2017 4:20 PM To: Kamble, Sagar A <sagar.a.kamble@intel.com>; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915/guc: Disable critical logging in GuC by default from GuC v9 Quoting Sagar Arun Kamble (2017-08-09 11:23:48) > From GuC v9 firmware (for KBL v9.39+), separate control is added to > enable critical logging in GuC to enable capturing minimal important > logs in production systems. > i915.guc_log_level controls the verbosity and logging in GuC for logs > other than critical logs. By default, logging in GuC is disabled > through i915.guc_log_level. > This patch introduces new kernel param i915.enable_guc_critical_logging. > For Linux release builds, if needed critical GuC logs can be enabled > separately through this parameter. GuC log snapshot captured in error > state will have these minimal critical events logged. > Default value for this parameter is currently set to false. > This patch updates the initialization parameter sent during GuC load > to disable critical logging unless i915.guc_log_level is set to enable > logging and ensures it is enabled/disabling while enabling/disabling > through debugfs based on i915.enable_guc_critical_logging. > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Spotswood John A <john.a.spotswood@intel.com> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_params.c | 5 +++++ > drivers/gpu/drm/i915/i915_params.h | 3 ++- > drivers/gpu/drm/i915/intel_guc_fwif.h | 14 +++++++++++++- > drivers/gpu/drm/i915/intel_guc_loader.c | 21 ++++++++++++++++++++- > drivers/gpu/drm/i915/intel_guc_log.c | 9 ++++++++- > 5 files changed, 48 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > index 14e2c2e..902bf2c 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -59,6 +59,7 @@ struct i915_params i915 __read_mostly = { > .enable_guc_loading = 0, > .enable_guc_submission = 0, > .guc_log_level = -1, > + .enable_guc_critical_logging = false, What's the point in having a log level if LOG_LEVEL_CRITICAL is not part of it? Please do explain why the current parameter does not cover this. -Chris <Sagar> Agree that this could have been enumerated as another log level. Having it as log level simplifies other capture code too which I am seeing needs fix with current change. Will check if this can be changed in upcoming releases.
>-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of >Kamble, Sagar A >Sent: Monday, August 14, 2017 3:45 AM >To: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915/guc: Disable critical logging in GuC >by default from GuC v9 > > > >-----Original Message----- >From: Chris Wilson [mailto:chris@chris-wilson.co.uk] >Sent: Wednesday, August 9, 2017 4:20 PM >To: Kamble, Sagar A <sagar.a.kamble@intel.com>; intel- >gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915/guc: Disable critical logging in GuC >by default from GuC v9 > >Quoting Sagar Arun Kamble (2017-08-09 11:23:48) >> From GuC v9 firmware (for KBL v9.39+), separate control is added to >> enable critical logging in GuC to enable capturing minimal important >> logs in production systems. >> i915.guc_log_level controls the verbosity and logging in GuC for logs >> other than critical logs. By default, logging in GuC is disabled >> through i915.guc_log_level. >> This patch introduces new kernel param i915.enable_guc_critical_logging. >> For Linux release builds, if needed critical GuC logs can be enabled >> separately through this parameter. GuC log snapshot captured in error >> state will have these minimal critical events logged. >> Default value for this parameter is currently set to false. >> This patch updates the initialization parameter sent during GuC load >> to disable critical logging unless i915.guc_log_level is set to enable >> logging and ensures it is enabled/disabling while enabling/disabling >> through debugfs based on i915.enable_guc_critical_logging. >> >> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >> Cc: Spotswood John A <john.a.spotswood@intel.com> >> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> --- >> drivers/gpu/drm/i915/i915_params.c | 5 +++++ >> drivers/gpu/drm/i915/i915_params.h | 3 ++- >> drivers/gpu/drm/i915/intel_guc_fwif.h | 14 +++++++++++++- >> drivers/gpu/drm/i915/intel_guc_loader.c | 21 ++++++++++++++++++++- >> drivers/gpu/drm/i915/intel_guc_log.c | 9 ++++++++- >> 5 files changed, 48 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_params.c >> b/drivers/gpu/drm/i915/i915_params.c >> index 14e2c2e..902bf2c 100644 >> --- a/drivers/gpu/drm/i915/i915_params.c >> +++ b/drivers/gpu/drm/i915/i915_params.c >> @@ -59,6 +59,7 @@ struct i915_params i915 __read_mostly = { >> .enable_guc_loading = 0, >> .enable_guc_submission = 0, >> .guc_log_level = -1, >> + .enable_guc_critical_logging = false, > >What's the point in having a log level if LOG_LEVEL_CRITICAL is not part of it? >Please do explain why the current parameter does not cover this. >-Chris I agree with Chris. Apart from introducing a new parameter, this is also introducing the dependency on an existing one - guc_log_level. Anusha ><Sagar> Agree that this could have been enumerated as another log level. Having >it as log level simplifies other capture code too which I am seeing needs fix with >current change. Will check if this can be changed in upcoming releases. > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 14e2c2e..902bf2c 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -59,6 +59,7 @@ struct i915_params i915 __read_mostly = { .enable_guc_loading = 0, .enable_guc_submission = 0, .guc_log_level = -1, + .enable_guc_critical_logging = false, .guc_firmware_path = NULL, .huc_firmware_path = NULL, .enable_dp_mst = true, @@ -232,6 +233,10 @@ struct i915_params i915 __read_mostly = { MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level (-1:disabled (default), 0-3:enabled)"); +module_param_named(enable_guc_critical_logging, i915.enable_guc_critical_logging, bool, 0400); +MODULE_PARM_DESC(enable_guc_critical_logging, + "Enable GuC firmware critical logging (default: false)"); + module_param_named_unsafe(guc_firmware_path, i915.guc_firmware_path, charp, 0400); MODULE_PARM_DESC(guc_firmware_path, "GuC firmware path to use instead of the default one"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index febbfdb..7c208f0 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -67,7 +67,8 @@ func(bool, nuclear_pageflip); \ func(bool, enable_dp_mst); \ func(bool, enable_dpcd_backlight); \ - func(bool, enable_gvt) + func(bool, enable_gvt); \ + func(bool, enable_guc_critical_logging) #define MEMBER(T, member) T member struct i915_params { diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index 5fa2860..353b081 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -132,6 +132,7 @@ #define GUC_WQ_TRACK_ENABLED (1 << 8) #define GUC_ADS_ENABLED (1 << 9) #define GUC_DEBUG_RESERVED (1 << 10) +#define GUC_V9_CRITICAL_LOGGING_DISABLED (1 << 10) #define GUC_ADS_ADDR_SHIFT 11 #define GUC_ADS_ADDR_MASK 0xfffff800 @@ -139,6 +140,16 @@ #define GUC_CTL_MAX_DWORDS (SOFT_SCRATCH_COUNT - 2) /* [1..14] */ +/* + * Critical logging in GuC is to be enabled always from GuC v9+. + * (for KBL - v9.39+) + */ +#define NEEDS_GUC_CRITICAL_LOGGING(dev_priv, guc_fw) \ + (((IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv)) && \ + guc_fw->major_ver_found >= 9) || \ + (IS_KABYLAKE(dev_priv) && guc_fw->major_ver_found >= 9 && \ + guc_fw->minor_ver_found >= 39)) + /** * DOC: GuC Firmware Layout * @@ -539,7 +550,8 @@ struct guc_log_buffer_state { u32 logging_enabled:1; u32 reserved1:3; u32 verbosity:4; - u32 reserved2:24; + u32 critical_logging_enabled:1; + u32 reserved2:23; }; u32 value; } __packed; diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 81e03a6..535c665 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -106,8 +106,10 @@ static u32 get_core_family(struct drm_i915_private *dev_priv) static void guc_params_init(struct drm_i915_private *dev_priv) { struct intel_guc *guc = &dev_priv->guc; + struct intel_uc_fw *guc_fw = &dev_priv->guc.fw; u32 params[GUC_CTL_MAX_DWORDS]; int i; + bool enable_critical_logging = false; memset(¶ms, 0, sizeof(params)); @@ -130,11 +132,28 @@ static void guc_params_init(struct drm_i915_private *dev_priv) params[GUC_CTL_LOG_PARAMS] = guc->log.flags; + /* + * Enable critical logging in GuC based on i915.guc_log_level and + * i915.enable_guc_critical_logging. + */ if (i915.guc_log_level >= 0) { params[GUC_CTL_DEBUG] = i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT; - } else + enable_critical_logging = true; + } else { params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED; + if (i915.enable_guc_critical_logging) + enable_critical_logging = true; + } + + if (NEEDS_GUC_CRITICAL_LOGGING(dev_priv, guc_fw)) { + if (enable_critical_logging) + params[GUC_CTL_DEBUG] &= + ~GUC_V9_CRITICAL_LOGGING_DISABLED; + else + params[GUC_CTL_DEBUG] |= + GUC_V9_CRITICAL_LOGGING_DISABLED; + } /* If GuC submission is enabled, set up additional parameters here */ if (i915.enable_guc_submission) { diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 16d3b87..2f9bf79 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -589,7 +589,7 @@ void intel_guc_log_destroy(struct intel_guc *guc) int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) { struct intel_guc *guc = &dev_priv->guc; - + struct intel_uc_fw *guc_fw = &dev_priv->guc.fw; union guc_log_control log_param; int ret; @@ -603,6 +603,13 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) if (!log_param.logging_enabled && (i915.guc_log_level < 0)) return 0; + if (NEEDS_GUC_CRITICAL_LOGGING(dev_priv, guc_fw)) { + log_param.critical_logging_enabled = 1; + if (!log_param.logging_enabled && + !i915.enable_guc_critical_logging) + log_param.critical_logging_enabled = 0; + } + ret = guc_log_control(guc, log_param.value); if (ret < 0) { DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);