[RFC] drm dynamic power off support

[RFC] drm dynamic power off support

[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.

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.

[PATCH 1/5] gpu/vga_switcheroo: add driver control power feature.

[Dave Airlie]

From: Dave Airlie <airlied@dhcp-40-90.bne.redhat.com>

For optimus and powerxpress muxless we really want the GPU
driver deciding when to power up/down the GPU, not userspace.

This adds the ability for a driver to dynamically power up/down
the GPU and remove the switcheroo from controlling it, the
switcheroo reports the dynamic state to userspace also.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/i915/i915_dma.c        |  2 +-
 drivers/gpu/drm/nouveau/nouveau_vga.c  |  2 +-
 drivers/gpu/drm/radeon/radeon_device.c |  2 +-
 drivers/gpu/vga/vga_switcheroo.c       | 44 ++++++++++++++++++++++++++++++----
 include/linux/vga_switcheroo.h         |  6 ++++-
 5 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 9cf7dfe..a377204 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1302,7 +1302,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_register_dsm_handler();
 
-	ret = vga_switcheroo_register_client(dev->pdev, &i915_switcheroo_ops);
+	ret = vga_switcheroo_register_client(dev->pdev, &i915_switcheroo_ops, false);
 	if (ret)
 		goto cleanup_vga_client;
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index 7bf7d13..37fcc9d 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -80,7 +80,7 @@ nouveau_vga_init(struct nouveau_drm *drm)
 {
 	struct drm_device *dev = drm->dev;
 	vga_client_register(dev->pdev, dev, NULL, nouveau_vga_set_decode);
-	vga_switcheroo_register_client(dev->pdev, &nouveau_switcheroo_ops);
+	vga_switcheroo_register_client(dev->pdev, &nouveau_switcheroo_ops, false);
 }
 
 void
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 7a3daeb..857cf5b 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1093,7 +1093,7 @@ int radeon_device_init(struct radeon_device *rdev,
 	/* this will fail for cards that aren't VGA class devices, just
 	 * ignore it */
 	vga_client_register(rdev->pdev, rdev, NULL, radeon_vga_set_decode);
-	vga_switcheroo_register_client(rdev->pdev, &radeon_switcheroo_ops);
+	vga_switcheroo_register_client(rdev->pdev, &radeon_switcheroo_ops, false);
 
 	r = radeon_init(rdev);
 	if (r)
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index e25cf31..efd147a 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -37,6 +37,7 @@ struct vga_switcheroo_client {
 	const struct vga_switcheroo_client_ops *ops;
 	int id;
 	bool active;
+	bool driver_power_control;
 	struct list_head list;
 };
 
@@ -132,7 +133,7 @@ EXPORT_SYMBOL(vga_switcheroo_unregister_handler);
 
 static int register_client(struct pci_dev *pdev,
 			   const struct vga_switcheroo_client_ops *ops,
-			   int id, bool active)
+			   int id, bool active, bool driver_power_control)
 {
 	struct vga_switcheroo_client *client;
 
@@ -145,6 +146,7 @@ static int register_client(struct pci_dev *pdev,
 	client->ops = ops;
 	client->id = id;
 	client->active = active;
+	client->driver_power_control = driver_power_control;
 
 	mutex_lock(&vgasr_mutex);
 	list_add_tail(&client->list, &vgasr_priv.clients);
@@ -160,10 +162,11 @@ static int register_client(struct pci_dev *pdev,
 }
 
 int vga_switcheroo_register_client(struct pci_dev *pdev,
-				   const struct vga_switcheroo_client_ops *ops)
+				   const struct vga_switcheroo_client_ops *ops,
+				   bool driver_power_control)
 {
 	return register_client(pdev, ops, -1,
-			       pdev == vga_default_device());
+			       pdev == vga_default_device(), driver_power_control);
 }
 EXPORT_SYMBOL(vga_switcheroo_register_client);
 
@@ -171,7 +174,7 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 					 const struct vga_switcheroo_client_ops *ops,
 					 int id, bool active)
 {
-	return register_client(pdev, ops, id | ID_BIT_AUDIO, active);
+	return register_client(pdev, ops, id | ID_BIT_AUDIO, active, false);
 }
 EXPORT_SYMBOL(vga_switcheroo_register_audio_client);
 
