[1/2] memory: samsung: exynos5422-dmc: Adjust polling interval and uptreshold
diff mbox series

Message ID 20200708153420.29484-2-lukasz.luba@arm.com
State Under Review
Headers show
Series
  • Exynos5422 DMC: adjust to new devfreq monitoring mechanism
Related show

Commit Message

Lukasz Luba July 8, 2020, 3:34 p.m. UTC
In order to react faster and make better decisions under some workloads,
benchmarking the memory subsystem behavior, adjust the polling interval
and upthreshold value used by the simple_ondemand governor.

Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Chanwoo Choi July 9, 2020, 4:08 a.m. UTC | #1
Hi Lukasz,

On 7/9/20 12:34 AM, Lukasz Luba wrote:
> In order to react faster and make better decisions under some workloads,
> benchmarking the memory subsystem behavior, adjust the polling interval
> and upthreshold value used by the simple_ondemand governor.
> 
> Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
> index 93e9c2429c0d..e03ee35f0ab5 100644
> --- a/drivers/memory/samsung/exynos5422-dmc.c
> +++ b/drivers/memory/samsung/exynos5422-dmc.c
> @@ -1466,10 +1466,10 @@ static int exynos5_dmc_probe(struct platform_device *pdev)
>  		 * Setup default thresholds for the devfreq governor.
>  		 * The values are chosen based on experiments.
>  		 */
> -		dmc->gov_data.upthreshold = 30;
> +		dmc->gov_data.upthreshold = 10;
>  		dmc->gov_data.downdifferential = 5;
>  
> -		exynos5_dmc_df_profile.polling_ms = 500;
> +		exynos5_dmc_df_profile.polling_ms = 100;
>  	}
>  
>  
> 

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Lukasz Luba July 10, 2020, 8:34 a.m. UTC | #2
Hi Chanwoo,

On 7/9/20 5:08 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> On 7/9/20 12:34 AM, Lukasz Luba wrote:
>> In order to react faster and make better decisions under some workloads,
>> benchmarking the memory subsystem behavior, adjust the polling interval
>> and upthreshold value used by the simple_ondemand governor.
>>
>> Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
>> index 93e9c2429c0d..e03ee35f0ab5 100644
>> --- a/drivers/memory/samsung/exynos5422-dmc.c
>> +++ b/drivers/memory/samsung/exynos5422-dmc.c
>> @@ -1466,10 +1466,10 @@ static int exynos5_dmc_probe(struct platform_device *pdev)
>>   		 * Setup default thresholds for the devfreq governor.
>>   		 * The values are chosen based on experiments.
>>   		 */
>> -		dmc->gov_data.upthreshold = 30;
>> +		dmc->gov_data.upthreshold = 10;
>>   		dmc->gov_data.downdifferential = 5;
>>   
>> -		exynos5_dmc_df_profile.polling_ms = 500;
>> +		exynos5_dmc_df_profile.polling_ms = 100;
>>   	}
>>   
>>   
>>
> 
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> 

Thank you for the review. Do you think this patch could go through
your tree together with your patches?

I don't know Krzysztof's opinion about the patch 2/2, but
I would expect, assuming the patch itself is correct, he would
like to take it into his next/dt branch.

