Re: [Intel-gfx] [PATCH 3/4] drm/i915: Add helpers for managing rps thresholds

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

On 23/05/2023 00:09, Andi Shyti wrote:
> Hi Tvrtko,
> 
> On Mon, May 22, 2023 at 12:59:27PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> In preparation for exposing via sysfs add helpers for managing rps
>> thresholds.
>>
>> v2:
>>   * Force sw and hw re-programming on threshold change.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@kernel.org>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_rps.c | 54 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/gt/intel_rps.h |  4 +++
>>   2 files changed, 58 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
>> index 333abc8f7ecb..afde601a6111 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
>> @@ -16,7 +16,9 @@
>>   #include "intel_gt.h"
>>   #include "intel_gt_clock_utils.h"
>>   #include "intel_gt_irq.h"
>> +#include "intel_gt_pm.h"
>>   #include "intel_gt_pm_irq.h"
>> +#include "intel_gt_print.h"
>>   #include "intel_gt_regs.h"
>>   #include "intel_mchbar_regs.h"
>>   #include "intel_pcode.h"
>> @@ -2574,6 +2576,58 @@ int intel_rps_set_min_frequency(struct intel_rps *rps, u32 val)
>>   		return set_min_freq(rps, val);
>>   }
>>   
>> +u8 intel_rps_get_up_threshold(struct intel_rps *rps)
>> +{
>> +	return rps->power.up_threshold;
>> +}
>> +
>> +static int rps_set_threshold(struct intel_rps *rps, u8 *threshold, u8 val)
>> +{
>> +	int ret;
>> +
>> +	if (val > 100)
>> +		return -EINVAL;
>> +
>> +	ret = mutex_lock_interruptible(&rps->lock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (*threshold == val)
>> +		goto out_unlock;
>> +
>> +	*threshold = val;
>> +
>> +	/* Force reset. */
>> +	rps->last_freq = -1;
>> +	mutex_lock(&rps->power.mutex);
>> +	rps->power.mode = -1;
>> +	mutex_unlock(&rps->power.mutex);
>> +
>> +	intel_rps_set(rps, clamp(rps->cur_freq,
>> +				 rps->min_freq_softlimit,
>> +				 rps->max_freq_softlimit));
> 
> why are you resetting here?

I want to ensure the next calls to rps_set go past the "if (val == 
rps->last_freq)" and "if (new_power == rps->power.mode)" checks (second 
one via gen6_rps_set_thresholds->rps_set_power" so new values are 
immediately programmed into the hardware and sw state reset and 
re-calculated.

Regards,

Tvrtko