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

[Belgaumkar, Vinay]

On 4/17/2023 6:39 PM, Andi Shyti wrote:
> 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?
The selftest was relying on setting min freq < RP1 to disable efficient 
freq, now that we have an interface, the test should use that (former 
method will not work). Should this be a separate 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?
I guess we should, will add.
>
>>   
>>   	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.
ok.
>
>> +
>> +	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.

ok.

Thanks,

Vinay.

>
> Andi