[Intel-gfx] drm/i915/slpc: Update current requested frequency

[Kamble, Sagar A]

On 8/20/2016 1:45 PM, Chris Wilson wrote:
> On Sat, Aug 20, 2016 at 10:39:10AM +0530, Sagar Arun Kamble wrote:
>> From: Tom O'Rourke <Tom.O'Rourke at intel.com>
>>
>> When SLPC is controlling requested frequency, the rps.cur_freq
>> value is not used to make the frequency request.
>>
>> Before using rps.cur_freq in sysfs or debugfs, read
>> requested frequency from register to get the value
>> most recently requested by SLPC firmware.
>>
>> v2: replace HAS_SLPC with intel_slpc_active (Paulo)
>> v3: Avoid magic numbers (Nick)
>>      Use a function for repeated code (Jon)
>>
>> Signed-off-by: Tom O'Rourke <Tom.O'Rourke at intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c | 6 ++++++
>>   drivers/gpu/drm/i915/i915_drv.h     | 5 +++++
>>   drivers/gpu/drm/i915/i915_reg.h     | 1 +
>>   drivers/gpu/drm/i915/i915_sysfs.c   | 8 ++++++++
>>   4 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 01ae5ee..a99a3f6 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1110,6 +1110,9 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
>>   
>>   	intel_runtime_pm_get(dev_priv);
>>   
>> +	if (intel_slpc_active(dev_priv))
>> +		dev_priv->rps.cur_freq = gen9_read_requested_freq(dev_priv);
> Do not alter cur_freq here, as we print out RPNSWEQ and updating
> cur_freq defeats the purpose of showing the internal value vs the hw
> value.
>
> Instead add "SLPC active" to the output.
Fixed in the next series.
>
>> +
>>   	if (IS_GEN5(dev)) {
>>   		u16 rgvswctl = I915_READ16(MEMSWCTL);
>>   		u16 rgvstat = I915_READ16(MEMSTAT_ILK);
>> @@ -2372,6 +2375,9 @@ static int i915_rps_boost_info(struct seq_file *m, void *data)
>>   	struct drm_i915_private *dev_priv = to_i915(dev);
>>   	struct drm_file *file;
> if (intel_slpc_active(dev_priv))
> 	return -ENODEV;
Fixed in the next series.
>>   
>> +	if (intel_slpc_active(dev_priv))
>> +		dev_priv->rps.cur_freq = gen9_read_requested_freq(dev_priv);
>> +
>>   	seq_printf(m, "RPS enabled? %d\n", dev_priv->rps.enabled);
>>   	seq_printf(m, "GPU busy? %s [%x]\n",
>>   		   yesno(dev_priv->gt.awake), dev_priv->gt.active_engines);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 764fad0..fcd2e98 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3913,4 +3913,9 @@ bool i915_memcpy_from_wc(void *dst, const void *src, unsigned long len);
>>   	__T;								\
>>   })
>>   
>> +static inline u8 gen9_read_requested_freq(struct drm_i915_private *dev_priv)
>> +{
>> +	return (u8) GEN9_GET_FREQUENCY(I915_READ(GEN6_RPNSWREQ));
>> +}
Removed in the next series.
> Move to sysfs and look carefully at what you wrote.
>
>>   #endif
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index d4adf28..1654245 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6997,6 +6997,7 @@ enum {
>>   #define   GEN6_FREQUENCY(x)			((x)<<25)
>>   #define   HSW_FREQUENCY(x)			((x)<<24)
>>   #define   GEN9_FREQUENCY(x)			((x)<<23)
>> +#define   GEN9_GET_FREQUENCY(x)			((x)>>23)
>>   #define   GEN6_OFFSET(x)			((x)<<19)
>>   #define   GEN6_AGGRESSIVE_TURBO			(0<<15)
>>   #define GEN6_RC_VIDEO_FREQ			_MMIO(0xA00C)
>> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
>> index f1ffde7..8404816 100644
>> --- a/drivers/gpu/drm/i915/i915_sysfs.c
>> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
>> @@ -302,6 +302,14 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
>>   	struct drm_device *dev = minor->dev;
>>   	struct drm_i915_private *dev_priv = to_i915(dev);
>>   
>> +	if (intel_slpc_active(dev_priv)) {
>> +		intel_runtime_pm_get(dev_priv);
> Use get_if_in_use and just show a stale value when the hw is asleep
> would be my preference. cur_freq is just our request, act_freq is the
> actual hw value.
With cur_freq not making sense currently with SLPC, I am removing this 
altogether and adding new sysfs interface
for knowing HW requested frequency which will be ideally SLPC requested.
>
>> +		mutex_lock(&dev_priv->rps.hw_lock);
> Useless mutex.
>
>> +		dev_priv->rps.cur_freq = gen9_read_requested_freq(dev_priv);
>> +		mutex_unlock(&dev_priv->rps.hw_lock);
>> +		intel_runtime_pm_put(dev_priv);
>> +	}
>> +
>>   	return snprintf(buf, PAGE_SIZE, "%d\n",
>>   			intel_gpu_freq(dev_priv,
>>   				       dev_priv->rps.cur_freq));
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx