On Mon, Sep 10, 2012 at 02:31:52PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at 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 at 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 at 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
