[Intel-gfx] [PATCH v3] drm/i915/slpc: Optmize waitboost for SLPC

[Belgaumkar, Vinay]

On 10/20/2022 4:36 PM, Dixit, Ashutosh wrote:
> On Thu, 20 Oct 2022 13:16:00 -0700, Belgaumkar, Vinay wrote:
>> On 10/20/2022 11:33 AM, Dixit, Ashutosh wrote:
>>> On Wed, 19 Oct 2022 17:29:44 -0700, Vinay Belgaumkar wrote:
>>> Hi Vinay,
>>>
>>>> Waitboost (when SLPC is enabled) results in a H2G message. This can result
>>>> in thousands of messages during a stress test and fill up an already full
>>>> CTB. There is no need to request for RP0 if GuC is already requesting the
>>>> same.
>>> But how are we sure that the freq will remain at RP0 in the future (when
>>> the waiting request or any requests which are ahead execute)?
>>>
>>> In the current waitboost implementation, set_param is sent to GuC ahead of
>>> the waiting request to ensure that the freq would be max when this waiting
>>> request executed on the GPU and the freq is kept at max till this request
>>> retires (considering just one waiting request). How can we ensure this if
>>> we don't send the waitboost set_param to GuC?
>> There is no way to guarantee the frequency will remain at RP0 till the
>> request retires. As a theoretical example, lets say the request boosted
>> freq to RP0, but a user changed min freq using sysfs immediately after.
> That would be a bug. If waitboost is in progress and in the middle user
> changed min freq, I would expect the freq to revert to the new min only
> after the waitboost phase was over.

The problem here is that GuC is unaware of this "boosting" phenomenon. 
Setting the min_freq_softlimit as well to boost when we send a boost 
request might help with this issue.

>
> In any case, I am not referring to this case. Since FW controls the freq
> there is nothing preventing FW to change the freq unless we raise min to
> max which is what waitboost does.
Ok, so maybe the solution here is to check if min_softlimit is already 
at boost freq, as it tracks the min freq changes. That should take care 
of server parts automatically as well.
>
>> Waitboost is done by a pending request to "hurry" the current requests. If
>> GT is already at boost frequency, that purpose is served.
> FW can bring the freq down later before the waiting request is scheduled.
>> Also, host algorithm already has this optimization as well.
> Host turbo is different from SLPC. Host turbo controls the freq algorithm
> so it knows freq will not come down till it itself brings the freq
> down. Unlike SLPC where FW is controling the freq. Therefore host turbo
> doesn't ever need to do a MMIO read but only needs to refer to its own
> state (rps->cur_freq etc.).
True. Host algorithm has a periodic timer where it updates frequency. 
Here, it checks num_waiters and sets client_boost every time that is 
non-zero.
>>> I had assumed we'll do this optimization for server parts where min is
>>> already RP0 in which case we can completely disable waitboost. But this
>>> patch is something else.

Hopefully the softlimit changes above will help with client and server.

Thanks,

Vinay.

> Thanks.
> --
> Ashutosh
>
>>>> v2: Add the tracing back, and check requested freq
>>>> in the worker thread (Tvrtko)
>>>> v3: Check requested freq in dec_waiters as well
>>>>
>>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gt/intel_rps.c         |  3 +++
>>>>    drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 14 +++++++++++---
>>>>    2 files changed, 14 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
>>>> index fc23c562d9b2..18b75cf08d1b 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
>>>> @@ -1016,6 +1016,9 @@ void intel_rps_boost(struct i915_request *rq)
>>>> 		if (rps_uses_slpc(rps)) {
>>>> 			slpc = rps_to_slpc(rps);
>>>>
>>>> +			GT_TRACE(rps_to_gt(rps), "boost fence:%llx:%llx\n",
>>>> +				 rq->fence.context, rq->fence.seqno);
>>>> +
>>>> 			/* Return if old value is non zero */
>>>> 			if (!atomic_fetch_inc(&slpc->num_waiters))
>>>> 				schedule_work(&slpc->boost_work);
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>>>> index b7cdeec44bd3..9dbdbab1515a 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>>>> @@ -227,14 +227,19 @@ static int slpc_force_min_freq(struct intel_guc_slpc *slpc, u32 freq)
>>>>    static void slpc_boost_work(struct work_struct *work)
>>>>    {
>>>> 	struct intel_guc_slpc *slpc = container_of(work, typeof(*slpc), boost_work);
>>>> +	struct intel_rps *rps = &slpc_to_gt(slpc)->rps;
>>>> 	int err;
>>>>
>>>> 	/*
>>>> 	 * Raise min freq to boost. It's possible that
>>>> 	 * this is greater than current max. But it will
>>>> 	 * certainly be limited by RP0. An error setting
>>>> -	 * the min param is not fatal.
>>>> +	 * the min param is not fatal. No need to boost
>>>> +	 * if we are already requesting it.
>>>> 	 */
>>>> +	if (intel_rps_get_requested_frequency(rps) == slpc->boost_freq)
>>>> +		return;
>>>> +
>>>> 	mutex_lock(&slpc->lock);
>>>> 	if (atomic_read(&slpc->num_waiters)) {
>>>> 		err = slpc_force_min_freq(slpc, slpc->boost_freq);
>>>> @@ -728,6 +733,7 @@ int intel_guc_slpc_set_boost_freq(struct intel_guc_slpc *slpc, u32 val)
>>>>
>>>>    void intel_guc_slpc_dec_waiters(struct intel_guc_slpc *slpc)
>>>>    {
>>>> +	struct intel_rps *rps = &slpc_to_gt(slpc)->rps;
>>>> 	/*
>>>> 	 * Return min back to the softlimit.
>>>> 	 * This is called during request retire,
>>>> @@ -735,8 +741,10 @@ void intel_guc_slpc_dec_waiters(struct intel_guc_slpc *slpc)
>>>> 	 * set_param fails.
>>>> 	 */
>>>> 	mutex_lock(&slpc->lock);
>>>> -	if (atomic_dec_and_test(&slpc->num_waiters))
>>>> -		slpc_force_min_freq(slpc, slpc->min_freq_softlimit);
>>>> +	if (atomic_dec_and_test(&slpc->num_waiters)) {
>>>> +		if (intel_rps_get_requested_frequency(rps) != slpc->min_freq_softlimit)
>>>> +			slpc_force_min_freq(slpc, slpc->min_freq_softlimit);
>>>> +	}
>>>> 	mutex_unlock(&slpc->lock);
>>>>    }
>>>>
>>>> --
>>>> 2.35.1
>>>>