@@ -258,10 +261,11 @@ static int vga_switcheroo_show(struct seq_file *m, void *v)
 	int i = 0;
 	mutex_lock(&vgasr_mutex);
 	list_for_each_entry(client, &vgasr_priv.clients, list) {
-		seq_printf(m, "%d:%s%s:%c:%s:%s\n", i,
+		seq_printf(m, "%d:%s%s:%c:%s%s:%s\n", i,
 			   client_id(client) == VGA_SWITCHEROO_DIS ? "DIS" : "IGD",
 			   client_is_vga(client) ? "" : "-Audio",
 			   client->active ? '+' : ' ',
+			   client->driver_power_control ? "Dyn" : "",
 			   client->pwr_state ? "Pwr" : "Off",
 			   pci_name(client->pdev));
 		i++;
@@ -277,6 +281,8 @@ static int vga_switcheroo_debugfs_open(struct inode *inode, struct file *file)
 
 static int vga_switchon(struct vga_switcheroo_client *client)
 {
+	if (client->driver_power_control)
+		return 0;
 	if (vgasr_priv.handler->power_state)
 		vgasr_priv.handler->power_state(client->id, VGA_SWITCHEROO_ON);
 	/* call the driver callback to turn on device */
@@ -287,6 +293,8 @@ static int vga_switchon(struct vga_switcheroo_client *client)
 
 static int vga_switchoff(struct vga_switcheroo_client *client)
 {
+	if (client->driver_power_control)
+		return 0;
 	/* call the driver callback to turn off device */
 	client->ops->set_gpu_state(client->pdev, VGA_SWITCHEROO_OFF);
 	if (vgasr_priv.handler->power_state)
@@ -401,6 +409,8 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
 		list_for_each_entry(client, &vgasr_priv.clients, list) {
 			if (client->active || client_is_audio(client))
 				continue;
+			if (client->driver_power_control)
+				continue;
 			set_audio_state(client->id, VGA_SWITCHEROO_OFF);
 			if (client->pwr_state == VGA_SWITCHEROO_ON)
 				vga_switchoff(client);
@@ -412,6 +422,8 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
 		list_for_each_entry(client, &vgasr_priv.clients, list) {
 			if (client->active || client_is_audio(client))
 				continue;
+			if (client->driver_power_control)
+				continue;
 			if (client->pwr_state == VGA_SWITCHEROO_OFF)
 				vga_switchon(client);
 			set_audio_state(client->id, VGA_SWITCHEROO_ON);
@@ -568,3 +580,25 @@ err:
 }
 EXPORT_SYMBOL(vga_switcheroo_process_delayed_switch);
 
+/* force a PCI device to a certain state - mainly to turn off audio clients */
+void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic, bool main_power)
+{
+	struct vga_switcheroo_client *client;
+
+	client = find_client_from_pci(&vgasr_priv.clients, pdev);
+	if (!client)
+		return;
+
+	if (!client->driver_power_control)
+		return;
+
+	if (main_power) {
+		if (vgasr_priv.handler->power_state)
+			vgasr_priv.handler->power_state(client->id, dynamic);
+		return;
+	}
+
+	set_audio_state(client->id, dynamic);
+	client->pwr_state = dynamic;
+}
+EXPORT_SYMBOL(vga_switcheroo_set_dynamic_switch);
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index ddb419c..f327093 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -45,7 +45,8 @@ struct vga_switcheroo_client_ops {
 #if defined(CONFIG_VGA_SWITCHEROO)
 void vga_switcheroo_unregister_client(struct pci_dev *dev);
 int vga_switcheroo_register_client(struct pci_dev *dev,
-				   const struct vga_switcheroo_client_ops *ops);
+				   const struct vga_switcheroo_client_ops *ops,
+				   bool driver_power_control);
 int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 					 const struct vga_switcheroo_client_ops *ops,
 					 int id, bool active);
@@ -60,6 +61,8 @@ int vga_switcheroo_process_delayed_switch(void);
 
 int vga_switcheroo_get_client_state(struct pci_dev *dev);
 
+void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic, bool main_power);
+
 #else
 
 static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
@@ -74,6 +77,7 @@ static inline void vga_switcheroo_unregister_handler(void) {}
 static inline int vga_switcheroo_process_delayed_switch(void) { return 0; }
 static inline int vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
 
+static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic, bool main_power) {}
 
 #endif
 #endif /* _LINUX_VGA_SWITCHEROO_H_ */
-- 
1.7.12

[PATCH 2/5] drm: Add initial dnyamic power off feature

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

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 <airlied@redhat.com>
---
 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(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 9238de4..9fae62a 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -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)
+{
+	queue_delayed_work(system_nrt_wq, &dev->dynamic_power_poll, POWER_OFF_PERIOD);
+}
+
+static void drm_power_poll_execute(struct work_struct *work)
+{
+	struct delayed_work *delayed_work = to_delayed_work(work);
+	struct drm_device *dev = container_of(delayed_work, struct drm_device, dynamic_power_poll);
+	bool ret;
+
+	/* ask driver if okay to power off */
+	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);
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f546d1e..9a2c56b 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -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);
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 5062eec..285e53f 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -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);
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 21bcd4a..0e56a40 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -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;
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 45ac8d6..850c210 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -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;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index d6b67bb..65154b0 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -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
-- 
1.7.12

