On Wed, Apr 05, 2023 at 12:42:30PM -0700, Dixit, Ashutosh wrote:
> 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?
that is not how guc efficient freq selection works. (unless my memory is
tricking me right now.)
So, if we select a min that is between RPe and RP0, guc will respect and
use the selected min. So we don't need to disable guc selection of the
efficient.
This is not true when we select a very low min like RPn. If we select RPn
as min and guc efficient freq selection is enabled guc will simply ignore
our request. So the only way to give the user what is asked, is to also
disable guc's efficient freq selection. (I probably confused you in the
previous email because I used 'RP0' when I meant 'RPn'. I hope it gets
clear now).
>
> * Also isn't RPe dynamic, so we can't say RPe == rp1 when using in KMD?
Oh... yeap, this is an issue indeed. Specially with i915 where we have
the soft values cached instead of asking guc everytime.
That's a good point. The variance is not big, but we will hit corner cases.
One way is to keep checking and updating everytime a sysfs is touched.
Other way is do what you are suggesting and let's just accept and deal
with the reality that is: "we cannot guarantee a min freq selection if user
doesn't disable the efficient freq selection".
>
> * 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.
Oh! so it was my bad memory indeed and everything is the other way around?
But I just looked to Xe code, my most recent memory, and I just needed
to toggle the efficient freq off on the case that I mentioned, when min
selection is below the efficient one. With that all the API expectation
that I coded in IGT works neatly.
>
> 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?
what cases it stays broken? This is why we need the IGT tests for all the
API behavior in place.
>
> > 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.
Yeap, the IGT needs to come with whatever we concluded here and we need to
stick with that afterwards, so let's think with care.
Vinay, Ashutosh's strongest argument is the variable RPe. Do you have thoughts
on that?
>
> 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.
in case I let you confused here, what I meant was RPn, not RP0.
> >
> > With this in mind, and having checked the code:
> >
> > Reviewed-by: Rodrigo Vivi <rodrigo.v...@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);
