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

[Daniel Vetter <daniel@ffwll.ch>]

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