Re: [PATCH 2/5] drm: Add initial dnyamic power off feature

[Daniel Vetter]

On Mon, Sep 10, 2012 at 02:31:52PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> 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 <airlied@redhat.com>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9238de4..9fae62a 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -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)
> +{
> +	queue_delayed_work(system_nrt_wq, &dev->dynamic_power_poll, POWER_OFF_PERIOD);
> +}
> +
> +static void drm_power_poll_execute(struct work_struct *work)
> +{
> +	struct delayed_work *delayed_work = to_delayed_work(work);
> +	struct drm_device *dev = container_of(delayed_work, struct drm_device, dynamic_power_poll);
> +	bool ret;
> +
> +	/* ask driver if okay to power off */

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 ...

/me needs morning coffee to think this through

> +	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);
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index f546d1e..9a2c56b 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -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);
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 5062eec..285e53f 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -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);
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index 21bcd4a..0e56a40 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -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;
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 45ac8d6..850c210 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -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;
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index d6b67bb..65154b0 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -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
> -- 
> 1.7.12
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm: Add initial dnyamic power off feature

[Dave Airlie]

(oops' forgot reply to all)

>
> 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.

> - 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..

> 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 ...

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.

Dave.

Re: [PATCH 2/5] drm: Add initial dnyamic power off feature

[Chris Wilson]

On Mon, 10 Sep 2012 18:23:05 +1000, Dave Airlie <airlied@gmail.com> wrote:
> (oops' forgot reply to all)
> 
> >
> > 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.

This reminds me of some of the power island patches that we making the
rounds a few years ago - effectively shadowing register blocks to avoid
waking up the device for trivial queries. In many ways it was just a
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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/5] drm: Add initial dnyamic power off feature

[Alan Cox]

> 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.

Badly. I don't believe the code ever worked properly. It certainly was
full of races. I've reworked chunks of it in the GMA500 oaktrail driver
codebase but it's not enabled and I may well delete it rather than fix it.

Alan

Re: [PATCH 2/5] drm: Add initial dnyamic power off feature

[Daniel Vetter]

On Mon, Sep 10, 2012 at 10:23 AM, Dave Airlie <airlied@gmail.com> wrote:
> (oops' forgot reply to all)
>
>>
>> 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.

Yeah, I've noticed that there are tons of places that need it. Otoh stuff
added to core should be scrunitized much more imo - I don't care about
races only in the drivers ;-) And I think you need to annotate all driver
paths anyway to make this race-free:
1. userspace does ioctl/syfs open 
2. core enables gpu
3. massive delay out of far left field
4. core decides to disable the gpu again
..
5. ioctl/sysf file access continues: boom

Afaics the only way to avoid 5 goin boom is to properly instrument all
driver paths that touch the hw, hold some sort of reference while doing so
and tell the core that it can't turn off the gpu while the refcoun > 0
(but also tell it to pls try again shortly if the refcount hold is only
temporary and not indefinite like an enabled kms output ...). So to get
this really right I think you can't avoid adding all these checks to all
the hw paths, but then with the added fun of fighting a midlayer that gets
in the way (since you'd have to re-issue any delayed power off switch
request when the refcount drops to zero).

And imo for the "meh, I don't care about these unlikely races" proof of
concept code it shouldn't be hard to wrap drm_ioctl and the other few
top-level entry points in the driver.

Imo this is very similar to the nouveau gpu reset discussion a while back
where they wanted to add a check in core to similarly guarantee that no
one can touch the hw while they change it. I've shot that down for similar
reasons. Imo that kind of device access bracketing really belongs into the
driver. And i915.ko has it. Yep, it's a pain to get right and you need
error injection + decent testsuite to have a decent guarantee it doesn't
blow up again.

>> - 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..

See above, I think I can poke holes into the current approach, too ;-)

