Re: [Intel-gfx] [PATCH] drm/i915/guc/slpc: Use non-blocking H2G for waitboost

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

On Wed, 22 Jun 2022 13:30:23 -0700, Belgaumkar, Vinay wrote:
> On 6/21/2022 5:26 PM, Dixit, Ashutosh wrote:
> > On Sat, 14 May 2022 23:05:06 -0700, Vinay Belgaumkar wrote:
> > The issue I have is what happens when we de-boost (restore min freq to its
> > previous value in intel_guc_slpc_dec_waiters()). It would seem that that
> > call is fairly important to get the min freq down when there are no pending
> > requests. Therefore what do we do in that case?
> >
> > This is the function:
> >
> > void intel_guc_slpc_dec_waiters(struct intel_guc_slpc *slpc)
> > {
> >          mutex_lock(&slpc->lock);
> >          if (atomic_dec_and_test(&slpc->num_waiters))
> >                  slpc_force_min_freq(slpc, slpc->min_freq_softlimit);
> >          mutex_unlock(&slpc->lock);
> > }
> >
> >
> > 1. First it would seem that at the minimum we need a similar drm_notice()
> >     in intel_guc_slpc_dec_waiters(). That would mean we need to put the
> >     drm_notice() back in slpc_force_min_freq() (replacing
> >     i915_probe_error()) rather than in slpc_boost_work() above?
> Sure.
> >
> > 2. Further, if de-boosting is important then maybe as was being discussed
> >     in v1 of this patch (see the bottom of
> >     https://patchwork.freedesktop.org/patch/485004/?series=103598&rev=1) do
> >     we need to use intel_guc_send_busy_loop() in the
> >     intel_guc_slpc_dec_waiters() code path?
>
> Using a busy_loop here would essentially be the same as blocking, right?

Well blocking waits for a response from GuC (so all previous requests need
to be processed by GuC) whereas busy_loop() just waits for space to be
available at the back of the queue (so just a few, or maybe just one,
request have to be processed by GuC).

> And it could still fail/timeout with blocking as well (which is the problem
> we are trying to solve here).

intel_guc_send_busy_loop() has an infinite wait without a drm_err()!! :)

> De-boosting is important, but in the worst case scenario, lets say this
> request was not processed by GuC. This would happen only if the system
> were really busy, which would mean there is a high likelihood we would
> boost/de-boost again anyways and it would probably go through at that
> point.

Not sure of this. The system was busy but now might have gone idle which is
why we are trying to de-boost. But GuC queue might still be full so we may
drop the de-boost request. Or if the system has gone really idle there will
be space in the GuC queue.

Also the problem with intel_guc_send_busy_loop() is that it just has a
sleep in it, so others might be adding requests in the GuC queue while
busy_loop() was sleeping (to avoid such situations we'd need a SW queue in
front of the real GuC queue).

So I am ok if we don't want to add intel_guc_send_busy_loop() for now and
"wait and watch". Unless John suggests otherwise since I don't have any
idea how likely is this to happen. If we change drm_notice to drm_err the
CI will quick tell us if this happening.

Anyway, so at least let's move drm_notice (or drm_err) into
slpc_force_min_freq() and I can ok the patch. Thanks.

> > At least we need to do 1. But for 2. we might as well just put
> > intel_guc_send_busy_loop() in guc_action_slpc_set_param_nb()? In both cases
> > (boost and de-boost) intel_guc_send_busy_loop() would be called from a work
> > item so looks doable (the way we were previously doing the blocking call
> > from the two places). Thoughts?
> >
> > Thanks.
> > --
> > Ashutosh