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

[Umesh Nerlige Ramappa]

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 at 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.

>
>> +
>> +
>>	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