>> 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 ...
>
> 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.

thread A				thread B
drm_open dGPU
drm_open_helper
dev->switch_power_state != DRM_SWITCH_POWER_ON check suceeds

					vga_switcheroo
					->can_switch check succeeds
					disables the dGPU

dev->open_count++ in drm_open

BOOM, you think the dGPU is on and working, but it isn't ...

I think we could fix this by killing ->can_switch into the ->set_power
callback an allowing that to return -EBUSY if switching isn't possible
right now. The switcheroo client could then do appropriate locking. But
since the entire drm->open_count story is a neat disaster anyway (never
try to unload while having a sysfs/debugfs file open ...) I'm in no hurry
to fix this ;-)

Cheers, Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm: Add initial dnyamic power off feature

[Alan Cox]

On Mon, 10 Sep 2012 14:31:52 +1000
Dave Airlie <airlied@gmail.com> wrote:

> From: Dave Airlie <airlied@redhat.com>
> 
> 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 ?

Alan

Re: [PATCH 2/5] drm: Add initial dnyamic power off feature

[Dave Airlie]

On Mon, Sep 10, 2012 at 9:07 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Mon, 10 Sep 2012 14:31:52 +1000
> Dave Airlie <airlied@gmail.com> wrote:
>
>> From: Dave Airlie <airlied@redhat.com>
>>
>> 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 ?

Not really, wanted to have something simple to demo first. This

"bus type drivers of the buses the devices are on are
 responsible for the actual handling of the autosuspend requests and
 wake-up events."

seemed to imply there was some magic in the PCI bus layer that is
generic, and in this case that doesn't exist.

We have a 3 specific ACPI calls to power off the GPU slot for nvidia,
radeon and apple.

Dave.

[PATCH 3/5] nouveau: Add interface to detect optimus support

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This is required for later patches.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_acpi.c | 4 ++++
 drivers/gpu/drm/nouveau/nouveau_acpi.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index e7369c8..b92833e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -35,6 +35,10 @@ static struct nouveau_dsm_priv {
 	acpi_handle rom_handle;
 } nouveau_dsm_priv;
 
+bool nouveau_is_optimus(void) {
+	return nouveau_dsm_priv.optimus_detected;
+}
+
 #define NOUVEAU_DSM_HAS_MUX 0x1
 #define NOUVEAU_DSM_HAS_OPT 0x2
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.h b/drivers/gpu/drm/nouveau/nouveau_acpi.h
index 08af677..c70b4d1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.h
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.h
@@ -4,6 +4,7 @@
 #define ROM_BIOS_PAGE 4096
 
 #if defined(CONFIG_ACPI)
+bool nouveau_is_optimus(void);
 void nouveau_register_dsm_handler(void);
 void nouveau_unregister_dsm_handler(void);
 void nouveau_switcheroo_optimus_dsm(void);
@@ -11,6 +12,7 @@ int nouveau_acpi_get_bios_chunk(uint8_t *bios, int offset, int len);
 bool nouveau_acpi_rom_supported(struct pci_dev *pdev);
 void *nouveau_acpi_edid(struct drm_device *, struct drm_connector *);
 #else
+static inline bool nouveau_is_optimus(void) { return false; };
 static inline void nouveau_register_dsm_handler(void) {}
 static inline void nouveau_unregister_dsm_handler(void) {}
 static inline void nouveau_switcheroo_optimus_dsm(void) {}
-- 
1.7.12

Re: [PATCH 3/5] nouveau: Add interface to detect optimus support

[Lekensteyn]

Hi Dave,

> +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:

	return nouveau_dsm_priv.optimus_detected || 
          nouveau_dsm_priv.dsm_detected;

Regards,
Peter

Re: [PATCH 3/5] nouveau: Add interface to detect optimus support

[Dave Airlie]

On Tue, Sep 11, 2012 at 2:25 AM, Lekensteyn <lekensteyn@gmail.com> wrote:
> Hi Dave,
>
>> +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:
>

Works fine on my T410s from what I can see, though yes I understand
the older ones probably won't work

I'm still not 100% sure how best to deal with the older ones.