Regards,
Lukasz
Krzysztof Kozlowski July 10, 2020, 12:45 p.m. UTC | #3
On Fri, Jul 10, 2020 at 09:34:45AM +0100, Lukasz Luba wrote:
> Hi Chanwoo,
> 
> On 7/9/20 5:08 AM, Chanwoo Choi wrote:
> > Hi Lukasz,
> > 
> > On 7/9/20 12:34 AM, Lukasz Luba wrote:
> > > In order to react faster and make better decisions under some workloads,
> > > benchmarking the memory subsystem behavior, adjust the polling interval
> > > and upthreshold value used by the simple_ondemand governor.
> > > 
> > > Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
> > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> > > ---
> > >   drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
> > > index 93e9c2429c0d..e03ee35f0ab5 100644
> > > --- a/drivers/memory/samsung/exynos5422-dmc.c
> > > +++ b/drivers/memory/samsung/exynos5422-dmc.c
> > > @@ -1466,10 +1466,10 @@ static int exynos5_dmc_probe(struct platform_device *pdev)
> > >   		 * Setup default thresholds for the devfreq governor.
> > >   		 * The values are chosen based on experiments.
> > >   		 */
> > > -		dmc->gov_data.upthreshold = 30;
> > > +		dmc->gov_data.upthreshold = 10;
> > >   		dmc->gov_data.downdifferential = 5;
> > > -		exynos5_dmc_df_profile.polling_ms = 500;
> > > +		exynos5_dmc_df_profile.polling_ms = 100;
> > >   	}
> > > 
> > 
> > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> > 
> 
> Thank you for the review. Do you think this patch could go through
> your tree together with your patches?
> 
> I don't know Krzysztof's opinion about the patch 2/2, but
> I would expect, assuming the patch itself is correct, he would
> like to take it into his next/dt branch.

In the cover letter you mentioned that this is a follow up for the
Chanwoo's patchset. But are these patches really depending on it? Can
they be picked up independently?

The DTS patch must go through arm soc, so I will take it. If it really
depends on driver changes, then it has to wait for next release.

Best regards,
Krzysztof
Lukasz Luba July 10, 2020, 12:56 p.m. UTC | #4
On 7/10/20 1:45 PM, Krzysztof Kozlowski wrote:
> On Fri, Jul 10, 2020 at 09:34:45AM +0100, Lukasz Luba wrote:
>> Hi Chanwoo,
>>
>> On 7/9/20 5:08 AM, Chanwoo Choi wrote:
>>> Hi Lukasz,
>>>
>>> On 7/9/20 12:34 AM, Lukasz Luba wrote:
>>>> In order to react faster and make better decisions under some workloads,
>>>> benchmarking the memory subsystem behavior, adjust the polling interval
>>>> and upthreshold value used by the simple_ondemand governor.
>>>>
>>>> Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>> ---
>>>>    drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
>>>> index 93e9c2429c0d..e03ee35f0ab5 100644
>>>> --- a/drivers/memory/samsung/exynos5422-dmc.c
>>>> +++ b/drivers/memory/samsung/exynos5422-dmc.c
>>>> @@ -1466,10 +1466,10 @@ static int exynos5_dmc_probe(struct platform_device *pdev)
>>>>    		 * Setup default thresholds for the devfreq governor.
>>>>    		 * The values are chosen based on experiments.
>>>>    		 */
>>>> -		dmc->gov_data.upthreshold = 30;
>>>> +		dmc->gov_data.upthreshold = 10;
>>>>    		dmc->gov_data.downdifferential = 5;
>>>> -		exynos5_dmc_df_profile.polling_ms = 500;
>>>> +		exynos5_dmc_df_profile.polling_ms = 100;
>>>>    	}
>>>>
>>>
>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>
>>
>> Thank you for the review. Do you think this patch could go through
>> your tree together with your patches?
>>
>> I don't know Krzysztof's opinion about the patch 2/2, but
>> I would expect, assuming the patch itself is correct, he would
>> like to take it into his next/dt branch.
> 
> In the cover letter you mentioned that this is a follow up for the
> Chanwoo's patchset. But are these patches really depending on it? Can
> they be picked up independently?


They are not heavily dependent on Chanwoo's patches.
Yes, they can be picked up independently.
I just wanted to mention that the patch 1/2 was produced on the
code base which had already applied Chanwoo's patch for DMC.
If you like to take both 1/2 and 2/2 into your tree, it's good.

Thank you for having a look on this.

Regards,
Lukasz


