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

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

On Mon, 20 Mar 2023 17:27:23 -0700, Umesh Nerlige Ramappa wrote:
>
> On Sun, Mar 19, 2023 at 08:56:00PM -0700, Dixit, Ashutosh wrote:
> > On Fri, 17 Mar 2023 16:16:41 -0700, Umesh Nerlige Ramappa wrote:
> >>
> >
> > Hi Umesh,
> >
> > Please read from bottom to top. Mostly nits but let's see what you think.
> >
> >> 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.
> >>
> >> v2: (Ashutosh)
> >> - Remove drm_notice from the driver load path
> >> - Log a drm_err when opening an OAM stream on affected steppings
> >>
> >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_perf.c | 41 ++++++++++++++++++++++++++++++++
> >>  1 file changed, 41 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> >> index 18afa76653b7..823379d63caf 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"
> >>
> >> @@ -4216,6 +4217,20 @@ static int read_properties_unlocked(struct i915_perf *perf,
> >>		return -EINVAL;
> >>	}
> >>
> >> +	/*
> >> +	 * Wa_14017512683: mtl[a0..c0): Use of OAM must be preceded with Media
> >> +	 * C6 disable in BIOS. Fail if Media C6 is enabled on steppings where OAM
> >> +	 * does not work as expected.
> >> +	 */
> >> +	if (IS_MTL_MEDIA_STEP(props->engine->i915, STEP_A0, STEP_C0) &&
> >> +	    props->engine->gt->type == GT_MEDIA &&
> >
> > Instead of gt type I think it's better to check perf_group->type. That is
> > why as I said below maybe better to have a valid perf_group even in this
> > case.
> >
> >> +	    intel_check_bios_c6_setup(&props->engine->gt->rc6)) {
> >> +		drm_dbg(&perf->i915->drm,
> >> +			"OAM requires media C6 to be disabled in BIOS\n");
> >> +		return -EINVAL;
> >> +	}
> >
> > So now we can change this check to something like:
> >
> >	if (engine->perf_group->type == OAM && i915_perf->mtl_bios_mc6_enabled)
>
> sure, that will work too.

This will probably also need to be moved after the engine_supports_oa check
below.

>
> >
> >> +
> >> +
> >>	if (!engine_supports_oa(props->engine)) {
> >>		drm_dbg(&perf->i915->drm,
> >>			"Engine not supported by OA %d:%d\n",
> >> @@ -4897,6 +4912,15 @@ 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. To disable use of OAM with media engines, set the
> >> +	 * oa_group to PERF_GROUP_INVALID.
> >> +	 */
> >> +	if (IS_MTL_MEDIA_STEP(engine->i915, STEP_A0, STEP_C0) &&
> >> +	    intel_check_bios_c6_setup(&engine->gt->rc6))
> >> +		return PERF_GROUP_INVALID;
> >
> > I think we should just remove this. Let the perf group be valid in this
> > case too since we cannot support OA for a different reason. Then we can use
> > the OAM perf group above.
>
> Sure, I will drop this
> >
> > Though if we drop this we have only 2 instances of the checks instead of 3
> > so maybe ok to not have i915_perf->mtl_bios_mc6_enabled?
>
> yes, since it's only 2 places, I would prefer to leave it as is.
>
> Thanks,
> Umesh