[Intel-gfx] [PATCH] drm/i915/guc/slpc: Provide sysfs for efficient freq

[Andi Shyti]

Hi Vinay,

Looks good, just few minor comments below,

[...]

> @@ -267,13 +267,11 @@ static int run_test(struct intel_gt *gt, int test_type)
>  	}
>  
>  	/*
> -	 * Set min frequency to RPn so that we can test the whole
> -	 * range of RPn-RP0. This also turns off efficient freq
> -	 * usage and makes results more predictable.
> +	 * Turn off efficient freq so RPn/RP0 ranges are obeyed
>  	 */
> -	err = slpc_set_min_freq(slpc, slpc->min_freq);
> +	err = intel_guc_slpc_set_ignore_eff_freq(slpc, true);
>  	if (err) {
> -		pr_err("Unable to update min freq!");
> +		pr_err("Unable to turn off efficient freq!");

drm_err()? or gt_err()? As we are here we can use a proper
printing.

How is this change related to the scope of this patch?

>  		return err;
>  	}
>  
> @@ -358,9 +356,10 @@ static int run_test(struct intel_gt *gt, int test_type)
>  			break;
>  	}
>  
> -	/* Restore min/max frequencies */
> -	slpc_set_max_freq(slpc, slpc_max_freq);
> +	/* Restore min/max frequencies and efficient flag */
>  	slpc_set_min_freq(slpc, slpc_min_freq);
> +	slpc_set_max_freq(slpc, slpc_max_freq);
> +	intel_guc_slpc_set_ignore_eff_freq(slpc, false);

mmhhh... do we care here about the return value?

>  
>  	if (igt_flush_test(gt->i915))
>  		err = -EIO;
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> index 026d73855f36..b1b70ee3001b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> @@ -277,6 +277,7 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
>  
>  	slpc->max_freq_softlimit = 0;
>  	slpc->min_freq_softlimit = 0;
> +	slpc->ignore_eff_freq = false;
>  	slpc->min_is_rpmax = false;
>  
>  	slpc->boost_freq = 0;
> @@ -457,6 +458,31 @@ int intel_guc_slpc_get_max_freq(struct intel_guc_slpc *slpc, u32 *val)
>  	return ret;
>  }
>  
> +int intel_guc_slpc_set_ignore_eff_freq(struct intel_guc_slpc *slpc, bool val)
> +{
> +	struct drm_i915_private *i915 = slpc_to_i915(slpc);
> +	intel_wakeref_t wakeref;
> +	int ret = 0;

no need to initialize ret here.

> +
> +	mutex_lock(&slpc->lock);
> +	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> +
> +	ret = slpc_set_param(slpc,
> +			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> +			     val);
> +	if (ret) {
> +		guc_probe_error(slpc_to_guc(slpc), "Failed to set efficient freq(%d): %pe\n",
> +				val, ERR_PTR(ret));
> +		goto out;
> +	}
> +
> +	slpc->ignore_eff_freq = val;

nit that you can ignore: if you put this under else and save
brackets and a goto.

Andi