> 
> The DTS patch must go through arm soc, so I will take it. If it really
> depends on driver changes, then it has to wait for next release.
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski July 10, 2020, 1:07 p.m. UTC | #5
On Fri, Jul 10, 2020 at 01:56:33PM +0100, Lukasz Luba wrote:
> 
> 
> On 7/10/20 1:45 PM, Krzysztof Kozlowski wrote:
> > On Fri, Jul 10, 2020 at 09:34:45AM +0100, Lukasz Luba wrote:
> > > Hi Chanwoo,
> > > 
> > > On 7/9/20 5:08 AM, Chanwoo Choi wrote:
> > > > Hi Lukasz,
> > > > 
> > > > On 7/9/20 12:34 AM, Lukasz Luba wrote:
> > > > > In order to react faster and make better decisions under some workloads,
> > > > > benchmarking the memory subsystem behavior, adjust the polling interval
> > > > > and upthreshold value used by the simple_ondemand governor.
> > > > > 
> > > > > Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
> > > > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> > > > > ---
> > > > >    drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
> > > > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
> > > > > index 93e9c2429c0d..e03ee35f0ab5 100644
> > > > > --- a/drivers/memory/samsung/exynos5422-dmc.c
> > > > > +++ b/drivers/memory/samsung/exynos5422-dmc.c
> > > > > @@ -1466,10 +1466,10 @@ static int exynos5_dmc_probe(struct platform_device *pdev)
> > > > >    		 * Setup default thresholds for the devfreq governor.
> > > > >    		 * The values are chosen based on experiments.
> > > > >    		 */
> > > > > -		dmc->gov_data.upthreshold = 30;
> > > > > +		dmc->gov_data.upthreshold = 10;
> > > > >    		dmc->gov_data.downdifferential = 5;
> > > > > -		exynos5_dmc_df_profile.polling_ms = 500;
> > > > > +		exynos5_dmc_df_profile.polling_ms = 100;
> > > > >    	}
> > > > > 
> > > > 
> > > > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> > > > 
> > > 
> > > Thank you for the review. Do you think this patch could go through
> > > your tree together with your patches?
> > > 
> > > I don't know Krzysztof's opinion about the patch 2/2, but
> > > I would expect, assuming the patch itself is correct, he would
> > > like to take it into his next/dt branch.
> > 
> > In the cover letter you mentioned that this is a follow up for the
> > Chanwoo's patchset. But are these patches really depending on it? Can
> > they be picked up independently?
> 
> 
> They are not heavily dependent on Chanwoo's patches.
> Yes, they can be picked up independently.
> I just wanted to mention that the patch 1/2 was produced on the
> code base which had already applied Chanwoo's patch for DMC.
> If you like to take both 1/2 and 2/2 into your tree, it's good.

Then let me take them. If there is more development ongoing, it would
create unnecessary merges between trees.

Best regards,
Krzysztof
Marek Szyprowski July 10, 2020, 1:13 p.m. UTC | #6
Hi Lukasz,

On 10.07.2020 10:34, Lukasz Luba wrote:
> Hi Chanwoo,
>
> On 7/9/20 5:08 AM, Chanwoo Choi wrote:
>> Hi Lukasz,
>>
>> On 7/9/20 12:34 AM, Lukasz Luba wrote:
>>> In order to react faster and make better decisions under some 
>>> workloads,
>>> benchmarking the memory subsystem behavior, adjust the polling interval
>>> and upthreshold value used by the simple_ondemand governor.
>>>
>>> Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>> ---
>>>   drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/memory/samsung/exynos5422-dmc.c 
>>> b/drivers/memory/samsung/exynos5422-dmc.c
>>> index 93e9c2429c0d..e03ee35f0ab5 100644
>>> --- a/drivers/memory/samsung/exynos5422-dmc.c
>>> +++ b/drivers/memory/samsung/exynos5422-dmc.c
>>> @@ -1466,10 +1466,10 @@ static int exynos5_dmc_probe(struct 
>>> platform_device *pdev)
>>>            * Setup default thresholds for the devfreq governor.
>>>            * The values are chosen based on experiments.
>>>            */
>>> -        dmc->gov_data.upthreshold = 30;
>>> +        dmc->gov_data.upthreshold = 10;
>>>           dmc->gov_data.downdifferential = 5;
>>>   -        exynos5_dmc_df_profile.polling_ms = 500;
>>> +        exynos5_dmc_df_profile.polling_ms = 100;
>>>       }
>>>
>>
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
>
> Thank you for the review. Do you think this patch could go through
> your tree together with your patches?
>
> I don't know Krzysztof's opinion about the patch 2/2, but
> I would expect, assuming the patch itself is correct, he would
> like to take it into his next/dt branch.

