Re: [Intel-gfx] [PATCH v6 12/12] drm/i915/perf: Wa_14017512683: Disable OAM if media C6 is enabled in BIOS

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Wed, 15 Mar 2023 18:01:01 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

Mostly looks good but one nit below.

> OAM does not work with media C6 enabled on some steppings of MTL.
> Disable OAM if we detect that media C6 was enabled in bios.
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_perf.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 77fae3d80128..4ac6535a0356 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -209,6 +209,7 @@
>  #include "gt/intel_gt_regs.h"
>  #include "gt/intel_lrc.h"
>  #include "gt/intel_lrc_reg.h"
> +#include "gt/intel_rc6.h"
>  #include "gt/intel_ring.h"
>  #include "gt/uc/intel_guc_slpc.h"
>
> @@ -4898,6 +4899,18 @@ static u32 num_perf_groups_per_gt(struct intel_gt *gt)
>
>  static u32 __oam_engine_group(struct intel_engine_cs *engine)
>  {
> +	/*
> +	 * Wa_14017512683: mtl[a0..c0): Use of OAM must be preceded with Media
> +	 * C6 disable in BIOS. Do not enable OA for media classes if MC6 is
> +	 * enabled in BIOS.
> +	 */
> +	if (IS_MTL_MEDIA_STEP(engine->i915, STEP_A0, STEP_C0) &&
> +	    intel_check_bios_c6_setup(&engine->gt->rc6)) {
> +		drm_notice_once(&engine->i915->drm,
> +				"OAM requires media C6 to be disabled in BIOS\n");

I think the typical mode of working with MTL would be to enable C6 unless OA
is needed. But this will print out this notice on every MTL system. So IMO
we should print this out only when a OAM perf stream is opened and that
fails.

Though not sure if it's ok to add a line to an already cluttered dmesg? The
default console log level is 4 (WARNING) so maybe ok?

https://linuxconfig.org/introduction-to-the-linux-kernel-log-levels

Though if we fail the opening of an OAM stream we could make it a drm_warn?

> +		return PERF_GROUP_INVALID;
> +	}
> +
>	if (GRAPHICS_VER_FULL(engine->i915) >= IP_VER(12, 70)) {
>		/*
>		 * There's 1 SAMEDIA gt and 1 OAM per SAMEDIA gt. All media slices
> @@ -5317,6 +5330,23 @@ int i915_perf_ioctl_version(struct drm_i915_private *i915)
>	 *
>	 * 7: Add support for video decode and enhancement classes.
>	 */
> +
> +	/*
> +	 * Wa_14017512683: mtl[a0..c0): Use of OAM must be preceded with Media
> +	 * C6 disable in BIOS. Do not enable OA for media classes if MC6 is
> +	 * enabled in BIOS.

Maybe add something like "Return version 6 to indicate non-support for OAM."
just to make clear.

> +	 */
> +	if (IS_MTL_MEDIA_STEP(i915, STEP_A0, STEP_C0)) {
> +		struct intel_gt *gt;
> +		int i;
> +
> +		for_each_gt(gt, i915, i) {
> +			if (gt->type == GT_MEDIA &&
> +			    intel_check_bios_c6_setup(&gt->rc6))
> +				return 6;
> +		}
> +	}
> +
>	return 7;
>  }
>
> --
> 2.36.1
>

Thanks.
--
Ashutosh