drm dynamic power off support

From: Dave Airlie <[email protected]>

For secondary GPUs in laptops, i.e. optimus or powerxpress, we have methods for powering down the GPU completely. This adds support to the drm core for powering back up the GPU on any access from ioctls or sysfs interfaces, and fires a 5s timer to test if we can power the GPU off.

This is just an initial implementation to get discussions started!

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/drm_drv.c       | 68 +++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_fb_helper.c |  2 +-
 drivers/gpu/drm/drm_fops.c      |  6 +++-
 drivers/gpu/drm/drm_stub.c      |  1 +
 drivers/gpu/drm/drm_sysfs.c     |  4 +++
 include/drm/drmP.h              |  9 ++++++
 6 files changed, 88 insertions(+), 2 deletions(-)

drivers/gpu/drm/drm_drv.c +10 -0

index 9238de4..9fae62a 100644
@@ -383,12 +383,17 @@ long drm_ioctl(struct file *filp,
 	char stack_kdata[128];
 	char *kdata = NULL;
 	unsigned int usize, asize;
+	int ret;
 
 	dev = file_priv->minor->dev;
 
 	if (drm_device_is_unplugged(dev))
 		return -ENODEV;
 
+	ret = drm_dynamic_power_wakeup(dev, __func__);
+	if (ret)
+		return ret;
+
 	atomic_inc(&dev->ioctl_count);
 	atomic_inc(&dev->counts[_DRM_STAT_IOCTLS]);
 	++file_priv->ioctl_count;

@@ -494,3 +499,66 @@ struct drm_local_map *drm_getsarea(struct drm_device *dev)
 	return NULL;
 }
 EXPORT_SYMBOL(drm_getsarea);
+
+#define POWER_OFF_PERIOD (5*HZ)
+
+static void drm_dynamic_enable_poll(struct drm_device *dev)
+{

drivers/gpu/drm/drm_drv.c +48 -0

+	ret = dev->driver->dynamic_off_check(dev);
+	if (ret == false)
+		goto out_requeue;
+
+	ret = dev->driver->dynamic_set_state(dev, DRM_SWITCH_POWER_DYNAMIC_OFF);
+	DRM_INFO("powering down\n");
+	return;
+out_requeue:
+	queue_delayed_work(system_nrt_wq, delayed_work, POWER_OFF_PERIOD);
+}
+
+int drm_dynamic_power_wakeup(struct drm_device *dev, const char *reason)
+{
+	int ret;
+
+	if (!dev->driver->dynamic_off_check)
+		return 0;
+
+	cancel_delayed_work_sync(&dev->dynamic_power_poll);
+
+	ret = mutex_lock_interruptible(&dev->dynamic_power_lock);
+	if (ret) {
+		drm_dynamic_enable_poll(dev);
+		return ret;
+	}
+
+	if (dev->switch_power_state != DRM_SWITCH_POWER_DYNAMIC_OFF) {
+		mutex_unlock(&dev->dynamic_power_lock);
+		drm_dynamic_enable_poll(dev);
+		return 0;
+	}
+
+	DRM_INFO("waking up GPU for %s\n", reason);
+	ret = dev->driver->dynamic_set_state(dev, DRM_SWITCH_POWER_ON);
+	mutex_unlock(&dev->dynamic_power_lock);
+
+	drm_dynamic_enable_poll(dev);
+	return 0;
+}
+EXPORT_SYMBOL(drm_dynamic_power_wakeup);
+
+void drm_dynamic_power_init(struct drm_device *dev)
+{
+	INIT_DELAYED_WORK(&dev->dynamic_power_poll, drm_power_poll_execute);
+	if (dev->driver->dynamic_off_check)
+		drm_dynamic_enable_poll(dev);
+}
+EXPORT_SYMBOL(drm_dynamic_power_init);

drivers/gpu/drm/drm_fb_helper.c +1 -1