Is there really a need to remove the interrupts property? imho they are 
correct hw description, it just a matter of the driver to use or not to 
use them.

Best regards
Krzysztof Kozlowski July 10, 2020, 1:19 p.m. UTC | #7
On Fri, Jul 10, 2020 at 03:13:18PM +0200, Marek Szyprowski wrote:
> Hi Lukasz,
> 
> On 10.07.2020 10:34, Lukasz Luba wrote:
> > Hi Chanwoo,
> >
> > On 7/9/20 5:08 AM, Chanwoo Choi wrote:
> >> Hi Lukasz,
> >>
> >> On 7/9/20 12:34 AM, Lukasz Luba wrote:
> >>> In order to react faster and make better decisions under some 
> >>> workloads,
> >>> benchmarking the memory subsystem behavior, adjust the polling interval
> >>> and upthreshold value used by the simple_ondemand governor.
> >>>
> >>> Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
> >>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> >>> ---
> >>>   drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/memory/samsung/exynos5422-dmc.c 
> >>> b/drivers/memory/samsung/exynos5422-dmc.c
> >>> index 93e9c2429c0d..e03ee35f0ab5 100644
> >>> --- a/drivers/memory/samsung/exynos5422-dmc.c
> >>> +++ b/drivers/memory/samsung/exynos5422-dmc.c
> >>> @@ -1466,10 +1466,10 @@ static int exynos5_dmc_probe(struct 
> >>> platform_device *pdev)
> >>>            * Setup default thresholds for the devfreq governor.
> >>>            * The values are chosen based on experiments.
> >>>            */
> >>> -        dmc->gov_data.upthreshold = 30;
> >>> +        dmc->gov_data.upthreshold = 10;
> >>>           dmc->gov_data.downdifferential = 5;
> >>>   -        exynos5_dmc_df_profile.polling_ms = 500;
> >>> +        exynos5_dmc_df_profile.polling_ms = 100;
> >>>       }
> >>>
> >>
> >> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> >>
> >
> > Thank you for the review. Do you think this patch could go through
> > your tree together with your patches?
> >
> > I don't know Krzysztof's opinion about the patch 2/2, but
> > I would expect, assuming the patch itself is correct, he would
> > like to take it into his next/dt branch.
> 
> Is there really a need to remove the interrupts property? imho they are 
> correct hw description, it just a matter of the driver to use or not to 
> use them.

That's actually very good point. I would also prefer to leave them.
However I understood that driver chooses mode depending on the property.

In such case, maybe as you said, let's switch to polling mode
unconditionally?

