| Message ID | 1505382908-6844-6-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 14 Sep 2017 11:55:07 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > From: "Kamble, Sagar A" <sagar.a.kamble@intel.com> > > With GuC v9, new type of Default/critical logging in GuC to enable > capturing minimal important logs in production systems efficiently. > This patch enables this logging in GuC by default always. It should > be noted that streaming support with half-full interrupt mechanism > that is present for normal logging is not present for this type of > logging. > > v2: Emulating GuC critical logging through i915.guc_log_level. Setting > this to 0 will make GuC critical logging ON and setting it to 1-4 will > communicate log level of 0-3 to GuC. Is this statement still valid ? If not, make sure to say so in later updates. > > v3: Commit message update. Enable default/critical logging in GuC always. > Fixed RPM wake during guc_log_unregister in the unload path. > > v4: Moved RPM wake change to separate patch. Removed GUC_DEBUG_RESERVED > and updated name of new bit to be version agnostic. Updated parameter to > struct intel_guc * and name of macro NEEDS_GUC_CRITICAL_LOGGING. > Removed explicit clearing of GUC_CRITICAL_LOGGING_DISABLED from > params[GUC_CTL_DEBUG] as it is unnecessary. (Michal Wajdeczko) > > v5: Removed GUC_CRITICAL_LOGGING_DISABLED. Added HAS_GUC check to > GUC_NEEDS_CRITICAL_LOGGING. (Michal Wajdeczko) > > Cc: Chheda Harsh J <harsh.j.chheda@intel.com> > Cc: Fry Gregory P <gregory.p.fry@intel.com> > Cc: Spotswood John A <john.a.spotswood@intel.com> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michał Winiarski <michal.winiarski@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/intel_guc_fwif.h | 15 +++++++++++++-- > drivers/gpu/drm/i915/intel_guc_log.c | 4 +++- > 2 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h > b/drivers/gpu/drm/i915/intel_guc_fwif.h > index 7eb6b4f..231b87e 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > @@ -127,7 +127,6 @@ > #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_ADS_ADDR_SHIFT 11 > #define GUC_ADS_ADDR_MASK 0xfffff800 > @@ -135,6 +134,17 @@ > #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 GUC_NEEDS_CRITICAL_LOGGING(guc) \ > + (HAS_GUC(guc_to_i915(guc)) && \ > + (((IS_SKYLAKE(guc_to_i915(guc)) || IS_BROXTON(guc_to_i915(guc))) && \ > + guc->fw.major_ver_found >= 9) || \ > + (IS_KABYLAKE(guc_to_i915(guc)) && guc->fw.major_ver_found >= 9 && \ > + guc->fw.minor_ver_found >= 39))) > + Hmm, I was thinking about something simpler like this: #define GUC_NEEDS_CRITICAL_LOGGING(guc) \ (HAS_GUC(guc_to_i915(guc) && \ (guc->fw.major_ver_found >= 9) && \ (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc) ? 39 : 0)) and I'm not sure that this macro shall be defined in fwif.h file. As it is only used in one place, maybe it can defined/expanded just there ? Michal > /** > * DOC: GuC Firmware Layout > * > @@ -539,7 +549,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_log.c > b/drivers/gpu/drm/i915/intel_guc_log.c > index 16d3b87..ba36162 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -589,7 +589,6 @@ 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; > - > union guc_log_control log_param; > int ret; > @@ -603,6 +602,9 @@ 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 (GUC_NEEDS_CRITICAL_LOGGING(guc)) > + log_param.critical_logging_enabled = 1; > + > ret = guc_log_control(guc, log_param.value); > if (ret < 0) { > DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
On 9/14/2017 6:43 PM, Michal Wajdeczko wrote: > On Thu, 14 Sep 2017 11:55:07 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> From: "Kamble, Sagar A" <sagar.a.kamble@intel.com> >> >> With GuC v9, new type of Default/critical logging in GuC to enable >> capturing minimal important logs in production systems efficiently. >> This patch enables this logging in GuC by default always. It should >> be noted that streaming support with half-full interrupt mechanism >> that is present for normal logging is not present for this type of >> logging. >> >> v2: Emulating GuC critical logging through i915.guc_log_level. Setting >> this to 0 will make GuC critical logging ON and setting it to 1-4 will >> communicate log level of 0-3 to GuC. > > Is this statement still valid ? > If not, make sure to say so in later updates. Will update this description. > >> >> v3: Commit message update. Enable default/critical logging in GuC >> always. >> Fixed RPM wake during guc_log_unregister in the unload path. >> >> v4: Moved RPM wake change to separate patch. Removed GUC_DEBUG_RESERVED >> and updated name of new bit to be version agnostic. Updated parameter to >> struct intel_guc * and name of macro NEEDS_GUC_CRITICAL_LOGGING. >> Removed explicit clearing of GUC_CRITICAL_LOGGING_DISABLED from >> params[GUC_CTL_DEBUG] as it is unnecessary. (Michal Wajdeczko) >> >> v5: Removed GUC_CRITICAL_LOGGING_DISABLED. Added HAS_GUC check to >> GUC_NEEDS_CRITICAL_LOGGING. (Michal Wajdeczko) >> >> Cc: Chheda Harsh J <harsh.j.chheda@intel.com> >> Cc: Fry Gregory P <gregory.p.fry@intel.com> >> Cc: Spotswood John A <john.a.spotswood@intel.com> >> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Michał Winiarski <michal.winiarski@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/intel_guc_fwif.h | 15 +++++++++++++-- >> drivers/gpu/drm/i915/intel_guc_log.c | 4 +++- >> 2 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h >> b/drivers/gpu/drm/i915/intel_guc_fwif.h >> index 7eb6b4f..231b87e 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >> @@ -127,7 +127,6 @@ >> #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_ADS_ADDR_SHIFT 11 >> #define GUC_ADS_ADDR_MASK 0xfffff800 >> @@ -135,6 +134,17 @@ >> #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 GUC_NEEDS_CRITICAL_LOGGING(guc) \ >> + (HAS_GUC(guc_to_i915(guc)) && \ >> + (((IS_SKYLAKE(guc_to_i915(guc)) || >> IS_BROXTON(guc_to_i915(guc))) && \ >> + guc->fw.major_ver_found >= 9) || \ >> + (IS_KABYLAKE(guc_to_i915(guc)) && guc->fw.major_ver_found >= 9 >> && \ >> + guc->fw.minor_ver_found >= 39))) >> + > > Hmm, I was thinking about something simpler like this: > > #define GUC_NEEDS_CRITICAL_LOGGING(guc) \ > (HAS_GUC(guc_to_i915(guc) && \ > (guc->fw.major_ver_found >= 9) && \ > (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc) ? 39 : 0)) > > and I'm not sure that this macro shall be defined in fwif.h file. > > As it is only used in one place, maybe it can defined/expanded just > there ? Sure. I will change this. > > Michal > >> /** >> * DOC: GuC Firmware Layout >> * >> @@ -539,7 +549,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_log.c >> b/drivers/gpu/drm/i915/intel_guc_log.c >> index 16d3b87..ba36162 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_log.c >> +++ b/drivers/gpu/drm/i915/intel_guc_log.c >> @@ -589,7 +589,6 @@ 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; >> - >> union guc_log_control log_param; >> int ret; >> @@ -603,6 +602,9 @@ 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 (GUC_NEEDS_CRITICAL_LOGGING(guc)) >> + log_param.critical_logging_enabled = 1; >> + >> ret = guc_log_control(guc, log_param.value); >> if (ret < 0) { >> DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", >> ret);
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index 7eb6b4f..231b87e 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -127,7 +127,6 @@ #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_ADS_ADDR_SHIFT 11 #define GUC_ADS_ADDR_MASK 0xfffff800 @@ -135,6 +134,17 @@ #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 GUC_NEEDS_CRITICAL_LOGGING(guc) \ + (HAS_GUC(guc_to_i915(guc)) && \ + (((IS_SKYLAKE(guc_to_i915(guc)) || IS_BROXTON(guc_to_i915(guc))) && \ + guc->fw.major_ver_found >= 9) || \ + (IS_KABYLAKE(guc_to_i915(guc)) && guc->fw.major_ver_found >= 9 && \ + guc->fw.minor_ver_found >= 39))) + /** * DOC: GuC Firmware Layout * @@ -539,7 +549,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_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 16d3b87..ba36162 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -589,7 +589,6 @@ 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; - union guc_log_control log_param; int ret; @@ -603,6 +602,9 @@ 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 (GUC_NEEDS_CRITICAL_LOGGING(guc)) + log_param.critical_logging_enabled = 1; + ret = guc_log_control(guc, log_param.value); if (ret < 0) { DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);