[PATCH] drm/radeon/pm: autoswitch power state when in balanced mode

[PATCH] drm/radeon/pm: autoswitch power state when in balanced mode

[Lucas Stach]

The current default of always using the performance power state leads
to increased power consumption of mobile devices, which have a dedicated
battery power state. Switch between the performance and battery power
state automatically, dpending on the current AC power status, when the
user asked for the balanced power state.

The user can still override this logic by asking for the performance
or battery power state explicitly.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
This saves about 1.2W on my Richland based laptop, whithout me having
to remember to ask for the battery state or have userspace set up in
a way to do this.
---
 drivers/gpu/drm/radeon/radeon_pm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
index 4b65425..326ad06 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -47,6 +47,7 @@ static bool radeon_pm_in_vbl(struct radeon_device *rdev);
 static bool radeon_pm_debug_check_in_vbl(struct radeon_device *rdev, bool finish);
 static void radeon_pm_update_profile(struct radeon_device *rdev);
 static void radeon_pm_set_clocks(struct radeon_device *rdev);
+static void radeon_pm_compute_clocks_dpm(struct radeon_device *rdev);
 
 int radeon_pm_get_type_index(struct radeon_device *rdev,
 			     enum radeon_pm_state_type ps_type,
@@ -79,6 +80,8 @@ void radeon_pm_acpi_event_handler(struct radeon_device *rdev)
 				radeon_dpm_enable_bapm(rdev, rdev->pm.dpm.ac_power);
 		}
 		mutex_unlock(&rdev->pm.mutex);