Best regards,
Krzysztof
Lukasz Luba July 10, 2020, 1:41 p.m. UTC | #8
On 7/10/20 2:19 PM, Krzysztof Kozlowski wrote:
> On Fri, Jul 10, 2020 at 03:13:18PM +0200, Marek Szyprowski wrote:
>> Hi Lukasz,
>>
>> On 10.07.2020 10:34, Lukasz Luba wrote:
>>> Hi Chanwoo,
>>>
>>> On 7/9/20 5:08 AM, Chanwoo Choi wrote:
>>>> Hi Lukasz,
>>>>
>>>> On 7/9/20 12:34 AM, Lukasz Luba wrote:
>>>>> In order to react faster and make better decisions under some
>>>>> workloads,
>>>>> benchmarking the memory subsystem behavior, adjust the polling interval
>>>>> and upthreshold value used by the simple_ondemand governor.
>>>>>
>>>>> Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
>>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>>> ---
>>>>>    drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/memory/samsung/exynos5422-dmc.c
>>>>> b/drivers/memory/samsung/exynos5422-dmc.c
>>>>> index 93e9c2429c0d..e03ee35f0ab5 100644
>>>>> --- a/drivers/memory/samsung/exynos5422-dmc.c
>>>>> +++ b/drivers/memory/samsung/exynos5422-dmc.c
>>>>> @@ -1466,10 +1466,10 @@ static int exynos5_dmc_probe(struct
>>>>> platform_device *pdev)
>>>>>             * Setup default thresholds for the devfreq governor.
>>>>>             * The values are chosen based on experiments.
>>>>>             */
>>>>> -        dmc->gov_data.upthreshold = 30;
>>>>> +        dmc->gov_data.upthreshold = 10;
>>>>>            dmc->gov_data.downdifferential = 5;
>>>>>    -        exynos5_dmc_df_profile.polling_ms = 500;
>>>>> +        exynos5_dmc_df_profile.polling_ms = 100;
>>>>>        }
>>>>>
>>>>
>>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>
>>>
>>> Thank you for the review. Do you think this patch could go through
>>> your tree together with your patches?
>>>
>>> I don't know Krzysztof's opinion about the patch 2/2, but
>>> I would expect, assuming the patch itself is correct, he would
>>> like to take it into his next/dt branch.
>>
>> Is there really a need to remove the interrupts property? imho they are
>> correct hw description, it just a matter of the driver to use or not to
>> use them.

Marek, I agree with you, they are correct hw description. Unfortunately,
I don't have TRM to experiment and maybe fix the interrupt mode code.

> 
> That's actually very good point. I would also prefer to leave them.
> However I understood that driver chooses mode depending on the property.

Correct

> 
> In such case, maybe as you said, let's switch to polling mode
> unconditionally?

I can make happen that the polling mode will be unconditionally
set as default.

Do you think that the interrupt mode code can still stay in the
driver, because maybe in future could be fixed?


> 
> Best regards,
> Krzysztof
>
Bartlomiej Zolnierkiewicz July 10, 2020, 1:49 p.m. UTC | #9
On 7/10/20 2:56 PM, Lukasz Luba wrote:
> 
> 
> On 7/10/20 1:45 PM, Krzysztof Kozlowski wrote:
>> On Fri, Jul 10, 2020 at 09:34:45AM +0100, Lukasz Luba wrote:
>>> Hi Chanwoo,
>>>
>>> On 7/9/20 5:08 AM, Chanwoo Choi wrote:
>>>> Hi Lukasz,
>>>>
>>>> On 7/9/20 12:34 AM, Lukasz Luba wrote:
>>>>> In order to react faster and make better decisions under some workloads,
>>>>> benchmarking the memory subsystem behavior, adjust the polling interval
>>>>> and upthreshold value used by the simple_ondemand governor.
>>>>>
>>>>> Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
>>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>>> ---
>>>>>    drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
>>>>> index 93e9c2429c0d..e03ee35f0ab5 100644
>>>>> --- a/drivers/memory/samsung/exynos5422-dmc.c
>>>>> +++ b/drivers/memory/samsung/exynos5422-dmc.c
>>>>> @@ -1466,10 +1466,10 @@ static int exynos5_dmc_probe(struct platform_device *pdev)
>>>>>             * Setup default thresholds for the devfreq governor.
>>>>>             * The values are chosen based on experiments.
>>>>>             */
>>>>> -        dmc->gov_data.upthreshold = 30;
>>>>> +        dmc->gov_data.upthreshold = 10;
>>>>>            dmc->gov_data.downdifferential = 5;
>>>>> -        exynos5_dmc_df_profile.polling_ms = 500;
>>>>> +        exynos5_dmc_df_profile.polling_ms = 100;
>>>>>        }
>>>>>
>>>>
>>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>
>>>
>>> Thank you for the review. Do you think this patch could go through
>>> your tree together with your patches?
>>>
>>> I don't know Krzysztof's opinion about the patch 2/2, but
>>> I would expect, assuming the patch itself is correct, he would
>>> like to take it into his next/dt branch.
>>
>> In the cover letter you mentioned that this is a follow up for the
>> Chanwoo's patchset. But are these patches really depending on it? Can
>> they be picked up independently?
> 
> 
> They are not heavily dependent on Chanwoo's patches.
> Yes, they can be picked up independently.

