On Wed, 05 Apr 2023 13:12:29 -0700, Rodrigo Vivi wrote: > > 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. This I believe not possible in all cases. Say the freq's are set through sysfs first and the workload starts later. In this case RPe will probably start changing after the workload starts, not when freq's are set in sysfs. > 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. >From what I saw the following bugs: https://gitlab.freedesktop.org/drm/intel/-/issues/6806 https://gitlab.freedesktop.org/drm/intel/-/issues/6786 and the following patches in response to these bugs (as well as the discussion on these patches): https://patchwork.freedesktop.org/series/111282/ https://patchwork.freedesktop.org/series/110574/ were all due to the fact that the max freq is not obeyed after RPe is enabled. These patches were never merged but I will now try to submit them again after this ignore efficient freq patch gets reviewed and merged. Thanks. -- Ashutosh > > > > 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.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);