index f546d1e..9a2c56b 100644
@@ -245,7 +245,7 @@ bool drm_fb_helper_force_kernel_mode(void)
 		return false;
 
 	list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) {
-		if (helper->dev->switch_power_state == DRM_SWITCH_POWER_OFF)
+		if (helper->dev->switch_power_state == DRM_SWITCH_POWER_OFF || helper->dev->switch_power_state == DRM_SWITCH_POWER_DYNAMIC_OFF)
 			continue;
 
 		ret = drm_fb_helper_restore_fbdev_mode(helper);

drivers/gpu/drm/drm_fops.c +5 -1

index 5062eec..285e53f 100644
@@ -239,9 +239,13 @@ static int drm_open_helper(struct inode *inode, struct file *filp,
 		return -EBUSY;	/* No exclusive opens */
 	if (!drm_cpu_valid())
 		return -EINVAL;
-	if (dev->switch_power_state != DRM_SWITCH_POWER_ON)
+	if (dev->switch_power_state != DRM_SWITCH_POWER_ON && dev->switch_power_state != DRM_SWITCH_POWER_DYNAMIC_OFF)
 		return -EINVAL;
 
+	ret = drm_dynamic_power_wakeup(dev, __func__);
+	if (ret)
+		return ret;
+
 	DRM_DEBUG("pid = %d, minor = %d\n", task_pid_nr(current), minor_id);
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);

drivers/gpu/drm/drm_stub.c +1 -0

index 21bcd4a..0e56a40 100644
@@ -273,6 +273,7 @@ int drm_fill_in_dev(struct drm_device *dev,
 	spin_lock_init(&dev->event_lock);
 	mutex_init(&dev->struct_mutex);
 	mutex_init(&dev->ctxlist_mutex);
+	mutex_init(&dev->dynamic_power_lock);
 
 	if (drm_ht_create(&dev->map_hash, 12)) {
 		return -ENOMEM;

drivers/gpu/drm/drm_sysfs.c +4 -0

index 45ac8d6..850c210 100644
@@ -162,6 +162,10 @@ static ssize_t status_show(struct device *device,
 	enum drm_connector_status status;
 	int ret;
 
+	ret = drm_dynamic_power_wakeup(connector->dev, __func__);
+	if (ret)
+		return ret;
+
 	ret = mutex_lock_interruptible(&connector->dev->mode_config.mutex);
 	if (ret)
 		return ret;

include/drm/drmP.h +9 -0

index d6b67bb..65154b0 100644
@@ -933,6 +933,9 @@ struct drm_driver {
 			    struct drm_device *dev,
 			    uint32_t handle);
 
+	bool (*dynamic_off_check)(struct drm_device *dev);
+	int (*dynamic_set_state)(struct drm_device *dev, int state);
+
 	/* Driver private ops for this object */
 	const struct vm_operations_struct *gem_vm_ops;
 

@@ -1197,11 +1200,15 @@ struct drm_device {
 	int switch_power_state;
 
 	atomic_t unplugged; /* device has been unplugged or gone away */
+
+	struct delayed_work dynamic_power_poll;
+	struct mutex dynamic_power_lock;
 };
 
 #define DRM_SWITCH_POWER_ON 0
 #define DRM_SWITCH_POWER_OFF 1
 #define DRM_SWITCH_POWER_CHANGING 2
+#define DRM_SWITCH_POWER_DYNAMIC_OFF 3
 
 static __inline__ int drm_core_check_feature(struct drm_device *dev,
 					     int feature)

@@ -1770,5 +1777,7 @@ static __inline__ bool drm_can_sleep(void)
 	return true;
 }
 
+void drm_dynamic_power_init(struct drm_device *dev);
+int drm_dynamic_power_wakeup(struct drm_device *dev, const char *reason);
 #endif				/* __KERNEL__ */
 #endif

Signature · 6 lines

1.7.12

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

My midlayer-smell-o-meter just cranked up to 11 when reading this comment ;-)

I'd have expected:
- Drivers to check the power state and enable the gpu if it's off in their cs ioctl (instead of the brute-force every ioctl there is approach in the drm core)

- Launch a delayed work item to cut the power again once they notice that the gpu is idle (dunno how radeon/nouveau work exactly, but i915 has this nice retire_requests work item which does a few similar things for power management, like lvds downclocking)

- Same thing for any other kind of usage, like e.g. kms: Drivers can wrap the crtc helper set_mode to ensure the gpu is on and also check for any enabled outputs before launching the delayed power off switch. Same applies to any sysfs/debugfs files (although in the case of i915.ko, many of these don't need the hw to be on).

I guess you could add a small vga_switcheroo_dynamic_power_state struct or something with a few helpers to do that to extract some duplicated code from drivers. But tbh managing a piece of state lazily with a timer/delayed work item is a common code pattern, so I don't think even that little bit of code sharing is worth it.

Cheers, Daniel

PS: I've read a bit around in the switcheroo code and I think the switcheroo ->can_switch callback is actually worse ... since the drm drivers just check the open_count (which is hilariously racy in itself, too) and there's no locking to ensure that stays the same between the ->can_switch check and the actual ->set_state calls ...

(oops' forgot reply to all)

Quoted 8 lines

My midlayer-smell-o-meter just cranked up to 11 when reading this comment ;-)

I'd have expected:
- Drivers to check the power state and enable the gpu if it's off in their cs ioctl (instead of the brute-force every ioctl there is approach in the drm core)

The problem is it won't just be the CS ioctl, so I just went with the larger hammer, I started annotating all the nouveau ioctls adding a wake up check at the top, and then realised that I need to start annotating all the non-gpu paths as well, like drm open, sysfs, basically anywhere that might cause us to access any GPU resources. Once I started annotating everywhere I realised just bashing it into the top most ioctl then sticking the whole lot into the drm core made sense. You have to think the device is gone completely, not the device is asleep. its not asleep, its effectively dead.

fine grained suspend/resume. Not sure if that concept helps here, but it may be worth digging around to see how they went about waking up individual devices.

(oops' forgot reply to all)

Quoted 8 lines

My midlayer-smell-o-meter just cranked up to 11 when reading this comment ;-)

I'd have expected:
- Drivers to check the power state and enable the gpu if it's off in their cs ioctl (instead of the brute-force every ioctl there is approach in the drm core)

The problem is it won't just be the CS ioctl, so I just went with the larger hammer, I started annotating all the nouveau ioctls adding a wake up check at the top, and then realised that I need to start annotating all the non-gpu paths as well, like drm open, sysfs, basically anywhere that might cause us to access any GPU resources. Once I started annotating everywhere I realised just bashing it into the top most ioctl then sticking the whole lot into the drm core made sense. You have to think the device is gone completely, not the device is asleep. its not asleep, its effectively dead.

Quoted 10 lines

- Launch a delayed work item to cut the power again once they notice that the gpu is idle (dunno how radeon/nouveau work exactly, but i915 has this nice retire_requests work item which does a few similar things for power management, like lvds downclocking)

- Same thing for any other kind of usage, like e.g. kms: Drivers can wrap the crtc helper set_mode to ensure the gpu is on and also check for any enabled outputs before launching the delayed power off switch. Same applies to any sysfs/debugfs files (although in the case of i915.ko, many of these don't need the hw to be on).

Yeah this is probably the way I'd like it to end up alright, it'll just be a lot more code and a shit load of missed corner cases until I get it right. So I preferred to start with something I knew would work..

Quoted 6 lines

I guess you could add a small vga_switcheroo_dynamic_power_state struct or something with a few helpers to do that to extract some duplicated code from drivers. But tbh managing a piece of state lazily with a timer/delayed work item is a common code pattern, so I don't think even that little bit of code sharing is worth it.

Quoted 7 lines

Cheers, Daniel

PS: I've read a bit around in the switcheroo code and I think the switcheroo ->can_switch callback is actually worse ... since the drm drivers just check the open_count (which is hilariously racy in itself, too) and there's no locking to ensure that stays the same between the ->can_switch check and the actual ->set_state calls ...

Yeah it just works by the fact that things are slow, but yeah in theory would need to lock-out userspace from accessing drm_open during the timeframe. The problem is doing that without userspace suddenly getting an error it didn't get before and things not working at all.

From: Dave Airlie <[email protected]>

For secondary GPUs in laptops, i.e. optimus or powerxpress, we have methods for powering down the GPU completely. This adds support to the drm core for powering back up the GPU on any access from ioctls or sysfs interfaces, and fires a 5s timer to test if we can power the GPU off.

Quoted 7 lines · On Mon, 10 Sep 2012 14:31:52 +1000 Dave Airlie <[email protected]> wrote:

From: Dave Airlie <[email protected]>

For secondary GPUs in laptops, i.e. optimus or powerxpress, we have methods for powering down the GPU completely. This adds support to the drm core for powering back up the GPU on any access from ioctls or sysfs interfaces, and fires a 5s timer to test if we can power the GPU off.

Is there a reason for basically re-inventing the existing Linux dynamic power management layer ?

Hi Dave,

drivers/gpu/drm/nouveau/nouveau_acpi.c static struct nouveau_dsm_priv { · line 40

Quoted 3 lines

drivers/gpu/drm/nouveau/nouveau_acpi.c +3 -0

+bool nouveau_is_optimus(void) {
+     return nouveau_dsm_priv.optimus_detected;
+}

This will only work for new Optimus laptops, not the older ones such as your Lenovo T410s or on a Dell Vostro 3500 with a GT 310M. Is that intended? If not, check for those models too:

For optimus and powerxpress setups where we can explicitly power off the GPU I'd like to do this under control of the driver. Now that I've added X server support for secondary GPUs, it makes sense to start powering them down more often if we can.

I've tested this code on two laptops, one intel/nouveau one intel/radeon It works, powertop says I use 5-6W less power.

Caveats:
There is a race at X server start with platform probing code, if the secondary device is off, we the wrong PCI info for it, I've got a patch that works around this I'll send to the xorg-devel.

Audio seems to get screwed at least on one machine, we power up and the alsa callbacks into snd_hda_intel get called but it can't find the hw properly, need to investigate a bit further.

Dave.

Quoted 13 lines · Dave Airlie

For optimus and powerxpress setups where we can explicitly power off the GPU I'd like to do this under control of the driver. Now that I've added X server support for secondary GPUs, it makes sense to start powering them down more often if we can.

I've tested this code on two laptops, one intel/nouveau one intel/radeon It works, powertop says I use 5-6W less power.

Caveats:
There is a race at X server start with platform probing code, if the secondary device is off, we the wrong PCI info for it, I've got a patch that works around this I'll send to the xorg-devel.

Quoted 6 lines

Audio seems to get screwed at least on one machine, we power up and the alsa callbacks into snd_hda_intel get called but it can't find the hw properly, need to investigate a bit further.

Dave.

Hi Takashi,

just wondering how well setup alsa would be for the dGPU powering up/down a lot more often,
139.529103] nouveau [ DRM][0000:01:00.0] resuming display...

[  139.833960] ALSA sound/pci/hda/hda_intel.c:2533 Enabling
0000:01:00.1 via VGA-switcheroo
[  139.844789] snd_hda_intel 0000:01:00.1: Refused to change power
state, currently in D3
[  139.915760] snd_hda_intel 0000:01:00.1: Refused to change power

state, currently in D3

[  140.917437] ALSA sound/pci/hda/hda_intel.c:813 spurious response
0x0:0x3, last cmd=0x301f0500
[  140.917449] ALSA sound/pci/hda/hda_intel.c:813 spurious response
0x0:0x3, last cmd=0x301f0500
[  140.917455] ALSA sound/pci/hda/hda_intel.c:813 spurious response
0x0:0x3, last cmd=0x301f0500
[  140.917460] ALSA sound/pci/hda/hda_intel.c:813 spurious response
0x0:0x3, last cmd=0x301f0500
[  140.917465] ALSA sound/pci/hda/hda_intel.c:813 spurious response

0x0:0x3, last cmd=0x301f0500

is just some of the things I see, if I turn off before snd_hda_intel, things go badly wrong when
I do power up the dGPU, if I delay the power off until audio is loaded, I start to see wierd things when pulseaudio starts when the dGPU is off.

At Mon, 10 Sep 2012 15:04:02 +1000,

Quoted 26 lines · Dave Airlie

Quoted 13 lines · Dave Airlie

For optimus and powerxpress setups where we can explicitly power off the GPU I'd like to do this under control of the driver. Now that I've added X server support for secondary GPUs, it makes sense to start powering them down more often if we can.

I've tested this code on two laptops, one intel/nouveau one intel/radeon It works, powertop says I use 5-6W less power.

Caveats:
There is a race at X server start with platform probing code, if the secondary device is off, we the wrong PCI info for it, I've got a patch that works around this I'll send to the xorg-devel.

Quoted 6 lines

Audio seems to get screwed at least on one machine, we power up and the alsa callbacks into snd_hda_intel get called but it can't find the hw properly, need to investigate a bit further.

Dave.

Hi Takashi,

just wondering how well setup alsa would be for the dGPU powering up/down a lot more often,
139.529103] nouveau [ DRM][0000:01:00.0] resuming display...

Quoted 6 lines

[  139.833960] ALSA sound/pci/hda/hda_intel.c:2533 Enabling
0000:01:00.1 via VGA-switcheroo
[  139.844789] snd_hda_intel 0000:01:00.1: Refused to change power
state, currently in D3
[  139.915760] snd_hda_intel 0000:01:00.1: Refused to change power

state, currently in D3

These come from PCI core, and...

Quoted 10 lines

[  140.917437] ALSA sound/pci/hda/hda_intel.c:813 spurious response
0x0:0x3, last cmd=0x301f0500
[  140.917449] ALSA sound/pci/hda/hda_intel.c:813 spurious response
0x0:0x3, last cmd=0x301f0500
[  140.917455] ALSA sound/pci/hda/hda_intel.c:813 spurious response
0x0:0x3, last cmd=0x301f0500
[  140.917460] ALSA sound/pci/hda/hda_intel.c:813 spurious response
0x0:0x3, last cmd=0x301f0500
[  140.917465] ALSA sound/pci/hda/hda_intel.c:813 spurious response

0x0:0x3, last cmd=0x301f0500

These are from hda-codec. The verb 0x301f0500 is GET_POWER_STATE verb, so it looks like that the hardware doesn't respond to any h/w state query / change.

Quoted 5 lines

is just some of the things I see, if I turn off before snd_hda_intel, things go badly wrong when
I do power up the dGPU, if I delay the power off until audio is loaded, I start to see wierd things when pulseaudio starts when the dGPU is off.

What does your patch do? Sorry, I still haven't followed your patch yet.

The message from PCI core makes me wonder whether the GPU is really active at that point. Since it's just a call of pci_set_power_state(PCI_D0) at the beginning of resume procedure, it's rather a problem in a deeper level than the audio controller itself. The following spurious response messages are likely the result of the controller being still in D3.

Quoted 52 lines · Takashi Iwai

At Mon, 10 Sep 2012 15:04:02 +1000,

Quoted 26 lines · Dave Airlie

Quoted 13 lines · Dave Airlie

For optimus and powerxpress setups where we can explicitly power off the GPU I'd like to do this under control of the driver. Now that I've added X server support for secondary GPUs, it makes sense to start powering them down more often if we can.

I've tested this code on two laptops, one intel/nouveau one intel/radeon It works, powertop says I use 5-6W less power.

Caveats:
There is a race at X server start with platform probing code, if the secondary device is off, we the wrong PCI info for it, I've got a patch that works around this I'll send to the xorg-devel.

Quoted 6 lines

Audio seems to get screwed at least on one machine, we power up and the alsa callbacks into snd_hda_intel get called but it can't find the hw properly, need to investigate a bit further.

Dave.

Hi Takashi,

just wondering how well setup alsa would be for the dGPU powering up/down a lot more often,
139.529103] nouveau [ DRM][0000:01:00.0] resuming display...

Quoted 6 lines

[  139.833960] ALSA sound/pci/hda/hda_intel.c:2533 Enabling
0000:01:00.1 via VGA-switcheroo
[  139.844789] snd_hda_intel 0000:01:00.1: Refused to change power
state, currently in D3
[  139.915760] snd_hda_intel 0000:01:00.1: Refused to change power

state, currently in D3

These come from PCI core, and...

Quoted 10 lines

[  140.917437] ALSA sound/pci/hda/hda_intel.c:813 spurious response
0x0:0x3, last cmd=0x301f0500
[  140.917449] ALSA sound/pci/hda/hda_intel.c:813 spurious response
0x0:0x3, last cmd=0x301f0500
[  140.917455] ALSA sound/pci/hda/hda_intel.c:813 spurious response
0x0:0x3, last cmd=0x301f0500
[  140.917460] ALSA sound/pci/hda/hda_intel.c:813 spurious response
0x0:0x3, last cmd=0x301f0500
[  140.917465] ALSA sound/pci/hda/hda_intel.c:813 spurious response

0x0:0x3, last cmd=0x301f0500

These are from hda-codec. The verb 0x301f0500 is GET_POWER_STATE verb, so it looks like that the hardware doesn't respond to any h/w state query / change.

Quoted 8 lines

Quoted 5 lines

is just some of the things I see, if I turn off before snd_hda_intel, things go badly wrong when
I do power up the dGPU, if I delay the power off until audio is loaded, I start to see wierd things when pulseaudio starts when the dGPU is off.

What does your patch do? Sorry, I still haven't followed your patch yet.

It turns the GPU off completely on a timer, so 5s after we have no activity we cut the power to the GPU completely,

but I call the switcheroo callbacks properly and it should be bringing the power back up correctly, unless there is some initialisation delay or the audio hw comes up in a wierd state.

Though it should be no different than using the ON/OFF stuff that we have now.

Quoted 6 lines

The message from PCI core makes me wonder whether the GPU is really active at that point. Since it's just a call of pci_set_power_state(PCI_D0) at the beginning of resume procedure, it's rather a problem in a deeper level than the audio controller itself. The following spurious response messages are likely the result of the controller being still in D3.

That can happen where the device has gone into a really wierd place and just won't come back

I might try adding some delays tomorrow.