Re: [PATCH v4 20/24] PM / devfreq: tegra30: Optimize upper average watermark selection

[Chanwoo Choi]

On 19. 7. 20. ìì 2:52, Dmitry Osipenko wrote:
> 19.07.2019 9:11, Chanwoo Choi ÐÐÑÐÑ:
>> On 19. 7. 19. ìí 3:09, Chanwoo Choi wrote:
>>> On 19. 7. 19. ìì 11:21, Dmitry Osipenko wrote:
>>>> Ð Fri, 19 Jul 2019 11:06:05 +0900
>>>> Chanwoo Choi <cw00.choi@xxxxxxxxxxx> ÐÐÑÐÑ:
>>>>
>>>>> On 19. 7. 19. ìì 10:59, Dmitry Osipenko wrote:
>>>>>> Ð Fri, 19 Jul 2019 10:36:30 +0900
>>>>>> Chanwoo Choi <cw00.choi@xxxxxxxxxxx> ÐÐÑÐÑ:
>>>>>>   
>>>>>>> On 19. 7. 8. ìì 7:32, Dmitry Osipenko wrote:  
>>>>>>>> I noticed that CPU may be crossing the dependency threshold very
>>>>>>>> frequently for some workloads and this results in a lot of
>>>>>>>> interrupts which could be avoided if MCALL client is keeping
>>>>>>>> actual EMC frequency at a higher rate.
>>>>>>>>
>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>>>>>>> ---
>>>>>>>>  drivers/devfreq/tegra30-devfreq.c | 23 ++++++++++++++++++-----
>>>>>>>>  1 file changed, 18 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/devfreq/tegra30-devfreq.c
>>>>>>>> b/drivers/devfreq/tegra30-devfreq.c index
>>>>>>>> c3cf87231d25..4d582809acb6 100644 ---
>>>>>>>> a/drivers/devfreq/tegra30-devfreq.c +++
>>>>>>>> b/drivers/devfreq/tegra30-devfreq.c @@ -314,7 +314,8 @@ static
>>>>>>>> void tegra_actmon_get_lower_upper(struct tegra_devfreq *tegra, }
>>>>>>>>  
>>>>>>>>  static void tegra_devfreq_update_avg_wmark(struct tegra_devfreq
>>>>>>>> *tegra,
>>>>>>>> -					   struct
>>>>>>>> tegra_devfreq_device *dev)
>>>>>>>> +					   struct
>>>>>>>> tegra_devfreq_device *dev,
>>>>>>>> +					   unsigned long freq)
>>>>>>>>  {
>>>>>>>>  	unsigned long avg_threshold, lower, upper;
>>>>>>>>  
>>>>>>>> @@ -323,6 +324,15 @@ static void
>>>>>>>> tegra_devfreq_update_avg_wmark(struct tegra_devfreq *tegra,
>>>>>>>> avg_threshold = dev->config->avg_dependency_threshold;
>>>>>>>> avg_threshold = avg_threshold * ACTMON_SAMPLING_PERIOD; 
>>>>>>>> +	/*
>>>>>>>> +	 * If cumulative EMC frequency selection is higher than
>>>>>>>> the
>>>>>>>> +	 * device's, then there is no need to set upper watermark
>>>>>>>> to
>>>>>>>> +	 * a lower value because it will result in unnecessary
>>>>>>>> upper
>>>>>>>> +	 * interrupts.
>>>>>>>> +	 */
>>>>>>>> +	if (freq * ACTMON_SAMPLING_PERIOD > upper)
>>>>>>>> +		upper = freq * ACTMON_SAMPLING_PERIOD;    
>>>>>>>
>>>>>>> Also, 'upper value is used on the patch5. You can combine this code
>>>>>>> to patch5 or if this patch depends on the cpu notifier, you can
>>>>>>> combine it to the patch of adding cpu notifier without separate
>>>>>>> patch.  
>>>>>>
>>>>>> Well okay, I'll try to squash some of the patches in the next
>>>>>> revision. Usually I'm receiving comments in the other direction,
>>>>>> asking to separate patches into smaller changes ;) So that's more a
>>>>>> personal preference of each maintainer, I'd say.
>>>>>>   
>>>>>
>>>>> Right. We have to make the patch with atomic attribute.
>>>>> But, if there are patches which touch the same code
>>>>> in the same patchset. We can squash or do refactorig
>>>>> of this code.
>>>>
>>>> The main benefit of having smaller logical changes is that when there is
>>>> a bug, it's easier to narrow down the offending change using bisection.
>>>> And it's just easier to review smaller patches, of course.
>>>
>>> I agree that the patch should contain the atomic feature.
>>> To remove the some communication confusion between us,
>>> I don't mean that you have to merge patches to only one patch.
>>
>> If each patch has the atomic attribute, it have to be made as the separate patch.
>> But, if some patches are included in the the following two case,
>> can combine patches to one patch.
>>
>>>
>>> It is important to remove the following two cases on the same patchset.
>>>
>>> 1. the front patch adds the code and then later patch remove the added code.
> 
> Okay, I agree that this is applicable to patch #11.
> 
>>> 2. the front patch changes the code and the later patch again modified
>>>    the changed code of the front patch
> 
> If patch A adds a new feature and then patch B adds another new feature
> on top of A, do you consider each of these patches as atomic?

Yes, if patch A and patch A have the different role
for the same device driver, it is possible to make them
as the separate patches.


> 
> [snip]
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics