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

Sat Oct 22 05:26:30 UTC 2022

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).

> >>+		}
> >>+	} 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?

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

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