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