From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chanwoo Choi Subject: Re: [PATCH v4 20/24] PM / devfreq: tegra30: Optimize upper average watermark selection Date: Wed, 24 Jul 2019 20:19:09 +0900 Message-ID: <2c6a20e0-2e56-ea5a-76e6-bf17994fc7df@samsung.com> References: <20190707223303.6755-1-digetx@gmail.com> <20190707223303.6755-21-digetx@gmail.com> <20190719045943.73b53e31@dimatab> <1cec80ae-c03e-98a7-1d9f-6b57690610c7@samsung.com> <20190719052111.6641285d@dimatab> <3c9c6a3a-8bce-d304-8472-029e6e673a99@samsung.com> <01b442e4-437d-799d-ed0d-4628bd0d683b@gmail.com> <6fa38946-7441-4c19-83fd-4da422be5732@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <6fa38946-7441-4c19-83fd-4da422be5732@samsung.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Dmitry Osipenko Cc: Thierry Reding , MyungJoo Ham , Kyungmin Park , Jonathan Hunter , Tomeu Vizoso , linux-pm@vger.kernel.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-tegra@vger.kernel.org On 19. 7. 24. 오후 8:17, Chanwoo Choi wrote: > 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 пишет: >>>>> >>>>>> On 19. 7. 19. 오전 10:59, Dmitry Osipenko wrote: >>>>>>> В Fri, 19 Jul 2019 10:36:30 +0900 >>>>>>> Chanwoo Choi пишет: >>>>>>> >>>>>>>> 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 >>>>>>>>> --- >>>>>>>>> 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 Sorry for my mistake. Modify the sentence as following: Yes, if patch A and patch B 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