On Wed, 05 Apr 2023 06:57:42 -0700, Rodrigo Vivi wrote:
>
Hi Rodrigo,
> 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.'
Hmm, I understand this even less now :)
* Why is RPe ignored when min < RPe? Since the freq can be between min and
max? Shouldn't the condition be min > RPe, that is turn RPe off if min
higher that RPe is requested?
* Also isn't RPe dynamic, so we can't say RPe == rp1 when using in KMD?
* Finally, we know that enabling RPe broke the kernel freq API because RPe
could go over max_freq. So it is actually the max freq which is not
obeyed after RPe is enabled.
So we ignore RPe in some select cases (which also I don't understand as
mentioned above but maybe I am missing something) to claim that we are
obeying the freq API, but let the freq API stay broken in other cases?
> 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).
I think we should take this patch as an opportunity to fix this and give
the user a clean interface to ignore RPe and remove this other implicit way
to ignore RPe. All IGT changes are unmerged at present.
Thanks.
--
Ashutosh
>
> 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@xxxxxxxxx>
>
> 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);