Hmmm, are you sure?

Sure, they will apply fine but without Chanwoo's patches won't they
cause the dmc driver to use using polling mode with deferred timer
(unintended/bad behavior) instead of IRQs (current behavior) or
polling mode with delayed timer (future behavior)?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> I just wanted to mention that the patch 1/2 was produced on the
> code base which had already applied Chanwoo's patch for DMC.
> If you like to take both 1/2 and 2/2 into your tree, it's good.
> 
> Thank you for having a look on this.
> 
> Regards,
> Lukasz
> 
> 
>>
>> The DTS patch must go through arm soc, so I will take it. If it really
>> depends on driver changes, then it has to wait for next release.
>>
>> Best regards,
>> Krzysztof
>>
Lukasz Luba July 10, 2020, 2 p.m. UTC | #10
On 7/10/20 2:49 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> On 7/10/20 2:56 PM, Lukasz Luba wrote:
>>
>>
>> On 7/10/20 1:45 PM, Krzysztof Kozlowski wrote:
>>> On Fri, Jul 10, 2020 at 09:34:45AM +0100, Lukasz Luba wrote:
>>>> Hi Chanwoo,
>>>>
>>>> On 7/9/20 5:08 AM, Chanwoo Choi wrote:
>>>>> Hi Lukasz,
>>>>>
>>>>> On 7/9/20 12:34 AM, Lukasz Luba wrote:
>>>>>> In order to react faster and make better decisions under some workloads,
>>>>>> benchmarking the memory subsystem behavior, adjust the polling interval
>>>>>> and upthreshold value used by the simple_ondemand governor.
>>>>>>
>>>>>> Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
>>>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>>>> ---
>>>>>>     drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
>>>>>> index 93e9c2429c0d..e03ee35f0ab5 100644
>>>>>> --- a/drivers/memory/samsung/exynos5422-dmc.c
>>>>>> +++ b/drivers/memory/samsung/exynos5422-dmc.c
>>>>>> @@ -1466,10 +1466,10 @@ static int exynos5_dmc_probe(struct platform_device *pdev)
>>>>>>              * Setup default thresholds for the devfreq governor.
>>>>>>              * The values are chosen based on experiments.
>>>>>>              */
>>>>>> -        dmc->gov_data.upthreshold = 30;
>>>>>> +        dmc->gov_data.upthreshold = 10;
>>>>>>             dmc->gov_data.downdifferential = 5;
>>>>>> -        exynos5_dmc_df_profile.polling_ms = 500;
>>>>>> +        exynos5_dmc_df_profile.polling_ms = 100;
>>>>>>         }
>>>>>>
>>>>>
>>>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>
>>>>
>>>> Thank you for the review. Do you think this patch could go through
>>>> your tree together with your patches?
>>>>
>>>> I don't know Krzysztof's opinion about the patch 2/2, but
>>>> I would expect, assuming the patch itself is correct, he would
>>>> like to take it into his next/dt branch.
>>>
>>> In the cover letter you mentioned that this is a follow up for the
>>> Chanwoo's patchset. But are these patches really depending on it? Can
>>> they be picked up independently?
>>
>>
>> They are not heavily dependent on Chanwoo's patches.
>> Yes, they can be picked up independently.
> 
> Hmmm, are you sure?

In a sense: in two phases (first the Chanwoo's changes land into
devfreq, then when Krzysztof prepares his topic branches for
arm soc, I assumed Chanwoo's patches are mainline and will be there
already).

