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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Mar 31, 2023 at 08:11:29PM -0700, Dixit, Ashutosh wrote:
> On Fri, 31 Mar 2023 19:00:49 -0700, Vinay Belgaumkar wrote:
> >
> 
> Hi Vinay,
> 
> > @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val)
> >	    val > slpc->max_freq_softlimit)
> >		return -EINVAL;
> >
> > +	/* Ignore efficient freq if lower min freq is requested */
> > +	ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq);
> > +	if (ret)
> > +		goto out;
> > +
> 
> I don't agree with this. If we are now providing an interface explicitly to
> ignore RPe, that should be /only/ way to ignore RPe. There should be no
> other "under the hood" ignoring of RPe. In other words, ignoring RPe should
> be minimized unless explicitly requested.
> 
> I don't clearly understand why this was done previously but it makes even
> less sense to me now after this patch.

well, I had suggested this previously. And just because without this we would
be breaking API expectations.

When user selects a minimal frequency it expect that to stick. But with the
efficient freq enabled in guc if minimal is less than the efficient one,
this request is likely ignored.

Well, even worse is that we are actually caching the request in the soft values.
So we show a minimal, but the hardware without any workload is operating at
efficient.

So, the thought process was: 'if user requested a very low minimal, we give them
the minimal requested, even if that means to disable the efficient freq.'

So, that was introduced to avoid API breakage. Removing it now would mean
breaking API. (Not sure if the IGT tests for the API got merged already,
but think that as the API contract).

But I do agree with you that having something selected from multiple places
also has the potential to cause some miss-expectations. So I was thinking
about multiple even orders where the user select the RP0 as minimal, then
enable the efficient or vice versa, but I couldn't think of a bad case.
Or at least not as bad as the user asking to get RP0 as minimal and only
getting RPe back.

With this in mind, and having checked the code:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

But I won't push this immediately because I'm still open to hear another
side/angle.

> 
> Thanks.
> --
> Ashutosh
> 
> 
> >	/* Need a lock now since waitboost can be modifying min as well */
> >	mutex_lock(&slpc->lock);
> >	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> >
> > -	/* Ignore efficient freq if lower min freq is requested */
> > -	ret = slpc_set_param(slpc,
> > -			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> > -			     val < slpc->rp1_freq);
> > -	if (ret) {
> > -		guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n",
> > -				ERR_PTR(ret));
> > -		goto out;
> > -	}
> > -
> >	ret = slpc_set_param(slpc,
> >			     SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
> >			     val);