[Intel-gfx] drm/i915/slpc: Update freq min/max softlimits

[Kamble, Sagar A]

On 8/24/2016 2:07 PM, Chris Wilson wrote:
> On Sun, Aug 21, 2016 at 09:39:17PM +0530, Kamble, Sagar A wrote:
>>
>> On 8/21/2016 2:09 PM, Chris Wilson wrote:
>>> On Sun, Aug 21, 2016 at 11:39:22AM +0530, Kamble, Sagar A wrote:
>>>> On 8/20/2016 1:32 PM, Chris Wilson wrote:
>>>>> On Sat, Aug 20, 2016 at 10:39:25AM +0530, Sagar Arun Kamble wrote:
>>>>>> +	obj = dev_priv->guc.slpc.vma->obj;
>>>>>> +	if (obj) {
>>>>> OOPS.
>>>> Fixed in next series.
>>>>>> +		intel_slpc_query_task_state(dev_priv);
>>>>>> +
>>>>>> +		page = i915_gem_object_get_page(obj, 0);
>>>>>> +		if (page)
>>>>>> +			pv = kmap_atomic(page);
>>>>>> +	}
>>>>>> +
>>>>>> +	if (pv) {
>>>>>> +		data = *(struct slpc_shared_data *) pv;
>>>>>> +		kunmap_atomic(pv);
>>>>> Can kmap_atomic return zero?
>>>> Fixed in next series.
>>>>>> +
>>>>>> +		/*
>>>>>> +		 * TODO: Define separate variables for slice and unslice
>>>>>> +		 *	 frequencies for driver state variable.
>>>>>> +		 */
>>>>>> +		dev_priv->rps.max_freq_softlimit =
>>>>>> +				data.task_state_data.freq_unslice_max;
>>>>>> +		dev_priv->rps.min_freq_softlimit =
>>>>>> +				data.task_state_data.freq_unslice_min;
>>>>> These are user values, you do not get to arbitrarily rewrite them.
>>>>>
>>>>> You control dev_priv->rps.[min|max]_freq.
>>>> With SLPC, GuC firmware SLPC S/W requested frequency be operated in
>>>> the softlimits analogous to
>>>> Host softlimits. Limits might be different with SLPC and can be
>>>> controlled through regular interfaces.
>>>> dev_priv->rps.[min|max]_freq are HW Min/Max.
>>> Exactly. The soft limits are *only* set by the user. They are not to
>>> modified by the driver. (The caveat would be a dynamic update of the hw
>>> range, but that too should never be required.)
>>> -Chris
>> This initialization is similar to following from intel_init_gt_powersave
>>          dev_priv->rps.max_freq_softlimit = dev_priv->rps.max_freq;
>>          dev_priv->rps.min_freq_softlimit = dev_priv->rps.min_freq;
>> I assume min_freq is hw min(RPn). With SLPC, min_freq(RPn) will not
>> be requested.
>> SLPC operating range today is (>Rpe, Rp0) so I wanted user to know
>> the min_softlimit being initialized by SLPC by default.
> Hmm, my mistake here was thinking this was more than a one off. Setting
> the initial soft (user) range on startup is fine. Continually changing
> them after userspace registration is not. (The value the user writes
> into the limit is what should be read back - without very good reason,
> such as the hard limits changing).
> -Chris
I interpreted the rewrite differently, thinking SLPC should not write it 
post init_gt_powersave.
As discussed, I will change this to make sure SLPC init does not touch 
these values once it is
initialized in the load path.
>