[Intel-gfx] [PATCH v3] drm/i915/slpc: Use platform limits for min/max frequency

[Belgaumkar, Vinay]

On 10/20/2022 3:57 PM, Dixit, Ashutosh wrote:
> On Tue, 18 Oct 2022 11:30:31 -0700, Vinay Belgaumkar wrote:
> Hi Vinay,
>
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c b/drivers/gpu/drm/i915/gt/selftest_slpc.c
>> index 4c6e9257e593..e42bc215e54d 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
>> @@ -234,6 +234,7 @@ static int run_test(struct intel_gt *gt, int test_type)
>> 	enum intel_engine_id id;
>> 	struct igt_spinner spin;
>> 	u32 slpc_min_freq, slpc_max_freq;
>> +	u32 saved_min_freq;
>> 	int err = 0;
>>
>> 	if (!intel_uc_uses_guc_slpc(&gt->uc))
>> @@ -252,20 +253,35 @@ static int run_test(struct intel_gt *gt, int test_type)
>> 		return -EIO;
>> 	}
>>
>> -	/*
>> -	 * FIXME: With efficient frequency enabled, GuC can request
>> -	 * frequencies higher than the SLPC max. While this is fixed
>> -	 * in GuC, we level set these tests with RPn as min.
>> -	 */
>> -	err = slpc_set_min_freq(slpc, slpc->min_freq);
>> -	if (err)
>> -		return err;
>> +	if (slpc_min_freq == slpc_max_freq) {
>> +		/* Server parts will have min/max clamped to RP0 */
>> +		if (slpc->min_is_rpmax) {
>> +			err = slpc_set_min_freq(slpc, slpc->min_freq);
>> +			if (err) {
>> +				pr_err("Unable to update min freq on server part");
>> +				return err;
>> +			}
>>
>> -	if (slpc->min_freq == slpc->rp0_freq) {
>> -		pr_err("Min/Max are fused to the same value\n");
>> -		return -EINVAL;
>> +		} else {
>> +			pr_err("Min/Max are fused to the same value\n");
>> +			return -EINVAL;
> Sorry but I am not following this else case here. Why are we saying min/max
> are fused to the same value? In this case we can't do
> "slpc_set_min_freq(slpc, slpc->min_freq)" ? That is, we can't change SLPC
> min freq?
This would be an error case due to a faulty part. We may come across a 
part where min/max is fused to the same value.
>
>> 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 fdd895f73f9f..b7cdeec44bd3 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>> @@ -263,6 +263,7 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
>>
>> 	slpc->max_freq_softlimit = 0;
>> 	slpc->min_freq_softlimit = 0;
>> +	slpc->min_is_rpmax = false;
>>
>> 	slpc->boost_freq = 0;
>> 	atomic_set(&slpc->num_waiters, 0);
>> @@ -588,6 +589,32 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc)
>> 	return 0;
>>   }
>>
>> +static bool is_slpc_min_freq_rpmax(struct intel_guc_slpc *slpc)
>> +{
>> +	int slpc_min_freq;
>> +
>> +	if (intel_guc_slpc_get_min_freq(slpc, &slpc_min_freq))
>> +		return false;
> I am wondering what happens if the above fails on server? Should we return
> true or false on server and what are the consequences of returning false on
> server?
>
> Any case I think we should at least put a drm_err or something here just in
> case this ever fails so we'll know something weird happened.

Makes sense.

Thanks,

Vinay.

>
>> +
>> +	if (slpc_min_freq == SLPC_MAX_FREQ_MHZ)
>> +		return true;
>> +	else
>> +		return false;
>> +}
>> +
>> +static void update_server_min_softlimit(struct intel_guc_slpc *slpc)
>> +{
>> +	/* For server parts, SLPC min will be at RPMax.
>> +	 * Use min softlimit to clamp it to RP0 instead.
>> +	 */
>> +	if (is_slpc_min_freq_rpmax(slpc) &&
>> +	    !slpc->min_freq_softlimit) {
>> +		slpc->min_is_rpmax = true;
>> +		slpc->min_freq_softlimit = slpc->rp0_freq;
>> +		(slpc_to_gt(slpc))->defaults.min_freq = slpc->min_freq_softlimit;
>> +	}
>> +}
>> +
>>   static int slpc_use_fused_rp0(struct intel_guc_slpc *slpc)
>>   {
>> 	/* Force SLPC to used platform rp0 */
>> @@ -647,6 +674,9 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)
>>
>> 	slpc_get_rp_values(slpc);
>>
>> +	/* Handle the case where min=max=RPmax */
>> +	update_server_min_softlimit(slpc);
>> +
>> 	/* Set SLPC max limit to RP0 */
>> 	ret = slpc_use_fused_rp0(slpc);
>> 	if (unlikely(ret)) {
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
>> index 82a98f78f96c..11975a31c9d0 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
>> @@ -9,6 +9,8 @@
>>   #include "intel_guc_submission.h"
>>   #include "intel_guc_slpc_types.h"
>>
>> +#define SLPC_MAX_FREQ_MHZ 4250
> This seems to be really a value (255 converted to freq) so seems ok to
> intepret in MHz.
>
> Thanks.
> --
> Ashutosh