Dave.

[PATCH 4/5] nouveau: add dynamic gpu power off support.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This adds the interfaces for nouveau to dynamically power off
the GPU in an optimus system.

It's based on nouveau git so may not apply everywhere.

Signed-off-by: Dave Airlie <airlied@redhat.com
---
 drivers/gpu/drm/nouveau/nouveau_drm.c |  2 ++
 drivers/gpu/drm/nouveau/nouveau_drm.h |  3 ++
 drivers/gpu/drm/nouveau/nouveau_pm.c  | 57 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/nouveau/nouveau_vga.c |  4 ++-
 4 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 9fb56b3..e7735a8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -604,6 +604,8 @@ driver = {
 	.dumb_map_offset = nouveau_display_dumb_map_offset,
 	.dumb_destroy = nouveau_display_dumb_destroy,
 
+	.dynamic_off_check = nouveau_dynamic_power_check,
+	.dynamic_set_state = nouveau_dynamic_power_set_state,
 	.name = DRIVER_NAME,
 	.desc = DRIVER_DESC,
 #ifdef GIT_REVISION
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.h b/drivers/gpu/drm/nouveau/nouveau_drm.h
index ab0c174..f39f814 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.h
@@ -148,4 +148,7 @@ int nouveau_drm_resume(struct pci_dev *);
 		NV_PRINTK(KERN_DEBUG, "D", drm, fmt, ##args);                  \
 } while (0)
 
+bool nouveau_dynamic_power_check(struct drm_device *dev);
+int nouveau_dynamic_power_set_state(struct drm_device *dev, int state);
+
 #endif
diff --git a/drivers/gpu/drm/nouveau/nouveau_pm.c b/drivers/gpu/drm/nouveau/nouveau_pm.c
index 3c55ec2..fc132c3 100644
--- a/drivers/gpu/drm/nouveau/nouveau_pm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_pm.c
@@ -28,12 +28,12 @@
 #include <linux/power_supply.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
-
+#include <linux/vga_switcheroo.h>
 #include "drmP.h"
-
+#include "drm_crtc_helper.h"
 #include "nouveau_drm.h"
 #include "nouveau_pm.h"
-
+#include "nouveau_acpi.h"
 #include <subdev/bios/gpio.h>
 #include <subdev/gpio.h>
 #include <subdev/timer.h>
@@ -962,6 +962,7 @@ nouveau_pm_init(struct drm_device *dev)
 	register_acpi_notifier(&pm->acpi_nb);
 #endif
 
+	drm_dynamic_power_init(dev);
 	return 0;
 }
 
@@ -1007,3 +1008,53 @@ nouveau_pm_resume(struct drm_device *dev)
 	nouveau_pm_perflvl_set(dev, perflvl);
 	nouveau_pwmfan_set(dev, pm->fan.percent);
 }