> 
> Sure, they will apply fine but without Chanwoo's patches won't they
> cause the dmc driver to use using polling mode with deferred timer
> (unintended/bad behavior) instead of IRQs (current behavior) or
> polling mode with delayed timer (future behavior)?

I was assuming that it will take longer, when Krzysztof is going to pick
patch 2/2, definitely after a while (and it could be also the case for
patch 1/1 if Krzysztof was going to take it).

I think there is no rush and it can go in two phases.

Good point Bartek for clarifying this. I wasn't clear in the messages.
Thank you for keeping eye on this.

Regards,
Lukasz


> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
>> I just wanted to mention that the patch 1/2 was produced on the
>> code base which had already applied Chanwoo's patch for DMC.
>> If you like to take both 1/2 and 2/2 into your tree, it's good.
>>
>> Thank you for having a look on this.
>>
>> Regards,
>> Lukasz
>>
>>
>>>
>>> The DTS patch must go through arm soc, so I will take it. If it really
>>> depends on driver changes, then it has to wait for next release.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>
Krzysztof Kozlowski July 10, 2020, 2:47 p.m. UTC | #11
On Fri, Jul 10, 2020 at 03:00:37PM +0100, Lukasz Luba wrote:
> 
> 
> On 7/10/20 2:49 PM, Bartlomiej Zolnierkiewicz wrote:
> > 
> > On 7/10/20 2:56 PM, Lukasz Luba wrote:
> > > 
> > > 
> > > On 7/10/20 1:45 PM, Krzysztof Kozlowski wrote:
> > > > On Fri, Jul 10, 2020 at 09:34:45AM +0100, Lukasz Luba wrote:
> > > > > Hi Chanwoo,
> > > > > 
> > > > > On 7/9/20 5:08 AM, Chanwoo Choi wrote:
> > > > > > Hi Lukasz,
> > > > > > 
> > > > > > On 7/9/20 12:34 AM, Lukasz Luba wrote:
> > > > > > > In order to react faster and make better decisions under some workloads,
> > > > > > > benchmarking the memory subsystem behavior, adjust the polling interval
> > > > > > > and upthreshold value used by the simple_ondemand governor.
> > > > > > > 
> > > > > > > Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
> > > > > > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> > > > > > > ---
> > > > > > >     drivers/memory/samsung/exynos5422-dmc.c | 4 ++--
> > > > > > >     1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
> > > > > > > index 93e9c2429c0d..e03ee35f0ab5 100644
> > > > > > > --- a/drivers/memory/samsung/exynos5422-dmc.c
> > > > > > > +++ b/drivers/memory/samsung/exynos5422-dmc.c
> > > > > > > @@ -1466,10 +1466,10 @@ static int exynos5_dmc_probe(struct platform_device *pdev)
> > > > > > >              * Setup default thresholds for the devfreq governor.
> > > > > > >              * The values are chosen based on experiments.
> > > > > > >              */
> > > > > > > -        dmc->gov_data.upthreshold = 30;
> > > > > > > +        dmc->gov_data.upthreshold = 10;
> > > > > > >             dmc->gov_data.downdifferential = 5;
> > > > > > > -        exynos5_dmc_df_profile.polling_ms = 500;
> > > > > > > +        exynos5_dmc_df_profile.polling_ms = 100;
> > > > > > >         }
> > > > > > > 
> > > > > > 
> > > > > > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> > > > > > 
> > > > > 
> > > > > Thank you for the review. Do you think this patch could go through
> > > > > your tree together with your patches?
> > > > > 
> > > > > I don't know Krzysztof's opinion about the patch 2/2, but
> > > > > I would expect, assuming the patch itself is correct, he would
> > > > > like to take it into his next/dt branch.
> > > > 
> > > > In the cover letter you mentioned that this is a follow up for the
> > > > Chanwoo's patchset. But are these patches really depending on it? Can
> > > > they be picked up independently?
> > > 
> > > 
> > > They are not heavily dependent on Chanwoo's patches.
> > > Yes, they can be picked up independently.
> > 
> > Hmmm, are you sure?
> 
> In a sense: in two phases (first the Chanwoo's changes land into
> devfreq, then when Krzysztof prepares his topic branches for
> arm soc, I assumed Chanwoo's patches are mainline and will be there
> already).
> 
> > 
> > Sure, they will apply fine but without Chanwoo's patches won't they
> > cause the dmc driver to use using polling mode with deferred timer
> > (unintended/bad behavior) instead of IRQs (current behavior) or
> > polling mode with delayed timer (future behavior)?
> 
> I was assuming that it will take longer, when Krzysztof is going to pick
> patch 2/2, definitely after a while (and it could be also the case for
> patch 1/1 if Krzysztof was going to take it).
> 
> I think there is no rush and it can go in two phases.
> 
> Good point Bartek for clarifying this. I wasn't clear in the messages.
> Thank you for keeping eye on this.

