Re: [Intel-gfx] [PATCH] drm/i915/guc/slpc: Add a new SLPC selftest

["Belgaumkar, Vinay" <vinay.belgaumkar@intel.com>]

On 6/24/2022 8:59 PM, Dixit, Ashutosh wrote:
> On Thu, 23 Jun 2022 16:33:20 -0700, Vinay Belgaumkar wrote:
>> +static int max_granted_freq(struct intel_guc_slpc *slpc, struct intel_rps *rps, u32 *max_act_freq)
>> +{
>> +	struct intel_gt *gt = rps_to_gt(rps);
>> +	u32 perf_limit_reasons;
>> +	int err = 0;
>>
>> -			igt_spinner_end(&spin);
>> -			st_engine_heartbeat_enable(engine);
>> -		}
>> +	err = slpc_set_min_freq(slpc, slpc->rp0_freq);
>> +	if (err)
>> +		return err;
>>
>> -		pr_info("Max actual frequency for %s was %d\n",
>> -			engine->name, max_act_freq);
>> +	*max_act_freq =  intel_rps_read_actual_frequency(rps);
>> +	if (!(*max_act_freq == slpc->rp0_freq)) {
> nit but '*max_act_freq != slpc->rp0_freq'
ok.
>
>
>> +		/* Check if there was some throttling by pcode */
>> +		perf_limit_reasons = intel_uncore_read(gt->uncore, GT0_PERF_LIMIT_REASONS);
>>
>> -		/* Actual frequency should rise above min */
>> -		if (max_act_freq == slpc_min_freq) {
>> -			pr_err("Actual freq did not rise above min\n");
>> +		/* If not, this is an error */
>> +		if (!(perf_limit_reasons && GT0_PERF_LIMIT_REASONS_MASK)) {
> Still wrong, should be & not &&
yup, third time's the charm.
>
>> +			pr_err("Pcode did not grant max freq\n");
>> 			err = -EINVAL;
>> -		}
>> +		} else {
>> +			pr_info("Pcode throttled frequency 0x%x\n", perf_limit_reasons);
> Another question, why are we using pr_err/info here rather than
> drm_err/info? pr_err/info is ok for mock selftests since there is no drm
> device but that is not the case here, I think this is done in other
> selftests too but maybe fix this as well if we are making so many changes
> here? Anyway can do later too.
>
> So let's settle issues in v2 thread first.

Thanks,

Vinay.

>
> Thanks.
> --
> Ashutosh