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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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@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);