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

[Belgaumkar, Vinay]

On 10/21/2022 10:26 PM, Dixit, Ashutosh wrote:
> On Fri, 21 Oct 2022 18:38:57 -0700, Belgaumkar, Vinay wrote:
>> 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.
> But even then the original check is much clearer since it is actually
> comparing the fused freq's:
>
> 	if (slpc->min_freq == slpc->rp0_freq)
>
> Because if min/max have been changed slpc_min_freq and slpc_max_freq are no
> longer fused freq.
>
> And also this check should be right at the top of run_test, right after if
> (!intel_uc_uses_guc_slpc), rather than in the middle here (otherwise
> because we are basically not doing any error rewinding so causing memory
> leaks if any of the functions return error).
ok.
>
>>>> +		}
>>>> +	} else {
>>>> +		/*
>>>> +		 * 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;
>>>> 	}
> So let's do what is suggested above and then see what remains here and if
> we need all these code changes. Most likely we can just do unconditionally
> what we were doing before, i.e.:
>
> 	err = slpc_set_min_freq(slpc, slpc->min_freq);
> 	if (err)
> 		return err;
>
>>>> +	saved_min_freq = slpc_min_freq;
>>>> +
>>>> +	/* New temp min freq = RPn */
>>>> +	slpc_min_freq = slpc->min_freq;
> Why do we need saved_min_freq? We can retain slpc_min_freq and in the check below:
>
> 	if (max_act_freq <= slpc_min_freq)
>
> We can just change the check to:
>
> 	if (max_act_freq <= slpc->min_freq)
>
> Looks like to have been a bug in the original code?
Not a bug, it wasn't needed until we didn't have server parts 
(slpc_min_freq would typically be slpc->min_freq on non-server parts).
>>>> +
>>>> 	intel_gt_pm_wait_for_idle(gt);
>>>> 	intel_gt_pm_get(gt);
>>>> 	for_each_engine(engine, gt, id) {
>>>> @@ -347,7 +363,7 @@ static int run_test(struct intel_gt *gt, int test_type)
>>>>
>>>> 	/* Restore min/max frequencies */
>>>> 	slpc_set_max_freq(slpc, slpc_max_freq);
>>>> -	slpc_set_min_freq(slpc, slpc_min_freq);
>>>> +	slpc_set_min_freq(slpc, saved_min_freq);
>>>>
>>>> 	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 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.
>>
>>>> +
>>>> +	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) {
> This should be swapped around:
>
> 	if (!slpc->min_freq_softlimit && is_slpc_min_freq_rpmax(slpc))
>
> So we should only have to call is_slpc_min_freq_rpmax if
> slpc->min_freq_softlimit is 0 (that is only once the first time during
> init).
ok.
>
>>>> +		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.

yes, GuC returns the value in MHz.

Thanks,

Vinay.

>>>
>>> Thanks.
>>> --
>>> Ashutosh