Hey,
Op 19-08-13 20:12, Peter Hurley schreef:
On 08/12/2013 07:50 AM, Maarten Lankhorst wrote:
This fixes a deadlock inversion when vblank is enabled/disabled by drm. &dev->vblank_time_lock is always taken when the vblank state is toggled, which caused a deadlock when &event->lock was also taken during event_get/put.
Solve the race by requiring that lock to change enable/disable state, and always keeping vblank on the event list. Core drm ignores unwanted vblanks, so extra calls to drm_handle_vblank are harmless.
I don't feel this is the appropriate solution to the lock inversion between vblank_time_lock and event->lock.
Preferably drm core should correct the interface layer bug; ie., calling into a sub-driver holding a lock _and_ requiring the sub-driver to call a drm helper function which claims the same lock is bad design. The console lock suffers from the same design flaw and is a constant problem.
Alternatively, the event trigger could be lockless; ie., the event list could be an RCU list instead. In this way, the event->lock does not need to be claimed, and thus no lock inversion is possible. The main drawback here is that currently the event->lock enforces non-overlapping lifetimes between the event handler and the event. Untangling object lifetimes in nouveau is a non-trivial exercise.
If only it was so easy..
nouveau is doing a non-standard thing with core/engine/software, specifically the release method. The real fix is reverting the commit that changes this to a nouveau_event type of thing, and reinstates the old locking. This is not going to happen though, so the second best fix is just telling it to lock the other lock too when changing enable state.
~Maarten