+		/* allow new DPM state to be picked */
+		radeon_pm_compute_clocks_dpm(rdev);
 	} else if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
 		if (rdev->pm.profile == PM_PROFILE_AUTO) {
 			mutex_lock(&rdev->pm.mutex);
@@ -882,7 +885,8 @@ static struct radeon_ps *radeon_dpm_pick_power_state(struct radeon_device *rdev,
 		dpm_state = POWER_STATE_TYPE_INTERNAL_3DPERF;
 	/* balanced states don't exist at the moment */
 	if (dpm_state == POWER_STATE_TYPE_BALANCED)
-		dpm_state = POWER_STATE_TYPE_PERFORMANCE;
+		dpm_state = rdev->pm.dpm.ac_power ?
+			POWER_STATE_TYPE_PERFORMANCE : POWER_STATE_TYPE_BATTERY;
 
 restart_search:
 	/* Pick the best power state based on current conditions */
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon/pm: autoswitch power state when in balanced mode

[Michel Dänzer]

[-- Attachment #1.1.1: Type: text/plain, Size: 2834 bytes --]

On 23/10/16 08:05 AM, Lucas Stach wrote:
> The current default of always using the performance power state leads
> to increased power consumption of mobile devices, which have a dedicated
> battery power state. Switch between the performance and battery power
> state automatically, dpending on the current AC power status, when the
> user asked for the balanced power state.
> 
> The user can still override this logic by asking for the performance
> or battery power state explicitly.
> 
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
> This saves about 1.2W on my Richland based laptop, whithout me having
> to remember to ask for the battery state or have userspace set up in
> a way to do this.
> ---
>  drivers/gpu/drm/radeon/radeon_pm.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
> index 4b65425..326ad06 100644
> --- a/drivers/gpu/drm/radeon/radeon_pm.c
> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
> @@ -47,6 +47,7 @@ static bool radeon_pm_in_vbl(struct radeon_device *rdev);
>  static bool radeon_pm_debug_check_in_vbl(struct radeon_device *rdev, bool finish);
>  static void radeon_pm_update_profile(struct radeon_device *rdev);
>  static void radeon_pm_set_clocks(struct radeon_device *rdev);
> +static void radeon_pm_compute_clocks_dpm(struct radeon_device *rdev);
>  
>  int radeon_pm_get_type_index(struct radeon_device *rdev,
>  			     enum radeon_pm_state_type ps_type,
> @@ -79,6 +80,8 @@ void radeon_pm_acpi_event_handler(struct radeon_device *rdev)
>  				radeon_dpm_enable_bapm(rdev, rdev->pm.dpm.ac_power);
>  		}
>  		mutex_unlock(&rdev->pm.mutex);
> +		/* allow new DPM state to be picked */
> +		radeon_pm_compute_clocks_dpm(rdev);
>  	} else if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
>  		if (rdev->pm.profile == PM_PROFILE_AUTO) {
>  			mutex_lock(&rdev->pm.mutex);
> @@ -882,7 +885,8 @@ static struct radeon_ps *radeon_dpm_pick_power_state(struct radeon_device *rdev,
>  		dpm_state = POWER_STATE_TYPE_INTERNAL_3DPERF;
>  	/* balanced states don't exist at the moment */
>  	if (dpm_state == POWER_STATE_TYPE_BALANCED)
> -		dpm_state = POWER_STATE_TYPE_PERFORMANCE;
> +		dpm_state = rdev->pm.dpm.ac_power ?
> +			POWER_STATE_TYPE_PERFORMANCE : POWER_STATE_TYPE_BATTERY;
>  
>  restart_search:
>  	/* Pick the best power state based on current conditions */
> 

I've been using this patch for a while on my Kaveri laptops and haven't
noticed any issues with it. I haven't compared the power consumption in
detail though.

Tested-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon/pm: autoswitch power state when in balanced mode

[Christian König]

Am 23.10.2016 um 01:05 schrieb Lucas Stach:
> The current default of always using the performance power state leads
> to increased power consumption of mobile devices, which have a dedicated
> battery power state. Switch between the performance and battery power
> state automatically, dpending on the current AC power status, when the
> user asked for the balanced power state.
>
> The user can still override this logic by asking for the performance
> or battery power state explicitly.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>

Nice addition, the only thing I can of hand see is that you probably 
want to remove the "balanced states don't exist at the moment" comment 
when you actually implement them (or abuse them).

Apart from that I'm not so deep into the PM stuff, so patch is only 
Acked-by: Christian König <christian.koenig@amd.com>.

Regards,
Christian.

> ---
> This saves about 1.2W on my Richland based laptop, whithout me having
> to remember to ask for the battery state or have userspace set up in
> a way to do this.
> ---
>   drivers/gpu/drm/radeon/radeon_pm.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
> index 4b65425..326ad06 100644
> --- a/drivers/gpu/drm/radeon/radeon_pm.c
> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
> @@ -47,6 +47,7 @@ static bool radeon_pm_in_vbl(struct radeon_device *rdev);
>   static bool radeon_pm_debug_check_in_vbl(struct radeon_device *rdev, bool finish);
>   static void radeon_pm_update_profile(struct radeon_device *rdev);
>   static void radeon_pm_set_clocks(struct radeon_device *rdev);
> +static void radeon_pm_compute_clocks_dpm(struct radeon_device *rdev);
>   
>   int radeon_pm_get_type_index(struct radeon_device *rdev,
>   			     enum radeon_pm_state_type ps_type,
> @@ -79,6 +80,8 @@ void radeon_pm_acpi_event_handler(struct radeon_device *rdev)
>   				radeon_dpm_enable_bapm(rdev, rdev->pm.dpm.ac_power);
>   		}
>   		mutex_unlock(&rdev->pm.mutex);
> +		/* allow new DPM state to be picked */
> +		radeon_pm_compute_clocks_dpm(rdev);
>   	} else if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
>   		if (rdev->pm.profile == PM_PROFILE_AUTO) {
>   			mutex_lock(&rdev->pm.mutex);
> @@ -882,7 +885,8 @@ static struct radeon_ps *radeon_dpm_pick_power_state(struct radeon_device *rdev,
>   		dpm_state = POWER_STATE_TYPE_INTERNAL_3DPERF;
>   	/* balanced states don't exist at the moment */
>   	if (dpm_state == POWER_STATE_TYPE_BALANCED)
> -		dpm_state = POWER_STATE_TYPE_PERFORMANCE;
> +		dpm_state = rdev->pm.dpm.ac_power ?
> +			POWER_STATE_TYPE_PERFORMANCE : POWER_STATE_TYPE_BATTERY;
>   
>   restart_search:
>   	/* Pick the best power state based on current conditions */


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon/pm: autoswitch power state when in balanced mode

[Alex Deucher]

On Mon, Oct 24, 2016 at 5:46 AM, Christian König
<christian.koenig@amd.com> wrote:
> Am 23.10.2016 um 01:05 schrieb Lucas Stach:
>>
>> The current default of always using the performance power state leads
>> to increased power consumption of mobile devices, which have a dedicated
>> battery power state. Switch between the performance and battery power
>> state automatically, dpending on the current AC power status, when the
>> user asked for the balanced power state.
>>
>> The user can still override this logic by asking for the performance
>> or battery power state explicitly.
>>
>> Signed-off-by: Lucas Stach <dev@lynxeye.de>
>
>
> Nice addition, the only thing I can of hand see is that you probably want to
> remove the "balanced states don't exist at the moment" comment when you
> actually implement them (or abuse them).
>
> Apart from that I'm not so deep into the PM stuff, so patch is only
> Acked-by: Christian König <christian.koenig@amd.com>.

IIRC, I had a similar patch years ago, and it was generally shot down
since it moved policy into the driver.  Also, certain userspace
packages like tlp do this already.  That said, I'm happy to apply it
if there are no objections.

Alex


>
> Regards,
> Christian.
>
>
>> ---
>> This saves about 1.2W on my Richland based laptop, whithout me having
>> to remember to ask for the battery state or have userspace set up in
>> a way to do this.
>> ---
>>   drivers/gpu/drm/radeon/radeon_pm.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c
>> b/drivers/gpu/drm/radeon/radeon_pm.c
>> index 4b65425..326ad06 100644
>> --- a/drivers/gpu/drm/radeon/radeon_pm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
>> @@ -47,6 +47,7 @@ static bool radeon_pm_in_vbl(struct radeon_device
>> *rdev);
>>   static bool radeon_pm_debug_check_in_vbl(struct radeon_device *rdev,
>> bool finish);
>>   static void radeon_pm_update_profile(struct radeon_device *rdev);
>>   static void radeon_pm_set_clocks(struct radeon_device *rdev);
>> +static void radeon_pm_compute_clocks_dpm(struct radeon_device *rdev);
>>     int radeon_pm_get_type_index(struct radeon_device *rdev,
>>                              enum radeon_pm_state_type ps_type,
>> @@ -79,6 +80,8 @@ void radeon_pm_acpi_event_handler(struct radeon_device
>> *rdev)
>>                                 radeon_dpm_enable_bapm(rdev,
>> rdev->pm.dpm.ac_power);
>>                 }
>>                 mutex_unlock(&rdev->pm.mutex);
>> +               /* allow new DPM state to be picked */
>> +               radeon_pm_compute_clocks_dpm(rdev);
>>         } else if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
>>                 if (rdev->pm.profile == PM_PROFILE_AUTO) {
>>                         mutex_lock(&rdev->pm.mutex);
>> @@ -882,7 +885,8 @@ static struct radeon_ps
>> *radeon_dpm_pick_power_state(struct radeon_device *rdev,
>>                 dpm_state = POWER_STATE_TYPE_INTERNAL_3DPERF;
>>         /* balanced states don't exist at the moment */
>>         if (dpm_state == POWER_STATE_TYPE_BALANCED)
>> -               dpm_state = POWER_STATE_TYPE_PERFORMANCE;
>> +               dpm_state = rdev->pm.dpm.ac_power ?
>> +                       POWER_STATE_TYPE_PERFORMANCE :
>> POWER_STATE_TYPE_BATTERY;
>>     restart_search:
>>         /* Pick the best power state based on current conditions */
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon/pm: autoswitch power state when in balanced mode

[Lucas Stach]

Am Montag, den 24.10.2016, 12:41 -0400 schrieb Alex Deucher:
> On Mon, Oct 24, 2016 at 5:46 AM, Christian König
> <christian.koenig@amd.com> wrote:
> > 
> > Am 23.10.2016 um 01:05 schrieb Lucas Stach:
> > > 
> > > 
> > > The current default of always using the performance power state
> > > leads
> > > to increased power consumption of mobile devices, which have a
> > > dedicated
> > > battery power state. Switch between the performance and battery
> > > power
> > > state automatically, dpending on the current AC power status,
> > > when the
> > > user asked for the balanced power state.
> > > 
> > > The user can still override this logic by asking for the
> > > performance
> > > or battery power state explicitly.
> > > 
> > > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> > 
> > 
> > Nice addition, the only thing I can of hand see is that you
> > probably want to
> > remove the "balanced states don't exist at the moment" comment when
> > you
> > actually implement them (or abuse them).
> > 
> > Apart from that I'm not so deep into the PM stuff, so patch is only
> > Acked-by: Christian König <christian.koenig@amd.com>.
> 
> IIRC, I had a similar patch years ago, and it was generally shot down
> since it moved policy into the driver.  Also, certain userspace
> packages like tlp do this already.  That said, I'm happy to apply it
> if there are no objections.

I can relate to that argument. But as there is an explicit "battery"
power state that's a strong hint that the hardware is designed to use
this state when running on battery power. This patch does not add any
new policy, but merely changes the one already present in the kernel
(clearly always using the "performance" power state in balanced mode
already is a policy on its own).

Also this patch doesn't prevent userspace to implement a different
policy.

I don't care deeply enough to try to convince anyone if there is
objection to this patch, but I think driving the hardware in the
designed way by default without the user needing to install additional
tools is a good thing.

Regards,
Lucas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon/pm: autoswitch power state when in balanced mode

[Daniel Vetter]

On Mon, Oct 24, 2016 at 12:41:04PM -0400, Alex Deucher wrote:
> On Mon, Oct 24, 2016 at 5:46 AM, Christian König
> <christian.koenig@amd.com> wrote:
> > Am 23.10.2016 um 01:05 schrieb Lucas Stach:
> >>
> >> The current default of always using the performance power state leads
> >> to increased power consumption of mobile devices, which have a dedicated
> >> battery power state. Switch between the performance and battery power
> >> state automatically, dpending on the current AC power status, when the
> >> user asked for the balanced power state.
> >>
> >> The user can still override this logic by asking for the performance
> >> or battery power state explicitly.
> >>
> >> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> >
> >
> > Nice addition, the only thing I can of hand see is that you probably want to
> > remove the "balanced states don't exist at the moment" comment when you
> > actually implement them (or abuse them).
> >
> > Apart from that I'm not so deep into the PM stuff, so patch is only
> > Acked-by: Christian König <christian.koenig@amd.com>.
> 
> IIRC, I had a similar patch years ago, and it was generally shot down
> since it moved policy into the driver.  Also, certain userspace
> packages like tlp do this already.  That said, I'm happy to apply it
> if there are no objections.

tbh I've stopped ignoring the "policy belongs into userspace" people for
areas where it essentially boils down to "we don't want to tune the hw
properly since it means more code is run and probably more bugs". I think
stuff like officially recommending that everyone run the autotune step of
powertop is just plain silly (and yes I'm argueing against my employer
here). We're trying to enable as much as possible by default (when it
makes sense), unfortunately there's many features with known bugs still :(

Anyway +1 from me for great defaults and auto-tuning drivers out of the
box. We're the kernel driver people, if we don't know how to best use the
hw, who else will? Certainly not users, and cargo-culting scripts doesn't
scale.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon/pm: autoswitch power state when in balanced mode

[Alex Deucher]

On Mon, Oct 24, 2016 at 5:31 PM, Lucas Stach <dev@lynxeye.de> wrote:
> Am Montag, den 24.10.2016, 12:41 -0400 schrieb Alex Deucher:
>> On Mon, Oct 24, 2016 at 5:46 AM, Christian König
>> <christian.koenig@amd.com> wrote:
>> >
>> > Am 23.10.2016 um 01:05 schrieb Lucas Stach:
>> > >
>> > >
>> > > The current default of always using the performance power state
>> > > leads
>> > > to increased power consumption of mobile devices, which have a
>> > > dedicated
>> > > battery power state. Switch between the performance and battery
>> > > power
>> > > state automatically, dpending on the current AC power status,
>> > > when the
>> > > user asked for the balanced power state.
>> > >
>> > > The user can still override this logic by asking for the
>> > > performance
>> > > or battery power state explicitly.
>> > >
>> > > Signed-off-by: Lucas Stach <dev@lynxeye.de>
>> >
>> >
>> > Nice addition, the only thing I can of hand see is that you
>> > probably want to
>> > remove the "balanced states don't exist at the moment" comment when
>> > you
>> > actually implement them (or abuse them).
>> >
>> > Apart from that I'm not so deep into the PM stuff, so patch is only
>> > Acked-by: Christian König <christian.koenig@amd.com>.
>>
>> IIRC, I had a similar patch years ago, and it was generally shot down
>> since it moved policy into the driver.  Also, certain userspace
>> packages like tlp do this already.  That said, I'm happy to apply it
>> if there are no objections.
>
> I can relate to that argument. But as there is an explicit "battery"
> power state that's a strong hint that the hardware is designed to use
> this state when running on battery power. This patch does not add any
> new policy, but merely changes the one already present in the kernel
> (clearly always using the "performance" power state in balanced mode
> already is a policy on its own).
>
> Also this patch doesn't prevent userspace to implement a different
> policy.
>
> I don't care deeply enough to try to convince anyone if there is
> objection to this patch, but I think driving the hardware in the
> designed way by default without the user needing to install additional
> tools is a good thing.

Applied.  Thanks!

Alex
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/radeon/pm: autoswitch power state when in balanced mode

[Lucas Stach]

The current default of always using the performance power state leads
to increased power consumption of mobile devices, which have a dedicated
battery power state. Switch between the performance and battery power
state automatically, depending on the current AC power status, when the
user asked for the balanced power state.

The user can still override this choice by asking for the performance
or battery power state explicitly.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
This saves about 1.2W for graphical desktop idle on my Richland based
laptop, whithout me having to remember to ask for the battery state or
the need to have userspace set up in a way to do this.
---
 drivers/gpu/drm/radeon/radeon_pm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
index 38226d9..9cc2bcf 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -47,6 +47,7 @@ static bool radeon_pm_in_vbl(struct radeon_device *rdev);
 static bool radeon_pm_debug_check_in_vbl(struct radeon_device *rdev, bool finish);
 static void radeon_pm_update_profile(struct radeon_device *rdev);
 static void radeon_pm_set_clocks(struct radeon_device *rdev);
+static void radeon_pm_compute_clocks_dpm(struct radeon_device *rdev);
 
 int radeon_pm_get_type_index(struct radeon_device *rdev,
 			     enum radeon_pm_state_type ps_type,
@@ -79,6 +80,8 @@ void radeon_pm_acpi_event_handler(struct radeon_device *rdev)
 				radeon_dpm_enable_bapm(rdev, rdev->pm.dpm.ac_power);
 		}
 		mutex_unlock(&rdev->pm.mutex);
+		/* allow new DPM state to be picked */
+		radeon_pm_compute_clocks_dpm(rdev);
 	} else if (rdev->pm.pm_method == PM_METHOD_PROFILE) {
 		if (rdev->pm.profile == PM_PROFILE_AUTO) {
 			mutex_lock(&rdev->pm.mutex);
@@ -877,7 +880,8 @@ static struct radeon_ps *radeon_dpm_pick_power_state(struct radeon_device *rdev,
 		dpm_state = POWER_STATE_TYPE_INTERNAL_3DPERF;
 	/* balanced states don't exist at the moment */
 	if (dpm_state == POWER_STATE_TYPE_BALANCED)
-		dpm_state = POWER_STATE_TYPE_PERFORMANCE;
+		dpm_state = rdev->pm.dpm.ac_power ?
+			POWER_STATE_TYPE_PERFORMANCE : POWER_STATE_TYPE_BATTERY;
 
 restart_search:
 	/* Pick the best power state based on current conditions */
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel