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

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

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'


> +		/* 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 &&

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