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