Re: [Intel-gfx] [RFC 4/4] drm/i915: Expose RPS thresholds in sysfs

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 19/05/2023 21:56, Rodrigo Vivi wrote:
> On Fri, Apr 28, 2023 at 09:44:53AM +0100, Tvrtko Ursulin wrote:
>>
>> On 28/04/2023 09:14, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> User feedback indicates significant performance gains are possible in
>>> specific games with non default RPS up/down thresholds.
>>>
>>> Expose these tunables via sysfs which will allow users to achieve best
>>> performance when running games and best power efficiency elsewhere.
>>>
>>> Note this patch supports non GuC based platforms only.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> References: https://gitlab.freedesktop.org/drm/intel/-/issues/8389
>>
>> [snip]
>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
>>> index a5a7315f5ace..f790e81546ff 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
>>> @@ -2588,7 +2588,12 @@ static int rps_set_threshold(struct intel_rps *rps, u8 *threshold, u8 val)
>>>    	ret = mutex_lock_interruptible(&rps->lock);
>>>    	if (ret)
>>>    		return ret;
>>> -	*threshold = val;
>>> +	if (*threshold != val) {
>>> +		*threshold = val;
>>> +		intel_rps_set(rps, clamp(rps->cur_freq,
>>> +					 rps->min_freq_softlimit,
>>> +					 rps->max_freq_softlimit));
>>> +	}
>>>    	mutex_unlock(&rps->lock);
>>>    	return 0;
>>
>> This hunk belongs to a previous patch - moved locally.
> 
> I probably missed something then becuase I didn't miss this in any
> of the previous patches. To the point that this looked like a new
> separated patch.

Depends how you look at it. 3/4 adds a helper which changes the 
threshold, but it was only updating the sw state and so when those 
values would actually get written into registers (derived) would depend 
on the workload. So I thought it was important to move the call to 
intel_rps_set, which actually will trigger hw reprogramming, into the 
patch which claims to be adding the helper.

Note v2 of the helper (just sent) which actually fixes this further by 
forcing this re-programming to happen.

Regards,

Tvrtko