Re: [PATCH] drm/i915/guc/slpc: Allow SLPC to use efficient frequency

["Belgaumkar, Vinay" <vinay.belgaumkar@xxxxxxxxx>]

On 8/15/2022 9:51 AM, Rodrigo Vivi wrote:
> On Tue, Aug 09, 2022 at 05:03:06PM -0700, Vinay Belgaumkar wrote:
> > Host Turbo operates at efficient frequency when GT is not idle unless
> > the user or workload has forced it to a higher level. Replicate the same
> > behavior in SLPC by allowing the algorithm to use efficient frequency.
> > We had disabled it during boot due to concerns that it might break
> > kernel ABI for min frequency. However, this is not the case, since
> > SLPC will still abide by the (min,max) range limits and pcode forces
> > frequency to 0 anyways when GT is in C6.
> >
> > We also see much better perf numbers with benchmarks like glmark2 with
> > efficient frequency usage enabled.
> >
> > Fixes: 025cb07bebfa ("drm/i915/guc/slpc: Cache platform frequency limits")
> >
> > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
> I'm honestly surprised that our CI passed cleanly. What happens when user
> request both min and max < rpe?
>
> I'm sure that in this case GuC SLPC will put us to rpe ignoring our requests.
> Or is this good enough for the users expectation because of the soft limits
> showing the requested freq and we not asking to guc what it currently has as
> minimal?
>
> I just want to be sure that we are not causing any confusion for end users
> out there in the case they request some min/max below RPe and start seeing
> mismatches on the expectation because GuC is forcing the real min request
> to RPe.
>
> My suggestion is to ignore the RPe whenever we have a min request below it.
> So GuC respects our (and users) chosen min. And restore whenever min request
> is abobe rpe.

Yup, I have already sent a patch yesterday with that change, doesn't 
look like CI has run on it yet. This was the old version.

Thanks,

Vinay.

> Thanks,
> Rodrigo.
>
> > ---
> >   drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 52 ---------------------
> >   1 file changed, 52 deletions(-)
> >
> > 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 e1fa1f32f29e..4b824da3048a 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
> > @@ -137,17 +137,6 @@ static int guc_action_slpc_set_param(struct intel_guc *guc, u8 id, u32 value)
> >   	return ret > 0 ? -EPROTO : ret;
> >   }
> >   
> > -static int guc_action_slpc_unset_param(struct intel_guc *guc, u8 id)
> > -{
> > -	u32 request[] = {
> > -		GUC_ACTION_HOST2GUC_PC_SLPC_REQUEST,
> > -		SLPC_EVENT(SLPC_EVENT_PARAMETER_UNSET, 1),
> > -		id,
> > -	};
> > -
> > -	return intel_guc_send(guc, request, ARRAY_SIZE(request));
> > -}
> > -
> >   static bool slpc_is_running(struct intel_guc_slpc *slpc)
> >   {
> >   	return slpc_get_state(slpc) == SLPC_GLOBAL_STATE_RUNNING;
> > @@ -201,16 +190,6 @@ static int slpc_set_param(struct intel_guc_slpc *slpc, u8 id, u32 value)
> >   	return ret;
> >   }
> >   
> > -static int slpc_unset_param(struct intel_guc_slpc *slpc,
> > -			    u8 id)
> > -{
> > -	struct intel_guc *guc = slpc_to_guc(slpc);
> > -
> > -	GEM_BUG_ON(id >= SLPC_MAX_PARAM);
> > -
> > -	return guc_action_slpc_unset_param(guc, id);
> > -}
> > -
> >   static int slpc_force_min_freq(struct intel_guc_slpc *slpc, u32 freq)
> >   {
> >   	struct drm_i915_private *i915 = slpc_to_i915(slpc);
> > @@ -597,29 +576,6 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc)
> >   	return 0;
> >   }
> >   
> > -static int slpc_ignore_eff_freq(struct intel_guc_slpc *slpc, bool ignore)
> > -{
> > -	int ret = 0;
> > -
> > -	if (ignore) {
> > -		ret = slpc_set_param(slpc,
> > -				     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> > -				     ignore);
> > -		if (!ret)
> > -			return slpc_set_param(slpc,
> > -					      SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
> > -					      slpc->min_freq);
> > -	} else {
> > -		ret = slpc_unset_param(slpc,
> > -				       SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY);
> > -		if (!ret)
> > -			return slpc_unset_param(slpc,
> > -						SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ);
> > -	}
> > -
> > -	return ret;
> > -}
> > -
> >   static int slpc_use_fused_rp0(struct intel_guc_slpc *slpc)
> >   {
> >   	/* Force SLPC to used platform rp0 */
> > @@ -679,14 +635,6 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)
> >   
> >   	slpc_get_rp_values(slpc);
> >   
> > -	/* Ignore efficient freq and set min to platform min */
> > -	ret = slpc_ignore_eff_freq(slpc, true);
> > -	if (unlikely(ret)) {
> > -		i915_probe_error(i915, "Failed to set SLPC min to RPn (%pe)\n",
> > -				 ERR_PTR(ret));
> > -		return ret;
> > -	}
> > -
> >   	/* Set SLPC max limit to RP0 */
> >   	ret = slpc_use_fused_rp0(slpc);
> >   	if (unlikely(ret)) {
> > --
> > 2.35.1