| Message ID | 1429824493-3162-2-git-send-email-dev@lynxeye.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Lucas Stach [mailto:dev@lynxeye.de] > Sent: Thursday, April 23, 2015 5:28 PM > To: Deucher, Alexander; Koenig, Christian; David Airlie > Cc: dri-devel@lists.freedesktop.org > Subject: [PATCH 2/2] drm/radeon: remove bapm callbacks > > Trying to disable BAPM on battery power does not fix the problematic > Trinity mobile parts. Fixing those probably need a more complex solution, > like doing a complete reinit of the DPM state. This will take more work > and most likely it will not be possible to map this to a single callback. > > Even worse trying to change the BAPM state mid flight is breaking BAPM > on Richland mobile parts that are otherwise completely stable with BAPM > enabled. Do you have any examples of this? This was added to fix bapm stability on mobile parts and did help on a number of systems. Bapm is disabled by default since it is so problematic on a lot of systems. Ripping all of this out doesn't seem like it will really help anything. Alex > > As there are no users of this hook anymore we can just remove the calling > infrastructure. > > Signed-off-by: Lucas Stach <dev@lynxeye.de> > --- > I've did some pretty extensive tests with one of the problematic Trinity > mobile parts. I was able to figure out a stable config with BAPM enabled > for this, but unfortunately the power consumption rose quite a bit. So on > Trinity one has to disable BAPM while on battery power. I found a sequence Can you elaborate on this? This seems to contradict what the patch does. > to enable/disable BAPM mid flight without hanging the GPU, but the SMC > seemed to be stuck after that, as no DPM power level changes were > happening > anymore. > --- > drivers/gpu/drm/radeon/radeon.h | 2 -- > drivers/gpu/drm/radeon/radeon_asic.c | 1 - > drivers/gpu/drm/radeon/radeon_asic.h | 1 - > drivers/gpu/drm/radeon/radeon_pm.c | 4 ---- > drivers/gpu/drm/radeon/trinity_dpm.c | 11 ----------- > 5 files changed, 19 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon.h > b/drivers/gpu/drm/radeon/radeon.h > index 33d5a4f..93659d2 100644 > --- a/drivers/gpu/drm/radeon/radeon.h > +++ b/drivers/gpu/drm/radeon/radeon.h > @@ -1980,7 +1980,6 @@ struct radeon_asic { > int (*force_performance_level)(struct radeon_device *rdev, > enum radeon_dpm_forced_level level); > bool (*vblank_too_short)(struct radeon_device *rdev); > void (*powergate_uvd)(struct radeon_device *rdev, bool > gate); > - void (*enable_bapm)(struct radeon_device *rdev, bool > enable); > void (*fan_ctrl_set_mode)(struct radeon_device *rdev, u32 > mode); > u32 (*fan_ctrl_get_mode)(struct radeon_device *rdev); > int (*set_fan_speed_percent)(struct radeon_device *rdev, > u32 speed); > @@ -2949,7 +2948,6 @@ static inline void radeon_ring_write(struct > radeon_ring *ring, uint32_t v) > #define radeon_dpm_force_performance_level(rdev, l) rdev->asic- > >dpm.force_performance_level((rdev), (l)) > #define radeon_dpm_vblank_too_short(rdev) rdev->asic- > >dpm.vblank_too_short((rdev)) > #define radeon_dpm_powergate_uvd(rdev, g) rdev->asic- > >dpm.powergate_uvd((rdev), (g)) > -#define radeon_dpm_enable_bapm(rdev, e) rdev->asic- > >dpm.enable_bapm((rdev), (e)) > > /* Common functions */ > /* AGP */ > diff --git a/drivers/gpu/drm/radeon/radeon_asic.c > b/drivers/gpu/drm/radeon/radeon_asic.c > index 2a33e38..fa11abc 100644 > --- a/drivers/gpu/drm/radeon/radeon_asic.c > +++ b/drivers/gpu/drm/radeon/radeon_asic.c > @@ -1818,7 +1818,6 @@ static struct radeon_asic trinity_asic = { > .print_power_state = &trinity_dpm_print_power_state, > .debugfs_print_current_performance_level = > &trinity_dpm_debugfs_print_current_performance_level, > .force_performance_level = > &trinity_dpm_force_performance_level, > - .enable_bapm = &trinity_dpm_enable_bapm, > }, > .pflip = { > .page_flip = &evergreen_page_flip, > diff --git a/drivers/gpu/drm/radeon/radeon_asic.h > b/drivers/gpu/drm/radeon/radeon_asic.h > index 85d76da..605be51 100644 > --- a/drivers/gpu/drm/radeon/radeon_asic.h > +++ b/drivers/gpu/drm/radeon/radeon_asic.h > @@ -673,7 +673,6 @@ void > trinity_dpm_debugfs_print_current_performance_level(struct > radeon_device *r > struct seq_file *m); > int trinity_dpm_force_performance_level(struct radeon_device *rdev, > enum radeon_dpm_forced_level > level); > -void trinity_dpm_enable_bapm(struct radeon_device *rdev, bool enable); > > /* DCE6 - SI */ > void dce6_bandwidth_update(struct radeon_device *rdev); > diff --git a/drivers/gpu/drm/radeon/radeon_pm.c > b/drivers/gpu/drm/radeon/radeon_pm.c > index c1ba83a..a78d146 100644 > --- a/drivers/gpu/drm/radeon/radeon_pm.c > +++ b/drivers/gpu/drm/radeon/radeon_pm.c > @@ -74,10 +74,6 @@ void radeon_pm_acpi_event_handler(struct > radeon_device *rdev) > rdev->pm.dpm.ac_power = true; > else > rdev->pm.dpm.ac_power = false; > - if (rdev->family == CHIP_ARUBA) { > - if (rdev->asic->dpm.enable_bapm) > - radeon_dpm_enable_bapm(rdev, rdev- > >pm.dpm.ac_power); > - } > mutex_unlock(&rdev->pm.mutex); > } else if (rdev->pm.pm_method == PM_METHOD_PROFILE) { > if (rdev->pm.profile == PM_PROFILE_AUTO) { > diff --git a/drivers/gpu/drm/radeon/trinity_dpm.c > b/drivers/gpu/drm/radeon/trinity_dpm.c > index 38dacb7..e473455 100644 > --- a/drivers/gpu/drm/radeon/trinity_dpm.c > +++ b/drivers/gpu/drm/radeon/trinity_dpm.c > @@ -1069,17 +1069,6 @@ static void trinity_update_requested_ps(struct > radeon_device *rdev, > pi->requested_rps.ps_priv = &pi->requested_ps; > } > > -void trinity_dpm_enable_bapm(struct radeon_device *rdev, bool enable) > -{ > - struct trinity_power_info *pi = trinity_get_pi(rdev); > - > - if (pi->enable_bapm) { > - trinity_acquire_mutex(rdev); > - trinity_dpm_bapm_enable(rdev, enable); > - trinity_release_mutex(rdev); > - } > -} > - > int trinity_dpm_enable(struct radeon_device *rdev) > { > struct trinity_power_info *pi = trinity_get_pi(rdev); > -- > 2.1.0
Am Donnerstag, den 23.04.2015, 21:41 +0000 schrieb Deucher, Alexander: > > -----Original Message----- > > From: Lucas Stach [mailto:dev@lynxeye.de] > > Sent: Thursday, April 23, 2015 5:28 PM > > To: Deucher, Alexander; Koenig, Christian; David Airlie > > Cc: dri-devel@lists.freedesktop.org > > Subject: [PATCH 2/2] drm/radeon: remove bapm callbacks > > > > Trying to disable BAPM on battery power does not fix the problematic > > Trinity mobile parts. Fixing those probably need a more complex solution, > > like doing a complete reinit of the DPM state. This will take more work > > and most likely it will not be possible to map this to a single callback. > > > > Even worse trying to change the BAPM state mid flight is breaking BAPM > > on Richland mobile parts that are otherwise completely stable with BAPM > > enabled. > > Do you have any examples of this? This was added to fix bapm > stability on mobile parts and did help on a number of systems. Bapm > is disabled by default since it is so problematic on a lot of systems. > Ripping all of this out doesn't seem like it will really help > anything. > I have a A10-5750M (Richland) that runs completely stable with BAPM enabled. The SMC seems to do the right thing on it's own as I can see that the CPU clock rate is constrained to the highest non-turbo clock when on battery power while it boosts up to max-turbo when on AC and sufficient thermal headroom is there. Trying to disable BAPM mid flight is the only thing that is able to make it hang. So for this machine the "fix" is clearly to blame for an instability. > Alex > > > > > As there are no users of this hook anymore we can just remove the calling > > infrastructure. > > > > Signed-off-by: Lucas Stach <dev@lynxeye.de> > > --- > > I've did some pretty extensive tests with one of the problematic Trinity > > mobile parts. I was able to figure out a stable config with BAPM enabled > > for this, but unfortunately the power consumption rose quite a bit. So on > > Trinity one has to disable BAPM while on battery power. I found a sequence > > Can you elaborate on this? This seems to contradict what the patch does. > This patch rips out the BAPM callback as it is most likely not sufficient to fix the stability problems. So to describe a bit more what I found on a A8-4500M (Trinity): BAPM is only stable with power gating disabled. Unfortunately disabling power gating adds about 1.5W to the idle power consumption, enabling BAPM adds another 8W to this, so this is clearly unacceptable on battery power. Or maybe it is if the user explicitly requested the performance power state. When properly en-/disabling power gating before a BAPM transition I can get a stable transition without hanging the GPU. I can see that it works as CPU clock rates are constrained to non-turbo without BAPM while they go up quite a bit (but not to max-turbo) with BAPM enabled. Now the problem is that after switching BAPM off for the first time the GPU is stuck at whatever power level it was at the moment of the BAPM transition. This can be any power level, low power if the transition happened with GPU idle or even the highest power level if the GPU was rendering at that moment. So simply telling the SMC to switch off BAPM is clearly not doing the right thing. Most likely the DPM state needs to be reinitialized when switching BAPM. The battery power state even has a flag to disable BAPM. So I think the right thing to do when switching between AC/DC is to do a complete re-evaluation of the power state and to do a complete DPM power state switch if necessary. The simple BAPM on/off callback won't be of much help for this. Regards, Lucas
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 33d5a4f..93659d2 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -1980,7 +1980,6 @@ struct radeon_asic { int (*force_performance_level)(struct radeon_device *rdev, enum radeon_dpm_forced_level level); bool (*vblank_too_short)(struct radeon_device *rdev); void (*powergate_uvd)(struct radeon_device *rdev, bool gate); - void (*enable_bapm)(struct radeon_device *rdev, bool enable); void (*fan_ctrl_set_mode)(struct radeon_device *rdev, u32 mode); u32 (*fan_ctrl_get_mode)(struct radeon_device *rdev); int (*set_fan_speed_percent)(struct radeon_device *rdev, u32 speed); @@ -2949,7 +2948,6 @@ static inline void radeon_ring_write(struct radeon_ring *ring, uint32_t v) #define radeon_dpm_force_performance_level(rdev, l) rdev->asic->dpm.force_performance_level((rdev), (l)) #define radeon_dpm_vblank_too_short(rdev) rdev->asic->dpm.vblank_too_short((rdev)) #define radeon_dpm_powergate_uvd(rdev, g) rdev->asic->dpm.powergate_uvd((rdev), (g)) -#define radeon_dpm_enable_bapm(rdev, e) rdev->asic->dpm.enable_bapm((rdev), (e)) /* Common functions */ /* AGP */ diff --git a/drivers/gpu/drm/radeon/radeon_asic.c b/drivers/gpu/drm/radeon/radeon_asic.c index 2a33e38..fa11abc 100644 --- a/drivers/gpu/drm/radeon/radeon_asic.c +++ b/drivers/gpu/drm/radeon/radeon_asic.c @@ -1818,7 +1818,6 @@ static struct radeon_asic trinity_asic = { .print_power_state = &trinity_dpm_print_power_state, .debugfs_print_current_performance_level = &trinity_dpm_debugfs_print_current_performance_level, .force_performance_level = &trinity_dpm_force_performance_level, - .enable_bapm = &trinity_dpm_enable_bapm, }, .pflip = { .page_flip = &evergreen_page_flip, diff --git a/drivers/gpu/drm/radeon/radeon_asic.h b/drivers/gpu/drm/radeon/radeon_asic.h index 85d76da..605be51 100644 --- a/drivers/gpu/drm/radeon/radeon_asic.h +++ b/drivers/gpu/drm/radeon/radeon_asic.h @@ -673,7 +673,6 @@ void trinity_dpm_debugfs_print_current_performance_level(struct radeon_device *r struct seq_file *m); int trinity_dpm_force_performance_level(struct radeon_device *rdev, enum radeon_dpm_forced_level level); -void trinity_dpm_enable_bapm(struct radeon_device *rdev, bool enable); /* DCE6 - SI */ void dce6_bandwidth_update(struct radeon_device *rdev); diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c index c1ba83a..a78d146 100644 --- a/drivers/gpu/drm/radeon/radeon_pm.c +++ b/drivers/gpu/drm/radeon/radeon_pm.c @@ -74,10 +74,6 @@ void radeon_pm_acpi_event_handler(struct radeon_device *rdev) rdev->pm.dpm.ac_power = true; else rdev->pm.dpm.ac_power = false; - if (rdev->family == CHIP_ARUBA) { - if (rdev->asic->dpm.enable_bapm) - radeon_dpm_enable_bapm(rdev, rdev->pm.dpm.ac_power); - } mutex_unlock(&rdev->pm.mutex); } else if (rdev->pm.pm_method == PM_METHOD_PROFILE) { if (rdev->pm.profile == PM_PROFILE_AUTO) { diff --git a/drivers/gpu/drm/radeon/trinity_dpm.c b/drivers/gpu/drm/radeon/trinity_dpm.c index 38dacb7..e473455 100644 --- a/drivers/gpu/drm/radeon/trinity_dpm.c +++ b/drivers/gpu/drm/radeon/trinity_dpm.c @@ -1069,17 +1069,6 @@ static void trinity_update_requested_ps(struct radeon_device *rdev, pi->requested_rps.ps_priv = &pi->requested_ps; } -void trinity_dpm_enable_bapm(struct radeon_device *rdev, bool enable) -{ - struct trinity_power_info *pi = trinity_get_pi(rdev); - - if (pi->enable_bapm) { - trinity_acquire_mutex(rdev); - trinity_dpm_bapm_enable(rdev, enable); - trinity_release_mutex(rdev); - } -} - int trinity_dpm_enable(struct radeon_device *rdev) { struct trinity_power_info *pi = trinity_get_pi(rdev);
Trying to disable BAPM on battery power does not fix the problematic Trinity mobile parts. Fixing those probably need a more complex solution, like doing a complete reinit of the DPM state. This will take more work and most likely it will not be possible to map this to a single callback. Even worse trying to change the BAPM state mid flight is breaking BAPM on Richland mobile parts that are otherwise completely stable with BAPM enabled. As there are no users of this hook anymore we can just remove the calling infrastructure. Signed-off-by: Lucas Stach <dev@lynxeye.de> --- I've did some pretty extensive tests with one of the problematic Trinity mobile parts. I was able to figure out a stable config with BAPM enabled for this, but unfortunately the power consumption rose quite a bit. So on Trinity one has to disable BAPM while on battery power. I found a sequence to enable/disable BAPM mid flight without hanging the GPU, but the SMC seemed to be stuck after that, as no DPM power level changes were happening anymore. --- drivers/gpu/drm/radeon/radeon.h | 2 -- drivers/gpu/drm/radeon/radeon_asic.c | 1 - drivers/gpu/drm/radeon/radeon_asic.h | 1 - drivers/gpu/drm/radeon/radeon_pm.c | 4 ---- drivers/gpu/drm/radeon/trinity_dpm.c | 11 ----------- 5 files changed, 19 deletions(-)