[v4,20/24] PM / devfreq: tegra30: Optimize upper average watermark selection
diff mbox series

Message ID 20190707223303.6755-21-digetx@gmail.com
State New
Headers show
Series
  • More improvements for Tegra30 devfreq driver
Related show

Commit Message

Dmitry Osipenko July 7, 2019, 10:32 p.m. UTC
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@gmail.com>
---
 drivers/devfreq/tegra30-devfreq.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Comments

Chanwoo Choi July 19, 2019, 1:36 a.m. UTC | #1
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@gmail.com>
> ---
>  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.

> +
>  	/*
>  	 * We want to get interrupts when MCCPU client crosses the
>  	 * dependency threshold in order to take into / out of account
> @@ -392,6 +402,7 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  			      struct tegra_devfreq_device *dev)
>  {
>  	u32 intr_status, dev_ctrl, avg_intr_mask, avg_count;
> +	unsigned long freq;
>  
>  	trace_device_isr_enter(tegra->regs, dev->config->offset,
>  			       dev->boost_freq, cpufreq_get(0));
> @@ -405,8 +416,10 @@ static void actmon_isr_device(struct tegra_devfreq *tegra,
>  	avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
>  			ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
>  
> -	if (intr_status & avg_intr_mask)
> -		tegra_devfreq_update_avg_wmark(tegra, dev);
> +	if (intr_status & avg_intr_mask) {
> +		freq = clk_get_rate(tegra->emc_clock) / KHZ;
> +		tegra_devfreq_update_avg_wmark(tegra, dev, freq);
> +	}
>  
>  	if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
>  		/*
> @@ -525,7 +538,7 @@ static int tegra_actmon_clk_notify_cb(struct notifier_block *nb,
>  	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>  		dev = &tegra->devices[i];
>  
> -		tegra_devfreq_update_avg_wmark(tegra, dev);
> +		tegra_devfreq_update_avg_wmark(tegra, dev, freq);
>  		tegra_devfreq_update_wmark(tegra, dev, freq);
>  	}
>  
> @@ -630,7 +643,7 @@ static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>  	device_writel(dev, dev->avg_freq * ACTMON_SAMPLING_PERIOD,
>  		      ACTMON_DEV_INIT_AVG);
>  
> -	tegra_devfreq_update_avg_wmark(tegra, dev);
> +	tegra_devfreq_update_avg_wmark(tegra, dev, dev->avg_freq);
>  	tegra_devfreq_update_wmark(tegra, dev, dev->avg_freq);
>  
>  	device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);
>
Dmitry Osipenko July 19, 2019, 1:59 a.m. UTC | #2
В Fri, 19 Jul 2019 10:36:30 +0900
Chanwoo Choi <cw00.choi@samsung.com> пишет:

> 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@gmail.com>
> > ---
> >  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.
Chanwoo Choi July 19, 2019, 2:06 a.m. UTC | #3
On 19. 7. 19. 오전 10:59, Dmitry Osipenko wrote:
> В Fri, 19 Jul 2019 10:36:30 +0900
> Chanwoo Choi <cw00.choi@samsung.com> пишет:
> 
>> 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@gmail.com>
>>> ---
>>>  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.

And also, if possible, I'd like you to make the patch
list according to the role of patch. For example,
the patches related to the 'watermark' could be sequentially
listed. But, it is not forced opinion. If just possible.
Dmitry Osipenko July 19, 2019, 2:21 a.m. UTC | #4
В Fri, 19 Jul 2019 11:06:05 +0900
Chanwoo Choi <cw00.choi@samsung.com> пишет:

> On 19. 7. 19. 오전 10:59, Dmitry Osipenko wrote:
> > В Fri, 19 Jul 2019 10:36:30 +0900
> > Chanwoo Choi <cw00.choi@samsung.com> пишет:
> >   
> >> 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@gmail.com>
> >>> ---
> >>>  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.

> And also, if possible, I'd like you to make the patch
> list according to the role of patch. For example,
> the patches related to the 'watermark' could be sequentially
> listed. But, it is not forced opinion. If just possible.

Okay, will take this into account.
Chanwoo Choi July 19, 2019, 6:09 a.m. UTC | #5
On 19. 7. 19. 오전 11:21, Dmitry Osipenko wrote:
> В Fri, 19 Jul 2019 11:06:05 +0900
> Chanwoo Choi <cw00.choi@samsung.com> пишет:
> 
>> On 19. 7. 19. 오전 10:59, Dmitry Osipenko wrote:
>>> В Fri, 19 Jul 2019 10:36:30 +0900
>>> Chanwoo Choi <cw00.choi@samsung.com> пишет:
>>>   
>>>> 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@gmail.com>
>>>>> ---
>>>>>  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.

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.
2. the front patch changes the code and the later patch again modified
   the changed code of the front patch


> 
>> And also, if possible, I'd like you to make the patch
>> list according to the role of patch. For example,
>> the patches related to the 'watermark' could be sequentially
>> listed. But, it is not forced opinion. If just possible.
> 
> Okay, will take this into account.
> 
> 
>
Chanwoo Choi July 19, 2019, 6:11 a.m. UTC | #6
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@samsung.com> пишет:
>>
>>> On 19. 7. 19. 오전 10:59, Dmitry Osipenko wrote:
>>>> В Fri, 19 Jul 2019 10:36:30 +0900
>>>> Chanwoo Choi <cw00.choi@samsung.com> пишет:
>>>>   
>>>>> 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@gmail.com>
>>>>>> ---
>>>>>>  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.
> 2. the front patch changes the code and the later patch again modified
>    the changed code of the front patch
> 
> 
>>
>>> And also, if possible, I'd like you to make the patch
>>> list according to the role of patch. For example,
>>> the patches related to the 'watermark' could be sequentially
>>> listed. But, it is not forced opinion. If just possible.
>>
>> Okay, will take this into account.
>>
>>
>>
> 
>
Dmitry Osipenko July 19, 2019, 5:52 p.m. UTC | #7
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@samsung.com> пишет:
>>>
>>>> On 19. 7. 19. 오전 10:59, Dmitry Osipenko wrote:
>>>>> В Fri, 19 Jul 2019 10:36:30 +0900
>>>>> Chanwoo Choi <cw00.choi@samsung.com> пишет:
>>>>>   
>>>>>> 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@gmail.com>
>>>>>>> ---
>>>>>>>  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?

[snip]
Chanwoo Choi July 24, 2019, 11:17 a.m. UTC | #8
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@samsung.com> пишет:
>>>>
>>>>> On 19. 7. 19. 오전 10:59, Dmitry Osipenko wrote:
>>>>>> В Fri, 19 Jul 2019 10:36:30 +0900
>>>>>> Chanwoo Choi <cw00.choi@samsung.com> пишет:
>>>>>>   
>>>>>>> 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@gmail.com>
>>>>>>>> ---
>>>>>>>>  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]
> 
>
Chanwoo Choi July 24, 2019, 11:19 a.m. UTC | #9
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 <cw00.choi@samsung.com> пишет:
>>>>>
>>>>>> On 19. 7. 19. 오전 10:59, Dmitry Osipenko wrote:
>>>>>>> В Fri, 19 Jul 2019 10:36:30 +0900
>>>>>>> Chanwoo Choi <cw00.choi@samsung.com> пишет:
>>>>>>>   
>>>>>>>> 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@gmail.com>
>>>>>>>>> ---
>>>>>>>>>  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]
>>
>>
> 
>

Patch
diff mbox series

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;
+
 	/*
 	 * We want to get interrupts when MCCPU client crosses the
 	 * dependency threshold in order to take into / out of account
@@ -392,6 +402,7 @@  static void actmon_isr_device(struct tegra_devfreq *tegra,
 			      struct tegra_devfreq_device *dev)
 {
 	u32 intr_status, dev_ctrl, avg_intr_mask, avg_count;
+	unsigned long freq;
 
 	trace_device_isr_enter(tegra->regs, dev->config->offset,
 			       dev->boost_freq, cpufreq_get(0));
@@ -405,8 +416,10 @@  static void actmon_isr_device(struct tegra_devfreq *tegra,
 	avg_intr_mask = ACTMON_DEV_INTR_AVG_BELOW_WMARK |
 			ACTMON_DEV_INTR_AVG_ABOVE_WMARK;
 
-	if (intr_status & avg_intr_mask)
-		tegra_devfreq_update_avg_wmark(tegra, dev);
+	if (intr_status & avg_intr_mask) {
+		freq = clk_get_rate(tegra->emc_clock) / KHZ;
+		tegra_devfreq_update_avg_wmark(tegra, dev, freq);
+	}
 
 	if (intr_status & ACTMON_DEV_INTR_CONSECUTIVE_UPPER) {
 		/*
@@ -525,7 +538,7 @@  static int tegra_actmon_clk_notify_cb(struct notifier_block *nb,
 	for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
 		dev = &tegra->devices[i];
 
-		tegra_devfreq_update_avg_wmark(tegra, dev);
+		tegra_devfreq_update_avg_wmark(tegra, dev, freq);
 		tegra_devfreq_update_wmark(tegra, dev, freq);
 	}
 
@@ -630,7 +643,7 @@  static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
 	device_writel(dev, dev->avg_freq * ACTMON_SAMPLING_PERIOD,
 		      ACTMON_DEV_INIT_AVG);
 
-	tegra_devfreq_update_avg_wmark(tegra, dev);
+	tegra_devfreq_update_avg_wmark(tegra, dev, dev->avg_freq);
 	tegra_devfreq_update_wmark(tegra, dev, dev->avg_freq);
 
 	device_writel(dev, ACTMON_COUNT_WEIGHT, ACTMON_DEV_COUNT_WEIGHT);