[Intel-gfx] [PATCH] drm/i915/guc/slpc: Restore efficient freq earlier

[Belgaumkar, Vinay]

On 7/21/2023 3:08 PM, Belgaumkar, Vinay wrote:
>
> On 7/21/2023 2:23 PM, Rodrigo Vivi wrote:
>> On Fri, Jul 21, 2023 at 01:44:34PM -0700, Belgaumkar, Vinay wrote:
>>> On 7/21/2023 1:41 PM, Rodrigo Vivi wrote:
>>>> On Fri, Jul 21, 2023 at 11:03:49AM -0700, Vinay Belgaumkar wrote:
>>>>> This should be done before the soft min/max frequencies are restored.
>>>>> When we disable the "Ignore efficient frequency" flag, GuC does not
>>>>> actually bring the requested freq down to RPn.
>>>>>
>>>>> Specifically, this scenario-
>>>>>
>>>>> - ignore efficient freq set to true
>>>>> - reduce min to RPn (from efficient)
>>>>> - suspend
>>>>> - resume (includes GuC load, restore soft min/max, restore 
>>>>> efficient freq)
>>>>> - validate min freq has been resored to RPn
>>>>>
>>>>> This will fail if we didn't first restore(disable, in this case) 
>>>>> efficient
>>>>> freq flag before setting the soft min frequency.
>>>> that's strange. so guc is returning the rpe when we request the min 
>>>> freq
>>>> during the soft config?
>>>>
>>>> we could alternatively change the soft config to actually get the min
>>>> and not be tricked by this.
>>>>
>>>> But also the patch below doesn't hurt.
>>>>
>>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>> (Although I'm still curious and want to understand exactly why
>>>> the soft min gets messed up when we don't tell guc to ignore the
>>>> efficient freq beforehand. Please help me to understand.)
>>> The soft min does not get messed up, but GuC keeps requesting RPe 
>>> even after
>>> disabling efficient freq. (unless we manually set min freq to RPn AFTER
>>> disabling efficient).
>> so it looks to me that the right solution would be to ensure that 
>> everytime
>> that we disable the efficient freq we make sure to also set the mim 
>> freq to RPn,
>> no?!
>
> Hmm, may not be applicable every time. What if someone disables 
> efficient frequency while running a workload or with frequency fixed 
> to 800, for example?

I'll take that back, it should not matter. GuC will not change it's 
request just because we switched min lower. I will resend the patch with 
the min setting as well.

Thanks,

Vinay.

>
> Thanks,
>
> Vinay.
>
>>
>>> Thanks,
>>>
>>> Vinay.
>>>
>>>>
>>>>> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8736
>>>>> Fixes: 55f9720dbf23 ("drm/i915/guc/slpc: Provide sysfs for 
>>>>> efficient freq")
>>>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 6 +++---
>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> 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 ee9f83af7cf6..f16dff7c3185 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
>>>>> @@ -743,6 +743,9 @@ int intel_guc_slpc_enable(struct 
>>>>> intel_guc_slpc *slpc)
>>>>>        intel_guc_pm_intrmsk_enable(slpc_to_gt(slpc));
>>>>> +    /* Set cached value of ignore efficient freq */
>>>>> +    intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq);
>>>>> +
>>>>>        slpc_get_rp_values(slpc);
>>>>>        /* Handle the case where min=max=RPmax */
>>>>> @@ -765,9 +768,6 @@ int intel_guc_slpc_enable(struct 
>>>>> intel_guc_slpc *slpc)
>>>>>        /* Set cached media freq ratio mode */
>>>>>        intel_guc_slpc_set_media_ratio_mode(slpc, 
>>>>> slpc->media_ratio_mode);
>>>>> -    /* Set cached value of ignore efficient freq */
>>>>> -    intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq);
>>>>> -
>>>>>        return 0;
>>>>>    }
>>>>> -- 
>>>>> 2.38.1
>>>>>