+
+/* rules for what we want
+   if we are an optimus laptop,
+   with no active crtc/encoders/connectors
+   with no channel activity for 4-5s
+*/
+
+bool nouveau_dynamic_power_check(struct drm_device *dev)
+{
+	struct nouveau_drm *drm = nouveau_drm(dev);
+	struct drm_crtc *crtc;
+
+	/* are we optimus enabled? */
+	if (!nouveau_is_optimus()) {
+		DRM_DEBUG_DRIVER("failing to power off - not optimus\n");
+		return false;
+	}
+
+	list_for_each_entry(crtc, &drm->dev->mode_config.crtc_list, head) {
+		if (crtc->enabled) {
+			DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
+			return false;
+		}
+	}
+	return true;
+}
+
+int nouveau_dynamic_power_set_state(struct drm_device *dev, int state)
+{
+	struct nouveau_drm *drm = nouveau_drm(dev);
+	pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
+
+	if (state == DRM_SWITCH_POWER_DYNAMIC_OFF) {
+		dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
+		drm_kms_helper_poll_disable(drm->dev);
+		vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_OFF, false);
+		nouveau_switcheroo_optimus_dsm();
+		nouveau_drm_suspend(drm->dev->pdev, pmm);
+		vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_OFF, true);
+	} else if (state == DRM_SWITCH_POWER_ON) {
+		vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_ON, true);
+		nouveau_drm_resume(dev->pdev);
+		vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_ON, false);
+		drm_kms_helper_poll_enable(dev);
+		dev->switch_power_state = DRM_SWITCH_POWER_ON;
+	}
+
+	return 0;
+}
+
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index 37fcc9d..539722f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -33,6 +33,8 @@ nouveau_switcheroo_set_state(struct pci_dev *pdev,
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
 
+	if (nouveau_is_optimus() && state == VGA_SWITCHEROO_OFF)
+		return;
 	if (state == VGA_SWITCHEROO_ON) {
 		printk(KERN_ERR "VGA switcheroo: switched nouveau on\n");
 		dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
@@ -80,7 +82,7 @@ nouveau_vga_init(struct nouveau_drm *drm)
 {
 	struct drm_device *dev = drm->dev;
 	vga_client_register(dev->pdev, dev, NULL, nouveau_vga_set_decode);
-	vga_switcheroo_register_client(dev->pdev, &nouveau_switcheroo_ops, false);
+	vga_switcheroo_register_client(dev->pdev, &nouveau_switcheroo_ops, nouveau_is_optimus());
 }
 
 void
-- 
1.7.12

Re: [PATCH 4/5] nouveau: add dynamic gpu power off support.

[Peter Wu]

Hi Dave,

> +int nouveau_dynamic_power_set_state(struct drm_device *dev, int state)
> +{
> +	struct nouveau_drm *drm = nouveau_drm(dev);
> +	pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
> +
> +	if (state == DRM_SWITCH_POWER_DYNAMIC_OFF) {
> +		dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
In existing set_state code, this switch_power_state is first set to 
DRM_SWITCH_POWER_CHANGING. Is it sensible to do the same thing here?

> +		drm_kms_helper_poll_disable(drm->dev);
> +		vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_OFF, 
false);
> +		nouveau_switcheroo_optimus_dsm();
> +		nouveau_drm_suspend(drm->dev->pdev, pmm);
> +		vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_OFF, 
true);
> +	} else if (state == DRM_SWITCH_POWER_ON) {
> +		vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_ON, 
true);
> +		nouveau_drm_resume(dev->pdev);
> +		vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_ON, 
false);
> +		drm_kms_helper_poll_enable(dev);
> +		dev->switch_power_state = DRM_SWITCH_POWER_ON;
Same here.

> +	}
> +
> +	return 0;
> +}

Regards,
Peter

[PATCH 5/5] radeon: add dynamic power off support

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This adds the start of dynamic power off support to radeon,
it probably turns off way to many GPUs but is an initial implementation
I've tested on an gm45/rv635 combination laptop.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/radeon/radeon.h        |  3 +++
 drivers/gpu/drm/radeon/radeon_device.c |  2 +-
 drivers/gpu/drm/radeon/radeon_drv.c    |  6 ++++++
 drivers/gpu/drm/radeon/radeon_pm.c     | 37 +++++++++++++++++++++++++++++++++-
 4 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 59a1531..0abad51 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -1839,6 +1839,9 @@ extern int radeon_acpi_init(struct radeon_device *rdev);
 static inline int radeon_acpi_init(struct radeon_device *rdev) { return 0; } 
 #endif 
 
+extern bool radeon_dynamic_power_check(struct drm_device *dev);
+extern int radeon_dynamic_power_set_state(struct drm_device *dev, int state);
+
 #include "radeon_object.h"
 
 #endif
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 857cf5b..c58849c 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1093,7 +1093,7 @@ int radeon_device_init(struct radeon_device *rdev,
 	/* this will fail for cards that aren't VGA class devices, just
 	 * ignore it */
 	vga_client_register(rdev->pdev, rdev, NULL, radeon_vga_set_decode);
-	vga_switcheroo_register_client(rdev->pdev, &radeon_switcheroo_ops, false);
+	vga_switcheroo_register_client(rdev->pdev, &radeon_switcheroo_ops, true);
 
 	r = radeon_init(rdev);
 	if (r)
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 8c593ea..b0a7211 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -118,6 +118,9 @@ struct dma_buf *radeon_gem_prime_export(struct drm_device *dev,
 struct drm_gem_object *radeon_gem_prime_import(struct drm_device *dev,
 					       struct dma_buf *dma_buf);
 
+bool radeon_dynamic_power_check(struct drm_device *dev);
+int radeon_dynamic_power_set_state(struct drm_device *dev, int state);
+
 #if defined(CONFIG_DEBUG_FS)
 int radeon_debugfs_init(struct drm_minor *minor);
 void radeon_debugfs_cleanup(struct drm_minor *minor);
@@ -385,6 +388,9 @@ static struct drm_driver kms_driver = {
 	.gem_prime_export = radeon_gem_prime_export,
 	.gem_prime_import = radeon_gem_prime_import,
 
+	.dynamic_off_check = radeon_dynamic_power_check,
+	.dynamic_set_state = radeon_dynamic_power_set_state,
+
 	.name = DRIVER_NAME,
 	.desc = DRIVER_DESC,
 	.date = DRIVER_DATE,
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
index 7ae6066..2763ff1 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -30,7 +30,7 @@
 #include <linux/power_supply.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
-
+#include <linux/vga_switcheroo.h>
 #define RADEON_IDLE_LOOP_MS 100
 #define RADEON_RECLOCK_DELAY_MS 200
 #define RADEON_WAIT_VBLANK_TIMEOUT 200
@@ -643,6 +643,8 @@ int radeon_pm_init(struct radeon_device *rdev)
 		DRM_INFO("radeon: power management initialized\n");
 	}
 
+	drm_dynamic_power_init(rdev->ddev);
+
 	return 0;
 }
 
@@ -877,3 +879,36 @@ static int radeon_debugfs_pm_init(struct radeon_device *rdev)
 	return 0;
 #endif
 }
