Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Leonard Crestez]

On 29.07.2019 04:49, Chanwoo Choi wrote:
> On 19. 7. 23. ìí 9:20, Artur ÅwigoÅ wrote:
>> This patch adds interconnect functionality to the exynos-bus devfreq
>> driver.
>>
>> The devfreq target() callback provided by exynos-bus now selects either the
>> frequency calculated by the devfreq governor or the frequency requested via
>> the interconnect API for the given node, whichever is higher.
> 
> Basically, I agree to support the QoS requirement between devices.
> But, I think that need to consider the multiple cases.
> 
> 1. When changing the devfreq governor by user,
> For example of the connection between bus_dmc/leftbus/display on patch8,
> there are possible multiple cases with various devfreq governor
> which is changed on the runtime by user through sysfs interface.
> 
> If users changes the devfreq governor as following:
> Before,
> - bus_dmc (simple_ondemand, available frequency 100/200/300/400 MHz)
> --> bus_leftbus(simple_ondemand, available frequency 100/200/300/400 MHz)
> ----> bus_display(passive)
> 
> After changed governor of bus_dmc,
> if the min_freq by interconnect requirement is 400Mhz,
> - bus_dmc (powersave) : min_freq and max_freq and cur_freq is 100MHz
> --> bus_leftbus(simple_ondemand) : cur_freq is 400Mhz
> ----> bus_display(passive)
> 
> The final frequency is 400MHz of bus_dmc
> even if the min_freq/max_freq/cur_freq is 100MHz.
> It cannot show the correct min_freq/max_freq through
> devfreq sysfs interface.
> 
> 
> 2. When disabling the some frequency by devfreq-thermal throttling,
> This patch checks the min_freq of interconnect requirement
> in the exynos_bus_target() and exynos_bus_passive_target().
> Also, it cannot show the correct min_freq/max_freq through
> devfreq sysfs interface.
> 
> For example of bus_dmc bus,
> - The available frequencies are 100MHz, 200MHz, 300MHz, 400MHz
> - Disable 400MHz by devfreq-thermal throttling
> - min_freq is 100MHz
> - max_freq is 300MHz
> - min_freq of interconnect is 400MHz
> 
> In result, the final frequency is 400MHz by exynos_bus_target()
> There are no problem for working. But, the user cannot know
> reason why cur_freq is 400MHz even if max_freq is 300MHz.
> 
> Basically, update_devfreq() considers the all constraints
> of min_freq/max_freq to decide the proper target frequency.

I think that applying the interconnect min_freq via dev_pm_qos can help 
with many of these concerns: update_devfreq controls all the min/max 
handling, sysfs is accurate and better decisions can be made regarding 
thermal. Enforcing constraints in the core is definitely better.

This wouldn't even be a very big change, you don't need to actually move 
the interconnect code outside of devfreq. Just make every devfreq/icc 
node register a dev_pm_qos_request for itself during registration and 
call dev_pm_qos_update_request inside exynos_bus_icc_set.

See: https://patchwork.kernel.org/patch/11084279/

--
Regards,
Leonard