... which means they are dependent on Chanwoo's patchset. They cannot go
in parallel, they cannot be picked up independently.

You know, when someone's patches are in mainline, it is never a
dependency anymore...

The patches then could wait for next cycle if there is no rush. Maybe
there will be some further revisions of this approach?

Best regards,
Krzysztof
Krzysztof Kozlowski July 10, 2020, 2:49 p.m. UTC | #12
On Fri, Jul 10, 2020 at 02:41:28PM +0100, Lukasz Luba wrote:
> 
> 
> On 7/10/20 2:19 PM, Krzysztof Kozlowski wrote:
> > On Fri, Jul 10, 2020 at 03:13:18PM +0200, Marek Szyprowski wrote:
 > In such case, maybe as you said, let's switch to polling mode
> > unconditionally?
> 
> I can make happen that the polling mode will be unconditionally
> set as default.
> 
> Do you think that the interrupt mode code can still stay in the
> driver, because maybe in future could be fixed?

How interrupt mode would exist in such case? Or rather: how would it be
used? There is no point to keep dead code and code once removed, can be
easily brought back.

Best regards,
Krzysztof
Lukasz Luba July 10, 2020, 3:45 p.m. UTC | #13
On 7/10/20 3:49 PM, Krzysztof Kozlowski wrote:
> On Fri, Jul 10, 2020 at 02:41:28PM +0100, Lukasz Luba wrote:
>>
>>
>> On 7/10/20 2:19 PM, Krzysztof Kozlowski wrote:
>>> On Fri, Jul 10, 2020 at 03:13:18PM +0200, Marek Szyprowski wrote:
>   > In such case, maybe as you said, let's switch to polling mode
>>> unconditionally?
>>
>> I can make happen that the polling mode will be unconditionally
>> set as default.
>>
>> Do you think that the interrupt mode code can still stay in the
>> driver, because maybe in future could be fixed?
> 
> How interrupt mode would exist in such case? Or rather: how would it be
> used? There is no point to keep dead code and code once removed, can be
> easily brought back.

I can make a module param i.e. irq-mode=1, while in default where
the user don't provide param, we use polling mode. Then I don't have to
remove DT interrupts and the related code from the driver.

Regards,
Lukasz

Patch
diff mbox series

diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c
index 93e9c2429c0d..e03ee35f0ab5 100644
--- a/drivers/memory/samsung/exynos5422-dmc.c
+++ b/drivers/memory/samsung/exynos5422-dmc.c
@@ -1466,10 +1466,10 @@  static int exynos5_dmc_probe(struct platform_device *pdev)
 		 * Setup default thresholds for the devfreq governor.
 		 * The values are chosen based on experiments.
 		 */
-		dmc->gov_data.upthreshold = 30;
+		dmc->gov_data.upthreshold = 10;
 		dmc->gov_data.downdifferential = 5;
 
-		exynos5_dmc_df_profile.polling_ms = 500;
+		exynos5_dmc_df_profile.polling_ms = 100;
 	}