+
+bool radeon_dynamic_power_check(struct drm_device *dev)
+{
+	struct drm_crtc *crtc;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		if (crtc->enabled) {
+			DRM_DEBUG_DRIVER("failing to power off - crtc active\n");
+			return false;
+		}
+	}
+	return true;
+}
+
+int radeon_dynamic_power_set_state(struct drm_device *dev, int state)
+{
+       	pm_message_t pmm = { .event = PM_EVENT_SUSPEND };
+
+	if (state == DRM_SWITCH_POWER_DYNAMIC_OFF) {
+		dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
+		vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_OFF, false);
+		drm_kms_helper_poll_disable(dev);
+		radeon_suspend_kms(dev, pmm);
+		vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_OFF, true);
+	} else {
+		vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_ON, true);
+		radeon_resume_kms(dev);
+		vga_switcheroo_set_dynamic_switch(dev->pdev, VGA_SWITCHEROO_ON, false);
+		drm_kms_helper_poll_enable(dev);
+		dev->switch_power_state = DRM_SWITCH_POWER_ON;
+	}
+	return 0;
+}
-- 
1.7.12

Re: [RFC] drm dynamic power off support

[Dave Airlie]

On Mon, Sep 10, 2012 at 2:31 PM, Dave Airlie <airlied@gmail.com> wrote:
> 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.
>
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.

Dave.

Re: [RFC] drm dynamic power off support

[Takashi Iwai]

At Mon, 10 Sep 2012 15:04:02 +1000,
Dave Airlie wrote:
> 
> On Mon, Sep 10, 2012 at 2:31 PM, Dave Airlie <airlied@gmail.com> wrote:
> > 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.
> >
> 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

These come from PCI core, and...

> [  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.

> 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.


thanks,

Takashi

Re: [RFC] drm dynamic power off support

[Dave Airlie]

On Mon, Sep 10, 2012 at 6:47 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Mon, 10 Sep 2012 15:04:02 +1000,
> Dave Airlie wrote:
>>
>> On Mon, Sep 10, 2012 at 2:31 PM, Dave Airlie <airlied@gmail.com> wrote:
>> > 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.
>> >
>> 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
>
> These come from PCI core, and...
>
>> [  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.
>
>> 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.

> 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.

Dave.

Re: [RFC] drm dynamic power off support

[Takashi Iwai]

At Mon, 10 Sep 2012 18:50:04 +1000,
Dave Airlie wrote:
> 
> On Mon, Sep 10, 2012 at 6:47 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Mon, 10 Sep 2012 15:04:02 +1000,
> > Dave Airlie wrote:
> >>
> >> On Mon, Sep 10, 2012 at 2:31 PM, Dave Airlie <airlied@gmail.com> wrote:
> >> > 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.
> >> >
> >> 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
> >
> > These come from PCI core, and...
> >
> >> [  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.
> >
> >> 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.
> 
> > 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.

Does your tree contain the recent patches in sound git tree for-next
branch, or it's based on 3.6-rc?

The former contains some patches to make the controller going to D3,
so this might have some side effect.


Takashi