| Message ID | 1506074867-32714-3-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 22 Sep 2017 12:07:47 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > 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: Emulated GuC critical logging through i915.guc_log_level. > > 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) > > v6: More refined version of GUC_NEEDS_CRITICAL_LOGGING. Commit message > update. (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> > Reviewed-by: Michal Wajdeczko <michal.wajdeczko@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 | 4 ++-- > drivers/gpu/drm/i915/intel_guc_log.c | 13 ++++++++++++- > 2 files changed, 14 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..fed875a 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 > @@ -539,7 +538,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..677ec3d 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -586,10 +586,18 @@ void intel_guc_log_destroy(struct intel_guc *guc) > i915_vma_unpin_and_release(&guc->log.vma); > } > +/* > + * 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)) && \ Nitpick: As this macro is now used locally in function that is called only for platforms with HAS_GUC() then maybe we can drop that check completely as it is redundant and only check version? Michal > + (guc->fw.major_ver_found >= 9) && \ > + (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc)) ? 39 : 0))) > + > 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 +611,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 Fri, 2017-09-22 at 15:37 +0530, Sagar Arun Kamble wrote: > 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. The commit message would be a good place to debrief the user impact. Do we have the tools to capture the new style of log? And question is why do we enable any logging by default, let it have much or little impact on performance? This flag should probably be set through debugfs or some other means when we know we're going to want debugging output. > v2: Emulated GuC critical logging through i915.guc_log_level. > > 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) > > v6: More refined version of GUC_NEEDS_CRITICAL_LOGGING. Commit message > update. (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> > Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> <SNIP> > @@ -586,10 +586,18 @@ void intel_guc_log_destroy(struct intel_guc *guc) > i915_vma_unpin_and_release(&guc->log.vma); > } > > +/* > + * 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)) && \ > + (guc->fw.major_ver_found >= 9) && \ > + (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc)) ? 39 : 0))) I'd like to avoid this kind of conditionals as much as possible, to the extent that we must consider just not loading GuC at all if we're not finding an at least the firmware version we specify our code needs. That's what the distros are going to package, so we don't have to be backwards compatible. So this would be a question of IS_PLATFORM()s like elsewhere in the code once the increased firmware version is specified in the kernel code. > @@ -603,6 +611,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; > + This would then become an "if (INTEL_GEN() >= XYZ)". And it would be highly preferrable to get a backport to all older versions, too. They could even ignore the value if it has no visible effect. I have to admit I think this would be a variable that we read and maybe print a debug if it's up, if the firmware knows it 'needs this flag up always'. Anyway, we just got to organize that CI has both firmware versions (current and desired) in-place, then patches like this just have the 'increase required GuC version number' patch as as a gating in in front of this. Regards, Joonas
On 9/22/2017 3:54 PM, Michal Wajdeczko wrote: > On Fri, 22 Sep 2017 12:07:47 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> 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: Emulated GuC critical logging through i915.guc_log_level. >> >> 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) >> >> v6: More refined version of GUC_NEEDS_CRITICAL_LOGGING. Commit message >> update. (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> >> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@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 | 4 ++-- >> drivers/gpu/drm/i915/intel_guc_log.c | 13 ++++++++++++- >> 2 files changed, 14 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..fed875a 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 >> @@ -539,7 +538,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..677ec3d 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_log.c >> +++ b/drivers/gpu/drm/i915/intel_guc_log.c >> @@ -586,10 +586,18 @@ void intel_guc_log_destroy(struct intel_guc *guc) >> i915_vma_unpin_and_release(&guc->log.vma); >> } >> +/* >> + * 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)) && \ > > Nitpick: As this macro is now used locally in function that is > called only for platforms with HAS_GUC() then maybe we can drop > that check completely as it is redundant and only check version? > > Michal Yes. Will update. > >> + (guc->fw.major_ver_found >= 9) && \ >> + (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc)) ? 39 >> : 0))) >> + >> 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 +611,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/22/2017 5:34 PM, Joonas Lahtinen wrote: > On Fri, 2017-09-22 at 15:37 +0530, Sagar Arun Kamble wrote: >> 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. > The commit message would be a good place to debrief the user impact. Do > we have the tools to capture the new style of log? This has been verified to have minor impact by GuC team. Greg, Harsh can clarify further. Goal was to allow to get GuC logs from production systems. These are the subset of same GuC logs available currently at all GuC log levels and are needed to be captured always whenever we want to capture GuC logs at i915.guc_log_level >=0 and <=3. Chris had suggested on why this is not made on of the log levels and on discussion with Harsh it is concluded that this will not be supported as log level as GuC is using fast functions to log the new critical logs. Also there will not be half-full streaming support for these type of logs. I did have a patch earlier to disable this by default or emulate it as log level to the end user but recommendation from GuC team is to always turn this ON. > And question is why do we enable any logging by default, let it have > much or little impact on performance? This flag should probably be set > through debugfs or some other means when we know we're going to want > debugging output. In my earlier patch we did have ability to turn this ON/OFF https://patchwork.freedesktop.org/patch/173884/ > >> v2: Emulated GuC critical logging through i915.guc_log_level. >> >> 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) >> >> v6: More refined version of GUC_NEEDS_CRITICAL_LOGGING. Commit message >> update. (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> >> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > <SNIP> > >> @@ -586,10 +586,18 @@ void intel_guc_log_destroy(struct intel_guc *guc) >> i915_vma_unpin_and_release(&guc->log.vma); >> } >> >> +/* >> + * 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)) && \ >> + (guc->fw.major_ver_found >= 9) && \ >> + (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc)) ? 39 : 0))) > I'd like to avoid this kind of conditionals as much as possible, to the > extent that we must consider just not loading GuC at all if we're not > finding an at least the firmware version we specify our code needs. > That's what the distros are going to package, so we don't have to be > backwards compatible. > > So this would be a question of IS_PLATFORM()s like elsewhere in the > code once the increased firmware version is specified in the kernel > code. Agree that if we make major-wanted = 9 we can avoid this check all together. Currently GuC firmware versions in 01.org are pre version 9 and I have verified that this change is not backward compatible w.r.t v9. Verified that logging does not work for pre-v9 GuC firmwares if we set log_param.critical_logging_enabled unconditionally. > >> @@ -603,6 +611,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; >> + > This would then become an "if (INTEL_GEN() >= XYZ)". And it would be > highly preferrable to get a backport to all older versions, too. They > could even ignore the value if it has no visible effect. > > I have to admit I think this would be a variable that we read and maybe > print a debug if it's up, if the firmware knows it 'needs this flag up > always'. > > Anyway, we just got to organize that CI has both firmware versions > (current and desired) in-place, then patches like this just have the > 'increase required GuC version number' patch as as a gating in in front > of this. > > Regards, Joonas
On Fri, 2017-09-22 at 20:16 +0530, Sagar Arun Kamble wrote: > > On 9/22/2017 5:34 PM, Joonas Lahtinen wrote: > > On Fri, 2017-09-22 at 15:37 +0530, Sagar Arun Kamble wrote: > > > 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. > > > > The commit message would be a good place to debrief the user impact. Do > > we have the tools to capture the new style of log? > > This has been verified to have minor impact by GuC team. Greg, Harsh can > clarify further. > Goal was to allow to get GuC logs from production systems. > These are the subset of same GuC logs available currently at all GuC log > levels and are needed to be captured > always whenever we want to capture GuC logs at i915.guc_log_level >=0 > and <=3. > Chris had suggested on why this is not made on of the log levels and on > discussion with Harsh it is > concluded that this will not be supported as log level as GuC is using > fast functions to log the new > critical logs. Also there will not be half-full streaming support for > these type of logs. > I did have a patch earlier to disable this by default or emulate it as > log level to the end user but recommendation > from GuC team is to always turn this ON. I see little point in detecting firmware version and unconditionally setting a flag up at boot from kernel driver, when the firmware can do it themselves knowing their own version. We should not be polluting driver codebase with such constructs. It's firmware, a new version can be provided easily unlike with hardware which might need W/As. > > And question is why do we enable any logging by default, let it have > > much or little impact on performance? This flag should probably be set > > through debugfs or some other means when we know we're going to want > > debugging output. > > In my earlier patch we did have ability to turn this ON/OFF > https://patchwork.freedesktop.org/patch/173884/ Yeah, I think we might want to extend the existing .enable_guc_logging parameter once we understand the relation between critical and regular logging in a released firmware. Could be as simple as; (...).enable_critical_logging = (i915_modparams.guc_log_level >= 0); For the time being, we shall wait for a fixed firmware. Regards, Joonas
On 9/25/2017 3:31 PM, Joonas Lahtinen wrote: > On Fri, 2017-09-22 at 20:16 +0530, Sagar Arun Kamble wrote: >> On 9/22/2017 5:34 PM, Joonas Lahtinen wrote: >>> On Fri, 2017-09-22 at 15:37 +0530, Sagar Arun Kamble wrote: >>>> 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. >>> The commit message would be a good place to debrief the user impact. Do >>> we have the tools to capture the new style of log? >> This has been verified to have minor impact by GuC team. Greg, Harsh can >> clarify further. >> Goal was to allow to get GuC logs from production systems. >> These are the subset of same GuC logs available currently at all GuC log >> levels and are needed to be captured >> always whenever we want to capture GuC logs at i915.guc_log_level >=0 >> and <=3. >> Chris had suggested on why this is not made on of the log levels and on >> discussion with Harsh it is >> concluded that this will not be supported as log level as GuC is using >> fast functions to log the new >> critical logs. Also there will not be half-full streaming support for >> these type of logs. >> I did have a patch earlier to disable this by default or emulate it as >> log level to the end user but recommendation >> from GuC team is to always turn this ON. > I see little point in detecting firmware version and unconditionally > setting a flag up at boot from kernel driver, when the firmware can do > it themselves knowing their own version. > > We should not be polluting driver codebase with such constructs. It's > firmware, a new version can be provided easily unlike with hardware > which might need W/As. Agree. > >>> And question is why do we enable any logging by default, let it have >>> much or little impact on performance? This flag should probably be set >>> through debugfs or some other means when we know we're going to want >>> debugging output. >> In my earlier patch we did have ability to turn this ON/OFF >> https://patchwork.freedesktop.org/patch/173884/ > Yeah, I think we might want to extend the existing .enable_guc_logging > parameter once we understand the relation between critical and regular > logging in a released firmware. > > Could be as simple as; > > (...).enable_critical_logging = (i915_modparams.guc_log_level >= 0); > > For the time being, we shall wait for a fixed firmware. > > Regards, Joonas Sure. Will request for this change to be made in the firmware.
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index 7eb6b4f..fed875a 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 @@ -539,7 +538,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..677ec3d 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -586,10 +586,18 @@ void intel_guc_log_destroy(struct intel_guc *guc) i915_vma_unpin_and_release(&guc->log.vma); } +/* + * 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)) && \ + (guc->fw.major_ver_found >= 9) && \ + (guc->fw.minor_ver_found >= (IS_KABYLAKE(guc_to_i915(guc)) ? 39 : 0))) + 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 +611,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);