Re: [PATCH v7 2/2] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx