[Intel-gfx] [PATCH 5/6] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9

[Kamble, Sagar A]

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 at intel.com> wrote:
>
>> From: "Kamble, Sagar A" <sagar.a.kamble at 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 at intel.com>
>> Cc: Fry Gregory P <gregory.p.fry at intel.com>
>> Cc: Spotswood John A <john.a.spotswood at intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Michał Winiarski <michal.winiarski at intel.com>
>> Signed-off-by: Jeff McGee <jeff.mcgee at intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at 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);